From 3cbae6255bc437ca2d87518c5db2c15fe50e8162 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 09:39:25 +0200 Subject: [PATCH 01/15] fix: prevent admin user table from overflowing its card The 6-column user table could not shrink below its content width at the 40rem main column, so the Created column was clipped past the card border. Tighten cell padding to fit all columns, and add overflow-x: auto on the table container as a safety net for longer content. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/static/style.css | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/porchlight/static/style.css b/src/porchlight/static/style.css index 17319ff..15dc79e 100644 --- a/src/porchlight/static/style.css +++ b/src/porchlight/static/style.css @@ -205,6 +205,10 @@ main { line-height: 1; } +#user-table-container { + overflow-x: auto; +} + .admin-table { width: 100%; border-collapse: collapse; @@ -214,7 +218,7 @@ main { .admin-table th, .admin-table td { text-align: left; - padding: var(--sp-2) var(--sp-3); + padding: var(--sp-2); border-bottom: 1px solid var(--border); } From 437ad596581f6c4f40154939e5bb5d282b0f6139 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 09:53:13 +0200 Subject: [PATCH 02/15] feat: add Profile link to admin nav The admin nav had no way back to the account management area. Add a Profile link to /manage/profile, mirroring the Admin link already present in the manage nav. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/templates/admin/base.html | 1 + 1 file changed, 1 insertion(+) diff --git a/src/porchlight/templates/admin/base.html b/src/porchlight/templates/admin/base.html index 686a815..4a82870 100644 --- a/src/porchlight/templates/admin/base.html +++ b/src/porchlight/templates/admin/base.html @@ -4,6 +4,7 @@ {% block admin_content %}{% endblock %} From c52778326e98f251cc0a9d152b13daabf56c72b4 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 10:23:32 +0200 Subject: [PATCH 03/15] fix(security): escape user input in validation error HTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit format_validation_errors interpolated Pydantic error messages directly into HTML. Some messages echo user input (e.g. "Invalid group name ''"), so a crafted group name was reflected as raw HTML — a stored/reflected XSS. HTML-escape each formatted message before interpolation. Refs: porchlight-due Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/validation.py | 5 ++++- tests/test_validation.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/porchlight/validation.py b/src/porchlight/validation.py index 5d3b48c..e675e0b 100644 --- a/src/porchlight/validation.py +++ b/src/porchlight/validation.py @@ -1,3 +1,4 @@ +import html import re from typing import Annotated from urllib.parse import urlparse @@ -166,7 +167,9 @@ def format_validation_errors(exc: ValidationError) -> str: display_msg = f"{label}: {raw}" else: display_msg = f"{label}: {msg}" - messages.append(display_msg) + # Escape: messages can echo user input (e.g. an invalid group name), + # and the result is interpolated into HTML below. + messages.append(html.escape(display_msg)) if len(messages) == 1: return f'
{messages[0]}
' diff --git a/tests/test_validation.py b/tests/test_validation.py index 2be1afc..93bf2ea 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -292,3 +292,15 @@ class TestFormatValidationErrors: result = format_validation_errors(exc) assert "Picture URL must be a valid HTTP or HTTPS URL" in result assert "Value error" not in result + + def test_escapes_user_input_in_error_message(self) -> None: + # A validation error message that echoes user input (e.g. an invalid + # group name) must not emit raw HTML — otherwise it is reflected XSS. + payload = "" + try: + GroupListInput(groups=payload) + except ValidationError as exc: + result = format_validation_errors(exc) + assert payload not in result + assert "<img src=x onerror=alert(1)>" in result + assert " Date: Thu, 4 Jun 2026 10:26:55 +0200 Subject: [PATCH 04/15] fix(security): reject consent scopes outside the original request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /consent POST handler trusted the scope values submitted in the form, so a forged consent submission could approve (and persist consent for) scopes that were never part of the originating authorization request — a scope-escalation vector. Intersect the submitted scopes with the originally requested set stored in the session before saving consent and completing the flow. Refs: porchlight-a03 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/oidc/endpoints.py | 6 ++++-- tests/test_oidc/test_consent_flow.py | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/porchlight/oidc/endpoints.py b/src/porchlight/oidc/endpoints.py index 67d4968..8bfca41 100644 --- a/src/porchlight/oidc/endpoints.py +++ b/src/porchlight/oidc/endpoints.py @@ -338,8 +338,10 @@ async def consent_submit(request: Request) -> Response: return RedirectResponse(f"{redirect_uri}?{params}", status_code=303) return HTMLResponse("

