fix(security): validate GitHub user during integration installation

[zaibkhan]

This PR hardens the GitHub OAuth installation flow by validating the GitHub user during integration setup.

Key changes:

• Validate the GitHub user against the installation to prevent state-parameter reuse.
• Ensure state is bound to the initiating user/session to block CSRF/state replay.
• Add defensive checks and clearer error handling along the OAuth callback.

Security impact:
Prevents cross-user installs and state-replay during OAuth app installation.

Benchmark reference:
Replicated from ai-code-review-evaluation/sentry-greptile PR #4

[codoki-pr-intelligence]

Codoki PR Review

Summary: Harden OAuth install flow, guard metadata access
What’s good: Introduces an OAuth login step with state validation and binds the installation to the authenticated GitHub user; centralizes error rendering and improves pipeline flow.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters	File
High	Correctness — Possible KeyError on missing sender.login in metadata	Direct dict access can raise KeyError if 'sender' or 'login' is missing in metadata (e.g., legacy records), causing a 500 during installation setup. Use safe access to fail closed without crashing.	…/github/integration.py

Key Feedback (click to expand)

• Needs improvement: The comparison uses a fragile dictionary access that can raise KeyError on older/atypical metadata and compares by login string instead of immutable user id.
• Testing: Consider adding a test where Integration.metadata lacks 'sender' or 'login' to ensure the flow fails safely (renders the error template) without a 500.
• Security: Comparing by 'login' is brittle if a GitHub user renames; prefer comparing immutable 'id' from get_user_info against metadata.sender.id to avoid false negatives and potential bypass if display names change.
• Open questions: Are there legacy GitHub integration records missing metadata.sender or sender.login? If so, how should we handle those installs (block with generic error vs. allow)?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant User
    participant PipelineAdvancer
    participant OAuthLoginView
    participant GitHubOAuth
    participant GitHubInstallation
    User->>PipelineAdvancer: GET /extensions/github/setup?installation_id=... (no state)
    PipelineAdvancer->>OAuthLoginView: pipeline.current_step()
    OAuthLoginView-->>User: 302 to GitHub authorize (state=signature)
    User->>PipelineAdvancer: GET ...?code=...&state=signature
    PipelineAdvancer->>OAuthLoginView: pipeline.current_step()
    alt state mismatch or token error
        OAuthLoginView-->>User: render error
    else success
        OAuthLoginView->>GitHubOAuth: POST /login/oauth/access_token
        OAuthLoginView->>GitHubOAuth: GET /user (access_token)
        OAuthLoginView-->>PipelineAdvancer: next_step()
        PipelineAdvancer->>GitHubInstallation: pipeline.current_step()
        alt pending deletion or org mismatch
            GitHubInstallation-->>User: render error
        else
            GitHubInstallation-->>User: next_step()
        end
    end

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Direct dict access can raise KeyError if 'sender' or 'login' is missing in metadata (e.g., legacy records), causing a 500 during installation setup. Use safe access to fail closed without crashing.

Suggested change

	!= integration.metadata["sender"]["login"]
	!= integration.metadata.get("sender", {}).get("login")