From 3665a7eacea8a5d8d86270ee3f19ab6aa63f3c9e Mon Sep 17 00:00:00 2001 From: Nick Venenga Date: Sat, 1 Aug 2026 13:07:46 -0400 Subject: [PATCH 1/3] feat(federation): expose the minted bearer on the response path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check() already injects `Authorization: Bearer ` on the request forwarded upstream, but that header is invisible to a browser SPA — it never reaches the client, only Vikunja's backend. A cert holder hitting Vikunja's web UI therefore still sees Vikunja's own client-side OIDC login flow, which this federator's OP was deliberately never built to handle (no /oauth/authorize route: the design mints codes server-to- server only, per the header comment in op/routes.py). Add `X-Authz-Bootstrap-Token` to `response_headers_to_add` whenever a fresh bearer is minted, so a downstream response filter (e.g. an Envoy Lua filter injecting a `localStorage` seed script into the HTML document) can hand the same token to the browser and let the SPA boot already authenticated, with no visible redirect. Omitted on the "client's own bearer already valid" branch, since nothing new was minted there. Verified end-to-end against a real Envoy + Vikunja stack with a real homelab mTLS client cert (manual local simulation, not part of this change): Firefox loaded the page fully authenticated with no login screen or redirect. --- docs/federation.md | 13 ++++++++++++- envoy_authz/grpc_service.py | 17 +++++++++++++++++ tests/unit/test_check.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/docs/federation.md b/docs/federation.md index 1685ed5..051b8b0 100644 --- a/docs/federation.md +++ b/docs/federation.md @@ -80,10 +80,21 @@ per-request `GET /api/v1/user`. | `get_bearer` result | `Check` response | |---|---| -| `str` | `OK` + `headers: Authorization: Bearer ` | +| `str` | `OK` + `headers: Authorization: Bearer ` + `response_headers_to_add: X-Authz-Bootstrap-Token: ` | | `None` | `OK` with **no** `Authorization` header (client's bearer allowed through) | | raises `DownstreamError` | deny | +`headers` (the `Authorization` bearer) is added to the request Envoy forwards +*upstream* to the backend — invisible to the client. `response_headers_to_add` +is added to the response Envoy sends back *downstream* to the client, only +when a fresh bearer was actually minted (never on the `None` "client bearer +already valid" branch, since there is nothing new to hand back). This exists +so a browser SPA — which has no way to see the upstream-only `Authorization` +header — can still discover the bearer: an Envoy Lua filter on the response +path reads `X-Authz-Bootstrap-Token` and seeds it into the document response +(see the compose/Envoy simulation notes), letting the SPA boot already +authenticated without any user-visible OAuth redirect. + **Deny status policy:** - `retryable=True` (Vikunja unreachable / `5xx`) → `PERMISSION_DENIED` + diff --git a/envoy_authz/grpc_service.py b/envoy_authz/grpc_service.py index 14e035f..f673eb3 100644 --- a/envoy_authz/grpc_service.py +++ b/envoy_authz/grpc_service.py @@ -209,6 +209,7 @@ def Check(self, request, context): logger.info("✓ Authorized", extra=log_extra) return_headers: list[HeaderValueOption] = [] + response_headers: list[HeaderValueOption] = [] host = request.attributes.request.http.host # For allowed requests to Frigate, add the trusted proxy token header @@ -261,6 +262,21 @@ def Check(self, request, context): ) ) logger.info("injected-bearer sub=%s", subject.sub) + # Handed to Envoy's *response* path too (a header on the + # HttpConnectionManager response, not the upstream + # request `headers` above), so a downstream Lua filter can + # bootstrap the browser's own session. The Authorization + # header above is never visible to the client's own JS — + # it is added to the request Envoy forwards to Vikunja, + # not to the response Envoy sends back. + response_headers.append( + HeaderValueOption( + header=HeaderValue( + key="X-Authz-Bootstrap-Token", + value=upstream, + ), + ) + ) else: logger.info("allowed-through-client-bearer sub=%s", subject.sub) @@ -268,6 +284,7 @@ def Check(self, request, context): status=status_pb2.Status(code=code_pb2.OK), ok_response=external_auth_pb2.OkHttpResponse( headers=return_headers, + response_headers_to_add=response_headers, ), ) else: diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index ae4d9c6..8b58724 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -83,6 +83,34 @@ def test_check_injects_federated_bearer( assert resp.status.code == code_pb2.OK added = {h.header.key: h.header.value for h in resp.ok_response.headers} assert added["Authorization"] == f"Bearer {bearer}" + # Also handed to Envoy's *response* path (not just the upstream request), + # so a response-side filter can seed the browser's own session — the SPA + # has no way to see the upstream-only Authorization header above. + to_browser = { + h.header.key: h.header.value for h in resp.ok_response.response_headers_to_add + } + assert to_browser["X-Authz-Bootstrap-Token"] == bearer + + +@respx.mock +def test_check_omits_bootstrap_token_when_client_bearer_already_valid( + grpc_servicer, email_client_cert_pem, monkeypatch +): + """get_bearer returns None (client's own bearer verified locally) → no + fresh token was minted, so there is nothing new to bootstrap the browser + with; the response-side header must be absent, not merely empty.""" + from envoy_authz import grpc_service + + monkeypatch.setattr(grpc_service, "get_bearer", lambda *a, **k: None) + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/api/v1", + client_cert_pem=email_client_cert_pem, + bearer="client-bearer", + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.OK + assert list(resp.ok_response.response_headers_to_add) == [] def test_check_allows_through_when_get_bearer_returns_none( From 24d51e741df8bcdf941c5faeeac233c7f2f8920f Mon Sep 17 00:00:00 2001 From: Nick Venenga Date: Sat, 1 Aug 2026 13:41:48 -0400 Subject: [PATCH 2/3] feat(federation): browser-bootstrap document navigations, drop the Lua-header approach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the X-Authz-Bootstrap-Token response header from the prior commit: nothing ever consumed it (the Envoy Lua filter it was meant for was local-only scaffolding, never committed), so it was dead plumbing for an abandoned design. Reverted in favor of a mechanism that needs zero Envoy/Contour changes. A browser hitting a federated host directly still fell through to Vikunja's own client-side OIDC login (the Authorization header get_bearer injects is only ever visible to Vikunja's backend, never to the browser's JS) — which redirects to this OP's authorization_endpoint, a route that has never existed here by design (op/routes.py: codes are minted server-to-server only). Check() now detects a real browser navigation (Sec-Fetch-Dest: document, falling back to Accept: text/html) to a federated host and denies it with an HTTP 200 whose body does exactly what Vikunja's "Login with " button would: stores a `state` in localStorage, then navigates to Vikunja's callback with a code minted via the same create_authorization_code() federate() already uses. DeniedHttpResponse's status/headers/body are fully caller-controlled, so this replicates Vikunja's real front-channel flow (its OpenIdAuth.vue hard-fails on a state mismatch, so a bare server-initiated redirect that skipped setting it first would not work) with Envoy never proxying the hit to Vikunja at all. Two Vikunja-owned paths are exempted from both this and the existing ladder, or the exchange loops/breaks: the frontend callback route the bootstrap page navigates to, and the backend endpoint that redeems the code (now a shared vikunja.callback_path() helper instead of being inlined only in federate()). New envoy_authz/federator/browser_bootstrap.py holds the pure, independently-tested helpers (navigation detection, path derivation, HTML rendering with explicit escaping). Manually re-verified end to end against a real Envoy + Vikunja stack with a real homelab mTLS client cert: Firefox loads Vikunja fully authenticated with no visible redirect, login screen, or Envoy config changes. --- docs/federation.md | 66 +++++++++--- envoy_authz/federator/browser_bootstrap.py | 77 ++++++++++++++ envoy_authz/federator/vikunja.py | 12 ++- envoy_authz/grpc_service.py | 117 +++++++++++++++------ tests/unit/test_browser_bootstrap.py | 71 +++++++++++++ tests/unit/test_check.py | 97 +++++++++++++---- 6 files changed, 370 insertions(+), 70 deletions(-) create mode 100644 envoy_authz/federator/browser_bootstrap.py create mode 100644 tests/unit/test_browser_bootstrap.py diff --git a/docs/federation.md b/docs/federation.md index 051b8b0..34b6e13 100644 --- a/docs/federation.md +++ b/docs/federation.md @@ -31,15 +31,56 @@ Envoy terminates the client mTLS certificate and forwards the verified cert PEM 2. On the **Frigate path** (`host == FRIGATE_HOST`) the existing behavior is preserved: inject `X-Proxy-Secret`. Federation does **not** run there. 3. If the request host is **claimed by a provider** (its `hosts` allowlist, see - `providers.yaml`), the service **federates**: it derives a stable subject from - the cert, runs the `get_bearer` decision ladder, and either injects - `Authorization: Bearer ` upstream, lets the client's own bearer through - unchanged, or denies. + `providers.yaml`), the service derives a stable subject from the cert, then: + - Vikunja's **own OIDC paths** (the frontend callback route and the backend + endpoint that redeems a code) are let through **untouched** — see + [Browser bootstrap](#browser-bootstrap-document-navigations) below. + - A **document navigation** (a real browser address-bar/link hit, not an + API/XHR call) gets the browser-bootstrap `denied_response` instead of the + silent ladder — see below. + - Everything else **federates**: it runs the `get_bearer` decision ladder, + and either injects `Authorization: Bearer ` upstream, lets the + client's own bearer through unchanged, or denies. 4. Any other allowed host passes through with **no header added**. Host scoping is an allowlist on purpose: attaching this ext_authz to a new vhost must not silently start injecting some other backend's bearer over the client's own credential. +## Browser bootstrap (document navigations) + +`get_bearer`'s `Authorization` header is added to the request Envoy forwards +*upstream* — invisible to a browser's own JS. A browser SPA hitting Vikunja +directly therefore never becomes "logged in" from that alone: it still falls +through to Vikunja's own client-side OIDC login, which redirects to this +service's OP `authorization_endpoint` — a route that does not exist (see +`op/routes.py`: codes are minted server-to-server only, by design). + +Reconciling this without any Envoy/Contour changes: `envoy_authz.federator. +browser_bootstrap` detects a top-level navigation (`Sec-Fetch-Dest: document`, +falling back to `Accept: text/html`) to a federated host and, instead of the +silent ladder, denies with an HTTP **200** whose body is a small page that +does exactly what Vikunja's own "Login with ``" button would have: +stores a `state` in `localStorage`, then navigates to Vikunja's callback route +with a code this service already minted (`store.create_authorization_code`, +the same call `federate()` uses). `DeniedHttpResponse.status`/`headers`/`body` +are fully caller-controlled — Envoy sends them straight to the browser without +ever proxying that hit to Vikunja — so this replicates Vikunja's real, +CSRF-checked front-channel flow (`OpenIdAuth.vue` hard-fails on a `state` +mismatch, so a bare server-side redirect that skipped setting it first would +not work). + +Two paths must be exempted from this (and from the ladder) or the exchange +breaks: `frontend_oidc_path(provider)` (the callback page the bootstrap script +navigates to) and `vikunja.callback_path(provider)` (the API call that page +makes to redeem the code) are always let through untouched. + +There is no way to tell from the server side whether a browser already holds +a valid Vikunja JWT, so this runs on **every** document navigation to a +federated host, not just the first. That is intentional, not a cache miss: +this whole model derives a session from the mTLS cert per request, so a full +page reload legitimately re-deriving a fresh one is consistent with treating +"logout" as meaningless while the cert is presented. + ## The `get_bearer` decision ladder Per request, for a derived `Subject` (`sub` is a 16-char SHA-256 of the cert's @@ -78,23 +119,16 @@ per-request `GET /api/v1/user`. ## Return contract +This ladder only runs for non-navigation requests (see +[Browser bootstrap](#browser-bootstrap-document-navigations) above for how a +real browser page load is handled instead): + | `get_bearer` result | `Check` response | |---|---| -| `str` | `OK` + `headers: Authorization: Bearer ` + `response_headers_to_add: X-Authz-Bootstrap-Token: ` | +| `str` | `OK` + `headers: Authorization: Bearer ` (upstream request only) | | `None` | `OK` with **no** `Authorization` header (client's bearer allowed through) | | raises `DownstreamError` | deny | -`headers` (the `Authorization` bearer) is added to the request Envoy forwards -*upstream* to the backend — invisible to the client. `response_headers_to_add` -is added to the response Envoy sends back *downstream* to the client, only -when a fresh bearer was actually minted (never on the `None` "client bearer -already valid" branch, since there is nothing new to hand back). This exists -so a browser SPA — which has no way to see the upstream-only `Authorization` -header — can still discover the bearer: an Envoy Lua filter on the response -path reads `X-Authz-Bootstrap-Token` and seeds it into the document response -(see the compose/Envoy simulation notes), letting the SPA boot already -authenticated without any user-visible OAuth redirect. - **Deny status policy:** - `retryable=True` (Vikunja unreachable / `5xx`) → `PERMISSION_DENIED` + diff --git a/envoy_authz/federator/browser_bootstrap.py b/envoy_authz/federator/browser_bootstrap.py new file mode 100644 index 0000000..d0b2f89 --- /dev/null +++ b/envoy_authz/federator/browser_bootstrap.py @@ -0,0 +1,77 @@ +"""Silent front-channel login for a browser hitting a federated host directly. + +The existing get_bearer ladder (session.py) authenticates API/XHR calls by +injecting `Authorization` into the *upstream* request — invisible to a +browser's own JS, so a browser SPA never becomes "logged in" from it alone. +Vikunja's frontend instead expects a real OIDC front-channel redirect (its +`/auth/openid/` route validates a `state` it stored in localStorage +before redirecting out) — but this federator's OP has no `/oauth/authorize` +(codes are minted server-to-server only, see op/routes.py). Reconciling this +without any Envoy/Contour changes: Check() detects a top-level browser +navigation and denies it with an HTTP 200 whose body is a tiny script that +does exactly what Vikunja's own "Login with " button would have +done (store `state`, then navigate to the callback route with a code we +already minted) before Vikunja's real page is ever served. Envoy sends a +`denied_response` straight to the client without proxying anything upstream, +so this never touches Vikunja. +""" + +import html +import secrets +from urllib.parse import urlparse + +from .providers import Provider + + +def is_document_navigation(headers: dict) -> bool: + """True for a top-level browser navigation, not an API/XHR/asset request. + + `Sec-Fetch-Dest` is sent by all current Chromium/Firefox releases and is + unambiguous (`document` only for a real address-bar/link navigation). + Falls back to `Accept: text/html` for clients that omit Sec-Fetch-* + (older browsers, curl, the API test suite), so those keep going through + the existing silent get_bearer ladder rather than getting a page. + """ + dest = headers.get("sec-fetch-dest") + if dest is not None: + return dest == "document" + return "text/html" in headers.get("accept", "") + + +def frontend_oidc_path(provider: Provider) -> str: + """Vikunja's own frontend OIDC-callback route (not the backend API path). + + A request here must be let through untouched: it is the page navigation + the bootstrap script above triggers, and Vikunja's SPA (OpenIdAuth.vue) + needs to actually render and run there. + """ + return urlparse(provider.redirect_url).path + + +def render_bootstrap_html(*, redirect_path: str, code: str, state: str) -> str: + """The whole HTTP response body for the browser-bootstrap denied_response. + + Mirrors Vikunja's frontend `redirectToProvider()` exactly (store `state` + in localStorage, then navigate) so its callback's CSRF check passes as if + the user had clicked its own "Login with " button. `code` and + `state` are our own (itsdangerous URL-safe base64 / secrets.token_urlsafe) + output and cannot contain a quote or ``, but both are still + HTML-escaped before being embedded in a JS string literal — templating + untrusted-shaped values into a script body without escaping is exactly how + an XSS hole gets introduced later if the value ever changes shape. + """ + safe_code = html.escape(code, quote=True) + safe_state = html.escape(state, quote=True) + safe_path = html.escape(redirect_path, quote=True) + return ( + '' + "" + "" + ) + + +def new_state() -> str: + return secrets.token_urlsafe(18) diff --git a/envoy_authz/federator/vikunja.py b/envoy_authz/federator/vikunja.py index c18d22c..dbcf806 100644 --- a/envoy_authz/federator/vikunja.py +++ b/envoy_authz/federator/vikunja.py @@ -24,6 +24,16 @@ _REFRESH_COOKIE_NAME = "vikunja_refresh_token" +def callback_path(provider: Provider) -> str: + """The Vikunja backend path that redeems a federation auth code. + + Shared with the browser-bootstrap path (grpc_service.Check): that path must + let a request to this exact path through untouched rather than treat it as + a plain federated request, or Vikunja's own callback exchange never runs. + """ + return f"/api/v1/auth/openid/{provider.provider_key}/callback" + + class DownstreamError(Exception): """Vikunja returned an error or was unreachable. @@ -124,7 +134,7 @@ def federate(self, subject: Subject) -> VikunjaSession: name=subject.name, nonce=None, ) - callback = f"/api/v1/auth/openid/{self._provider.provider_key}/callback" + callback = callback_path(self._provider) try: resp = self._client.post( callback, diff --git a/envoy_authz/grpc_service.py b/envoy_authz/grpc_service.py index f673eb3..9f587c9 100644 --- a/envoy_authz/grpc_service.py +++ b/envoy_authz/grpc_service.py @@ -13,10 +13,17 @@ from envoy_authz.identity import parse_client_identity from .config import Config, verify_client_cert +from .federator.browser_bootstrap import ( + frontend_oidc_path, + is_document_navigation, + new_state, + render_bootstrap_html, +) from .federator.providers import PROVIDERS, get_provider, provider_for_host from .federator.session import SessionCache, get_bearer +from .federator.store import create_authorization_code from .federator.subject import derive_subject -from .federator.vikunja import DownstreamError, VikunjaClient +from .federator.vikunja import DownstreamError, VikunjaClient, callback_path logger = logging.getLogger(__name__) @@ -209,7 +216,6 @@ def Check(self, request, context): logger.info("✓ Authorized", extra=log_extra) return_headers: list[HeaderValueOption] = [] - response_headers: list[HeaderValueOption] = [] host = request.attributes.request.http.host # For allowed requests to Frigate, add the trusted proxy token header @@ -235,7 +241,7 @@ def Check(self, request, context): _vikunja is not None and _SESSIONS is not None and client_cert is not None - and provider_for_host(host) is not None + and (provider := provider_for_host(host)) is not None ): try: # identity may be None here if the best-effort parse above @@ -246,45 +252,92 @@ def Check(self, request, context): except Exception: logger.exception("Failed to derive subject for federation") return _deny(retryable=False) - incoming_bearer = _extract_bearer(headers) - try: - upstream = get_bearer(subject, incoming_bearer, _vikunja, _SESSIONS) - except DownstreamError as exc: - logger.warning("denied-federation-failure sub=%s", subject.sub) - return _deny(exc.retryable) - if upstream is not None: - return_headers.append( - HeaderValueOption( - header=HeaderValue( - key="Authorization", - value=f"Bearer {upstream}", - ), + + if path in (frontend_oidc_path(provider), callback_path(provider)): + # Vikunja's own OIDC machinery: the callback page the + # bootstrap script below navigates to, and the backend + # call that page makes to redeem the code. Neither needs + # (or should get) an injected bearer, and intercepting + # either would break the exchange rather than complete it. + logger.info("allowed-through-oidc-path sub=%s", subject.sub) + elif is_document_navigation(headers): + # A real address-bar/link navigation, not an API/XHR call. + # The Authorization header the ladder below injects is only + # ever visible to Vikunja's *backend* — never to the + # browser's own JS — so a browser hitting this host + # directly would still fall through to Vikunja's own + # client-side OIDC login, which this federator's OP has no + # /oauth/authorize to answer (op/routes.py: codes are + # minted server-to-server only). Do what clicking + # Vikunja's "Login" button would have done instead: mint a + # code, then deny with a page that stores `state` and + # navigates to the callback exactly as that button would + # — Envoy sends `denied_response` straight to the browser + # without ever proxying this hit to Vikunja. + if not subject.email: + logger.warning( + "cannot bootstrap sub=%s: client cert has no email " + "(rfc822Name SAN)", + subject.sub, ) + return _deny(retryable=False) + code = create_authorization_code( + client_id=provider.client_id, + redirect_uri=provider.redirect_url, + scope=provider.scope, + user_id=subject.sub, + email=subject.email, + name=subject.name, + nonce=None, ) - logger.info("injected-bearer sub=%s", subject.sub) - # Handed to Envoy's *response* path too (a header on the - # HttpConnectionManager response, not the upstream - # request `headers` above), so a downstream Lua filter can - # bootstrap the browser's own session. The Authorization - # header above is never visible to the client's own JS — - # it is added to the request Envoy forwards to Vikunja, - # not to the response Envoy sends back. - response_headers.append( - HeaderValueOption( - header=HeaderValue( - key="X-Authz-Bootstrap-Token", - value=upstream, + logger.info("bootstrap-redirect sub=%s", subject.sub) + return external_auth_pb2.CheckResponse( + status=status_pb2.Status(code=code_pb2.PERMISSION_DENIED), + denied_response=external_auth_pb2.DeniedHttpResponse( + status=http_status_pb2.HttpStatus( + code=http_status_pb2.StatusCode.OK ), - ) + headers=[ + HeaderValueOption( + header=HeaderValue( + key="Content-Type", + value="text/html; charset=utf-8", + ), + ), + ], + body=render_bootstrap_html( + redirect_path=frontend_oidc_path(provider), + code=code, + state=new_state(), + ), + ), ) else: - logger.info("allowed-through-client-bearer sub=%s", subject.sub) + incoming_bearer = _extract_bearer(headers) + try: + upstream = get_bearer( + subject, incoming_bearer, _vikunja, _SESSIONS + ) + except DownstreamError as exc: + logger.warning("denied-federation-failure sub=%s", subject.sub) + return _deny(exc.retryable) + if upstream is not None: + return_headers.append( + HeaderValueOption( + header=HeaderValue( + key="Authorization", + value=f"Bearer {upstream}", + ), + ) + ) + logger.info("injected-bearer sub=%s", subject.sub) + else: + logger.info("allowed-through-client-bearer sub=%s", subject.sub) return external_auth_pb2.CheckResponse( status=status_pb2.Status(code=code_pb2.OK), ok_response=external_auth_pb2.OkHttpResponse( headers=return_headers, - response_headers_to_add=response_headers, ), ) else: diff --git a/tests/unit/test_browser_bootstrap.py b/tests/unit/test_browser_bootstrap.py new file mode 100644 index 0000000..b8a427d --- /dev/null +++ b/tests/unit/test_browser_bootstrap.py @@ -0,0 +1,71 @@ +"""Unit tests for the browser-bootstrap helpers (pure functions, no gRPC).""" + +from envoy_authz.federator.browser_bootstrap import ( + frontend_oidc_path, + is_document_navigation, + render_bootstrap_html, +) +from envoy_authz.federator.providers import Provider + + +def _provider(**overrides) -> Provider: + defaults = { + "hosts": ["vikunja.example.com"], + "client_id": "vikunja", + "client_secret": "s", + "redirect_url": "https://vikunja.example.com/auth/openid/broker", + "api_base": "http://vikunja:3456", + "provider_key": "broker", + } + defaults.update(overrides) + return Provider(**defaults) + + +def test_is_document_navigation_true_for_sec_fetch_dest_document(): + assert is_document_navigation({"sec-fetch-dest": "document"}) is True + + +def test_is_document_navigation_false_for_sec_fetch_dest_other_values(): + # A real browser sends this for scripts/styles/images/XHR — must not be + # treated as a page load even though the same browser is asking. + for dest in ("script", "style", "image", "empty"): + assert is_document_navigation({"sec-fetch-dest": dest}) is False + + +def test_is_document_navigation_falls_back_to_accept_header(): + # No Sec-Fetch-Dest (older browser / curl / the existing API tests) — + # Accept: text/html is the fallback signal. + assert is_document_navigation({"accept": "text/html,application/xhtml+xml"}) + assert not is_document_navigation({"accept": "application/json"}) + assert not is_document_navigation({}) + + +def test_frontend_oidc_path_is_the_redirect_url_path_component(): + provider = _provider(redirect_url="https://vikunja.example.com/auth/openid/broker") + assert frontend_oidc_path(provider) == "/auth/openid/broker" + + +def test_render_bootstrap_html_sets_state_before_navigating(): + body = render_bootstrap_html( + redirect_path="/auth/openid/broker", code="a.b.c", state="xyz123" + ) + # Order matters: Vikunja's own redirectToProvider() stores state THEN + # navigates — reversing this would fail its CSRF check every time. + set_state_index = body.index("localStorage.setItem('state', 'xyz123')") + navigate_index = body.index("window.location.href") + assert set_state_index < navigate_index + assert "/auth/openid/broker?code=a.b.c&state=xyz123" in body + + +def test_render_bootstrap_html_escapes_html_special_characters(): + # code/state are our own itsdangerous/secrets output and never contain + # these characters in practice, but the render must not silently trust + # that — an unescaped `` or quote would be a real XSS hole the + # moment either value's shape ever changes. + body = render_bootstrap_html( + redirect_path="/auth/openid/broker", + code="", + state="'; alert(2); '", + ) + assert "" not in body + assert "'; alert(2); '" not in body diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index 8b58724..1fb4173 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -83,22 +83,13 @@ def test_check_injects_federated_bearer( assert resp.status.code == code_pb2.OK added = {h.header.key: h.header.value for h in resp.ok_response.headers} assert added["Authorization"] == f"Bearer {bearer}" - # Also handed to Envoy's *response* path (not just the upstream request), - # so a response-side filter can seed the browser's own session — the SPA - # has no way to see the upstream-only Authorization header above. - to_browser = { - h.header.key: h.header.value for h in resp.ok_response.response_headers_to_add - } - assert to_browser["X-Authz-Bootstrap-Token"] == bearer -@respx.mock -def test_check_omits_bootstrap_token_when_client_bearer_already_valid( +def test_check_allows_through_when_get_bearer_returns_none( grpc_servicer, email_client_cert_pem, monkeypatch ): - """get_bearer returns None (client's own bearer verified locally) → no - fresh token was minted, so there is nothing new to bootstrap the browser - with; the response-side header must be absent, not merely empty.""" + # get_bearer returns None → OK with NO Authorization header (client's + # incoming bearer was verified locally). from envoy_authz import grpc_service monkeypatch.setattr(grpc_service, "get_bearer", lambda *a, **k: None) @@ -110,27 +101,91 @@ def test_check_omits_bootstrap_token_when_client_bearer_already_valid( ) resp = grpc_servicer.servicer.Check(req, None) assert resp.status.code == code_pb2.OK - assert list(resp.ok_response.response_headers_to_add) == [] + added = {h.header.key: h.header.value for h in resp.ok_response.headers} + assert "Authorization" not in added -def test_check_allows_through_when_get_bearer_returns_none( +def test_check_document_navigation_returns_bootstrap_page( + grpc_servicer, email_client_cert_pem +): + """A real browser navigation (Sec-Fetch-Dest: document) to a federated + host gets a denied_response whose HTTP status is 200 and whose body is a + page that stores `state` and navigates to the callback — mirroring + Vikunja's own "Login" button — rather than the silent Authorization + header, which a browser SPA can never see.""" + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/", + client_cert_pem=email_client_cert_pem, + headers={"sec-fetch-dest": "document"}, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.PERMISSION_DENIED + assert resp.denied_response.status.code == http_status_pb2.StatusCode.OK + added = {h.header.key: h.header.value for h in resp.denied_response.headers} + assert added["Content-Type"] == "text/html; charset=utf-8" + assert "localStorage.setItem('state'" in resp.denied_response.body + assert "/auth/openid/broker?code=" in resp.denied_response.body + + +def test_check_denies_bootstrap_when_cert_has_no_email( + grpc_servicer, trusted_client_cert_pem +): + # trusted_client_cert_pem carries no rfc822Name SAN — same "cannot + # provision a downstream user" constraint as the existing federate() path. + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/", + client_cert_pem=trusted_client_cert_pem, + headers={"sec-fetch-dest": "document"}, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.PERMISSION_DENIED + assert resp.denied_response.status.code == http_status_pb2.StatusCode.Unauthorized + + +def test_check_frontend_oidc_path_passes_through_untouched( grpc_servicer, email_client_cert_pem, monkeypatch ): - # get_bearer returns None → OK with NO Authorization header (client's - # incoming bearer was verified locally). + """The callback page the bootstrap script navigates to must render + normally, not get intercepted into another bootstrap page (which would + loop) or federated (get_bearer must not run for it).""" from envoy_authz import grpc_service - monkeypatch.setattr(grpc_service, "get_bearer", lambda *a, **k: None) + def _boom(*a, **k): + raise AssertionError("get_bearer must not run on Vikunja's own OIDC path") + + monkeypatch.setattr(grpc_service, "get_bearer", _boom) req = grpc_servicer.check_request( host="vikunja.example.com", - path="/api/v1", + path="/auth/openid/broker", client_cert_pem=email_client_cert_pem, - bearer="client-bearer", + headers={"sec-fetch-dest": "document"}, ) resp = grpc_servicer.servicer.Check(req, None) assert resp.status.code == code_pb2.OK - added = {h.header.key: h.header.value for h in resp.ok_response.headers} - assert "Authorization" not in added + assert [h.header.key for h in resp.ok_response.headers] == [] + + +def test_check_backend_oidc_callback_path_passes_through_untouched( + grpc_servicer, email_client_cert_pem, monkeypatch +): + """The frontend's own POST redeeming the code must reach Vikunja + untouched too, even though it is not a document navigation.""" + from envoy_authz import grpc_service + + def _boom(*a, **k): + raise AssertionError("get_bearer must not run on Vikunja's own OIDC path") + + monkeypatch.setattr(grpc_service, "get_bearer", _boom) + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/api/v1/auth/openid/broker/callback", + client_cert_pem=email_client_cert_pem, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.OK + assert [h.header.key for h in resp.ok_response.headers] == [] def test_check_denies_503_on_retryable_federation_failure( From ef53097aee9d0333ae0810c9de62f42a89fb58f8 Mon Sep 17 00:00:00 2001 From: Nick Venenga Date: Sat, 1 Aug 2026 13:52:08 -0400 Subject: [PATCH 3/3] fix(federation): match bootstrap-exempt paths without the query string Envoy's request.path always includes the query string, so the OIDC callback route (hit as .../broker?code=...&state=...) never matched the bare-path comparison and fell through to another bootstrap cycle. Found via live curl verification against a real Envoy + Vikunja stack. --- envoy_authz/grpc_service.py | 9 ++++++++- tests/unit/test_check.py | 5 ++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/envoy_authz/grpc_service.py b/envoy_authz/grpc_service.py index 9f587c9..79a8083 100644 --- a/envoy_authz/grpc_service.py +++ b/envoy_authz/grpc_service.py @@ -253,7 +253,14 @@ def Check(self, request, context): logger.exception("Failed to derive subject for federation") return _deny(retryable=False) - if path in (frontend_oidc_path(provider), callback_path(provider)): + # Envoy's request.path includes the query string (Vikunja's + # callback is always hit as .../broker?code=...&state=...), so + # comparing the raw path against a bare route never matches. + path_no_query = path.split("?", 1)[0] + if path_no_query in ( + frontend_oidc_path(provider), + callback_path(provider), + ): # Vikunja's own OIDC machinery: the callback page the # bootstrap script below navigates to, and the backend # call that page makes to redeem the code. Neither needs diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index 1fb4173..3fbc284 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -156,9 +156,12 @@ def _boom(*a, **k): raise AssertionError("get_bearer must not run on Vikunja's own OIDC path") monkeypatch.setattr(grpc_service, "get_bearer", _boom) + # Real Envoy always includes the query string in `path` — this is exactly + # how the bootstrap script's own navigation (?code=...&state=...) arrives. + # A bare-path comparison with no query string would pass vacuously here. req = grpc_servicer.check_request( host="vikunja.example.com", - path="/auth/openid/broker", + path="/auth/openid/broker?code=abc123&state=xyz789", client_cert_pem=email_client_cert_pem, headers={"sec-fetch-dest": "document"}, )