fix(security): validate GitHub user during integration installation - #1
Open
linxia0415 wants to merge 5 commits into
Open
fix(security): validate GitHub user during integration installation#1linxia0415 wants to merge 5 commits into
linxia0415 wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds OAuth user authentication as a dedicated pipeline step before GitHub installation setup. It introduces error handling helpers, validates installation ownership to prevent cross-organization conflicts, and verifies the authenticated GitHub user matches the installation sender. The pipeline advancer is simplified by removing a provider constant. ChangesGitHub OAuth Installation Authentication
Sequence DiagramsequenceDiagram
participant Client
participant OAuthLoginView
participant GitHub API
participant GitHubInstallation
Client->>OAuthLoginView: Setup initiated
OAuthLoginView->>Client: Redirect to GitHub authorize
Client->>GitHub API: OAuth callback with code/state
OAuthLoginView->>GitHub API: Exchange code for access token
GitHub API->>OAuthLoginView: Return access token and user
OAuthLoginView->>GitHubInstallation: Advance to next step with user login
GitHubInstallation->>GitHubInstallation: Validate installation ownership
GitHubInstallation->>GitHubInstallation: Verify OAuth user matches sender
GitHubInstallation->>Client: Complete or return error
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sentry/integrations/github/integration.py`:
- Around line 493-505: The code assumes integration.metadata["sender"]["login"]
exists and will KeyError for older integrations; update the validation in the
block after retrieving Integration (function/class using Integration,
pipeline.fetch_state, error, self.active_organization) to first check that
integration.metadata is a dict and contains "sender" with a mapping that has
"login" (e.g., using .get or explicit key checks), and if missing treat it as an
invalid installation by calling error(request, self.active_organization); only
perform the equality comparison with
pipeline.fetch_state("github_authenticated_user") when the login value is
present.
- Around line 475-490: The check that sets installations_exist currently counts
this installation even when it's linked to self.active_organization; change the
query so it only checks for OrganizationIntegration records owned by a different
org by excluding self.active_organization (e.g., use
OrganizationIntegration.objects.filter(integration=Integration.objects.get(external_id=installation_id)).exclude(organization=self.active_organization).exists()),
keep the Integration.DoesNotExist handling and subsequent
pipeline.next_step()/error(...) behavior the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e099580-597a-42b6-b3c0-44f8c30f934c
📒 Files selected for processing (3)
src/sentry/integrations/github/integration.pysrc/sentry/web/frontend/pipeline_advancer.pytests/sentry/integrations/github/test_integration.py
Comment on lines
475
to
490
| try: | ||
| # We want to limit GitHub integrations to 1 organization | ||
| installations_exist = OrganizationIntegration.objects.filter( | ||
| integration=Integration.objects.get(external_id=request.GET["installation_id"]) | ||
| integration=Integration.objects.get(external_id=installation_id) | ||
| ).exists() | ||
|
|
||
| except Integration.DoesNotExist: | ||
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| return pipeline.next_step() | ||
|
|
||
| if installations_exist: | ||
| document_origin = "document.origin" | ||
| if self.active_organization and features.has( | ||
| "organizations:customer-domains", self.active_organization.organization | ||
| ): | ||
| document_origin = ( | ||
| f'"{generate_organization_url(self.active_organization.organization.slug)}"' | ||
| ) | ||
| return render_to_response( | ||
| "sentry/integrations/github-integration-failed.html", | ||
| context={ | ||
| "error": ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, | ||
| "payload": { | ||
| "success": False, | ||
| "data": {"error": _("Github installed on another Sentry organization.")}, | ||
| }, | ||
| "document_origin": document_origin, | ||
| }, | ||
| request=request, | ||
| return error( | ||
| request, | ||
| self.active_organization, | ||
| error_short="Github installed on another Sentry organization.", | ||
| error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, | ||
| ) |
There was a problem hiding this comment.
Exclude the current organization from the cross-org ownership check.
installations_exist becomes True even when this installation is already linked to self.active_organization, so re-running setup for the same org now returns the “another Sentry organization” error. This should only fail for a different owning org.
Suggested fix
+ active_org_id = self.active_organization.organization.id if self.active_organization else None
try:
# We want to limit GitHub integrations to 1 organization
- installations_exist = OrganizationIntegration.objects.filter(
- integration=Integration.objects.get(external_id=installation_id)
+ integration = Integration.objects.get(
+ external_id=installation_id,
+ status=ObjectStatus.ACTIVE,
+ )
+ installations_exist = OrganizationIntegration.objects.filter(
+ integration=integration
+ ).exclude(
+ organization_id=active_org_id
).exists()
except Integration.DoesNotExist:
return pipeline.next_step()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/integrations/github/integration.py` around lines 475 - 490, The
check that sets installations_exist currently counts this installation even when
it's linked to self.active_organization; change the query so it only checks for
OrganizationIntegration records owned by a different org by excluding
self.active_organization (e.g., use
OrganizationIntegration.objects.filter(integration=Integration.objects.get(external_id=installation_id)).exclude(organization=self.active_organization).exists()),
keep the Integration.DoesNotExist handling and subsequent
pipeline.next_step()/error(...) behavior the same.
Comment on lines
+493
to
+505
| try: | ||
| integration = Integration.objects.get( | ||
| external_id=installation_id, status=ObjectStatus.ACTIVE | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| return error(request, self.active_organization) | ||
|
|
||
| # 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"] | ||
| ): | ||
| return error(request, self.active_organization) |
There was a problem hiding this comment.
Guard missing sender metadata before indexing into it.
Older integrations, or records created without webhook sender data, will raise KeyError on integration.metadata["sender"]["login"] and turn this validation into a 500 instead of the generic invalid-installation response.
Suggested fix
- if (
- pipeline.fetch_state("github_authenticated_user")
- != integration.metadata["sender"]["login"]
- ):
+ sender_login = integration.metadata.get("sender", {}).get("login")
+ if (
+ not sender_login
+ or pipeline.fetch_state("github_authenticated_user") != sender_login
+ ):
return error(request, self.active_organization)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/integrations/github/integration.py` around lines 493 - 505, The
code assumes integration.metadata["sender"]["login"] exists and will KeyError
for older integrations; update the validation in the block after retrieving
Integration (function/class using Integration, pipeline.fetch_state, error,
self.active_organization) to first check that integration.metadata is a dict and
contains "sender" with a mapping that has "login" (e.g., using .get or explicit
key checks), and if missing treat it as an invalid installation by calling
error(request, self.active_organization); only perform the equality comparison
with pipeline.fetch_state("github_authenticated_user") when the login value is
present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
stateparameter supplied by user and pipeline signatureaccess_tokenfrom thecode(wrong or attempt of re-use ofcode).In all those cases, this error is shown:
Summary by CodeRabbit
New Features
Improvements