fix: Prevent agent HTTP auth redirect leaks#3978
Conversation
|
@buildsworth-bk review please @lox sorry didn't mean for Codex to review as well |
There was a problem hiding this comment.
Pull request overview
This PR hardens internal/agenthttp authenticated HTTP clients against credential leakage across redirects by refusing to follow redirects that change scheme or host, while keeping default redirect behavior for unauthenticated clients.
Changes:
- Add a
CheckRedirectpolicy for authenticated clients to stop on cross-origin (scheme/host) redirects. - Add tests covering authenticated cross-origin redirect blocking, authenticated same-origin redirect following, and unauthenticated cross-origin redirect following.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/agenthttp/client.go | Adds an authenticated-only redirect policy to prevent credential leaks on cross-origin redirects. |
| internal/agenthttp/client_test.go | Adds regression tests for redirect behavior with and without authentication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func checkAuthenticatedRedirect(req *http.Request, via []*http.Request) error { | ||
| if len(via) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| origin := via[0].URL | ||
| if req.URL.Scheme != origin.Scheme || req.URL.Host != origin.Host { | ||
| return http.ErrUseLastResponse | ||
| } | ||
|
|
||
| return nil | ||
| } |
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
This wires a CheckRedirect onto the authenticated clients so a redirect that changes scheme or host returns http.ErrUseLastResponse instead of being followed with the agent token/bearer attached — closing the leak that came from injecting credentials in the transport layer, below Go's redirect sanitizer. The approach is sound: only the authenticated clients (api.Client, bkgql) get the policy, so the unauthenticated artifact downloaders still follow their S3/GCS redirects, and the tests cover the cross-origin, same-origin, and unauthenticated cases well.
Two things to keep in mind:
- One inline note about Go's default 10-redirect cap being dropped along with the default policy.
- For authenticated clients a cross-origin redirect now surfaces to the caller as a 3xx (e.g.
checkResponsetreats it as an error) rather than being followed. That looks like the intended trade-off, and the description frames the scenario as hypothetical — just flagging it for the reviewer.
There's no risk: label on the PR; since this changes credential-handling and redirect behaviour, it's worth a human pass.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 597, then answer my questions about the findings.
| } | ||
| } | ||
|
|
||
| func checkAuthenticatedRedirect(req *http.Request, via []*http.Request) error { |
There was a problem hiding this comment.
Non-blocking: Setting a custom CheckRedirect replaces Go's default redirect policy entirely, including its built-in cap of 10 redirects — defaultCheckRedirect only runs when CheckRedirect is nil. Because this returns nil for every same-origin hop, an authenticated client following same-origin redirects now has no limit; the only bound is the client Timeout (60s for both the API and GraphQL clients), so a same-origin redirect loop would spin until the timeout rather than stopping after 10.
Worth re-adding the cap to preserve the previous behaviour:
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}(needs an fmt/errors import). A small test for the loop case would lock it in.
|
I had a thought about this, since I had a quick look yesterday, but didn't respond. Here it is.
On the surface this seems very unlikely. Redirecting HTTPS to HTTP seems remarkably unlikely. A different host is more plausible. To be safe, the backend would have to limit any such redirects to agent versions after this change, or until after all old agents are no longer used, which would limit the utility of such a redirect. I don't think this is an argument against this change. |
Authenticated agent HTTP clients currently inject credentials in the transport layer, after Go's redirect sanitizer has already decided whether to strip
Authorization. If a trusted Buildkite endpoint ever redirected to a different host or scheme, the redirected request could receive the agent token or bearer credential.This keeps the normal redirect behavior for unauthenticated clients, but authenticated clients now stop at redirects that change scheme or host and return the redirect response instead. Same-origin redirects still follow normally and continue to include the expected authorization header.
For example, an authenticated request from
https://agent.buildkite.com/...to aLocation: https://example.invalid/...response will no longer follow that redirect with credentials attached, while/oldto/newon the same origin continues to work.