Security hardening: rate limit login, remove SESSION_SECRET, harden cookie secure flag#7
Conversation
…ookie secure flag - Add in-memory per-IP rate limiter to login action (5 failures in 15 min → 429) with global counter defending against distributed brute force (50 failures across all IPs in 15 min → 429); successful login clears the per-IP counter; stale entries swept every 5 minutes - Flip cookie secure flag default to true; only NODE_ENV=development disables it (fail-safe instead of fail-open) - Remove SESSION_SECRET from .env.example and README — variable was never used; sessions rely on a random 32-byte token stored server-side in SQLite - Add comment to createSession in auth.ts explaining the session token strategy https://claude.ai/code/session_01BuWpZ7GP3i3i5LHb5N4GBV
There was a problem hiding this comment.
Pull request overview
This PR hardens authentication/session handling by adding rate limiting to the login action, tightening cookie transport security defaults, and removing unused SESSION_SECRET documentation/config hints.
Changes:
- Add an in-memory failed-login rate limiter (per-IP + global counters with periodic sweeping) to the login action.
- Default the session cookie to
secure: trueoutside development. - Remove
SESSION_SECRETfrom.env.exampleand README; add documentation inauth.tsabout opaque server-stored session tokens.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/routes/+page.server.ts |
Adds login rate limiting and hardens session cookie secure behavior |
src/lib/server/auth.ts |
Documents the server-side session token strategy |
README.md |
Removes SESSION_SECRET setup instructions |
.env.example |
Removes unused SESSION_SECRET variable and guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Per-IP failed attempt tracking | ||
| const failedAttempts = new Map<string, { count: number; firstAttempt: number }>(); | ||
|
|
||
| // Global counter for distributed brute force detection | ||
| let globalFailed = { count: 0, firstAttempt: 0 }; |
There was a problem hiding this comment.
The rate-limiter state is kept in-process (failedAttempts/globalFailed). In adapter-node deployments with multiple app instances/processes (or after restarts), limits won’t be enforced consistently and can be bypassed by spreading attempts across instances. Consider moving counters to a shared store (SQLite/Redis) or explicitly documenting that the limiter is best-effort per-process only.
| // Sweep stale entries every 5 minutes | ||
| setInterval(() => { | ||
| const now = Date.now(); | ||
| for (const [ip, entry] of failedAttempts) { | ||
| if (now - entry.firstAttempt > WINDOW_MS) failedAttempts.delete(ip); |
There was a problem hiding this comment.
This module-level setInterval will run for the lifetime of the server process and can be duplicated on dev/HMR reloads, creating multiple sweep timers. Consider removing the interval and doing request-driven cleanup only, or guarding it (e.g., via globalThis) and calling timer.unref() so it doesn’t keep processes alive unexpectedly.
| const ip = getClientAddress(); | ||
| const now = Date.now(); |
There was a problem hiding this comment.
getClientAddress() commonly resolves to the reverse proxy’s IP when the app is behind a proxy (the repo includes a Caddy reverse_proxy example in deploy/Caddyfile.example). If that’s the case, all users would share the same rate-limit bucket. Consider deriving the client IP from trusted forwarded headers (and rejecting spoofed values) or documenting the required proxy/header setup for accurate IP rate limiting.
| if (globalFailed.count > 0 && now - globalFailed.firstAttempt > WINDOW_MS) { | ||
| globalFailed = { count: 0, firstAttempt: 0 }; | ||
| } | ||
| if (globalFailed.count >= GLOBAL_MAX) { | ||
| return fail(429, { error: 'Too many attempts. Try again in a few minutes.' }); |
There was a problem hiding this comment.
The global failure limit (GLOBAL_MAX) creates a trivial denial-of-service: an attacker can intentionally submit 50 bad PINs to block all logins for 15 minutes. Consider making the global counter non-blocking (e.g., only logging/alerting), scoping it (per-user/per-username/per-IP-range), or using an adaptive backoff instead of a hard global lockout.
- Replace hardcoded UTC-7 offset with Intl.DateTimeFormat('America/Los_Angeles')
so DST transitions are handled correctly; also read the full HH:MM from the
lunch_cutoff setting (previously only the hour was compared)
- Validate cc_fee_rate, kitchen_pct, and bar_liquor_pct in saveSettings: each
must be a number in [0, 100], and kitchen_pct + bar_liquor_pct must not exceed
100% combined; invalid input returns a 400 with a descriptive error message
https://claude.ai/code/session_01BuWpZ7GP3i3i5LHb5N4GBV
with global counter defending against distributed brute force (50 failures across
all IPs in 15 min → 429); successful login clears the per-IP counter; stale
entries swept every 5 minutes
(fail-safe instead of fail-open)
sessions rely on a random 32-byte token stored server-side in SQLite
https://claude.ai/code/session_01BuWpZ7GP3i3i5LHb5N4GBV