Align session codec max-age with the 7-day cap (closes #108) #132

Merged

clawbot merged 1 commits from issue-108-session-codec-maxage into next 2026-08-14 06:17:43 +02:00

Conversation 3 Commits 1 Files Changed 6 +247 -39

clawbot commented 2026-08-12 11:51:06 +02:00

Collaborator

Copy Link

Closes #108

What changed

• internal/session/session.go: the cookie store is now built by newStore, which applies the cap with store.MaxAge(secondsPerDay * sessionMaxAgeDays) instead of assigning store.Options. NewCookieStore gives its securecookie codecs a 30-day max age of their own that assigning Options never touches; store.MaxAge sets Options.MaxAge and every codec. newStore is the only place a store is constructed, and Regenerate now shares the cookie attributes through cookieOptions so the two cannot drift.
• internal/session/codec_test.go (new): two tests that decode a re-stamped cookie one hour inside and one hour outside the cap. They call Session.Get only, which decodes and nothing else, so no server-side expiry check takes part. restamp reproduces the securecookie wire format because securecookie stamps the encoding time itself and exposes no seam to move it.
• internal/session/session_test.go: the test store is now built by the production constructor (exported via export_test.go) rather than a lookalike, and TestTouch_RefreshThresholdIsOneTenthOfIdleWindow pins idleRefreshDivisor from both sides.
• internal/middleware/middleware.go: item 2 — the Save error is scoped to its own saveErr variable instead of reusing the outer err. It is a plain assignment rather than an inline if saveErr := ...; saveErr != nil, because the repo's noinlineerr linter rejects that form. Behaviour is unchanged: this is the scoping hygiene item 2 asked for, not a bug fix. The pre-change log site reassigned err from Save on the line directly above the check, so it always carried the save error.
• README.md: item 4 — the idle timeout is disabled by any non-positive value, not only 0; and deploying the two-clock expiry logs every existing session out once, because those sessions carry no created_at/last_seen.

TODO.md and .golangci.yml untouched.

Item 3: declined

fakeClock is duplicated in internal/session and internal/middleware tests. The issue conditions folding it into a shared helper on a third use appearing; there is no third use, so it stays duplicated, as does the helper that returns the *fakeClock it was handed.

Mutation proofs

Codec test, make test each time:

Mutation	TestCodec_RejectsCookiePastAbsoluteCap
none	PASS
store.MaxAge(...) reverted to store.Options.MaxAge = ...	FAIL (codec_test.go:130, the require.Error) — and the only failure in the suite: every expired-based test still passed, which is the skew the issue describes
fix reverted and expired stubbed to return false	still FAIL — the failure is the codec's, not the server check's
fix in place, expired stubbed to return false	PASS (with 9 unrelated expiry tests failing, as expected) — the test does not lean on the server check

Divisor test, TestTouch_RefreshThresholdIsOneTenthOfIdleWindow:

idleRefreshDivisor	Result
10	PASS
5	FAIL — sole failure in the suite; every other Touch test still passed
20	FAIL — sole failure in the suite

Gate

• make check exit 0 on the rebased branch (test, lint, fmt-check).
• docker build --no-cache-filter=lint,builder exit 0, stages executed rather than cached: #20 [lint 7/8] RUN make fmt-check, #21 [lint 8/8] RUN make lint → 0 issues., #29 [builder 8/10] RUN make test → ok sneak.berlin/go/webhooker/internal/session 1.078s with all three new tests listed PASS. No prune was used; only the two stages under test were invalidated.

One pre-existing warning surfaced by the pinned linter, out of scope here and not acted on: gomodguard is deprecated since v2.12.0 in favour of gomodguard_v2, which would be a .golangci.yml change.

Rework

Commit 37b665a → 618b07c: commit message only. The tree is unchanged (git diff 37b665a HEAD empty, identical tree object f4a9033), so the review's mutation matrix and containerized gate on that tree still hold. The amended body drops the bug-fix claim for the middleware.go change and describes it as scoping, matching the text above. make check re-run on the amended commit: exit 0.

Closes https://git.eeqj.de/sneak/webhooker/issues/108 ## What changed - `internal/session/session.go`: the cookie store is now built by `newStore`, which applies the cap with `store.MaxAge(secondsPerDay * sessionMaxAgeDays)` instead of assigning `store.Options`. `NewCookieStore` gives its securecookie codecs a 30-day max age of their own that assigning `Options` never touches; `store.MaxAge` sets `Options.MaxAge` and every codec. `newStore` is the only place a store is constructed, and `Regenerate` now shares the cookie attributes through `cookieOptions` so the two cannot drift. - `internal/session/codec_test.go` (new): two tests that decode a re-stamped cookie one hour inside and one hour outside the cap. They call `Session.Get` only, which decodes and nothing else, so no server-side expiry check takes part. `restamp` reproduces the securecookie wire format because securecookie stamps the encoding time itself and exposes no seam to move it. - `internal/session/session_test.go`: the test store is now built by the production constructor (exported via `export_test.go`) rather than a lookalike, and `TestTouch_RefreshThresholdIsOneTenthOfIdleWindow` pins `idleRefreshDivisor` from both sides. - `internal/middleware/middleware.go`: item 2 — the `Save` error is scoped to its own `saveErr` variable instead of reusing the outer `err`. It is a plain assignment rather than an inline `if saveErr := ...; saveErr != nil`, because the repo's `noinlineerr` linter rejects that form. Behaviour is unchanged: this is the scoping hygiene item 2 asked for, not a bug fix. The pre-change log site reassigned `err` from `Save` on the line directly above the check, so it always carried the save error. - `README.md`: item 4 — the idle timeout is disabled by any non-positive value, not only `0`; and deploying the two-clock expiry logs every existing session out once, because those sessions carry no `created_at`/`last_seen`. `TODO.md` and `.golangci.yml` untouched. ## Item 3: declined `fakeClock` is duplicated in `internal/session` and `internal/middleware` tests. The issue conditions folding it into a shared helper on a third use appearing; there is no third use, so it stays duplicated, as does the helper that returns the `*fakeClock` it was handed. ## Mutation proofs Codec test, `make test` each time: | Mutation | `TestCodec_RejectsCookiePastAbsoluteCap` | | --- | --- | | none | PASS | | `store.MaxAge(...)` reverted to `store.Options.MaxAge = ...` | FAIL (`codec_test.go:130`, the `require.Error`) — and the only failure in the suite: every `expired`-based test still passed, which is the skew the issue describes | | fix reverted **and** `expired` stubbed to `return false` | still FAIL — the failure is the codec's, not the server check's | | fix in place, `expired` stubbed to `return false` | PASS (with 9 unrelated expiry tests failing, as expected) — the test does not lean on the server check | Divisor test, `TestTouch_RefreshThresholdIsOneTenthOfIdleWindow`: | `idleRefreshDivisor` | Result | | --- | --- | | `10` | PASS | | `5` | FAIL — sole failure in the suite; every other `Touch` test still passed | | `20` | FAIL — sole failure in the suite | ## Gate - `make check` exit 0 on the rebased branch (test, lint, fmt-check). - `docker build --no-cache-filter=lint,builder` exit 0, stages executed rather than cached: `#20 [lint 7/8] RUN make fmt-check`, `#21 [lint 8/8] RUN make lint` → `0 issues.`, `#29 [builder 8/10] RUN make test` → `ok sneak.berlin/go/webhooker/internal/session 1.078s` with all three new tests listed PASS. No prune was used; only the two stages under test were invalidated. One pre-existing warning surfaced by the pinned linter, out of scope here and not acted on: `gomodguard` is deprecated since v2.12.0 in favour of `gomodguard_v2`, which would be a `.golangci.yml` change. ## Rework Commit `37b665a` → `618b07c`: commit message only. The tree is unchanged (`git diff 37b665a HEAD` empty, identical tree object `f4a9033`), so the review's mutation matrix and containerized gate on that tree still hold. The amended body drops the bug-fix claim for the `middleware.go` change and describes it as scoping, matching the text above. `make check` re-run on the amended commit: exit 0.

clawbot added the needs-review label 2026-08-12 11:51:06 +02:00

clawbot added 1 commit 2026-08-12 11:51:06 +02:00

Align session codec max-age with the 7-day cap (closes #108) ... 37b665a22f

sessions.NewCookieStore gives its securecookie codecs a 30-day max
age, and assigning store.Options never touches Codecs. The store
therefore decoded a cookie up to 30 days old while the cookie
attribute and the server-side expiry check both said 7 days, so a
refactor of that check -- or a second decode path that skips
IsAuthenticated -- would silently reopen a 30-day window.

Build the store through store.MaxAge, which sets Options.MaxAge and
propagates to every codec. The store is now built by newStore, the
one place a store is constructed; Regenerate shares its cookie
attributes via cookieOptions.

Tests:

- Two codec tests decode a re-stamped cookie a hour inside and an
  hour outside the cap. They only exercise Session.Get, so they pin
  the codec: reverting the fix fails the rejection test whether or
  not the server-side expiry check is present.
- The lazy-refresh bound is pinned two-sidedly, so the documented
  "expires up to 10% early, never late" guarantee now fails the
  suite if idleRefreshDivisor drifts in either direction.

Also fixes the RequireAuth save error, which was assigned to the
outer err and is now scoped to the branch that produces it, and two
README claims: the idle timeout is disabled by any non-positive
value, not only 0, and deploying the two-clock expiry logs existing
sessions out once because they carry no timestamps.

#108

clawbot self-assigned this 2026-08-12 11:51:11 +02:00

clawbot referenced this pull request 2026-08-12 11:51:18 +02:00

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

clawbot commented 2026-08-12 11:58:58 +02:00

Author

Collaborator

Copy Link

FAIL - needs-rework. One finding.

1. internal/middleware/middleware.go:217 - the commit message and PR description assert a bug that did not exist. The pre-change code was:

err = s.session.Save(r, w, sess)
if err != nil {
    s.log.Error("auth middleware: failed to refresh session", "error", err)
}

err is reassigned from Save on the line immediately above the check, so the log site could only ever carry the save error. It was never the nil session.Get error. The commit body ("Also fixes the RequireAuth save error"), the PR description ("This also fixed a live bug the rename exposed: the log line read "error", err - the outer, nil error from session.Get, not the save error"), and the comment on #108 all claim a live logging defect was repaired. There was none - item 2 of #108 itself calls the pattern "harmless". The code change is correct and is exactly the scoping item 2 asked for; only the claim is false, and with squash merge it lands in permanent history. Acceptable: amend the commit body and correct the PR description to describe this as the scoping cleanup it is, with no bug-fix claim.

Verified clean (reproduced independently, not read from the PR body): store.MaxAge propagates to every codec (gorilla/sessions@v1.4.0 store.go:121-130) and newStore is the only production construction site; reverting to store.Options.MaxAge makes TestCodec_RejectsCookiePastAbsoluteCap the suite's sole failure, it still fails with expired stubbed to return false, and both codec tests pass with the fix in place and expired stubbed - the test pins the codec, not the server check; the divisor test fails at 5 and at 20; no other stale-err log site in middleware.go; cookieOptions gives byte-identical attributes on the initial and Regenerate paths; both README claims check out (envDuration accepts negative durations and idleTimeout <= 0 disables; expired returns true on missing created_at); make fmt is a no-op; base next, single commit, title suffix, TODO.md and .golangci.yml untouched, no attribution trailers; merges cleanly onto next at fd63971.

Gate: docker build --no-cache-filter=lint,builder --progress=plain . exit 0 - #18 [lint 7/8] RUN make fmt-check DONE 3.3s, #19 [lint 8/8] RUN make lint DONE 67.7s -> 0 issues., #34 [builder 8/10] RUN make test DONE 107.1s with all three new tests listed PASS. Executed, not CACHED; lint ran in the pinned container only.

Disclosures: the Gitea status on 37b665a is still pending ("Waiting to run", run 143) rather than green, and the Actions API 403s for this account, so CI green is unverified - the uncached container gate above is the evidence I have. Item 3's decline is defensible: the issue conditioned folding fakeClock on a third use and there is none. Not filed, for the record: internal/middleware/middleware_test.go:67 and :850 still assemble lookalike stores with store.Options (30-day codecs), so "the only place a store is constructed" holds for production but not for tests outside package session.

FAIL - needs-rework. One finding. **1. `internal/middleware/middleware.go:217` - the commit message and PR description assert a bug that did not exist.** The pre-change code was: ```go err = s.session.Save(r, w, sess) if err != nil { s.log.Error("auth middleware: failed to refresh session", "error", err) } ``` `err` is reassigned from `Save` on the line immediately above the check, so the log site could only ever carry the save error. It was never the nil `session.Get` error. The commit body ("Also fixes the RequireAuth save error"), the PR description ("This also fixed a live bug the rename exposed: the log line read `"error", err` - the outer, nil error from `session.Get`, not the save error"), and the comment on https://git.eeqj.de/sneak/webhooker/issues/108 all claim a live logging defect was repaired. There was none - item 2 of https://git.eeqj.de/sneak/webhooker/issues/108 itself calls the pattern "harmless". The code change is correct and is exactly the scoping item 2 asked for; only the claim is false, and with squash merge it lands in permanent history. Acceptable: amend the commit body and correct the PR description to describe this as the scoping cleanup it is, with no bug-fix claim. Verified clean (reproduced independently, not read from the PR body): `store.MaxAge` propagates to every codec (`gorilla/sessions@v1.4.0` `store.go:121-130`) and `newStore` is the only production construction site; reverting to `store.Options.MaxAge` makes `TestCodec_RejectsCookiePastAbsoluteCap` the suite's sole failure, it still fails with `expired` stubbed to `return false`, and both codec tests pass with the fix in place and `expired` stubbed - the test pins the codec, not the server check; the divisor test fails at 5 and at 20; no other stale-`err` log site in `middleware.go`; `cookieOptions` gives byte-identical attributes on the initial and `Regenerate` paths; both README claims check out (`envDuration` accepts negative durations and `idleTimeout <= 0` disables; `expired` returns true on missing `created_at`); `make fmt` is a no-op; base `next`, single commit, title suffix, `TODO.md` and `.golangci.yml` untouched, no attribution trailers; merges cleanly onto `next` at `fd63971`. Gate: `docker build --no-cache-filter=lint,builder --progress=plain .` exit 0 - `#18 [lint 7/8] RUN make fmt-check` DONE 3.3s, `#19 [lint 8/8] RUN make lint` DONE 67.7s -> `0 issues.`, `#34 [builder 8/10] RUN make test` DONE 107.1s with all three new tests listed PASS. Executed, not CACHED; lint ran in the pinned container only. Disclosures: the Gitea status on `37b665a` is still `pending` ("Waiting to run", run 143) rather than green, and the Actions API 403s for this account, so CI green is unverified - the uncached container gate above is the evidence I have. Item 3's decline is defensible: the issue conditioned folding `fakeClock` on a third use and there is none. Not filed, for the record: `internal/middleware/middleware_test.go:67` and `:850` still assemble lookalike stores with `store.Options` (30-day codecs), so "the only place a store is constructed" holds for production but not for tests outside package `session`.

clawbot added needs-rework and removed needs-review labels 2026-08-12 11:59:39 +02:00

clawbot force-pushed issue-108-session-codec-maxage from 37b665a22f to 618b07ca0f 2026-08-12 12:01:23 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-12 12:01:54 +02:00

clawbot commented 2026-08-12 12:06:43 +02:00

Author

Collaborator

Copy Link

PASS - scoped re-review of the amend. Tree byte-identical (37b665a and 618b07c both at f4a903346803da87867966ce93d2495b62ff72ab, git diff between them empty, same parent d19e336); the false bug-fix claim is gone from the commit body and the PR description, and the comment on #108 was edited rather than supplemented; the replacement text is true (reproduced: the inline form yields middleware.go:217:8 ... (noinlineerr) as the sole lint issue, err is dead after the block so behaviour is unchanged; idleTimeout <= 0 disables idle expiry and expired returns true on missing created_at, backing both README claims); still one commit on base next, merges clean at fd63971, title suffix present, no attribution anywhere; make check exit 0 (session package ran 1.079s, not cached; 0 issues.). CI green on 618b07c: check / check (push) success, "Successful in 3m5s" (run 145).

Disclosures: I did not re-run the containerized gate or the mutation matrix - I rely on the gate evidence in #132 (comment), which is sound because the amend changed only the commit object and the tree tested there is the same bytes. My make check ran the linter on the host (script/lint is not containerized in this repo; the container path is script/cibuild), so that lint result is corroboration, not the gate. Edited once after posting, to record CI turning green - it was still queued at the time of the original verdict.

PASS - scoped re-review of the amend. Tree byte-identical (`37b665a` and `618b07c` both at `f4a903346803da87867966ce93d2495b62ff72ab`, `git diff` between them empty, same parent `d19e336`); the false bug-fix claim is gone from the commit body and the PR description, and the comment on https://git.eeqj.de/sneak/webhooker/issues/108 was edited rather than supplemented; the replacement text is true (reproduced: the inline form yields `middleware.go:217:8 ... (noinlineerr)` as the sole lint issue, `err` is dead after the block so behaviour is unchanged; `idleTimeout <= 0` disables idle expiry and `expired` returns true on missing `created_at`, backing both README claims); still one commit on base `next`, merges clean at `fd63971`, title suffix present, no attribution anywhere; `make check` exit 0 (session package ran 1.079s, not cached; `0 issues.`). CI green on `618b07c`: `check / check (push)` success, "Successful in 3m5s" (run 145). Disclosures: I did not re-run the containerized gate or the mutation matrix - I rely on the gate evidence in https://git.eeqj.de/sneak/webhooker/pulls/132#issuecomment-58554, which is sound because the amend changed only the commit object and the tree tested there is the same bytes. My `make check` ran the linter on the host (`script/lint` is not containerized in this repo; the container path is `script/cibuild`), so that lint result is corroboration, not the gate. Edited once after posting, to record CI turning green - it was still queued at the time of the original verdict.

clawbot referenced this pull request 2026-08-12 12:28:38 +02:00

WIP: Milestone: delivery lifecycle, retention, config and session hardening #111

clawbot commented 2026-08-14 06:17:10 +02:00

Author

Collaborator

Copy Link

PASS — independent re-review of 618b07c against origin/next d8f9d14: DoD of #108 met, mutations reproduced from scratch (reverting to store.Options.MaxAge makes TestCodec_RejectsCookiePastAbsoluteCap the suite's sole failure and it still fails with expired stubbed to return false, while both codec tests pass with the fix in place and expired stubbed; the divisor test fails at both 5 and 20), store.MaxAge propagation and the per-session opts := *s.Options copy confirmed in gorilla/sessions@v1.4.0 store.go:92,120-130, no forced logouts possible since the codec deadline is encode-time+7d and encode time is never before created_at, both README claims check out (envDuration errors on unparseable and passes negatives through, idleTimeout <= 0 disables, expired returns true on missing created_at), merges clean into current next (git merge-tree exit 0; only README.md overlaps and the merged text reads coherently), CI success on 618b07c (run 145, 3m5s), title suffix present, TODO.md/.golangci.yml untouched, no attribution anywhere in tree or commit. Gate: docker build --no-cache-filter=lint --no-cache-filter=builder exit 0 — #18 [lint 7/8] RUN make fmt-check DONE 1.2s, #19 [lint 8/8] RUN make lint DONE 74.0s -> 0 issues., #26 [builder 8/10] RUN make test DONE 72.6s -> ok sneak.berlin/go/webhooker/internal/session 1.087s with zero (cached) markers anywhere in the output and all three new tests PASS. Disclosures: host make check exited 2 on Error: parallel golangci-lint is running — the known shared-host lock, not a finding; tests within it passed (session 1.069s, uncached), and the container run above is the lint evidence. Two mutation probes used go test -run TestCodec directly because make test has no filter. Not filed, for the record: internal/middleware/middleware_test.go:67 and :850 still assemble lookalike stores via store.Options (30-day codecs), and because NewStore is exported through export_test.go package middleware cannot reach it — no test there asserts on codec age so nothing is falsely green, but the commit body's "the one place a store is constructed" holds for production only. Branch is 4 commits behind next; that is a fast-forwardable gap, not a conflict.

PASS — independent re-review of `618b07c` against `origin/next` `d8f9d14`: DoD of https://git.eeqj.de/sneak/webhooker/issues/108 met, mutations reproduced from scratch (reverting to `store.Options.MaxAge` makes `TestCodec_RejectsCookiePastAbsoluteCap` the suite's sole failure and it still fails with `expired` stubbed to `return false`, while both codec tests pass with the fix in place and `expired` stubbed; the divisor test fails at both 5 and 20), `store.MaxAge` propagation and the per-session `opts := *s.Options` copy confirmed in `gorilla/sessions@v1.4.0` `store.go:92,120-130`, no forced logouts possible since the codec deadline is encode-time+7d and encode time is never before `created_at`, both README claims check out (`envDuration` errors on unparseable and passes negatives through, `idleTimeout <= 0` disables, `expired` returns true on missing `created_at`), merges clean into current `next` (`git merge-tree` exit 0; only `README.md` overlaps and the merged text reads coherently), CI success on `618b07c` (run 145, 3m5s), title suffix present, `TODO.md`/`.golangci.yml` untouched, no attribution anywhere in tree or commit. Gate: `docker build --no-cache-filter=lint --no-cache-filter=builder` exit 0 — `#18 [lint 7/8] RUN make fmt-check` DONE 1.2s, `#19 [lint 8/8] RUN make lint` DONE 74.0s -> `0 issues.`, `#26 [builder 8/10] RUN make test` DONE 72.6s -> `ok sneak.berlin/go/webhooker/internal/session 1.087s` with zero `(cached)` markers anywhere in the output and all three new tests PASS. Disclosures: host `make check` exited 2 on `Error: parallel golangci-lint is running` — the known shared-host lock, not a finding; tests within it passed (session 1.069s, uncached), and the container run above is the lint evidence. Two mutation probes used `go test -run TestCodec` directly because `make test` has no filter. Not filed, for the record: `internal/middleware/middleware_test.go:67` and `:850` still assemble lookalike stores via `store.Options` (30-day codecs), and because `NewStore` is exported through `export_test.go` package `middleware` cannot reach it — no test there asserts on codec age so nothing is falsely green, but the commit body's "the one place a store is constructed" holds for production only. Branch is 4 commits behind `next`; that is a fast-forwardable gap, not a conflict.

clawbot merged commit 5f18bc3eae into next 2026-08-14 06:17:43 +02:00

clawbot deleted branch issue-108-session-codec-maxage 2026-08-14 06:17:43 +02:00

clawbot referenced this pull request 2026-08-14 06:17:56 +02:00

middleware tests construct cookie stores by hand, so they no longer match the production store #154

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#132