Qr cross device#2
Merged
Merged
Conversation
Adds a modern passwordless flow: desktop renders a QR code, user
scans on phone, approves on phone, desktop signs in. Same UX
pattern most consumer apps have shipped in the last few years.
Backend:
- 00003_cross_device_pairings.sql: pending -> approved -> consumed
lifecycle, hashed code, partial index on (code_hash WHERE status=
'pending'), TTL 90s, terminal-row grace 1h
- POST /auth/pair/start anonymous; returns pairingId + raw code +
qrUrl + expiresIn
- GET /auth/pair/wait anonymous; desktop polls. 425 while pending,
200 + TokenPair on first call after approve (atomic consume race
resolves to one winner), 410 on any terminal state
- POST /auth/pair/approve authed via phone session; flips pending
to approved bound to phone's user_id, app-id-guarded so phone
signed into app A can't approve a pairing for app B
- POST /auth/pair/cancel authed; flips pending to denied
- GET /auth/pair/qr public; renders text as a 256x256 PNG via
skip2/go-qrcode (added to go.mod)
- Janitor sweep added (matches OIDC pattern)
UI:
- GET /x/{slug}/apps/{appId}/qr-sign-in - desktop hosted page,
pure inline JS, starts pairing on load, displays QR, polls,
on success redirects to ?return_to= with tokens in the fragment
(same delivery as magic-link). Validates return_to is http(s)
to defend against javascript:/data: schemes
- GET /x/{slug}/apps/{appId}/pair?c=... - phone landing, mounts
AppKit for sign-in if needed, then reveals an Approve/Cancel
overlay that POSTs with credentials:include
- Both pages have X-Frame-Options + frame-ancestors CSP
Security:
- 32-byte random code (256 bits) hashed server-side; raw never
persisted
- Atomic single-statement transitions (UPDATE...WHERE...RETURNING)
for pending->approved AND approved->consumed so concurrent
callers race cleanly
- Cross-app approve rejected via aud-bound JWT check at /approve
plus app_id guard at the repo UPDATE
- Anonymous /start is rate-limit-bounded by the existing per-app
attempts machinery
- Requires cookie transport mode (the approve POST relies on a
same-origin session cookie; local-mode falls through to 401)
13 tests cover: start, wait-pending=425, happy path mints tokens,
double-consume=410, approve-without-session=401, unknown-code=404,
cross-app approve=401, cancel makes wait return 410, QR PNG signature,
QR empty text=400, qr-sign-in page renders + anti-clickjacking,
javascript: return_to rejected, /pair page anti-clickjacking.
Customer integration: link your app to
/x/{slug}/apps/{appId}/qr-sign-in?return_to=<your-callback>
and tokens arrive in the fragment on the customer's domain.
Three findings from audit pass on the QR commit: 1. **Open redirector on /qr-sign-in (security).** Previously any http(s) return_to was accepted, letting an attacker craft a URL that routed tokens to evil.com via the fragment. Now: when a return_to is supplied, it MUST match app.AppURL's host (the customer-configured "where my app lives" field). If app.AppURL isn't set, no return_to is accepted at all. Pattern mirrors OAuth redirect_uri allowlisting using existing config — no new schema. 2. **/pair only worked in cookie transport mode.** The phone-side approve POST was relying on credentials:include (cookies) which silently 401'd for local-mode apps (the default!). Now the page also captures the JWT via AppKit's onJWT callback and sends it in Authorization: Bearer alongside credentials:include — both transport modes work seamlessly. Server-side auth check is unchanged (the existing GetSession reads either source). 3. **Cache-Control: no-store on token-bearing responses.** /start returns the raw pairing code; /wait returns the full TokenPair on approve. Both now carry no-store + Pragma: no-cache so intermediate proxies don't keep them. Matches the OIDC /token posture. Plus three regression tests: return_to rejected when app_url unset, return_to with mismatched host rejected even with app_url set, Cache-Control: no-store asserted on /start AND /wait responses. 17 pairing tests pass (was 13). Full repo green.
Three findings from a second-pass fresh audit on the QR branch: - Remove unused IsApprovedAndAvailable method on CrossDevicePairing — defined as a v0 convenience but never actually called; /wait does the same logic inline against the repo-side WHERE clauses anyway. - Remove `_ = userID` carry-over from the earlier audit's commented intent in the happy-path test. The test now actually uses userID for the assertion below. - Strengthen TestPair_HappyPath_StartApproveWaitMintsTokens: it previously only checked the access token was three-segment-shaped (i.e. "is JWT"). Now it base64-decodes the payload and asserts the sub claim equals the approver's user_id — catches a class of "tokens minted for wrong user" bug the structural check missed. Items reviewed and deliberately accepted: - /start still unrate-limited (janitor TTL bounds row growth at ~90s × spam-rate; existing per-app attempts machinery could be threaded if it ever matters) - Port-strict return_to host check (different port = different origin per RFC 6454, correct) - Brief AppKit flicker on /pair before approve panel reveals (cosmetic, fixable with default-hidden CSS state if it bugs anyone) - max_sessions could prune the phone session on desktop sign-in (existing per-app behaviour, applies to every sign-in flow) - /pair has no abandon-flow UI if the user never signs in (minor UX — refresh-the-tab works) 17 pairing tests pass. Full repo green.
Closes the gap between "feature exists" and "feature usable":
Backend
- 00004_app_qr_sign_in.sql: qr_sign_in_enabled boolean DEFAULT
false. Off-by-default, matches OIDC + new-feature convention.
- App.QRSignInEnabled on core.App; threaded through appColumnsReturning
+ scanAppFull + all SELECT/Scan sites in appRepo.go.
- Gating on /auth/pair/start, /qr-sign-in, /pair (phone landing) —
404 when toggle off so the entire QR surface is invisible.
- Admin endpoint PUT /admin/.../apps/{appId}/qr-sign-in-config in
appHandlerQRSignIn.go. Returns extended response with the
customer-facing QR URL. Enable requires app_url (returns
error.qrSignInRequiresAppURL otherwise) — same shape as
OIDC's ErrOIDCRequiresCookieTransport.
- AppResource (public app config) exposes qrSignInEnabled so AppKit
knows whether to render the button.
Fragment-timestamp fix
- The /qr-sign-in JS was sending mr_expires as RFC3339, but
AppKit's fragment reader does parseInt(...) * 1000 — silently
produced NaN and AppKit would reject the tokens. Now converts
to Unix seconds inline. The happy path was technically broken
end-to-end before this fix.
AppKit-side
- qrSignInEnabled prop on Auth + AppKit, plumbed from the public
app config.
- "Phone" button in the OAuth row on the login screen, conditionally
rendered. Navigates to /qr-sign-in?return_to=window.location.href;
tokens come back in the fragment (same delivery as magic-link),
AppKit's existing fragment reader picks them up.
- New "qrcode" FontAwesome icon registration.
Admin UI
- New "QR sign-in" tab in AppAuthMethods.tsx. Enable toggle with
app_url-required warning, copy-paste QR sign-in URL display.
Tests (3 new admin + 2 new gate, on top of 16 existing):
- TestAdminQRConfig_RejectsEnableWithoutAppURL
- TestAdminQRConfig_EnableSucceedsWithAppURL
- TestAdminQRConfig_DisableAlwaysAllowed
- TestPair_StartRespects404WhenDisabled
- TestPair_QRSignInPageRespects404WhenDisabled
- TestPair_QRSignInPageRejectsReturnToWhenAppURLNotSet updated for
the new fixture (which pre-sets app_url).
21 QR-related tests passing. Full repo green. tsc + vite build
clean for both appkit-ui and manyrows-ui.
1. Admin UI was showing a wrong QR sign-in URL.
The QRSignInCard built the URL from cardURL (the admin-side
/admin/workspace/<wsId>/products/<prodId>/apps/<appId> path)
plus "/qr-sign-in". But the actual endpoint lives at the PUBLIC
per-app path /x/<workspaceSlug>/apps/<appId>/qr-sign-in. The
string rendered in the admin tab was a 404. Customers copying
it would paste a broken link into their app.
Fix: surface QRSignInURL via toAdminAppResponse (server-computed
using AppBaseURL + workspace.Slug + app.ID), same pattern as the
Google/Apple/Microsoft/Github redirect URIs that already live
alongside. UI reads app.qrSignInUrl directly; if BASE_URL isn't
pinned yet, the field is empty and the UI shows a "URL will
appear once MANYROWS_BASE_URL is pinned" hint instead of a
broken value.
2. /pair/wait was ungated by the toggle.
The toggle only gated /start, /qr-sign-in (page), and /pair
(phone landing). In-flight pairings approved BEFORE the admin
flipped off could still mint tokens via /wait — bounded by the
90s TTL, but "disable means disable, except for 90 seconds"
is a confusing semantic.
Fix: gate /pair/wait too. /approve and /cancel stay ungated —
they're idempotent and don't mint tokens, so letting the phone
complete a cancel after admin-flip is fine.
Tests:
- TestAdminQRConfig_URLSurfacedOnRegularGet: verifies qrSignInUrl
appears on the regular GET /apps/{id} admin response, and that
it contains /x/<slug>/ (public) not /admin/ (the bug).
- TestPair_WaitRespects404WhenDisabled: approves a pairing, flips
the toggle off, asserts /wait returns 404 instead of minting.
24 QR-related tests pass. Full repo green. UI tsc clean.
After the previous audit added QRSignInURL to adminAppResponse, the
custom adminAppQRSignInResponse wrapper became:
- Redundant — QRSignInEnabled (via embedded core.App) and
QRSignInURL (now on adminAppResponse) both appeared at outer
and inner levels; Go's JSON encoder picked the outer.
- Inconsistent — the outer QRSignInURL was conditionally empty
when disabled, while the inner was always populated. So
GET /apps/{id} (toAdminAppResponse) showed the URL on disabled
apps, but PUT /qr-sign-in-config with enabled=false hid it.
Same endpoint family, different visibility behaviour for the
same data.
The URL is just a static pattern — disabled apps still have one,
it just 404s when hit. Always-show is the right semantics.
Fix: drop adminAppQRSignInResponse entirely, return plain
adminAppResponse from the admin handler. JSON-shape-compatible
with existing UI (qrSignInEnabled / qrSignInUrl on the same
field names).
New regression test TestAdminQRConfig_URLPresentEvenWhenDisabled
locks in that the URL appears on disabled responses too. 25 QR
tests pass. Full repo green.
The desktop polling JS had three branches: 200 (success), 425 (pending, keep polling), 410 (terminal). Everything else fell through — the next setInterval tick would re-fetch and re-fall-through forever, until the deadline timer eventually surfaced "Expired." Phase 2's /wait gate (return 404 when toggle disabled mid-flight) turned this latent bug into a real UX cliff: admin flips QR off, every active desktop spins until its 90s pairing TTL expires. Fix: any non-200, non-425, non-5xx response now stops polling and shows the cancellation message. Server failures (5xx) still keep polling — those are transient and the deadline bounds them. The change is JS-only inside the qrSignInTmpl template literal — no Go test can directly exercise it (would need a browser). The existing TestPair_QRSignInPageRenders verifies the page integrity. 25 QR tests still pass. Full repo green.
The Tier-1 OAuth authorize handler validated the popup openerOrigin only against the app's per-app CORS allowlist. That breaks social sign-in (Google/Apple/Microsoft/GitHub) on any page ManyRows serves AppKit itself: - QR sign-in's /pair landing (served from the auth host) - OIDC's /oidc/login shim (served from the auth host) On those pages window.location.origin is the auth host (e.g. https://auth.drumkingdom.com), which is never in the customer app's CORS list (that holds the app domain, e.g. drumkingdom.com), so the popup flow was rejected with error.invalidOrigin — surfaced to the user as "the opener origin is not allowed for this app." Fix: also allow originFromBaseURL(AppBaseURL(app)) — the install's own origin. Safe: an attacker can't receive a postMessage targeted at the auth host unless their window is actually served from it, and only ManyRows pages are. Closes the same latent bug in the OIDC hosted-login flow. New internal unit test covers the origin-extraction helper (the opener-origin check had zero coverage before).
The /pair landing mounts AppKit, which rendered its full login UI including the QR "Phone" button (the app has qr_sign_in_enabled). Circular: the user is already on their phone approving a phone sign-in, being offered to sign in with their phone. New AppKit init option suppressQRSignIn (plumbed main.tsx -> AppKit -> Auth's qrSignInEnabled), set by the /pair template. The QR button is hidden on that page; everything else (password, OAuth, passkey) renders as normal so the user can still authenticate to approve.
HandleAuthPairWait minted the desktop's TokenPair and returned it as JSON only. In cookie-transport mode the session lives in HttpOnly cookies, which JS cannot set — so delivering raw tokens in the URL fragment can't establish a session. The desktop redirected back to the app host and stayed logged out. Magic-link sets cookies AND the fragment; /wait now does the same. The desktop's /qr-sign-in page fetches /wait same-origin (the auth host), so the browser stores the Set-Cookie headers; with a parent-domain cookie_domain (.example.com) those cookies are then visible to the customer app host the page redirects to. Bearer/ local-mode apps ignore the cookies and read the fragment as before. Regression test asserts /wait emits HttpOnly access + refresh cookies carrying the same tokens as the JSON body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.