Fix #54: add HTTP server timeouts #60

.golangci.yaml collides with the .golangci.yml already on master.
Dead euid vars will be tripped by the new unused linter.
fmt.Errorf in templates.go uses %v instead of %w and has a trailing \n.
Conditional templates.Init changes startup semantics.
Upload→upload rename leaves the handler-naming convention inconsistent.
logout.go comments-out instead of deleting.

Pre-existing — out of scope for this PR but worth a follow-up issue

TLS-bypass via spoofed X-Forwarded-Proto in serve.go:checkTLS when TrustXForwarded=true and the server is reachable directly. Trust should be tied to a CIDR/peer check, not a config flag.
No http.MaxBytesReader around uploads (upload.go): ParseMultipartForm(2<<20) only bounds the in-memory portion; the request body itself is unbounded. Same neighbourhood as #54.
FD leak in the zip multi-upload path (upload.go): f.Open() returns a multipart.File (io.ReadCloser) but the loop assigns to a local io.Reader and never closes it.
FindFile(key) filepath.Joins an arbitrary key into the store dir — path-traversal surface; the download handler strips a URL prefix and passes the rest as key.
Zip-slip in archive uploads (upload.go): filepath.Join(basename, f.Filename) with attacker-controlled f.Filename. Stored, not extracted server-side, so impact is on whoever unzips it later.
MaxHeaderBytes not explicitly set in buildHTTPServer() — defense-in-depth nit.
Symlink / TOCTOU on the new store/templates pre-flight Stats (serve.go, templates.go): cosmetic at init time, but worth filepath.EvalSymlinks if the paths are user-supplied.

Verdict

Nothing critical introduced. The lint-config collision and dead euid vars are the only real blockers (CI will fail once the new linters run). After fixing those two, the rest is style/cleanup.

review (co)authored by ai:gpt-5.1

Security fan-out review (4 parallel agents). ### In-scope (introduced by this PR) — see inline comments - `.golangci.yaml` collides with the `.golangci.yml` already on master. - Dead `euid` vars will be tripped by the new `unused` linter. - `fmt.Errorf` in `templates.go` uses `%v` instead of `%w` and has a trailing `\n`. - Conditional `templates.Init` changes startup semantics. - `Upload`→`upload` rename leaves the handler-naming convention inconsistent. - `logout.go` comments-out instead of deleting. ### Pre-existing — out of scope for this PR but worth a follow-up issue - **TLS-bypass via spoofed `X-Forwarded-Proto`** in `serve.go:checkTLS` when `TrustXForwarded=true` and the server is reachable directly. Trust should be tied to a CIDR/peer check, not a config flag. - **No `http.MaxBytesReader` around uploads** (`upload.go`): `ParseMultipartForm(2<<20)` only bounds the in-memory portion; the request body itself is unbounded. Same neighbourhood as #54. - **FD leak in the zip multi-upload path** (`upload.go`): `f.Open()` returns a `multipart.File` (`io.ReadCloser`) but the loop assigns to a local `io.Reader` and never closes it. - **`FindFile(key)` `filepath.Join`s an arbitrary key into the store dir** — path-traversal surface; the download handler strips a URL prefix and passes the rest as `key`. - **Zip-slip in archive uploads** (`upload.go`): `filepath.Join(basename, f.Filename)` with attacker-controlled `f.Filename`. Stored, not extracted server-side, so impact is on whoever unzips it later. - **`MaxHeaderBytes` not explicitly set** in `buildHTTPServer()` — defense-in-depth nit. - **Symlink / TOCTOU** on the new store/templates pre-flight `Stat`s (`serve.go`, `templates.go`): cosmetic at init time, but worth `filepath.EvalSymlinks` if the paths are user-supplied. ### Verdict Nothing critical introduced. The lint-config collision and dead `euid` vars are the only real blockers (CI will fail once the new linters run). After fixing those two, the rest is style/cleanup. *review (co)authored by ai:gpt-5.1*

