Sentry PR #29 — Live Benchmark

Golden Comments (2/3 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	OAuth state uses `pipeline.signature` (static) instead of a per-request random value	MEDIUM	✓ Found	"OAuth2 `state` is set to `pipeline.signature`, a deterministic md5 of the pipeline-view class names — identical for every session, user, and installation. The check at line 411 therefore does not bind the callback to the initiating session and provides no real CSRF protection. Use a random per-session nonce stored in pipeline state" (integration.py L402). Note: dataset rates this MEDIUM, FriendlyReviewer rated it High
#2	The code attempts to access `integration.metadata[sender][login]` without checking for the existence of the sender key. This causes a `KeyError` for integrations where the sender metadata was not set during creation	HIGH	✓ Found	"`integration.metadata["sender"]["login"]` raises `KeyError` for Integrations created before this feature or not via the webhook path, since `build_integration` only sets `metadata["sender"]` when `state.get("sender")` is present. Use a defensive lookup (e.g. `integration.metadata.get("sender", {}).get("login")`)" (integration.py L503). Note: dataset rates this HIGH, FriendlyReviewer rated it Medium
#3	Null reference if `github_authenticated_user` state is missing	MEDIUM	✗ Not found	Judge's reasoning: FriendlyReviewer identifies the neighbor — a missing/stale Redis pipeline at the OAuth callback (pipeline_advancer.py L49) drops the user to the site root instead of re-routing into setup — but the golden targets precisely `pipeline.fetch_state("github_authenticated_user")` dereferenced in `GitHubInstallation.dispatch` without checking that the state exists; that exact case is never named. Adjacent bug, not the same. Likely location: `src/sentry/integrations/github/integration.py:502` (comparison `pipeline.fetch_state("github_authenticated_user") != integration.metadata["sender"]["login"]` without verifying the state exists)

Supplementary Findings (5 findings)

Finding	File	Severity	Legitimate?
The access-token exchange omits `redirect_uri` (and `grant_type`) even though the authorize URL built at line 406 includes it	integration.py:415	HIGH	✓ Per GitHub's OAuth contract this yields `redirect_uri_mismatch` and routes every legitimate install into the failure branch — arguably the most damaging finding on this PR
Missing/stale Redis pipeline session at the OAuth callback drops the user to the site root instead of re-routing into setup	pipeline_advancer.py:49	MEDIUM	✓ Real flow bug (Redis flushed, session cookie lost, TTL exceeded); adjacent to the missed golden #3 but a distinct bug
`get_user_info(payload["access_token"])` calls `resp.raise_for_status()` — invalid/expired token raises an unhandled `HTTPError`, producing a 500 instead of the error page	integration.py:432	MEDIUM	✓ Defeats the new failure branch for the exact case it was built for
`test_installation_not_found` now passes a wrong `state`, so it exits on the state-mismatch branch before `GitHubInstallation` runs — the 404 stub is dead code	test_integration.py:393	MEDIUM	✓ Test regression: the 404 path it is named for is no longer covered
No test exercises the access-token failure branch (`if "access_token" not in payload:`) — all stubs return a valid token	integration.py:429	LOW	✓ Legitimate coverage gap, low impact

Honest Analysis

Recall is 66.7% (2/3) — the review's only miss. The missed golden ("Null reference if github_authenticated_user state is missing") targets the exact pipeline.fetch_state("github_authenticated_user") dereference at integration.py:502, compared against integration.metadata["sender"]["login"] without verifying the state exists. FriendlyReviewer covered the neighborhood: its pipeline_advancer.py L49 finding describes a missing/stale Redis pipeline at the callback dropping the user to the site root instead of re-routing into setup. That is an adjacent bug — the same flow, the same class of failure (unchecked pipeline state) — but not the named one, and the judge correctly did not count it. Honest assessment: a real miss, not a coverage failure; FriendlyReviewer was in the right files, it just took a different angle on the same line region.

The partial flag is relevant here: one sub-task of this review was delivered with the partial flag, so its findings may be less fully verified. The review itself acknowledges this in its opening paragraph. Despite that, both found goldens are in the fully-verified zone, and the diagnosis quality on golden #1 is stronger than the dataset's: FriendlyReviewer shows the state is a deterministic public constant (visible in tests as 9cae5e88803f35ed7970fc131e6e65d3), so the check is forgeable by anyone who knows it, and it contradicts the repo's own secrets.token_hex() pattern. The over-classification (dataset Medium → FR High) is justified by the CSRF-forgery argument.

The five extras are all legitimate, and one of them — the omitted redirect_uri in the token exchange (L415) — is arguably the most damaging finding of the entire review: per GitHub's OAuth contract, omitting redirect_uri when it was included in the authorize request yields redirect_uri_mismatch, routing every legitimate installation into the new failure branch. It is a functional blocker that no golden captures, and it also shows why the tests can't catch it (the token endpoint is stubbed). The unhandled HTTPError (L432) and the test_installation_not_found regression (L393) are concrete and verifiable.

Severity discordance runs both ways on this PR: dataset Medium → FR High on the state finding, and dataset High → FR Medium on the KeyError finding — The latter is an under-classification, since a 500 on every pre-existing integration is a crash, not a nit. The extras carry real weight here: the review's highest-value finding (the redirect_uri omission) is outside the reference set entirely.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
2 high, 4 medium, 1 low	1 ok, 3 mitigated

This MR adds a GitHub OAuth2 authorize step to the GitHub integration installation pipeline, together with three new failure branches (user/installer mismatch, state mismatch, and access-token failure), each rendering the shared github-integration-failed.html template. The core wiring into GitHubInstallation.dispatch and the new error() helper are correct and the user-mismatch check is solid. Note: this review is partial — one sub-task was delivered with a partial flag, so its findings may be less fully verified.

However, there are blocking concerns. The OAuth state is derived from pipeline.signature, which is a deterministic constant identical across all sessions/users, providing no real anti-CSRF binding (contradicting the repo's secrets.token_hex() OAuth pattern). The access-token exchange omits redirect_uri even though the authorize step sends it — per GitHub's contract this can return redirect_uri_mismatch, routing every legitimate install into the new failure branch. In addition, get_user_info exceptions are unhandled (500 instead of the error page), the sender metadata lookup can raise KeyError on pre-existing integrations, and a stale/missing Redis pipeline session at the OAuth callback drops the user to the site root instead of re-routing into setup. Test coverage is also incomplete: test_installation_not_found now exercises the state-mismatch branch (no longer the 404 path it's named for), and there is no test for the access-token failure branch. These issues should be addressed before merge.

These might need a close look

🔴 src/sentry/integrations/github/integration.py (L402)
OAuth2 state is set to pipeline.signature, which is a deterministic md5 of the pipeline-view class names — identical for every session, user, and installation (visible in tests as 9cae5e88803f35ed7970fc131e6e65d3). The check at line 411 (request.GET.get("state") != pipeline.signature) therefore does not bind the callback to the initiating session and provides no real CSRF protection, since any caller knowing the public constant can forge the state. This contradicts the repo's established pattern (OAuth2LoginView uses secrets.token_hex() bound to session state). Use a random per-session nonce stored in pipeline state and validate against it; keep pipeline.signature only for is_valid() view-list integrity checking.
🔴 src/sentry/integrations/github/integration.py (L415)
The access-token exchange data dict contains only code, client_id, client_secret and omits redirect_uri (and grant_type), even though the authorize URL built at line 406 includes redirect_uri. GitHub's OAuth contract requires redirect_uri in the token exchange when it was included in the authorize request, and identical; omitting it yields redirect_uri_mismatch (no access_token), which would route every legitimate installation into the failure branch at line 429. This diverges from OAuth2CallbackView.get_token_params — add redirect_uri and grant_type to the payload. Tests can't catch this because the token endpoint is stubbed.

Worth checking

🟡 src/sentry/web/frontend/pipeline_advancer.py (L49)
The forwarding branch (lines 39–47) only routes a pipeline-less GitHub request back into setup when request.GET.get("setup_action") == "install" (the direct 'install from GitHub' hop). The new OAuth2 authorize callback (?code=…&state=…, emitted by OAuthLoginView via sentry-extension-setup) never carries setup_action. If the Redis pipeline session is missing or stale at the callback hop (Redis flushed, session cookie lost, different browser, or TTL 60 min exceeded), pipeline resolves to None, the condition is false, and control falls here producing "Invalid request." and a redirect to / — dropping the user out of setup instead of re-routing them into the org picker. Treat a pipeline-less GitHub request carrying state/code (an OAuth callback) the same way and forward to the org picker.
🟡 src/sentry/integrations/github/integration.py (L432)
get_user_info(payload["access_token"]) calls resp.raise_for_status(), so an invalid/expired token raises an unhandled HTTPError, producing a 500 instead of the intended error page. The if "login" not in authenticated_user_info guard only handles a 2xx response missing the field. Wrap the get_user_info call (or the upstream exception) so this path returns error(...).
🟡 src/sentry/integrations/github/integration.py (L503)
integration.metadata["sender"]["login"] raises KeyError for Integrations created before this feature or not via the webhook path, since build_integration only sets metadata["sender"] when state.get("sender") is present. Use a defensive lookup (e.g. integration.metadata.get("sender", {}).get("login")) and treat a missing sender as a failure instead of a 500.
🟡 tests/sentry/integrations/github/test_integration.py (L393)
test_installation_not_found now passes a wrong state value (ddd023d87a913d5226e2a882c4c4cc05) instead of the correct signature (9cae5e88803f35ed7970fc131e6e65d3 used by assert_setup_flow), so it fails the state check at line 411 and returns "Invalid installation request." before GitHubInstallation/build_integration runs. It no longer covers the 404 → "The GitHub installation could not be found." branch (the responses.replace(..., status=404) stub at line 381 is dead code) and becomes an incidental state-mismatch test. Use the correct state so the flow reaches build_integration and asserts the 404 error, or rename/repurpose and add a dedicated state-mismatch test.

Small things (take or leave)

🔵 src/sentry/integrations/github/integration.py (L429)
No test exercises the access-token failure branch (if "access_token" not in payload:). All stubs return a valid access_token=... body, so the requirement that a wrong/re-used code triggers the intended error is not verified. Add a test stubbing https://github.com/login/oauth/access_token to return an error body (e.g. error=bad_verification_code) and assert the rendered github-integration-failed.html contains "Invalid installation request.".

✅ Feature-level checklist

Looks good

✅ The pipeline must fail when there is a mismatch between the currently authenticated GitHub user (from OAuth2 authorize) and the user who installed the GitHub app.
The user-mismatch check in GitHubInstallation.dispatch is properly implemented and is the strongest of the three branches. Caveat: the integration.metadata["sender"]["login"] lookup can raise KeyError on pre-existing integrations, causing a 500 instead of the intended error page — this is tracked as a technical finding.

Partially covered

⚠️ Add a GitHub OAuth2 authorize step to the installation pipeline that is transparent from the UX perspective (data exchange without user interaction).
The OAuth2 step is wired in and transparent in the normal flow, but the missing/stale Redis pipeline routing gap at the callback (pipeline_advancer.py:49) and the extra redirect hop for customer-domain orgs (redirect_uri without url_prefix) can degrade the UX in edge cases.
⚠️ The pipeline must fail when there is a mismatch between the state parameter supplied by the user and the pipeline signature.
The failure branch is implemented and returns the intended error, but the state is a deterministic constant derived from pipeline.signature rather than a per-session nonce, so the check provides no real anti-CSRF binding and does not constrain an attacker who knows the public constant. The test_installation_not_found regression also means the state-mismatch path is now exercised inadvertently rather than by an explicit test.
⚠️ The pipeline must fail when GitHub cannot generate a correct access_token from the code (wrong code or attempt to re-use a code).
The access-token failure branch exists and returns the intended error, but there is no test coverage for it. More importantly, the token exchange omits redirect_uri, which can cause GitHub to return redirect_uri_mismatch for legitimate installs and trigger this failure branch even when the code is correct — this is a blocking technical finding.

GitHub OAuth Security Enhancement