fix(gateway): split QR login requester and requestee tokens #363

caution-bot commented 2026年07月04日 02:58:29 +02:00

Summary

• generate a separate requestee token for the QR login URL while keeping the requester token for status polling
• store and look up the requestee token for browser-side QR authentication and completion
• add a focused regression test that ensures the QR URL does not expose the requester token

Fixes #336

Test plan

• cargo test -p gateway qr_login_url_uses_requestee_token_only -- --nocapture
• cargo test -p gateway
• cargo clippy -p gateway --all-targets --all-features (passes with existing warnings)
• cargo test -p api -p gateway
• git diff --check

## Summary - generate a separate requestee token for the QR login URL while keeping the requester token for status polling - store and look up the requestee token for browser-side QR authentication and completion - add a focused regression test that ensures the QR URL does not expose the requester token Fixes #336 ## Test plan - `cargo test -p gateway qr_login_url_uses_requestee_token_only -- --nocapture` - `cargo test -p gateway` - `cargo clippy -p gateway --all-targets --all-features` (passes with existing warnings) - `cargo test -p api -p gateway` - `git diff --check`

vkobel commented 2026年07月08日 17:23:25 +02:00

Pushed two follow-up fixes to the consume path, both verified end-to-end against a local stack:

1. consume_qr_login_session_id was broken — SET session_id = NULL ... RETURNING session_id returns the post-update value (NULL), so it failed to decode and 500'd on every completed poll, while the UPDATE still committed. Net effect: every successful QR login would 500 and permanently lose its session. Fixed by capturing the old value in a FOR UPDATE CTE.
2. Consume-before-fetch was fragile — reordered so the auth session is fetched before consuming; a transient failure (e.g. get_auth_session error, dropped response) no longer orphans a genuine login — the next poll recovers it.

Verified: gateway unit tests, seeded one-shot/split/retry checks, a begin+authenticate split walk, and a full ceremony with a real signed WebAuthn assertion (Chrome virtual authenticator) — first poll returns the session, second returns none, requestee token can't poll status. Also added the retry-safety/one-shot/split cases to tests/cross-device-qr.md

Pushed two follow-up fixes to the consume path, both verified end-to-end against a local stack: 1. `consume_qr_login_session_id` was broken — `SET session_id = NULL ... RETURNING session_id` returns the post-update value (NULL), so it failed to decode and 500'd on every completed poll, while the UPDATE still committed. Net effect: every successful QR login would 500 and permanently lose its session. Fixed by capturing the old value in a FOR UPDATE CTE. 2. Consume-before-fetch was fragile — reordered so the auth session is fetched before consuming; a transient failure (e.g. get_auth_session error, dropped response) no longer orphans a genuine login — the next poll recovers it. Verified: gateway unit tests, seeded one-shot/split/retry checks, a begin+authenticate split walk, and a full ceremony with a real signed WebAuthn assertion (Chrome virtual authenticator) — first poll returns the session, second returns none, requestee token can't poll status. Also added the retry-safety/one-shot/split cases to `tests/cross-device-qr.md`

vkobel commented 2026年07月08日 17:46:31 +02:00

Addressed the review:

• Automated test (the main gap): added tests/e2e/test_qr_login.sh + make test-e2e-qr-login, asserting the requester/requestee split, one-shot consumption, requestee-can't-poll-status, and retry-safety (fault-injects a failing get_auth_session and verifies the session id survives). 8/8 pass; it fails on the pre-fix code.
• Reworded the now-inaccurate runbook comment (consume happens after the session fetch, not after building the response).
• Added a tracing::debug! when a completed poll finds the session already consumed.

On the concurrent-poll race: the e2e covers one-shot behavior, but a true concurrency test for the FOR UPDATE CTE needs a DB-backed harness this crate doesn't have yet — flagging as a follow-up rather than expanding scope here. Left the lost-race response as completed/session_id: null (no new sub-status) since the requester token is polled by a single client sequentially, so that path is just the expected second poll.

Addressed the review: - Automated test (the main gap): added tests/e2e/test_qr_login.sh + make test-e2e-qr-login, asserting the requester/requestee split, one-shot consumption, requestee-can't-poll-status, and retry-safety (fault-injects a failing get_auth_session and verifies the session id survives). 8/8 pass; it fails on the pre-fix code. - Reworded the now-inaccurate runbook comment (consume happens after the session fetch, not after building the response). - Added a tracing::debug! when a completed poll finds the session already consumed. On the concurrent-poll race: the e2e covers one-shot behavior, but a true concurrency test for the FOR UPDATE CTE needs a DB-backed harness this crate doesn't have yet — flagging as a follow-up rather than expanding scope here. Left the lost-race response as completed/session_id: null (no new sub-status) since the requester token is polled by a single client sequentially, so that path is just the expected second poll.