Skip to content

fix: preserve original GTFS source in reports - #2178

Open
mackenziereading19 wants to merge 3 commits into
MobilityData:masterfrom
mackenziereading19:feat/1539-original-source-display
Open

mackenziereading19 wants to merge 3 commits into
MobilityData:masterfrom
mackenziereading19:feat/1539-original-source-display

Conversation

@mackenziereading19

Copy link
Copy Markdown
Contributor

Summary

Fixes #1539 by preserving the human-readable GTFS source separately from the operational source used by the validator.

For web uploads, reports now show the original uploaded filename instead of the temporary server-side filename. For URL submissions, reports show the original URL, as requested in the issue discussion.

Implementation

  • Add optional originalGtfsSource provenance to ValidationRunnerConfig while retaining gtfsSource for operational file/URL access.
  • Add a single display-source fallback (originalGtfsSource when present, otherwise gtfsSource) used by JSON and HTML reports.
  • Forward uploaded filenames from the web client through CreateJobRequest and persisted JobMetadata.
  • Persist URL provenance even when no country code is supplied.
  • Forward persisted provenance through ValidationController and ValidationHandler without changing the operational temporary file URI.
  • Preserve compatibility with existing callers and previously persisted JobMetadata that does not contain originalGtfsSource.

Validation

  • Full :main:test passed.
  • Full :web:service:test passed.
  • :main:spotlessCheck passed.
  • :web:service:spotlessCheck passed.
  • npm run check passed with 0 errors and 0 warnings (3 pre-existing hints).
  • Full Cypress suite passed: 7/7 tests.
  • Regression coverage verifies uploaded filename propagation to /create-job.
  • Regression coverage verifies URL and filename provenance persistence.
  • Regression coverage verifies operational gtfsSource remains separate from originalGtfsSource.
  • Regression coverage verifies JSON and HTML report source selection.
  • Backward-compatibility test verifies old two-field JobMetadata JSON still deserializes with originalGtfsSource == null.
  • git diff --check passed.

Closes #1539

@mackenziereading19

Copy link
Copy Markdown
Contributor Author

Quick CI note: the current Web service CI failure occurs at google-github-actions/auth@v2, before the Gradle tasks run. The other workflows are passing, and the full :web:service:test suite passed locally. I don’t currently see a code-related failure in this PR, but I’m happy to rerun or make changes if maintainers spot anything implementation-specific.


/** @param {string=} url **/
function createJob(url) {
/** @param {string=} url @param {string=} filename **/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: The empty line should not be removed.

if (!Strings.isNullOrEmpty(body.getCountryCode())) {
storageHelper.saveJobMetadata(new JobMetadata(jobId, body.getCountryCode()));
String originalGtfsSource =
!Strings.isNullOrEmpty(body.getUrl()) ? body.getUrl() : body.getFilename();

@jcpitre jcpitre Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sanitize the URL that we're going to put in the report to remove any possible user and password.

@mackenziereading19

Copy link
Copy Markdown
Contributor Author

Thanks @jcpitre, I've addressed both comments in 65c2032.

  • Restored the blank line in +page.svelte.
  • Sanitized URL user-info credentials before saving source provenance to report metadata, while retaining the original URL for downloading.
  • Added a regression test covering both the sanitized stored URL and unchanged download URL.

All 26 web-service tests and Spotless pass locally with Java 17. Fresh CI is running now.

@jcpitre

jcpitre commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for the change, but there is still a problem that I should have pointed out initially.
Some feeds require the authentication to be in the query string. e.g. https://api.wmata.com/gtfs/bus-gtfs-static.zip?api_key=...
The problem is that the api_key_parameter_name can vary (in the example it's api_key, but it could be key or anything really.) So it's hard at this point to identify the part of the query string we should drop.
I suggest that we just drop the entire query string (?...).
In some case the resulting URL might make little sense (for example if the server uses a query string parameter to identify the exact dataset amongst different datasets it serves), but I think it's a price to pay.

Another point is that the sanitizing should not throw an exception that would result in the whole download operation failing. If there's any problem with sanitizing we should just revert to the URL as it was displayed before (even if we agree it's not that useful)

@mackenziereading19

Copy link
Copy Markdown
Contributor Author

Thanks @jcpitre, I’ve addressed the follow-up in e3b7712.

  • Stored report provenance now drops the entire query string as well as URL user-info credentials.
  • The original URL is still passed unchanged to the downloader.
  • Sanitization now falls back to the original source if parsing cannot be completed safely, so it cannot block the download flow.
  • Added regression coverage for query-only auth, combined user-info + query sanitization, and malformed-URL fallback.
    The full :web:service:test suite and Spotless pass locally with Java 17. Fresh CI is running now.

@jcpitre

jcpitre commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for the change.
But there is still one improvement needed.
It should not return the unsanitized URL, even if it's erroneous (return sourceUrl; in 2 locations). There is still a risk of revealing a secret. Examples of URLs that would throw an exception are:

  • https://host/feed{1}.zip?api_key=SECRET (braces in the path)
  • https://host/feed.zip?name=a|b&api_key=SECRET (pipe | in one of the parameters)

In both these cases if we return the unsanitized url the secret would be revealed.

It should return null. The caller can handle a null originalGtfsSource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filename in web validator is not informative

4 participants