style: cargo fmt across audit-2026-06-11 tier-3 changes

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

security_audit/06_files_pathtraversal.md

@@ -0,0 +1,122 @@
+# Security Audit — File Storage & Path Traversal
+
+**Scope:** v1.1.5 `files::*` SDK + admin API; FS layout `<PICLOUD_FILES_ROOT>/files/<app_id>/<collection>/<id[0:2]>/<id>`.
+**Reviewed files:**
+- `/home/fabi/PiCloud/crates/manager-core/src/files_service.rs`
+- `/home/fabi/PiCloud/crates/manager-core/src/files_repo.rs`
+- `/home/fabi/PiCloud/crates/manager-core/src/files_api.rs`
+- `/home/fabi/PiCloud/crates/manager-core/src/files_sweep.rs`
+- `/home/fabi/PiCloud/crates/shared/src/files.rs`
+- `/home/fabi/PiCloud/crates/executor-core/src/sdk/files.rs`
+- `/home/fabi/PiCloud/dashboard/src/routes/apps/[slug]/files/+page.svelte`
+- `/home/fabi/PiCloud/dashboard/src/lib/api.ts`
+- `/home/fabi/PiCloud/caddy/Caddyfile`
+
+## Counts by severity
+| Severity | Count |
+|---|---|
+| Critical | 0 |
+| High     | 2 |
+| Medium   | 4 |
+| Low      | 4 |
+| Info     | 2 |
+
+---
+
+## Findings
+
+### High
+
+#### F-FS-001 — Missing MIME allowlist enables stored XSS via `Content-Disposition: inline` on the admin download endpoint
+- **Severity:** High (storage half — rendering half is agent 7's call)
+- **Location:** `crates/manager-core/src/files_api.rs:153-161` (`get_file`), `crates/shared/src/files.rs` (`NewFile::validate`)
+- **Summary:** `get_file` echoes the script-supplied `content_type` straight into the response `Content-Type` header, paired with `Content-Disposition: inline; filename="..."`. There is no allowlist on `content_type` at the SDK boundary — `NewFile::validate` only caps its length at 127 bytes. A Rhai script can `files::create(#{ name: "x.svg", content_type: "image/svg+xml", data: <svg onload=...> })` (or `text/html`), and any logged-in operator who clicks Download in the dashboard renders the attacker payload **same-origin** under `/api/v1/admin/...`, with the picloud admin session cookie attached. That is admin-session theft.
+- **Storage-side fix:** maintain a small allowlist (`image/png|jpeg|gif|webp`, `application/pdf`, `application/octet-stream`, `text/plain`, `application/json`) at `NewFile::validate` + `FileUpdate::validate`. Anything else stored becomes `application/octet-stream` on serve, OR the admin endpoint forces `Content-Disposition: attachment` and `X-Content-Type-Options: nosniff` + `Content-Security-Policy: sandbox` for non-allowlisted types.
+- **Cross-ref:** **agent 7** owns the rendering / response-header conclusion (CSP, sandbox, `nosniff`, force-attachment). Flag jointly.
+
+#### F-FS-002 — Admin files API skips collection validation; underlying `head`/`get`/`list`/`delete` skip the FS-path guard too
+- **Severity:** High (defense-in-depth gap; **not** exploitable today, but one bug away from arbitrary-FS-read/write/unlink)
+- **Location:** `crates/manager-core/src/files_api.rs:78-184` (no `validate_collection`); `crates/manager-core/src/files_repo.rs:321,395` (`guard_collection` is only called from `create` and `update` — `head`, `get`, `list`, `delete` do **not** call it).
+- **Summary:** The admin endpoints accept `collection` as a path/query parameter and pass it raw to the repo. The repo's `head`/`get`/`list` only execute parametrized SQL (no immediate FS access), and `delete` only builds `final_path` and `remove_file`s *after* a matching DB row is found. **Today** this is safe because `create`/`update` validate before insert, so no row can exist with a traversal collection. **Tomorrow** it's one missed validation away from full filesystem reach. Specifically: `FsFilesRepo::get` → `read_verify_at(..., collection, id, ...)` and `FsFilesRepo::delete` → `self.final_path(app_id, collection, id)` + `std::fs::remove_file(path)` both build paths from the unvalidated collection. A future code path that inserts a row with `collection = "../../etc"` (a buggy migration, an admin restore tool, a backup loader) would immediately give cross-app file-read and unlink.
+- **Fix shape:** call `validate_files_collection` at the top of every admin endpoint (`list_files`, `get_file`, `delete_file`) AND add `Self::guard_collection(collection)?;` to `FsFilesRepo::{head, get, list, delete}`. Both layers — depth — because the audit posture for this surface is "no traversal possible regardless of caller."
+
+---
+
+### Medium
+
+#### F-FS-003 — No per-app storage quota; one tenant can fill the host disk
+- **Severity:** Medium (cross-ref **agent 8** for the host-DoS angle)
+- **Location:** `crates/manager-core/src/files_service.rs`, `crates/manager-core/src/files_repo.rs` — no quota check anywhere.
+- **Summary:** Only the per-file cap (`PICLOUD_FILES_MAX_FILE_SIZE_BYTES`, default 100 MB) is enforced. An app can create unlimited rows of up to 100 MB each via any public Rhai script that calls `files::create`. With multi-tenant isolation as a stated goal, this is the cheapest path to host-level DoS: one anonymous public HTTP script + a loop. CLAUDE.md acknowledges per-app quotas are deferred to v1.2, but the threat surface exists today.
+- **Fix shape:** track `SUM(size_bytes) GROUP BY app_id` in a small counter table (`files_app_usage`), increment/decrement in the same tx as the `files` row mutation, reject `create`/`update` when projected usage exceeds `PICLOUD_FILES_APP_QUOTA_BYTES` (default e.g. 5 GB).
+
+#### F-FS-004 — Content-Disposition filename is not RFC 5987-encoded; non-ASCII names lose data or break the header
+- **Severity:** Medium
+- **Location:** `crates/manager-core/src/files_api.rs:153-156`
+- **Summary:**
+  ```rust
+  let disposition = format!(
+      "inline; filename=\"{}\"",
+      meta.name.replace('"', "").replace(['\r', '\n'], "")
+  );
+  ```
+  CRLF and `"` are stripped (good — no header injection), but no RFC 5987 `filename*=UTF-8''<pct-encoded>` form. Non-ASCII filenames (`café.pdf`, `合同.pdf`) either round-trip through a lossy ASCII strip (browsers vary), or become an invalid `filename="..."` token if any high-bit byte is preserved. Combined with `inline`, the response then renders without an obvious filename, weakening the user's ability to spot a malicious upload.
+- **Fix shape:** emit both forms — `filename="<ASCII-sanitized fallback>"; filename*=UTF-8''<percent-encoded>` — per RFC 6266 §5.
+
+#### F-FS-005 — Temp file mode honors umask (typically 0644) instead of 0600
+- **Severity:** Medium (downgraded by the 0o700 shard-dir guard, but defense-in-depth misses)
+- **Location:** `crates/manager-core/src/files_repo.rs:267` — `std::fs::File::create(&tmp)`
+- **Summary:** The temp file gets default `OpenOptions::create(true).write(true).truncate(true)` mode, masked through the process umask. On a typical Linux box that's 0644. The 0o700 parent shard dir protects against world access today, but if `PICLOUD_FILES_ROOT` ever lives somewhere mounted with `nosuid,nodev` for a service user, or if the operator chowns the root to a less-restrictive group, file bytes leak.
+- **Fix shape:** on Unix, set `OpenOptionsExt::mode(0o600)` on the temp file create; then `set_permissions(0o600)` after rename to harden against the umask not applying as expected.
+
+#### F-FS-006 — No symlink-aware path resolution; a stray symlink inside the root escapes
+- **Severity:** Medium (theoretical — there is no code path that creates symlinks under `PICLOUD_FILES_ROOT`, but the sweeper and the read path do not detect them)
+- **Location:** `crates/manager-core/src/files_repo.rs:282-311` (`read_verify_at`); `crates/manager-core/src/files_sweep.rs:75-111` (`walk`)
+- **Summary:** `read_verify_at` opens via `std::fs::read(&path)` which follows symlinks. The orphan sweeper's `walk` uses `entry.file_type()` (which returns the symlink type if applicable), and skips non-files — symlinks-to-files would be skipped, but symlinks-to-dirs are also skipped, OK. The bigger gap is read: if **any** code path (a future backup-restore, an operator drop-in, a docker volume mount) plants a symlink at `<root>/files/<app>/<col>/<sh>/<id>` pointing to `/etc/shadow`, a subsequent `get_file` reads it. There's no `canonicalize` + prefix check.
+- **Fix shape:** in `read_verify_at` / `write_atomic_at`, after constructing the path, `canonicalize` it and verify the canonical form starts with the canonicalized files-root; reject otherwise. For the sweeper, use `symlink_metadata` instead of `metadata` to avoid traversing.
+
+---
+
+### Low
+
+#### F-FS-007 — Download endpoint relies on session cookie only; no signed-URL option
+- **Severity:** Low (auth is required via cookie — but the URL is unbearer-able)
+- **Location:** `crates/manager-core/src/files_api.rs:130-165`, `dashboard/src/lib/api.ts:956-957`
+- **Summary:** `api.files.downloadUrl(...)` returns a plain `/api/v1/admin/...` URL relying on the `picloud_session` cookie for auth. Authn is enforced (auth middleware + `require(... AppFilesRead)`), so it is **not** open. The only concern is operator hygiene: if an admin copies the URL and shares it (intending to share the file), the recipient gets auth-free 403 instead of the file — fine, defense-wise. No fix required; just noting.
+
+#### F-FS-008 — Filename-byte-length cap of 255 doesn't account for Windows / FAT-style ceilings or POSIX `NAME_MAX` on platforms with smaller caps
+- **Severity:** Low
+- **Location:** `crates/shared/src/files.rs:27,197`
+- **Summary:** 255 bytes is fine on ext4/btrfs/xfs. On some platforms (HFS+: 255 UTF-16 code units; ZFS: usually OK), edge cases. The name is also reflected into `Content-Disposition` and the dashboard table, so the cap is more a header-hygiene concern than an FS one.
+
+#### F-FS-009 — `head`/`get`/`list` don't reject NUL bytes in `collection` before SQL
+- **Severity:** Low (Postgres rejects NUL in TEXT, so the SQL fails cleanly with a 500 — but the user sees "internal error" instead of "invalid collection")
+- **Location:** `crates/manager-core/src/files_repo.rs:347-516`
+- **Summary:** Adding `Self::guard_collection` (see F-FS-002) gives a precise 4xx instead of a 5xx for `\0` in collection names.
+
+#### F-FS-010 — `Content-Length` from `bytes.len()` is set, but no `Cache-Control` / `X-Content-Type-Options` on the download
+- **Severity:** Low
+- **Location:** `crates/manager-core/src/files_api.rs:158-164`
+- **Summary:** Missing `X-Content-Type-Options: nosniff` lets the browser MIME-sniff a stored `text/plain` payload into `text/html` and render JS. Pairs with F-FS-001 as the response-header hardening pass.
+
+---
+
+### Info
+
+#### F-FS-011 — File IDs are server-side `Uuid::new_v4()`; safe from caller-controlled bytes
+- **Location:** `crates/manager-core/src/files_repo.rs:322` and `:268` (in-memory test). UUIDs are formatted via `to_string()` (lowercase hex + dashes, never traversal-shaped). Good.
+
+#### F-FS-012 — Atomic write protocol is correct; delete order is correct
+- **Location:** `files_repo.rs:217-277` and `:431-473`. Temp + fsync + rename + dir-fsync; row delete inside tx then unlink after commit. The "metadata gone, bytes still on disk" leak is bounded (rare crash window after the tx commit) and is reclaimable by a future reconcile sweep (currently only `*.tmp.*` orphans are reaped by `files_sweep.rs`). No bug — just noting the design is sound and the residual leak is documented.
+
+---
+
+## Cross-references for other agents
+
+- **Agent 7 (XSS / rendering):** F-FS-001 — the storage side allows arbitrary `content_type`; rendering side owns CSP / `X-Content-Type-Options: nosniff` / forced `attachment`. Joint sev = High.
+- **Agent 8 (DoS / resource):** F-FS-003 (no per-app quota) — same concern they may have raised for `kv::set` / `docs::*` / `pubsub` / `queue` unbounded payloads (cf. F-S-001 in AUDIT.md). Files has a per-row cap, but no per-app aggregate.
+- **Agent 10 (CLI / config files):** CLI credentials are `0o600` (verified, `crates/picloud-cli/src/config.rs:98,107`); no finding from the FS angle.
+
+## Most-important takeaway
+
+Today nothing escapes `PICLOUD_FILES_ROOT` — `create` and `update` guard `collection`, and FS paths are only ever built from server-generated UUIDs and validated collections. The exposure that is **live** is F-FS-001: a public-route script can stash `text/html` or `image/svg+xml` content that the operator's browser renders same-origin under the admin session cookie. The exposure that is **one mistake away** is F-FS-002: every read/delete path through the admin API trusts collection-validation done elsewhere, and the repo's `head`/`get`/`list`/`delete` don't have the belt-and-suspenders guard that `create`/`update` carry. Add the missing `validate_files_collection` calls at the admin layer and `guard_collection` calls at the repo layer, and the surface becomes traversal-proof regardless of caller. The MIME allowlist + `Content-Disposition: attachment` for non-image types closes the stored-XSS class jointly with agent 7's response-header pass.