ReviewOS

stacks/analyticshq

[auth] Set the auth cookie server-side at login, so the first dashboard render is owner-scoped

#33
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Split out of #32 — same code, do them together.

The gap

The dashboard's pre-paint bootstrap has a "cookie absent → set it → reload" branch. It never fires, because the session store's mirror effect (resources/stores/session.ts) writes the cookie first. Measured in a headless browser: a cold start with a localStorage token and no cookie produces exactly one Document request, and the cookie is set — so the reload branch is dead code.

That is fine in the normal flow, because sign-in happens on /login and the cookie exists before you ever navigate to /dashboard.

It is not fine when the cookie expires or is cleared while the localStorage token survives. Then:

  • the Document request for /dashboard carries no cookie
  • the server cannot owner-scope, so no site switcher and no setup panel
  • the store writes the cookie a moment later
  • nothing reloads, because the bootstrap now sees a cookie present

The user gets a logged-in-looking dashboard missing its owner UI, and only a manual reload fixes it.

Fix

Have the login action set the auth cookie server-side in its response, so the very first authenticated Document request carries it. Then the pre-paint bootstrap can be deleted rather than repaired.

Decision this forces

@stacksjs/auth/dist/cookie-auth.js hardcodes HttpOnly. An HttpOnly cookie cannot be written or read by document.cookie, which makes the session store's mirror effect a permanently-failing write.

  • Non-HttpOnly — smaller change, keeps the mirror, no XSS win.
  • HttpOnly — correct, and deletes code: the mirror effect, both pre-paint blocks, and the useCookie call in the dashboard bootstrap all go away. Needs cookie-clearing added to LogoutAction.

Recommend HttpOnly.

9 days ago

Root cause confirmed upstream and filed as stacksjs/stacks#2306 — this cannot be fixed app-side without forking a framework action.

What I verified, rather than inferred:

  • POST /login against a real account returns HTTP 200 with **zero Set-Cookie headers; the body carries access_token, refresh_token, token, token_type, expires_in, user. Measured against the shipped @stacksjs/defaults`.
  • LoginAction is framework code — there is no app/Actions/Auth/LoginAction.ts here.
  • The capability exists and is already used: SocialCallbackAction.ts:64 does redirect.headers.append('Set-Cookie', authCookie(session.token)). So social sign-in leaves a cookie and password sign-in does not, from the same framework directory.
  • authCookie() hardcodes HttpOnly, exactly as this issue anticipated.

One correction to the plan here. This issue assumes adopting the framework cookie is a drop-in. It is not, quite: we read our own analyticshq_token (written by the session store's mirror, read in dashboard.stx:238), while the framework's helpers resolve to auth-token. That is reconcilable by config rather than code — authCookieName() checks config.auth.cookie.name first, and cookie: AuthCookieConfig is a supported key on AuthOptions — so when #2306 lands we set the name in config/auth.ts instead of migrating the reader.

Keeping this open and blocked on #2306. The recommendation in the issue body still stands: take HttpOnly, then delete the mirror effect, both pre-paint blocks, and the useCookie call, and add clearAuthCookie() to logout.

9 days ago

Fixed in ad9dc4c, taking the HttpOnly option this issue recommended.

Unblocked by the upstream fix. stacksjs/stacks#2306 landed in 0.70.369 — LoginAction sets the cookie in its own response, RegisterAction and RefreshTokenAction do the same, and LogoutAction clears it. Picked up by removing bun.lock and node_modules and reinstalling; the caret ranges resolved 0.70.362 → 0.70.369 with no package.json edit.

config.auth.cookie.name is now a real key on AuthOptions — the key this issue noted didn't exist — so naming the cookie analyticshq_token there makes the framework's writer and this app's reader agree. Verified rather than assumed:

authCookieName() -> analyticshq_token
authCookie(tok)  -> analyticshq_token=…; Path=/; Max-Age=3600; HttpOnly; SameSite=Lax

HttpOnly is what made this delete code. It isn't just that the mirror became redundant — document.cookie cannot write an HttpOnly cookie, so every write that effect made was being refused by the browser. Removed:

  • the useCookie mirror and its effect in resources/stores/session.ts
  • sessionCookieSignal and its two type aliases
  • the cookie half of the dashboard's pre-paint block

What's left of the pre-paint block is only the signed-out redirect, which the server can't do for us.

Sign-out had to change, or this would have been a regression. An HttpOnly cookie can only be cleared server-side. Left alone, signOut would clear localStorage and navigate while the cookie survived — and the dashboard's <script server> block authenticates from that cookie, so the page would say signed out while the server kept rendering their sites. Worse than the bug being fixed. signOut now POSTs /logout and awaits it before navigating, so the Set-Cookie isn't raced. A failure still signs out locally rather than trapping anyone; the cookie then survives only until it expires, which tokenExpiry caps at an hour.

Measured the same way this issue was opened. It was filed on Set-Cookie count: 0. Running the real LoginAction against the real database:

status: 200
Set-Cookie count: 1
  analyticshq_token=…; Path=/; Max-Age=3600; HttpOnly; SameSite=Lax

10 checks: the cookie carries the same token as the response body, is HttpOnly and SameSite=Lax, and clearAuthCookie matches it on every attribute except the value — which it must, or the browser keeps the original beside the expired one and sign-out does nothing.

Also corrected five comments across login, register, dashboard and sites/new that described the mirror as current. One claimed the register response sets the cookie; I checked RegisterAction rather than leave it asserted, and it does.

338 tests pass, tsc clean, lint clean on every changed file.

One thing to note separately: pickier . now reports 72 warnings, all Tailwind class ordering inside the re-vendored storage/framework/defaults tree. That directory is gitignored and isn't ours — same leak the bun test gate had this morning. Worth excluding from the repo-level lint on its own rather than inside an auth change.

Sign in to comment on this issue.