fix: set Secure/HttpOnly/SameSite on session cookies (closes #47) #48

clawbot · 2026-08-07T16:24:26+02:00

clawbot commented

2026-08-07 16:24:26 +02:00

closes #47

Fixes the two remaining gosec findings on main, both G124
(http.Cookie missing or has insecure Secure, HttpOnly, or
SameSite attribute):

internal/session/session.go:84 (CreateSession, the login
set-cookie path)
internal/session/session.go:128 (ClearSession, the logout
delete-cookie path)

What changed

Both cookie-writing paths now unconditionally set Secure: true,
HttpOnly: true, and SameSite: http.SameSiteStrictMode.
The secure field (previously wired to !config.Debug) and the
sameSite field are removed from session.Manager, and the dead
secure-toggle parameter is removed from session.NewManager, which
now takes only the signing key (reviewer-directed; the mechanical
call-shape updates in session_test.go leave every assertion
untouched).
TDD per repo rules: the first commit adds
TestSessionCookieAttributesAlwaysSecure (failing), asserting that
every cookie emitted by the session manager carries HttpOnly,
Secure, and SameSite of Lax or stricter, for both write paths.
The second commit makes it pass.
TODO.md updated per its Workflow section (Next Step completed,
next Future Step promoted, stale "10 open findings" Status text
corrected).

Attribute choices and reasoning

Secure: true always: the G124 analyzer only accepts a constant
true store, and there is no legitimate configuration in which the
authentication cookie should be sent over plaintext HTTP. The old
behavior disabled Secure whenever debug was on. Local development
over http://localhost keeps working: browsers treat localhost as
a trustworthy origin and accept Secure cookies there. Any
plain-HTTP flow on a non-localhost host will no longer keep a
session, which is the point of the fix.
SameSite: Strict (unchanged from current production behavior, and
stricter than the Lax minimum): the login form is a same-origin POST
to / followed by a same-site redirect, so Strict breaks nothing.
HttpOnly: true (unchanged).

Verification

make check (tests, golangci-lint, fmt-check) is fully green on the
branch head cb9e14e: all tests pass and the linter reports 0 issues,
independently confirmed by the reviewer in a fresh worktree. Commit
history: ca15f52 (failing test) → 02ca16a (fix + TODO.md, closes
#47) → cb9e14e (drop the dead NewManager parameter).

closes #47 Fixes the two remaining `gosec` findings on `main`, both `G124` (http.Cookie missing or has insecure `Secure`, `HttpOnly`, or `SameSite` attribute): - `internal/session/session.go:84` (`CreateSession`, the login set-cookie path) - `internal/session/session.go:128` (`ClearSession`, the logout delete-cookie path) ## What changed - Both cookie-writing paths now unconditionally set `Secure: true`, `HttpOnly: true`, and `SameSite: http.SameSiteStrictMode`. - The `secure` field (previously wired to `!config.Debug`) and the `sameSite` field are removed from `session.Manager`, and the dead secure-toggle parameter is removed from `session.NewManager`, which now takes only the signing key (reviewer-directed; the mechanical call-shape updates in `session_test.go` leave every assertion untouched). - TDD per repo rules: the first commit adds `TestSessionCookieAttributesAlwaysSecure` (failing), asserting that every cookie emitted by the session manager carries `HttpOnly`, `Secure`, and `SameSite` of Lax or stricter, for both write paths. The second commit makes it pass. - `TODO.md` updated per its Workflow section (Next Step completed, next Future Step promoted, stale "10 open findings" Status text corrected). ## Attribute choices and reasoning - `Secure: true` always: the `G124` analyzer only accepts a constant `true` store, and there is no legitimate configuration in which the authentication cookie should be sent over plaintext HTTP. The old behavior disabled `Secure` whenever `debug` was on. Local development over `http://localhost` keeps working: browsers treat `localhost` as a trustworthy origin and accept `Secure` cookies there. Any plain-HTTP flow on a non-localhost host will no longer keep a session, which is the point of the fix. - `SameSite: Strict` (unchanged from current production behavior, and stricter than the Lax minimum): the login form is a same-origin POST to `/` followed by a same-site redirect, so `Strict` breaks nothing. - `HttpOnly: true` (unchanged). ## Verification `make check` (tests, golangci-lint, fmt-check) is fully green on the branch head `cb9e14e`: all tests pass and the linter reports 0 issues, independently confirmed by the reviewer in a fresh worktree. Commit history: `ca15f52` (failing test) → `02ca16a` (fix + TODO.md, closes #47) → `cb9e14e` (drop the dead `NewManager` parameter).

clawbot added 2 commits 2026-08-07 16:24:27 +02:00

test: require Secure/HttpOnly/SameSite on all session cookies ca15f52dcc

Add a failing test asserting that every cookie written by the session
manager (both the CreateSession set-cookie path and the ClearSession
delete-cookie path) carries HttpOnly, Secure, and a SameSite mode of
Lax or stricter, regardless of constructor arguments. Session cookies
carry authentication state and must never be sent over plaintext HTTP.

Currently fails for the constructor secure=false case, which produces
cookies without the Secure attribute (gosec G124 at
internal/session/session.go:84 and :128).

fix: always set Secure/HttpOnly/SameSite on session cookies (closes #47 )

check / check (push) Successful in 2m4s

Details

02ca16a68a

Resolve the two remaining gosec G124 findings (internal/session/
session.go:84 and :128): session cookies are now unconditionally
Secure, HttpOnly, and SameSite=Strict on both the CreateSession
set-cookie path and the ClearSession delete-cookie path. gosec requires
these attributes to be constant, and there is no legitimate
configuration in which the authentication cookie should be weaker, so
the former secure toggle (wired to !config.Debug) is removed rather
than kept as a variable.

The toggle parameter on NewManager is retained as an ignored blank
parameter so existing call sites (including tests) keep compiling;
removing it is tracked as a Future Step in TODO.md. Local development
over http://localhost keeps working because browsers treat localhost as
a trustworthy origin and accept Secure cookies there.

Update TODO.md per its Workflow section: record this step as completed,
promote the manual auth/URL-flow test pass to Next Step, and correct
the stale Status text (make check is now green).

clawbot commented

2026-08-07 16:24:35 +02:00

Scope-guard enumeration (fresh make check on main at b6e9ac2 before any changes, golangci-lint via script/lint):

Total gosec findings: 2 — well under the ~30 stop threshold from issue #47, and matching the 2026-08-07 scope update on that issue. No other linters reported issues; all tests were green.

G124 — internal/session/session.go:84 — "http.Cookie missing or has insecure Secure, HttpOnly, or SameSite attribute" (CreateSession: Secure and SameSite were non-constant fields, with Secure wired to !config.Debug)
G124 — internal/session/session.go:128 — same message (ClearSession: identical pattern on the delete-cookie path)

Both are genuine findings and are fixed in this PR by hardcoding Secure: true, HttpOnly: true, SameSite: http.SameSiteStrictMode; no //nolint suppressions and no linter config changes. make check is fully green on the branch head (02ca16a).

Scope-guard enumeration (fresh `make check` on `main` at b6e9ac2 before any changes, golangci-lint via `script/lint`): Total gosec findings: **2** — well under the ~30 stop threshold from issue #47, and matching the 2026-08-07 scope update on that issue. No other linters reported issues; all tests were green. 1. `G124` — `internal/session/session.go:84` — "http.Cookie missing or has insecure Secure, HttpOnly, or SameSite attribute" (`CreateSession`: `Secure` and `SameSite` were non-constant fields, with `Secure` wired to `!config.Debug`) 2. `G124` — `internal/session/session.go:128` — same message (`ClearSession`: identical pattern on the delete-cookie path) Both are genuine findings and are fixed in this PR by hardcoding `Secure: true`, `HttpOnly: true`, `SameSite: http.SameSiteStrictMode`; no `//nolint` suppressions and no linter config changes. `make check` is fully green on the branch head (02ca16a).

clawbot commented

2026-08-07 16:53:43 +02:00

Review: FAIL — one blocking item

Independently verified at head 02ca16a in a fresh worktree:

What passes:

make check fully green (all tests, golangci-lint 0 issues, fmt-check clean). For the record: the first two verification runs failed spuriously because the shared golangci-lint analysis cache replayed results recorded under a since-deleted sibling worktree path; golangci-lint cache clean resolved it. Nothing in this PR was at fault.
The fix is correct and minimal: both cookie-writing paths (CreateSession, ClearSession) now carry constant Secure: true, HttpOnly: true, SameSite: Strict — constants are exactly what the G124 analyzer requires, so no //nolint was needed and none was used. SameSite: Strict preserves the pre-existing production value; the only behavior change is Secure no longer being disabled in debug mode, which is the point of the fix, and http://localhost development is unaffected since browsers treat localhost as a trustworthy origin.
TDD per repo rules: first commit is test-only (TestSessionCookieAttributesAlwaysSecure), covering both write paths × both constructor arguments, asserting all three attributes with diagnostic logging, and it failed for the right reason before the fix. Existing tests are untouched. The linter config is untouched.
TODO.md updated correctly per its Workflow section; commit messages are clean, the finishing commit title ends with (closes #47), and there are no forbidden trailers.

Blocking:

NewManager(signingKey string, _ bool) keeps a dead, ignored parameter, with a doc comment promising later removal and a deferred P1 entry in TODO.md. That is a half-measure of exactly the kind sneak rejected on PR #46 — pre-1.0 there is no installed base to keep compiling against, so update everything in one pass:

Change the signature to NewManager(signingKey string).
Update the internal/handlers/handlers.go call site.
Mechanically update the eight NewManager call sites in internal/session/session_test.go and the one in the new attributes test — call shape only; no assertion may change. (This call-site edit is reviewer-directed under the one-pass precedent; if sneak prefers the strict reading of the "no modifying existing tests without approval" rule, say so here and the parameter stays with the P1 follow-up instead.)
Drop the now-moot P1 Future Step from TODO.md and the "retained only so call sites keep compiling" language from the godoc.
Note the new attributes test loses its constructorBoolArg dimension in the process — collapsing that loop is expected, its assertions stay.

make check green, push, and summarize here for re-review.

## Review: FAIL — one blocking item Independently verified at head `02ca16a` in a fresh worktree: **What passes:** - `make check` fully green (all tests, golangci-lint 0 issues, fmt-check clean). For the record: the first two verification runs failed spuriously because the shared golangci-lint analysis cache replayed results recorded under a since-deleted sibling worktree path; `golangci-lint cache clean` resolved it. Nothing in this PR was at fault. - The fix is correct and minimal: both cookie-writing paths (`CreateSession`, `ClearSession`) now carry constant `Secure: true`, `HttpOnly: true`, `SameSite: Strict` — constants are exactly what the `G124` analyzer requires, so no `//nolint` was needed and none was used. `SameSite: Strict` preserves the pre-existing production value; the only behavior change is `Secure` no longer being disabled in debug mode, which is the point of the fix, and `http://localhost` development is unaffected since browsers treat localhost as a trustworthy origin. - TDD per repo rules: first commit is test-only (`TestSessionCookieAttributesAlwaysSecure`), covering both write paths × both constructor arguments, asserting all three attributes with diagnostic logging, and it failed for the right reason before the fix. Existing tests are untouched. The linter config is untouched. - `TODO.md` updated correctly per its Workflow section; commit messages are clean, the finishing commit title ends with ` (closes #47)`, and there are no forbidden trailers. **Blocking:** `NewManager(signingKey string, _ bool)` keeps a dead, ignored parameter, with a doc comment promising later removal and a deferred P1 entry in `TODO.md`. That is a half-measure of exactly the kind sneak rejected on PR #46 — pre-1.0 there is no installed base to keep compiling against, so update everything in one pass: - Change the signature to `NewManager(signingKey string)`. - Update the `internal/handlers/handlers.go` call site. - Mechanically update the eight `NewManager` call sites in `internal/session/session_test.go` and the one in the new attributes test — call shape only; no assertion may change. (This call-site edit is reviewer-directed under the one-pass precedent; if sneak prefers the strict reading of the "no modifying existing tests without approval" rule, say so here and the parameter stays with the P1 follow-up instead.) - Drop the now-moot P1 Future Step from `TODO.md` and the "retained only so call sites keep compiling" language from the godoc. - Note the new attributes test loses its `constructorBoolArg` dimension in the process — collapsing that loop is expected, its assertions stay. `make check` green, push, and summarize here for re-review.

clawbot added the needs-rework label 2026-08-07 16:54:56 +02:00

clawbot referenced this pull request

2026-08-07 16:55:00 +02:00

Fix remaining gosec lint findings on main #47

sneak referenced this issue from a commit

2026-08-07 17:24:17 +02:00

refactor: drop the ignored secure toggle parameter from session.NewManager

sneak added 1 commit 2026-08-07 17:24:17 +02:00

refactor: drop the ignored secure toggle parameter from session.NewManager

check / check (push) Successful in 1m48s

Details

cb9e14eee7

Per review on PR #48: pre-1.0 there is no installed base to keep
compiling against, so remove the dead parameter in one pass instead of
deferring. NewManager now takes only the signing key. Update the
handlers.go call site and its stale comment, mechanically update the
NewManager call shapes in the session tests (assertions unchanged),
collapse the now-meaningless constructor-argument loop in the cookie
attributes test, and drop the moot P1 Future Step from TODO.md.

Reviewer-directed test call-site updates; cookie behavior is unchanged
from the previous commit (always Secure/HttpOnly/SameSite=Strict).

clawbot commented

2026-08-07 17:24:25 +02:00

Rework for the blocking item is pushed: cb9e14e.

session.NewManager now takes only the signing key: NewManager(signingKey string). The ignored _ bool parameter and the "retained only so call sites keep compiling" godoc language are gone.
internal/handlers/handlers.go call site updated, along with its stale comment about the ignored argument.
The eight NewManager call sites in internal/session/session_test.go updated — call shape only, per the review's direction; no assertion was changed (one stale // Test with secure=true comment removed).
The constructorBoolArg loop in session_cookie_attributes_test.go collapsed as anticipated in the review; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, both write paths, with diagnostic logging) are identical.
The now-moot P1 Future Step removed from TODO.md (make fmt run).

make check is fully green at cb9e14e (all tests pass, golangci-lint 0 issues after a golangci-lint cache clean to flush the stale-worktree-path cache issue noted in the review, fmt-check clean). Cookie behavior is unchanged from 02ca16a. Ready for re-review.

Rework for the blocking item is pushed: `cb9e14e`. - `session.NewManager` now takes only the signing key: `NewManager(signingKey string)`. The ignored `_ bool` parameter and the "retained only so call sites keep compiling" godoc language are gone. - `internal/handlers/handlers.go` call site updated, along with its stale comment about the ignored argument. - The eight `NewManager` call sites in `internal/session/session_test.go` updated — call shape only, per the review's direction; no assertion was changed (one stale `// Test with secure=true` comment removed). - The `constructorBoolArg` loop in `session_cookie_attributes_test.go` collapsed as anticipated in the review; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, both write paths, with diagnostic logging) are identical. - The now-moot P1 Future Step removed from `TODO.md` (`make fmt` run). `make check` is fully green at `cb9e14e` (all tests pass, golangci-lint 0 issues after a `golangci-lint cache clean` to flush the stale-worktree-path cache issue noted in the review, fmt-check clean). Cookie behavior is unchanged from `02ca16a`. Ready for re-review.

clawbot commented

2026-08-07 17:29:52 +02:00

Re-review: PASS

Verified at head cb9e14e in a fresh worktree with an isolated lint cache: make check fully green (all tests, golangci-lint 0 issues, fmt-check clean) — independently run, not taken from the implementor's report.

The rework addressed the blocking item exactly as directed:

NewManager(signingKey string) — dead parameter gone, godoc no longer promises a follow-up.
internal/handlers/handlers.go call site and its stale "second argument is ignored" comment fixed.
All eight session_test.go call sites updated to the one-argument shape — I diffed them individually: call shape only, every assertion untouched. The one removed comment (// Test with secure=true) described the deleted argument, so its removal is correct.
The attributes test collapsed its constructorBoolArg loop as anticipated; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, both CreateSession and ClearSession paths) are identical, and diagnostic logging is retained.
The moot P1 Future Step is gone from TODO.md.

Branch history is clean and tells the right story: ca15f52 (failing test) → 02ca16a (fix + TODO, closes #47) → cb9e14e (reviewer-directed parameter removal). No forbidden trailers anywhere. Cookie behavior is unchanged from the reviewed fix: constant Secure/HttpOnly/SameSite=Strict on both write paths, which is what closes the two G124 findings and makes make check green on main once merged.

Marking merge-ready and assigning sneak for merge into protected main. Merging this closes #47.

## Re-review: PASS Verified at head `cb9e14e` in a fresh worktree with an isolated lint cache: `make check` fully green (all tests, golangci-lint 0 issues, fmt-check clean) — independently run, not taken from the implementor's report. The rework addressed the blocking item exactly as directed: - `NewManager(signingKey string)` — dead parameter gone, godoc no longer promises a follow-up. - `internal/handlers/handlers.go` call site and its stale "second argument is ignored" comment fixed. - All eight `session_test.go` call sites updated to the one-argument shape — I diffed them individually: call shape only, every assertion untouched. The one removed comment (`// Test with secure=true`) described the deleted argument, so its removal is correct. - The attributes test collapsed its `constructorBoolArg` loop as anticipated; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, both `CreateSession` and `ClearSession` paths) are identical, and diagnostic logging is retained. - The moot P1 Future Step is gone from `TODO.md`. Branch history is clean and tells the right story: `ca15f52` (failing test) → `02ca16a` (fix + TODO, `closes #47`) → `cb9e14e` (reviewer-directed parameter removal). No forbidden trailers anywhere. Cookie behavior is unchanged from the reviewed fix: constant `Secure`/`HttpOnly`/`SameSite=Strict` on both write paths, which is what closes the two `G124` findings and makes `make check` green on `main` once merged. Marking `merge-ready` and assigning sneak for merge into protected `main`. Merging this closes #47.

clawbot added merge-ready and removed needs-rework labels 2026-08-07 17:31:58 +02:00

sneak was assigned by clawbot

2026-08-07 17:31:59 +02:00

clawbot referenced this pull request

2026-08-07 17:35:33 +02:00

refactor: extract signature package from imgcache #46

sneak merged commit 275e145a6d into main

2026-08-07 17:41:03 +02:00

sneak deleted branch fix/gosec-findings

2026-08-07 17:41:03 +02:00

sneak referenced this issue from a commit

2026-08-07 17:41:04 +02:00