Commit cf5b8f3
authored
fix(security): P2 dashboard batch + P1-8 dead-code retraction (#50)
* fix(security): use constant-time compare for CSRF token verification
ValidateCSRFToken compared the request-supplied token against the
session-bound token with `!=`, which is byte-by-byte and exits on the
first mismatch. The values are both 256-bit cryptographically random
strings, so the practical exposure on a non-pathological network was
low — but every other secret comparison in the codebase (the agent's
API key, webhook HMAC) uses crypto/subtle.ConstantTimeCompare. CSRF
should match.
P2-1 from the 2026-05 security audit.
* fix(security): add hard TTL on top of sliding session timeout
The session expiration check used a single sliding-window timeout that
ExtendSession refreshed on every request. A heavily-used account
(notably admins with the dashboard open in a long-lived browser tab)
could effectively never re-authenticate — a stolen session cookie was
useful indefinitely.
Adds a configurable absolute hard TTL alongside the existing sliding
window. The new sessionStartKey ("session_start") is set ONCE at login
and never touched by ExtendSession. GetUser now checks both:
1. Sliding idle timeout (existing behavior, sessionLoginKey).
2. Absolute hard TTL (new, sessionStartKey + absoluteTimeout).
Configuration:
- SESSION_ABSOLUTE_HOURS env var (defaults to 24h).
- 0 disables the hard TTL — keeps the legacy sliding-only behavior
for deployments that want it.
- absoluteTimeout < timeout is rejected at NewManager time (would mean
the hard TTL fires before the sliding window ever could; almost
certainly a config error).
The cookie's MaxAge now uses the larger of (sliding, absolute) so the
browser drops the cookie no later than the server-side max — even if a
later bug skipped server-side expiry.
Older sessions that predate this commit don't carry sessionStartKey.
GetUser treats those as if they just started (falls back to
sessionLoginKey) — they hit the hard TTL one window later than fresh
sessions, but the alternative is logging existing users out at deploy
time.
P2-3 from the 2026-05 security audit.
* fix(security): emit Strict-Transport-Security + Permissions-Policy headers
Two security-relevant headers were missing from the response from the
SecurityHeaders middleware:
- Strict-Transport-Security: tells the browser to never speak plain
HTTP to this origin again for the next year (and to apply that to
subdomains, and to be eligible for the HSTS preload list). Without
it, a MITM-positioned attacker can force a downgrade on the very
first visit after a TLS-cert-rotation reload, or after the user
clicks an http:// link to the dashboard. The header is only honored
over HTTPS, so emitting unconditionally is safe for dev :3000.
- Permissions-Policy: explicitly disables camera, microphone, geolocation,
USB, payment, and a few Chrome-specific tracking surfaces (interest-
cohort, browsing-topics). None of these are used anywhere in the
dashboard — so a future XSS or iframe-embedded asset can't silently
ask the browser to enable them without a corresponding source change
to this middleware.
Both headers are best-practice defenses that pair with the existing
CSP / X-Frame-Options / Referrer-Policy stack; cost is zero bytes of
extra logic and a couple hundred bytes per response.
Tests verify presence + the key tokens (max-age, includeSubDomains,
preload for HSTS; geolocation, microphone, camera, usb, payment for
Permissions-Policy).
P2-6 from the 2026-05 security audit.
* chore(security): remove dead Alpine.js dialog components (P1-8 retraction)
components/dialog.templ declared three Alpine.js-based templates
(Dialog, ConfirmDialog, AlertDialog) that used x-data / x-show /
@click.away / @keydown.escape.window directives. The 2026-05 audit
flagged this as broken on the assumption that Alpine wasn't loaded
anywhere in the base layout — and the audit was correct about Alpine
not being loaded, but WRONG about the implication.
Closer reading: zero callers anywhere in the repo reference
components.Dialog / components.ConfirmDialog / components.AlertDialog.
The real modal infrastructure is two separate vanilla-JS systems:
- framework/templates/layouts/base.templ:917+ — ConfirmDialog(),
PromptDialog(), AlertDialog() templates wired to vanilla
showConfirmDialog() / showPromptDialog() / showAlertDialog()
functions. This is what every gear page actually calls.
- framework/ui/modal.templ — Modal(id, title, size) + ConfirmModal
for custom-content modals (notes-modal, delete-modal, etc.),
also vanilla JS via closeModal(id).
So components/dialog.templ is dead code: never invoked, but a latent
trap because someone reading it might assume Alpine is in play, add it
to base.templ, and break the CSP without realizing the file they were
"making work" was never used.
Drop the .templ source. The local _templ.go build artifact is
already gitignored.
Retracts the P1-8 misframing in docs/security-review/2026-05-findings.md
(the doc still lives on PR #40; will be updated to reflect this
deletion in a follow-up commit there).
* fix(security): close hard-TTL gaps in legacy sessions + cookie MaxAge
Copilot review on PR #50 caught two real holes in the initial P2-3
implementation (a25795c):
1. **Legacy sessions never anchored.** GetUser's fallback for sessions
that pre-date sessionStartKey compared against sessionLoginKey —
which ExtendSession refreshes on every request. So a legacy session
that stayed warm via keepalive would slide indefinitely and never
hit the absolute hard TTL. The whole point of the hard TTL.
2. **Cookie MaxAge re-sliding.** NewManager set the store-wide cookie
MaxAge to the absolute timeout, but every Save (including
ExtendSession's keepalive) re-emitted Set-Cookie with that same
absolute value measured from "now" — so the browser's cookie expiry
slid forward forever, not anchored to the session's original login.
Fixes both in ExtendSession:
- (1) When sessionStartKey is absent, capture the CURRENT
sessionLoginKey as the anchor BEFORE overwriting it. Idempotent for
already-anchored sessions. Legacy sessions get one full
absoluteTimeout window from the next post-deploy ExtendSession call,
then re-authenticate like everyone else.
- (2) Per-save, override session.Options.MaxAge to
min(sliding-timeout, remaining-absolute-TTL). Uses a copied Options
struct so the override doesn't bleed into other sessions sharing the
store. NewManager's store-wide MaxAge simplifies to the sliding
timeout (which is what login gets when the absolute window is full).
ExtendSession now also refuses to extend a session that's already past
the absolute deadline, returning an explicit error — caller path then
fails on the next GetUser, forcing re-auth.
Two new tests:
- TestManager_ExtendSession_AnchorsLegacySession: strip sessionStartKey
from a real cookie, call ExtendSession, assert sessionStartKey is set
in the result.
- TestManager_ExtendSession_CookieMaxAgeShrinks: rewind sessionStartKey
to 30m ago with 1h sliding + 1h absolute, call ExtendSession, assert
the Set-Cookie MaxAge is ~1800 (remaining absolute) not 3600 (sliding).
Refs: #501 parent c174f39 commit cf5b8f3
8 files changed
Lines changed: 428 additions & 197 deletions
File tree
- gearbox
- cmd/server
- internal/framework
- auth
- config
- middleware
- models
- templates/components
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
150 | 150 | | |
151 | 151 | | |
152 | 152 | | |
153 | | - | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
154 | 156 | | |
| 157 | + | |
155 | 158 | | |
156 | 159 | | |
157 | | - | |
| 160 | + | |
158 | 161 | | |
159 | 162 | | |
160 | 163 | | |
161 | | - | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
162 | 168 | | |
163 | 169 | | |
164 | 170 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
21 | 26 | | |
22 | 27 | | |
23 | 28 | | |
| |||
27 | 32 | | |
28 | 33 | | |
29 | 34 | | |
30 | | - | |
31 | | - | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
32 | 42 | | |
33 | 43 | | |
34 | 44 | | |
35 | | - | |
| 45 | + | |
| 46 | + | |
36 | 47 | | |
37 | 48 | | |
38 | 49 | | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
39 | 53 | | |
40 | | - | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
41 | 59 | | |
42 | 60 | | |
43 | 61 | | |
| |||
48 | 66 | | |
49 | 67 | | |
50 | 68 | | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
55 | 74 | | |
56 | 75 | | |
57 | 76 | | |
| |||
131 | 150 | | |
132 | 151 | | |
133 | 152 | | |
134 | | - | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
135 | 159 | | |
136 | 160 | | |
137 | | - | |
| 161 | + | |
| 162 | + | |
138 | 163 | | |
139 | 164 | | |
140 | 165 | | |
| |||
200 | 225 | | |
201 | 226 | | |
202 | 227 | | |
203 | | - | |
| 228 | + | |
204 | 229 | | |
205 | 230 | | |
206 | 231 | | |
207 | 232 | | |
208 | | - | |
209 | 233 | | |
210 | | - | |
| 234 | + | |
211 | 235 | | |
212 | 236 | | |
213 | 237 | | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
214 | 257 | | |
215 | 258 | | |
216 | 259 | | |
| |||
278 | 321 | | |
279 | 322 | | |
280 | 323 | | |
281 | | - | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
282 | 331 | | |
283 | 332 | | |
284 | 333 | | |
| |||
292 | 341 | | |
293 | 342 | | |
294 | 343 | | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
295 | 365 | | |
296 | 366 | | |
297 | 367 | | |
| |||
304 | 374 | | |
305 | 375 | | |
306 | 376 | | |
307 | | - | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
308 | 410 | | |
309 | 411 | | |
310 | 412 | | |
| |||
561 | 663 | | |
562 | 664 | | |
563 | 665 | | |
564 | | - | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
565 | 672 | | |
566 | 673 | | |
567 | | - | |
| 674 | + | |
| 675 | + | |
568 | 676 | | |
569 | 677 | | |
570 | 678 | | |
| |||
0 commit comments