ReviewOS

stacks/bun-router

session security: file-store SID collision, no auto-regenerate on auth change, CSRF outside session

#879
Open glennmichael123 opened this 8 days ago ·
8 days ago

TL;DR

Three session-related security findings from a downstream audit (stacksjs/stacks#1878). Filing as a tracker so the upstream fix lands cleanly and Stacks can drop its pantry-side workarounds.

Se-1: File-store sanitization silently collides session IDs (HIGH)

src/session/file-store.ts:35. The SID sanitizer is sid.replace(/[^a-zA-Z0-9_-]/g, '') — invalid characters get stripped instead of the request being rejected. Both 'a/b' and 'ab' resolve to the same file. A hostile or buggy client can collide with another user's session by submitting a malformed SID.

Suggested fix: strict validation against ^[A-Za-z0-9_-]{1,128}$. Throw or return null on mismatch so the session is treated as missing (caller regenerates a fresh SID).

private filePath(sid: string): string {
  if (!/^[a-zA-Z0-9_-]{1,128}$/.test(sid)) {
    throw new Error('[session/file-store] invalid session id')
  }
  return join(this.dir, `${sid}.json`)
}

Se-2: No loginAs(userId) / forgetUser() helpers — session fixation surface (HIGH)

src/session/index.ts:184. regenerate() exists but the framework provides no convention-over-config helper for the "session must be regenerated on auth state change" pattern. Apps that forget to call regenerate() in their login handler are vulnerable to session-fixation — an attacker plants a known SID on the victim's browser, victim logs in, attacker hijacks.

Suggested fix: add two methods to SessionManager:

/** Mark the session as belonging to a newly-authed user.
 *  Regenerates the SID so a pre-auth fixed cookie can't ride along. */
async loginAs(userId: number | string): Promise<string> {
  const newId = await this.regenerate()
  this.data.userId = userId
  await this.store.set(newId, this.data)
  return newId
}

/** Mark the session as logged out. Regenerates the SID and drops
 *  the userId; flash messages / intended URL persist. */
async forgetUser(): Promise<string> {
  delete this.data.userId
  return this.regenerate()
}

regenerate() should also keep data.id in sync with the new SID so consumers reading session.data.id after a regenerate see the right value.

Se-3: CSRF tokens live in a process-wide static map (HIGH)

src/middleware/csrf.ts:6. Tokens are stored in Csrf.tokens = new Map<string, string>(). Problems:

  1. Static map is lost on server restart — every user gets a 419 on the first form submit after a deploy.
  2. No per-session isolation — two users can collide on the same token (low probability but undetected).
  3. Csrf.tokens.set(token, token) is wasteful — the map's value duplicates the key.

Suggested fix: persist the token inside the session payload under a reserved key (_csrf). Fall back to the static map only when no session middleware is installed.

const CSRF_SESSION_KEY = '_csrf'

private storeToken(req: EnhancedRequest, token: string): void {
  const session = (req as any).session
  if (session?.data) {
    session.data[CSRF_SESSION_KEY] = token
    return
  }
  Csrf.tokens.set(token, token) // fallback only
}

private tokenIsKnown(req: EnhancedRequest, token: string): boolean {
  const session = (req as any).session
  if (session?.data) return session.data[CSRF_SESSION_KEY] === token
  return Csrf.tokens.has(token)
}

Why upstream

Stacks pinned a local-pantry copy of bun-router and applied these patches there as a workaround for the audit. The pantry is gitignored so the fixes don't survive a clean checkout; landing them in stacksjs/bun-router lets every consumer benefit and unblocks Stacks dropping the pantry patches.

No urgency — Stacks runs with the patched pantry locally. Stable upstream API for the new loginAs / forgetUser helpers is the long-term goal.

These additional session findings are deferred (encryption-at-rest, concurrent-write protection, cookie SameSite defaults). Worth tracking but not blocking.

Sign in to comment on this issue.