GitHub OAuth Security Enhancement

[CodingKylo]

Martian Code Review Benchmark PR (mirrored from source #4)

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 94/100 · CRITICAL

Dimension	Level
Likelihood	🟢 Low
Impact	🟢 Low
Detectability	🟢 Low

Intent

Harden the GitHub OAuth-based installation setup flow by adding a signed OAuth state verification step and adjusting frontend pipeline forwarding so direct GitHub redirects land in the correct setup path.

Summary

The change introduces a new OAuthLoginView into the GitHub integration pipeline and updates frontend pipeline forwarding logic for GitHub installs. It also adds tests that stub GitHub OAuth endpoints and assert exact redirect query parameters, including a constructed redirect_uri. The top risks to verify are (1) correctness/security of the OAuth state signature verification and how it binds the authenticated user to the pipeline, and (2) routing/pipeline contract changes in pipeline_advancer that could mis-forward installs when pipeline is missing. You should also verify that the tests’ stubs match the real parsing/headers behavior of safe_urlopen/safe_urlread and that failure templates are consistent across pipeline stages.

🎯 Review Focus

Confirm that the new OAuthLoginView’s state signature verification correctly rejects invalid/mismatched state before advancing the pipeline, and that the frontend pipeline_advancer routing change cannot mis-forward GitHub installs when pipeline is missing (pipeline=None) for any supported GitHub provider_id/setup_action combinations.

Key Findings

• 🚨 [src/sentry/web/frontend/pipeline_advancer.py:L34] CRITICAL: if ( provider_id == "github" and request.GET.get("setup_action") == "install" and pipeline is None ): — This hardcodes forwarding behavior to provider_id=="github" and removes the previous allowlist (FORWARD_INSTALL_FOR = ["github"]). If any other GitHub-related provider_id values, aliases, or future pipeline steps rely on the old constant/contract, installs can be misrouted (setup_action=install with pipeline=None) and users may be sent to the wrong org picker or stuck in an inconsistent state. Fix: restore the explicit allowlist/contract (or make it data-driven) and add a regression test that covers the “redirect here without being in the pipeline” case for all supported GitHub provider_id values and setup_action combinations. Concretely, reintroduce FORWARD_INSTALL_FOR (or equivalent) and gate on membership rather than a single literal, e.g. if provider_id in FORWARD_INSTALL_FOR and ... plus tests asserting correct redirect target when pipeline is None.

✅ Action Checklist

• [ ] CRITICAL [src/sentry/web/frontend/pipeline_advancer.py:L34] — if ( provider_id == "github" and request.GET.get("setup_action") == "install" and pipeline is None ): — This hardcodes forwarding behavior to provider_id=="github" and removes the previous allowlist (FORWARD_INSTALL_FOR = ["github"]). If any other GitHub-related provider_id values, aliases, or future pipeline steps rely on the old constant/contract, installs can be misrouted (setup_action=install with pipeline=None) and users may be sent to the wrong org picker or stuck in an inconsistent state. Fix: restore the explicit allowlist/contract (or make it data-driven) and add a regression test that covers the “redirect here without being in the pipeline” case for all supported GitHub provider_id values and setup_action combinations. Concretely, reintroduce FORWARD_INSTALL_FOR (or equivalent) and gate on membership rather than a single literal, e.g. if provider_id in FORWARD_INSTALL_FOR and ... plus tests asserting correct redirect target when pipeline is None.
• [ ] SUGGESTION — Verify OAuth state signature verification and binding logic end-to-end in src/sentry/integrations/github/integration.py (new OAuthLoginView.dispatch). Add/extend tests for: (a) missing/empty state, (b) invalid signature, (c) valid signature but authenticated user mismatch, and (d) replayed/expired state. Ensure the view rejects before calling pipeline.next_step() and that the error response matches the intended template/HTTP status.
• [ ] SUGGESTION — Align test stubs with real OAuth token exchange parsing in src/sentry/integrations/github/integration.py where safe_urlopen/safe_urlread are used. In tests/sentry/integrations/github/test_integration.py, the stub currently returns only body=f"access_token={access_token}" for https://github.com/login/oauth/access_token. Update the stub to match the actual response format expected by the production parser (e.g., include Content-Type: application/x-www-form-urlencoded and any additional fields the code reads). Also assert request headers/body for the token exchange (client_id/client_secret/code) to catch regressions.
• [ ] SUGGESTION — Reduce brittleness of exact redirect query assertions in tests/sentry/integrations/github/test_integration.py. The test asserts an exact redirect.query == "client_id=...&state=...&redirect_uri=...". Instead, parse the query string and assert required keys/values independently (client_id, state, redirect_uri) so ordering/encoding changes don’t cause false failures while still validating correctness.
• [ ] SUGGESTION — Add a cross-view failure contract test for missing/invalid installation identifiers. Since the new flow adds ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST and introduces new state/installation_id handling, add tests that ensure OAuthLoginView and the subsequent GitHubInstallation step render consistent errors when installation_id/state are missing or invalid (not just the happy path and one failure template).

Suggestions

• Verify OAuth state signature verification and binding logic end-to-end in src/sentry/integrations/github/integration.py (new OAuthLoginView.dispatch). Add/extend tests for: (a) missing/empty state, (b) invalid signature, (c) valid signature but authenticated user mismatch, and (d) replayed/expired state. Ensure the view rejects before calling pipeline.next_step() and that the error response matches the intended template/HTTP status.
• Align test stubs with real OAuth token exchange parsing in src/sentry/integrations/github/integration.py where safe_urlopen/safe_urlread are used. In tests/sentry/integrations/github/test_integration.py, the stub currently returns only body=f"access_token={access_token}" for https://github.com/login/oauth/access_token. Update the stub to match the actual response format expected by the production parser (e.g., include Content-Type: application/x-www-form-urlencoded and any additional fields the code reads). Also assert request headers/body for the token exchange (client_id/client_secret/code) to catch regressions.
• Reduce brittleness of exact redirect query assertions in tests/sentry/integrations/github/test_integration.py. The test asserts an exact redirect.query == "client_id=...&state=...&redirect_uri=...". Instead, parse the query string and assert required keys/values independently (client_id, state, redirect_uri) so ordering/encoding changes don’t cause false failures while still validating correctness.
• Add a cross-view failure contract test for missing/invalid installation identifiers. Since the new flow adds ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST and introduces new state/installation_id handling, add tests that ensure OAuthLoginView and the subsequent GitHubInstallation step render consistent errors when installation_id/state are missing or invalid (not just the happy path and one failure template).

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🟡 Risk Score: 28/100 · MEDIUM

Dimension	Level
Likelihood	🟡 Medium
Impact	🟡 Medium
Detectability	🟢 Low

Intent

Harden Sentry’s GitHub OAuth/install flow and adjust the redirect/pipeline handling so GitHub app installations securely resume into the correct setup pipeline.

Summary

The PR introduces a new OAuth step for GitHub app setup (including an invalid-installation-request error path) and changes frontend pipeline forwarding logic to only special-case provider_id == "github". It also updates tests to account for additional outbound OAuth calls and new /login/oauth/authorize redirect behavior. The top risks to verify are (1) correctness/security of the new OAuth installation request authenticity checks (state/code/installation_id handling across the redirect boundary) and (2) pipeline resumption/redirect routing consistency between integration.py and pipeline_advancer.py so the OAuth callback binds to the expected pipeline state. You should also check that the tests are not brittle (call-index based) and that negative cases (missing/malformed query params) are covered.

🎯 Review Focus

Verify the new OAuth installation request authenticity/state handling end-to-end (including negative cases) and confirm that the OAuth callback redirect correctly resumes the intended pipeline instance without falling into the fallback org-picker redirect.

Key Findings

• 🚨 [src/sentry/integrations/github/integration.py:L113-L140] CRITICAL: def error(...): return render_to_response("sentry/integrations/githu — The diff is truncated mid-string, which strongly suggests the actual change may have introduced a broken template path or syntax error in this function. If this is real in the PR, it will crash the setup flow at runtime when the error path is hit (e.g., invalid installation request). Fix: ensure the template string is complete and correct (e.g., "sentry/integrations/github/error.html" or whatever the actual template is), and add/verify a unit test that triggers ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST to exercise this exact error() path.
• ⚠️ [src/sentry/web/frontend/pipeline_advancer.py:L34-L52] WARNING: if ( provider_id == "github" and request.GET.get("setup_action") == "install" and pipeline is None ): — This changes the forwarding behavior from FORWARD_INSTALL_FOR = ["github"] to a narrower condition, but it still relies on setup_action=install and pipeline is None to decide routing. If the OAuth callback introduces different query params (or if pipeline creation is delayed), you can end up incorrectly redirecting users to the org picker instead of resuming the OAuth/pipeline state. Fix: assert/encode the exact expected OAuth callback parameters (e.g., presence of code/state or installation identifiers) before forwarding, and add an integration test that covers the full redirect chain when the GitHub redirect arrives without an existing pipeline.

✅ Action Checklist

• [ ] CRITICAL [src/sentry/integrations/github/integration.py:L113-L140] — def error(...): return render_to_response("sentry/integrations/githu — The diff is truncated mid-string, which strongly suggests the actual change may have introduced a broken template path or syntax error in this function. If this is real in the PR, it will crash the setup flow at runtime when the error path is hit (e.g., invalid installation request). Fix: ensure the template string is complete and correct (e.g., "sentry/integrations/github/error.html" or whatever the actual template is), and add/verify a unit test that triggers ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST to exercise this exact error() path.
• [ ] WARNING [src/sentry/web/frontend/pipeline_advancer.py:L34-L52] — if ( provider_id == "github" and request.GET.get("setup_action") == "install" and pipeline is None ): — This changes the forwarding behavior from FORWARD_INSTALL_FOR = ["github"] to a narrower condition, but it still relies on setup_action=install and pipeline is None to decide routing. If the OAuth callback introduces different query params (or if pipeline creation is delayed), you can end up incorrectly redirecting users to the org picker instead of resuming the OAuth/pipeline state. Fix: assert/encode the exact expected OAuth callback parameters (e.g., presence of code/state or installation identifiers) before forwarding, and add an integration test that covers the full redirect chain when the GitHub redirect arrives without an existing pipeline.
• [ ] SUGGESTION — Add negative tests for the new OAuth redirect boundary inputs in tests/sentry/integrations/github/test_integration.py (around the updated setup flow assertions). Specifically cover: missing code, missing state, mismatched state, and mismatched/invalid installation_id. This should validate that ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST is rendered and that no pipeline state is resumed when authenticity checks fail. (Tie to the new error template path exercised by error() in src/sentry/integrations/github/integration.py.)
• [ ] SUGGESTION — Make the GitHub OAuth outbound-call assertions resilient by matching on request URL/headers rather than call indices. In tests/sentry/integrations/github/test_integration.py, replace any responses.calls[N] style checks with assertions that search responses.calls for the expected endpoint (e.g., /login/oauth/access_token, /user, and the installation access token endpoint). This prevents future pipeline-order changes from breaking tests without changing behavior.
• [ ] SUGGESTION — Centralize the GitHub-specific redirect/pipeline resumption logic to avoid coordinated edits across src/sentry/integrations/github/integration.py and src/sentry/web/frontend/pipeline_advancer.py. Concretely: move the “redirect here without being in the pipeline” handling into a single helper used by both the integration pipeline registration and the advancer forwarding decision, and add a regression test for the “GitHub redirect without pipeline” scenario.

Suggestions

• Add negative tests for the new OAuth redirect boundary inputs in tests/sentry/integrations/github/test_integration.py (around the updated setup flow assertions). Specifically cover: missing code, missing state, mismatched state, and mismatched/invalid installation_id. This should validate that ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST is rendered and that no pipeline state is resumed when authenticity checks fail. (Tie to the new error template path exercised by error() in src/sentry/integrations/github/integration.py.)
• Make the GitHub OAuth outbound-call assertions resilient by matching on request URL/headers rather than call indices. In tests/sentry/integrations/github/test_integration.py, replace any responses.calls[N] style checks with assertions that search responses.calls for the expected endpoint (e.g., /login/oauth/access_token, /user, and the installation access token endpoint). This prevents future pipeline-order changes from breaking tests without changing behavior.
• Centralize the GitHub-specific redirect/pipeline resumption logic to avoid coordinated edits across src/sentry/integrations/github/integration.py and src/sentry/web/frontend/pipeline_advancer.py. Concretely: move the “redirect here without being in the pipeline” handling into a single helper used by both the integration pipeline registration and the advancer forwarding decision, and add a regression test for the “GitHub redirect without pipeline” scenario.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🟡 Risk Score: 28/100 · MEDIUM

Dimension	Level
Likelihood	🟡 Medium
Impact	🟡 Medium
Detectability	🟢 Low

Intent

Harden the GitHub OAuth + installation setup flow and ensure direct GitHub redirects are forwarded into the correct Sentry integration install UI.

Summary

The PR removes the generic forward-redirect mechanism and now forwards only when provider_id == "github" and setup_action=install with no pipeline. It also extends the GitHub OAuth setup flow to perform additional external HTTP calls (OAuth access token exchange and /user) and introduces new error handling for invalid installation requests. The main risks to verify are (1) whether the new forwarding contract breaks any existing assumptions for other providers or edge cases, and (2) whether the OAuth flow correctly validates code/state and fails safely without leaking partial state. Finally, the updated tests appear tightly coupled to exact redirect query-string ordering; verify behavior rather than string formatting to avoid brittle failures.

🎯 Review Focus

Verify the end-to-end OAuth/install setup failure handling and forwarding contract: specifically, confirm code/state are validated before external calls and that direct GitHub redirects are forwarded only under the intended conditions without breaking any prior extensibility assumptions.

Key Findings

• 🚨 [src/sentry/web/frontend/pipeline_advancer.py:L34] CRITICAL: provider_id == "github" hard-codes forwarding and removes FORWARD_INSTALL_FOR = ["github"] — if any caller previously relied on the forward list semantics (or if future providers are added), direct redirects will silently stop forwarding and strand users in the wrong UI state. Fix: reintroduce a shared forwardable-provider mechanism (e.g., a constant/utility used by both backend and frontend) and add a regression test that asserts forwarding behavior for github and that non-github providers do not forward (or, if intended, that additional providers can be configured without code changes).
• ⚠️ [src/sentry/integrations/github/integration.py:L126] WARNING: New helper def error(...): return render_to_response("sentry/integrations/githu... introduces a new error rendering path but the diff is truncated; ensure the template path is correct and that all failure modes in the OAuth/install flow use this helper consistently. Fix: verify the full template string (no truncation/typo), and update/extend tests to cover every new failure mode introduced by the OAuth/pipeline changes (e.g., missing/invalid code, missing/invalid state, invalid installation request).

✅ Action Checklist

• [ ] CRITICAL [src/sentry/web/frontend/pipeline_advancer.py:L34] — provider_id == "github" hard-codes forwarding and removes FORWARD_INSTALL_FOR = ["github"] — if any caller previously relied on the forward list semantics (or if future providers are added), direct redirects will silently stop forwarding and strand users in the wrong UI state. Fix: reintroduce a shared forwardable-provider mechanism (e.g., a constant/utility used by both backend and frontend) and add a regression test that asserts forwarding behavior for github and that non-github providers do not forward (or, if intended, that additional providers can be configured without code changes).
• [ ] WARNING [src/sentry/integrations/github/integration.py:L126] — New helper def error(...): return render_to_response("sentry/integrations/githu... introduces a new error rendering path but the diff is truncated; ensure the template path is correct and that all failure modes in the OAuth/install flow use this helper consistently. Fix: verify the full template string (no truncation/typo), and update/extend tests to cover every new failure mode introduced by the OAuth/pipeline changes (e.g., missing/invalid code, missing/invalid state, invalid installation request).
• [ ] SUGGESTION — [src/sentry/integrations/github/integration.py:L4-L20] Add explicit validation for OAuth query params before any network calls: validate presence and expected format of code and state (and that state matches what you issued). Concretely, before calling safe_urlopen/safe_urlread (imported in this file), do something like if not code or not state: return error(request, org, error_short="Missing OAuth parameters", error_long=...); and ensure invalid state returns the new ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST path.
• [ ] SUGGESTION — [tests/sentry/integrations/github/test_integration.py:L218-L244] The test asserts exact redirect query-string ordering: assert redirect.query == "client_id=...&state=...&redirect_uri=...". This is brittle. Fix: parse query params into a dict and assert key/value pairs, e.g. params = dict(parse_qsl(redirect.query)); assert params["client_id"] == ...; assert params["state"] == ...; assert params["redirect_uri"] == ...;.
• [ ] SUGGESTION — [tests/sentry/integrations/github/test_integration.py:L111-L130] The OAuth stubs now include /login/oauth/access_token and /user. Add negative-path tests for failure modes (invalid/missing code, missing state, and /user returning non-200) and assert the user is shown the new error template/context (the one introduced via the new error() helper).
• [ ] SUGGESTION — [src/sentry/web/frontend/pipeline_advancer.py:L28-L45] Add a regression test for the forwarding contract change: verify that when provider_id != "github" (or when setup_action != "install") the redirect does not forward into the org picker, and when provider_id == "github" and setup_action=install with no pipeline it does forward. This ensures the hard-coded behavior matches intent.

Suggestions

• [src/sentry/integrations/github/integration.py:L4-L20] Add explicit validation for OAuth query params before any network calls: validate presence and expected format of code and state (and that state matches what you issued). Concretely, before calling safe_urlopen/safe_urlread (imported in this file), do something like if not code or not state: return error(request, org, error_short="Missing OAuth parameters", error_long=...); and ensure invalid state returns the new ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST path.
• [tests/sentry/integrations/github/test_integration.py:L218-L244] The test asserts exact redirect query-string ordering: assert redirect.query == "client_id=...&state=...&redirect_uri=...". This is brittle. Fix: parse query params into a dict and assert key/value pairs, e.g. params = dict(parse_qsl(redirect.query)); assert params["client_id"] == ...; assert params["state"] == ...; assert params["redirect_uri"] == ...;.
• [tests/sentry/integrations/github/test_integration.py:L111-L130] The OAuth stubs now include /login/oauth/access_token and /user. Add negative-path tests for failure modes (invalid/missing code, missing state, and /user returning non-200) and assert the user is shown the new error template/context (the one introduced via the new error() helper).
• [src/sentry/web/frontend/pipeline_advancer.py:L28-L45] Add a regression test for the forwarding contract change: verify that when provider_id != "github" (or when setup_action != "install") the redirect does not forward into the org picker, and when provider_id == "github" and setup_action=install with no pipeline it does forward. This ensures the hard-coded behavior matches intent.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🟡 Risk Score: 28/100 · MEDIUM

Dimension	Level
Likelihood	🟡 Medium
Impact	🟡 Medium
Detectability	🟢 Low

Intent

Harden the GitHub OAuth + installation setup flow and ensure direct GitHub redirects are forwarded into the correct Sentry integration install UI.

Summary

The PR removes the generic forward-redirect mechanism and now forwards only when provider_id == "github" and setup_action=install with no pipeline. It also extends the GitHub OAuth setup flow to perform additional external HTTP calls (OAuth access token exchange and /user) and introduces new error handling for invalid installation requests. The main risks to verify are (1) whether the new forwarding contract breaks any existing assumptions for other providers or edge cases, and (2) whether the OAuth flow correctly validates code/state and fails safely without leaking partial state. Finally, the updated tests appear tightly coupled to exact redirect query-string ordering; verify behavior rather than string formatting to avoid brittle failures.

🎯 Review Focus

Verify the end-to-end OAuth/install setup failure handling and forwarding contract: specifically, confirm code/state are validated before external calls and that direct GitHub redirects are forwarded only under the intended conditions without breaking any prior extensibility assumptions.

Key Findings

• 🚨 [src/sentry/web/frontend/pipeline_advancer.py:L34] CRITICAL: provider_id == "github" hard-codes forwarding and removes FORWARD_INSTALL_FOR = ["github"] — if any caller previously relied on the forward list semantics (or if future providers are added), direct redirects will silently stop forwarding and strand users in the wrong UI state. Fix: reintroduce a shared forwardable-provider mechanism (e.g., a constant/utility used by both backend and frontend) and add a regression test that asserts forwarding behavior for github and that non-github providers do not forward (or, if intended, that additional providers can be configured without code changes).
• ⚠️ [src/sentry/integrations/github/integration.py:L126] WARNING: New helper def error(...): return render_to_response("sentry/integrations/githu... introduces a new error rendering path but the diff is truncated; ensure the template path is correct and that all failure modes in the OAuth/install flow use this helper consistently. Fix: verify the full template string (no truncation/typo), and update/extend tests to cover every new failure mode introduced by the OAuth/pipeline changes (e.g., missing/invalid code, missing/invalid state, invalid installation request).

✅ Action Checklist

• [ ] CRITICAL [src/sentry/web/frontend/pipeline_advancer.py:L34] — provider_id == "github" hard-codes forwarding and removes FORWARD_INSTALL_FOR = ["github"] — if any caller previously relied on the forward list semantics (or if future providers are added), direct redirects will silently stop forwarding and strand users in the wrong UI state. Fix: reintroduce a shared forwardable-provider mechanism (e.g., a constant/utility used by both backend and frontend) and add a regression test that asserts forwarding behavior for github and that non-github providers do not forward (or, if intended, that additional providers can be configured without code changes).
• [ ] WARNING [src/sentry/integrations/github/integration.py:L126] — New helper def error(...): return render_to_response("sentry/integrations/githu... introduces a new error rendering path but the diff is truncated; ensure the template path is correct and that all failure modes in the OAuth/install flow use this helper consistently. Fix: verify the full template string (no truncation/typo), and update/extend tests to cover every new failure mode introduced by the OAuth/pipeline changes (e.g., missing/invalid code, missing/invalid state, invalid installation request).
• [ ] SUGGESTION — [src/sentry/integrations/github/integration.py:L4-L20] Add explicit validation for OAuth query params before any network calls: validate presence and expected format of code and state (and that state matches what you issued). Concretely, before calling safe_urlopen/safe_urlread (imported in this file), do something like if not code or not state: return error(request, org, error_short="Missing OAuth parameters", error_long=...); and ensure invalid state returns the new ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST path.
• [ ] SUGGESTION — [tests/sentry/integrations/github/test_integration.py:L218-L244] The test asserts exact redirect query-string ordering: assert redirect.query == "client_id=...&state=...&redirect_uri=...". This is brittle. Fix: parse query params into a dict and assert key/value pairs, e.g. params = dict(parse_qsl(redirect.query)); assert params["client_id"] == ...; assert params["state"] == ...; assert params["redirect_uri"] == ...;.
• [ ] SUGGESTION — [tests/sentry/integrations/github/test_integration.py:L111-L130] The OAuth stubs now include /login/oauth/access_token and /user. Add negative-path tests for failure modes (invalid/missing code, missing state, and /user returning non-200) and assert the user is shown the new error template/context (the one introduced via the new error() helper).
• [ ] SUGGESTION — [src/sentry/web/frontend/pipeline_advancer.py:L28-L45] Add a regression test for the forwarding contract change: verify that when provider_id != "github" (or when setup_action != "install") the redirect does not forward into the org picker, and when provider_id == "github" and setup_action=install with no pipeline it does forward. This ensures the hard-coded behavior matches intent.

Suggestions

• [src/sentry/integrations/github/integration.py:L4-L20] Add explicit validation for OAuth query params before any network calls: validate presence and expected format of code and state (and that state matches what you issued). Concretely, before calling safe_urlopen/safe_urlread (imported in this file), do something like if not code or not state: return error(request, org, error_short="Missing OAuth parameters", error_long=...); and ensure invalid state returns the new ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST path.
• [tests/sentry/integrations/github/test_integration.py:L218-L244] The test asserts exact redirect query-string ordering: assert redirect.query == "client_id=...&state=...&redirect_uri=...". This is brittle. Fix: parse query params into a dict and assert key/value pairs, e.g. params = dict(parse_qsl(redirect.query)); assert params["client_id"] == ...; assert params["state"] == ...; assert params["redirect_uri"] == ...;.
• [tests/sentry/integrations/github/test_integration.py:L111-L130] The OAuth stubs now include /login/oauth/access_token and /user. Add negative-path tests for failure modes (invalid/missing code, missing state, and /user returning non-200) and assert the user is shown the new error template/context (the one introduced via the new error() helper).
• [src/sentry/web/frontend/pipeline_advancer.py:L28-L45] Add a regression test for the forwarding contract change: verify that when provider_id != "github" (or when setup_action != "install") the redirect does not forward into the org picker, and when provider_id == "github" and setup_action=install with no pipeline it does forward. This ensures the hard-coded behavior matches intent.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

🚨 Risk Score: 90/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟠 High

Intent

Add a secure OAuth pipeline step for GitHub login that validates the OAuth state and completes the authorization-code exchange, plus update tests for the new flow and error handling.

Summary

Behaviorally, this introduces a new GitHub OAuthLoginView stage into the pipeline and adds an invalid-installation error rendering path. The highest risk is correctness/security around hand-constructed OAuth redirect URLs (query parameter encoding) and the new pipeline/forwarding contract when the pipeline is missing. You should verify that the authorize redirect URL is built with proper URL encoding, that state validation cannot be bypassed for any request shape, and that the pipeline_advancer forwarding logic still routes users correctly for all GitHub callback/installation entry points. Also confirm the new error path is exercised and that tests are not brittle due to positional mocking of outbound HTTP calls.

🎯 Review Focus

Verify the OAuth authorize redirect URL construction and state validation end-to-end: ensure state is validated against the correct pipeline signature for every callback shape, and ensure the authorize URL is built with correct URL encoding so GitHub parses state and redirect_uri exactly as intended.

Key Findings

• 🚨 [src/sentry/web/frontend/pipeline_advancer.py:L34] CRITICAL: The forwarding logic was changed from a provider list to a hard-coded provider check, which can silently break the intended contract for GitHub-only flows if provider_id values differ across entry points. Evidence: provider_id == "github" replaces provider_id in FORWARD_INSTALL_FOR and the list constant was removed. — Why: If any caller passes a different provider_id string for GitHub (case/alias/namespace), the redirect-to-org-picker behavior will stop working only for those paths, potentially stranding users mid-install or causing them to re-initiate OAuth incorrectly. This is a correctness/security-adjacent issue because it changes the authentication/installation state machine. — Fix: Restore a robust mapping rather than a single literal. Example: reintroduce FORWARD_INSTALL_FOR = {"github"} (or include known aliases) and use if provider_id in FORWARD_INSTALL_FOR .... Also add an integration test for the exact entry point where pipeline is None and verify the redirect goes to the org picker for the real provider_id value used by the app.

✅ Action Checklist

• [ ] CRITICAL [src/sentry/web/frontend/pipeline_advancer.py:L34] — The forwarding logic was changed from a provider list to a hard-coded provider check, which can silently break the intended contract for GitHub-only flows if provider_id values differ across entry points. Evidence: provider_id == "github" replaces provider_id in FORWARD_INSTALL_FOR and the list constant was removed. — Why: If any caller passes a different provider_id string for GitHub (case/alias/namespace), the redirect-to-org-picker behavior will stop working only for those paths, potentially stranding users mid-install or causing them to re-initiate OAuth incorrectly. This is a correctness/security-adjacent issue because it changes the authentication/installation state machine. — Fix: Restore a robust mapping rather than a single literal. Example: reintroduce FORWARD_INSTALL_FOR = {"github"} (or include known aliases) and use if provider_id in FORWARD_INSTALL_FOR .... Also add an integration test for the exact entry point where pipeline is None and verify the redirect goes to the org picker for the real provider_id value used by the app.
• [ ] SUGGESTION — tests/sentry/integrations/github/test_integration.py:L120: The test stub currently only returns access_token=... and omits the rest of the OAuth exchange contract. Strengthen it by asserting the full expected response parsing behavior (e.g., ensure the code handles missing/extra fields deterministically and that subsequent /user call is made with the access token). Concretely: update the stub to return a realistic token response body (including token_type, scope, etc. if the integration expects them) and assert on the integration’s downstream behavior.
• [ ] SUGGESTION — src/sentry/integrations/github/integration.py:L438: Centralize OAuth URL construction and update tests to validate query parameters via parsing. Concretely: in tests, replace redirect.query == "client_id=...&state=...&redirect_uri=..." with parse_qs(redirect.query) assertions for client_id, state, and redirect_uri values. This prevents brittle failures and ensures encoding correctness is preserved.
• [ ] SUGGESTION — tests/sentry/integrations/github/test_integration.py:L229: The test changes appear to rely on positional ordering of responses.calls (per the risk assessment). Make the test resilient by locating the mocked request by URL/endpoint (e.g., search responses.calls for /login/oauth/access_token and /user) and assert headers/body there, rather than using fixed indices.
• [ ] SUGGESTION — src/sentry/integrations/github/integration.py:L126: Add explicit test coverage for the new invalid-installation error contract (short vs long message) rather than only asserting the template name. Concretely: trigger the invalid installation request path and assert the rendered context contains error_short == "Invalid installation request." and error_long == ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST (or whatever keys the template uses).

Suggestions

• tests/sentry/integrations/github/test_integration.py:L120: The test stub currently only returns access_token=... and omits the rest of the OAuth exchange contract. Strengthen it by asserting the full expected response parsing behavior (e.g., ensure the code handles missing/extra fields deterministically and that subsequent /user call is made with the access token). Concretely: update the stub to return a realistic token response body (including token_type, scope, etc. if the integration expects them) and assert on the integration’s downstream behavior.
• src/sentry/integrations/github/integration.py:L438: Centralize OAuth URL construction and update tests to validate query parameters via parsing. Concretely: in tests, replace redirect.query == "client_id=...&state=...&redirect_uri=..." with parse_qs(redirect.query) assertions for client_id, state, and redirect_uri values. This prevents brittle failures and ensures encoding correctness is preserved.
• tests/sentry/integrations/github/test_integration.py:L229: The test changes appear to rely on positional ordering of responses.calls (per the risk assessment). Make the test resilient by locating the mocked request by URL/endpoint (e.g., search responses.calls for /login/oauth/access_token and /user) and assert headers/body there, rather than using fixed indices.
• src/sentry/integrations/github/integration.py:L126: Add explicit test coverage for the new invalid-installation error contract (short vs long message) rather than only asserting the template name. Concretely: trigger the invalid installation request path and assert the rendered context contains error_short == "Invalid installation request." and error_long == ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST (or whatever keys the template uses).

📝 This review includes 2 inline comments (1 warning, 1 note)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

⚠️ WARNING

The redirect is now built by concatenating raw query parameters into the authorize URL. That is a real correctness/security risk because state and redirect_uri are not URL-encoded here, so any reserved characters would break the OAuth request and could allow parameter injection if those values ever contain attacker-controlled data. The root cause is that the code is manually assembling a URL instead of using a query encoder or urllib.parse.urlencode.

---

re-entry.ai

[re-entry-ai]

💡 NOTE

This test stub is intentionally incomplete: it only returns access_token=... from GitHub's token endpoint and omits the rest of the OAuth exchange details. That is not a bug in the product code, but it does mean the test is coupled to a very narrow response shape and may miss regressions in how the integration parses token responses. Consider asserting the full expected response contract instead of relying on a minimal body.

---

re-entry.ai

[re-entry-local]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 82/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Harden the GitHub app installation setup flow by adding a signed OAuth state verification step and ensuring direct-from-GitHub redirects land in the correct integration install pipeline.

Summary

The PR inserts a new OAuthLoginView into the GitHub integration pipeline and updates frontend pipeline forwarding logic so only GitHub install redirects are forwarded when no pipeline exists. It also adds new error handling for invalid installation requests and extends tests to cover the OAuth redirect and some failure cases. The highest risks to verify are (1) correctness/security of the OAuth redirect_uri/state construction and parsing (query encoding and signature binding), and (2) the pipeline forwarding contract between pipeline_advancer.py and the new OAuth pipeline step to avoid misrouting or bypassing expected steps. Reviewers must confirm the OAuth state verification is actually bound to the expected pipeline instance and that the frontend forwarding change doesn’t break legitimate install resumption paths.

🎯 Review Focus

The security/correctness boundary between OAuthLoginView’s signed state verification and the frontend pipeline forwarding contract (pipeline_advancer.py): confirm that a valid state is required to bind the authenticated GitHub user to the correct pipeline instance, and that the new forwarding condition cannot misroute or skip required steps when pipeline is None.

✅ Action Checklist

• [ ] SUGGESTION — Verify OAuth redirect_uri/state construction and parsing are robust to URL encoding changes: in tests/sentry/integrations/github/test_integration.py the test asserts an exact query string (redirect.query == "client_id=...&state=...&redirect_uri=http://testserver/extensions/github/setup/"). Prefer parsing/normalizing (e.g., parse_qs(redirect.query)) and asserting individual parameters, so the contract is about values not string formatting.
• [ ] SUGGESTION — Add an end-to-end test that covers the new forwarding contract when pipeline is missing: since src/sentry/web/frontend/pipeline_advancer.py now forwards only when provider_id == "github" and setup_action=install with pipeline is None, add a test that starts from the direct GitHub redirect path and asserts the user lands in the correct integration install UI after OAuthLoginView runs (or fails) rather than being dropped or double-advanced.
• [ ] SUGGESTION — In src/sentry/integrations/github/integration.py, ensure the OAuth state signature check binds to the correct pipeline instance and expected redirect_uri/client_id: add/extend tests for malformed/missing code/state and for tampered state to confirm the error path (ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST) triggers and does not proceed to token exchange or installation creation.

Suggestions

• Verify OAuth redirect_uri/state construction and parsing are robust to URL encoding changes: in tests/sentry/integrations/github/test_integration.py the test asserts an exact query string (redirect.query == "client_id=...&state=...&redirect_uri=http://testserver/extensions/github/setup/"). Prefer parsing/normalizing (e.g., parse_qs(redirect.query)) and asserting individual parameters, so the contract is about values not string formatting.
• Add an end-to-end test that covers the new forwarding contract when pipeline is missing: since src/sentry/web/frontend/pipeline_advancer.py now forwards only when provider_id == "github" and setup_action=install with pipeline is None, add a test that starts from the direct GitHub redirect path and asserts the user lands in the correct integration install UI after OAuthLoginView runs (or fails) rather than being dropped or double-advanced.
• In src/sentry/integrations/github/integration.py, ensure the OAuth state signature check binds to the correct pipeline instance and expected redirect_uri/client_id: add/extend tests for malformed/missing code/state and for tampered state to confirm the error path (ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST) triggers and does not proceed to token exchange or installation creation.

📝 This review includes 3 inline comments (3 warnings)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-local]

⚠️ WARNING

Quote: access_token = "xxxxx-xxxxxxxxx-xxxxxxxxx-xxxxxxxxxxxx"

Issue: This looks like a placeholder token value, but the test stubs the OAuth access token exchange and then proceeds through the flow. If any other part of the test suite (or code under test) logs or persists the token, this could accidentally become a real secret in CI logs or fixtures. The diff introduces a new token-like string.

Fix: Use a clearly non-secret sentinel and ensure it is never logged/persisted, e.g. access_token = "test-access-token", and/or assert that the token is not stored in the DB/logs for this flow. (see also L120, L122)

---

re-entry.ai

[re-entry-local]

⚠️ WARNING

Quote: auth_header = responses.calls[2].request.headers["Authorization"]

Issue: Indexing into responses.calls[2] is brittle: adding/removing stubs or changing call order will make the test assert against the wrong request (or raise IndexError / KeyError). This can create false failures or, worse, false passes if the wrong call still has an Authorization header.

Fix: Find the call by URL/method instead of a numeric index, e.g.:

call = next(c for c in responses.calls if c.request.url == expected_url)
auth_header = call.request.headers["Authorization"]

---

re-entry.ai

[re-entry-local]

⚠️ WARNING

Quote: webhook_event = json.loads(INSTALLATION_EVENT_EXAMPLE)

Issue: The test uses a webhook example payload and then mutates it to simulate an attacker. However, the diff does not show any stubbing/verification that the webhook signature is actually validated against the mutated payload (it only provides a static HTTP_X_HUB_SIGNATURE). If the signature validation logic depends on the exact body bytes, this test could be invalid or could accidentally pass due to how the server reads/normalizes the request body.

Fix: Ensure the signature corresponds to the exact JSON bytes sent. Compute the signature in-test from the request body (or use the framework helper) rather than hardcoding sha1=.... (see also L392)

---

re-entry.ai

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 82/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Harden the GitHub app installation setup flow by adding a signed OAuth state verification step and adjusting frontend pipeline forwarding so direct GitHub redirects land in the correct setup path.

Summary

The PR inserts a new OAuthLoginView stage into the GitHub integration pipeline and updates frontend pipeline forwarding so only GitHub install redirects are forwarded when no pipeline exists. It also adds new error handling for invalid installation requests and extends tests to cover the new OAuth redirect/callback step. The highest risks to verify are (1) correctness and robustness of the OAuth callback exchange when required parameters like code are missing, and (2) whether the narrowed forwarding contract in pipeline_advancer.py unintentionally breaks other provider flows or edge cases. You should also verify that the new error rendering path and template context are consistent with existing expectations and that tests cover the OAuth state/user mismatch logic beyond the happy path.

🎯 Review Focus

Verify the OAuth callback correctness and failure handling in OAuthLoginView.dispatch()—specifically that required OAuth parameters (code, state) are validated before token exchange, that state/signature verification cannot be bypassed, and that the resulting user binding and pipeline advancement are consistent with the new forwarding contract when pipeline is None.

Key Findings

• ⚠️ [src/sentry/web/frontend/pipeline_advancer.py:L34] WARNING: Forwarding behavior was narrowed from a shared allowlist to provider_id == "github" only; if any other code paths relied on the previous FORWARD_INSTALL_FOR semantics, this can silently break install resumption for other providers or future additions—restore an allowlist or explicitly document/encode the contract across all callers.

✅ Action Checklist

• WARNING [src/sentry/web/frontend/pipeline_advancer.py:L34] — Forwarding behavior was narrowed from a shared allowlist to provider_id == "github" only; if any other code paths relied on the previous FORWARD_INSTALL_FOR semantics, this can silently break install resumption for other providers or future additions—restore an allowlist or explicitly document/encode the contract across all callers.
• SUGGESTION — In src/sentry/integrations/github/integration.py, add an explicit guard before building the token exchange payload and calling safe_urlopen/safe_urlread. Example:

code = request.GET.get("code")
if not code:
    return error(request, self.active_organization)

data = {
    "code": code,
    "client_id": github_client_id,
    "client_secret": github_client_secret,
}

Then narrow the try/except Exception to the specific failure modes you expect (network vs parse) so you don’t mask actionable debugging signals.

• SUGGESTION — In src/sentry/web/frontend/pipeline_advancer.py, reintroduce an explicit allowlist (or a named constant) rather than hard-coding provider_id == "github". If the intent is truly “only GitHub ever redirects without a pipeline,” encode that as a documented invariant and add a regression test that asserts other providers do not get forwarded (and that GitHub still does) to prevent future silent contract drift.

Suggestions

• In src/sentry/integrations/github/integration.py, add an explicit guard before building the token exchange payload and calling safe_urlopen/safe_urlread. Example:

code = request.GET.get("code")
if not code:
    return error(request, self.active_organization)

data = {
    "code": code,
    "client_id": github_client_id,
    "client_secret": github_client_secret,
}

Then narrow the try/except Exception to the specific failure modes you expect (network vs parse) so you don’t mask actionable debugging signals.

• In src/sentry/web/frontend/pipeline_advancer.py, reintroduce an explicit allowlist (or a named constant) rather than hard-coding provider_id == "github". If the intent is truly “only GitHub ever redirects without a pipeline,” encode that as a documented invariant and add a regression test that asserts other providers do not get forwarded (and that GitHub still does) to prevent future silent contract drift.

📝 This review includes 1 inline comment (1 warning)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

Issue: OAuth callback does not validate that code is present before exchanging it for an access token. data is built with "code": request.GET.get("code"), and then safe_urlopen is called regardless. If code is missing/empty, the token exchange may fail and the code falls back to payload = {} (broad exception), resulting in a generic error and potentially making debugging/abuse harder.

Fix: Add an explicit guard before exchanging:

code = request.GET.get("code")
if not code:
    return error(request, self.active_organization)

data = {
    "code": code,
    "client_id": github_client_id,
    "client_secret": github_client_secret,
}

Also narrow the exception handling around the exchange/parse so you can distinguish network vs parse errors.