fix: require a matching Origin on consent and name the binding cookie per flow (SEC-17, SEC-19)
This commit is contained in:
1 parent
8be859243d
commit
782d416e46
5 files changed
+306
-19
No files matched your search
@@ -23,7 +23,7 @@ from starlette.responses import HTMLResponse, RedirectResponse, Response
|
||||
from starlette.routing import Route, request_response
|
||||
from starlette.types import ASGIApp, Receive, Scope, Send
|
||||
|
||||
from aegis_gitea_mcp.oauth.provider import CONSENT_PATH, GiteaOAuthProvider
|
||||
from aegis_gitea_mcp.oauth.provider import CONSENT_PATH, GiteaOAuthProvider, flow_tag
|
||||
from aegis_gitea_mcp.oauth.upstream import CALLBACK_PATH
|
||||
|
||||
COOKIE_TTL_SECONDS = 600
|
||||
@@ -64,7 +64,11 @@ class _Cookies:
|
||||
self.secure = urlsplit(base_url).scheme == "https"
|
||||
prefix = "__Host-" if self.secure else ""
|
||||
self.csrf = f"{prefix}aegis-oauth-csrf"
|
||||
self.binding = f"{prefix}aegis-oauth-flow"
|
||||
self._binding_prefix = f"{prefix}aegis-oauth-flow-"
|
||||
|
||||
def binding(self, tag: str) -> str:
|
||||
"""Name of the binding cookie of the flow with the given tag (one per Gitea state)."""
|
||||
return f"{self._binding_prefix}{tag}"
|
||||
|
||||
def set(self, response: Response, name: str, value: str, *, same_site: str) -> None:
|
||||
response.set_cookie(
|
||||
@@ -103,6 +107,26 @@ def _redirect(location: str, status_code: int) -> RedirectResponse:
|
||||
return RedirectResponse(location, status_code=status_code, headers=_SECURITY_HEADERS)
|
||||
|
||||
|
||||
def _request_origin(request: Request) -> str | None:
|
||||
"""Origin of a form POST: the ``Origin`` header, else the origin of ``Referer``.
|
||||
|
||||
Browsers send ``Origin`` on form POSTs. Without it (or with ``Origin: null``) the
|
||||
``Referer`` is the fallback; when neither names an origin the result is None
|
||||
and the request is refused.
|
||||
"""
|
||||
origin = request.headers.get("origin")
|
||||
if origin is not None:
|
||||
return origin
|
||||
referer = request.headers.get("referer")
|
||||
if not referer:
|
||||
return None
|
||||
try:
|
||||
parts = urlsplit(referer)
|
||||
except ValueError:
|
||||
return None
|
||||
return f"{parts.scheme}://{parts.netloc}" if parts.scheme and parts.netloc else None
|
||||
|
||||
|
||||
def _text(value: Any, limit: int = 200) -> str:
|
||||
return value[:limit] if isinstance(value, str) else ""
|
||||
|
||||
@@ -164,8 +188,7 @@ def build_browser_routes(provider: GiteaOAuthProvider) -> list[Route]:
|
||||
return response
|
||||
|
||||
async def consent_submit(request: Request) -> Response:
|
||||
origin = request.headers.get("origin")
|
||||
if origin is not None and origin != expected_origin:
|
||||
if _request_origin(request) != expected_origin:
|
||||
return _invalid()
|
||||
try:
|
||||
form = await request.form(
|
||||
@@ -183,21 +206,28 @@ def build_browser_routes(provider: GiteaOAuthProvider) -> list[Route]:
|
||||
return _invalid()
|
||||
response = _redirect(decision.redirect, 303)
|
||||
cookies.clear(response, cookies.csrf, same_site="strict")
|
||||
if decision.binding_token is not None:
|
||||
if decision.binding_token is not None and decision.flow_tag is not None:
|
||||
# Lax: the cookie must come back on Gitea's top-level redirect to us.
|
||||
cookies.set(response, cookies.binding, decision.binding_token, same_site="lax")
|
||||
cookies.set(
|
||||
response,
|
||||
cookies.binding(decision.flow_tag),
|
||||
decision.binding_token,
|
||||
same_site="lax",
|
||||
)
|
||||
return response
|
||||
|
||||
async def callback(request: Request) -> Response:
|
||||
params = request.query_params
|
||||
state = _text(params.get("state"))
|
||||
cookie_name = cookies.binding(flow_tag(state))
|
||||
location = await provider.complete_callback(
|
||||
_text(params.get("state")),
|
||||
_text(request.cookies.get(cookies.binding)),
|
||||
state,
|
||||
_text(request.cookies.get(cookie_name)),
|
||||
_text(params.get("code"), 4096),
|
||||
_text(params.get("error")),
|
||||
)
|
||||
response = _invalid() if location is None else _redirect(location, 302)
|
||||
cookies.clear(response, cookies.binding, same_site="lax")
|
||||
cookies.clear(response, cookie_name, same_site="lax")
|
||||
return response
|
||||
|
||||
return [
|
||||
|
||||
@@ -129,6 +129,16 @@ def same_resource(left: str, right: str) -> bool:
|
||||
return normalized[0] == normalized[1]
|
||||
|
||||
|
||||
def flow_tag(state: str) -> str:
|
||||
"""Eight hex characters derived from a Gitea ``state`` (names the browser-binding cookie).
|
||||
|
||||
Concurrent sign-ins in one browser each get their own cookie, so a second flow
|
||||
cannot overwrite the binding of the first. The tag is public (it is a hash
|
||||
prefix of a value that is in the callback URL anyway) and grants nothing.
|
||||
"""
|
||||
return token_hash(state)[:8]
|
||||
|
||||
|
||||
def s256(verifier: str) -> str:
|
||||
"""PKCE S256 challenge for ``verifier``."""
|
||||
digest = hashlib.sha256(verifier.encode("ascii")).digest()
|
||||
@@ -214,6 +224,8 @@ class ConsentDecision:
|
||||
|
||||
redirect: str = field(repr=False)
|
||||
binding_token: str | None = field(default=None, repr=False)
|
||||
#: Short public tag of the Gitea state, used to name this flow's binding cookie.
|
||||
flow_tag: str | None = None
|
||||
|
||||
|
||||
class GiteaOAuthProvider(
|
||||
@@ -438,7 +450,7 @@ class GiteaOAuthProvider(
|
||||
location = self._upstream.authorize_url(
|
||||
state=state, code_challenge=s256(pending.code_verifier)
|
||||
)
|
||||
return ConsentDecision(redirect=location, binding_token=binding)
|
||||
return ConsentDecision(redirect=location, binding_token=binding, flow_tag=flow_tag(state))
|
||||
|
||||
async def complete_callback(
|
||||
self, state: str, binding: str, code: str, error: str
|
||||
|
||||
Reference in new issue
Block a user