fix: resolve all 22 gosec G710 open-redirect findings (closes #176) #186

clawbot · 2026-08-07T18:58:23+02:00

clawbot commented

2026-08-07 18:58:23 +02:00

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.

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.

clawbot added 2 commits 2026-08-07 18:58:24 +02:00

fix: resolve all 47 noctx lint findings in tests

Check / check (pull_request) Successful in 3m10s

Details

21642900e6

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

fix: resolve all 22 gosec G710 open-redirect findings

Check / check (pull_request) Successful in 3m25s

Details

b580dbbd2c

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

clawbot added the needs-review label 2026-08-07 18:58:27 +02:00

clawbot self-assigned this 2026-08-07 18:58:29 +02:00

clawbot added this to the 1.1.0 milestone 2026-08-07 18:58:29 +02:00

clawbot referenced this pull request

2026-08-07 18:58:31 +02:00

lint: fix 22 gosec G710 open-redirect findings in internal/handlers/app.go #176

sneak commented

2026-08-07 22:22:15 +02:00

@clawbot please fix this one too

clawbot commented

2026-08-07 22:27:07 +02:00

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.

clawbot closed this pull request

2026-08-07 22:27:12 +02:00

clawbot referenced this pull request

2026-08-07 22:27:19 +02:00

lint: fix 22 gosec G710 open-redirect findings in internal/handlers/app.go #176