GitHub OAuth Security Enhancement - #3
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: GitHub OAuth Security Enhancement
Problem
This PR prevents GitHub App installation hijacking — where an attacker links their own GitHub App installation into a victim's Sentry organization — by adding an OAuth login step that authenticates the GitHub user before the installation is linked.
Solution Reviewed
A new OAuthLoginView pipeline step is inserted before GitHubInstallation. It redirects the user to GitHub OAuth, exchanges the code for an access token, fetches the user's GitHub login via get_user_info, and binds it to pipeline state as github_authenticated_user. GitHubInstallation then checks that this authenticated user matches integration.metadata["sender"]["login"] (set by the installation webhook) before proceeding. The FORWARD_INSTALL_FOR constant in pipeline_advancer.py is inlined to a provider_id == "github" check (behavior-preserving — the list only contained "github").
Summary
The security approach is sound in concept but has two blocking defects: the OAuth state parameter is a constant (not a per-session nonce), making the CSRF protection ineffective; and the sender check can raise an unhandled KeyError when sender metadata is absent. The OAuth round-trip also reorders the pipeline so ensure_integration can overwrite the webhook's sender metadata, undermining both the new check and the existing installation.py endpoint. Several error-handling gaps and test coverage issues need addressing before merge.
Files Reviewed
src/sentry/integrations/github/integration.py— deeply reviewedsrc/sentry/web/frontend/pipeline_advancer.py— deeply reviewed (no issues; inlining is behavior-preserving)tests/sentry/integrations/github/test_integration.py— deeply reviewed- ~34
.github/workflows/files — skipped: benchmark scaffolding per the PR description ("Benchmark PR recreated from getsentry#67876"). All are workflow file deletions with no source code impact.
Verification
- No Python virtualenv available in this environment; tests were not run locally. Analysis was performed by reading source files in the worktree and verifying behavior against the codebase (e.g.,
sentry.utils.jsonuses compact separators,pipeline.signaturecomputation,ensure_integrationmetadata update semantics,installation.pysender guard). - The webhook signature in
test_github_user_mismatchwas verified as valid:sentry.utils.json.dumpsusesseparators=(",", ":")by default, so the hardcoded HMAC-SHA1 matches the transmitted body.
Verdict
Request changes before merge. Two blocking security/correctness issues must be fixed: the constant OAuth state and the unguarded sender metadata access. The ensure_integration metadata overwrite is the root cause that should be addressed alongside them.
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
||
| if not request.GET.get("state"): | ||
| state = pipeline.signature |
There was a problem hiding this comment.
🔴 Blocking — OAuth state is a constant, not a per-session CSRF nonce
state = pipeline.signature where pipeline.signature is md5(*[f"{v.__module__}.{v.__name__}" for v in pipeline_views]).hexdigest() — a fixed, publicly-computable hash of the pipeline view class names (9cae5e88803f35ed7970fc131e6e65d3), identical for every user, session, and organization (the tests hardcode it as a constant).
The PR adds this as the OAuth state CSRF parameter. Since it's a public constant, the state check on the callback (if request.GET.get("state") != pipeline.signature) provides no per-session CSRF binding. An attacker who knows the constant can craft a callback URL with their own GitHub OAuth code and installation_id. If a victim has an active in-progress GitHub install pipeline (resolved via session → Redis), the victim's pipeline redeems the attacker's code (binding github_authenticated_user to the attacker's login) and binds the attacker's installation_id to pipeline state.
The sender check does not close this hole: the attacker controls both github_authenticated_user (their code) and sender (their own GitHub App install webhook), so they match. The check also doesn't run on fresh installs (DoesNotExist path at line 482).
Fix: Use a cryptographically random per-flow nonce: pipeline.bind_state("oauth_state", uuid4().hex) at authorize time, and compare that on callback instead of pipeline.signature.
| # 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"] |
There was a problem hiding this comment.
🔴 Blocking — KeyError when sender is absent from integration metadata
integration.metadata["sender"]["login"] is accessed without first checking that "sender" is present in metadata. The sender key is only set by the installation created webhook (webhook.py:204); pipeline-created integrations never carry it (build_integration at line 376 only sets sender when state.get("sender") is truthy, and pipeline state never has sender — confirmed by test_basic_flow which asserts post-install metadata has no sender).
The sibling endpoint installation.py:44 performs the exact same lookup and explicitly guards: if "sender" not in integration.metadata: return HttpResponse(status=404). That guard proves ACTIVE integrations with no OrganizationIntegration and no sender are a real, recognized state — e.g. a prior ensure_integration that replaced metadata (see comment on line 344), or a legacy integration created before the webhook sender feature.
For any such integration reaching this path, the new code raises an unhandled KeyError → HTTP 500 instead of the intended error() page. The security check also fails-open to a crash rather than a deliberate block.
Fix: Mirror installation.py:44 — use integration.metadata.get("sender", {}).get("login") and treat None as a mismatch, returning error().
|
|
||
| def get_pipeline_views(self) -> Sequence[PipelineView]: | ||
| return [GitHubInstallation()] | ||
| return [OAuthLoginView(), GitHubInstallation()] |
There was a problem hiding this comment.
🟡 Non-blocking — OAuth step reorders pipeline, letting ensure_integration overwrite the webhook's sender metadata
Pre-PR, get_pipeline_views() returned only [GitHubInstallation()], so finish_pipeline (and its ensure_integration) typically ran before the webhook, creating the Integration without sender; the late webhook then updated metadata to add sender.
Adding OAuthLoginView first inserts a multi-second OAuth authorize/callback round-trip before GitHubInstallation, which reliably delays finish_pipeline until after the webhook has created the Integration with sender. Now ensure_integration's get_or_create finds the existing row (created=False) and calls integration.update(metadata={...without sender...}) — a full-field replace (not a key merge) that drops sender.
This is the root cause of the KeyError on line 503 and also regresses the installation.py sender-exposure feature (which 404s when sender is absent).
Fix: Preserve sender in build_integration/ensure_integration (copy the existing metadata["sender"] into the update) or make ensure_integration merge metadata instead of replacing it.
| } | ||
|
|
||
| # similar to OAuth2CallbackView.exchange_token | ||
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) |
There was a problem hiding this comment.
🟡 Non-blocking — Uncaught exceptions from safe_urlopen and get_user_info produce 500s
The try/except at lines 425–429 wraps only safe_urlread(req).decode() and dict(parse_qsl(body)). It does not cover:
safe_urlopen(...)(line 423) — raises on connection/timeout errorsget_user_info(payload["access_token"])(line 434) — callsresp.raise_for_status()which raisesrequests.HTTPErroron any non-2xx (e.g. GitHub rate-limit 429, transient 5xx on/user), andresp.json()raises on non-JSON bodies
Each propagates as an unhandled exception → HTTP 500, bypassing the error() page the view deliberately returns for every adjacent failure mode (missing access_token, missing login, state mismatch, Integration.DoesNotExist).
Additionally, the except Exception: payload = {} silently swallows all token-exchange failures with no logging or Sentry event. The canonical OAuth2CallbackView._exchange_token (identity/oauth2.py:347-362) calls raise_for_status() and routes failures through sentry_sdk.capture_exception.
Fix: Broaden the try/except to cover both safe_urlopen and get_user_info. Add raise_for_status() on the token exchange response and sentry_sdk.capture_exception on failure so transient outages are distinguishable from genuine auth failures.
| 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.
🟡 Non-blocking — TOCTOU: sender check and integration creation are not atomic
The sender check here (step 1) reads integration.metadata["sender"]["login"] against pipeline.fetch_state("github_authenticated_user"), then calls next_step() to reach finish_pipeline (step 2), which independently re-fetches the Integration via ensure_integration and creates the OrganizationIntegration via add_organization. These are separate HTTP requests/DB operations with no enclosing transaction.
A concurrent webhook (installation deleted → status=DISABLED, or a re-created install altering sender) between the check and finish_pipeline can mutate the Integration's status/metadata. ensure_integration then unconditionally re-sets status=ACTIVE and replaces metadata without re-validating that the authenticated user still matches the installer.
Fix: Re-validate sender inside the same transaction as the OrganizationIntegration creation, or bind the org-integration only if the validated Integration row is the one being linked.
| github_client_id = ghip.get_oauth_client_id() | ||
| github_client_secret = ghip.get_oauth_client_secret() | ||
|
|
||
| installation_id = request.GET.get("installation_id") |
There was a problem hiding this comment.
🟡 Non-blocking — Pipeline state leaks across simultaneous installation flows in the same session
Pipeline state (installation_id, github_authenticated_user) is stored in a single RedisSessionStore keyed by request.session["store:integration_pipeline"] — one key per session, not per-flow. OrganizationIntegrationSetupView.handle calls pipeline.initialize() → regenerate() on every entry, which writes a new redis_key into that session slot, orphaning any in-flight flow's state.
A user who opens a second GitHub install flow (a second tab) while the first is mid-flight causes the first flow's next request to resolve redis_key to the second flow's store and read the second flow's installation_id/github_authenticated_user. No privilege boundary is crossed (same authenticated user), but the wrong installation_id can be bound/installed for an organization.
Fix: Scope pipeline state per-flow (a per-flow pipeline id in the session key / OAuth state) or reject overlapping flows.
| "{}?{}".format( | ||
| self.setup_path, | ||
| urlencode( | ||
| {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"} |
There was a problem hiding this comment.
🟡 Non-blocking — test_installation_not_found is mislabeled; tests state mismatch, not installation-not-found
This test stubs GET /app/installations/{id} to 404 (line 381), implying it tests the GitHub-installation-not-found case. But the second GET sends state=ddd023d87a913d5226e2a882c4c4cc05, which is NOT the real pipeline.signature (9cae5e88803f35ed7970fc131e6e65d3). OAuthLoginView.dispatch returns the "Invalid installation request." error on the state mismatch before token exchange or GitHubInstallation run, so the 404 mock is never hit (dead stub).
The test actually exercises the OAuth state-mismatch path; the genuine installation-not-found scenario is no longer covered, and there is no clearly-named state-mismatch test.
Fix: Use the correct state value so the test reaches GitHubInstallation, and add a separate test for the OAuth state-mismatch path.
| assert b"Invalid installation request." in resp.content | ||
|
|
||
| @responses.activate | ||
| def test_github_user_mismatch(self): |
There was a problem hiding this comment.
🟡 Non-blocking — Missing test coverage for OAuth failure branches and missing-sender case
test_github_user_mismatch correctly exercises the sender-mismatch path (the webhook signature is valid — sentry.utils.json.dumps uses compact (",", ":") separators by default). However, several new failure branches have no tests:
- (a) missing
codeparam - (b) token-exchange failure (no
access_tokenin response) - (c)
get_user_inforeturning nologinor raisingHTTPError - (d) missing
integration.metadata["sender"]— theKeyErrorpath from line 503
The KeyError case (d) is particularly important since it's a blocking bug with no test coverage.
| reverse("sentry-extension-setup", kwargs={"provider_id": "github"}) | ||
| ) | ||
| return self.redirect( | ||
| f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" |
There was a problem hiding this comment.
💡 Suggestion — URL-encode redirect_uri in the OAuth authorize URL
The authorize URL is built by raw f-string interpolation with an unencoded redirect_uri. RFC 6749 §3.1.2 requires redirect_uri to be application/x-www-form-urlencoded. The established codebase pattern for the same provider's OAuth flow uses urlencode(params) — see OAuth2LoginView.dispatch at identity/oauth2.py:415 (f"{authorize_url}?{urlencode(params)}").
GitHub currently tolerates this because the Sentry callback URL happens to contain no reserved query characters (&, =, #), but if system.url-prefix or the route ever introduced such a character, the callback URL would be silently truncated/corrupted.
Fix: Build the query string with urlencode({"client_id": ..., "state": ..., "redirect_uri": redirect_uri}).
1 participant
Benchmark PR recreated from getsentry#67876