fix(security): escape error text in OIDC error pages
OIDC error responses interpolated parse-error/exception and error_description text straight into HTML. idpyoidc currently emits canned messages, but this is the same reflected-XSS class as the validation-error fix; relying on upstream not to echo input is fragile. Add a shared _error_page() helper that HTML-escapes the message and route all six dynamic error responses through it. Refs: porchlight-8iw Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
71a7c23bdd
commit
7c4dbf2cd9
2 changed files with 26 additions and 6 deletions
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import html
|
||||
import json
|
||||
from types import SimpleNamespace
|
||||
from urllib.parse import urlencode
|
||||
|
|
@ -14,6 +15,15 @@ from porchlight.oidc.claims import PorchlightUserInfo, user_to_claims
|
|||
|
||||
router = APIRouter(tags=["oidc"])
|
||||
|
||||
|
||||
def _error_page(message: object, status_code: int = 400, title: str = "Error") -> HTMLResponse:
|
||||
"""Render an error page, escaping the (possibly request-derived) message."""
|
||||
return HTMLResponse(
|
||||
f"<h1>{html.escape(title)}</h1><p>{html.escape(str(message))}</p>",
|
||||
status_code=status_code,
|
||||
)
|
||||
|
||||
|
||||
SCOPE_DESCRIPTIONS: dict[str, str] = {
|
||||
"openid": "Sign you in (required)",
|
||||
"profile": "Your name and profile information",
|
||||
|
|
@ -59,11 +69,11 @@ async def authorization(request: Request) -> Response:
|
|||
try:
|
||||
parsed = endpoint.parse_request(query_params)
|
||||
except Exception as exc:
|
||||
return HTMLResponse(f"<h1>Invalid Request</h1><p>{exc}</p>", status_code=400)
|
||||
return _error_page(exc, title="Invalid Request")
|
||||
|
||||
if "error" in parsed:
|
||||
error_desc = parsed.get("error_description", parsed["error"])
|
||||
return HTMLResponse(f"<h1>Error</h1><p>{error_desc}</p>", status_code=400)
|
||||
return _error_page(error_desc)
|
||||
|
||||
# Check if user is authenticated
|
||||
userid = request.session.get("userid")
|
||||
|
|
@ -95,11 +105,11 @@ async def authorization_complete(request: Request) -> Response:
|
|||
try:
|
||||
parsed = endpoint.parse_request(auth_request_params)
|
||||
except Exception as exc:
|
||||
return HTMLResponse(f"<h1>Error</h1><p>{exc}</p>", status_code=400)
|
||||
return _error_page(exc)
|
||||
|
||||
if "error" in parsed:
|
||||
error_desc = parsed.get("error_description", parsed["error"])
|
||||
return HTMLResponse(f"<h1>Error</h1><p>{error_desc}</p>", status_code=400)
|
||||
return _error_page(error_desc)
|
||||
|
||||
return await _check_consent_or_complete(
|
||||
request, oidc_server, endpoint, parsed, userid, username, auth_request_params
|
||||
|
|
@ -172,7 +182,7 @@ async def _complete_authorization( # noqa: PLR0913
|
|||
if "error" in result.get("response_args", {}):
|
||||
response_args = result["response_args"]
|
||||
error_desc = response_args.get("error_description", response_args["error"])
|
||||
return HTMLResponse(f"<h1>Error</h1><p>{error_desc}</p>", status_code=400)
|
||||
return _error_page(error_desc)
|
||||
|
||||
# Build redirect URL
|
||||
response_args = result.get("response_args", {})
|
||||
|
|
@ -361,6 +371,6 @@ async def consent_submit(request: Request) -> Response:
|
|||
if "error" in parsed:
|
||||
raise ValueError(parsed.get("error_description", parsed["error"]))
|
||||
except Exception as exc:
|
||||
return HTMLResponse(f"<h1>Error</h1><p>{exc}</p>", status_code=400)
|
||||
return _error_page(exc)
|
||||
|
||||
return await _complete_authorization(request, oidc_server, endpoint, parsed, userid, username)
|
||||
|
|
|
|||
|
|
@ -2,6 +2,16 @@ import secrets
|
|||
|
||||
from httpx import AsyncClient
|
||||
|
||||
from porchlight.oidc.endpoints import _error_page
|
||||
|
||||
|
||||
def test_error_page_escapes_html() -> None:
|
||||
# OIDC error pages must not interpolate request-derived text as raw HTML.
|
||||
resp = _error_page("<script>alert(1)</script>")
|
||||
body = resp.body.decode()
|
||||
assert "<script>" not in body
|
||||
assert "<script>alert(1)</script>" in body
|
||||
|
||||
|
||||
def _register_test_client(
|
||||
client: AsyncClient,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue