From cba63280fb4358b2f97ebd83ea286c8121fcd4b4 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Mon, 8 Jun 2026 10:26:57 +0200 Subject: [PATCH 1/4] fix(security): set an explicit session cookie lifetime The session cookie relied on Starlette's default max_age (two weeks), which is easy to miss and longer than an OP session should live. Add a session_max_age setting (default 8 hours) and pass it to SessionMiddleware. Refs: porchlight-1lg Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/app.py | 1 + src/porchlight/config.py | 1 + tests/test_app.py | 9 +++++++++ 3 files changed, 11 insertions(+) diff --git a/src/porchlight/app.py b/src/porchlight/app.py index e07d51f..c186d8d 100644 --- a/src/porchlight/app.py +++ b/src/porchlight/app.py @@ -143,6 +143,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: secret_key=session_secret, same_site="lax", https_only=settings.session_https_only, + max_age=settings.session_max_age, ) # Rate limiting diff --git a/src/porchlight/config.py b/src/porchlight/config.py index a8225d2..d2e6cc1 100644 --- a/src/porchlight/config.py +++ b/src/porchlight/config.py @@ -48,6 +48,7 @@ class Settings(BaseSettings): # Session session_secret: str | None = None # If None, a random secret is generated per process session_https_only: bool = True + session_max_age: int = 28800 # Cookie lifetime in seconds (default 8 hours) # WebAuthn user verification requirement: "preferred" (default), "required", # or "discouraged". Identity providers may want "required". diff --git a/tests/test_app.py b/tests/test_app.py index 14c3aca..5ce3bcf 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -75,3 +75,12 @@ def test_create_app_allows_missing_secret_on_localhost() -> None: def test_create_app_allows_missing_secret_in_debug() -> None: settings = Settings(issuer="https://op.example.com", sqlite_path=":memory:", debug=True) assert create_app(settings) is not None + + +async def test_session_cookie_has_explicit_max_age(client: AsyncClient) -> None: + # Visiting /login establishes a session (CSRF token), setting the cookie. + res = await client.get("/login") + set_cookies = res.headers.get_list("set-cookie") + session_cookies = [c for c in set_cookies if c.startswith("session=")] + assert session_cookies, "no session cookie set" + assert "Max-Age=28800" in session_cookies[0] From c7550cbf0957e8fdbb7de23090f4a9144e8d950c Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Mon, 8 Jun 2026 15:21:27 +0200 Subject: [PATCH 2/4] fix(security): lock down signing-key file permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Private JWK files were written under the default umask (observed 0664 — group and world readable). Create the key directory 0700, chmod private key files (private_jwks.json, token_jwks.json) to 0600 after they are written, and refuse to start if a pre-existing private key is group/world accessible. Tests now use an isolated per-test key directory. Refs: porchlight-91i Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/oidc/provider.py | 27 ++++++++++++++++++++++++++- tests/conftest.py | 10 ++++++++-- tests/test_client_registration.py | 8 +++++--- tests/test_oidc/test_provider.py | 28 ++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/porchlight/oidc/provider.py b/src/porchlight/oidc/provider.py index aae64d3..bcf1b71 100644 --- a/src/porchlight/oidc/provider.py +++ b/src/porchlight/oidc/provider.py @@ -1,5 +1,6 @@ """idpyoidc Server initialization.""" +import os from pathlib import Path from idpyoidc.server import Server @@ -144,8 +145,32 @@ def _build_server_config(settings: Settings) -> dict: } +_PRIVATE_KEY_FILES = ("private_jwks.json", "token_jwks.json") + + def create_oidc_server(settings: Settings) -> Server: - """Create and configure an idpyoidc Server instance.""" + """Create and configure an idpyoidc Server instance. + + Private signing keys are written to ``signing_key_path``; lock the directory + to 0700 and the private key files to 0600, and refuse to start if a + pre-existing private key is group/world accessible (a key disclosure). + """ + key_path = Path(settings.signing_key_path) + key_path.mkdir(parents=True, exist_ok=True) + os.chmod(key_path, 0o700) + + # Fail on pre-existing keys with loose permissions (left by a prior run). + for name in _PRIVATE_KEY_FILES: + f = key_path / name + if f.exists() and (f.stat().st_mode & 0o077): + raise RuntimeError(f"Insecure permission on {f}: private key is group/world accessible") + config = _build_server_config(settings) server = Server(conf=config) + + # Lock down any keys idpyoidc just wrote (umask may have left them 0644). + for name in _PRIVATE_KEY_FILES: + f = key_path / name + if f.exists(): + os.chmod(f, 0o600) return server diff --git a/tests/conftest.py b/tests/conftest.py index cc9eee3..72256b3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import re from collections.abc import AsyncIterator +from pathlib import Path import pytest from httpx import ASGITransport, AsyncClient @@ -10,8 +11,13 @@ from porchlight.rate_limit import limiter @pytest.fixture -def settings() -> Settings: - return Settings(issuer="http://localhost:8000", sqlite_path=":memory:", session_https_only=False) +def settings(tmp_path: Path) -> Settings: + return Settings( + issuer="http://localhost:8000", + sqlite_path=":memory:", + session_https_only=False, + signing_key_path=str(tmp_path / "keys"), + ) @pytest.fixture diff --git a/tests/test_client_registration.py b/tests/test_client_registration.py index ea6ba37..acfe41f 100644 --- a/tests/test_client_registration.py +++ b/tests/test_client_registration.py @@ -19,7 +19,7 @@ scope = ["openid", "profile"] toml_file = tmp_path / "test.toml" toml_file.write_text(toml_content) - settings = Settings(_toml_file=str(toml_file), session_secret="x" * 32) + settings = Settings(_toml_file=str(toml_file), session_secret="x" * 32, signing_key_path=str(tmp_path / "keys")) app = create_app(settings) async with ( @@ -39,9 +39,11 @@ scope = ["openid", "profile"] assert cdb_entry["allowed_scopes"] == ["openid", "profile"] -async def test_manage_app_always_registered() -> None: +async def test_manage_app_always_registered(tmp_path: Path) -> None: """The internal manage-app client is always registered, even without config file clients.""" - settings = Settings(issuer="https://test.example.com", session_secret="x" * 32) + settings = Settings( + issuer="https://test.example.com", session_secret="x" * 32, signing_key_path=str(tmp_path / "keys") + ) app = create_app(settings) async with ( diff --git a/tests/test_oidc/test_provider.py b/tests/test_oidc/test_provider.py index ebfbf97..e57eb92 100644 --- a/tests/test_oidc/test_provider.py +++ b/tests/test_oidc/test_provider.py @@ -1,6 +1,9 @@ +import os import shutil from pathlib import Path +import pytest + from porchlight.config import Settings from porchlight.oidc.claims import PorchlightUserInfo from porchlight.oidc.provider import create_oidc_server @@ -53,3 +56,28 @@ def test_create_server_userinfo_is_porchlight() -> None: assert isinstance(server.context.userinfo, PorchlightUserInfo) finally: shutil.rmtree(key_path, ignore_errors=True) + + +def test_signing_key_files_have_strict_permissions(tmp_path: Path) -> None: + key_path = tmp_path / "keys" + settings = Settings(issuer="http://localhost:8000", sqlite_path=":memory:", signing_key_path=str(key_path)) + create_oidc_server(settings) + + assert (key_path.stat().st_mode & 0o077) == 0, "key directory must not be group/world accessible" + for name in ("private_jwks.json", "token_jwks.json"): + f = key_path / name + assert f.exists() + assert (f.stat().st_mode & 0o077) == 0, f"{name} must be 0600" + + +def test_startup_fails_on_world_readable_private_key(tmp_path: Path) -> None: + key_path = tmp_path / "keys" + key_path.mkdir() + # Simulate a pre-existing private key left group/world readable. + leaked = key_path / "private_jwks.json" + leaked.write_text("{}") + os.chmod(leaked, 0o644) + + settings = Settings(issuer="http://localhost:8000", sqlite_path=":memory:", signing_key_path=str(key_path)) + with pytest.raises(RuntimeError, match="permission"): + create_oidc_server(settings) From 519e3659a1fbb58ffb731d3cdeef5f1a19cee47e Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Wed, 10 Jun 2026 08:53:49 +0200 Subject: [PATCH 3/4] feat(security): add baseline security-header middleware No security headers were set. Add SecurityHeadersMiddleware applying Content-Security-Policy (configurable), X-Content-Type-Options: nosniff, X-Frame-Options: DENY, Referrer-Policy, and Strict-Transport-Security on HTTPS deployments. Verified HTMX/WebAuthn/forms still work under the CSP. Refs: porchlight-1ph Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/app.py | 8 ++++++ src/porchlight/config.py | 5 ++++ src/porchlight/security_headers.py | 42 ++++++++++++++++++++++++++++++ tests/test_app.py | 27 ++++++++++++++++++- 4 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 src/porchlight/security_headers.py diff --git a/src/porchlight/app.py b/src/porchlight/app.py index c186d8d..272f9ce 100644 --- a/src/porchlight/app.py +++ b/src/porchlight/app.py @@ -26,6 +26,7 @@ from porchlight.manage.routes import router as manage_router from porchlight.oidc.endpoints import router as oidc_router from porchlight.oidc.provider import create_oidc_server from porchlight.rate_limit import limiter +from porchlight.security_headers import SecurityHeadersMiddleware from porchlight.store.sqlite.db import open_db from porchlight.store.sqlite.repositories import ( SQLiteConsentRepository, @@ -130,6 +131,13 @@ def create_app(settings: Settings | None = None) -> FastAPI: app.state.settings = settings + # Security headers (outermost so they apply to every response) + app.add_middleware( + SecurityHeadersMiddleware, # ty: ignore[invalid-argument-type] + csp=settings.content_security_policy, + hsts=settings.session_https_only, + ) + # Session middleware session_secret = _resolve_session_secret(settings) app.state.session_secret = session_secret diff --git a/src/porchlight/config.py b/src/porchlight/config.py index d2e6cc1..20ff9ea 100644 --- a/src/porchlight/config.py +++ b/src/porchlight/config.py @@ -50,6 +50,11 @@ class Settings(BaseSettings): session_https_only: bool = True session_max_age: int = 28800 # Cookie lifetime in seconds (default 8 hours) + # Content-Security-Policy applied to all responses. + content_security_policy: str = ( + "default-src 'self'; img-src 'self' data: https:; frame-ancestors 'none'; base-uri 'self'; form-action 'self'" + ) + # WebAuthn user verification requirement: "preferred" (default), "required", # or "discouraged". Identity providers may want "required". webauthn_user_verification: str = "preferred" diff --git a/src/porchlight/security_headers.py b/src/porchlight/security_headers.py new file mode 100644 index 0000000..4cc419b --- /dev/null +++ b/src/porchlight/security_headers.py @@ -0,0 +1,42 @@ +"""ASGI middleware adding baseline security response headers.""" + +from starlette.types import ASGIApp, Message, Receive, Scope, Send + + +class SecurityHeadersMiddleware: + """Attach security headers to every HTTP response. + + Sets Content-Security-Policy, X-Content-Type-Options, X-Frame-Options and + Referrer-Policy on all responses, plus Strict-Transport-Security when the + deployment is HTTPS. Existing headers are not overwritten. + """ + + def __init__(self, app: ASGIApp, csp: str, hsts: bool) -> None: + self.app = app + self._csp = csp + self._hsts = hsts + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + async def send_with_headers(message: Message) -> None: + if message["type"] == "http.response.start": + headers = message.setdefault("headers", []) + existing = {name.decode().lower() for name, _ in headers} + + def add(name: str, value: str) -> None: + if name.lower() not in existing: + headers.append((name.encode(), value.encode())) + existing.add(name.lower()) + + add("content-security-policy", self._csp) + add("x-content-type-options", "nosniff") + add("x-frame-options", "DENY") + add("referrer-policy", "strict-origin-when-cross-origin") + if self._hsts: + add("strict-transport-security", "max-age=63072000; includeSubDomains") + await send(message) + + await self.app(scope, receive, send_with_headers) diff --git a/tests/test_app.py b/tests/test_app.py index 5ce3bcf..a46d16d 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -1,7 +1,8 @@ +from pathlib import Path from unittest.mock import MagicMock import pytest -from httpx import AsyncClient +from httpx import ASGITransport, AsyncClient from porchlight.app import create_app from porchlight.config import Settings @@ -84,3 +85,27 @@ async def test_session_cookie_has_explicit_max_age(client: AsyncClient) -> None: session_cookies = [c for c in set_cookies if c.startswith("session=")] assert session_cookies, "no session cookie set" assert "Max-Age=28800" in session_cookies[0] + + +async def test_security_headers_present(client: AsyncClient) -> None: + res = await client.get("/login") + assert res.headers.get("x-content-type-options") == "nosniff" + assert res.headers.get("x-frame-options") == "DENY" + assert "default-src 'self'" in res.headers.get("content-security-policy", "") + assert "frame-ancestors 'none'" in res.headers.get("content-security-policy", "") + assert res.headers.get("referrer-policy") == "strict-origin-when-cross-origin" + + +async def test_hsts_present_when_https_only(tmp_path: Path) -> None: + settings = Settings( + issuer="https://op.example.com", + sqlite_path=":memory:", + session_secret="x" * 32, + session_https_only=True, + signing_key_path=str(tmp_path / "keys"), + ) + app = create_app(settings) + transport = ASGITransport(app=app) + async with app.router.lifespan_context(app), AsyncClient(transport=transport, base_url=settings.issuer) as ac: + res = await ac.get("/health") + assert "max-age=" in res.headers.get("strict-transport-security", "") From 27763d19ea6d11a1423b37e23f48fe9aaa100641 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Wed, 10 Jun 2026 09:25:49 +0200 Subject: [PATCH 4/4] fix(security): don't mint new ID tokens on refresh; confirm offline_access gating idpyoidc already gates refresh-token issuance on the offline_access scope (verified by test), but the refresh-token grant was configured to also mint fresh ID tokens. Drop id_token from the refresh_token grant's supports_minting so refreshing yields only access (and a rotated refresh) token; ID tokens come from authentication. Refresh-token rotation is retained. Refs: porchlight-553 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/oidc/provider.py | 5 ++- tests/test_oidc/test_e2e_flow.py | 70 +++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/porchlight/oidc/provider.py b/src/porchlight/oidc/provider.py index bcf1b71..c4b170e 100644 --- a/src/porchlight/oidc/provider.py +++ b/src/porchlight/oidc/provider.py @@ -67,7 +67,10 @@ def _build_server_config(settings: Settings) -> dict: "expires_in": 3600, }, "refresh_token": { - "supports_minting": ["access_token", "refresh_token", "id_token"], + # Rotate refresh tokens (mint a new one) and mint + # access tokens, but do NOT mint fresh ID tokens on + # refresh — re-authentication should issue ID tokens. + "supports_minting": ["access_token", "refresh_token"], "expires_in": 86400, }, }, diff --git a/tests/test_oidc/test_e2e_flow.py b/tests/test_oidc/test_e2e_flow.py index cd3f547..b0f367c 100644 --- a/tests/test_oidc/test_e2e_flow.py +++ b/tests/test_oidc/test_e2e_flow.py @@ -33,8 +33,8 @@ async def _setup_rp_and_user(app: FastAPI) -> None: "redirect_uris": [(REDIRECT_URI, {})], "response_types_supported": ["code"], "token_endpoint_auth_method": "client_secret_basic", - "scope": ["openid", "profile", "email"], - "allowed_scopes": ["openid", "profile", "email"], + "scope": ["openid", "profile", "email", "offline_access"], + "allowed_scopes": ["openid", "profile", "email", "offline_access"], "client_salt": secrets.token_hex(8), } oidc_server.keyjar.add_symmetric(CLIENT_ID, CLIENT_SECRET) @@ -263,3 +263,69 @@ async def test_pkce_wrong_verifier_rejected(client: AsyncClient) -> None: }, ) assert res.status_code != 200, f"PKCE verifier mismatch should be rejected, got {res.status_code}: {res.text}" + + +async def _authorize_with_offline_access(client: AsyncClient, state: str, nonce: str) -> str: + """Run the auth-code flow requesting offline_access, return the auth code.""" + scope = "openid profile offline_access" + # offline_access requires prompt=consent per the OIDC spec. + await client.get( + "/authorization", + params={ + "response_type": "code", + "client_id": CLIENT_ID, + "redirect_uri": REDIRECT_URI, + "scope": scope, + "state": state, + "nonce": nonce, + "prompt": "consent", + }, + follow_redirects=False, + ) + token = await get_csrf_token(client) + await client.post( + "/login/password", + data={"username": "alice", "password": "testpass"}, + headers={"HX-Request": "true", "X-CSRF-Token": token}, + ) + complete = await client.get("/authorization/complete", follow_redirects=False) + assert "/consent" in complete.headers["location"] + token = await get_csrf_token(client) + consent = await client.post( + "/consent", + data={"action": "allow", "scope": ["openid", "profile", "offline_access"]}, + headers={"X-CSRF-Token": token}, + follow_redirects=False, + ) + return parse_qs(urlparse(consent.headers["location"]).query)["code"][0] + + +async def test_refresh_token_issued_only_with_offline_access(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + await _setup_rp_and_user(app) + + # Without offline_access: no refresh token. + code = await _login_and_authorize(client, secrets.token_urlsafe(8), secrets.token_urlsafe(8)) + no_offline = await _exchange_token(client, code) + assert "refresh_token" not in no_offline + + +async def test_refresh_grant_does_not_mint_id_token(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + await _setup_rp_and_user(app) + + code = await _authorize_with_offline_access(client, secrets.token_urlsafe(8), secrets.token_urlsafe(8)) + token_data = await _exchange_token(client, code) + assert "refresh_token" in token_data, "offline_access should yield a refresh token" + + # Use the refresh token; the response must not contain a new ID token. + auth_header = b64encode(f"{CLIENT_ID}:{CLIENT_SECRET}".encode()).decode() + res = await client.post( + "/token", + data={"grant_type": "refresh_token", "refresh_token": token_data["refresh_token"]}, + headers={"Authorization": f"Basic {auth_header}", "Content-Type": "application/x-www-form-urlencoded"}, + ) + assert res.status_code == 200, res.text + refreshed = res.json() + assert "access_token" in refreshed + assert "id_token" not in refreshed