Skip to content

GitHub OAuth Security Enhancement#4

Open
everettbu wants to merge 1 commit intooauth-state-vulnerablefrom
oauth-state-secure
Open

GitHub OAuth Security Enhancement#4
everettbu wants to merge 1 commit intooauth-state-vulnerablefrom
oauth-state-secure

Conversation

@everettbu
Copy link
Contributor

@everettbu everettbu commented Jul 29, 2025

Test 4

…#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)
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements critical security enhancements to the GitHub OAuth integration flow, addressing vulnerabilities in state validation and user authentication. The changes span three key files:

Core Security Implementation (src/sentry/integrations/github/integration.py):

  • Introduces a new OAuthLoginView class that implements proper OAuth 2.0 flow with CSRF protection
  • Adds state parameter validation by comparing request.GET.get('state') with pipeline.signature to prevent OAuth state fixation attacks
  • Implements user verification by ensuring the authenticated GitHub user matches the app installer (pipeline.fetch_state('github_authenticated_user') != integration.metadata['sender']['login'])
  • Refactors error handling with a centralized error() function for consistent security responses

Pipeline Logic Simplification (src/sentry/web/frontend/pipeline_advancer.py):

  • Removes the global FORWARD_INSTALL_FOR constant and inlines the GitHub provider check directly
  • Maintains the same functional behavior for redirecting GitHub installations that bypass the normal pipeline
  • Improves code readability by moving comments closer to the relevant logic

Test Coverage Enhancement (tests/sentry/integrations/github/test_integration.py):

  • Adds comprehensive test coverage for the OAuth access token exchange process
  • Implements state parameter validation testing in OAuth callbacks
  • Introduces user mismatch detection tests to verify installation hijacking prevention
  • Updates existing tests to properly simulate the complete OAuth flow with security validations

These changes transform a vulnerable OAuth implementation into a secure flow that follows OAuth 2.0 best practices. The modifications integrate seamlessly with Sentry's existing pipeline architecture while adding essential security boundaries to prevent CSRF attacks and unauthorized integration installations.

Confidence score: 4/5

  • This PR significantly improves security by implementing proper OAuth state validation and user verification, making it much safer to merge
  • The confidence score reflects that while the security improvements are solid, the changes touch critical authentication flows that require careful testing
  • src/sentry/integrations/github/integration.py needs the most attention due to the new OAuth flow implementation and security checks

3 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

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