Validate slugs, version tags, and Content-Disposition at all boundaries (C-1, M-5, M-8) #96

Merged

qwc merged 1 commit from fix/input-validation-boundaries into main

2026-05-18 15:51:56 +02:00

qwc commented

2026-05-18 15:47:23 +02:00

Owner

Summary

Workstream item #1 from the local audit. Three findings, all involving attacker-controlled strings flowing into filesystem paths or response headers without validation. One PR because they share the same fix (a new internal/validation package).

C-1 (Critical): r.FormValue("version") flowed directly into storage.EnsureVersionDir / storage.VersionPath. An editor uploading with version=../../<other-project>/<tag> could write extracted files outside the project's version directory, and on extraction failure storage.DeleteVersion would os.RemoveAll the traversed path — arbitrary directory write/delete.
M-5 (Medium): handleAdminCreateProject / handleAdminUpdateProject did not validate the form slug, while the API and auto-create paths did. Admin could create slugs containing /, .., whitespace, etc.
M-8 (Medium): PDF/zip download Content-Disposition was built with fmt.Sprintf and a quoted attacker-controlled tag — CR/LF or " could inject header parameters.

Changes

New internal/validation package with IsValidSlug and IsValidVersionTag (strict regexes; unit tests cover path separators, CR/LF, leading dot, length).
Moved isValidSlug out of internal/handler/auto_create.go. All three callers (API auto-create, API create-project, web upload) now use validation.IsValidSlug.
Wired validation.IsValidVersionTag at both upload entry points (web + API) before any filesystem call.
Wired validation.IsValidSlug into handleAdminCreateProject and handleAdminUpdateProject.
Replaced fmt.Sprintf Content-Disposition with mime.FormatMediaType plus a plain-ASCII fallback. The existing download test was updated to the RFC-correct unquoted form for token filenames.
New regression test file upload_validation_test.go covering malicious inputs against both the API upload and admin-create endpoints.

Test plan

go test ./... passes
Validation unit tests cover the dangerous inputs
Regression test confirms a request with version=../escape (and other malicious tags) is rejected with 400
Regression test confirms admin form rejects slugs with /, uppercase, whitespace, etc.
Manual sanity: editor uploads with a normal version tag still works
Manual sanity: download still produces a sensible Content-Disposition (attachment; filename=slug-tag.ext)

🤖 Generated with Claude Code

## Summary Workstream item #1 from the local audit. Three findings, all involving attacker-controlled strings flowing into filesystem paths or response headers without validation. One PR because they share the same fix (a new `internal/validation` package). - **C-1 (Critical)**: `r.FormValue("version")` flowed directly into `storage.EnsureVersionDir` / `storage.VersionPath`. An editor uploading with `version=../../<other-project>/<tag>` could write extracted files outside the project's version directory, and on extraction failure `storage.DeleteVersion` would `os.RemoveAll` the traversed path — arbitrary directory write/delete. - **M-5 (Medium)**: `handleAdminCreateProject` / `handleAdminUpdateProject` did not validate the form `slug`, while the API and auto-create paths did. Admin could create slugs containing `/`, `..`, whitespace, etc. - **M-8 (Medium)**: PDF/zip download Content-Disposition was built with `fmt.Sprintf` and a quoted attacker-controlled tag — CR/LF or `"` could inject header parameters. ## Changes - New `internal/validation` package with `IsValidSlug` and `IsValidVersionTag` (strict regexes; unit tests cover path separators, CR/LF, leading dot, length). - Moved `isValidSlug` out of `internal/handler/auto_create.go`. All three callers (API auto-create, API create-project, web upload) now use `validation.IsValidSlug`. - Wired `validation.IsValidVersionTag` at both upload entry points (web + API) before any filesystem call. - Wired `validation.IsValidSlug` into `handleAdminCreateProject` and `handleAdminUpdateProject`. - Replaced `fmt.Sprintf` Content-Disposition with `mime.FormatMediaType` plus a plain-ASCII fallback. The existing download test was updated to the RFC-correct unquoted form for token filenames. - New regression test file `upload_validation_test.go` covering malicious inputs against both the API upload and admin-create endpoints. ## Test plan - [x] `go test ./...` passes - [x] Validation unit tests cover the dangerous inputs - [x] Regression test confirms a request with `version=../escape` (and other malicious tags) is rejected with 400 - [x] Regression test confirms admin form rejects slugs with `/`, uppercase, whitespace, etc. - [ ] Manual sanity: editor uploads with a normal version tag still works - [ ] Manual sanity: download still produces a sensible Content-Disposition (`attachment; filename=slug-tag.ext`) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

qwc added 1 commit

2026-05-18 15:47:23 +02:00

Add internal/validation; reject path-unsafe slugs, version tags, and Content-Disposition values

CI / test (pull_request) Successful in 1m18s

Details

CI / build (pull_request) Successful in 58s

Details

CI / docker (pull_request) Has been skipped

Details

93e43accb1

Three findings from the local audit, all involving attacker-controlled
strings flowing into filesystem paths or response headers without validation:

C-1 (Critical): r.FormValue("version") flowed directly into the project's
storage filesystem path via storage.EnsureVersionDir / storage.VersionPath.
An editor uploading with version=../../<other-project>/<tag> could write
extracted files outside the project's version directory, and on extraction
failure storage.DeleteVersion would os.RemoveAll the traversed path.

M-5 (Medium): handleAdminCreateProject / handleAdminUpdateProject did not
validate the form's slug field, while the API and auto-create paths did via
isValidSlug. An admin could create projects with slugs containing "/", "..",
or whitespace, corrupting the storage tree.

M-8 (Medium): the version download path built Content-Disposition with
fmt.Sprintf and a quoted filename; the version tag was attacker-controlled
and could contain CR/LF or " to inject header parameters.

Changes:
- New internal/validation package with IsValidSlug and IsValidVersionTag.
  Strict regexes; unit tests cover the dangerous inputs (path separators,
  CR/LF, leading dot, length bounds).
- Move isValidSlug from internal/handler/auto_create.go to the new package;
  update its three callers in api.go and upload.go.
- Wire IsValidVersionTag at both upload entry points (web form + API) before
  any filesystem call.
- Wire IsValidSlug into handleAdminCreateProject and handleAdminUpdateProject.
- Replace fmt.Sprintf Content-Disposition with mime.FormatMediaType, with a
  plain-ASCII fallback. Existing test updated to the RFC-correct unquoted
  form for token filenames.
- Regression tests at internal/handler/upload_validation_test.go cover the
  malicious-input rejection on both the upload and admin-create endpoints.

Audit findings: C-1, M-5, M-8 (workstream item #1).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

qwc merged commit ba9bf514a3 into main

2026-05-18 15:51:56 +02:00

qwc deleted branch fix/input-validation-boundaries

2026-05-18 15:51:56 +02:00

qwc referenced this pull request from a commit

2026-05-18 15:51:58 +02:00

Merge pull request 'Validate slugs, version tags, and Content-Disposition at all boundaries (C-1, M-5, M-8)' (#96) from fix/input-validation-boundaries into main

qwc referenced this pull request

2026-05-18 15:55:52 +02:00

Move PDF viewer wrapper to html/template — fix stored XSS (C-2, L-6) #97

qwc referenced this pull request from a commit

2026-05-18 19:42:47 +02:00