fix(wave): stop a skewed device clock from logging users out in the browser - #1969
Conversation
…rowser Wave login state is derived from the access-token cookie in two places: the server hook (server clock) and the universal wave layout load in the browser (document.cookie + Date.now()). When a device clock runs 15+ minutes fast, a token issued seconds ago already fails the client-side expiry check, so the client renders the user as logged out while the server still sees them as logged in. Guarded pages then bounce between themselves and /wave/login. The server layout now returns its own clock, and the browser measures the skew against it once per fresh server time, passing a corrected clock into getUserData. The server keeps calling getUserData with the default clock, so server behaviour is unchanged.
|
/ryph reviewer review this thoroughly. safe to merge? |
|
✅ Reviewer finished — ryph run Reviewer posted what it did in this thread. No code changes were needed.
Proposed by Reviewer (ryph run |
A +layout.ts module is evaluated on the server as well, so module-level mutable state there is shared across every request in the Node process. It was only safe because the call sat behind a browser check. Move the offset into a small utility that returns the plain wall clock on the server before touching any state, so the layout stays stateless.
|
Follow-up commit: the clock-skew offset no longer lives as module-level state in |
Exercises the real modules under a device clock 20 minutes fast: the offset is measured against server time, advances with the local clock between server loads, is re-measured when a new server time arrives, falls back to the device clock without one, and is never touched on the server. Also checks that a fresh token is rejected on the raw device clock but accepted on the corrected one, and that a genuinely expired token is still rejected.
|
Verification, since this module had no tests:
|
There was a problem hiding this comment.
Assessment: ✅ LGTM
Verified independently (not just trusting the PR description):
npx prettier --checkandnpx eslinton the four touched files: clean.npx svelte-check --threshold error: 0 errors attributable to these files (the 440 pre-existing errors are all in unrelated$lib/graphql/__generated__consumers, a known baseline in this checkout).
Logic review:
- The actual authorization boundary is untouched — the backend/cookie
expandhooks.server.ts's server-side check still use the real server clock. This change only affects what the browser uses to decide whether to render the user as logged in in the universal load, so there's no security regression from letting a client claim anynowMsit wants. correctedNowis careful about where its module-level mutable state can be touched: the!browsershort-circuit runs first, so the SSR codepath (where the module is a process-wide singleton across requests) never writeslastServerTime/clockOffsetMs. Good catch already made in the follow-up commit — moving this out of+layout.ts(which is genuinely shared server-side) and into a dedicated module is the right shape.- Memoization behavior matches the stated intent: the offset is recomputed only when a new
serverTimearrives (i.e. after the server load reruns), and holds steady across client-side navigations where the parent server load doesn't rerun — avoiding the offset drifting away from a legitimate elapsed-time delta into noise. getUserData's newnowMsparameter defaults toDate.now(), so the one other call site (hooks.server.ts, and the server branch in+layout.ts) is unaffected and needed no changes.serverTimemissing (undefined) falls back toDate.now(), preserving today's behavior as a safe default — can't in practice happen given+layout.server.tsalways sets it, but it's a sensible defensive default anyway.
No changes requested. Nit-level observations only: no unit test was added for auth.ts/server-clock.ts, but that's consistent with there being no existing test file for auth.ts at all — not a regression this PR introduces.
Safe to merge.
What
src/routes/(pages)/wave/+layout.server.tsnow returnsserverTime: Date.now()next towaveAccessToken.getUserDatainsrc/lib/utils/wave/auth.tstakes an optionalnowMs(defaultDate.now()) and runs the existingEXPIRY_BUFFER_SECONDScheck against it. The server hook calls it unchanged.src/routes/(pages)/wave/+layout.ts, when running in the browser, measures the offset betweendata.serverTimeand the device clock and passesDate.now() + offsetintogetUserData. The offset is memoized at module level and recomputed only when a newserverTimearrives, because the universal load re-runs on client-side navigations while the server-load data may not. IfserverTimeis missing it falls back toDate.now(), i.e. today's behaviour.Why
Wave login state is decided in two places. Server side,
hooks.server.tsreads thewave_access_tokencookie and checks itsexpwith the server clock. Client side, the wave layout's universal load reads the same cookie fromdocument.cookieand ran the same check withDate.now(), which is the device clock.Access tokens live 15 minutes. When a device clock is 15 or more minutes ahead of real time, a token issued seconds ago already looks expired to the browser, so the client renders the user as logged out while the server, using its own clock, renders them as logged in. Guarded pages redirect to
/wave/login, whose load sees the server-side user and redirects back, and the user is stuck in a login loop.The cookie itself survives the skew because Chrome corrects cookie expiry using the server
Dateheader; only this JavaScript expiry check breaks. Two users were stuck in this loop today.The server keeps its own clock throughout, so this only changes what the browser compares against.
Verification
npx prettier --checkon the three touched files: cleannpx eslinton the three touched files: cleannpx svelte-check --threshold error --output human: 0 errors (178 pre-existing warnings)src/lib/utils/wave/auth.ts, so none was added.