Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 129 additions & 31 deletions src/sentry/integrations/vsts/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,23 @@
import re
from collections.abc import Mapping, MutableMapping, Sequence
from time import time
from typing import Any
from typing import Any, TypedDict
from urllib.parse import parse_qs, quote, unquote, urlencode, urlparse

from django import forms
from django.http.request import HttpRequest
from django.http.response import HttpResponseBase
from django.utils.translation import gettext as _
from rest_framework.fields import CharField
from rest_framework.serializers import Serializer

from sentry import features, http
from sentry import features, http, options
from sentry.auth.exceptions import IdentityNotValid
from sentry.constants import ObjectStatus
from sentry.identity.oauth2 import OAuth2ApiStep
from sentry.identity.pipeline import IdentityPipeline
from sentry.identity.services.identity.model import RpcIdentity
from sentry.identity.vsts.provider import get_user_info
from sentry.identity.vsts.provider import VSTSNewIdentityProvider, get_user_info
from sentry.integrations.base import (
FeatureDescription,
IntegrationData,
Expand Down Expand Up @@ -47,7 +50,8 @@
from sentry.models.apitoken import generate_token
from sentry.models.repository import Repository
from sentry.organizations.services.organization.model import RpcOrganization
from sentry.pipeline.views.base import PipelineView
from sentry.pipeline.types import PipelineStepResult
from sentry.pipeline.views.base import ApiPipelineSteps, PipelineView
from sentry.pipeline.views.nested import NestedPipelineView
from sentry.shared_integrations.exceptions import (
ApiError,
Expand Down Expand Up @@ -135,6 +139,30 @@
logger = logging.getLogger("sentry.integrations")


def get_account_from_id(
account_id: str, accounts: Sequence[Mapping[str, Any]]
) -> Mapping[str, Any] | None:
for account in accounts:
if account["accountId"] == account_id:
return account
return None


def get_accounts(access_token: str, user_id: str) -> Any | None:
url = f"https://app.vssps.visualstudio.com/_apis/accounts?memberId={user_id}&api-version=4.1"
Comment thread
evanpurkhiser marked this conversation as resolved.
with http.build_session() as session:
response = session.get(
url,
headers={
"Content-Type": "application/json",
"Authorization": f"Bearer {access_token}",
},
)
if response.status_code == 200:
return response.json()

Check warning on line 162 in src/sentry/integrations/vsts/integration.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

JSONDecodeError not caught when parsing Azure DevOps API response

The `get_accounts` function calls `response.json()` at line 162 without catching `JSONDecodeError`. According to production data (SENTRY-5CKF: 30,593 events), Azure DevOps responses can be truncated or malformed even with a 200 status code. If the Azure DevOps API returns an incomplete or malformed JSON response, this will raise an uncaught `JSONDecodeError`, causing the integration setup to fail with a 500 error instead of a graceful user-facing message.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
return None


class VstsIntegration(RepositoryIntegration[VstsApiClient], VstsIssuesSpec):
logger = logger
comment_key = "sync_comments"
Expand Down Expand Up @@ -414,6 +442,74 @@
return None


class VstsAccountSelectionSerializer(Serializer):
account = CharField(required=True)


class VstsAccountStepData(TypedDict):
accountId: str
accountName: str


class VstsAccountSelectionStepData(TypedDict):
accounts: list[VstsAccountStepData]


class VstsAccountSelectionApiStep:
step_name = "account_selection"

def get_serializer_cls(self) -> type:
return VstsAccountSelectionSerializer

def get_step_data(
self, pipeline: IntegrationPipeline, request: HttpRequest
) -> VstsAccountSelectionStepData:
with IntegrationPipelineViewEvent(
IntegrationPipelineViewType.ACCOUNT_CONFIG,
IntegrationDomain.SOURCE_CODE_MANAGEMENT,
VstsIntegrationProvider.key,
).capture() as lifecycle:
oauth_data = pipeline.fetch_state("oauth_data") or {}
access_token: str = oauth_data["access_token"]

Check warning on line 473 in src/sentry/integrations/vsts/integration.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

KeyError when oauth_data is missing access_token key

Line 473 uses direct dict access `oauth_data["access_token"]` after setting `oauth_data = pipeline.fetch_state("oauth_data") or {}`. If the OAuth step didn't complete successfully or pipeline state is missing/corrupted (e.g., session timeout, cache eviction), `oauth_data` will be an empty dict and this line will raise `KeyError: 'access_token'`. This matches the VSTS JSONDecodeError pattern (SENTRY-5CKF) where missing/invalid state leads to unhandled exceptions during integration setup.
user = get_user_info(access_token)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unhandled None access_token crashes get_step_data

Medium Severity

In get_step_data, oauth_data.get("access_token") can return None, which is then passed to get_user_info. That function builds an auth header "bearer None" and calls raise_for_status() on the resulting 401 response, raising an unhandled HTTPError that surfaces as a 500 to the caller. Unlike the legacy AccountConfigView.dispatch (which shares this weakness), this new API-driven path has no intermediary error handling, so the raw exception propagates directly to the API endpoint. A guard or early error return before calling get_user_info would prevent the crash.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 12fdf4e. Configure here.


accounts = get_accounts(access_token, user["uuid"])
extra = {
"organization_id": pipeline.organization.id if pipeline.organization else None,
"user_id": request.user.id,
"accounts": accounts,
}

if not accounts or not accounts.get("value"):
lifecycle.record_halt(IntegrationPipelineHaltReason.NO_ACCOUNTS, extra=extra)
pipeline.bind_state("accounts", [])
return {"accounts": []}

accounts_list = accounts["value"]
pipeline.bind_state("accounts", accounts_list)
return {
"accounts": [
{"accountId": account["accountId"], "accountName": account["accountName"]}
for account in accounts_list
]
}

def handle_post(
self,
validated_data: dict[str, str],
pipeline: IntegrationPipeline,
request: HttpRequest,
) -> PipelineStepResult:
account_id = validated_data["account"]
state_accounts: Sequence[Mapping[str, Any]] | None = pipeline.fetch_state(key="accounts")
account = get_account_from_id(account_id, state_accounts or [])
if account is None:
return PipelineStepResult.error("Invalid Azure DevOps account")

pipeline.bind_state("account", account)
return PipelineStepResult.advance()


class VstsIntegrationProvider(IntegrationProvider):
key = IntegrationProviderSlug.AZURE_DEVOPS.value
name = "Azure DevOps"
Expand Down Expand Up @@ -486,8 +582,34 @@
AccountConfigView(),
]

def get_pipeline_api_steps(self) -> ApiPipelineSteps[IntegrationPipeline]:
return [
self._make_oauth_api_step(),
VstsAccountSelectionApiStep(),
]

def _make_oauth_api_step(self) -> OAuth2ApiStep:
provider = VSTSNewIdentityProvider(
oauth_scopes=sorted(self.get_scopes()),
redirect_url=absolute_uri(self.oauth_redirect_url),
)
extra_authorize_params = {"response_mode": "query"}
if options.get("vsts.consent-prompt"):
extra_authorize_params["prompt"] = "consent"

return provider.make_oauth_api_step(
bind_key="oauth_data",
extra_authorize_params=extra_authorize_params,
)

def build_integration(self, state: Mapping[str, Any]) -> IntegrationData:
data = state["identity"]["data"]
# TODO: legacy views write token data to state["identity"]["data"] via
# NestedPipelineView. API steps write directly to state["oauth_data"].
# Remove the legacy path once the old views are retired.
if "oauth_data" in state:
data = state["oauth_data"]
else:
data = state["identity"]["data"]
oauth_data = self.get_oauth_data(data)
account = state["account"]
user = get_user_info(data["access_token"])
Expand Down Expand Up @@ -679,7 +801,7 @@
state_accounts: Sequence[Mapping[str, Any]] | None = pipeline.fetch_state(
key="accounts"
)
account = self.get_account_from_id(account_id, state_accounts or [])
account = get_account_from_id(account_id, state_accounts or [])
if account is not None:
pipeline.bind_state("account", account)
return pipeline.next_step()
Expand All @@ -688,7 +810,7 @@
access_token = (state or {}).get("data", {}).get("access_token")
user = get_user_info(access_token)

accounts = self.get_accounts(access_token, user["uuid"])
accounts = get_accounts(access_token, user["uuid"])
extra = {
"organization_id": pipeline.organization.id if pipeline.organization else None,
"user_id": request.user.id,
Expand All @@ -710,30 +832,6 @@
request=request,
)

def get_account_from_id(
self, account_id: str, accounts: Sequence[Mapping[str, Any]]
) -> Mapping[str, Any] | None:
for account in accounts:
if account["accountId"] == account_id:
return account
return None

def get_accounts(self, access_token: str, user_id: int) -> Any | None:
url = (
f"https://app.vssps.visualstudio.com/_apis/accounts?memberId={user_id}&api-version=4.1"
)
with http.build_session() as session:
response = session.get(
url,
headers={
"Content-Type": "application/json",
"Authorization": f"Bearer {access_token}",
},
)
if response.status_code == 200:
return response.json()
return None


class AccountForm(forms.Form):
def __init__(self, accounts: Sequence[Mapping[str, str]], *args: Any, **kwargs: Any) -> None:
Expand Down
Loading
Loading