.golangci.yaml Outdated

					
				@ -0,0 +1,6 @@

				linters:

[med] Collides with the existing .golangci.yml (already on master, golangci-lint v2 schema with version: "2"). After merge both files exist with conflicting settings — wsl here vs. wsl_v5 there, plus gosec/unused only in this one and no version field. Pick one config and delete the other.

**[med]** Collides with the existing `.golangci.yml` (already on master, golangci-lint v2 schema with `version: "2"`). After merge both files exist with conflicting settings — `wsl` here vs. `wsl_v5` there, plus `gosec`/`unused` only in this one and no `version` field. Pick one config and delete the other.

heiko marked this conversation as resolved

v2/cmd/once-server/euid_linux.go Outdated

					
				@ -0,0 +4,4 @@

				import "golang.org/x/sys/unix"

				var euid = unix.Geteuid()

[med] euid is dead code: main.go:88 calls unix.Geteuid() directly. With the unused linter just enabled by this same PR, this will fail CI. Either drop both euid_*.go files or wire euid into the root check in main.go.

**[med]** `euid` is dead code: `main.go:88` calls `unix.Geteuid()` directly. With the `unused` linter just enabled by this same PR, this will fail CI. Either drop both `euid_*.go` files or wire `euid` into the root check in `main.go`.

heiko marked this conversation as resolved

