fix(auth): close the backslash open-redirect bypass; finish the P1 matrix (SEC-05, SC-12, MT-05)

[camreeves]

The defect

sanitize_continue is the open-redirect guard for every continue target. It returns a value as-is when it looks like a relative path — starts with /, contains no //. That // test ran against the raw string, and it is not what a browser checks.

WHATWG URL parsing treats \ exactly like / while resolving a special (http/https) URL — relative slash state: if c is U+002F (/) or U+005C (\), go to special authority ignore slashes state. So a browser reads

Location: /\evil.example/x

as //evil.example/x — scheme-relative — and navigates off-site. Both /\evil.example and /\/evil.example walked past the guard and were emitted verbatim.

"/dashboard"          RETURNED-AS-SAFE-RELATIVE
"//evil.example/x"    falls through to URI.parse (host-checked)   <- guard works
"/\evil.example/x"    RETURNED-AS-SAFE-RELATIVE                   <- bypass
"/\/evil.example/x"   RETURNED-AS-SAFE-RELATIVE                   <- bypass

Crystal's parser disagrees with the browser here — URI.parse("/\\evil.example").host is nil, i.e. it reports the hostile value as harmless. A spec asserting through URI.parse would have confirmed the bug as safe. The new specs resolve Location the way a browser does instead.

The fix normalises backslashes before the check, pushing those inputs down the absolute-URL branch where the authority-host comparison rejects them.

Where it was reachable

• POST /auth/signin — sanitize_continue(continue) || "/"
• GET /auth/logout — same guard
• the SSO callback — set_continue sanitises on write and provider_callbacks.cr:195 replays the stored value without re-checking, so a hostile continue could be parked on the session and fired after a successful login

Coverage

SEC-05 / SC-12 — a table-driven sweep of 7 hostile targets across signin, logout, login and the provider hand-off:

form	example
protocol-relative	//evil.example/x
backslash	/\evil.example/x
mixed	/\/evil.example/x
absolute	https://evil.example/x, http://evil.example/x
host suffix	https://localhost.evil.example/x
userinfo	https://localhost@evil.example/x

…plus the safe cases that must keep working (relative path preserved byte-for-byte, // allowed inside a query string, same-host absolute reduced to its path), and the full three-hop SSO round trip in oauth_provider_flows_spec.cr.

The authorize grant redirect is already constrained to a registered redirect_uri — authorize_validation_spec.cr (AU-03/AU-04) and authorize_flow_spec.cr (deny path) — so it is referenced, not duplicated.

MT-05 — the verified asset-access cookie on /auth/authority, the most-called endpoint in the estate. Re-issued for a Bearer token and for an X-API-Key, not issued to an unauthenticated caller, and — the subtle one — a stale session's teardown must not clobber the fresh cookie. configure_asset_access runs after signed_in? deliberately, because resolving a dead session calls remove_session → clear_asset_access. Reversed, a client holding a logged-out session cookie and a good token gets the expired cookie while being told token_valid: true, then bounces to /auth/login on its next asset fetch. The comment said so; nothing enforced it. Moving the call now fails that spec and only that spec.

One thing pinned rather than fixed — wants your call

login_failure reflects the Referer header verbatim into Location (sessions.cr:195). I did not change it:

• it is Ruby parity — dropin-audit.md:22 records the legacy login_failure -> redirect_to referer;
• the exposure is narrow: the victim must already be on the attacker's page and submit a login form from it, at which point the attacker can navigate them anywhere anyway;
• and the session cookie is deliberately SameSite=None for embedded login, so a cross-site Referer here can be a legitimate embedder that the retry is meant to return to. Restricting it to same-host is a product decision about embedded login, not pure hardening.

It is now pinned by a spec that states all of the above, so the behaviour is a reviewed choice rather than an accident. Say the word and I'll restrict it.

Verification

• 312 examples, 0 failures, 1 pending (the known AU-11 RFC-SHOULD half).
• Red-checked three ways: without the backslash normalisation the signin sweep, the logout sweep and the SSO round-trip fail; moving configure_asset_access above signed_in? fails the stale-session spec alone. Nothing else moves in either case.
• crystal tool format --check src spec clean.

Matrix state after this PR

Every P1 row is now closed except three, all blocked on something outside the repo: ID-13 (needs dev SSO credentials), CFG-03 and CFG-05 (need the parked helm/compose changes).

Two P1 rows were closed outside auth.cr and are not in this diff: SC-06 is now automated in tasks/PPT-2536/config-parity/check.sh (compose passes — auth and nginx share .env.secret_key; helm WARNs, because there is no nginx chart in k8s-helm at all, so the pairing is unverifiable from committed config — same root cause as CFG-02).

And a correction worth flagging: I re-read every P1 row marked ❌/◐ against the specs, and 17 of them were already covered — ID-03 (SAML signature enforcement) was marked "❌ deferred" while carrying five assertions including defect 1's own regression test. Those are now marked VERIFIED-COVERED with the covering example named.

🤖 Generated with Claude Code