fix: add OAUTH_REDIRECT_ALLOWLIST for client registration and waive the open default (SEC-05)
This commit is contained in:
1 parent
79aa4f7fc0
commit
8be859243d
7 files changed
+283
-11
No files matched your search
@@ -88,6 +88,64 @@ def _split_list(value: Any) -> Any:
|
||||
return value
|
||||
|
||||
|
||||
def redirect_allowlist_entry_problem(entry: str) -> str | None:
|
||||
"""Return why ``entry`` cannot be an ``OAUTH_REDIRECT_ALLOWLIST`` item, or None when it can.
|
||||
|
||||
An item is an exact redirect URI (``https://client.example/cb``) or a prefix
|
||||
that ends in ``/`` (``https://claude.ai/``, which allows every path on that
|
||||
host). A bare origin without a path is refused as ambiguous.
|
||||
"""
|
||||
if any(ch.isspace() or not ch.isprintable() for ch in entry):
|
||||
return "whitespace or control characters"
|
||||
try:
|
||||
parts = urlsplit(entry)
|
||||
parts.port # noqa: B018 (accessing .port validates it)
|
||||
except ValueError:
|
||||
return "not a valid URL"
|
||||
host = parts.hostname or ""
|
||||
if not host:
|
||||
return "no host"
|
||||
if parts.username is not None or parts.password is not None:
|
||||
return "credentials in the URL"
|
||||
if parts.query or parts.fragment or "?" in entry or "#" in entry:
|
||||
return "query string or fragment"
|
||||
if parts.scheme == "http" and not is_loopback_host(host):
|
||||
return "http is only allowed for loopback hosts"
|
||||
if parts.scheme not in {"http", "https"}:
|
||||
return "scheme must be https (or http for loopback)"
|
||||
if not parts.path:
|
||||
return f"no path: write {entry}/ to allow the whole host or give the full callback URL"
|
||||
return None
|
||||
|
||||
|
||||
def redirect_matches_allowlist(uri: str, allowlist: list[str]) -> bool:
|
||||
"""True when ``uri`` equals an allowlist item or lies under one of its ``/`` prefixes.
|
||||
|
||||
An empty allowlist allows every acceptable redirect URI. The comparison of a
|
||||
prefix is by scheme, host (case-insensitive) and path prefix, so
|
||||
``https://claude.ai/`` never matches ``https://claude.ai.evil.example/``.
|
||||
"""
|
||||
if not allowlist:
|
||||
return True
|
||||
try:
|
||||
target = urlsplit(uri)
|
||||
except ValueError:
|
||||
return False
|
||||
for entry in allowlist:
|
||||
if uri == entry:
|
||||
return True
|
||||
if not entry.endswith("/"):
|
||||
continue
|
||||
prefix = urlsplit(entry)
|
||||
if (
|
||||
target.scheme == prefix.scheme
|
||||
and (target.netloc.lower() == prefix.netloc.lower())
|
||||
and target.path.startswith(prefix.path)
|
||||
):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _normalize_http_url(value: str, *, field: str) -> str:
|
||||
"""Validate an http(s) URL and return it without query, fragment or trailing slash."""
|
||||
candidate = value.strip()
|
||||
@@ -142,6 +200,7 @@ class Settings(BaseSettings):
|
||||
oauth_scopes: Annotated[list[str], NoDecode] = Field(
|
||||
default_factory=lambda: DEFAULT_OAUTH_SCOPES.split(",")
|
||||
)
|
||||
oauth_redirect_allowlist: Annotated[list[str], NoDecode] = Field(default_factory=list)
|
||||
|
||||
# State
|
||||
data_dir: Path = Field(default_factory=default_data_dir)
|
||||
@@ -250,7 +309,14 @@ class Settings(BaseSettings):
|
||||
)
|
||||
return value
|
||||
|
||||
@field_validator("allowed_hosts", "oauth_scopes", "toolsets", "disabled_tools", mode="before")
|
||||
@field_validator(
|
||||
"allowed_hosts",
|
||||
"oauth_scopes",
|
||||
"oauth_redirect_allowlist",
|
||||
"toolsets",
|
||||
"disabled_tools",
|
||||
mode="before",
|
||||
)
|
||||
@classmethod
|
||||
def _parse_list(cls, value: Any) -> Any:
|
||||
"""Accept comma or whitespace separated strings for list settings."""
|
||||
@@ -271,6 +337,18 @@ class Settings(BaseSettings):
|
||||
raise ValueError("OAUTH_SCOPES must not be empty")
|
||||
return list(dict.fromkeys(value))
|
||||
|
||||
@field_validator("oauth_redirect_allowlist")
|
||||
@classmethod
|
||||
def _validate_redirect_allowlist(cls, value: list[str]) -> list[str]:
|
||||
"""Accept exact redirect URIs and ``https://host/`` prefixes, nothing looser."""
|
||||
checked: list[str] = []
|
||||
for entry in value:
|
||||
problem = redirect_allowlist_entry_problem(entry)
|
||||
if problem is not None:
|
||||
raise ValueError(f"OAUTH_REDIRECT_ALLOWLIST entry is not acceptable: {problem}")
|
||||
checked.append(entry)
|
||||
return list(dict.fromkeys(checked))
|
||||
|
||||
@field_validator("toolsets")
|
||||
@classmethod
|
||||
def _validate_toolsets(cls, value: list[str]) -> list[str]:
|
||||
|
||||
@@ -59,7 +59,7 @@ from pydantic import AnyHttpUrl, ValidationError
|
||||
|
||||
from aegis_gitea_mcp.audit import AuditLogger, get_audit_logger
|
||||
from aegis_gitea_mcp.auth import MCP_SCOPE
|
||||
from aegis_gitea_mcp.config import Settings
|
||||
from aegis_gitea_mcp.config import Settings, redirect_matches_allowlist
|
||||
from aegis_gitea_mcp.oauth.store import (
|
||||
ClientRecord,
|
||||
OAuthStore,
|
||||
@@ -87,6 +87,7 @@ CONSENT_PATH = "/oauth/consent"
|
||||
|
||||
_LOOPBACK_REDIRECT_HOSTS = frozenset({"localhost", "127.0.0.1", "::1"})
|
||||
_CLIENT_AUTH_METHODS = frozenset({"none", "client_secret_post", "client_secret_basic"})
|
||||
_NOT_ALLOWED = "redirect URI is not on this server's allowlist (OAUTH_REDIRECT_ALLOWLIST)"
|
||||
_REDIRECT_RULE = (
|
||||
"redirect URIs must use https://, or http:// on localhost, 127.0.0.1 or [::1], "
|
||||
"without credentials or a fragment"
|
||||
@@ -96,8 +97,12 @@ V = TypeVar("V")
|
||||
Clock = Callable[[], float]
|
||||
|
||||
|
||||
def redirect_uri_problem(uri: str) -> str | None:
|
||||
"""Return why ``uri`` is not an acceptable redirect URI, or None when it is."""
|
||||
def redirect_uri_problem(uri: str, allowlist: list[str] | None = None) -> str | None:
|
||||
"""Return why ``uri`` is not an acceptable redirect URI, or None when it is.
|
||||
|
||||
With a non-empty ``allowlist`` an https URI must also match it; http loopback
|
||||
URIs (native clients on the user's own machine) stay allowed.
|
||||
"""
|
||||
try:
|
||||
parts = urlsplit(uri)
|
||||
parts.port # noqa: B018 (accessing .port validates it)
|
||||
@@ -108,10 +113,10 @@ def redirect_uri_problem(uri: str) -> str | None:
|
||||
host = (parts.hostname or "").lower()
|
||||
if not host:
|
||||
return _REDIRECT_RULE
|
||||
if parts.scheme == "https":
|
||||
return None
|
||||
if parts.scheme == "http" and host in _LOOPBACK_REDIRECT_HOSTS:
|
||||
return None
|
||||
if parts.scheme == "https":
|
||||
return None if redirect_matches_allowlist(uri, allowlist or []) else _NOT_ALLOWED
|
||||
return _REDIRECT_RULE
|
||||
|
||||
|
||||
@@ -306,7 +311,8 @@ class GiteaOAuthProvider(
|
||||
async def register_client(self, client_info: OAuthClientInformationFull) -> None:
|
||||
"""Validate and persist a dynamically registered client.
|
||||
|
||||
Redirect URIs must be https, or http on a loopback host. When
|
||||
Redirect URIs must be https, or http on a loopback host, and match
|
||||
``OAUTH_REDIRECT_ALLOWLIST`` when that is set. When
|
||||
:data:`MAX_CLIENTS` is reached, the least recently used client without
|
||||
any session is evicted; when every client has a session, registration
|
||||
is refused.
|
||||
@@ -320,7 +326,7 @@ class GiteaOAuthProvider(
|
||||
"invalid_redirect_uri", f"between 1 and {MAX_REDIRECT_URIS} redirect URIs required"
|
||||
)
|
||||
for uri in uris:
|
||||
problem = redirect_uri_problem(uri)
|
||||
problem = redirect_uri_problem(uri, self._settings.oauth_redirect_allowlist)
|
||||
if problem is not None:
|
||||
raise RegistrationError("invalid_redirect_uri", problem)
|
||||
if client_info.token_endpoint_auth_method not in _CLIENT_AUTH_METHODS:
|
||||
@@ -368,6 +374,12 @@ class GiteaOAuthProvider(
|
||||
"""
|
||||
if params.resource is not None and not same_resource(params.resource, self.resource):
|
||||
raise AuthorizeError("invalid_target", "unknown resource")
|
||||
# A client registered before the allowlist was set must not slip through it.
|
||||
if (
|
||||
redirect_uri_problem(str(params.redirect_uri), self._settings.oauth_redirect_allowlist)
|
||||
is not None
|
||||
):
|
||||
raise AuthorizeError("invalid_request", "redirect URI is not allowed")
|
||||
request_id = secrets.token_urlsafe(32)
|
||||
pending = PendingAuthorization(
|
||||
client_id=client.client_id,
|
||||
|
||||
@@ -44,6 +44,11 @@ GITEA_OAUTH_CLIENT_SECRET=
|
||||
# Encrypts stored user tokens. At least 32 characters. Generate with:
|
||||
# openssl rand -hex 32
|
||||
TOKEN_ENCRYPTION_KEY=
|
||||
# Optional allowlist of redirect URIs clients may register (comma separated): exact
|
||||
# URIs, or https://host/ prefixes. Empty accepts any https URI (and http on
|
||||
# loopback); the consent page is then the only check. For a public deployment list the
|
||||
# hosted clients you use, e.g. https://claude.ai/,https://chat.mistral.ai/
|
||||
OAUTH_REDIRECT_ALLOWLIST=
|
||||
# Granular Gitea scopes only (read:/write: activitypub, admin, issue, misc,
|
||||
# notification, organization, package, repository, user). Unknown values are rejected.
|
||||
OAUTH_SCOPES=write:activitypub,write:admin,write:issue,write:misc,write:notification,write:organization,write:package,write:repository,write:user
|
||||
|
||||
Reference in new issue
Block a user