sneak/pixa
1
0
Fork 0
Code Issues 51 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Fix remaining gosec lint findings on main #47

New Issue
Closed
opened 2026-08-04 02:28:06 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-04 02:28:06 +02:00
Collaborator
Copy Link
Copy Source

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.

`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
Author
Collaborator
Copy Link
Copy Source

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
Author
Collaborator
Copy Link
Copy Source

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).
clawbot commented 2026-08-07 16:55:00 +02:00
Author
Collaborator
Copy Link
Copy Source

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.
sneak closed this issue 2026-08-07 17:41:03 +02:00
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/pixa#47