GitHub OAuth Security Enhancement#8
Conversation
…#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: 
|
@rupakInfinitiBit 👋 I've started reviewing this pull request. I'll post a detailed review once I'm done — this may take a moment. |
🔍 Reviewing PR #8
⏳ Analyzing changes — review incoming shortly. |
![graphbit-prflow-dev[bot]](https://avatars.githubusercontent.com/in/2502123?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Automated PR Review
PR Title: fix(security): validate GitHub user during integration installation
Overall Quality Score: 7/10
Recommendation: REQUEST_CHANGES
🧠 Summary
The PR improves security by validating GitHub user authentication during integration installation, but contains critical metadata validation gaps and security vulnerabilities that need addressing before approval.
⚠️ Critical Issues
- Missing validation for pipeline state consistency - integration.metadata may not contain required 'sender' key before access
- User authentication validation logic has potential for bypass if sender metadata is missing or corrupted
- OAuth flow and user mismatch testing lack coverage for critical edge cases including missing sender metadata scenarios
✅ Key Suggestions
- Replace simple string comparison with
hmac.compare_digest()for state parameter validation to prevent timing attacks - Add comprehensive validation for integration.metadata structure before accessing nested keys
- Implement specific exception handling instead of bare
except Exception:clause - Add OAuth access token format validation to ensure tokens follow expected GitHub patterns
- Expand test coverage to include failure scenarios, edge cases, and security attack vectors
| return error(request, self.active_organization) | ||
|
|
||
| # Check that the authenticated GitHub user is the same as who installed the app. | ||
| if ( |
There was a problem hiding this comment.
HIGH: Missing validation for pipeline state consistency
# 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"]
Suggested change:
+ if 'sender' not in integration.metadata or 'login' not in integration.metadata['sender']: return error(request, self.active_organization)Add validation to ensure integration.metadata contains 'sender' key before accessing: if 'sender' not in integration.metadata or 'login' not in integration.metadata['sender']: return error(request, self.active_organization)
| external_id=installation_id, status=ObjectStatus.ACTIVE | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| return error(request, self.active_organization) |
There was a problem hiding this comment.
HIGH: User authentication validation logic has potential for bypass if sender metadata is missing
)
except Integration.DoesNotExist:
return error(request, self.active_organization)
# Check that the authenticated GitHub user is the same as who installed the app.
Suggested change:
+ if not integration.metadata.get('sender') or not integration.metadata['sender'].get('login'): return error(request, self.active_organization, error_short="Invalid installation metadata.", error_long="Installation metadata is corrupted or missing.")Add comprehensive validation: if not integration.metadata.get('sender') or not integration.metadata['sender'].get('login'): return error(request, self.active_organization, error_short="Invalid installation metadata.", error_long="Installation metadata is corrupted or missing.")
| @@ -218,14 +229,32 @@ def assert_setup_flow(self): | |||
| redirect = urlparse(resp["Location"]) | |||
There was a problem hiding this comment.
HIGH: OAuth flow testing only covers happy path scenarios
Suggestion: Add test cases for OAuth failures: invalid codes, network errors, malformed responses, and state parameter mismatches
| assert b"Invalid installation request." in resp.content | ||
|
|
||
| @responses.activate | ||
| def test_github_user_mismatch(self): |
There was a problem hiding this comment.
HIGH: User mismatch test doesn't cover edge cases like missing sender metadata
Suggested change:
+ def test_github_user_mismatch_missing_sender_metadata():Add test case: def test_github_user_mismatch_missing_sender_metadata(): to test when integration.metadata['sender'] is None or missing 'login' key
![pr-review-agent-rupakinfinitibit[bot]](https://avatars.githubusercontent.com/u/236351362?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔧 Changes Requested
████████░░ 7.5/10 · 3 files reviewed · 7 issues found
Key concerns:
- Overly broad exception handling in OAuth token exchange could mask critical errors
- Unsafe access to integration metadata without proper validation could cause KeyError
- Missing validation for installation_id parameter format and bounds
- Test coverage could be more comprehensive for attack scenarios
📝 Walkthrough (3 files)
| File | Changes | Summary |
|---|---|---|
src/sentry/integrations/github/integration.py |
✏️ +118/-41 | Adds GitHub OAuth2 user validation step before integration installation and refactors error handling to use common helper functions. |
src/sentry/web/frontend/pipeline_advancer.py |
✏️ +4/-7 | Simplified GitHub-specific installation logic by removing the FORWARD_INSTALL_FOR constant and inlining the check. |
tests/sentry/integrations/github/test_integration.py |
✏️ +125/-2 | Updated tests to cover the new OAuth authentication flow and user validation, including comprehensive test cases for user mismatch scenarios. |
🔍 Issues
🟠 src/sentry/integrations/github/integration.py
-
🟠 Line 442 — Overly broad exception handling masks important errors
The bare
except Exception:clause catches all exceptions during token exchange, including network errors, JSON parsing errors, and other important failures. This makes debugging OAuth issues extremely difficult and could hide security-relevant errors.
try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except (UnicodeDecodeError, ValueError, OSError) as e:
logger.warning("GitHub OAuth token exchange failed: %s", e)
payload = {}
-
🟡 Line 500 — Unsafe access to nested metadata without validation
Direct access to
integration.metadata["sender"]["login"]could raise KeyError if the metadata structure is missing or malformed. This could happen if the integration was created through a different flow or if GitHub's webhook payload structure changes.
# Check that the authenticated GitHub user is the same as who installed the app.
authentic_user = pipeline.fetch_state("github_authenticated_user")
installer_login = integration.metadata.get("sender", {}).get("login")
if not installer_login:
logger.warning("Integration %s missing sender metadata", integration.id)
return error(request, self.active_organization)
if authentic_user != installer_login:
return error(request, self.active_organization)
-
🔵 Line 447 — Missing validation for installation_id parameter
The installation_id from the request is not validated for format or reasonable bounds before being used in database queries and external API calls.
installation_id = request.GET.get(
"installation_id", pipeline.fetch_state("installation_id")
)
if installation_id is None:
return self.redirect(self.get_app_url())
# Validate installation_id format
if not installation_id.isdigit() or len(installation_id) > 20:
return error(request, self.active_organization)
pipeline.bind_state("installation_id", installation_id)
-
🔵 Line 446 — Inconsistent error handling between OAuth and installation views
The OAuthLoginView and GitHubInstallation views handle errors differently - OAuth view calls error() helper while installation view previously had inline error handling. Consider standardizing the approach.
🔵 src/sentry/web/frontend/pipeline_advancer.py
-
🔵 Line 40 — Hardcoded provider check could be more maintainable
The hardcoded check for 'github' could be made more maintainable by using a provider-based approach or constant, especially if other integrations need similar behavior.
if (
provider_id == GitHubIntegrationProvider.key
and request.GET.get("setup_action") == "install"
and pipeline is None
):
🟡 tests/sentry/integrations/github/test_integration.py
-
ℹ️ Line 118 — Good test coverage for OAuth flow
The test updates properly cover the new OAuth flow by stubbing the necessary GitHub OAuth endpoints and validating the complete flow.
-
🟡 Line 399 — User mismatch test could be more comprehensive
The test_github_user_mismatch test creates a webhook event with an 'attacker' user but doesn't verify that the original authenticated user differs from the webhook sender. Consider making the test more explicit about the attack scenario.
💡 Suggestion: # Before creating webhook with attacker, verify the original authenticated user
original_integration = Integration.objects.get(external_id=self.installation_id)
original_sender = original_integration.metadata.get('sender', {}).get('login', 'octocat')
assert original_sender != 'attacker', 'Test setup should use different users'
webhook_event = json.loads(INSTALLATION_EVENT_EXAMPLE)
✨ Strengths
- ✅ Addresses important security vulnerability by validating GitHub user identity
- ✅ Clean refactoring of error handling into reusable helper functions
- ✅ Comprehensive test coverage for the new OAuth flow and edge cases
🤖 PRFlow · 🔴 Critical 🟠 Important 🟡 Suggestion 🔵 Minor ℹ️ Note
| except Integration.DoesNotExist: | ||
| return error(request, self.active_organization) | ||
|
|
||
| # Check that the authenticated GitHub user is the same as who installed the app. |
There was a problem hiding this comment.
🔧 Improvement | 🟡 Suggestion
Unsafe access to nested metadata without validation
Direct access to integration.metadata["sender"]["login"] could raise KeyError if the metadata structure is missing or malformed. This could happen if the integration was created through a different flow or if GitHub's webhook payload structure changes.
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| authentic_user = pipeline.fetch_state("github_authenticated_user") | |
| installer_login = integration.metadata.get("sender", {}).get("login") | |
| if not installer_login: | |
| logger.warning("Integration %s missing sender metadata", integration.id) | |
| return error(request, self.active_organization) | |
| if authentic_user != installer_login: | |
| return error(request, self.active_organization) |
🤖 Prompt for AI Agents
In `@src/sentry/integrations/github/integration.py` around line 500, Direct access to `integration.metadata["sender"]["login"]` could raise KeyError if the metadata structure is missing or malformed. This could happen if the integration was created through a different flow or if GitHub's webhook payload structure changes.; apply the proposed fix to resolve this validation.
| assert b"Invalid installation request." in resp.content | ||
|
|
||
| @responses.activate | ||
| def test_github_user_mismatch(self): |
There was a problem hiding this comment.
🔧 Improvement | 🟡 Suggestion
User mismatch test could be more comprehensive
The test_github_user_mismatch test creates a webhook event with an 'attacker' user but doesn't verify that the original authenticated user differs from the webhook sender. Consider making the test more explicit about the attack scenario.
💡 Suggestion: # Before creating webhook with attacker, verify the original authenticated user
original_integration = Integration.objects.get(external_id=self.installation_id)
original_sender = original_integration.metadata.get('sender', {}).get('login', 'octocat')
assert original_sender != 'attacker', 'Test setup should use different users'
webhook_event = json.loads(INSTALLATION_EVENT_EXAMPLE)
🤖 Prompt for AI Agents
In `@tests/sentry/integrations/github/test_integration.py` around line 399, The test_github_user_mismatch test creates a webhook event with an 'attacker' user but doesn't verify that the original authenticated user differs from the webhook sender. Consider making the test more explicit about the attack scenario.; apply the proposed fix to resolve this testing.
rupakInfinitiBit
changed the title
Co-authored-by: pr-review-agent-rupakinfinitibit[bot] <240878464+pr-review-agent-rupakinfinitibit[bot]@users.noreply.github.com>
|
@rupakInfinitiBit 👋 I've started reviewing this pull request. I'll post a detailed review once I'm done — this may take a moment. |
There was a problem hiding this comment.
🔍 Automated PR Review
PR Title: GitHub OAuth Security Enhancement
Overall Quality Score: 6/10
Recommendation: REQUEST_CHANGES
🧠 Summary
The code contains multiple structural issues including malformed class definitions and incorrect indentation that prevent proper execution, alongside some security improvements for OAuth token handling.
⚠️ Critical Issues
- Syntax error with malformed class definition structure and orphaned try-except block at line 442
- Missing class declaration for GitHubInstallation at line 450 - needs proper class definition:
class GitHubInstallation(PipelineView): - Method definition
get_app_urlexists outside of class scope at line 450 and must be moved inside the GitHubInstallation class with correct indentation
✅ Key Suggestions
- Fix the structural issues by properly defining the GitHubInstallation class and moving methods into appropriate class scope
- Improve error logging security by removing sensitive details from OAuth token exchange failure logs
- Add validation for GitHub authenticated user in pipeline state to prevent null/empty comparisons
- Replace broad exception handling with more specific exception types for better error handling
| return pipeline.next_step() | ||
|
|
||
|
|
||
| try: |
There was a problem hiding this comment.
CRITICAL: Syntax error - malformed class definition structure with orphaned try-except block
Suggestion: The try-except block appears to be incorrectly placed. It should either be part of the OAuthLoginView class or moved to the appropriate location within GitHubInstallation class
| payload = {} | ||
| def get_app_url(self) -> str: | ||
| name = options.get("github-app.name") | ||
| return f"https://github.com/apps/{slugify(name)}" |
There was a problem hiding this comment.
CRITICAL: Indentation inconsistency and missing class declaration for GitHubInstallation
Suggested change:
+ class GitHubInstallation(PipelineView):Add proper class declaration: class GitHubInstallation(PipelineView):
🔍 Reviewing PR #8
⏳ Analyzing changes — review incoming shortly. |
![graphbit-prflow[bot]](https://avatars.githubusercontent.com/in/2396293?s=60&v=4){kind=small}
There was a problem hiding this comment.
💬 Review Complete
███████░░░ 7.0/10 · 0 files reviewed · 0 issues found
Key concerns:
- Automated review failed: Single '}' encountered in format string
📝 Walkthrough (3 files)
| File | Summary | |
|---|---|---|
src/sentry/integrations/github/integration.py |
✏️ +124/-42 | Review unavailable due to service error. |
src/sentry/web/frontend/pipeline_advancer.py |
✏️ +4/-7 | Review unavailable due to service error. |
tests/sentry/integrations/github/test_integration.py |
✏️ +125/-2 | Review unavailable due to service error. |
✅ No Issues Found
Looks good — no actionable issues detected.
🤖 PRFlow · 🔴 Critical 🟠 Important 🟡 Suggestion 🔵 Minor ℹ️ Note
🔍 Reviewing PR #8
⏳ Analyzing changes — review incoming shortly. |
There was a problem hiding this comment.
💬 Review Complete
███████░░░ 7.0/10 · 0 files reviewed · 0 issues found
Key concerns:
- Automated review failed: Single '}' encountered in format string
📝 Walkthrough (3 files)
| File | Summary | |
|---|---|---|
src/sentry/integrations/github/integration.py |
✏️ +124/-42 | Review unavailable due to service error. |
src/sentry/web/frontend/pipeline_advancer.py |
✏️ +4/-7 | Review unavailable due to service error. |
tests/sentry/integrations/github/test_integration.py |
✏️ +125/-2 | Review unavailable due to service error. |
✅ No Issues Found
Looks good — no actionable issues detected.
🤖 PRFlow · 🔴 Critical 🟠 Important 🟡 Suggestion 🔵 Minor ℹ️ Note
3 participants
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.