Skip to content

feat(oauth): Implement PKCE (S256) and enforce redirect_uri binding #99452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dcramer wants to merge 8 commits into from
Closed

feat(oauth): Implement PKCE (S256) and enforce redirect_uri binding #99452

dcramer wants to merge 8 commits into from

Conversation

@dcramer
Copy link
Member

@dcramer dcramer commented Sep 14, 2025

RFCs:

  • RFC 7636 (PKCE): §4.1/§4.2/§4.3/§4.4/§4.6
  • RFC 6749 (OAuth 2.0): §4.1.3 (redirect_uri binding)
  • OAuth 2.1 draft (draft-ietf-oauth-v2-1-13)

Changes:

  • Model/migration: add ApiGrant.code_challenge + code_challenge_method (nullable)
  • Authorization: accept + persist code_challenge(+_method); default S256; deny plain by default
  • Token: require code_verifier when grant has challenge; length/charset per RFC 7636; S256 verification; optional plain behind flag
  • Token: enforce redirect_uri equality when stored on grant (no fallback)
  • Tests: S256 happy path; missing/mismatch/length/charset; optional plain; redirect binding

Notes:

  • Requirements locked behind version=1
  • Local constants gate plain method; discovery metadata excluded (separate task)
  • No implicit flow changes; non-cacheable responses preserved

Refs: #99002

@dcramer
feat(oauth): Implement PKCE (S256) and enforce redirect_uri binding
48c7279 
RFCs:
- RFC 7636 (PKCE): §4.1/§4.2/§4.3/§4.4/§4.6
- RFC 6749 (OAuth 2.0): §4.1.3 (redirect_uri binding)
- OAuth 2.1 draft (draft-ietf-oauth-v2-1-13)

Changes:
- Model/migration: add ApiGrant.code_challenge + code_challenge_method (nullable)
- Authorization: accept + persist code_challenge(+_method); default S256; deny plain by default
- Token: require code_verifier when grant has challenge; length/charset per RFC 7636; S256 verification; optional plain behind flag
- Token: enforce redirect_uri equality when stored on grant (no fallback)
- Tests: S256 happy path; missing/mismatch/length/charset; optional plain; redirect binding

Notes:
- Local constants gate plain method; discovery metadata excluded (separate task)
- No implicit flow changes; non-cacheable responses preserved

Refs: #99002
@dcramer
ref(oauth): Version-gated legacy behavior uses logger warnings, not m…
cf9375b 
…etrics

- v0: log warnings when redirect_uri fallback or missing PKCE verifier are used
- v1: unchanged strict behavior

RFCs: RFC 7636; RFC 6749 §4.1.3

Refs: #99002
@dcramer dcramer requested review from a team as code owners September 14, 2025 18:25
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 14, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0981_apigrant_pkce_fields.py

for 0981_apigrant_pkce_fields in sentry

--
-- Add field code_challenge to apigrant
--
ALTER TABLE "sentry_apigrant" ADD COLUMN "code_challenge" varchar(128) NULL;
--
-- Add field code_challenge_method to apigrant
--
ALTER TABLE "sentry_apigrant" ADD COLUMN "code_challenge_method" varchar(10) NULL;

cursor[bot]

This comment was marked as outdated.

Sign in to view

@dcramer
ref(oauth): Remove PKCE_ALLOW_PLAIN; gate plain via ApiApplication.ve…
2188991 
…rsion

- v1: reject code_challenge_method=plain at authorize and token
- v0: allow plain for compatibility
- Update tests to switch app version for plain success path

RFCs: RFC 7636 (PKCE); RFC 6749 §4.1.3

Refs: #99002
@codecov
Copy link

codecov bot commented Sep 14, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 82.35294% with 12 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 84.31% 8 Missing ⚠️
src/sentry/web/frontend/oauth_authorize.py 73.33% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #99452      +/-   ##
==========================================
- Coverage   81.32%   81.21%   -0.11%     
==========================================
  Files        8590     8584       -6     
  Lines      382261   380205    -2056     
  Branches    24106    24106              
==========================================
- Hits       310864   308778    -2086     
- Misses      71051    71081      +30     
  Partials      346      346

@dcramer
Copy link
Member Author

dcramer commented Sep 14, 2025

This one will need a bit more thorough of a review. 2.1 changes the hard requirements so once again we're not enforcing these til version=1.

Will give it another pass myself

mdtro
mdtro reviewed Sep 15, 2025
View reviewed changes
@mdtro mdtro self-assigned this Sep 17, 2025
mdtro
mdtro reviewed Sep 18, 2025
View reviewed changes
mdtro
mdtro reviewed Sep 18, 2025
View reviewed changes
cursor[bot]

This comment was marked as outdated.

Sign in to view

cursor[bot]

This comment was marked as outdated.

Sign in to view

@getsantry getsantry bot added the Stale label Nov 5, 2025
@getsantry getsantry bot closed this Nov 13, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2025
@dcramer dcramer reopened this Dec 4, 2025
@dcramer dcramer removed the Stale label Dec 4, 2025
@dcramer
Copy link
Member Author

dcramer commented Dec 4, 2025

I'm going to redo this patch from scratch given the model changes, but working on this now.

@dcramer dcramer closed this Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

+1 more reviewer

@mdtro mdtro mdtro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mdtro mdtro

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dcramer @mdtro