Sentry / PR #4

GitHub OAuth Security Enhancement (clean redo)

getsentry/sentry#67876
Golden recall: 66.7% Extra findings: 5

Golden Comments (2/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Null reference if github_authenticated_user state is missing MEDIUM × Missed FriendlyReviewer did not flag this specific case (missing state on github_authenticated_user).
#2 OAuth state uses pipeline.signature (static) instead of a per-request random value MEDIUM ✓ Found FriendlyReviewer: "deterministic MD5 hash of pipeline view class names, not a random per-request nonce" (integration.py L364). Over-classified to HIGH.
#3 integration.metadata[sender][login] without checking sender → KeyError ✓ Found FriendlyReviewer found a deeper issue: a race condition where ensure_integration removes sender from metadata entirely. Proposes metadata.get('sender', {}).get('login').

Supplementary Findings (9 findings)

Finding File Severity Legitimate?
Race condition: ensure_integration can remove sender from metadata integration.py:495 MEDIUM ✓ Same finding as golden #3, deeper analysis of root cause
installation_id not validated → URL with "None" (pipeline_advancer) pipeline_advancer.py:45 MEDIUM ✓ Real bug
Inconsistent error handling: broad OAuth token catch vs get_user_info without try/except integration.py:388 MEDIUM ✓ Robustness issue
Test test_installation_not_found no longer tests what it claims test_integration.py:385 MEDIUM ✓ Test regression
Fragile assertion: responses.calls[2] depends on fixed call order test_integration.py:262 MEDIUM ✓ Fragility
Test test_basic_flow no longer asserts new OAuth calls test_integration.py:281 LOW ✓ Acceptable
Pipeline leaked in test_github_user_mismatch test_integration.py:416 LOW ✓ Acceptable
Missing assertion on redirect status code test_integration.py:366 LOW ✓ Acceptable
Org resolution fragile in multi-org / custom domains integration.py:354 LOW ✓ Acceptable

Honest Analysis

66.7% golden recall — FriendlyReviewer missed #1 (null reference on missing github_authenticated_user state). The original golden considered this a Medium bug, and it's a real crash path, not an edge case: if GitHub's OAuth callback lacks the user identity, the code crashes with an unhandled TypeError. FriendlyReviewer reviewed the OAuth flow thoroughly but simply didn't trace through the response parsing path that would have hit it.

The supplementary findings include 5 LOW-severity items out of 9 total (55% noise). The test regression, fragile assertion, missing redirect assertion, leaked pipeline fixture — these are all real issues, but they're the kind of nitpicks any linting tool or human reviewer would produce in quantity. The real signal is the 2 HIGH and 2 MEDIUM findings: the race condition in ensure_integration (which is genuinely deeper than golden #3), the installation_id validation gap, and the inconsistent error handling.

The race condition finding is the standout: FriendlyReviewer connected the metadata access pattern to the ensure_integration method that runs earlier and can remove the sender key. That's a non-trivial cross-method reasoning that most surface-level static analysis would miss.

But the missed golden #1 is a reminder that FriendlyReviewer's coverage isn't comprehensive even on a small, clean diff (4 files, 250 lines). It still has blind spots in following data flow through response parsing and error handling paths.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

☁️ Tech
2 high, 4 medium, 4 low

This PR enhances GitHub OAuth security by adding state validation and proper user identity verification. The changes look mostly safe, but there are several correctness and robustness issues worth addressing, particularly around the integration metadata access pattern, the pipeline signature used as OAuth state, and testing gaps.

These might need a close look

  • 🔴 src/sentry/integrations/github/integration.py (L364)
    HIGH: The OAuth state parameter is set to pipeline.signature, which is a deterministic MD5 hash of the pipeline view class names. This is not a random per-request nonce, making CSRF protection weaker than expected. Consider using secrets.token_urlsafe(32) stored in the session, or at minimum pipeline.signature + secrets.token_hex(16) to add per-request entropy.
  • 🔴 src/sentry/integrations/github/integration.py (L495)
    HIGH: integration.metadata["sender"]["login"] is accessed without checking if "sender" exists in the metadata dictionary. The ensure_integration method (called earlier in the flow) can actually remove the "sender" key from metadata when it reinstalls the integration, which means this access can raise KeyError in production. Use integration.metadata.get("sender", {}).get("login") as a safe access pattern to avoid this.

Worth checking

  • 🟡 src/sentry/integrations/github/pipeline_advancer.py (L45)
    MEDIUM: installation_id is passed directly into a URL template without validation. If installation_id is None or missing, the resulting URL will contain the literal string "None", leading to confusing 404 errors downstream. Validate that installation_id is present and non-null before constructing the URL.
  • 🟡 src/sentry/integrations/github/integration.py (L388)
    MEDIUM: The OAuth token exchange uses a broad except Exception catch, while get_user_info (called immediately after) has no try/except at all. This asymmetry means network errors during user info retrieval will propagate as unhandled 500 errors, while token exchange errors are silently swallowed. Either wrap both calls consistently in error handling, or let both propagate to a global handler.
  • 🟡 tests/sentry/integrations/github/test_integration.py (L385)
    MEDIUM: The test test_installation_not_found no longer tests what its name claims — the installation-not-found path is handled differently in the new code and the test passes for the wrong reason, creating a false sense of coverage.
  • 🟡 tests/sentry/integrations/github/test_integration.py (L262)
    MEDIUM: responses.calls[2] is a fragile assertion that depends on a fixed call order. Adding or reordering mocked responses will silently break this test. Use a more explicit assertion that checks for the expected call by its URL or body content.

Small things (take or leave)

  • 🔵 tests/sentry/integrations/github/test_integration.py (L281)
    LOW: test_basic_flow no longer asserts the new OAuth calls are made — the test should verify that the full OAuth flow (state generation, token exchange, user info retrieval) completes correctly.
  • 🔵 tests/sentry/integrations/github/test_integration.py (L416)
    LOW: A pipeline fixture is leaked into test_github_user_mismatch — the test creates a pipeline but doesn't clean it up, which can pollute subsequent tests.
  • 🔵 tests/sentry/integrations/github/test_integration.py (L366)
    LOW: Missing assertion on the redirect status code. The test checks the redirect URL but never verifies that the HTTP response code is 302.
  • 🔵 src/sentry/integrations/github/integration.py (L354)
    LOW: Org resolution is fragile in multi-org or custom domain setups — the logic assumes a single org mapping, which may resolve incorrectly for enterprise customers with custom GitHub domains.