sneak/webhooker
1
0
Fork 0
Code Issues 67 Pull Requests Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 08:19:57 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 08:19:57 +02:00
Collaborator
Copy Link
Copy Source

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.
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 commented 2026-08-12 11:51:18 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in #132 (base next).

The store is now built by newStore, which applies the cap with store.MaxAge(...) so Options.MaxAge and every codec agree; it is the only place a store is constructed. Two codec tests decode a re-stamped cookie an hour inside and an hour outside the cap through Session.Get alone, so no server-side check participates: reverting the fix fails the rejection test both with expired present and with expired stubbed out, and stubbing expired alone leaves both tests passing. The refresh divisor is pinned two-sidedly and fails at 5 and at 20. Items 2 and 4 fixed; item 3 declined for want of a third use. Verified with make check (exit 0) and docker build --no-cache-filter=lint,builder with the lint and test stages executed rather than cached. Details and the full mutation table are in the PR.

Correction to the original text of this comment: it claimed the item 2 rename also caught the log line reporting the outer nil error rather than the save error. That was wrong. The pre-change code reassigned err from Save on the line immediately above the check, so that log site always carried the save error. Item 2 is scoping hygiene with no behaviour change, exactly as the issue describes it.

Implemented in https://git.eeqj.de/sneak/webhooker/pulls/132 (base `next`). The store is now built by `newStore`, which applies the cap with `store.MaxAge(...)` so `Options.MaxAge` and every codec agree; it is the only place a store is constructed. Two codec tests decode a re-stamped cookie an hour inside and an hour outside the cap through `Session.Get` alone, so no server-side check participates: reverting the fix fails the rejection test both with `expired` present and with `expired` stubbed out, and stubbing `expired` alone leaves both tests passing. The refresh divisor is pinned two-sidedly and fails at 5 and at 20. Items 2 and 4 fixed; item 3 declined for want of a third use. Verified with `make check` (exit 0) and `docker build --no-cache-filter=lint,builder` with the lint and test stages executed rather than cached. Details and the full mutation table are in the PR. Correction to the original text of this comment: it claimed the item 2 rename also caught the log line reporting the outer nil error rather than the save error. That was wrong. The pre-change code reassigned `err` from `Save` on the line immediately above the check, so that log site always carried the save error. Item 2 is scoping hygiene with no behaviour change, exactly as the issue describes it.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#108