feat(oauth): add client_secret_basic and RFC errors by dcramer · Pull Request #99148 · getsentry/sentry

dcramer · 2025-09-09T19:47:58Z

Prefer Authorization: Basic over body; both present → invalid_request (400). Missing/invalid creds → invalid_client (401) with WWW-Authenticate.
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
Fix existing bug with expires_in for the authorize response

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

codecov · 2025-09-09T20:00:26Z

Codecov Report

❌ Patch coverage is 91.83673% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/web/frontend/oauth_token.py	91.48%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #99148      +/-   ##
==========================================
+ Coverage   77.04%   80.97%   +3.93%     
==========================================
  Files        8733     8734       +1     
  Lines      388558   388613      +55     
  Branches    24641    24641              
==========================================
+ Hits       299357   314696   +15339     
+ Misses      88841    73557   -15284     
  Partials      360      360

dcramer · 2025-09-10T17:43:43Z

worth noting small chance this breaks an oauth consumer, but the chances are very low as most folks wouldnt be confirming the token_type value, and our oauth provider isnt extremely wide spread

dcramer · 2025-09-10T23:06:16Z

Gonna refactor this one a bit more as I'm still not happy w/ the abstraction, but the general functionality should be good

- 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)

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.

…ive 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

…yping Switch header injection from **dict to explicit HTTP_AUTHORIZATION kw to align with client.post type hints and fix mypy arg-type errors.

- 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

- 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

…isms - 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

- 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

Copilot

Pull Request Overview

This PR enhances OAuth 2.0 compliance by implementing RFC 6749 requirements for client authentication and error handling. The changes standardize token responses to use "Bearer" (capitalized), add support for HTTP Basic authentication with proper conflict detection, enforce redirect URI binding for authorization code grants, and fix an existing bug where expires_in was calculated as a negative value.

Key Changes:

Add client_secret_basic authentication method with validation and conflict detection between header and body credentials
Standardize error codes: missing credentials → invalid_client (401), missing grant_type → invalid_request (400), with WWW-Authenticate headers
Fix expires_in calculation bug in authorize response (was timezone.now() - token.expires_at, now correctly token.expires_at - timezone.now())

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/sentry/web/frontend/oauth_token.py	Core implementation of Basic auth extraction, error standardization, redirect_uri binding enforcement, and token_type capitalization
src/sentry/web/frontend/oauth_authorize.py	Fix expires_in bug, standardize token_type to "Bearer", update logger name
tests/sentry/web/frontend/test_oauth_token.py	Comprehensive test coverage for Basic auth scenarios, error code updates, redirect_uri binding
tests/sentry/web/frontend/test_oauth_authorize.py	Update assertions for Bearer token_type and validate expires_in as positive integer
tests/sentry/api/test_authentication.py	Add test for case-insensitive bearer scheme acceptance

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-23T20:30:14Z

+                        status=401,
+                    )
+                try:
+                    decoded = base64.b64decode(b64.encode("ascii"), validate=True).decode("utf-8")


The broad exception handler on line 240 catches all exceptions including encoding errors. Consider logging the specific exception type or message to aid debugging when Basic auth fails (e.g., 'Invalid Basic auth header: <exception_type>').

BYK

Looks good overall. Our invalid expires_in logic in the old code is mind blowing.

