Skip to content

GitHub OAuth Security Enhancement#12

Open
akshayutture-augment wants to merge 1 commit into
oauth-state-vulnerablefrom
oauth-state-secure
Open

GitHub OAuth Security Enhancement#12
akshayutture-augment wants to merge 1 commit into
oauth-state-vulnerablefrom
oauth-state-secure

Conversation

@akshayutture-augment

@akshayutture-augment akshayutture-augment commented Nov 17, 2025

Copy link
Copy Markdown

No description provided.

…#67876)

We're adding one more step in the GitHub integration installation
pipeline, namely GitHub OAuth2 authorize. This is transparent from the
UX perspective as the data exchange happens without user interaction.

The pipeline will now fail in these cases:
- If there is a mismatch between currently authenticated GitHub user
(derived from OAuth2 authorize step) and the user who installed the
GitHub app (https://github.com/apps/sentry-io)
- If there is a mismatch between `state` parameter supplied by user and
pipeline signature
- If GitHub could not generate correct `access_token` from the `code`
(wrong or attempt of re-use of `code`).

In all those cases, this error is shown:

![image](https://github.com/getsentry/sentry/assets/1127549/18923861-2ead-4cf5-adda-7738aef801d7)

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

# Check that the authenticated GitHub user is the same as who installed the app.
if (
pipeline.fetch_state("github_authenticated_user")
!= integration.metadata["sender"]["login"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Directly indexing integration.metadata["sender"]["login"] can raise a KeyError if sender or login is absent (e.g., race conditions or unexpected payload), which would 500 the setup flow. Consider guarding for missing keys and returning the same error path to fail gracefully (also applies to other locations in the PR).

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants