From 9d06a6d2068ad74584c6d2878b2c3f415c82f3c7 Mon Sep 17 00:00:00 2001 From: Domagoj Zecevic Date: Thu, 24 Sep 2026 12:56:12 +0200 Subject: [PATCH] feat(auth): add local HTTP cookie opt-in --- .ai/HANDOFF.md | 54 ++++++++++++++++++++++++++++ .ai/PLAN.md | 45 +++++++++++++++++++++-- .ai/REVIEW.md | 23 ++++++++++++ .ai/TASKS.md | 2 ++ README.md | 30 +++++++++++++++- ROADMAP.md | 21 +++++++++++ compose.yaml | 3 +- internal/web/app.go | 26 ++++++++++---- internal/web/app_test.go | 77 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 270 insertions(+), 11 deletions(-) diff --git a/.ai/HANDOFF.md b/.ai/HANDOFF.md index 949eaaf..f9369e4 100644 --- a/.ai/HANDOFF.md +++ b/.ai/HANDOFF.md @@ -218,3 +218,57 @@ Each entry uses this exact structure. Omit fields marked as role-specific when t | Next Role | none | --- + +### T-007 — plan — 2026-09-24T10:38:36Z + +| Field | Value | +|-------|-------| +| Agent | claude | +| Summary | rework_plan T-002: diagnosed the user's local-HTTP login redirect loop as the hardcoded `Secure: true` session cookie (internal/web/app.go) being dropped by the browser over non-HTTPS origins; user chose to add an off-by-default local-testing opt-in rather than requiring a reverse proxy. Updated ROADMAP.md (new auth requirement), .ai/PLAN.md (scope, Phase 7, validation), and added task T-007 to .ai/TASKS.md as an additive follow-on to the already-shipped T-002. | +| Files Changed | ROADMAP.md, .ai/PLAN.md, .ai/TASKS.md, .ai/HANDOFF.md | +| Validation | n/a (planning only, no code changed) | +| Commit | n/a (planner does not commit) | +| Next Role | implement | + +--- + +### T-007 — dev — 2026-09-24T10:47:05Z + +| Field | Value | +|-------|-------| +| Agent | codex | +| Summary | Added an explicit local-only insecure-cookie opt-in so plain-HTTP testing can complete the login and password-change flow without changing secure defaults. | +| Files Changed | README.md, compose.yaml, internal/web/app.go, internal/web/app_test.go, .ai/TASKS.md, .ai/REVIEW.md, .ai/HANDOFF.md | +| Validation | `go fmt ./...`, `go vet ./...`, and `go test ./...` passed in `golang:1.24`; default and opt-in Compose configuration plus `bash scripts/test-production-deployment.sh` passed. | +| Commit | `feat(auth): add local HTTP cookie opt-in` | +| Verdict | PASS | +| Blocking Findings | none | +| Next Role | human approval | + +--- + +### T-007 — commit_task — 2026-09-24T10:48:43Z + +| Field | Value | +|-------|-------| +| Agent | codex | +| Summary | Approved the reviewed T-007 local HTTP cookie opt-in and prepared its single task commit. | +| Files Changed | .ai/TASKS.md, .ai/HANDOFF.md | +| Validation | Review validation recorded in `.ai/REVIEW.md`. | +| Commit | `feat(auth): add local HTTP cookie opt-in` | +| Next Role | none | + +--- + +### T-008 — plan — 2026-09-24T10:55:45Z + +| Field | Value | +|-------|-------| +| Agent | claude | +| Summary | rework_plan T-004: user reported wanting to stream .mp4/.mkv; investigated and confirmed a real bug — `isVideoFile`/delivery `Content-Type` in internal/web/app.go rely on `mime.TypeByExtension`, which resolves nothing for video extensions inside the actual `alpine:3.21` production image (no OS MIME database, no video entries in Go's built-in fallback table), verified by inspecting the base image directly. Updated ROADMAP.md (Priority 3 note) and .ai/PLAN.md (Phase 8, validation), and added corrective task T-008 to .ai/TASKS.md as a follow-on to the already-shipped T-004, requiring an app-internal video extension/MIME table and documenting the .mkv in-browser playback caveat. | +| Files Changed | ROADMAP.md, .ai/PLAN.md, .ai/TASKS.md, .ai/HANDOFF.md | +| Validation | n/a (planning only, no code changed); bug confirmed by running `alpine:3.21` and `golang:1.24-alpine` in Docker to inspect MIME data availability and Go's builtin type table | +| Commit | n/a (planner does not commit) | +| Next Role | implement | + +--- diff --git a/.ai/PLAN.md b/.ai/PLAN.md index 540b716..370ca8b 100644 --- a/.ai/PLAN.md +++ b/.ai/PLAN.md @@ -29,6 +29,9 @@ zero server-side transcoding. - Documentation: `README.md` is kept current with every behavior/interface/ workflow/config change in the same commit that makes the change (no follow-up doc tasks). +- Local-testing-only opt-in to disable the `Secure` session-cookie + attribute (default: secure/on), so login works over plain `http://` for + local development without a reverse proxy. ## Out of scope (this cycle) @@ -87,8 +90,36 @@ zero server-side transcoding. credential flow, forced password change, reverse-proxy TLS + Range passthrough requirement). -Tasks are sequential (T-001 → T-006); each phase's acceptance criteria and -required documentation updates are tracked per-task in `.ai/TASKS.md`. +### Phase 7 — Local-testing insecure-cookie opt-in (T-007) +- Added after T-001..T-006 shipped: an explicit, off-by-default env var + that disables the `Secure` attribute on the session cookie so login works + over plain `http://` for local development (e.g. non-`https://` access, + including non-`localhost` origins where the browser doesn't treat the + origin as a secure context). Default behavior (Secure cookie) is + unchanged for anyone who doesn't set the var. `README.md` gets a new + "Local development over HTTP" section spelling out that this switch must + never be enabled on a real/production/exposed deployment. + +### Phase 8 — Fix video type detection for real containers (T-008) +- Bug fix against the shipped T-004 behavior: `isVideoFile` and the + `Content-Type` served for downloads/streams currently rely on + `mime.TypeByExtension`, which returns nothing useful for `.mp4`/`.mkv`/etc. + inside the actual Alpine-based runtime image (no OS MIME database present, + and Go's built-in fallback table has no video entries). Replace this with + an app-internal extension → MIME-type table covering at least `.mp4`, + `.m4v`, `.mov`, `.webm`, `.mkv`, `.avi`, `.mpg`/`.mpeg`, `.wmv`, `.flv`, + used consistently for both the "stream" link visibility check and the + `Content-Type` header on file delivery, independent of the OS/container + MIME database. `README.md` documents the supported video extensions and + the `.mkv`-in-browser caveat (downloads always work; native in-browser + playback of the MKV container is browser-dependent and not guaranteed, + since no transcoding is performed per the project's no-encoding + requirement). + +Tasks are sequential (T-001 → T-006); T-007 and T-008 are additive +follow-on tasks built on top of the already-shipped T-002 and T-004 work, +respectively. Each phase's acceptance criteria and required documentation +updates are tracked per-task in `.ai/TASKS.md`. ## Validation @@ -106,3 +137,13 @@ Task-specific manual/E2E checks (documented per task in `.ai/TASKS.md`): - `Range`-header request against a video file, confirming `206 Partial Content` and correct byte range. - `docker build` + `docker run` smoke test with a sample mounted folder. +- With the insecure-cookie opt-in unset (default), confirm the session + cookie still has `Secure` set. With it explicitly enabled, confirm login + over plain `http://` succeeds and the cookie no longer has `Secure` set, + while `HttpOnly`/`SameSite`/expiry/forced-password-change behavior is + unchanged. +- Inside the actual production Docker image (not the `golang:*` build + image, which does have a fuller MIME setup) — request an `.mp4` file and + confirm `Content-Type: video/mp4` and a visible "stream" link; same for + `.mkv` with the correct Matroska `Content-Type`; confirm a `Range` + request against the `.mp4` still returns `206 Partial Content`. diff --git a/.ai/REVIEW.md b/.ai/REVIEW.md index 9295a1f..dea83f6 100644 --- a/.ai/REVIEW.md +++ b/.ai/REVIEW.md @@ -132,3 +132,26 @@ build context excludes repository-only artifacts, and both documented launch paths preserve the root-configuration and persistent-data mounts. README now covers Compose and Docker Run deployment, first-login administration, the forced-password-change flow, and reverse-proxy TLS and Range requirements. + +## T-007 — 2026-09-24T10:47:05Z + +Verdict: PASS + +Findings: none. + +Validation: + +- `go fmt ./...` (in `golang:1.24`): PASS +- `go vet ./...` (in `golang:1.24`): PASS +- `go test ./...` (in `golang:1.24`): PASS +- `docker compose config`: PASS with the opt-in unset (empty) and explicitly + set to `true`; the variable reaches the service only when requested. +- `bash scripts/test-production-deployment.sh`: PASS. + +The review confirms that `WEB_FILE_BROWSER_INSECURE_COOKIES` is secure by +default and only disables the `Secure` attribute for an explicit `true` value. +The HTTP cookie-jar test proves that a browser can keep the non-secure cookie +over plain HTTP, remains gated at forced password change, and reaches the home +page after changing the temporary password. `HttpOnly`, `SameSite=Lax`, the +session expiry, and the cookie path remain unchanged. README contains explicit +local-only Compose and Docker Run instructions with an unsafe-use warning. diff --git a/.ai/TASKS.md b/.ai/TASKS.md index 72bd86f..9ed904a 100644 --- a/.ai/TASKS.md +++ b/.ai/TASKS.md @@ -26,3 +26,5 @@ Command expectations: | T-004 | Download & video streaming: raw-byte download endpoint with correct `Content-Type`/`Content-Disposition`, full HTTP `Range` support for video files (seek/partial content/resume) with zero transcoding anywhere in the request path | done | Downloading any file returns identical bytes to the source file (checksum match); a `Range` request against a video file returns `206 Partial Content` with the correct byte range and headers; no encoding/thumbnailing/transcoding code path exists; `README.md` documents the reverse-proxy Range-header-passthrough requirement | PASS: root-confined regular-file opens; authenticated byte-identical attachment download; video `206` `Range` response; unauthenticated delivery gate; `go fmt ./...`, `go vet ./...`, `go test ./...` in `golang:1.24` | none | | T-005 | Admin user management: admin-only screens to list users, create a user (temporary password, `must_change_password=true`), reset a user's password, disable/delete a user; role-based route protection (403 for authenticated non-admins, not a redirect that leaks page existence) | done | Non-admin authenticated user gets 403 on all admin routes; admin can create a user who is then forced through the change-password gate on first login; admin can reset/disable a user; `README.md` documents the admin user-management flow | PASS: admin lifecycle/session-invalidation and HTTP role-protection/temporary-password tests; `go fmt ./...`, `go vet ./...`, `go test ./...` in `golang:1.24`; E2E not configured | none | | T-006 | Docker packaging & deployment docs: finalize multi-stage Dockerfile (small image, no build tools in final image), finalize docker-compose example with volume mounts + root config, complete `README.md` (build/run, root config format, first-login/admin credential flow, forced password change, reverse-proxy TLS + Range passthrough requirement) | done | `docker build` produces a working image; `docker run` with a sample volume mount and root config serves the login page only until authenticated, per Priority 1; `README.md` is complete enough for a new operator to deploy without reading source code | PASS: Compose configuration validation; production image build/run, non-root runtime, and unauthenticated login boundary smoke test; `go fmt ./...`, `go vet ./...`, `go test ./...` in `golang:1.24` | none | +| T-007 | Local-testing insecure-cookie opt-in: add an off-by-default env var (e.g. `WEB_FILE_BROWSER_INSECURE_COOKIES`) that, when explicitly enabled, omits the `Secure` attribute from the session cookie so login works over plain `http://` (non-TLS, non-`localhost`-only origins) for local development; default behavior unchanged; no other session/auth behavior (`HttpOnly`, `SameSite`, expiry, forced password change) may be weakened; `README.md` gets a new "Local development over HTTP" section that explicitly warns this must never be enabled on a real/production/exposed deployment | done | With the var unset, session cookie still has `Secure` set and behavior is unchanged from T-002/T-005; with the var explicitly enabled, login over plain `http://` succeeds (no redirect loop back to `/login`), the cookie has `Secure` unset, and all other cookie/session attributes and the forced-password-change gate are unchanged; `README.md` documents local-only Docker Compose and Docker Run setup with an explicit warning | PASS: default-secure and plain-HTTP cookie-jar login/forced-change tests; Compose default/opt-in configuration validation; `go fmt ./...`, `go vet ./...`, `go test ./...` in `golang:1.24`; deployment E2E passed | none | +| T-008 | Bug fix (regression against shipped T-004): `isVideoFile` and the delivered `Content-Type` currently rely on `mime.TypeByExtension`, which resolves to nothing for `.mp4`/`.mkv`/etc. inside the actual Alpine-based production image (no OS MIME database present, no video entries in Go's built-in fallback table) — verified by inspecting `alpine:3.21` directly. Replace with an app-internal extension → MIME-type table (`.mp4`, `.m4v`, `.mov`, `.webm`, `.mkv`, `.avi`, `.mpg`/`.mpeg`, `.wmv`, `.flv` at minimum) used consistently by both the "stream" link visibility check and the file-delivery `Content-Type` header, independent of any OS/container MIME database. `README.md` documents the supported video extensions and the `.mkv`-in-browser playback caveat (download always works; native in-browser streaming of the MKV container is browser-dependent, not guaranteed, and no transcoding is performed per the project's no-encoding requirement) | ready_for_implement | Inside the actual production Docker image (not the Go build image): requesting an `.mp4` returns `Content-Type: video/mp4` and the folder listing shows a "stream" link for it; requesting an `.mkv` returns the correct Matroska `Content-Type` and a "stream" link; a `Range` request against the `.mp4` still returns `206 Partial Content` with correct byte range; unit tests cover the new extension table directly (no dependency on host/OS MIME data) so the bug can't silently regress; `README.md` lists supported video extensions and the `.mkv` browser-support caveat | n/a | implement | diff --git a/README.md b/README.md index 3eae1d3..769dc90 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ included deployment configuration. The Compose service restarts unless stopped explicitly. It persists SQLite data in the `webfilebrowser-data` named volume and mounts every configured browse -root read-only. The server listens on `http://localhost:8080` and logs every +root read-only. The server listens on `http://localhost:8180` and logs every configured root at startup. `/login` is the only public application route; every other route redirects to login until it has a valid server-side session. @@ -108,6 +108,34 @@ send the secure cookie over plain HTTP. Bind the application to loopback as in the Docker Run example, terminate TLS at the reverse proxy, and forward the original `Host` and `Range` request headers to the application. +## Local development over HTTP + +> **Warning:** `WEB_FILE_BROWSER_INSECURE_COOKIES=true` is for local +> development or isolated testing only. It lets browsers send an authentication +> cookie over unencrypted HTTP. Never set it for a real, production, or +> publicly exposed deployment. + +Session cookies are `Secure` by default. If you need to test directly over +plain HTTP—for example through a LAN address where the browser will not keep a +secure cookie—set the opt-in variable explicitly. All other session behavior, +including `HttpOnly`, `SameSite=Lax`, sliding expiry, and the forced +password-change gate, remains enabled: + +```bash +WEB_FILE_BROWSER_INSECURE_COOKIES=true \ + MEDIA_DIR=/srv/webfilebrowser/media \ + docker compose up --build +``` + +The included Compose file publishes port `8180`, so the local test URL is +`http://localhost:8180` (or `http://:8180` on a trusted test LAN). +For Docker Run, add the following flag to the command above only for the same +local-only purpose: + +```bash +--env WEB_FILE_BROWSER_INSECURE_COOKIES=true +``` + ## User administration After changing the seeded administrator password, use **Manage users** on the diff --git a/ROADMAP.md b/ROADMAP.md index 7b58f09..f01b5b3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -34,6 +34,15 @@ page/route (plus static login assets). TLS; the app itself serves plain HTTP inside the container. This assumption and the required reverse-proxy config (incl. passing through HTTP Range headers) must be documented in `README.md`. +- Session cookies are `Secure` by default (required for any real/production + deployment, per the TLS assumption above). For local development/testing + only, an explicit opt-in switch (env var, off by default) may disable the + `Secure` cookie attribute so login works over plain `http://` (e.g. + `http://localhost:PORT` on a non-loopback-only bind, or a LAN IP). This + switch must default to the secure behavior, must be clearly documented as + unsafe for anything but local testing, and must not weaken any other + auth/session behavior (forced password change, session expiry, HttpOnly, + SameSite all remain unchanged). ## Priority 2 — Folder browsing UI @@ -58,6 +67,18 @@ browsing configured root folders. ## Priority 3 — File download & video streaming (no transcoding) +Note: video-type detection and `Content-Type` for streaming/download must not +depend on the container's OS-level MIME database (e.g. `/etc/mime.types`, +`/usr/share/mime/globs2`) — the minimal Alpine runtime image ships none of +these, and Go's built-in `mime` fallback table does not include any video +extensions. Video extensions (`.mp4`, `.m4v`, `.mov`, `.webm`, `.mkv`, `.avi`, +`.mpg`/`.mpeg`, `.wmv`, `.flv`, at minimum) must be recognized and given a +correct `Content-Type` via an extension table built into the app itself. +`.mkv` (Matroska) must still be downloadable, but in-browser inline streaming +of `.mkv` is inherently browser-dependent (most browsers do not support the +MKV container for native `