Follow no redirect on a Go token request

Merged
JE
4d ago
+147-3

A token request's form carries secrets: the authorization code with its PKCE verifier, or the refresh token, plus the client secret. Go's net/http keeps the body when it follows a 307 or 308. So oauth.Exchanger (Exchange, Refresh) and AuthManager.Refresh sent those secrets to whatever host a redirect named, and took that host's answer as their tokens. AuthManager then stored those tokens.

What changes (Go)

A token endpoint never redirects. Each token request now goes through a copy of the client whose CheckRedirect answers http.ErrUseLastResponse, so a 3xx is the non-200 error it is and the form goes nowhere else.

  • oauth.NewExchanger copies the client it's given (or http.DefaultClient's settings).
  • AuthManager.refreshLocked does the same per refresh. Given no client, it now takes http.DefaultClient's settings instead of panicking, as NewExchanger already does.
  • The caller's own client is never changed.

TypeScript already sends its token requests with redirect: "manual". The Rust and Kotlin token paths are not part of this change.

Red first

The first commit adds the tests on main's code:

  • TestTokenRequestsFollowNoRedirect: Exchange and Refresh, with the default and a supplied client, each succeeded with the other server's access_token.
  • TestAuthManager_RefreshFollowsNoRedirect: the refresh succeeded through the 307.
  • The two "caller's client keeps its redirect policy" checks pass before the fix; they guard it.

Verified

go test ./pkg/hey/ ./pkg/hey/oauth/ and golangci-lint are green. Nothing outside go/ changes. The repo-wide make check failures that main already has (cargo deny's webpki-root-certs advisory; locally, Swift 6.3 warnings-as-errors) are untouched by this. Merges cleanly with #268, #269 and #270.

Summary by cubic

Stops Go token requests from following redirects, so the authorization code with its PKCE verifier, the refresh token, and the client secret are no longer sent to an arbitrary Location host whose answer would have been treated as the new tokens and, for AuthManager, stored.

  • oauth.Exchanger.Exchange, oauth.Exchanger.Refresh, and AuthManager.Refresh now send each request through a copy of the client whose CheckRedirect returns http.ErrUseLastResponse, making a 3xx the non-200 error it is.
  • The caller's client is never modified; AuthManager with no client now uses http.DefaultClient's settings instead of panicking.