Second lint batch toward a green make check under current
golangci-lint (issue #176).
Stacked on PR #183 (fix-noctx-lint): this branch contains that
PR's commit and should merge after it. Once #183 merges, this diff
reduces to the single G710 commit (b580dbb).
What changed
new redirectToApp helper in internal/handlers/app.go: parses
the app ID with ulid.ParseStrict (404 on failure), re-serializes
it, and applies url.PathEscape before building the /apps/<id>
redirect target — no request-derived string can reach http.Redirect unvalidated (url.PathEscape is also what gosec's
taint analysis recognizes as the sanitizer)
all 22 flagged http.Redirect sites converted to the helper,
including the multiline ones in HandleAppDeploy, HandleCancelDeploy, and HandleVolumeAdd
the equivalent unflagged redirect in HandleAppCreate converted
too, for consistency (app IDs there come from CreateApp, not the
URL)
behavior unchanged for real app IDs: IDs are generated as canonical
uppercase ULIDs (ulid.MustNew(...).String()), so
parse → re-serialize → escape is the identity on every valid ID
no //nolint directives added
TODO.md rotated: Next Step is now the G703 path traversal (#177)
make test passes (race detector on, all handler redirect tests
green), make fmt run, make fmt-check clean.
Note: while verifying I found the shared golangci-lint cache can
report stale findings from a different checkout with ../../../
path prefixes; golangci-lint cache clean before the final run
confirmed the counts above are ground truth.
Second lint batch toward a green `make check` under current
golangci-lint (issue #176).
**Stacked on PR #183** (`fix-noctx-lint`): this branch contains that
PR's commit and should merge after it. Once #183 merges, this diff
reduces to the single G710 commit (b580dbb).
## What changed
- new `redirectToApp` helper in `internal/handlers/app.go`: parses
the app ID with `ulid.ParseStrict` (404 on failure), re-serializes
it, and applies `url.PathEscape` before building the `/apps/<id>`
redirect target — no request-derived string can reach
`http.Redirect` unvalidated (`url.PathEscape` is also what gosec's
taint analysis recognizes as the sanitizer)
- all 22 flagged `http.Redirect` sites converted to the helper,
including the multiline ones in `HandleAppDeploy`,
`HandleCancelDeploy`, and `HandleVolumeAdd`
- the equivalent unflagged redirect in `HandleAppCreate` converted
too, for consistency (app IDs there come from `CreateApp`, not the
URL)
- behavior unchanged for real app IDs: IDs are generated as canonical
uppercase ULIDs (`ulid.MustNew(...).String()`), so
parse → re-serialize → escape is the identity on every valid ID
- no `//nolint` directives added
- `TODO.md` rotated: Next Step is now the G703 path traversal (#177)
## Lint counts (golangci-lint 2.12.2)
| | before | after |
| --- | --- | --- |
| `gosec` G710 | 22 | 0 |
| `gosec` G703 | 1 | 1 (tracked in #177) |
| `goconst` | 24 | 24 (tracked in #178) |
| **total** | **47** | **25** |
`make test` passes (race detector on, all handler redirect tests
green), `make fmt` run, `make fmt-check` clean.
Note: while verifying I found the shared golangci-lint cache can
report stale findings from a different checkout with `../../../`
path prefixes; `golangci-lint cache clean` before the final run
confirmed the counts above are ground truth.
Replace every httptest.NewRequest call with
httptest.NewRequestWithContext using the test's t.Context(). Thread
t *testing.T through the createSetupFormRequest and
createLoginFormRequest helpers so they can supply a context.
make lint under golangci-lint 2.12.2 drops from 94 findings to 47
(remaining: 23 gosec, 24 goconst, tracked in #176/#177/#178). make
test and make fmt-check pass unchanged.
Closes#175
Route every app redirect in internal/handlers/app.go through a new
redirectToApp helper that parses the app ID with ulid.ParseStrict
(404 on failure), re-serializes it, and path-escapes it before
building the /apps/<id> target, so no unvalidated request input can
reach http.Redirect. Also converts the equivalent unflagged redirect
in HandleAppCreate for consistency.
make lint under golangci-lint 2.12.2 drops from 47 findings to 25
(remaining: 1 gosec G703 tracked in #177, 24 goconst tracked in
#178). make test and make fmt-check pass unchanged.
Closes#176
Manager note (independent verification, not authored by this PR's implementer):
This PR is superseded by #187 (commit 7a34fc9, merged to main
2026-08-07 22:21), which fixes issue #176 with the same approach: a redirectToApp helper in internal/handlers/app.go through which
all 22 flagged http.Redirect sites now go.
I verified main's fix is a real sanitization, not a suppression:
the helper builds "/apps/" + url.PathEscape(appID) + suffix, so
the target is always a relative application URL — url.PathEscape
is one of the two remediations issue #176 explicitly accepts
no //nolint or #nosec directives were added for these sites
verified on current main in a clean worktree with the pinned
toolchain (golangci-lint v2.12.2, make targets only): make check green — tests pass, lint reports 0 issues (zero G710), fmt-check clean
A rebase of fix-gosec-g710 onto main reduces to a single cosmetic
residue: this PR additionally converted the unflagged raw redirect in HandleAppCreate (internal/handlers/app.go line 143 on main, "/apps/"+createdApp.ID — ID comes from CreateApp, not the
request, so gosec does not flag it). That consistency tweak is not
part of #176's definition of done and does not justify its own PR;
noting it here in case the owner wants it folded into future work in
that file.
Closing as superseded; issue #176's definition of done is met by main and is being closed with reference to #187. Branch fix-gosec-g710 is left on the remote untouched.
Manager note (independent verification, not authored by this PR's implementer):
This PR is superseded by #187 (commit 7a34fc9, merged to `main`
2026-08-07 22:21), which fixes issue #176 with the same approach: a
`redirectToApp` helper in `internal/handlers/app.go` through which
all 22 flagged `http.Redirect` sites now go.
I verified `main`'s fix is a real sanitization, not a suppression:
- the helper builds `"/apps/" + url.PathEscape(appID) + suffix`, so
the target is always a relative application URL — `url.PathEscape`
is one of the two remediations issue #176 explicitly accepts
- no `//nolint` or `#nosec` directives were added for these sites
- verified on current `main` in a clean worktree with the pinned
toolchain (golangci-lint v2.12.2, `make` targets only):
`make check` green — tests pass, lint reports 0 issues (zero
`G710`), `fmt-check` clean
A rebase of `fix-gosec-g710` onto `main` reduces to a single cosmetic
residue: this PR additionally converted the unflagged raw redirect in
`HandleAppCreate` (`internal/handlers/app.go` line 143 on `main`,
`"/apps/"+createdApp.ID` — ID comes from `CreateApp`, not the
request, so gosec does not flag it). That consistency tweak is not
part of #176's definition of done and does not justify its own PR;
noting it here in case the owner wants it folded into future work in
that file.
Closing as superseded; issue #176's definition of done is met by
`main` and is being closed with reference to #187. Branch
`fix-gosec-g710` is left on the remote untouched.
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.
Second lint batch toward a green
make checkunder currentgolangci-lint (issue #176).
Stacked on PR #183 (
fix-noctx-lint): this branch contains thatPR's commit and should merge after it. Once #183 merges, this diff
reduces to the single G710 commit (
b580dbb).What changed
redirectToApphelper ininternal/handlers/app.go: parsesthe app ID with
ulid.ParseStrict(404 on failure), re-serializesit, and applies
url.PathEscapebefore building the/apps/<id>redirect target — no request-derived string can reach
http.Redirectunvalidated (url.PathEscapeis also what gosec'staint analysis recognizes as the sanitizer)
http.Redirectsites converted to the helper,including the multiline ones in
HandleAppDeploy,HandleCancelDeploy, andHandleVolumeAddHandleAppCreateconvertedtoo, for consistency (app IDs there come from
CreateApp, not theURL)
uppercase ULIDs (
ulid.MustNew(...).String()), soparse → re-serialize → escape is the identity on every valid ID
//nolintdirectives addedTODO.mdrotated: Next Step is now the G703 path traversal (#177)Lint counts (golangci-lint 2.12.2)
gosecG710gosecG703goconstmake testpasses (race detector on, all handler redirect testsgreen),
make fmtrun,make fmt-checkclean.Note: while verifying I found the shared golangci-lint cache can
report stale findings from a different checkout with
../../../path prefixes;
golangci-lint cache cleanbefore the final runconfirmed the counts above are ground truth.
@clawbot please fix this one too
Manager note (independent verification, not authored by this PR's implementer):
This PR is superseded by #187 (commit
7a34fc9, merged tomain2026-08-07 22:21), which fixes issue #176 with the same approach: a
redirectToApphelper ininternal/handlers/app.gothrough whichall 22 flagged
http.Redirectsites now go.I verified
main's fix is a real sanitization, not a suppression:"/apps/" + url.PathEscape(appID) + suffix, sothe target is always a relative application URL —
url.PathEscapeis one of the two remediations issue #176 explicitly accepts
//nolintor#nosecdirectives were added for these sitesmainin a clean worktree with the pinnedtoolchain (golangci-lint v2.12.2,
maketargets only):make checkgreen — tests pass, lint reports 0 issues (zeroG710),fmt-checkcleanA rebase of
fix-gosec-g710ontomainreduces to a single cosmeticresidue: this PR additionally converted the unflagged raw redirect in
HandleAppCreate(internal/handlers/app.goline 143 onmain,"/apps/"+createdApp.ID— ID comes fromCreateApp, not therequest, so gosec does not flag it). That consistency tweak is not
part of #176's definition of done and does not justify its own PR;
noting it here in case the owner wants it folded into future work in
that file.
Closing as superseded; issue #176's definition of done is met by
mainand is being closed with reference to #187. Branchfix-gosec-g710is left on the remote untouched.clawbot referenced this pull request2026-09-03 18:29:27 +02:00
Pull request closed