Session codec max-age still 30 days: bring securecookie codecs in line with the 7-day cap #108

New Issue

clawbot · 2026-08-09T08:19:57+02:00

clawbot commented

2026-08-09 08:19:57 +02:00

Follow-up from the independent review of PR #105 (#66). Not blocking there, because #105 adds a stricter server-side check that shadows this — but the underlying skew remains and should be closed at the source.

The skew

internal/session/session.go:148 assigns store.Options wholesale. In gorilla/sessions@v1.4.0, NewCookieStore sets the codec max-age to 30 days via cs.MaxAge(...), and assigning store.Options directly never touches Codecs. So the securecookie layer will still decode a cookie up to 30 days old; only the new expired check in #105 rejects it.

Verified by execution during the #105 review with a throwaway probe: the app's pattern still decodes a cookie past its Options.MaxAge (returns true), while store.MaxAge(1) correctly rejects it (false).

Why it matters even though #105 shadows it

Before #105 this was a genuine hole — the 7-day absolute cap was enforced only by the browser, and a retained cookie would have been honoured by the server for up to 30 days. #105 closes the exploitable path, but leaves two layers disagreeing about the same policy. That is exactly the shape that breaks later: anyone refactoring the expired check, or adding a second entry point that decodes a session without going through IsAuthenticated, silently reopens a 30-day window. Defence in depth belongs at the codec too.

Fix: replace the wholesale store.Options assignment with store.MaxAge(secondsPerDay * sessionMaxAgeDays), which sets Options.MaxAge and propagates to every codec. Check whether the same pattern appears anywhere else a store is constructed.

Done when: a test proves codec-level rejection of a cookie older than the 7-day cap — i.e. it fails if the expired check from #105 is removed, so it is genuinely testing the codec and not the new server check.

Also from the same review

The 10% lazy-refresh bound is unpinned. Changing idleRefreshDivisor from 10 to 5 leaves the full suite green (verified). The PR documents "expires up to 10% early, never late" as a deliberate tradeoff, so the constant deserves a test that fails if it drifts — otherwise the documented guarantee is prose only.
internal/middleware/middleware.go:217 reuses the outer err rather than scoping it inside the if. Harmless, inconsistent with the surrounding style.
Test helpers return the *fakeClock they were handed, and fakeClock is duplicated across two test packages. Fold into a shared helper if a third use appears.
README gaps: it omits that deploying this forces a one-time re-login for existing sessions (they have no created_at/last_seen), and it says the timeout is disabled by setting 0 where the code disables on any non-positive value.

Definition of done

Codec max-age matches the absolute cap, with a test that isolates the codec layer.
The refresh divisor is pinned by a test.
Items 2-4 fixed or explicitly declined.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Follow-up from the independent review of PR #105 (#66). Not blocking there, because #105 adds a stricter server-side check that shadows this — but the underlying skew remains and should be closed at the source. ## The skew `internal/session/session.go:148` assigns `store.Options` wholesale. In `gorilla/sessions@v1.4.0`, `NewCookieStore` sets the **codec** max-age to 30 days via `cs.MaxAge(...)`, and assigning `store.Options` directly never touches `Codecs`. So the securecookie layer will still decode a cookie up to 30 days old; only the new `expired` check in #105 rejects it. Verified by execution during the #105 review with a throwaway probe: the app's pattern still decodes a cookie past its `Options.MaxAge` (returns `true`), while `store.MaxAge(1)` correctly rejects it (`false`). ## Why it matters even though #105 shadows it Before #105 this was a genuine hole — the 7-day absolute cap was enforced **only by the browser**, and a retained cookie would have been honoured by the server for up to 30 days. #105 closes the exploitable path, but leaves two layers disagreeing about the same policy. That is exactly the shape that breaks later: anyone refactoring the `expired` check, or adding a second entry point that decodes a session without going through `IsAuthenticated`, silently reopens a 30-day window. Defence in depth belongs at the codec too. **Fix:** replace the wholesale `store.Options` assignment with `store.MaxAge(secondsPerDay * sessionMaxAgeDays)`, which sets `Options.MaxAge` **and** propagates to every codec. Check whether the same pattern appears anywhere else a store is constructed. **Done when:** a test proves codec-level rejection of a cookie older than the 7-day cap — i.e. it fails if the `expired` check from #105 is removed, so it is genuinely testing the codec and not the new server check. ## Also from the same review 1. **The 10% lazy-refresh bound is unpinned.** Changing `idleRefreshDivisor` from 10 to 5 leaves the full suite green (verified). The PR documents "expires up to 10% early, never late" as a deliberate tradeoff, so the constant deserves a test that fails if it drifts — otherwise the documented guarantee is prose only. 2. **`internal/middleware/middleware.go:217`** reuses the outer `err` rather than scoping it inside the `if`. Harmless, inconsistent with the surrounding style. 3. **Test helpers return the `*fakeClock` they were handed**, and `fakeClock` is duplicated across two test packages. Fold into a shared helper if a third use appears. 4. **README gaps:** it omits that deploying this forces a one-time re-login for existing sessions (they have no `created_at`/`last_seen`), and it says the timeout is disabled by setting `0` where the code disables on any non-positive value. ## Definition of done - Codec max-age matches the absolute cap, with a test that isolates the codec layer. - The refresh divisor is pinned by a test. - Items 2-4 fixed or explicitly declined. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.

clawbot referenced this issue

2026-08-09 08:20:15 +02:00

Add inactivity-based session timeout (closes #66) #105

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#108