v2/cmd/once-server/logout.go Outdated

					
				@ -6,3 +6,1 @@

				import "net/http"

				func Logout(w http.ResponseWriter, r *http.Request) {

				/* not in use currently

[nit] Commenting out a function leaves dead source. Either delete the file or guard it behind //go:build never so the compiler still type-checks it without linking.

**[nit]** Commenting out a function leaves dead source. Either delete the file or guard it behind `//go:build never` so the compiler still type-checks it without linking.

heiko marked this conversation as resolved

v2/cmd/once-server/serve.go Outdated

					
				@ -118,1 +122,4 @@

					go func() {

						defer wg.Done()

						if cf.Global.TemplatesDir != "" {

[low] Wrapping templates.Init in if cf.Global.TemplatesDir != "" changes behaviour: previously Init("") was always called and parsed the embedded html/*.html set eagerly; now tt stays nil until the first request and ExecuteTemplate does it in-band on a hot path. If that's intentional, fine — but the pre-flight goroutine becomes a no-op for the default config, which defeats the framing. Suggest calling templates.Init(cf.Global.TemplatesDir) unconditionally and letting Init decide.

**[low]** Wrapping `templates.Init` in `if cf.Global.TemplatesDir != ""` changes behaviour: previously `Init("")` was always called and parsed the embedded `html/*.html` set eagerly; now `tt` stays nil until the first request and `ExecuteTemplate` does it in-band on a hot path. If that's intentional, fine — but the pre-flight goroutine becomes a no-op for the default config, which defeats the framing. Suggest calling `templates.Init(cf.Global.TemplatesDir)` unconditionally and letting `Init` decide.

v2/cmd/once-server/templates/templates.go Outdated

					
				@ -104,2 +104,4 @@

					if dir != "" {

						if fi, err := os.Stat(dir); err != nil {

							return nil, fmt.Errorf("templates dir: %v", err)

[low] Use %w to wrap the underlying error: fmt.Errorf("templates dir: %w", err). %v flattens the error and breaks errors.Is(err, fs.ErrNotExist) for callers.

**[low]** Use `%w` to wrap the underlying error: `fmt.Errorf("templates dir: %w", err)`. `%v` flattens the error and breaks `errors.Is(err, fs.ErrNotExist)` for callers.

v2/cmd/once-server/templates/templates.go Outdated

					
				@ -106,0 +106,4 @@

						if fi, err := os.Stat(dir); err != nil {

							return nil, fmt.Errorf("templates dir: %v", err)

						} else if !fi.IsDir() {

							return nil, fmt.Errorf("templates dir: not a directory: %q\n", dir)

[nit] Drop the trailing \n from the Errorf format string — go vet flags it.

**[nit]** Drop the trailing `\n` from the `Errorf` format string — `go vet` flags it.

v2/cmd/once-server/upload.go Outdated

					
				@ -32,3 +32,3 @@

				const FormFilename = ".file"

				func Upload(w http.ResponseWriter, r *http.Request) {

				func upload(w http.ResponseWriter, r *http.Request) {

[nit] Upload → upload while Download, Remove, User stay exported. None of them are used outside package main, so lowercasing all four would be the consistent move.

**[nit]** `Upload` → `upload` while `Download`, `Remove`, `User` stay exported. None of them are used outside `package main`, so lowercasing all four would be the consistent move.

v2/cmd/once-server/upload.go Outdated

					
				@ -159,3 +160,3 @@

					zipW := zip.NewWriter(zipfile)

					defer zipW.Close()

					defer zipW.Close() //nolint:errcheck // closed explictly later

[nit] Typo: explictly → explicitly.

**[nit]** Typo: `explictly` → `explicitly`.

heiko added 2 commits

2026-05-24 19:54:09 +02:00

lint: whitespace 75ad978373

Fix linter issues: remove dead code, fix error wrapping, normalize handler naming cdf563e6e9

heiko added 1 commit

2026-05-24 19:58:22 +02:00

Restore cross-platform euid support for privilege checks dcdd7d04ff

- Restore euid_linux.go with unix.Geteuid() for Linux
- Restore euid_windows.go with euid = -1 for Windows
- Update main.go to use euid variable instead of unix.Geteuid() directly
- Remove unnecessary unix import from main.go

heiko commented

2026-05-24 20:05:25 +02:00

Security Review: Privilege & Authentication ✓

Excellent restoration! The cross-platform root check is solid:

Build Tags & Isolation: euid_linux.go and euid_windows.go use mutually exclusive build tags (//go:build linux and //go:build windows), ensuring clean platform separation. No risk of accidental mixed builds.

Init-Time Evaluation: Both implementations correctly initialize at package init time—Linux via unix.Geteuid() (from golang.org/x/sys/unix), Windows via sentinel -1. This is evaluated before main() runs, guaranteeing the check at line 86 operates on a valid value.

Privilege Check: The guard if euid == 0 && !cf.Global.RootOk is well-positioned early in main() and properly gates root execution with a documented config override. Clear failure message helps operators.

Windows Compatibility: No issues—euid = -1 on Windows means the check safely short-circuits (never matches euid == 0). ✓

No Dead Code Issues: The euid variable is actively referenced on line 86; linters should be satisfied.

Minor note: The rootOk override is appropriately guarded by config, not a flag—good security posture.

Review by privilege & auth security agent

**Security Review: Privilege & Authentication ✓** Excellent restoration! The cross-platform root check is solid: **Build Tags & Isolation**: `euid_linux.go` and `euid_windows.go` use mutually exclusive build tags (`//go:build linux` and `//go:build windows`), ensuring clean platform separation. No risk of accidental mixed builds. **Init-Time Evaluation**: Both implementations correctly initialize at package init time—Linux via `unix.Geteuid()` (from `golang.org/x/sys/unix`), Windows via sentinel `-1`. This is evaluated before `main()` runs, guaranteeing the check at line 86 operates on a valid value. **Privilege Check**: The guard `if euid == 0 && !cf.Global.RootOk` is well-positioned early in main() and properly gates root execution with a documented config override. Clear failure message helps operators. **Windows Compatibility**: No issues—euid = -1 on Windows means the check safely short-circuits (never matches `euid == 0`). ✓ **No Dead Code Issues**: The `euid` variable is actively referenced on line 86; linters should be satisfied. Minor note: The `rootOk` override is appropriately guarded by config, not a flag—good security posture. _Review by privilege & auth security agent_

heiko commented

2026-05-24 20:05:44 +02:00

Security Review: Filesystem & Input Handling ✓

✓ Security posture maintained — filesystem & input handling remain sound:

Pre-flight checks: Both serve.go and templates.go properly validate directory existence and type at startup via os.Stat(). This prevents runtime surprises and ensures the store/template directories are legitimate directories, not symlinks to untrusted locations.

Error wrapping fix: The change from %v to %w in templates.go is the correct Go practice for error chain preservation. Also cleaned up an unnecessary newline in error messages.

Handler naming: Lowercasing Download/Remove to download/remove is pure Go idiom (unexported functions) — no security implications since registration via http.HandlerFunc() remains identical.

Path traversal protection: Handlers properly sanitize untrusted input:

remove.go: Uses path.Base() to extract keys
download.go: Validates path containment before extraction
upload.go: Validates exact path matching
store.go: Uses filepath.Base() in CreateFile

Concurrent download safety: The Claim() method's atomic os.Rename() prevents TOCTOU vulnerabilities.

UUID-based storage: Files stored under UUIDs, not user filenames — excellent defense-in-depth against enumeration.

No new security concerns introduced. ✓

Review by filesystem & input security agent

**Security Review: Filesystem & Input Handling ✓** ✓ Security posture maintained — filesystem & input handling remain sound: **Pre-flight checks:** Both `serve.go` and `templates.go` properly validate directory existence and type at startup via `os.Stat()`. This prevents runtime surprises and ensures the store/template directories are legitimate directories, not symlinks to untrusted locations. **Error wrapping fix:** The change from `%v` to `%w` in `templates.go` is the correct Go practice for error chain preservation. Also cleaned up an unnecessary newline in error messages. **Handler naming:** Lowercasing `Download/Remove` to `download/remove` is pure Go idiom (unexported functions) — no security implications since registration via `http.HandlerFunc()` remains identical. **Path traversal protection:** Handlers properly sanitize untrusted input: - `remove.go`: Uses `path.Base()` to extract keys - `download.go`: Validates path containment before extraction - `upload.go`: Validates exact path matching - `store.go`: Uses `filepath.Base()` in `CreateFile` **Concurrent download safety:** The `Claim()` method's atomic `os.Rename()` prevents TOCTOU vulnerabilities. **UUID-based storage:** Files stored under UUIDs, not user filenames — excellent defense-in-depth against enumeration. No new security concerns introduced. ✓ _Review by filesystem & input security agent_

heiko commented

2026-05-24 20:05:44 +02:00

Security Review: HTTP Server Hardening ✓

Excellent hardening work! The timeout implementation is solid:

Strengths:

All four timeouts correctly configured: ReadHeaderTimeout (5s), ReadTimeout (30s), WriteTimeout (1m), IdleTimeout (2m). Good balance—aggressive enough to mitigate SlowLoris attacks, yet permissive enough for legitimate uploads.
Configurable & validated via TOML with sensible defaults; parse errors caught at startup.
Comprehensive test coverage verifies default values, custom values, and error handling.
TLS enforcement via checkTLS() is strict by default (rejects non-TLS unless InsecureOk=true); supports X-Forwarded-Proto for proxies.
Handler registration correctly updated to lowercase names; all three endpoints (upload, download, remove) wrapped with TLS checks.
Multipart upload correctly limited to 2 MiB in-memory threshold.

Pre-existing issue (not blocking):
Request body is unbounded—no explicit MaxBytesReader limit. The 30s ReadTimeout provides some DoS mitigation, but consider adding an explicit body size limit (e.g., io.LimitReader) in upload handlers for defense-in-depth.

Overall: Well-executed timeout hardening with strong testing. Ready to merge.

Review by HTTP & DoS security agent

**Security Review: HTTP Server Hardening ✓** Excellent hardening work! The timeout implementation is solid: **Strengths:** - All four timeouts correctly configured: **ReadHeaderTimeout** (5s), **ReadTimeout** (30s), **WriteTimeout** (1m), **IdleTimeout** (2m). Good balance—aggressive enough to mitigate SlowLoris attacks, yet permissive enough for legitimate uploads. - **Configurable & validated** via TOML with sensible defaults; parse errors caught at startup. - **Comprehensive test coverage** verifies default values, custom values, and error handling. - **TLS enforcement** via `checkTLS()` is strict by default (rejects non-TLS unless `InsecureOk=true`); supports `X-Forwarded-Proto` for proxies. - **Handler registration** correctly updated to lowercase names; all three endpoints (`upload`, `download`, `remove`) wrapped with TLS checks. - Multipart upload correctly limited to **2 MiB** in-memory threshold. **Pre-existing issue (not blocking):** Request body is unbounded—no explicit `MaxBytesReader` limit. The 30s `ReadTimeout` provides some DoS mitigation, but consider adding an explicit body size limit (e.g., `io.LimitReader`) in upload handlers for defense-in-depth. Overall: **Well-executed timeout hardening with strong testing.** Ready to merge. _Review by HTTP & DoS security agent_

heiko commented

2026-05-24 20:05:44 +02:00

Security Review: Error Handling & Code Quality ✓

All key areas check out well:

✅ Lint Configuration: Removed .golangci.yaml correctly (eliminated collision). No conflicting lint config files remain, allowing clean default behavior.

✅ Dead Code Removal: logout.go was properly deleted—confirmed it was unused and marked "does not work yet."

✅ Error Handling: templates.go properly uses %w for error wrapping without trailing newlines. Error messages to clients stay secure (logged server-side only, returning only HTTP status codes).

✅ Resource Management: Password file pre-flight check uses _ = fh.Close() appropriately. Multipart upload handlers use explicit //nolint:errcheck with clear comments where close errors are non-critical (files already synced before close).

✅ No Information Disclosure: All handlers (download, upload, remove, user) sanitize responses—credentials/paths stay internal, clients see only safe status codes or "Not Authorized" messages.

✅ Cross-platform Support: euid checks now use build-tagged files (euid_linux.go/euid_windows.go) instead of Unix-only imports. Better for portability.

✅ Code Quality: Function naming normalized (public→private). Removed unnecessary linter exceptions. Store package simplified. All whitespace/formatting improvements.

Zero security concerns identified. This is a solid cleanup with proper attention to error handling and information disclosure.

Review by error handling & code quality security agent

**Security Review: Error Handling & Code Quality ✓** All key areas check out well: ✅ **Lint Configuration**: Removed `.golangci.yaml` correctly (eliminated collision). No conflicting lint config files remain, allowing clean default behavior. ✅ **Dead Code Removal**: `logout.go` was properly deleted—confirmed it was unused and marked "does not work yet." ✅ **Error Handling**: `templates.go` properly uses `%w` for error wrapping without trailing newlines. Error messages to clients stay secure (logged server-side only, returning only HTTP status codes). ✅ **Resource Management**: Password file pre-flight check uses `_ = fh.Close()` appropriately. Multipart upload handlers use explicit `//nolint:errcheck` with clear comments where close errors are non-critical (files already synced before close). ✅ **No Information Disclosure**: All handlers (download, upload, remove, user) sanitize responses—credentials/paths stay internal, clients see only safe status codes or "Not Authorized" messages. ✅ **Cross-platform Support**: euid checks now use build-tagged files (`euid_linux.go`/`euid_windows.go`) instead of Unix-only imports. Better for portability. ✅ **Code Quality**: Function naming normalized (public→private). Removed unnecessary linter exceptions. Store package simplified. All whitespace/formatting improvements. **Zero security concerns identified.** This is a solid cleanup with proper attention to error handling and information disclosure. _Review by error handling & code quality security agent_