Given that we messed that up, I think we should add a test that ensure the value is correct now (didn't see that in the new tests).

Didn't scrutinize the test code much but they look fine overall.

BYK · 2025-10-23T20:19:08Z

@@ -439,9 +439,9 @@ def approve(
                redirect_uri,
                {
                    "access_token": token.token,
-                    "expires_in": int((timezone.now() - token.expires_at).total_seconds()),


BYK · 2025-10-23T20:24:17Z

+        if cred_error is not None:
+            return cred_error


What is this, golang? 🙄

dcramer · 2025-10-23T21:41:19Z

will merge tomorrow morning

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

- Prefer Authorization: Basic over body; both present → invalid_request (400). Missing/invalid creds → invalid_client (401) with WWW-Authenticate. - 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 - Fix existing bug with expires_in for the authorize response 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

dcramer requested a review from a team as a code owner September 9, 2025 19:47

github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 9, 2025

vercel Bot deployed to Preview September 9, 2025 19:49 View deployment

github-advanced-security AI found potential problems Sep 9, 2025

View reviewed changes

Comment thread src/sentry/web/frontend/oauth_token.py Fixed

seer-by-sentry Bot reviewed Sep 9, 2025

View reviewed changes

Comment thread src/sentry/web/frontend/oauth_token.py Outdated

vercel Bot deployed to Preview September 9, 2025 20:05 View deployment

dcramer force-pushed the oauth-client-secret-basic branch from ed682a6 to 09f63d2 Compare September 9, 2025 20:34

vercel Bot deployed to Preview September 9, 2025 20:37 View deployment

vercel Bot deployed to Preview September 9, 2025 21:07 View deployment

dcramer mentioned this pull request Sep 10, 2025

OAuth 2.1 — Plan and Tracking #99002

Open

59 tasks

mdtro reviewed Sep 10, 2025

View reviewed changes

Comment thread src/sentry/web/frontend/oauth_token.py Outdated

dcramer changed the title ~~feat(oauth): client_secret_basic, RFC errors, cache headers~~ feat(oauth): add client_secret_basic and RFC errors Sep 10, 2025

vercel Bot deployed to Preview September 10, 2025 17:02 View deployment

vercel Bot deployed to Preview September 10, 2025 17:33 View deployment

vercel Bot deployed to Preview September 10, 2025 17:59 View deployment

dcramer requested a review from leedongwei September 10, 2025 23:04

vercel Bot deployed to Preview September 10, 2025 23:17 View deployment

vercel Bot deployed to Preview September 10, 2025 23:31 View deployment

vercel Bot deployed to Preview September 10, 2025 23:39 View deployment

vercel Bot deployed to Preview September 11, 2025 04:49 View deployment

mdtro self-assigned this Sep 17, 2025

vercel Bot deployed to Preview September 23, 2025 22:08 View deployment

dcramer added 5 commits October 23, 2025 11:11

tests(oauth_token): pass HTTP_AUTHORIZATION via named kw to satisfy t…

0dad25d

…yping Switch header injection from **dict to explicit HTTP_AUTHORIZATION kw to align with client.post type hints and fix mypy arg-type errors.

dcramer added 9 commits October 23, 2025 11:11

thanks ai

7f8db50

Clean up abstraction

e99254e

improve logging

e3ddb4d

fix expires_in

71a7f39

fix validation

6f5e5c5

dcramer force-pushed the oauth-client-secret-basic branch from 5ae9f6c to 6f5e5c5 Compare October 23, 2025 18:23

vercel Bot deployed to Preview October 23, 2025 18:26 View deployment

dcramer force-pushed the oauth-client-secret-basic branch from 6f5e5c5 to dedbfcd Compare October 23, 2025 19:54

vercel Bot deployed to Preview October 23, 2025 19:56 View deployment

dcramer requested a review from Copilot October 23, 2025 20:29

Copilot AI reviewed Oct 23, 2025

View reviewed changes

vercel Bot deployed to Preview October 23, 2025 20:38 View deployment

BYK reviewed Oct 23, 2025

View reviewed changes

Act on feedback

cdf423f

dcramer force-pushed the oauth-client-secret-basic branch from 246de02 to cdf423f Compare October 23, 2025 21:43

dcramer requested a review from Copilot October 23, 2025 21:43

vercel Bot deployed to Preview October 23, 2025 21:45 View deployment

github-advanced-security AI found potential problems Oct 23, 2025

View reviewed changes

Comment thread src/sentry/web/frontend/oauth_token.py Dismissed

Copilot AI reviewed Oct 23, 2025

BYK approved these changes Oct 24, 2025

View reviewed changes

dcramer merged commit 3c2bc4a into master Oct 24, 2025
69 checks passed

dcramer deleted the oauth-client-secret-basic branch October 24, 2025 16:54

github-actions Bot locked and limited conversation to collaborators Nov 9, 2025

Uh oh!

Conversation

dcramer commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

dcramer commented Sep 10, 2025

Uh oh!

dcramer commented Sep 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

BYK Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

BYK Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcramer commented Oct 23, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dcramer commented Sep 9, 2025 •

edited

Loading

codecov Bot commented Sep 9, 2025 •

edited

Loading