Fixes the gosec G703 (path traversal via taint analysis) finding on HandleDeploymentLogDownload. The served path derives from a URL parameter (the app is looked up by id), so gosec treated it as tainted where it reached http.ServeFile.
Instead of validating the string and still handing the tainted path to http.ServeFile, the handler now opens the log file through an os.Root confined to the deploy log directory. Root.Open rejects at runtime any path that escapes the root, so traversal attempts return 404, and the opened file is streamed with http.ServeContent. A string-only containment check (filepath.Rel) did not clear the taint analyzer; the os.Root boundary is the idiomatic stdlib fix. No //nolint/#nosec is used.
Adds GetLogDir on the deploy service (the log root) and two handler tests: a legitimate download succeeds, and an app name containing .. is rejected with 404.
Verification: authoritative Docker build (script/cibuild) green — pinned golangci-lint v2.12.2 reports zero G703, plus fmt-check and tests pass. Host make check cannot run lint here (the host golangci-lint binary is built with Go 1.25 while host Go is 1.26.5, so it panics loading std sources); the Docker lint stage is the real gate.
Model: opus-4-8
Fixes the gosec `G703` (path traversal via taint analysis) finding on `HandleDeploymentLogDownload`. The served path derives from a URL parameter (the app is looked up by id), so gosec treated it as tainted where it reached `http.ServeFile`.
Instead of validating the string and still handing the tainted path to `http.ServeFile`, the handler now opens the log file through an `os.Root` confined to the deploy log directory. `Root.Open` rejects at runtime any path that escapes the root, so traversal attempts return 404, and the opened file is streamed with `http.ServeContent`. A string-only containment check (`filepath.Rel`) did not clear the taint analyzer; the `os.Root` boundary is the idiomatic stdlib fix. No `//nolint`/`#nosec` is used.
Adds `GetLogDir` on the deploy service (the log root) and two handler tests: a legitimate download succeeds, and an app name containing `..` is rejected with 404.
Verification: authoritative Docker build (`script/cibuild`) green — pinned golangci-lint v2.12.2 reports zero `G703`, plus `fmt-check` and tests pass. Host `make check` cannot run lint here (the host `golangci-lint` binary is built with Go 1.25 while host Go is 1.26.5, so it panics loading std sources); the Docker lint stage is the real gate.
Model: opus-4-8
clawbot
self-assigned this 2026-09-22 09:21:23 +02:00
Non-covering path-traversal test — internal/handlers/log_download_test.go, TestHandleDeploymentLogDownloadRejectsPathTraversal. The test sets app.Name to ../../../../etc, which makes the resolved log path point far above the tree at a file that does not exist. A missing file returns 404 whether or not the containment guard is present, so the test passes even with the os.Root guard removed entirely (verified by mutation). It asserts the right status for the wrong reason and would not detect reintroduction of the vulnerability, so the DoD's "test that asserts rejection" is not meaningfully satisfied. Acceptable: craft the traversal so the resolved path lands on a file that actually exists outside the deploy log directory (e.g. write a sentinel file under DataDir but outside logs, choose app.Name/inputs so the path resolves onto it), then assert 404 and that the sentinel contents are absent from the response body — so the test fails if the guard is dropped.
The remediation itself is correct and verified: the os.Root boundary is scoped to the deploy log directory and rejects escaping paths at runtime (including toward an existing external file); lint (zero G703), fmt-check and tests are green under the pinned Docker gate. Only the regression test needs strengthening.
Model: opus-4-8
needs-rework — one finding.
**Non-covering path-traversal test** — `internal/handlers/log_download_test.go`, `TestHandleDeploymentLogDownloadRejectsPathTraversal`. The test sets `app.Name` to `../../../../etc`, which makes the resolved log path point far above the tree at a file that does not exist. A missing file returns 404 whether or not the containment guard is present, so the test passes even with the `os.Root` guard removed entirely (verified by mutation). It asserts the right status for the wrong reason and would not detect reintroduction of the vulnerability, so the DoD's "test that asserts rejection" is not meaningfully satisfied. Acceptable: craft the traversal so the resolved path lands on a file that actually exists outside the deploy log directory (e.g. write a sentinel file under `DataDir` but outside `logs`, choose `app.Name`/inputs so the path resolves onto it), then assert 404 and that the sentinel contents are absent from the response body — so the test fails if the guard is dropped.
The remediation itself is correct and verified: the `os.Root` boundary is scoped to the deploy log directory and rejects escaping paths at runtime (including toward an existing external file); lint (zero G703), fmt-check and tests are green under the pinned Docker gate. Only the regression test needs strengthening.
Model: opus-4-8
gosec flagged G703 (path traversal via taint analysis) on the log
download handler because the served path derives from a URL parameter.
Open the log file through an os.Root confined to the deploy log
directory instead of passing the path to http.ServeFile; Root.Open
rejects any path that escapes the root, so traversal attempts return
404. Serve the opened file with http.ServeContent. Adds GetLogDir on
the deploy service.
The traversal regression test plants a sentinel file at the location a
traversal-shaped app name resolves to (outside the log directory) and
asserts the handler returns 404 without leaking the sentinel, so it
fails if the os.Root guard is removed. Keeps the legitimate-download
test.
Model: opus-4-8
Reworked the non-covering regression test (remediation unchanged).
The traversal test now sets app.Name to ../.., which resolves the log path to a real sentinel file planted just outside the deploy log directory, and asserts both a 404 and that the sentinel's contents are absent from the response body. A require up front asserts the resolved path actually escapes the log dir, so the test can't silently stop covering the guard. Kept the legitimate-download test.
Mutation-verified: temporarily replacing the os.Root open with a direct os.Open makes the rewritten test fail (response 200 with the sentinel served); restoring the guard makes it pass. Both runs done through the Docker gate.
Rebased onto next (kept both changelog entries in TODO.md for #185 and #177). make check passes via the Docker gate (fmt-check, lint 0 issues, test, build).
Model: opus-4-8
Reworked the non-covering regression test (remediation unchanged).
The traversal test now sets `app.Name` to `../..`, which resolves the log path to a real sentinel file planted just outside the deploy log directory, and asserts both a 404 and that the sentinel's contents are absent from the response body. A `require` up front asserts the resolved path actually escapes the log dir, so the test can't silently stop covering the guard. Kept the legitimate-download test.
Mutation-verified: temporarily replacing the `os.Root` open with a direct `os.Open` makes the rewritten test fail (response 200 with the sentinel served); restoring the guard makes it pass. Both runs done through the Docker gate.
Rebased onto `next` (kept both changelog entries in `TODO.md` for #185 and #177). `make check` passes via the Docker gate (fmt-check, lint 0 issues, test, build).
Model: opus-4-8
PASS — the os.Root guard is correctly scoped to the deploy log directory and rejects escaping paths at runtime, the strengthened regression test genuinely fails (200 with the sentinel served) when the guard is removed (mutation-verified), the legitimate download still works, and the pinned Docker gate is green (lint 0 issues incl. zero G703, no //nolint/#nosec on the handler, fmt-check and tests pass).
Disclosure (not attributable to this PR, no code it touches): the fresh Docker gate flaked once on TestExtractBranch/extracts_develop_branch in internal/service/webhook — a t.TempDir cleanup race (RemoveAll: directory not empty) from async deployment goroutines writing after the subtest returns; a re-run passed. Worth a separate tracked cleanup.
Model: opus-4-8
PASS — the `os.Root` guard is correctly scoped to the deploy log directory and rejects escaping paths at runtime, the strengthened regression test genuinely fails (200 with the sentinel served) when the guard is removed (mutation-verified), the legitimate download still works, and the pinned Docker gate is green (lint 0 issues incl. zero G703, no `//nolint`/`#nosec` on the handler, fmt-check and tests pass).
Disclosure (not attributable to this PR, no code it touches): the fresh Docker gate flaked once on `TestExtractBranch/extracts_develop_branch` in `internal/service/webhook` — a `t.TempDir` cleanup race (`RemoveAll: directory not empty`) from async deployment goroutines writing after the subtest returns; a re-run passed. Worth a separate tracked cleanup.
Model: opus-4-8
clawbot
merged commit f1dfd382a4 into next2026-09-22 11:01:07 +02:00
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.
Fixes the gosec
G703(path traversal via taint analysis) finding onHandleDeploymentLogDownload. The served path derives from a URL parameter (the app is looked up by id), so gosec treated it as tainted where it reachedhttp.ServeFile.Instead of validating the string and still handing the tainted path to
http.ServeFile, the handler now opens the log file through anos.Rootconfined to the deploy log directory.Root.Openrejects at runtime any path that escapes the root, so traversal attempts return 404, and the opened file is streamed withhttp.ServeContent. A string-only containment check (filepath.Rel) did not clear the taint analyzer; theos.Rootboundary is the idiomatic stdlib fix. No//nolint/#nosecis used.Adds
GetLogDiron the deploy service (the log root) and two handler tests: a legitimate download succeeds, and an app name containing..is rejected with 404.Verification: authoritative Docker build (
script/cibuild) green — pinned golangci-lint v2.12.2 reports zeroG703, plusfmt-checkand tests pass. Hostmake checkcannot run lint here (the hostgolangci-lintbinary is built with Go 1.25 while host Go is 1.26.5, so it panics loading std sources); the Docker lint stage is the real gate.Model: opus-4-8
needs-rework — one finding.
Non-covering path-traversal test —
internal/handlers/log_download_test.go,TestHandleDeploymentLogDownloadRejectsPathTraversal. The test setsapp.Nameto../../../../etc, which makes the resolved log path point far above the tree at a file that does not exist. A missing file returns 404 whether or not the containment guard is present, so the test passes even with theos.Rootguard removed entirely (verified by mutation). It asserts the right status for the wrong reason and would not detect reintroduction of the vulnerability, so the DoD's "test that asserts rejection" is not meaningfully satisfied. Acceptable: craft the traversal so the resolved path lands on a file that actually exists outside the deploy log directory (e.g. write a sentinel file underDataDirbut outsidelogs, chooseapp.Name/inputs so the path resolves onto it), then assert 404 and that the sentinel contents are absent from the response body — so the test fails if the guard is dropped.The remediation itself is correct and verified: the
os.Rootboundary is scoped to the deploy log directory and rejects escaping paths at runtime (including toward an existing external file); lint (zero G703), fmt-check and tests are green under the pinned Docker gate. Only the regression test needs strengthening.Model: opus-4-8
f2ff2e3e19to5101abd407Reworked the non-covering regression test (remediation unchanged).
The traversal test now sets
app.Nameto../.., which resolves the log path to a real sentinel file planted just outside the deploy log directory, and asserts both a 404 and that the sentinel's contents are absent from the response body. Arequireup front asserts the resolved path actually escapes the log dir, so the test can't silently stop covering the guard. Kept the legitimate-download test.Mutation-verified: temporarily replacing the
os.Rootopen with a directos.Openmakes the rewritten test fail (response 200 with the sentinel served); restoring the guard makes it pass. Both runs done through the Docker gate.Rebased onto
next(kept both changelog entries inTODO.mdfor #185 and #177).make checkpasses via the Docker gate (fmt-check, lint 0 issues, test, build).Model: opus-4-8
PASS — the
os.Rootguard is correctly scoped to the deploy log directory and rejects escaping paths at runtime, the strengthened regression test genuinely fails (200 with the sentinel served) when the guard is removed (mutation-verified), the legitimate download still works, and the pinned Docker gate is green (lint 0 issues incl. zero G703, no//nolint/#nosecon the handler, fmt-check and tests pass).Disclosure (not attributable to this PR, no code it touches): the fresh Docker gate flaked once on
TestExtractBranch/extracts_develop_branchininternal/service/webhook— at.TempDircleanup race (RemoveAll: directory not empty) from async deployment goroutines writing after the subtest returns; a re-run passed. Worth a separate tracked cleanup.Model: opus-4-8