ReviewOS

also looking at this

stacks/bench-review

Close unauthenticated /api/requests CRUD, and make the session cookie HttpOnly

#77
Merged glennmichael123 wants to merge fix/request-autocrud-and-httponly into main
11 files +552 -57
9 days ago

Two security fixes from the recent audit. Independent of #76 — no overlapping files, so they can merge in either order.

1. Unauthenticated full CRUD on /api/requests

app/Models/Request.ts carried useApi: true, and useApi.routes is opt-OUT: omitting it makes the ORM generator emit index, show, store, update, destroy and a bulk-delete, with no 401 path on any of the writes. storage/framework/orm/routes.ts resolves the authed user only to feed the optional authedFill hook, and its sole 401 branch sits inside if (own.enforced), which needs an ownership config this model never declared.

The requests table is request telemetry — ip_address, user_agent, path, status_code:

  • Readable, it is an unmetered log of who visited what. On an app whose premise is anonymous reviews that is a de-anonymisation primitive: correlate an IP against a POST /api/reviews hit and the anonymous author is named.
  • Writable, anyone can forge or wipe telemetry that the framework dashboard renders (defaults/routes/dashboard.ts → Dashboard/Infrastructure/RequestIndexAction).

No live data leak — the table has no rows and nothing in this app reads or writes it. The endpoints were real regardless.

Fix is routes: [].

Two things worth recording:

  • app/Models/commerce/* (~29 models, same useApi with no routes) are inert: the loader at storage/framework/orm/routes.ts:19-33 does readdirSync over app/Models then if (st.isDirectory()) continue — it never recurses. Don't rely on it; a loader change would expose all of them at once.

  • This arrived with c5131e97 ("scaffold sync; commerce + email models"), i.e. after the earlier lockdown pass. So re-audit after every scaffold sync:

    for f in $(find app/Models -maxdepth 1 -name '*.ts'); do
      grep -q useApi "$f" && ! grep -qE '^\s*routes:\s*\[' "$f" && echo "OPEN: $f"
    done

2. The session token was readable from JavaScript

resources/stores/auth.ts took the token out of the login JSON and did document.cookie = 'auth-token=…'. A cookie written that way can never be HttpOnly — the entire point of document.cookie is that scripts write it, and whatever a script can write a script can read. The token sat there for 30 days, readable by any injected script. This app renders user-submitted rich HTML, so that turns a single XSS into session theft — and the reviewer anonymity the product promises is downstream of those sessions.

This did not need the auth redesign the notes assumed

The framework already does cookie auth end-to-end; the app simply wasn't using it. Nothing server-side had to be taught anything new:

  • authCookie() (@stacksjs/auth) serialises the cookie and always includes HttpOnly, resolving the name through authCookieName(), the lifetime from config.auth.tokenExpiry, and Secure from shouldSecureAuthCookie() — which deliberately omits Secure on loopback so local http sessions keep working.
  • requestToken() — which Auth.user(), Auth.logout() and the auth middleware all funnel through — checks bearerToken(), then the Authorization header, then falls back to the cookie. A cookie-only request already authenticated.

config/auth.ts sets defaultTokenName: 'auth-token' and declares no cookie block, so the resolved name is auth-token — already what the store and resources/middleware/auth.ts key off. The three stay in lock-step for free.

So the change is just moving the write to the server

  • app/Support/sessionCookie.ts — withSessionCookie / withClearedSessionCookie, both Cache-Control: no-store, since a cached auth response would hand one visitor another's session.
  • app/Actions/Auth/{Login,Register,Logout}Action.ts — app-local overrides, behaviourally identical to the framework defaults except for the Set-Cookie. App actions shadow defaults, so this needs no edit to the vendored storage/framework/ tree that a sync would revert. JSON bodies stay byte-compatible with the framework shape for non-browser clients.
  • resources/stores/auth.ts — no longer writes or reads the credential. A module-level sessionToken mirrors it in memory for the Authorization header within the tab that logged in; it is never persisted and dies on reload, after which the cookie carries the session alone. clearAuthCookies() now only clears the non-credential user cookie, because a document.cookie expiry for an HttpOnly cookie is ignored by the browser — the server clears that one.

Two consequences worth stating

  • isAuthenticated() is now keyed off user, not token. The token is in-memory only, so it is empty after a reload while the session is still perfectly valid. Two callers used token() as an "am I signed in" flag and would have regressed on every hard refresh — the notifications store (the header bell silently stops fetching) and AdminShell (a valid admin session bounced to /admin/login). Both now read isAuthenticated().
  • The host's same-origin /api/* proxy is now load-bearing for auth, not just convenience: the cookie is SameSite=Lax and is only sent same-site. That proxy is already in the deploy contract (DEPLOY.md, landing in #76).

CSRF posture is unchanged, not worsened. requestToken() already accepted the cookie before this, so cookie-borne requests were always possible, and SameSite=Lax is what withholds it on cross-site POST/PUT/DELETE. That is what keeps the blanket .skipCsrf() in routes/api.ts defensible, and it holds as long as no GET mutates state — currently true, and worth keeping true.

Verification

lintPASS
typecheck:appPASS (0 errors)
tests/unit/session-cookie.test.ts8 tests, 23 assertions, PASS
gate16/17 — unchanged; the one failure is the pre-existing empty-database check

The strongest check: the built bundle now contains zero auth-token= cookie writes, where it previously wrote the credential on every login.

The new tests cover the cookie contract (HttpOnly, resolved name, server-side round-trip through authCookieToken(), expiry on clear, loopback Secure handling), both response helpers, and the routes: [] opt-out from part 1.

I confirmed app actions genuinely shadow framework defaults rather than trusting the comment that says so: this repo's app/Actions/Auth/VerifyEmailAction.ts is already a distinct 33-line override of the 25-line default, routed through the same 'Actions/Auth/…' string form, and that is the one the flow runs.

Not verified

A real login → reload → still-signed-in round trip. That needs the dev server and a seeded user (the database is empty), so it is worth exercising before deploy. The failure mode if the cookie somehow isn't sent is graceful — the user appears logged out rather than locked out, and can sign in again.

Also note tests/feature/ui.test.ts fails on main already (document is not defined — a placeholder stub with no DOM registered), so the test job stays red independently of this PR.

🤖 Generated with Claude Code

11 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.