Fix remaining gosec lint findings on main #47

New Issue

clawbot · 2026-08-04T02:28:06+02:00

clawbot commented

2026-08-04 02:28:06 +02:00

main carries a set of open gosec findings (≈10 as of the 2026-07-07 survey) that keep make check red. Clear them.

Definition of done

make check (golangci-lint) reports 0 gosec findings.
Every fix is behavior-preserving. Where a finding is a genuine false positive, use a narrowly-scoped //nolint:gosec // G### <reason> with a real justification rather than a blanket disable.
Existing tests remain green; no functional behavior change.
Scope guard: if the actual finding count is much larger than expected (more than ~30) or any fix would require a real behavior change, STOP and report on the issue instead of forcing it — so it can be split.

The finishing commit's title must end with (closes #N) referencing this issue.

`main` carries a set of open `gosec` findings (≈10 as of the 2026-07-07 survey) that keep `make check` red. Clear them. ## Definition of done 1. `make check` (golangci-lint) reports **0** `gosec` findings. 2. Every fix is behavior-preserving. Where a finding is a genuine false positive, use a narrowly-scoped `//nolint:gosec // G### <reason>` with a real justification rather than a blanket disable. 3. Existing tests remain green; no functional behavior change. 4. Scope guard: if the actual finding count is much larger than expected (more than ~30) or any fix would require a real behavior change, STOP and report on the issue instead of forcing it — so it can be split. The finishing commit's title must end with ` (closes #N)` referencing this issue.

clawbot commented

2026-08-07 15:39:20 +02:00

Dispatching this now, per the definition of done in the issue body. Additional direction:

Branch fix/gosec-findings from current main.
First, run make check and enumerate the actual gosec findings in a comment on the PR (count, rule IDs, locations) before fixing — this documents the scope-guard check from the issue body.
Fix each finding individually; a //nolint:gosec // G### <reason> is acceptable only for genuine false positives with a real justification. No blanket suppressions, no linter config changes.
One logical change per commit; make check green before each commit.
Open a PR against main whose body contains closes #47, and end the finishing commit's title with (closes #47).
Note: PR #46 (refactor/extract-signature) is in flight and touches internal/imgcache/internal/signature. Base on main as it is; if a finding is in code being moved by #46, fix it in main's current layout anyway — any conflict gets resolved at merge time.
Progress notes and questions go in comments on the PR once it exists.

Dispatching this now, per the definition of done in the issue body. Additional direction: - Branch `fix/gosec-findings` from current `main`. - First, run `make check` and enumerate the actual gosec findings in a comment on the PR (count, rule IDs, locations) before fixing — this documents the scope-guard check from the issue body. - Fix each finding individually; a `//nolint:gosec // G### <reason>` is acceptable only for genuine false positives with a real justification. No blanket suppressions, no linter config changes. - One logical change per commit; `make check` green before each commit. - Open a PR against `main` whose body contains `closes #47`, and end the finishing commit's title with ` (closes #47)`. - Note: PR #46 (`refactor/extract-signature`) is in flight and touches `internal/imgcache`/`internal/signature`. Base on `main` as it is; if a finding is in code being moved by #46, fix it in `main`'s current layout anyway — any conflict gets resolved at merge time. - Progress notes and questions go in comments on the PR once it exists.

clawbot commented

2026-08-07 15:40:09 +02:00

Scope update from a fresh make check run (golangci-lint 2.12.2): the "≈10 findings" figure in the issue body is stale. Current state is exactly 2 gosec findings, both G124 (http.Cookie missing or insecure Secure/HttpOnly/SameSite attributes):

internal/session/session.go:84
internal/session/session.go:128

All tests pass; these two findings are the only thing keeping make check red. Well within the scope guard.

Additional direction for the fix:

This is a security bug in production code (session cookies without HttpOnly/Secure/SameSite), so per repo rules: write a failing test FIRST asserting the cookies carry the correct attributes, commit it, then fix until green — do not //nolint these; they are genuine findings, and the fix is to set the attributes properly.
SameSite should be Lax or stricter; HttpOnly true; Secure true. If Secure: true would break any documented plain-HTTP dev flow, note the reasoning in the PR body rather than weakening the cookie.
Update TODO.md in the finishing commit per its Workflow section (move Next Step to Completed Steps, promote the next Future Step, and correct the stale "10 findings" status text).

Scope update from a fresh `make check` run (golangci-lint 2.12.2): the "≈10 findings" figure in the issue body is stale. Current state is exactly **2** gosec findings, both `G124` (http.Cookie missing or insecure `Secure`/`HttpOnly`/`SameSite` attributes): - `internal/session/session.go:84` - `internal/session/session.go:128` All tests pass; these two findings are the only thing keeping `make check` red. Well within the scope guard. Additional direction for the fix: - This is a security bug in production code (session cookies without `HttpOnly`/`Secure`/`SameSite`), so per repo rules: write a failing test FIRST asserting the cookies carry the correct attributes, commit it, then fix until green — do not `//nolint` these; they are genuine findings, and the fix is to set the attributes properly. - `SameSite` should be `Lax` or stricter; `HttpOnly` true; `Secure` true. If `Secure: true` would break any documented plain-HTTP dev flow, note the reasoning in the PR body rather than weakening the cookie. - Update `TODO.md` in the finishing commit per its Workflow section (move Next Step to Completed Steps, promote the next Future Step, and correct the stale "10 findings" status text).

sneak referenced this issue from a commit

2026-08-07 16:14:22 +02:00

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

clawbot referenced a pull request that will close this issue

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

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

clawbot referenced this issue

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

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

clawbot commented

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

Review of PR #48 came back FAIL with one blocking item (labelled needs-rework): the ignored former secure-toggle parameter on session.NewManager must be removed in this PR rather than deferred — pre-1.0, everything updates in one pass. Full rework direction is in the review comment on the PR: #48 (comment). The mechanical call-site updates in internal/session/session_test.go (call shape only, assertions unchanged) are reviewer-directed; the accompanying TODO.md P1 deferral entry gets dropped in the same commit.

Review of PR #48 came back FAIL with one blocking item (labelled `needs-rework`): the ignored former secure-toggle parameter on `session.NewManager` must be removed in this PR rather than deferred — pre-1.0, everything updates in one pass. Full rework direction is in the review comment on the PR: https://git.eeqj.de/sneak/pixa/pulls/48#issuecomment-43557. The mechanical call-site updates in `internal/session/session_test.go` (call shape only, assertions unchanged) are reviewer-directed; the accompanying TODO.md P1 deferral entry gets dropped in the same commit.

clawbot referenced a pull request that will close this issue

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

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

sneak closed this issue

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

sneak referenced this issue from a commit

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

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

Sign in to join this conversation.