Skip to content
Jump to bottom

Automated Test: oauth-state-secure #312

Closed
wants to merge 1 commit into from
+247 −50
Closed

Automated Test: oauth-state-secure #312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
159 changes: 118 additions & 41 deletions src/sentry/integrations/github/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,19 @@
import re
from collections.abc import Collection, Mapping, Sequence
from typing import Any
from urllib.parse import parse_qsl

from django.http import HttpResponse
from django.urls import reverse
from django.utils.text import slugify
from django.utils.translation import gettext_lazy as _
from rest_framework.request import Request

from sentry import features, options
from sentry.api.utils import generate_organization_url
from sentry.constants import ObjectStatus
from sentry.http import safe_urlopen, safe_urlread
from sentry.identity.github import GitHubIdentityProvider, get_user_info
from sentry.integrations import (
FeatureDescription,
IntegrationFeatures,
Expand All @@ -35,6 +39,7 @@
from sentry.tasks.integrations.github.constants import RATE_LIMITED_MESSAGE
from sentry.tasks.integrations.link_all_repos import link_all_repos
from sentry.utils import metrics
from sentry.utils.http import absolute_uri
from sentry.web.helpers import render_to_response

from .client import GitHubAppsClient, GitHubClientMixin
Expand Down Expand Up @@ -108,6 +113,9 @@
ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG = _(
"It seems that your GitHub account has been installed on another Sentry organization. Please uninstall and try again."
)
ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST = _(
"We could not verify the authenticity of the installation request. We recommend restarting the installation process."
)
ERR_INTEGRATION_PENDING_DELETION = _(
"It seems that your Sentry organization has an installation pending deletion. Please wait ~15min for the uninstall to complete and try again."
)
Expand All @@ -118,6 +126,32 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str) -
return f"{account_type}:{name} {query}".encode()


def error(
request,
org,
error_short="Invalid installation request.",
error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST,
):
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
"error": error_long,
"payload": {
"success": False,
"data": {"error": _(error_short)},
},
"document_origin": get_document_origin(org),
},
request=request,
)


def get_document_origin(org) -> str:
if org and features.has("organizations:customer-domains", org.organization):
return f'"{generate_organization_url(org.organization.slug)}"'
return "document.origin"


# Github App docs and list of available endpoints
# https://docs.github.com/en/rest/apps/installations
# https://docs.github.com/en/rest/overview/endpoints-available-for-github-apps
Expand Down Expand Up @@ -307,7 +341,7 @@ def post_install(
)

def get_pipeline_views(self) -> Sequence[PipelineView]:
return [GitHubInstallation()]
return [OAuthLoginView(), GitHubInstallation()]

def get_installation_info(self, installation_id: str) -> Mapping[str, Any]:
client = self.get_client()
Expand Down Expand Up @@ -352,15 +386,72 @@ def setup(self) -> None:
)


class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
self.determine_active_organization(request)

ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()

installation_id = request.GET.get("installation_id")
if installation_id:
pipeline.bind_state("installation_id", installation_id)

if not request.GET.get("state"):
state = pipeline.signature

redirect_uri = absolute_uri(
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}"
)

# At this point, we are past the GitHub "authorize" step
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)

# similar to OAuth2CallbackView.get_token_params
data = {
"code": request.GET.get("code"),
"client_id": github_client_id,
"client_secret": github_client_secret,
}

# similar to OAuth2CallbackView.exchange_token
req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data)

try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except Exception:
payload = {}

if "access_token" not in payload:
return error(request, self.active_organization)

authenticated_user_info = get_user_info(payload["access_token"])
if "login" not in authenticated_user_info:
return error(request, self.active_organization)

pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"])
return pipeline.next_step()


class GitHubInstallation(PipelineView):
def get_app_url(self) -> str:
name = options.get("github-app.name")
return f"https://github.com/apps/{slugify(name)}"

def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
if "installation_id" not in request.GET:
installation_id = request.GET.get(
"installation_id", pipeline.fetch_state("installation_id")
)
if installation_id is None:
return self.redirect(self.get_app_url())

pipeline.bind_state("installation_id", installation_id)
Comment on lines 447 to +454
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how installation_id is used downstream after being bound to state
rg -n "installation_id" src/sentry/integrations/github/integration.py -C2

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1895

🏁 Script executed:

#!/bin/bash
# Look at build_integration method to see if there's validation handling
sed -n '351,380p' src/sentry/integrations/github/integration.py

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1371

Changed from if not installation_id to if installation_id is None — this allows empty strings to proceed.

request.GET.get("installation_id") returns None when absent but "" when present but empty. The old check (if not installation_id) rejected both None and "", while the new check only rejects None. An empty string would now:

  1. Bypass the redirect at line 451-452
  2. Bind an empty string to pipeline state (line 454)
  3. Reach get_installation_info() with an empty string, creating an invalid API call to /app/installations/ (line 348)

