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).
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).
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).
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).
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.
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).
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.
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
closes #47
Fixes the two remaining
gosecfindings onmain, bothG124(http.Cookie missing or has insecure
Secure,HttpOnly, orSameSiteattribute):internal/session/session.go:84(CreateSession, the loginset-cookie path)
internal/session/session.go:128(ClearSession, the logoutdelete-cookie path)
What changed
Secure: true,HttpOnly: true, andSameSite: http.SameSiteStrictMode.securefield (previously wired to!config.Debug) and thesameSitefield are removed fromsession.Manager, and the deadsecure-toggle parameter is removed from
session.NewManager, whichnow takes only the signing key (reviewer-directed; the mechanical
call-shape updates in
session_test.goleave every assertionuntouched).
TestSessionCookieAttributesAlwaysSecure(failing), asserting thatevery cookie emitted by the session manager carries
HttpOnly,Secure, andSameSiteof Lax or stricter, for both write paths.The second commit makes it pass.
TODO.mdupdated per its Workflow section (Next Step completed,next Future Step promoted, stale "10 open findings" Status text
corrected).
Attribute choices and reasoning
Secure: truealways: theG124analyzer only accepts a constanttruestore, and there is no legitimate configuration in which theauthentication cookie should be sent over plaintext HTTP. The old
behavior disabled
Securewheneverdebugwas on. Local developmentover
http://localhostkeeps working: browsers treatlocalhostasa trustworthy origin and accept
Securecookies there. Anyplain-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, andstricter than the Lax minimum): the login form is a same-origin POST
to
/followed by a same-site redirect, soStrictbreaks nothing.HttpOnly: true(unchanged).Verification
make check(tests, golangci-lint, fmt-check) is fully green on thebranch 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 deadNewManagerparameter).Scope-guard enumeration (fresh
make checkonmainatb6e9ac2before any changes, golangci-lint viascript/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:SecureandSameSitewere non-constant fields, withSecurewired 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//nolintsuppressions and no linter config changes.make checkis fully green on the branch head (02ca16a).Review: FAIL — one blocking item
Independently verified at head
02ca16ain a fresh worktree:What passes:
make checkfully 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 cleanresolved it. Nothing in this PR was at fault.CreateSession,ClearSession) now carry constantSecure: true,HttpOnly: true,SameSite: Strict— constants are exactly what theG124analyzer requires, so no//nolintwas needed and none was used.SameSite: Strictpreserves the pre-existing production value; the only behavior change isSecureno longer being disabled in debug mode, which is the point of the fix, andhttp://localhostdevelopment is unaffected since browsers treat localhost as a trustworthy origin.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.mdupdated 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 inTODO.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:NewManager(signingKey string).internal/handlers/handlers.gocall site.NewManagercall sites ininternal/session/session_test.goand 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.)TODO.mdand the "retained only so call sites keep compiling" language from the godoc.constructorBoolArgdimension in the process — collapsing that loop is expected, its assertions stay.make checkgreen, push, and summarize here for re-review.Rework for the blocking item is pushed:
cb9e14e.session.NewManagernow takes only the signing key:NewManager(signingKey string). The ignored_ boolparameter and the "retained only so call sites keep compiling" godoc language are gone.internal/handlers/handlers.gocall site updated, along with its stale comment about the ignored argument.NewManagercall sites ininternal/session/session_test.goupdated — call shape only, per the review's direction; no assertion was changed (one stale// Test with secure=truecomment removed).constructorBoolArgloop insession_cookie_attributes_test.gocollapsed as anticipated in the review; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, both write paths, with diagnostic logging) are identical.TODO.md(make fmtrun).make checkis fully green atcb9e14e(all tests pass, golangci-lint 0 issues after agolangci-lint cache cleanto flush the stale-worktree-path cache issue noted in the review, fmt-check clean). Cookie behavior is unchanged from02ca16a. Ready for re-review.Re-review: PASS
Verified at head
cb9e14ein a fresh worktree with an isolated lint cache:make checkfully 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.gocall site and its stale "second argument is ignored" comment fixed.session_test.gocall 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.constructorBoolArgloop as anticipated; its assertions (HttpOnly, Secure, SameSite Lax-or-stricter, bothCreateSessionandClearSessionpaths) are identical, and diagnostic logging is retained.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: constantSecure/HttpOnly/SameSite=Stricton both write paths, which is what closes the twoG124findings and makesmake checkgreen onmainonce merged.Marking
merge-readyand assigning sneak for merge into protectedmain. Merging this closes #47.