Skip to content

fix(sentry-app): Adds better validation for invalid token request bodies - #80289

Merged
GabeVillalobos merged 3 commits into
masterfrom
gv/fix-refresh-token-misising-in-endpoint
Nov 5, 2024
Merged

GabeVillalobos merged 3 commits into
masterfrom
gv/fix-refresh-token-misising-in-endpoint

Conversation

@GabeVillalobos

Copy link
Copy Markdown
Member

Fixes SENTRY-3HBC

Adds serializers for both token Authorization and Refresh flows.

This affects POST requests to the /sentry-app-installations/<uuid>/authorization/ endpoint, providing explicit 400 status codes with invalid field information when a request body is misconfigured.

Additional Context

Some fields in the request body (such as client_id and secret), are validated by a a separate Authentication flow prior to the defined serializer code, resulting in a 401 exception instead, hence the tests asserting for both cases.

@GabeVillalobos
GabeVillalobos requested a review from a team as a code owner November 5, 2024 21:57
@GabeVillalobos
GabeVillalobos requested a review from a team November 5, 2024 21:57
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 5, 2024
@Christinarlong

Christinarlong commented Nov 5, 2024 •

Copy link
Copy Markdown
Contributor

curious Q: How does this fix SENTRY-3HBC ? Were we passing in code for the refresh_token field ? Or an empty field? So the query ApiToken.get(refresh_token="") would return inf. tokens?

Comment thread src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py

@markstory markstory left a comment

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.

Looks good to me. I tried locally with empty string and the response codes were correct as well.

@GabeVillalobos
GabeVillalobos enabled auto-merge (squash) November 5, 2024 22:33
@ameliahsu

Copy link
Copy Markdown
Contributor

@Christinarlong Yup, no refresh_token was being passed in so ApiToken.objects.get(refresh_token=self.refresh_token) was returning like 200k+ tokens 😳

@GabeVillalobos
GabeVillalobos enabled auto-merge (squash) November 5, 2024 22:55
@GabeVillalobos
GabeVillalobos merged commit 8a52d39 into master Nov 5, 2024
@GabeVillalobos
GabeVillalobos deleted the gv/fix-refresh-token-misising-in-endpoint branch November 5, 2024 23:28
jan-auer added a commit that referenced this pull request Nov 6, 2024
* master: (67 commits)
  feat(dynamic-sampling): Sampling breakdown (#80304)
  feat(profiling): add organizations:continuous-profiling to the list of exposable features (#80236)
  chore(broadcasts): remove cta column from broadcast model (#80201)
  feat(dynamic-sampling): Use sample rates endpoint (#80235)
  feat(issues): Rearrange all events columns, sizes (#80296)
  fix(issues): All event table pagination counts (#80297)
  fix(issues): Preserve query parameters on all events close (#80295)
  feat(issues): Hide "comment" button until focused (#80283)
  fix(sentry-app): Adds better validation for invalid token request bodies (#80289)
  feat(workflow_engine): Add in hook for producing occurrences from the stateful detector (#80168)
  feat(issue summary) New structured issue summary design (#80273)
  feat(workflow_engine): Return status change messages when a stateful detector resolves (#80122)
  feat(insights): Add insights query date range footer hook (#80276)
  ref(crons): Switch to cronsim in sample data generator (#80278)
  feat(issue-details): Hide merged/similar issues for non-error issues (#80284)
  feat(issue summary) Update issue summary model (#80270)
  feat(crons): Add cronsim behind an option (#80271)
  fix(anomaly detection): add alerts analytics reqs to utils/analytics.tsx (#80281)
  feat(trace-explorer): Sort traces by timestamp in EAP (#80274)
  feat(workflow_engine): Implement basic evaluation in `DataCondition` (#80118)
  ...
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 21, 2024

This branch was successfully deployed

1 active deployment
Preview — 274fa2f2 Deployed Nov 5, 2024 by vercel[bot]
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.

5 participants