This is inconsistent with the pattern used earlier at line 398 (if installation_id:) and creates a behavioral regression. Either revert to the truthy check or explicitly validate that installation_id is non-empty.

🤖 Prompt for AI Agents 
In `@src/sentry/integrations/github/integration.py` around lines 447 - 454, The
dispatch method now only checks "if installation_id is None" which lets empty
strings through and binds them via pipeline.bind_state, causing downstream
get_installation_info calls to hit an invalid /app/installations/ endpoint;
change the check to reject empty strings (e.g., use a truthy check like "if not
installation_id" or explicitly validate installation_id.strip() is non-empty)
before redirecting, ensuring pipeline.fetch_state/pipeline.bind_state only see a
valid non-empty installation_id used by get_installation_info.

self.determine_active_organization(request)

integration_pending_deletion_exists = False
Expand All @@ -374,57 +465,43 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
).exists()

if integration_pending_deletion_exists:
document_origin = "document.origin"
if self.active_organization and features.has(
"organizations:customer-domains", self.active_organization.organization
):
document_origin = (
f'"{generate_organization_url(self.active_organization.organization.slug)}"'
)
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
"error": ERR_INTEGRATION_PENDING_DELETION,
"payload": {
"success": False,
"data": {"error": _("GitHub installation pending deletion.")},
},
"document_origin": document_origin,
},
request=request,
return error(
request,
self.active_organization,
error_short="GitHub installation pending deletion.",
error_long=ERR_INTEGRATION_PENDING_DELETION,
)

try:
# We want to limit GitHub integrations to 1 organization
installations_exist = OrganizationIntegration.objects.filter(
integration=Integration.objects.get(external_id=request.GET["installation_id"])
integration=Integration.objects.get(external_id=installation_id)
).exists()

except Integration.DoesNotExist:
pipeline.bind_state("installation_id", request.GET["installation_id"])
return pipeline.next_step()

if installations_exist:
document_origin = "document.origin"
if self.active_organization and features.has(
"organizations:customer-domains", self.active_organization.organization
):
document_origin = (
f'"{generate_organization_url(self.active_organization.organization.slug)}"'
)
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
"error": ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
"payload": {
"success": False,
"data": {"error": _("Github installed on another Sentry organization.")},
},
"document_origin": document_origin,
},
request=request,
return error(
request,
self.active_organization,
error_short="Github installed on another Sentry organization.",
error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
)

# OrganizationIntegration does not exist, but Integration does exist.
pipeline.bind_state("installation_id", request.GET["installation_id"])
try:
integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)
except Integration.DoesNotExist:
return error(request, self.active_organization)

# 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"]
):
return error(request, self.active_organization)
Comment on lines +493 to +505
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

integration.metadata["sender"] will KeyError if the integration was created before this change or via a path that doesn't set "sender".

build_integration only sets metadata["sender"] conditionally (Line 376–377: if state.get("sender")). Pre-existing integrations or those created through flows that don't populate sender in state will lack this key, causing a KeyError at Line 503.

🐛 Proposed fix — guard against missing "sender" metadata 
         # 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"]
-        ):
+        sender = integration.metadata.get("sender")
+        if not sender or (
+            pipeline.fetch_state("github_authenticated_user")
+            != sender.get("login")
+        ):
             return error(request, self.active_organization)
🤖 Prompt for AI Agents 
In `@src/sentry/integrations/github/integration.py` around lines 493 - 505, The
code assumes integration.metadata["sender"] always exists and will raise
KeyError for older integrations; update the check that compares
pipeline.fetch_state("github_authenticated_user") with
integration.metadata["sender"]["login"] to safely handle missing metadata by
using dict.get (e.g. integration.metadata.get("sender", {}) or retrieving sender
= integration.metadata.get("sender")) or by catching KeyError, and if sender or
sender["login"] is absent treat it as a mismatch and return error(request,
self.active_organization); ensure you reference Integration and
pipeline.fetch_state("github_authenticated_user") when making the change so the
comparison remains robust.


return pipeline.next_step()
11 changes: 4 additions & 7 deletions src/sentry/web/frontend/pipeline_advancer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@
PIPELINE_CLASSES = [IntegrationPipeline, IdentityProviderPipeline]


# GitHub apps may be installed directly from GitHub, in which case
# they will redirect here *without* being in the pipeline. If that happens
# redirect to the integration install org picker.
FORWARD_INSTALL_FOR = ["github"]


from rest_framework.request import Request


Expand All @@ -40,8 +34,11 @@ def handle(self, request: Request, provider_id: str) -> HttpResponseBase:
if pipeline:
break

# GitHub apps may be installed directly from GitHub, in which case
# they will redirect here *without* being in the pipeline. If that happens
# redirect to the integration install org picker.
if (
provider_id in FORWARD_INSTALL_FOR
provider_id == "github"
and request.GET.get("setup_action") == "install"
and pipeline is None
):
Expand Down