From 10983354dbbebaf64769ca048f56ea34a058295f Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 9 Sep 2025 12:47:10 -0700 Subject: [PATCH 01/15] feat(oauth): client_secret_basic, RFC errors, cache headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Prefer Basic auth over body; conflict → invalid_request (400) - Map missing/bad creds to invalid_client (401) + WWW-Authenticate - Add Cache-Control: no-store, Pragma: no-cache on success/errors - Standardize token_type to “Bearer” (token + authorize fragment) - Enforce redirect_uri binding in token exchange - Treat missing grant_type as invalid_request (400) - Remove redirect_uri from error logs - Accept case-insensitive “Basic”/“Bearer” schemes (input) --- src/sentry/web/frontend/oauth_authorize.py | 2 +- src/sentry/web/frontend/oauth_token.py | 86 ++++++++++++---- tests/sentry/api/test_authentication.py | 14 +++ .../web/frontend/test_oauth_authorize.py | 4 +- tests/sentry/web/frontend/test_oauth_token.py | 99 +++++++++++++------ 5 files changed, 153 insertions(+), 52 deletions(-) diff --git a/src/sentry/web/frontend/oauth_authorize.py b/src/sentry/web/frontend/oauth_authorize.py index 0565e415c9f8f6..5367d1386cabec 100644 --- a/src/sentry/web/frontend/oauth_authorize.py +++ b/src/sentry/web/frontend/oauth_authorize.py @@ -441,7 +441,7 @@ def approve( "access_token": token.token, "expires_in": int((timezone.now() - token.expires_at).total_seconds()), "expires_at": token.expires_at.strftime("%Y-%m-%dT%H:%M:%S.%fZ"), - "token_type": "bearer", + "token_type": "Bearer", "scope": " ".join(token.get_scopes()), "state": state, }, diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index 8f61ed10ae9d88..8f3c6b5f753c86 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -36,7 +36,7 @@ class _TokenInformation(TypedDict): refresh_token: str | None expires_in: int | None expires_at: datetime | None - token_type: Literal["bearer"] + token_type: Literal["Bearer"] scope: str user: _TokenInformationUser id_token: NotRequired[OpenIDToken] @@ -53,7 +53,6 @@ def dispatch(self, request, *args, **kwargs): # Note: the reason parameter is for internal use only def error(self, request: HttpRequest, name, reason=None, status=400): client_id = request.POST.get("client_id") - redirect_uri = request.POST.get("redirect_uri") logging.error( "oauth.token-error", @@ -61,19 +60,66 @@ def error(self, request: HttpRequest, name, reason=None, status=400): "error_name": name, "status": status, "client_id": client_id, - "redirect_uri": redirect_uri, "reason": reason, }, ) - return HttpResponse( + resp = HttpResponse( json.dumps({"error": name}), content_type="application/json", status=status ) + # RFC 6749 §5.2, §5.1 cache prevention + resp["Cache-Control"] = "no-store" + resp["Pragma"] = "no-cache" + # RFC 6749 §5.2 invalid_client requires WWW-Authenticate header + if name == "invalid_client": + resp["WWW-Authenticate"] = 'Basic realm="oauth"' + return resp @method_decorator(never_cache) def post(self, request: Request) -> HttpResponse: grant_type = request.POST.get("grant_type") - client_id = request.POST.get("client_id") - client_secret = request.POST.get("client_secret") + + # Extract client credentials per OAuth 2.1 (prefer Basic header over body) + auth_header = request.META.get("HTTP_AUTHORIZATION", "") or request.headers.get( + "Authorization", "" + ) + basic_client_id = basic_client_secret = None + if isinstance(auth_header, str) and auth_header: + import base64 + + scheme, _, param = auth_header.partition(" ") + if scheme and scheme.lower() == "basic" and param: + b64 = param.strip() + try: + decoded = base64.b64decode(b64).decode("utf-8") + # format: client_id:client_secret (client_secret may be empty) + if ":" not in decoded: + raise ValueError("missing colon in basic credentials") + basic_client_id, basic_client_secret = decoded.split(":", 1) + except Exception: + logger.warning("Invalid Basic auth header", extra={"client_id": None}) + return self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) + + body_client_id = request.POST.get("client_id") + body_client_secret = request.POST.get("client_secret") + + # If both Basic header and body credentials are present -> invalid_request + if basic_client_id is not None and (body_client_id or body_client_secret): + logger.info( + "oauth.basic-and-body-credentials", + extra={"client_id": basic_client_id or body_client_id, "reason": "conflict"}, + ) + return self.error(request=request, name="invalid_request", reason="credential conflict") + + # Choose credentials: header preferred, then body + client_id = basic_client_id if basic_client_id is not None else body_client_id + client_secret = ( + basic_client_secret if basic_client_secret is not None else body_client_secret + ) metrics.incr( "oauth_token.post.start", @@ -84,13 +130,16 @@ def post(self, request: Request) -> HttpResponse: }, ) - if not client_id: - return self.error(request=request, name="missing_client_id", reason="missing client_id") - if not client_secret: + if not client_id or not client_secret: return self.error( - request=request, name="missing_client_secret", reason="missing client_secret" + request=request, + name="invalid_client", + reason="missing client credentials", + status=401, ) + if not grant_type: + return self.error(request=request, name="invalid_request", reason="missing grant_type") if grant_type not in [GrantTypes.AUTHORIZATION, GrantTypes.REFRESH]: return self.error(request=request, name="unsupported_grant_type") @@ -106,7 +155,7 @@ def post(self, request: Request) -> HttpResponse: logger.warning("Invalid client_id / secret pair", extra={"client_id": client_id}) return self.error( request=request, - name="invalid_credentials", + name="invalid_client", reason="invalid client_id or client_secret", status=401, ) @@ -138,10 +187,9 @@ def get_access_tokens(self, request: Request, application: ApiApplication) -> di if grant.is_expired(): return {"error": "invalid_grant", "reason": "grant expired"} + # Enforce redirect_uri binding (RFC 6749 §4.1.3) redirect_uri = request.POST.get("redirect_uri") - if not redirect_uri: - redirect_uri = application.get_default_redirect_uri() - elif grant.redirect_uri != redirect_uri: + if grant.redirect_uri and grant.redirect_uri != redirect_uri: return {"error": "invalid_grant", "reason": "invalid redirect URI"} try: @@ -152,7 +200,7 @@ def get_access_tokens(self, request: Request, application: ApiApplication) -> di if grant.has_scope("openid") and options.get("codecov.signing_secret"): open_id_token = OpenIDToken( - request.POST.get("client_id"), + application.client_id, grant.user_id, options.get("codecov.signing_secret"), nonce=request.POST.get("nonce"), @@ -194,7 +242,7 @@ def process_token_details( else None ), "expires_at": token.expires_at, - "token_type": "bearer", + "token_type": "Bearer", "scope": " ".join(token.get_scopes()), "user": { "id": str(token.user.id), @@ -207,7 +255,11 @@ def process_token_details( token_information["id_token"] = id_token if token.scoping_organization_id: token_information["organization_id"] = str(token.scoping_organization_id) - return HttpResponse( + resp = HttpResponse( json.dumps(token_information), content_type="application/json", ) + # RFC 6749 §5.1 cache prevention + resp["Cache-Control"] = "no-store" + resp["Pragma"] = "no-cache" + return resp diff --git a/tests/sentry/api/test_authentication.py b/tests/sentry/api/test_authentication.py index d2c1b19644ce97..72ea8513d45a54 100644 --- a/tests/sentry/api/test_authentication.py +++ b/tests/sentry/api/test_authentication.py @@ -176,6 +176,20 @@ def test_no_match(self) -> None: with pytest.raises(AuthenticationFailed): self.auth.authenticate(request) + def test_authenticate_bearer_scheme_case_insensitive(self) -> None: + """ + Allow lowercase 'bearer' scheme for input authorization header. + """ + request = _drf_request() + request.META["HTTP_AUTHORIZATION"] = f"bearer {self.token}" + + result = self.auth.authenticate(request) + assert result is not None + user, auth = result + assert user.is_anonymous is False + assert user.id == self.user.id + assert AuthenticatedToken.from_token(auth) == AuthenticatedToken.from_token(self.api_token) + def test_inactive_key(self) -> None: self.org_auth_token.update(date_deactivated=datetime.now(UTC)) request = _drf_request() diff --git a/tests/sentry/web/frontend/test_oauth_authorize.py b/tests/sentry/web/frontend/test_oauth_authorize.py index eadc891caf6d6f..62dcb2d28d751d 100644 --- a/tests/sentry/web/frontend/test_oauth_authorize.py +++ b/tests/sentry/web/frontend/test_oauth_authorize.py @@ -341,10 +341,10 @@ def test_minimal_params_approve_flow(self) -> None: assert location == "https://example.com" fragment_d = parse_qs(fragment) assert fragment_d["access_token"] == [token.token] - assert fragment_d["token_type"] == ["bearer"] + assert fragment_d["token_type"] == ["Bearer"] assert "refresh_token" not in fragment_d assert fragment_d["expires_in"] - assert fragment_d["token_type"] == ["bearer"] + assert fragment_d["token_type"] == ["Bearer"] def test_minimal_params_code_deny_flow(self) -> None: self.login_as(self.user) diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index 683a47c7d49541..b8426ca24be7bb 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -30,7 +30,7 @@ def test_missing_grant_type(self) -> None: resp = self.client.post(self.path, {"client_id": "abcd", "client_secret": "abcd"}) assert resp.status_code == 400 - assert json.loads(resp.content) == {"error": "unsupported_grant_type"} + assert json.loads(resp.content) == {"error": "invalid_request"} def test_invalid_grant_type(self) -> None: self.login_as(self.user) @@ -59,21 +59,67 @@ def setUp(self) -> None: user=self.user, application=self.application, redirect_uri="https://example.com" ) - def test_missing_client_id(self) -> None: + def _basic_auth_header(self) -> dict[str, str]: + import base64 + + creds = f"{self.application.client_id}:{self.client_secret}".encode() + return {"HTTP_AUTHORIZATION": f"Basic {base64.b64encode(creds).decode('ascii')}"} + + def test_basic_auth_success(self) -> None: self.login_as(self.user) + headers = self._basic_auth_header() + resp = self.client.post( + self.path, + { + "grant_type": "authorization_code", + "redirect_uri": self.application.get_default_redirect_uri(), + "code": self.grant.code, + }, + **headers, + ) + assert resp.status_code == 200 + data = resp.json() + assert isinstance(data["expires_in"], int) + assert data["token_type"] == "Bearer" + assert "no-store" in resp["Cache-Control"] + assert resp["Pragma"] == "no-cache" + def test_basic_and_body_conflict(self) -> None: + self.login_as(self.user) + headers = self._basic_auth_header() resp = self.client.post( self.path, { "grant_type": "authorization_code", "redirect_uri": self.application.get_default_redirect_uri(), "code": self.grant.code, + "client_id": self.application.client_id, "client_secret": self.client_secret, }, + **headers, ) - assert resp.status_code == 400 - assert json.loads(resp.content) == {"error": "missing_client_id"} + assert resp.json() == {"error": "invalid_request"} + assert "no-store" in resp["Cache-Control"] + assert resp["Pragma"] == "no-cache" + + def test_missing_client_id(self) -> None: + self.login_as(self.user) + + resp = self.client.post( + self.path, + { + "grant_type": "authorization_code", + "redirect_uri": self.application.get_default_redirect_uri(), + "code": self.grant.code, + "client_secret": self.client_secret, + }, + ) + assert resp.status_code == 401 + assert json.loads(resp.content) == {"error": "invalid_client"} + assert resp["WWW-Authenticate"].startswith("Basic ") + assert "no-store" in resp["Cache-Control"] + assert resp["Pragma"] == "no-cache" def test_invalid_client_id(self) -> None: self.login_as(self.user) @@ -88,7 +134,7 @@ def test_invalid_client_id(self) -> None: }, ) assert resp.status_code == 401 - assert json.loads(resp.content) == {"error": "invalid_credentials"} + assert json.loads(resp.content) == {"error": "invalid_client"} def test_missing_client_secret(self) -> None: self.login_as(self.user) @@ -102,9 +148,8 @@ def test_missing_client_secret(self) -> None: "code": self.grant.code, }, ) - - assert resp.status_code == 400 - assert json.loads(resp.content) == {"error": "missing_client_secret"} + assert resp.status_code == 401 + assert json.loads(resp.content) == {"error": "invalid_client"} def test_invalid_client_secret(self) -> None: self.login_as(self.user) @@ -119,9 +164,8 @@ def test_invalid_client_secret(self) -> None: "client_secret": "rodrick_rules", }, ) - assert resp.status_code == 401 - assert json.loads(resp.content) == {"error": "invalid_credentials"} + assert json.loads(resp.content) == {"error": "invalid_client"} def test_missing_code(self) -> None: self.login_as(self.user) @@ -262,9 +306,10 @@ def test_no_open_id_token(self) -> None: data = json.loads(resp.content) assert "id_token" not in data - def test_valid_no_redirect_uri(self) -> None: + def test_missing_redirect_uri_when_bound(self) -> None: """ - Checks that we get the correct redirect URI if we don't pass one in + When the grant stored a redirect_uri, the token request must include + the exact same redirect_uri. """ self.login_as(self.user) @@ -277,20 +322,8 @@ def test_valid_no_redirect_uri(self) -> None: "client_secret": self.client_secret, }, ) - - assert resp.status_code == 200 - data = json.loads(resp.content) - - token = ApiToken.objects.get(token=data["access_token"]) - assert token.application == self.application - assert token.user == self.grant.user - assert token.get_scopes() == self.grant.get_scopes() - - assert data["access_token"] == token.token - assert data["refresh_token"] == token.refresh_token - assert isinstance(data["expires_in"], int) - assert data["token_type"] == "bearer" - assert data["user"]["id"] == str(token.user_id) + assert resp.status_code == 400 + assert json.loads(resp.content) == {"error": "invalid_grant"} def test_valid_params(self) -> None: self.login_as(self.user) @@ -317,7 +350,9 @@ def test_valid_params(self) -> None: assert data["access_token"] == token.token assert data["refresh_token"] == token.refresh_token assert isinstance(data["expires_in"], int) - assert data["token_type"] == "bearer" + assert data["token_type"] == "Bearer" + assert "no-store" in resp["Cache-Control"] + assert resp["Pragma"] == "no-cache" assert data["user"]["id"] == str(token.user_id) def test_valid_params_id_token(self) -> None: @@ -348,7 +383,7 @@ def test_valid_params_id_token(self) -> None: assert data["refresh_token"] == token.refresh_token assert data["access_token"] == token.token assert isinstance(data["expires_in"], int) - assert data["token_type"] == "bearer" + assert data["token_type"] == "Bearer" assert data["user"]["id"] == str(token.user_id) assert data["id_token"].count(".") == 2 @@ -381,7 +416,7 @@ def test_valid_params_id_token_additional_scopes(self) -> None: assert data["refresh_token"] == token.refresh_token assert data["access_token"] == token.token assert isinstance(data["expires_in"], int) - assert data["token_type"] == "bearer" + assert data["token_type"] == "Bearer" assert data["user"]["id"] == str(token.user_id) assert data["id_token"].count(".") == 2 @@ -419,8 +454,8 @@ def test_missing_client_id(self) -> None: }, ) - assert resp.status_code == 400 - assert json.loads(resp.content) == {"error": "missing_client_id"} + assert resp.status_code == 401 + assert json.loads(resp.content) == {"error": "invalid_client"} def test_invalid_client_id(self) -> None: self.login_as(self.user) @@ -436,7 +471,7 @@ def test_invalid_client_id(self) -> None: ) assert resp.status_code == 401 - assert json.loads(resp.content) == {"error": "invalid_credentials"} + assert json.loads(resp.content) == {"error": "invalid_client"} def test_missing_refresh_token(self) -> None: self.login_as(self.user) From 97f76b31506a38292bbdaa7b0a14c3c7a6f6c85f Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 9 Sep 2025 13:03:21 -0700 Subject: [PATCH 02/15] oauth/token: bound Basic auth b64 size to prevent DoS Validate the length of the Base64 segment in Authorization: Basic before decoding. If it exceeds 4096 bytes, reject with invalid_client (401). This prevents unbounded memory allocation from maliciously large headers. Add a test to cover the oversized header case. --- src/sentry/web/frontend/oauth_token.py | 13 +++++++++++++ tests/sentry/web/frontend/test_oauth_token.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index 8f3c6b5f753c86..56e7191a858647 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -24,6 +24,11 @@ logger = logging.getLogger("sentry.api.oauth_token") +# Max allowed length (in bytes/characters) of the Base64 section of a Basic +# Authorization header to prevent excessive memory allocation on decode. +# Client credentials (client_id:client_secret) should be small; 4KB is generous. +MAX_BASIC_AUTH_B64_LEN = 4096 + class _TokenInformationUser(TypedDict): id: str @@ -89,6 +94,14 @@ def post(self, request: Request) -> HttpResponse: scheme, _, param = auth_header.partition(" ") if scheme and scheme.lower() == "basic" and param: b64 = param.strip() + if len(b64) > MAX_BASIC_AUTH_B64_LEN: + logger.warning("Invalid Basic auth header: too long", extra={"client_id": None}) + return self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) try: decoded = base64.b64decode(b64).decode("utf-8") # format: client_id:client_secret (client_secret may be empty) diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index b8426ca24be7bb..456090d10abca8 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -65,6 +65,23 @@ def _basic_auth_header(self) -> dict[str, str]: creds = f"{self.application.client_id}:{self.client_secret}".encode() return {"HTTP_AUTHORIZATION": f"Basic {base64.b64encode(creds).decode('ascii')}"} + def test_basic_auth_header_too_large(self) -> None: + self.login_as(self.user) + oversized = "A" * 5001 # valid base64 chars, exceeds limit + headers = {"HTTP_AUTHORIZATION": f"Basic {oversized}"} + + resp = self.client.post( + self.path, + { + "grant_type": "authorization_code", + "redirect_uri": self.application.get_default_redirect_uri(), + "code": self.grant.code, + }, + **headers, + ) + assert resp.status_code == 401 + assert resp.json() == {"error": "invalid_client"} + def test_basic_auth_success(self) -> None: self.login_as(self.user) headers = self._basic_auth_header() From e5daec808863bd4ee6441c35b7c54233b7b92f8c Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 9 Sep 2025 13:28:51 -0700 Subject: [PATCH 03/15] tests: fix redirect_uri-bound code path and move bearer case-insensitive input test - Add redirect_uri to test_no_open_id_token to satisfy strict binding - Move lowercase 'bearer' input test under UserAuthTokenAuthentication where it applies - Remove misplaced case-insensitive test from OrgAuthToken auth class --- tests/sentry/api/test_authentication.py | 28 +++++++++---------- tests/sentry/web/frontend/test_oauth_token.py | 1 + 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/tests/sentry/api/test_authentication.py b/tests/sentry/api/test_authentication.py index 72ea8513d45a54..2faaa2c9333aaa 100644 --- a/tests/sentry/api/test_authentication.py +++ b/tests/sentry/api/test_authentication.py @@ -176,20 +176,6 @@ def test_no_match(self) -> None: with pytest.raises(AuthenticationFailed): self.auth.authenticate(request) - def test_authenticate_bearer_scheme_case_insensitive(self) -> None: - """ - Allow lowercase 'bearer' scheme for input authorization header. - """ - request = _drf_request() - request.META["HTTP_AUTHORIZATION"] = f"bearer {self.token}" - - result = self.auth.authenticate(request) - assert result is not None - user, auth = result - assert user.is_anonymous is False - assert user.id == self.user.id - assert AuthenticatedToken.from_token(auth) == AuthenticatedToken.from_token(self.api_token) - def test_inactive_key(self) -> None: self.org_auth_token.update(date_deactivated=datetime.now(UTC)) request = _drf_request() @@ -231,6 +217,20 @@ def test_no_match(self) -> None: with pytest.raises(AuthenticationFailed): self.auth.authenticate(request) + def test_authenticate_bearer_scheme_case_insensitive(self) -> None: + """ + Allow lowercase 'bearer' scheme for input authorization header. + """ + request = _drf_request() + request.META["HTTP_AUTHORIZATION"] = f"bearer {self.token}" + + result = self.auth.authenticate(request) + assert result is not None + user, auth = result + assert user.is_anonymous is False + assert user.id == self.user.id + assert AuthenticatedToken.from_token(auth) == AuthenticatedToken.from_token(self.api_token) + @control_silo_test class TestOrgScopedAppTokenAuthentication(TestCase): diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index 456090d10abca8..af1448918b19c7 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -314,6 +314,7 @@ def test_no_open_id_token(self) -> None: { "grant_type": "authorization_code", "code": self.grant.code, + "redirect_uri": self.application.get_default_redirect_uri(), "client_id": self.application.client_id, "client_secret": self.client_secret, }, From 0dad25d5c79e6a68c82416c7c610bdf436266eb6 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 9 Sep 2025 14:02:45 -0700 Subject: [PATCH 04/15] tests(oauth_token): pass HTTP_AUTHORIZATION via named kw to satisfy typing Switch header injection from **dict to explicit HTTP_AUTHORIZATION kw to align with client.post type hints and fix mypy arg-type errors. --- tests/sentry/web/frontend/test_oauth_token.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index af1448918b19c7..e784c3f7985c4e 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -59,16 +59,15 @@ def setUp(self) -> None: user=self.user, application=self.application, redirect_uri="https://example.com" ) - def _basic_auth_header(self) -> dict[str, str]: + def _basic_auth_value(self) -> str: import base64 creds = f"{self.application.client_id}:{self.client_secret}".encode() - return {"HTTP_AUTHORIZATION": f"Basic {base64.b64encode(creds).decode('ascii')}"} + return f"Basic {base64.b64encode(creds).decode('ascii')}" def test_basic_auth_header_too_large(self) -> None: self.login_as(self.user) oversized = "A" * 5001 # valid base64 chars, exceeds limit - headers = {"HTTP_AUTHORIZATION": f"Basic {oversized}"} resp = self.client.post( self.path, @@ -77,14 +76,14 @@ def test_basic_auth_header_too_large(self) -> None: "redirect_uri": self.application.get_default_redirect_uri(), "code": self.grant.code, }, - **headers, + HTTP_AUTHORIZATION=f"Basic {oversized}", ) assert resp.status_code == 401 assert resp.json() == {"error": "invalid_client"} def test_basic_auth_success(self) -> None: self.login_as(self.user) - headers = self._basic_auth_header() + auth_value = self._basic_auth_value() resp = self.client.post( self.path, { @@ -92,7 +91,7 @@ def test_basic_auth_success(self) -> None: "redirect_uri": self.application.get_default_redirect_uri(), "code": self.grant.code, }, - **headers, + HTTP_AUTHORIZATION=auth_value, ) assert resp.status_code == 200 data = resp.json() @@ -103,7 +102,7 @@ def test_basic_auth_success(self) -> None: def test_basic_and_body_conflict(self) -> None: self.login_as(self.user) - headers = self._basic_auth_header() + auth_value = self._basic_auth_value() resp = self.client.post( self.path, { @@ -113,7 +112,7 @@ def test_basic_and_body_conflict(self) -> None: "client_id": self.application.client_id, "client_secret": self.client_secret, }, - **headers, + HTTP_AUTHORIZATION=auth_value, ) assert resp.status_code == 400 assert resp.json() == {"error": "invalid_request"} From 85294907d6d2987374a3f0704bf64387572d7ebd Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 09:57:34 -0700 Subject: [PATCH 05/15] refactor(oauth): rely on never_cache for token responses - Remove manual Cache-Control/Pragma headers; add spec-referenced comment. - Apply never_cache at dispatch so all methods inherit headers; drop per-method decorator. Refs: https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 --- .pr-99148-body.txt | 10 ++++++++++ src/sentry/web/frontend/oauth_token.py | 12 +++++------- 2 files changed, 15 insertions(+), 7 deletions(-) create mode 100644 .pr-99148-body.txt diff --git a/.pr-99148-body.txt b/.pr-99148-body.txt new file mode 100644 index 00000000000000..c02025dec24e5b --- /dev/null +++ b/.pr-99148-body.txt @@ -0,0 +1,10 @@ +- Prefer Authorization: Basic over body; both present → invalid_request (400). Missing/invalid creds → invalid_client (401) with WWW-Authenticate. Responses are non-cacheable via Django never_cache per RFC 6749 §§5.1/5.2; no manual headers. +- Standardize token_type to Bearer; enforce redirect_uri binding; missing grant_type → invalid_request (400); accept case-insensitive schemes. +- Bound Basic base64 to 4096 bytes to prevent DoS; tests updated. + +Refs: +https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-13 +https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 +https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 +https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 +https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.3 diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index 56e7191a858647..da9d952bdbc716 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -50,6 +50,11 @@ class _TokenInformation(TypedDict): @control_silo_view class OAuthTokenView(View): + # OAuth 2.0 requires token endpoint responses to be non-cacheable. + # RFC 6749 §5.1 (Successful Response) and §5.2 (Error Response) specify + # that authorization servers SHOULD include cache prevention (e.g. no-store). + # We rely on Django's never_cache at dispatch so all methods inherit the + # appropriate headers, instead of setting them manually per response. @csrf_exempt @method_decorator(never_cache) def dispatch(self, request, *args, **kwargs): @@ -71,15 +76,11 @@ def error(self, request: HttpRequest, name, reason=None, status=400): resp = HttpResponse( json.dumps({"error": name}), content_type="application/json", status=status ) - # RFC 6749 §5.2, §5.1 cache prevention - resp["Cache-Control"] = "no-store" - resp["Pragma"] = "no-cache" # RFC 6749 §5.2 invalid_client requires WWW-Authenticate header if name == "invalid_client": resp["WWW-Authenticate"] = 'Basic realm="oauth"' return resp - @method_decorator(never_cache) def post(self, request: Request) -> HttpResponse: grant_type = request.POST.get("grant_type") @@ -272,7 +273,4 @@ def process_token_details( json.dumps(token_information), content_type="application/json", ) - # RFC 6749 §5.1 cache prevention - resp["Cache-Control"] = "no-store" - resp["Pragma"] = "no-cache" return resp From 2af724ff17a2acaf625df3760faf0a68fe6b6737 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 09:58:06 -0700 Subject: [PATCH 06/15] refactor(oauth): rely on never_cache for token responses - Remove manual Cache-Control/Pragma headers; add spec-referenced comment. - Apply never_cache at dispatch so all methods inherit headers; drop per-method decorator. Refs: https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 --- .git-commit-msg.txt | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .git-commit-msg.txt diff --git a/.git-commit-msg.txt b/.git-commit-msg.txt new file mode 100644 index 00000000000000..873bc3fc48b87c --- /dev/null +++ b/.git-commit-msg.txt @@ -0,0 +1,8 @@ +refactor(oauth): rely on never_cache for token responses + +- Remove manual Cache-Control/Pragma headers; add spec-referenced comment. +- Apply never_cache at dispatch so all methods inherit headers; drop per-method decorator. + +Refs: +https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 +https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 From 7f8db506480cf9f4d802ef45edbacaed4956661f Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 09:59:55 -0700 Subject: [PATCH 07/15] thanks ai --- .git-commit-msg.txt | 8 -------- .pr-99148-body.txt | 10 ---------- 2 files changed, 18 deletions(-) delete mode 100644 .git-commit-msg.txt delete mode 100644 .pr-99148-body.txt diff --git a/.git-commit-msg.txt b/.git-commit-msg.txt deleted file mode 100644 index 873bc3fc48b87c..00000000000000 --- a/.git-commit-msg.txt +++ /dev/null @@ -1,8 +0,0 @@ -refactor(oauth): rely on never_cache for token responses - -- Remove manual Cache-Control/Pragma headers; add spec-referenced comment. -- Apply never_cache at dispatch so all methods inherit headers; drop per-method decorator. - -Refs: -https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 -https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 diff --git a/.pr-99148-body.txt b/.pr-99148-body.txt deleted file mode 100644 index c02025dec24e5b..00000000000000 --- a/.pr-99148-body.txt +++ /dev/null @@ -1,10 +0,0 @@ -- Prefer Authorization: Basic over body; both present → invalid_request (400). Missing/invalid creds → invalid_client (401) with WWW-Authenticate. Responses are non-cacheable via Django never_cache per RFC 6749 §§5.1/5.2; no manual headers. -- Standardize token_type to Bearer; enforce redirect_uri binding; missing grant_type → invalid_request (400); accept case-insensitive schemes. -- Bound Basic base64 to 4096 bytes to prevent DoS; tests updated. - -Refs: -https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-13 -https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 -https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 -https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 -https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.3 From 21b41078003bfe5a1447d31b8be56198bfa57771 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 10:08:24 -0700 Subject: [PATCH 08/15] refactor(oauth): extract Basic auth parsing into helper - Parse Authorization: Basic from request.META only; return (id, secret) with error response. - Simplify token view; keep RFC semantics (invalid_client on malformed header). Refs: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-13 --- src/sentry/web/frontend/oauth_token.py | 81 ++++++++++++++++---------- 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index da9d952bdbc716..9c71f3425a12dd 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -1,5 +1,6 @@ from __future__ import annotations +import base64 import logging from datetime import datetime from typing import Literal, NotRequired, TypedDict @@ -83,40 +84,12 @@ def error(self, request: HttpRequest, name, reason=None, status=400): def post(self, request: Request) -> HttpResponse: grant_type = request.POST.get("grant_type") - # Extract client credentials per OAuth 2.1 (prefer Basic header over body) - auth_header = request.META.get("HTTP_AUTHORIZATION", "") or request.headers.get( - "Authorization", "" + (basic_client_id, basic_client_secret), header_error = self._extract_basic_auth_credentials( + request ) - basic_client_id = basic_client_secret = None - if isinstance(auth_header, str) and auth_header: - import base64 - - scheme, _, param = auth_header.partition(" ") - if scheme and scheme.lower() == "basic" and param: - b64 = param.strip() - if len(b64) > MAX_BASIC_AUTH_B64_LEN: - logger.warning("Invalid Basic auth header: too long", extra={"client_id": None}) - return self.error( - request=request, - name="invalid_client", - reason="invalid basic auth", - status=401, - ) - try: - decoded = base64.b64decode(b64).decode("utf-8") - # format: client_id:client_secret (client_secret may be empty) - if ":" not in decoded: - raise ValueError("missing colon in basic credentials") - basic_client_id, basic_client_secret = decoded.split(":", 1) - except Exception: - logger.warning("Invalid Basic auth header", extra={"client_id": None}) - return self.error( - request=request, - name="invalid_client", - reason="invalid basic auth", - status=401, - ) + if header_error is not None: + return header_error body_client_id = request.POST.get("client_id") body_client_secret = request.POST.get("client_secret") @@ -189,6 +162,50 @@ def post(self, request: Request) -> HttpResponse: id_token=token_data["id_token"] if "id_token" in token_data else None, ) + def _extract_basic_auth_credentials( + self, request: Request + ) -> tuple[tuple[str | None, str | None], HttpResponse | None]: + """ + Parse client credentials from Authorization header (Basic scheme). + + Returns ((client_id, client_secret), error_response). If parsing fails in a way + that requires an immediate HTTP response (e.g., invalid_client), error_response + will be a populated HttpResponse; otherwise it will be None and credentials may + be None when Basic auth is not present. + """ + auth_header = request.META.get("HTTP_AUTHORIZATION", "") + if not isinstance(auth_header, str) or not auth_header: + return (None, None), None + + scheme, _, param = auth_header.partition(" ") + if not scheme or scheme.lower() != "basic" or not param: + return (None, None), None + + b64 = param.strip() + if len(b64) > MAX_BASIC_AUTH_B64_LEN: + logger.warning("Invalid Basic auth header: too long", extra={"client_id": None}) + return (None, None), self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) + try: + decoded = base64.b64decode(b64).decode("utf-8") + # format: client_id:client_secret (client_secret may be empty) + if ":" not in decoded: + raise ValueError("missing colon in basic credentials") + client_id, client_secret = decoded.split(":", 1) + return (client_id, client_secret), None + except Exception: + logger.warning("Invalid Basic auth header", extra={"client_id": None}) + return (None, None), self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) + def get_access_tokens(self, request: Request, application: ApiApplication) -> dict: code = request.POST.get("code") try: From 88d8a57e839941493d03b39ad774b4b51e47c85b Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 10:24:05 -0700 Subject: [PATCH 09/15] refactor(oauth): simplify credential selection; disallow mixed mechanisms - Read Basic from request.META; set client_id/client_secret from header or body (not both). - Return invalid_request on mixed header+body credentials; keep invalid_client for missing/invalid creds. Refs: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-13 --- src/sentry/web/frontend/oauth_token.py | 30 ++++++++++++++------------ 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index 9c71f3425a12dd..fd2efcbfe6be8b 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -84,29 +84,31 @@ def error(self, request: HttpRequest, name, reason=None, status=400): def post(self, request: Request) -> HttpResponse: grant_type = request.POST.get("grant_type") - # Extract client credentials per OAuth 2.1 (prefer Basic header over body) + # Extract Basic credentials if present; if body credentials are also present, + # treat as invalid_request per spec (only one auth mechanism may be used). (basic_client_id, basic_client_secret), header_error = self._extract_basic_auth_credentials( request ) if header_error is not None: return header_error + client_id = basic_client_id + client_secret = basic_client_secret + body_client_id = request.POST.get("client_id") body_client_secret = request.POST.get("client_secret") - # If both Basic header and body credentials are present -> invalid_request - if basic_client_id is not None and (body_client_id or body_client_secret): - logger.info( - "oauth.basic-and-body-credentials", - extra={"client_id": basic_client_id or body_client_id, "reason": "conflict"}, - ) - return self.error(request=request, name="invalid_request", reason="credential conflict") - - # Choose credentials: header preferred, then body - client_id = basic_client_id if basic_client_id is not None else body_client_id - client_secret = ( - basic_client_secret if basic_client_secret is not None else body_client_secret - ) + if body_client_id or body_client_secret: + if client_id is not None or client_secret is not None: + logger.info( + "oauth.basic-and-body-credentials", + extra={"client_id": client_id or body_client_id, "reason": "conflict"}, + ) + return self.error( + request=request, name="invalid_request", reason="credential conflict" + ) + client_id = body_client_id + client_secret = body_client_secret metrics.incr( "oauth_token.post.start", From 170943c2e92104924a5e599faf928a2661d34781 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 10:54:43 -0700 Subject: [PATCH 10/15] test(oauth): drop Pragma assertions; rely on never_cache - Remove Pragma checks on token responses; Cache-Control no-store remains. Refs: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Pragma https://datatracker.ietf.org/doc/html/rfc6749#section-5.1 https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 --- tests/sentry/web/frontend/test_oauth_token.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index e784c3f7985c4e..66ffa2ff8496ad 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -98,7 +98,6 @@ def test_basic_auth_success(self) -> None: assert isinstance(data["expires_in"], int) assert data["token_type"] == "Bearer" assert "no-store" in resp["Cache-Control"] - assert resp["Pragma"] == "no-cache" def test_basic_and_body_conflict(self) -> None: self.login_as(self.user) @@ -117,7 +116,6 @@ def test_basic_and_body_conflict(self) -> None: assert resp.status_code == 400 assert resp.json() == {"error": "invalid_request"} assert "no-store" in resp["Cache-Control"] - assert resp["Pragma"] == "no-cache" def test_missing_client_id(self) -> None: self.login_as(self.user) @@ -135,7 +133,6 @@ def test_missing_client_id(self) -> None: assert json.loads(resp.content) == {"error": "invalid_client"} assert resp["WWW-Authenticate"].startswith("Basic ") assert "no-store" in resp["Cache-Control"] - assert resp["Pragma"] == "no-cache" def test_invalid_client_id(self) -> None: self.login_as(self.user) @@ -369,7 +366,6 @@ def test_valid_params(self) -> None: assert isinstance(data["expires_in"], int) assert data["token_type"] == "Bearer" assert "no-store" in resp["Cache-Control"] - assert resp["Pragma"] == "no-cache" assert data["user"]["id"] == str(token.user_id) def test_valid_params_id_token(self) -> None: From e99254e5523f298e9aec428911209622e31a75bb Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 18:15:17 -0500 Subject: [PATCH 11/15] Clean up abstraction --- src/sentry/web/frontend/oauth_token.py | 172 +++++++++++++++---------- 1 file changed, 107 insertions(+), 65 deletions(-) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index fd2efcbfe6be8b..ee0cc0f8550ea0 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -51,11 +51,9 @@ class _TokenInformation(TypedDict): @control_silo_view class OAuthTokenView(View): - # OAuth 2.0 requires token endpoint responses to be non-cacheable. - # RFC 6749 §5.1 (Successful Response) and §5.2 (Error Response) specify - # that authorization servers SHOULD include cache prevention (e.g. no-store). - # We rely on Django's never_cache at dispatch so all methods inherit the - # appropriate headers, instead of setting them manually per response. + # Token responses must not be cached per RFC 6749 §5.1/§5.2. We apply + # never_cache at dispatch so every response from this endpoint is marked + # appropriately without repeating headers across handlers. @csrf_exempt @method_decorator(never_cache) def dispatch(self, request, *args, **kwargs): @@ -83,32 +81,37 @@ def error(self, request: HttpRequest, name, reason=None, status=400): return resp def post(self, request: Request) -> HttpResponse: + """OAuth 2.0 token endpoint (RFC 6749 §3.2). + + Purpose + - Exchanges an authorization code for tokens, or uses a refresh token to + obtain a new access token. + + Supported grant types + - `authorization_code` (RFC 6749 §4.1): requires `code` and, if bound, + a matching `redirect_uri` (§4.1.3). If `openid` scope was granted and + signing is configured, an `id_token` (OIDC Core 1.0) is included. + - `refresh_token` (RFC 6749 §6): requires `refresh_token`. Supplying `scope` + is not supported here and returns `invalid_request`. + + Client authentication + - Either Authorization header (Basic) or form fields `client_id`/`client_secret` + (RFC 6749 §2.3.1). Only one method may be used per request. + + Responses + - Success (RFC 6749 §5.1): 200 JSON with `access_token`, `refresh_token`, + `expires_in`/`expires_at`, `token_type` (Bearer), `scope`, `user`, and + optionally `id_token` and `organization_id`. + - Errors (RFC 6749 §5.2): 400 JSON for `invalid_request`, `invalid_grant`, + `unsupported_grant_type`; 401 JSON for `invalid_client` (with + `WWW-Authenticate: Basic realm="oauth"`). + """ grant_type = request.POST.get("grant_type") - # Extract Basic credentials if present; if body credentials are also present, - # treat as invalid_request per spec (only one auth mechanism may be used). - (basic_client_id, basic_client_secret), header_error = self._extract_basic_auth_credentials( - request - ) - if header_error is not None: - return header_error - - client_id = basic_client_id - client_secret = basic_client_secret - - body_client_id = request.POST.get("client_id") - body_client_secret = request.POST.get("client_secret") - if body_client_id or body_client_secret: - if client_id is not None or client_secret is not None: - logger.info( - "oauth.basic-and-body-credentials", - extra={"client_id": client_id or body_client_id, "reason": "conflict"}, - ) - return self.error( - request=request, name="invalid_request", reason="credential conflict" - ) - client_id = body_client_id - client_secret = body_client_secret + # Determine client credentials from header or body (mutually exclusive). + (client_id, client_secret), cred_error = self._extract_basic_auth_credentials(request) + if cred_error is not None: + return cred_error metrics.incr( "oauth_token.post.start", @@ -168,45 +171,84 @@ def _extract_basic_auth_credentials( self, request: Request ) -> tuple[tuple[str | None, str | None], HttpResponse | None]: """ - Parse client credentials from Authorization header (Basic scheme). - - Returns ((client_id, client_secret), error_response). If parsing fails in a way - that requires an immediate HTTP response (e.g., invalid_client), error_response - will be a populated HttpResponse; otherwise it will be None and credentials may - be None when Basic auth is not present. + Extract client credentials from the request. + + Supported mechanisms (mutually exclusive): + - HTTP Authorization header with the Basic scheme + (RFC 6749 §2.3.1; Basic syntax per RFC 7617: base64(client_id:client_secret)). + - Form body fields: `client_id` and `client_secret` (RFC 6749 §2.3.1). + + Returns ((client_id, client_secret), error_response). + - If both mechanisms are present, returns `invalid_request` (client MUST NOT + use more than one authentication method; RFC 6749 §2.3). + - If the Basic header is malformed, returns `invalid_client` with 401 + (error semantics per RFC 6749 §5.2; Basic auth per §2.3.1). + - If neither mechanism is present, returns (None, None), None and the + caller enforces `invalid_client` as appropriate (RFC 6749 §5.2). + + Note: This helper enforces a conservative upper bound on Basic header + payload size for robustness; this limit is an implementation detail and + not dictated by the RFCs. """ + # Check for Basic auth header first auth_header = request.META.get("HTTP_AUTHORIZATION", "") - if not isinstance(auth_header, str) or not auth_header: - return (None, None), None - - scheme, _, param = auth_header.partition(" ") - if not scheme or scheme.lower() != "basic" or not param: - return (None, None), None + has_auth_header = isinstance(auth_header, str) and bool(auth_header) - b64 = param.strip() - if len(b64) > MAX_BASIC_AUTH_B64_LEN: - logger.warning("Invalid Basic auth header: too long", extra={"client_id": None}) - return (None, None), self.error( - request=request, - name="invalid_client", - reason="invalid basic auth", - status=401, - ) - try: - decoded = base64.b64decode(b64).decode("utf-8") - # format: client_id:client_secret (client_secret may be empty) - if ":" not in decoded: - raise ValueError("missing colon in basic credentials") - client_id, client_secret = decoded.split(":", 1) - return (client_id, client_secret), None - except Exception: - logger.warning("Invalid Basic auth header", extra={"client_id": None}) - return (None, None), self.error( - request=request, - name="invalid_client", - reason="invalid basic auth", - status=401, - ) + body_client_id = request.POST.get("client_id") + body_client_secret = request.POST.get("client_secret") + has_body_credentials = bool(body_client_id) or bool(body_client_secret) + + # If both mechanisms are present, this is an invalid request per spec. + if has_auth_header: + scheme, _, param = auth_header.partition(" ") + if scheme and scheme.lower() == "basic" and param: + if has_body_credentials: + logger.info( + "oauth.basic-and-body-credentials", + extra={ + "client_id": body_client_id, + "reason": "conflict", + }, + ) + return (None, None), self.error( + request=request, + name="invalid_request", + reason="credential conflict", + ) + + # Enforce a reasonable upper bound on the Base64 payload to + # avoid excessive memory use on decode. + b64 = param.strip() + if len(b64) > MAX_BASIC_AUTH_B64_LEN: + logger.warning("Invalid Basic auth header: too long", extra={"client_id": None}) + return (None, None), self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) + try: + decoded = base64.b64decode(b64).decode("utf-8") + # format: client_id:client_secret (client_secret may be empty) + if ":" not in decoded: + raise ValueError("missing colon in basic credentials") + client_id, client_secret = decoded.split(":", 1) + return (client_id, client_secret), None + except Exception: + logger.warning("Invalid Basic auth header", extra={"client_id": None}) + return (None, None), self.error( + request=request, + name="invalid_client", + reason="invalid basic auth", + status=401, + ) + + # No usable Basic header; fall back to body credentials if provided. + if has_body_credentials: + return (body_client_id, body_client_secret), None + + # Neither header nor body provided credentials. + return (None, None), None def get_access_tokens(self, request: Request, application: ApiApplication) -> dict: code = request.POST.get("code") From e3ddb4d29147334cae2738fec86cdffa93b5ddd0 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 18:29:44 -0500 Subject: [PATCH 12/15] improve logging --- src/sentry/web/frontend/oauth_authorize.py | 4 ++-- src/sentry/web/frontend/oauth_token.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/sentry/web/frontend/oauth_authorize.py b/src/sentry/web/frontend/oauth_authorize.py index 5367d1386cabec..2f922b912016a5 100644 --- a/src/sentry/web/frontend/oauth_authorize.py +++ b/src/sentry/web/frontend/oauth_authorize.py @@ -20,7 +20,7 @@ from sentry.utils import metrics from sentry.web.frontend.auth_login import AuthLoginView -logger = logging.getLogger("sentry.api.oauth_authorize") +logger = logging.getLogger("sentry.oauth") class OAuthAuthorizeView(AuthLoginView): @@ -66,7 +66,7 @@ def error( client_id=None, err_response=None, ): - logging.error( + logger.error( "oauth.authorize-error", extra={ "error_name": name, diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index ee0cc0f8550ea0..a06bb77afeb06a 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -23,7 +23,7 @@ from sentry.web.frontend.base import control_silo_view from sentry.web.frontend.openidtoken import OpenIDToken -logger = logging.getLogger("sentry.api.oauth_token") +logger = logging.getLogger("sentry.oauth") # Max allowed length (in bytes/characters) of the Base64 section of a Basic # Authorization header to prevent excessive memory allocation on decode. @@ -63,7 +63,7 @@ def dispatch(self, request, *args, **kwargs): def error(self, request: HttpRequest, name, reason=None, status=400): client_id = request.POST.get("client_id") - logging.error( + logger.error( "oauth.token-error", extra={ "error_name": name, @@ -98,6 +98,9 @@ def post(self, request: Request) -> HttpResponse: - Either Authorization header (Basic) or form fields `client_id`/`client_secret` (RFC 6749 §2.3.1). Only one method may be used per request. + Request format + - Requests are `application/x-www-form-urlencoded` as defined in RFC 6749 §3.2. + Responses - Success (RFC 6749 §5.1): 200 JSON with `access_token`, `refresh_token`, `expires_in`/`expires_at`, `token_type` (Bearer), `scope`, `user`, and From 71a7f39a5959e3862732a2008b26568f6baf3b30 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 10 Sep 2025 18:37:36 -0500 Subject: [PATCH 13/15] fix expires_in --- src/sentry/web/frontend/oauth_authorize.py | 2 +- tests/sentry/web/frontend/test_oauth_authorize.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/web/frontend/oauth_authorize.py b/src/sentry/web/frontend/oauth_authorize.py index 2f922b912016a5..ce927c531697f2 100644 --- a/src/sentry/web/frontend/oauth_authorize.py +++ b/src/sentry/web/frontend/oauth_authorize.py @@ -439,7 +439,7 @@ def approve( redirect_uri, { "access_token": token.token, - "expires_in": int((timezone.now() - token.expires_at).total_seconds()), + "expires_in": int((token.expires_at - timezone.now()).total_seconds()), "expires_at": token.expires_at.strftime("%Y-%m-%dT%H:%M:%S.%fZ"), "token_type": "Bearer", "scope": " ".join(token.get_scopes()), diff --git a/tests/sentry/web/frontend/test_oauth_authorize.py b/tests/sentry/web/frontend/test_oauth_authorize.py index 62dcb2d28d751d..e2ed66e49c9851 100644 --- a/tests/sentry/web/frontend/test_oauth_authorize.py +++ b/tests/sentry/web/frontend/test_oauth_authorize.py @@ -343,7 +343,9 @@ def test_minimal_params_approve_flow(self) -> None: assert fragment_d["access_token"] == [token.token] assert fragment_d["token_type"] == ["Bearer"] assert "refresh_token" not in fragment_d + # expires_in should be a positive integer number of seconds until expiry assert fragment_d["expires_in"] + assert int(fragment_d["expires_in"][0]) > 0 assert fragment_d["token_type"] == ["Bearer"] def test_minimal_params_code_deny_flow(self) -> None: From 6f5e5c580a67d2d9655bd0a824fa1d65b58a5b6a Mon Sep 17 00:00:00 2001 From: David Cramer Date: Tue, 23 Sep 2025 15:06:15 -0700 Subject: [PATCH 14/15] fix validation --- src/sentry/web/frontend/oauth_token.py | 2 +- tests/sentry/web/frontend/test_oauth_token.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index a06bb77afeb06a..d34b425eae96b4 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -231,7 +231,7 @@ def _extract_basic_auth_credentials( status=401, ) try: - decoded = base64.b64decode(b64).decode("utf-8") + decoded = base64.b64decode(b64.encode("ascii"), validate=True).decode("utf-8") # format: client_id:client_secret (client_secret may be empty) if ":" not in decoded: raise ValueError("missing colon in basic credentials") diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index 66ffa2ff8496ad..3d2ba70ccb86ad 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -99,6 +99,23 @@ def test_basic_auth_success(self) -> None: assert data["token_type"] == "Bearer" assert "no-store" in resp["Cache-Control"] + def test_basic_auth_invalid_base64_character(self) -> None: + self.login_as(self.user) + invalid_value = f"{self._basic_auth_value()}$" + resp = self.client.post( + self.path, + { + "grant_type": "authorization_code", + "redirect_uri": self.application.get_default_redirect_uri(), + "code": self.grant.code, + }, + HTTP_AUTHORIZATION=invalid_value, + ) + assert resp.status_code == 401 + assert resp.json() == {"error": "invalid_client"} + assert resp["WWW-Authenticate"].startswith("Basic ") + assert "no-store" in resp["Cache-Control"] + def test_basic_and_body_conflict(self) -> None: self.login_as(self.user) auth_value = self._basic_auth_value() From cdf423f7e44db90034505c967ce84ad1376b3921 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Thu, 23 Oct 2025 14:34:17 -0700 Subject: [PATCH 15/15] Act on feedback --- src/sentry/web/frontend/oauth_token.py | 24 ++++++++++++-- tests/sentry/web/frontend/test_oauth_token.py | 32 +++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index d34b425eae96b4..dab2345230885b 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -155,6 +155,25 @@ def post(self, request: Request) -> HttpResponse: status=401, ) + # Defense-in-depth: verify the application's client_id matches the request. + # This should always be true given the query above, but protects against + # potential logic bugs or database inconsistencies. + if application.client_id != client_id: + logger.error( + "Application client_id mismatch", + extra={ + "requested_client_id": client_id, + "application_client_id": application.client_id, + "application_id": application.id, + }, + ) + return self.error( + request=request, + name="invalid_client", + reason="client_id mismatch", + status=401, + ) + if grant_type == GrantTypes.AUTHORIZATION: token_data = self.get_access_tokens(request=request, application=application) else: @@ -227,7 +246,7 @@ def _extract_basic_auth_credentials( return (None, None), self.error( request=request, name="invalid_client", - reason="invalid basic auth", + reason="invalid basic auth (too long)", status=401, ) try: @@ -333,8 +352,7 @@ def process_token_details( token_information["id_token"] = id_token if token.scoping_organization_id: token_information["organization_id"] = str(token.scoping_organization_id) - resp = HttpResponse( + return HttpResponse( json.dumps(token_information), content_type="application/json", ) - return resp diff --git a/tests/sentry/web/frontend/test_oauth_token.py b/tests/sentry/web/frontend/test_oauth_token.py index 3d2ba70ccb86ad..6d603e8f8f7366 100644 --- a/tests/sentry/web/frontend/test_oauth_token.py +++ b/tests/sentry/web/frontend/test_oauth_token.py @@ -385,6 +385,38 @@ def test_valid_params(self) -> None: assert "no-store" in resp["Cache-Control"] assert data["user"]["id"] == str(token.user_id) + def test_expires_in_value(self) -> None: + """ + Verify that expires_in correctly represents seconds until expiry. + The old code incorrectly calculated (now - expires_at) instead of + (expires_at - now), producing negative values for valid tokens. + """ + self.login_as(self.user) + + resp = self.client.post( + self.path, + { + "grant_type": "authorization_code", + "redirect_uri": self.application.get_default_redirect_uri(), + "code": self.grant.code, + "client_id": self.application.client_id, + "client_secret": self.client_secret, + }, + ) + + assert resp.status_code == 200 + data = json.loads(resp.content) + + # Default token expiration is 30 days (2,592,000 seconds) + # expires_in should be positive and close to 30 days + expires_in = data["expires_in"] + assert isinstance(expires_in, int) + assert expires_in > 0, "expires_in should be positive (seconds until expiry)" + # Allow for a few seconds of test execution time, but should be close to 30 days + expected_seconds = 30 * 24 * 60 * 60 # 2,592,000 seconds + assert expires_in >= expected_seconds - 60, "expires_in should be close to 30 days" + assert expires_in <= expected_seconds, "expires_in should not exceed 30 days" + def test_valid_params_id_token(self) -> None: self.login_as(self.user) open_id_grant = ApiGrant.objects.create(