Repository navigation
Conversation
Token responses carry access and refresh tokens, and RFC 6749 section 5.1 and OAuth 2.1 section 3.2.3 require them not to be cached. /oauth/token sent no Cache-Control header. Set it before any response is written, so error responses carry it too.
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
Small, well-scoped fix, and setting the header before params parsing does cover every error the handler itself returns (they're written later to the same ResponseWriter). Two scope notes below. Neither blocks this PR; they're more about whether "errors carry it too" holds end to end.
|
|
||
| // Token responses carry credentials and must not be cached (RFC 6749 section 5.1, | ||
| // OAuth 2.1 section 3.2.3). Set before any response is written, so errors carry it too. | ||
| w.Header().Set("Cache-Control", "no-store") |
There was a problem hiding this comment.
Coverage gap for error responses: in api.go the route is r.With(api.requireOAuthClientAuth).Post("/token", api.oauthServer.OAuthToken), so any client-authentication failure (bad or missing client_secret → invalid_client) comes back from the middleware before OAuthToken runs, and won't have Cache-Control: no-store. Those bodies don't contain tokens, so the risk is low. But if the goal is "every token-endpoint response", setting the header in a tiny middleware ahead of requireOAuthClientAuth on this route (or in requireOAuthClientAuth itself) would cover it. The new test calls OAuthToken directly with the client already in context, so it wouldn't catch this.
Related follow-ups, possibly out of scope for this split-out PR: the first-party POST /token in internal/api/token.go (password / refresh_token / pkce / id_token grants) also returns access and refresh tokens, and I couldn't find a Cache-Control header set anywhere on that path. OAuthServerClientDynamicRegister / AdminOAuthServerClientRegister / regenerate_secret return a plaintext client_secret too. They'd likely benefit from the same treatment.
There was a problem hiding this comment.
Thanks, both check out. requireOAuthClientAuth returns invalid_client before OAuthToken runs, so those responses missed the header. Pushed 12e42b0: the header is now set by a small noStore middleware that runs ahead of client authentication on this route. The test goes through the router and covers a wrong client secret, an unknown client, and an unknown code. All three fail without the change.
The first-party POST /token and the endpoints that return a client_secret are a separate surface, so I'll open a follow-up PR for those rather than grow this one.
…cation requireOAuthClientAuth returns invalid_client before OAuthToken runs, so those responses did not carry Cache-Control: no-store. Set the header in a middleware that runs first on the token route, and test it through the router: a wrong client secret, an unknown client, and an unknown code all carry the header.
What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
POST /oauth/tokenreturns access and refresh tokens with noCache-Controlheader. RFC 6749 §5.1 and OAuth 2.1 §3.2.3 require token responses to carryCache-Control: no-store, so shared caches and proxies don't keep credentials.What is the new behavior?
A small
noStoremiddleware setsCache-Control: no-storeon the token route ahead ofrequireOAuthClientAuth. Every response from the endpoint carries it, including client-authentication errors returned beforeOAuthTokenruns.Pragma: no-cacheis left out, following OAuth 2.1 (RFC 9111 deprecatesPragma).The test goes through the router and checks the header on a wrong client secret, an unknown client, and an unknown code. It fails without the change.
Split out of #2914 so it can land on its own. The first-party
POST /tokenwill follow in a separate PR.