sneak/upaas
1
0
Fork 1
Code Issues 4 Pull Requests 1 Actions Packages Projects Releases 1 Wiki Activity

lint: fix gosec G703 path traversal finding in deploy log download handler #177

New Issue
Closed
opened 2026-08-07 18:40:39 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-07 18:40:39 +02:00
Collaborator
Copy Link
Copy Source

Surveyed 2026-08-07: golangci-lint 2.12.2 reports one
G703: Path traversal via taint analysis finding at
internal/handlers/app.go:629 — http.ServeFile called with a
logPath derived from request data:

filename := filepath.Base(logPath)
writer.Header().Set("Content-Type", "text/plain; charset=utf-8")
writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"")
http.ServeFile(writer, request, logPath)

Remediation: before serving, canonicalize the path
(filepath.Clean, resolving against the configured data directory)
and verify containment — the resolved path must be inside the deploy
log directory (e.g. filepath.Rel result must not start with ..).
Return 404 on violation. Do not use //nolint.

Definition of done:

  • make lint (golangci-lint ≥ 2.12) reports zero G703 findings
  • a unit test exercises the handler with a traversal-shaped input
    (e.g. an id containing ..) and asserts it is rejected
  • legitimate log download still works (make test passes)
  • make fmt run before commit; TODO.md updated per the repo workflow
  • lands via PR from a feature branch off main
Surveyed 2026-08-07: golangci-lint 2.12.2 reports one `G703: Path traversal via taint analysis` finding at `internal/handlers/app.go:629` — `http.ServeFile` called with a `logPath` derived from request data: ```go filename := filepath.Base(logPath) writer.Header().Set("Content-Type", "text/plain; charset=utf-8") writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"") http.ServeFile(writer, request, logPath) ``` Remediation: before serving, canonicalize the path (`filepath.Clean`, resolving against the configured data directory) and verify containment — the resolved path must be inside the deploy log directory (e.g. `filepath.Rel` result must not start with `..`). Return 404 on violation. Do not use `//nolint`. Definition of done: - `make lint` (golangci-lint ≥ 2.12) reports zero `G703` findings - a unit test exercises the handler with a traversal-shaped input (e.g. an id containing `..`) and asserts it is rejected - legitimate log download still works (`make test` passes) - `make fmt` run before commit; `TODO.md` updated per the repo workflow - lands via PR from a feature branch off `main`
clawbot added this to the 1.1.0 milestone 2026-08-07 18:40:39 +02:00
clawbot commented 2026-09-22 09:21:37 +02:00
Author
Collaborator
Copy Link
Copy Source

Fixed. The log download handler now opens the log file through an os.Root confined to the deploy log directory (Root.Open rejects any path that escapes the root) instead of passing the request-derived path to http.ServeFile; the opened file is streamed with http.ServeContent, and traversal attempts return 404. A string-only filepath.Rel containment check did not clear the gosec taint analyzer, so the os.Root boundary is used; no //nolint or #nosec. Added GetLogDir on the deploy service and two handler tests (legitimate download succeeds; an app name containing .. is rejected).

PR: #194

Note: the DoD said branch off main; per the repo branch workflow the PR targets next instead.

Model: opus-4-8

Fixed. The log download handler now opens the log file through an `os.Root` confined to the deploy log directory (`Root.Open` rejects any path that escapes the root) instead of passing the request-derived path to `http.ServeFile`; the opened file is streamed with `http.ServeContent`, and traversal attempts return 404. A string-only `filepath.Rel` containment check did not clear the gosec taint analyzer, so the `os.Root` boundary is used; no `//nolint` or `#nosec`. Added `GetLogDir` on the deploy service and two handler tests (legitimate download succeeds; an app name containing `..` is rejected). PR: https://git.eeqj.de/sneak/upaas/pulls/194 Note: the DoD said branch off `main`; per the repo branch workflow the PR targets `next` instead. Model: opus-4-8
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
 notplanned
question
More information is needed
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone 1.1.0
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/upaas#177