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
66 changes: 65 additions & 1 deletion src/sentry/api/endpoints/api_application_details.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from urllib.parse import urlparse

from django.db import router, transaction
from rest_framework import serializers
from rest_framework.authentication import SessionAuthentication
Expand All @@ -14,9 +16,71 @@
from sentry.models.apiapplication import ApiApplication, ApiApplicationStatus


class CustomSchemeURLField(serializers.CharField):
"""URLField that only allows specific safe schemes for OAuth redirect URIs.

Uses an allowlist approach for maximum security, only permitting:
- http/https: Standard web protocols
- sentry-mobile-agent: Sentry's custom mobile app scheme
"""

# Only these schemes are allowed for OAuth redirect URIs
ALLOWED_SCHEMES = {
"http", # Standard HTTP
"https", # Secure HTTP
"sentry-mobile-agent", # Sentry mobile app custom scheme
}

default_error_messages = {
"invalid": "Enter a valid URL.",
"disallowed_scheme": "This URL scheme is not allowed. Only http, https, and sentry-mobile-agent schemes are permitted.",
}

def run_validation(self, data):
# First run the standard CharField validations
data = super().run_validation(data)

if data is None:
return data

if data == "":
self.fail("invalid")
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: URL Field Ignores Blank Allowance

The CustomSchemeURLField unconditionally rejects empty strings, even when allow_blank=True is configured. This bypasses the intended allow_blank behavior, causing validation to fail unexpectedly for empty inputs.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, we don't want to have allow_blank=True, since we do want to require a nonempty value for this field.


# Basic URL structure validation
try:
parsed = urlparse(data)
if not parsed.scheme:
self.fail("invalid")

# Check if the scheme is in the allowlist
scheme_lower = parsed.scheme.lower()
if scheme_lower not in self.ALLOWED_SCHEMES:
self.fail("disallowed_scheme")

# All URIs must use :// format (not just :/)
# This ensures proper URI structure: scheme://netloc or scheme://path
if "://" not in data:
self.fail("invalid")

# For http/https, netloc is required (must have a domain)
if scheme_lower in {"http", "https"}:
if not parsed.netloc:
self.fail("invalid")
else:
# Custom schemes must have netloc (the part after ://)
# This ensures sentry-mobile-agent://callback is valid
# but sentry-mobile-agent:// is not
if not parsed.netloc:
self.fail("invalid")
except Exception:
self.fail("invalid")

return data


class ApiApplicationSerializer(serializers.Serializer):
name = serializers.CharField(max_length=64)
redirectUris = ListField(child=serializers.URLField(max_length=255), required=False)
redirectUris = ListField(child=CustomSchemeURLField(max_length=255), required=False)
allowedOrigins = ListField(
# TODO(dcramer): make this validate origins
child=serializers.CharField(max_length=255),
Expand Down
90 changes: 90 additions & 0 deletions tests/sentry/api/endpoints/test_api_application_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,96 @@ def test_simple(self) -> None:
app = ApiApplication.objects.get(id=app.id)
assert app.name == "foobaz"

def test_redirect_uris_with_allowed_schemes(self) -> None:
app = ApiApplication.objects.create(owner=self.user, name="a")

self.login_as(self.user)
url = reverse("sentry-api-0-api-application-details", args=[app.client_id])

# Test allowed schemes - all must use :// format
allowed_uris = [
"http://example.com/callback",
"https://example.com/callback",
"sentry-mobile-agent://callback",
"sentry-mobile-agent://callback/path",
]

response = self.client.put(url, data={"redirectUris": allowed_uris})
assert response.status_code == 200, (response.status_code, response.content)

app = ApiApplication.objects.get(id=app.id)
saved_uris = app.get_redirect_uris()
assert len(saved_uris) == 4
assert "http://example.com/callback" in saved_uris
assert "https://example.com/callback" in saved_uris
assert "sentry-mobile-agent://callback" in saved_uris
assert "sentry-mobile-agent://callback/path" in saved_uris

def test_invalid_redirect_uris_rejected(self) -> None:
app = ApiApplication.objects.create(owner=self.user, name="a")

self.login_as(self.user)
url = reverse("sentry-api-0-api-application-details", args=[app.client_id])

# Test invalid URIs
invalid_uris = [
"not-a-url", # No scheme or netloc
"://missing-scheme.com", # No scheme
"http://", # http with no domain
"https://", # https with no domain
"sentry-mobile-agent://", # Custom scheme with no content after ://
"sentry-mobile-agent:", # Missing ://
"sentry-mobile-agent:/callback", # Single slash format not allowed
"", # Empty string should be rejected
]

response = self.client.put(url, data={"redirectUris": invalid_uris})
assert response.status_code == 400, (response.status_code, response.content)
assert "redirectUris" in response.data

def test_disallowed_schemes_rejected(self) -> None:
app = ApiApplication.objects.create(owner=self.user, name="a")

self.login_as(self.user)
url = reverse("sentry-api-0-api-application-details", args=[app.client_id])

# Test schemes that are not in the allowlist
disallowed_uris = [
# Dangerous schemes
"javascript:alert('xss')",
"vbscript:msgbox('xss')",
"data:text/html,<script>alert('xss')</script>",
"file:///etc/passwd",
# Database connections
"jdbc:mysql://localhost:3306/db",
"odbc:DSN=myDataSource",
# Administrative/system
"admin://system",
"rdar://problem/12345",
"shortcuts://run-shortcut?name=test",
# Communication
"mailto:test@example.com",
"tel:+1234567890",
"sms:+1234567890",
# File transfer
"ftp://example.com/file",
"ftps://example.com/file",
"sftp://example.com/file",
# Custom schemes not in allowlist
"myapp://callback",
"com.example.app://auth",
"custom-scheme://redirect",
]

for disallowed_uri in disallowed_uris:
response = self.client.put(url, data={"redirectUris": [disallowed_uri]})
assert response.status_code == 400, (
f"Expected {disallowed_uri} to be rejected",
response.status_code,
response.content,
)
assert "redirectUris" in response.data


@control_silo_test
class ApiApplicationDeleteTest(APITestCase):
Expand Down
Loading