Error

Invalid action

", status_code=400) - # Allow — collect approved scopes - approved_scopes: list[str] = [str(s) for s in form.getlist("scope")] + # Allow — collect approved scopes, rejecting anything outside the + # originally requested set (a forged form must not escalate scope). + requested_scopes = set(auth_params.get("scope", "openid").split()) + approved_scopes: list[str] = [str(s) for s in form.getlist("scope") if str(s) in requested_scopes] if "openid" not in approved_scopes: approved_scopes = ["openid", *list(approved_scopes)] diff --git a/tests/test_oidc/test_consent_flow.py b/tests/test_oidc/test_consent_flow.py index 91d3d73..9b40006 100644 --- a/tests/test_oidc/test_consent_flow.py +++ b/tests/test_oidc/test_consent_flow.py @@ -223,6 +223,30 @@ async def test_partial_consent_filters_scopes(client: AsyncClient) -> None: assert set(consent.scopes) == {"openid", "profile"} +async def test_consent_rejects_scopes_outside_request(client: AsyncClient) -> None: + """A forged consent POST cannot approve scopes that were not requested.""" + app = client._transport.app # type: ignore[union-attr] + _register_test_rp(app) + await _create_test_user(app) + + # Only openid + profile were requested; "email" is forged into the POST. + await _login_and_start_auth(client, scope="openid profile") + token = await get_csrf_token(client) + res = await client.post( + "/consent", + data={"action": "allow", "scope": ["openid", "profile", "email"]}, + headers={"X-CSRF-Token": token}, + follow_redirects=False, + ) + assert res.status_code == 303 + + consent_repo = app.state.consent_repo + consent = await consent_repo.get_consent("lusab-consent", "consent-rp") + assert consent is not None + assert "email" not in consent.scopes + assert set(consent.scopes) == {"openid", "profile"} + + # -- Test helpers -- From 91a22776647cfe1292cf7dc3bbf2861b1b1b0358 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 10:36:18 +0200 Subject: [PATCH 05/15] fix(security): store only a hash of magic-link tokens Magic-link tokens were persisted in plaintext, so a database read disclosed usable login/invite tokens. The service now hashes tokens (HMAC-SHA256 when a pepper is configured, else SHA-256 of the high-entropy token) and persists only the hash; the raw token is exposed solely in the registration URL and is re-attached to objects returned to callers. Refs: porchlight-42h Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/invite/service.py | 41 +++++++--- .../test_register_magic_link.py | 74 ++++++------------- tests/test_invite/test_service.py | 12 +++ 3 files changed, 65 insertions(+), 62 deletions(-) diff --git a/src/porchlight/invite/service.py b/src/porchlight/invite/service.py index fd5bebf..b8b5ef8 100644 --- a/src/porchlight/invite/service.py +++ b/src/porchlight/invite/service.py @@ -1,3 +1,5 @@ +import hashlib +import hmac import secrets from datetime import UTC, datetime, timedelta @@ -6,11 +8,23 @@ from porchlight.store.protocols import MagicLinkRepository class MagicLinkService: - """Magic link token lifecycle management.""" + """Magic link token lifecycle management. - def __init__(self, repo: MagicLinkRepository, ttl: int = 86400) -> None: + The raw token is only ever exposed to the user (in the registration URL). + Only a hash of the token is persisted, so a database disclosure does not + yield usable tokens. A pepper, when configured, keys the hash (HMAC); + otherwise a plain SHA-256 of the high-entropy token is stored. + """ + + def __init__(self, repo: MagicLinkRepository, ttl: int = 86400, pepper: str | None = None) -> None: self._repo = repo self._ttl = ttl + self._pepper = pepper + + def _hash_token(self, token: str) -> str: + if self._pepper: + return hmac.new(self._pepper.encode(), token.encode(), hashlib.sha256).hexdigest() + return hashlib.sha256(token.encode()).hexdigest() async def create( self, @@ -18,34 +32,39 @@ class MagicLinkService: created_by: str | None = None, note: str | None = None, ) -> MagicLink: - """Generate and store a new magic link for the given username.""" + """Generate and store a new magic link for the given username. + + Persists only the hashed token; the returned MagicLink carries the raw + token so the caller can build the registration URL. + """ token = secrets.token_urlsafe(32) expires_at = datetime.now(UTC) + timedelta(seconds=self._ttl) - link = MagicLink( - token=token, + stored = MagicLink( + token=self._hash_token(token), username=username, expires_at=expires_at, created_by=created_by, note=note, ) - return await self._repo.create(link) + await self._repo.create(stored) + return stored.model_copy(update={"token": token}) async def validate(self, token: str) -> MagicLink | None: """Validate a magic link token. - Returns the MagicLink if it exists, is not used, and has not expired. - Returns None otherwise. + Returns the MagicLink (with the raw token re-attached) if it exists, is + not used, and has not expired. Returns None otherwise. """ - link = await self._repo.get_by_token(token) + link = await self._repo.get_by_token(self._hash_token(token)) if link is None or link.used: return None if link.expires_at < datetime.now(UTC): return None - return link + return link.model_copy(update={"token": token}) async def mark_used(self, token: str) -> bool: """Mark a magic link as used. Returns True if found and marked.""" - return await self._repo.mark_used(token) + return await self._repo.mark_used(self._hash_token(token)) async def cleanup_expired(self) -> int: """Delete expired unused links. Returns count deleted.""" diff --git a/tests/test_auth_routes/test_register_magic_link.py b/tests/test_auth_routes/test_register_magic_link.py index 1a03fed..2d4ed65 100644 --- a/tests/test_auth_routes/test_register_magic_link.py +++ b/tests/test_auth_routes/test_register_magic_link.py @@ -1,8 +1,7 @@ -from datetime import UTC, datetime, timedelta - from httpx import AsyncClient -from porchlight.models import MagicLink, User +from porchlight.invite.service import MagicLinkService +from porchlight.models import User async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None: @@ -13,42 +12,29 @@ async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> async def test_register_expired_token_returns_error_page(client: AsyncClient) -> None: app = client._transport.app # type: ignore[union-attr] - repo = app.state.magic_link_repo - await repo.create( - MagicLink( - token="expired", - username="newuser", - expires_at=datetime.now(UTC) - timedelta(hours=1), - ) - ) + # An already-expired link (negative TTL), stored hashed like the real service. + expired_service = MagicLinkService(repo=app.state.magic_link_repo, ttl=-3600) + link = await expired_service.create(username="newuser") - res = await client.get("/register/expired", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code == 400 assert "Invalid or expired" in res.text async def test_register_valid_token_creates_user_and_redirects(client: AsyncClient) -> None: app = client._transport.app # type: ignore[union-attr] - magic_link_repo = app.state.magic_link_repo + magic_link_service = app.state.magic_link_service user_repo = app.state.user_repo - await magic_link_repo.create( - MagicLink( - token="t1", - username="newuser", - expires_at=datetime.now(UTC) + timedelta(hours=1), - ) - ) + link = await magic_link_service.create(username="newuser") - res = await client.get("/register/t1", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code in (302, 303) assert "/manage/credentials" in res.headers["location"] assert "setup=1" in res.headers["location"] - # Token should be marked used - link = await magic_link_repo.get_by_token("t1") - assert link is not None - assert link.used is True + # Token should be consumed (no longer valid) + assert await magic_link_service.validate(link.token) is None # User should exist user = await user_repo.get_by_username("newuser") @@ -58,48 +44,34 @@ async def test_register_valid_token_creates_user_and_redirects(client: AsyncClie async def test_register_used_token_returns_error(client: AsyncClient) -> None: app = client._transport.app # type: ignore[union-attr] - repo = app.state.magic_link_repo - await repo.create( - MagicLink( - token="used", - username="newuser", - expires_at=datetime.now(UTC) + timedelta(hours=1), - used=True, - ) - ) + magic_link_service = app.state.magic_link_service + link = await magic_link_service.create(username="newuser") + await magic_link_service.mark_used(link.token) - res = await client.get("/register/used", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code == 400 async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient) -> None: - """When initial-admin creates a user, the invite link should log them in.""" + """When initial-admin creates a user (no credentials yet), the invite link + should let them set up their account.""" app = client._transport.app # type: ignore[union-attr] - magic_link_repo = app.state.magic_link_repo + magic_link_service = app.state.magic_link_service user_repo = app.state.user_repo - # Pre-create the user (as initial-admin would) + # Pre-create the user (as initial-admin would), with no credentials. user = User(userid="lusab-bansen", username="admin", groups=["admin", "users"]) await user_repo.create(user) - # Create invite for the same username - await magic_link_repo.create( - MagicLink( - token="admin-setup", - username="admin", - expires_at=datetime.now(UTC) + timedelta(hours=1), - ) - ) + link = await magic_link_service.create(username="admin") - res = await client.get("/register/admin-setup", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code in (302, 303) assert "/manage/credentials" in res.headers["location"] assert "setup=1" in res.headers["location"] - # Token should be marked used - link = await magic_link_repo.get_by_token("admin-setup") - assert link is not None - assert link.used is True + # Token should be consumed + assert await magic_link_service.validate(link.token) is None # Original user should still exist with original groups existing = await user_repo.get_by_username("admin") diff --git a/tests/test_invite/test_service.py b/tests/test_invite/test_service.py index 6464fce..e7a20bd 100644 --- a/tests/test_invite/test_service.py +++ b/tests/test_invite/test_service.py @@ -42,6 +42,18 @@ async def test_create_returns_magic_link(service: MagicLinkService) -> None: assert link.expires_at > datetime.now(UTC) +async def test_token_stored_hashed_not_plaintext(service: MagicLinkService, repo: SQLiteMagicLinkRepository) -> None: + # The raw token handed to the user must never be the value persisted in + # the store; a DB read must not yield a usable token. + link = await service.create(username="alice") + raw = link.token + assert await repo.get_by_token(raw) is None + # ...but the raw token must still validate through the service. + validated = await service.validate(raw) + assert validated is not None + assert validated.username == "alice" + + async def test_create_generates_unique_tokens(service: MagicLinkService) -> None: link1 = await service.create(username="alice") link2 = await service.create(username="bob") From e4eb539e3f5067136d656fc935152a3fc858620d Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 10:46:38 +0200 Subject: [PATCH 06/15] fix(security): consume magic-link tokens atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validation and marking-used were two separate steps, so two concurrent requests for the same registration token could both pass validation before either marked it used — a replay window. Add an atomic consume() at the repository (conditional UPDATE ... WHERE used = 0 AND not expired, gated on rowcount) and service layers, and switch the /register handler to consume() instead of validate()+mark_used(). Refs: porchlight-ur7 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/authn/routes.py | 5 ++-- src/porchlight/invite/service.py | 13 +++++++++ src/porchlight/store/protocols.py | 2 ++ src/porchlight/store/sqlite/repositories.py | 19 +++++++++++++ tests/test_invite/test_service.py | 16 +++++++++++ .../test_store/test_sqlite_magic_link_repo.py | 28 +++++++++++++++++++ 6 files changed, 80 insertions(+), 3 deletions(-) diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index c710f3f..529e1e3 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -77,7 +77,8 @@ async def register_magic_link(request: Request, token: str) -> Response: magic_link_service = request.app.state.magic_link_service user_repo = request.app.state.user_repo - link = await magic_link_service.validate(token) + # Atomically validate and consume the token (single-use, no replay race). + link = await magic_link_service.consume(token) if link is None: return HTMLResponse("

Invalid or expired registration link.

", status_code=400) @@ -91,8 +92,6 @@ async def register_magic_link(request: Request, token: str) -> Response: user = User(userid=userid, username=link.username, groups=["users"]) await user_repo.create(user) - await magic_link_service.mark_used(token) - request.session["userid"] = user.userid request.session["username"] = user.username diff --git a/src/porchlight/invite/service.py b/src/porchlight/invite/service.py index b8b5ef8..4766442 100644 --- a/src/porchlight/invite/service.py +++ b/src/porchlight/invite/service.py @@ -66,6 +66,19 @@ class MagicLinkService: """Mark a magic link as used. Returns True if found and marked.""" return await self._repo.mark_used(self._hash_token(token)) + async def consume(self, token: str) -> MagicLink | None: + """Atomically validate and consume a token in one step. + + Prefer this over validate()+mark_used(): it closes the replay race + where two concurrent requests could both pass validation before either + marks the token used. Returns the link (raw token re-attached) on + success, else None. + """ + link = await self._repo.consume(self._hash_token(token)) + if link is None: + return None + return link.model_copy(update={"token": token}) + async def cleanup_expired(self) -> int: """Delete expired unused links. Returns count deleted.""" return await self._repo.delete_expired() diff --git a/src/porchlight/store/protocols.py b/src/porchlight/store/protocols.py index ba1af34..034d6d9 100644 --- a/src/porchlight/store/protocols.py +++ b/src/porchlight/store/protocols.py @@ -55,6 +55,8 @@ class MagicLinkRepository(Protocol): async def mark_used(self, token: str) -> bool: ... + async def consume(self, token: str) -> MagicLink | None: ... + async def delete_expired(self) -> int: ... diff --git a/src/porchlight/store/sqlite/repositories.py b/src/porchlight/store/sqlite/repositories.py index 37d02bb..6acc941 100644 --- a/src/porchlight/store/sqlite/repositories.py +++ b/src/porchlight/store/sqlite/repositories.py @@ -311,6 +311,25 @@ class SQLiteMagicLinkRepository: await self._db.commit() return cursor.rowcount > 0 + async def consume(self, token: str) -> MagicLink | None: + """Atomically validate-and-mark a token as used. + + The conditional UPDATE is the single point of decision, so two + concurrent requests cannot both consume the same token. Returns the + link if this call won the race (unused and unexpired), else None. + """ + now = datetime.now(UTC).isoformat() + cursor = await self._db.execute( + "UPDATE magic_links SET used = 1 WHERE token = ? AND used = 0 AND expires_at > ?", + (token, now), + ) + await self._db.commit() + if cursor.rowcount == 0: + return None + async with self._db.execute("SELECT * FROM magic_links WHERE token = ?", (token,)) as c: + row = await c.fetchone() + return self._row_to_magic_link(row) if row is not None else None + async def delete_expired(self) -> int: now = datetime.now(UTC).isoformat() cursor = await self._db.execute("DELETE FROM magic_links WHERE expires_at < ? AND used = 0", (now,)) diff --git a/tests/test_invite/test_service.py b/tests/test_invite/test_service.py index e7a20bd..166e7cd 100644 --- a/tests/test_invite/test_service.py +++ b/tests/test_invite/test_service.py @@ -102,6 +102,22 @@ async def test_validate_expired_token(service: MagicLinkService, repo: SQLiteMag assert result is None +async def test_consume_validates_and_is_single_use(service: MagicLinkService) -> None: + link = await service.create(username="alice") + consumed = await service.consume(link.token) + assert consumed is not None + assert consumed.username == "alice" + assert consumed.token == link.token # raw token re-attached + # A second consume of the same token fails. + assert await service.consume(link.token) is None + + +async def test_consume_expired_returns_none(service: MagicLinkService, repo: SQLiteMagicLinkRepository) -> None: + expired_service = MagicLinkService(repo=repo, ttl=-1) + link = await expired_service.create(username="alice") + assert await service.consume(link.token) is None + + async def test_mark_used_returns_true(service: MagicLinkService) -> None: link = await service.create(username="alice") result = await service.mark_used(link.token) diff --git a/tests/test_store/test_sqlite_magic_link_repo.py b/tests/test_store/test_sqlite_magic_link_repo.py index c2c9571..bb4f8b6 100644 --- a/tests/test_store/test_sqlite_magic_link_repo.py +++ b/tests/test_store/test_sqlite_magic_link_repo.py @@ -63,6 +63,34 @@ async def test_mark_used_not_found(magic_link_repo: SQLiteMagicLinkRepository) - assert marked is False +async def test_consume_marks_used_and_returns_link(magic_link_repo: SQLiteMagicLinkRepository) -> None: + await magic_link_repo.create(_make_link()) + + consumed = await magic_link_repo.consume("abc123") + assert consumed is not None + assert consumed.used is True + assert consumed.username == "alice" + + +async def test_consume_is_single_use(magic_link_repo: SQLiteMagicLinkRepository) -> None: + await magic_link_repo.create(_make_link()) + + first = await magic_link_repo.consume("abc123") + assert first is not None + # A second consume of the same token must fail (atomic single-use). + second = await magic_link_repo.consume("abc123") + assert second is None + + +async def test_consume_expired_returns_none(magic_link_repo: SQLiteMagicLinkRepository) -> None: + await magic_link_repo.create(_make_link(token="exp", expires_at=datetime.now(UTC) - timedelta(hours=1))) + assert await magic_link_repo.consume("exp") is None + + +async def test_consume_nonexistent_returns_none(magic_link_repo: SQLiteMagicLinkRepository) -> None: + assert await magic_link_repo.consume("nope") is None + + async def test_delete_expired(magic_link_repo: SQLiteMagicLinkRepository) -> None: expired = _make_link(token="expired", expires_at=datetime.now(UTC) - timedelta(hours=1)) await magic_link_repo.create(expired) From faeecaed5945b4d041a684144b6b69b063e29486 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 10:51:01 +0200 Subject: [PATCH 07/15] fix(security): invite links must not log in accounts with credentials A registration/re-invite link auto-established a session for any existing active user, so re-inviting a fully set-up user acted as a passwordless login. Invite links are for account setup only. After consuming the token, refuse to establish a session when the target account already has a password or WebAuthn credential. Credential-less accounts (e.g. freshly created by initial-admin) can still complete setup. Account recovery for set-up accounts must use a separate, authenticated flow. Refs: porchlight-a3a Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/authn/routes.py | 12 +++++++++ .../test_register_magic_link.py | 25 ++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index 529e1e3..5fb98eb 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -86,6 +86,18 @@ async def register_magic_link(request: Request, token: str) -> Response: if existing_user is not None: if not existing_user.active: return HTMLResponse("

This account has been deactivated.

", status_code=400) + # An invite link is for account setup, not authentication. If the + # account already has credentials, refuse to establish a session — + # otherwise a re-invite would be a passwordless login. Account + # recovery must go through a separate, explicitly authenticated flow. + cred_repo = request.app.state.credential_repo + has_password = await cred_repo.get_password_by_user(existing_user.userid) is not None + has_webauthn = bool(await cred_repo.get_webauthn_by_user(existing_user.userid)) + if has_password or has_webauthn: + return HTMLResponse( + "

This account is already set up. Please sign in instead.

", + status_code=400, + ) user = existing_user else: userid = await generate_unique_userid(user_repo) diff --git a/tests/test_auth_routes/test_register_magic_link.py b/tests/test_auth_routes/test_register_magic_link.py index 2d4ed65..4a79408 100644 --- a/tests/test_auth_routes/test_register_magic_link.py +++ b/tests/test_auth_routes/test_register_magic_link.py @@ -1,7 +1,9 @@ +from argon2 import PasswordHasher from httpx import AsyncClient +from porchlight.authn.password import PasswordService from porchlight.invite.service import MagicLinkService -from porchlight.models import User +from porchlight.models import PasswordCredential, User async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None: @@ -78,3 +80,24 @@ async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient) assert existing is not None assert existing.userid == "lusab-bansen" assert "admin" in existing.groups + + +async def test_register_rejects_user_that_already_has_credentials(client: AsyncClient) -> None: + """An invite/re-invite link must not act as a passwordless login for an + account that already has credentials. Recovery is a separate flow.""" + app = client._transport.app # type: ignore[union-attr] + magic_link_service = app.state.magic_link_service + user_repo = app.state.user_repo + cred_repo = app.state.credential_repo + + user = User(userid="lusab-hascreds", username="hascreds", groups=["users"]) + await user_repo.create(user) + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("existing-pass"))) + + link = await magic_link_service.create(username="hascreds") + res = await client.get(f"/register/{link.token}", follow_redirects=False) + + # No passwordless login: rejected, not redirected to setup. + assert res.status_code == 400 + assert "setup=1" not in res.headers.get("location", "") From 71a7c23bdd04476777ead058f97bb0b78b7ac655 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 11:00:30 +0200 Subject: [PATCH 08/15] fix(security): verify S256 PKCE when relying parties use it idpyoidc advertised PKCE support but did not actually verify the code challenge at the token endpoint, so a sent code_challenge provided no protection. Enable the PKCE add-on restricted to S256. Configured as non-essential: relying parties that do not send a code_challenge continue to work (no breaking change), but any RP that uses PKCE must use S256, and the code_verifier is verified at token exchange. Flip essential=True (or per-client pkce_essential) to require PKCE once all clients have migrated. Refs: porchlight-s48 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/oidc/provider.py | 15 +++++ tests/test_oidc/test_e2e_flow.py | 96 ++++++++++++++++++++++++++------ 2 files changed, 95 insertions(+), 16 deletions(-) diff --git a/src/porchlight/oidc/provider.py b/src/porchlight/oidc/provider.py index d1ac38b..aae64d3 100644 --- a/src/porchlight/oidc/provider.py +++ b/src/porchlight/oidc/provider.py @@ -3,6 +3,7 @@ from pathlib import Path from idpyoidc.server import Server +from idpyoidc.server.oauth2.add_on.pkce import CC_METHOD from porchlight.config import Settings from porchlight.oidc.claims import PorchlightUserInfo @@ -126,6 +127,20 @@ def _build_server_config(settings: Settings) -> dict: "phone": ["phone_number", "phone_number_verified"], }, "authentication": {}, + # PKCE: advertise and verify S256 (only). Not "essential", so existing + # relying parties that do not yet send a code_challenge keep working; + # but any RP that does use PKCE must use S256 and is verified at the + # token endpoint. Set essential=True (or per-client pkce_essential) to + # require it once all clients have migrated. + "add_on": { + "pkce": { + "function": "idpyoidc.server.oauth2.add_on.pkce.add_support", + "kwargs": { + "essential": False, + "code_challenge_methods": {"S256": CC_METHOD["S256"]}, + }, + }, + }, } diff --git a/tests/test_oidc/test_e2e_flow.py b/tests/test_oidc/test_e2e_flow.py index b1b9545..cd3f547 100644 --- a/tests/test_oidc/test_e2e_flow.py +++ b/tests/test_oidc/test_e2e_flow.py @@ -1,6 +1,7 @@ +import hashlib import json import secrets -from base64 import b64encode +from base64 import b64encode, urlsafe_b64encode from datetime import UTC, datetime from typing import Any from urllib.parse import parse_qs, urlparse @@ -54,19 +55,36 @@ async def _setup_rp_and_user(app: FastAPI) -> None: await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("testpass"))) -async def _login_and_authorize(client: AsyncClient, state: str, nonce: str) -> str: +def _pkce_pair() -> tuple[str, str]: + """Return a (code_verifier, S256 code_challenge) pair.""" + verifier = secrets.token_urlsafe(64) + challenge = urlsafe_b64encode(hashlib.sha256(verifier.encode()).digest()).rstrip(b"=").decode() + return verifier, challenge + + +async def _login_and_authorize( + client: AsyncClient, + state: str, + nonce: str, + code_challenge: str | None = None, + code_challenge_method: str | None = None, +) -> str: """Perform authorization request, login, consent, and return the auth code.""" + params = { + "response_type": "code", + "client_id": CLIENT_ID, + "redirect_uri": REDIRECT_URI, + "scope": "openid profile email", + "state": state, + "nonce": nonce, + } + if code_challenge is not None: + params["code_challenge"] = code_challenge + params["code_challenge_method"] = code_challenge_method or "S256" # Step 1: Authorization request (unauthenticated) -> redirect to /login auth_res = await client.get( "/authorization", - params={ - "response_type": "code", - "client_id": CLIENT_ID, - "redirect_uri": REDIRECT_URI, - "scope": "openid profile email", - "state": state, - "nonce": nonce, - }, + params=params, follow_redirects=False, ) assert auth_res.status_code in (302, 303), f"Expected redirect to /login, got {auth_res.status_code}" @@ -116,16 +134,19 @@ async def _login_and_authorize(client: AsyncClient, state: str, nonce: str) -> s return code -async def _exchange_token(client: AsyncClient, code: str) -> dict[str, Any]: +async def _exchange_token(client: AsyncClient, code: str, code_verifier: str | None = None) -> dict[str, Any]: """Exchange authorization code for tokens.""" auth_header = b64encode(f"{CLIENT_ID}:{CLIENT_SECRET}".encode()).decode() + data = { + "grant_type": "authorization_code", + "code": code, + "redirect_uri": REDIRECT_URI, + } + if code_verifier is not None: + data["code_verifier"] = code_verifier token_res = await client.post( "/token", - data={ - "grant_type": "authorization_code", - "code": code, - "redirect_uri": REDIRECT_URI, - }, + data=data, headers={ "Authorization": f"Basic {auth_header}", "Content-Type": "application/x-www-form-urlencoded", @@ -199,3 +220,46 @@ async def test_full_authorization_code_flow(client: AsyncClient) -> None: # sub should match between id_token and userinfo assert userinfo_data["sub"] == id_token_sub + + +async def test_discovery_advertises_s256_pkce(client: AsyncClient) -> None: + res = await client.get("/.well-known/openid-configuration") + assert res.status_code == 200 + assert res.json().get("code_challenge_methods_supported") == ["S256"] + + +async def test_pkce_s256_flow_succeeds(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + await _setup_rp_and_user(app) + state, nonce = secrets.token_urlsafe(16), secrets.token_urlsafe(16) + verifier, challenge = _pkce_pair() + + code = await _login_and_authorize(client, state, nonce, code_challenge=challenge) + token_data = await _exchange_token(client, code, code_verifier=verifier) + assert "access_token" in token_data + + +async def test_pkce_wrong_verifier_rejected(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + await _setup_rp_and_user(app) + state, nonce = secrets.token_urlsafe(16), secrets.token_urlsafe(16) + _verifier, challenge = _pkce_pair() + + code = await _login_and_authorize(client, state, nonce, code_challenge=challenge) + + # Exchange with a verifier that does not match the challenge -> rejected. + auth_header = b64encode(f"{CLIENT_ID}:{CLIENT_SECRET}".encode()).decode() + res = await client.post( + "/token", + data={ + "grant_type": "authorization_code", + "code": code, + "redirect_uri": REDIRECT_URI, + "code_verifier": "wrong-" + secrets.token_urlsafe(48), + }, + headers={ + "Authorization": f"Basic {auth_header}", + "Content-Type": "application/x-www-form-urlencoded", + }, + ) + assert res.status_code != 200, f"PKCE verifier mismatch should be rejected, got {res.status_code}: {res.text}" From 7c4dbf2cd97c4f4d043d101c70bb77edcbad947b Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 4 Jun 2026 11:06:08 +0200 Subject: [PATCH 09/15] 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) --- src/porchlight/oidc/endpoints.py | 22 ++++++++++++++++------ tests/test_oidc/test_authorization.py | 10 ++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/porchlight/oidc/endpoints.py b/src/porchlight/oidc/endpoints.py index 8bfca41..12af079 100644 --- a/src/porchlight/oidc/endpoints.py +++ b/src/porchlight/oidc/endpoints.py @@ -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"

{html.escape(title)}

{html.escape(str(message))}

", + 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"

Invalid Request

{exc}

", 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"

Error

{error_desc}

", 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"

Error

{exc}

", status_code=400) + return _error_page(exc) if "error" in parsed: error_desc = parsed.get("error_description", parsed["error"]) - return HTMLResponse(f"

Error

{error_desc}

", 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"

Error

{error_desc}

", 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"

Error

{exc}

", status_code=400) + return _error_page(exc) return await _complete_authorization(request, oidc_server, endpoint, parsed, userid, username) diff --git a/tests/test_oidc/test_authorization.py b/tests/test_oidc/test_authorization.py index 5994134..de3c70b 100644 --- a/tests/test_oidc/test_authorization.py +++ b/tests/test_oidc/test_authorization.py @@ -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("") + body = resp.body.decode() + assert "