Send per-feed auth headers when fetching GTFS-RT feeds#6
Merged
Conversation
Protected GTFS-RT feeds (e.g. Swiftly) require an Authorization header, but the validator fetched the vehicle-positions, trip-updates, and service-alerts feeds with no auth and got HTTP 401 — failing every RT check even though the feeds were reachable. Add a per-data-source RealtimeHeaders map to the config, thread it through FetchRealtime, and apply it to the three RT feeds. The static GTFS bundle is fetched without these headers so a feed's credential never leaks to the bundle host.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds support for per-source HTTP headers on GTFS-RT realtime feed requests. The ChangesRealtime Headers Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@feeds/fetch_test.go`:
- Line 35: The test HTTP handler in fetch_test.go calls w.Write([]byte("rt"))
without checking its error; update the handler that contains the w.Write call to
capture the returned (int, error) and handle the error (e.g. if err != nil {
t.Fatalf("write failed: %v", err) } or use require.NoError(t, err)) so the test
fails loudly on write errors and satisfies errcheck; reference the w.Write call
in the test handler in fetch_test.go.
In `@validator/prepare_test.go`:
- Around line 51-53: The test currently only checks auth["/static"] == "" which
can pass if the "/static" handler was never invoked; first assert that the
"/static" entry was actually recorded (e.g., check for presence with _, ok :=
auth["/static"] or a request counter/hits map) and fail the test immediately if
it was not hit (use t.Fatalf or t.Fatalf-like assertion), then proceed to assert
the header value is empty; update the checks around the auth map access in
prepare_test.go so you explicitly verify the request occurred before asserting
auth["/static"] == "".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b52270fe-117d-4549-a29f-11fda3a512d0
📒 Files selected for processing (6)
config/config.goconfig/config_test.gofeeds/fetch.gofeeds/fetch_test.govalidator/prepare_test.govalidator/validator.go
…tion - TestFetchRealtimeSendsHeaders: handle the httptest handler's w.Write error so the test fails loudly instead of silently dropping a write. - TestPrepareSendsRealtimeAuthHeaders: assert /static was actually requested before checking its Authorization header was empty, so the credential-leak check can't pass vacuously when the feed is never hit.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Authorizationheader, but the validator fetched the vehicle-positions, trip-updates, and service-alerts feeds with no auth and got HTTP 401 — failing every RT check (rt-freshness,*-sampling,service-alert-crossref) even though the feeds were reachable.RealtimeHeaders map[string]stringto the config, threads it throughFetchRealtime(ctx, url, hdr)→get(), and applies it to the three RT feeds.realtimeHeadersin the validator config it generates (OneBusAway/obacloud — see that PR).Test plan
go test ./...— all packages greengo vet ./...— cleanTestFetchRealtimeSendsHeaders(fetch),TestLoadParsesRealtimeHeaders(config),TestPrepareSendsRealtimeAuthHeaders(wiring — asserts the header reaches the RT feed and is absent on the static feed)status 401(reproduces the failure); with-header →200, 9465 bytes of protobufReview notes (non-blocking)
util.go/document.go) scrubsapiKey/db_passbut not the new feed header value. The common 401 path does not leak (errors contain only URL + status; the Swiftly key is a header, not a query param). Defense-in-depth would add the feed header values to the redaction set — left as a follow-up to keep this change focused.Summary by CodeRabbit
New Features
Tests