ReviewOS

stacks/stx

A page action cannot set a cookie, so it cannot sign anyone in — the one form #1847 most needs still can't work without JS

#1927
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

Page actions (#1847) landed and the shape is right — a page declares action, gets its own non-GET request, and returns either a redirect or a context merge that repopulates the form it drew. I went to adopt it across five forms in a real app and got stopped on the first one: an action cannot set a cookie, so it cannot sign anyone in.

The contract, from page-action.js

const result = await context.action({ request, form, params, query, cookies })
if (result && typeof result === 'object') {
  const to = result.redirect
  if (typeof to === 'string' && to) return { redirect: to, ran: true }
  Object.assign(context, result)
}
return { ran: true }

and the redirect it produces:

export function actionRedirectResponse(to) {
  return new Response(null, {
    status: 303,
    headers: { Location: to, 'Cache-Control': 'no-store' },
  })
}

PageActionResult is { redirect?: string, ran: boolean }. An action reads cookies on the way in and has no way to write one on the way out — no header hook, no Set-Cookie, and actionRedirectResponse builds its headers from a literal.

Why that blocks the canonical case

Sign-in is the form everyone reaches for first, and it is defined by establishing a session. The no-JS flow is: POST credentials → verify → set the session cookie → 303 to the destination. Steps 1, 2 and 4 are expressible; step 3 is not.

The same applies to sign-up, "remember me", accepting an invite that pins a tenant, and switching a locale or theme that must survive the redirect. Everything that is not a session cookie has a workaround; a session cookie does not, because the browser must receive it on the response that redirects.

In my app this is the difference between four of five forms adopting page actions and all five. Sign-in and sign-up have to keep posting to a separate JSON endpoint that can set the cookie, which leaves exactly the split #1847 set out to remove: one handler for the no-JS path, another for the enhanced one.

Suggested shape

Anything that lets an action contribute response headers would do. In rough order of how little it changes:

  1. A cookies field on the result, mirroring the read side:
    return { redirect: '/dashboard', cookies: { loghq_token: { value: token, maxAge: 2592000, sameSite: 'Lax', httpOnly: true, path: '/' } } }
    Symmetric with what the action already receives, and needs no Response in the action.
  2. headers on the result, merged into whatever response is produced — more general, less guided.
  3. Let an action return a Response and use it verbatim. Most flexible, but it moves redirect-status and cache-header decisions back to every call site, which actionRedirectResponse was written to centralise.

(1) fits the existing design best: PageActionContext already hands cookies in, so handing them back is the obvious other half.

Environment

  • stx 0.2.176 built from source, bun 1.3.14, macOS 15
  • 38-view Stacks app used as the e2e surface for stx; five forms, all currently preventDefault + fetch, which is what #1847 measured
14 days ago

Done in ff1b76f — option (1), the shape you proposed, for the reason you gave: the read side already hands cookies in, so handing them back is the other half of a symmetry that was simply missing.

return {
  redirect: '/dashboard',
  cookies: { session: { value: token, maxAge: 2592000, httpOnly: true, sameSite: 'Lax', path: '/' } },
}

A bare string works for the simple case (cookies: { locale: 'en-GB' }).

Verified end to end against a real dev server rather than only in unit tests — your canonical flow, no JS:

POST email=a@b.test          -> 303 See Other
                                set-cookie: session=tok-123; Max-Age=2592000; Path=/; HttpOnly; SameSite=Lax
                                location: /dashboard
                                cache-control: no-store

POST email=wrong             -> 200 OK
                                set-cookie: csrf=rotated
                                (form redrawn, "Invalid credentials" in the body)

Three decisions worth stating

Serialized inside runPageAction, not at each call site. A session cookie that is HttpOnly on the redirect path and not on the re-render path is a security bug rather than an inconsistency, so the callers receive finished Set-Cookie strings and only append them.

Applied on the re-render path too, not just the redirect. That half is easy to leave out and matters as much: a failed sign-in redraws the form, and that response is what carries a rotated CSRF token or a cleared session. Cookies on success only is the harder failure to notice.

cookies and redirect are stripped before the context merge. They are the action's protocol with the caller, not page data — and merging cookies would have overwritten the request cookies the template reads under that same name. Pinned by a test.

Two things found while wiring it

The dev server had already drifted from production. It hand-rolled its own 303 instead of calling actionRedirectResponse, and omitted the Cache-Control: no-store the production one sets — so a redirect answering a submission was cacheable in dev only. It now uses the shared builder, which is what the wrapper's own comment said it existed to guarantee.

There were three byte-identical Set-Cookie serializers (ssr.ts, edge-runtime.ts, and the client-side one in use-cookie.ts). Adding a fourth is the point at which copying stops being cheaper than sharing, so the server-side ones now go through one cookie-serialize.ts. The client-side one is deliberately left: it writes document.cookie, where HttpOnly is meaningless.

That extraction caught a live trap worth knowing about — Max-Age must be compared against undefined, not tested for truthiness, because Max-Age=0 is how a cookie is deleted. A truthiness check drops exactly that case, leaving the cookie in place while the caller believes it cleared it. There is a test for it.

On options (2) and (3)

I did not add headers or allow returning a Response. Your reasoning for (1) over (3) was right — it keeps redirect-status and cache-header decisions centralised — and a general headers escape hatch is easy to add later if a real need turns up that cookies do not cover. Worth a separate issue if you hit one.

Verification: 11 tests, sabotage-checked twice (dropping the cookies from the result fails 5; dropping the append from the redirect fails 2). Full suite 11913 pass; the 2 failures are the craft binary resolution pair, which fail only on machines with craft installed.

Sign in to comment on this issue.