Skip to content

feat(oauth): Add strict redirect URI validation mode#99003

Merged
dcramer merged 7 commits into
masterfrom
oauth-redirect-uri-exact
Sep 11, 2025
Merged

feat(oauth): Add strict redirect URI validation mode#99003
dcramer merged 7 commits into
masterfrom
oauth-redirect-uri-exact

Conversation

@dcramer

@dcramer dcramer commented Sep 6, 2025

Copy link
Copy Markdown
Member

Introduces version-gated exact redirect URI matching per OAuth 2.0 spec.

  • Exact match required for version 1+ apps (RFC 6749 §3.1.2.3)
  • Loopback ephemeral port exception for native apps (RFC 8252)
  • Legacy prefix-matching preserved for version 0 apps

Refs:

Related: #99002

@dcramer dcramer requested a review from a team as a code owner September 6, 2025 22:56
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 6, 2025
@github-actions

github-actions Bot commented Sep 6, 2025

Copy link
Copy Markdown
Contributor

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

for 0974_add_allow_redirect_prefix_match in sentry

--
-- Add field allow_redirect_prefix_match to apiapplication
--
ALTER TABLE "sentry_apiapplication" ADD COLUMN "allow_redirect_prefix_match" boolean DEFAULT false NOT NULL;
--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL

@dcramer

dcramer commented Sep 6, 2025

Copy link
Copy Markdown
Member Author

Open to changing the migration here, and while it has the chance to cause a minor blip for some folks, its hugely unlikely and saves us from having to make sure any paths that update the apps are also updated to write the actual-default

@dcramer

dcramer commented Sep 6, 2025

Copy link
Copy Markdown
Member Author

oh one other open q: can you use a diff db_default vs default and have it do what you might want here? e.g. db_default=True, default=False, and have new entries created via the app be inverted? I dont recall how all this works

aside this table should be tiny which is why i didnt bother with nullable ddl

@codecov

codecov Bot commented Sep 6, 2025

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
src/sentry/models/apiapplication.py 83.87% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #99003      +/-   ##
==========================================
- Coverage   81.29%   81.28%   -0.01%     
==========================================
  Files        8545     8545              
  Lines      377549   377688     +139     
  Branches    23999    23999              
==========================================
+ Hits       306923   307003      +80     
- Misses      70275    70334      +59     
  Partials      351      351              

@dcramer

dcramer commented Sep 7, 2025

Copy link
Copy Markdown
Member Author

I'm actually considering just adding a versioning flag to the api application, as theres going to be a ton of minor incompatible changes and we wont want flags for each one. This one is kind of ok, and may be one of the few that has real consequences, but for example theres things like 'implicit' grants being deprecated etc.

@dcramer

dcramer commented Sep 9, 2025

Copy link
Copy Markdown
Member Author

I'm going to swap to a version field - otherwise we need to add a bitflag here and id pref to avoid that unless really needed. We'll then give folks a time window (if needed) and deprecate the legacy applications (force upgrade them).

Will keep the logging pattern for any legacy/future-unsupported behavior.

#99002 (comment)

@github-actions

github-actions Bot commented Sep 9, 2025

Copy link
Copy Markdown
Contributor

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

for 0976_add_apiapplication_version in sentry

--
-- Add field version to apiapplication
--
ALTER TABLE "sentry_apiapplication" ADD COLUMN "version" integer DEFAULT 0 NOT NULL CHECK ("version" >= 0);
CREATE INDEX CONCURRENTLY "sentry_apiapplication_version_aeec73fd" ON "sentry_apiapplication" ("version");

@dcramer

dcramer commented Sep 10, 2025

Copy link
Copy Markdown
Member Author

@oioki oof ancient PR - if theres any others your aware of, or low hanging fruit that just couldnt get pushed through lmk happy to help

@dcramer dcramer requested a review from leedongwei September 10, 2025 23:40
Add `allow_redirect_prefix_match` field to ApiApplication model to control redirect URI validation behavior. When False (default for new apps), redirect URIs must match exactly. When True (migrated existing apps), legacy prefix matching is allowed.

This improves OAuth security by preventing redirect URI manipulation attacks while maintaining backward compatibility for existing applications.

Fixes #99001
@github-actions

Copy link
Copy Markdown
Contributor

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

for 0979_add_apiapplication_version in sentry

--
-- Add field version to apiapplication
--
ALTER TABLE "sentry_apiapplication" ADD COLUMN "version" integer DEFAULT 0 NOT NULL CHECK ("version" >= 0);
CREATE INDEX CONCURRENTLY "sentry_apiapplication_version_aeec73fd" ON "sentry_apiapplication" ("version");

Comment thread src/sentry/models/apiapplication.py Outdated
Comment thread src/sentry/models/apiapplication.py Outdated
# defaults accordingly.
version = BoundedPositiveIntegerField(
default=0,
db_index=True,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we be querying on version directly, or on some compound key?

# default and db_default to 1 and add a migration to update the field
# defaults accordingly.
version = BoundedPositiveIntegerField(
default=0,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to set both default and db_default. I'd just go with db_default here.

@dcramer

dcramer commented Sep 11, 2025

Copy link
Copy Markdown
Member Author

🙏 ty @mdtro

@dcramer dcramer merged commit 4890c8c into master Sep 11, 2025
64 checks passed
@dcramer dcramer deleted the oauth-redirect-uri-exact branch September 11, 2025 21:18
@dcramer

dcramer commented Sep 11, 2025

Copy link
Copy Markdown
Member Author

just realzed after merging, but lets come back and delete the index later - i doubt we really need it so its a waste of memory

can do it as part of the final migration where we update the versions

@github-actions github-actions Bot locked and limited conversation to collaborators Sep 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants