From 8b15c96c0f7a98735140d3776d7152642c2e71fc Mon Sep 17 00:00:00 2001 From: JD Date: Sat, 7 Nov 2026 21:00:50 -0500 Subject: [PATCH] feat(files): let the File Viewer edit workspace dotenv files The plain preview already serves a workspace dotenv file in full to the same user, so refusing the edit protected nothing. The edit gate now passes allowDotenv to the sensitive-path check, which skips only the two dotenv patterns; every other credential pattern, a dotenv file inside a blocked tree and Codeman's own data-dir login file stay blocked. Downloads and attachments are unchanged. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- docs/file-viewer-edit-plan.md | 7 +- docs/wiki/Working-With-Files.md | 4 + src/config/attachment-guard.ts | 10 +- src/config/file-editing.ts | 14 +- src/web/routes/file-routes.ts | 11 +- src/web/sensitive-path.ts | 25 ++- test/file-editing-policy.test.ts | 43 ++++- .../file-routes-attachment-extensions.test.ts | 83 ++++++++++ test/routes/file-routes.test.ts | 21 +++ test/routes/file-write-routes.test.ts | 156 +++++++++++++++++- test/sensitive-path.test.ts | 73 +++++++- 13 files changed, 417 insertions(+), 34 deletions(-) create mode 100644 test/routes/file-routes-attachment-extensions.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 245016ee..2f4a313a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -241,7 +241,7 @@ The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLin **Filesystem path picker** (every Add Case "Browse": Link Existing, Create New's custom folder, Clone Repo, Docker; + the mobile keyboard's `📁 Path` key): lazy one-directory browsing via `GET /api/filesystem/browse`, with `GET /api/filesystem/preview` for the tapped file (Docker's Container Workdir picker lists through its own `fetchListing` over `POST /api/docker-cases/browse` instead, so the allowlist, blocked-tree and ownership rules below do not apply to it). A Browse fills its form field; the Path key inserts the path **without** Enter, so the prompt is never submitted; the sibling `⌫ All` key clears only the unsent prompt and must never send the agent's `/clear`. ⚠️ This is a **second file-serving surface and inherits neither the attachment confinement nor its ownership scoping**: it allowlists Home, `CASES_DIR`, `/mnt/d` and `CODEMAN_FILE_PICKER_ROOTS`, blocks sensitive trees, and rejects symlink escapes **after** `realpath`. ⚠️ The optional `sessionId` is an ownership boundary that must be `canAccessOwned`-checked by hand (it does not go through `findSessionOrFail`), and in multi-user mode a non-admin gets only their own `userSpacePath` as a root: per-user spaces live INSIDE `homedir()`, so a `Home` root exposes every other user's workspace. Previews go through the same global conversion limiter, and Markdown/TXT/JSON are served as inert `text/plain`. **New folder** (opt-in per caller, `allowNewFolder`; Link Existing, Create New's custom folder and Clone Repo's parent folder have it): `POST /api/filesystem/mkdir` `{ path, name, sessionId? }` creates ONE folder (non-recursive) directly inside a folder the picker may browse, resolved through the same roots, ownership and blocked-tree rules as browse (`403` outside them or in a blocked tree or unwritable, `404` missing parent, `409` exists, `400` a parent that is a file, or a name outside letters, digits, space and `_ - . ~`, containing `..`, starting with a dot or a tilde or over 255 bytes, so every name it accepts is one Link Existing accepts, and `400` a parent that is a cases directory, where a folder is already a case (Add Case > Create New with no custom folder makes one there); `showHidden` is forwarded like browse's); the picker reloads the folder and selects the new one. A picker that browses somewhere this route cannot create in (the container-workdir picker, `fetchListing`) must not set the flag. → [architecture-invariants#filesystem-path-picker](docs/architecture-invariants.md#filesystem-path-picker) -**File Viewer edit mode** (issue #212): the file-preview overlay edits workspace text files in place: `GET .../file-content?edit=1` + `PUT /api/sessions/:id/file-content`, policy in `src/config/file-editing.ts`. This is a **third file surface and the only one that writes file contents** (the path picker's New folder creates folders only): read-path confinement (realpath + workspace + ownership, where an absolute path inside a followed outside worktree (`SessionState.outsideWorktree`) counts as a second workspace root) plus sensitive/blocked/`.git` denies and an extension **allowlist**; writes are `wx`-temp + rename (no `O_CREAT` anywhere = edit-in-place is structural); optimistic concurrency via sha256 `baseHash` → 409. ⚠️ `edit=1` never truncates and the client must never save a plain-preview buffer (the 500-line truncation would silently delete the rest). ⚠️ CRLF/UTF-8 guards: EOL re-applied server-side, non-UTF-8 refused via round-trip compare. → [architecture-invariants#file-viewer-edit-mode](docs/architecture-invariants.md#file-viewer-edit-mode), `docs/file-viewer-edit-plan.md` +**File Viewer edit mode** (issue #212): the file-preview overlay edits workspace text files in place: `GET .../file-content?edit=1` + `PUT /api/sessions/:id/file-content`, policy in `src/config/file-editing.ts`. This is a **third file surface and the only one that writes file contents** (the path picker's New folder creates folders only): read-path confinement (realpath + workspace + ownership, where an absolute path inside a followed outside worktree (`SessionState.outsideWorktree`) counts as a second workspace root) plus sensitive/blocked/`.git` denies and an extension **allowlist**; dotenv is the one sensitive family it opens (`allowDotenv`, because the plain preview already serves it), so every other credential pattern, a dotenv file inside a blocked tree and the data dir's own `.env` still refuse; writes are `wx`-temp + rename (no `O_CREAT` anywhere = edit-in-place is structural); optimistic concurrency via sha256 `baseHash` → 409. ⚠️ `edit=1` never truncates and the client must never save a plain-preview buffer (the 500-line truncation would silently delete the rest). ⚠️ CRLF/UTF-8 guards: EOL re-applied server-side, non-UTF-8 refused via round-trip compare. → [architecture-invariants#file-viewer-edit-mode](docs/architecture-invariants.md#file-viewer-edit-mode), `docs/file-viewer-edit-plan.md` **Files panel search** (COD-236, the `q` param on `GET /api/sessions/:id/files`): `compileFileQuery()` (`utils/file-query.ts`, pure) compiles the query into a predicate the server-side walk prunes with; a query returns a FLAT match list and the walk recurses past non-matching directories. An empty, whitespace-only or overlong (`MAX_QUERY_LENGTH`, 256) query compiles to `null`, keeping the default tree response byte-identical. ⚠️ **Never compile a glob into a RegExp** (`*a*a*a…` backtracks and freezes the event loop for the whole server): `globMatch()` is a two-pointer wildcard walk. Tree and search share one walk shape: a per-directory round-robin (breadth-first, so fair per directory, not per subtree) over one 5,000-node budget, where a folder's own files count as one share beside the subfolders it just found (`walkTurnEnd()`). ⚠️ That budget caps what is LISTED, never what is read: a read keeps only what the listing can use (`readWalkFolder()`) and `MAX_WALK_ENTRIES_READ` (250,000) stops opening folders, or one GET reads the whole workspace into memory. ⚠️ A folder below the walk's root is probed (#516) before its `readdir`, and anything but `directory` is skipped unflagged. ⚠️ A directory has its node before it is read, so every cursor still queued or unread when the walk ends must get `incomplete: true` or it reads as an empty folder. → [architecture-invariants#files-panel-search](docs/architecture-invariants.md#files-panel-search) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index d280b477..ac9a7980 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -470,7 +470,7 @@ The general rule: **any new endpoint that turns a caller-supplied `sessionId` in **File Viewer edit mode** (issue #212, design in `docs/file-viewer-edit-plan.md`): the file-preview overlay can edit workspace text files in place — `GET /api/sessions/:id/file-content?edit=1` (read-for-edit) + `PUT /api/sessions/:id/file-content` (save), policy in `src/config/file-editing.ts`, UI in `panels-ui.js`. This is the **only file surface that writes**, so it carries every rule the read surfaces have plus its own: -- **Confinement is the read path's, plus write-only gates.** `findSessionOrFail` (ownership) → `validateSessionFilePath` (realpath + workspace boundary; escapes report as 404, same as reads) → sensitive-path + attachment-guard blocklists (403) → `.git/` subtree deny (403 — `.git/hooks/*` is code execution) → extension **allowlist** (400; `svg` and `env` deliberately excluded). ⚠️ **There is no `O_CREAT` anywhere in the handler** — that absence is what makes "edit-in-place only, never create" a structural property instead of a convention. Do not add a create path without treating it as a new security surface. +- **Confinement is the read path's, plus write-only gates.** `findSessionOrFail` (ownership) → `validateSessionFilePath` (realpath + workspace boundary; escapes report as 404, same as reads) → sensitive-path + attachment-guard blocklists (403) → `.git/` subtree deny (403 — `.git/hooks/*` is code execution) → extension **allowlist** (400; `svg` and `env` deliberately excluded from `EDITABLE_EXTENSIONS`). ⚠️ **Dotenv is the one sensitive family the editor opens**: both the gate (`assertEditableTarget`) and the `editable` flag on the plain preview call `isBlockedAttachmentPath(..., { allowDotenv: true })`, which skips only `DOTENV_PATTERNS` in `sensitive-path.ts`, and `isEditableFileName` accepts `.env`, `.env.*` and `*.env` by NAME. ⚠️ Never add `env` to `EDITABLE_EXTENSIONS`: it is also `TEXT_ATTACHMENT_EXTENSIONS`, so it would let cross-workspace attachments read any `prod.env` on the host, which no sensitive pattern matches. The reason is that the plain preview and `file-raw` never consult the blocklist (realpath containment is their control), so refusing the edit protected nothing the preview had not already shown. Every other credential pattern still refuses, so does a dotenv file inside a blocked tree (`.ssh/.env`, a config backup), and the data dir's own `.env` (the `codeman attach` login fallback) has its own entry in `SENSITIVE_PATTERNS` so a session whose workspace is `$HOME` cannot open it. Downloads, attachments and the path picker keep the full list. Keep the two call sites in step. Accepted limits: `/\.env\./` also matches a FOLDER named `.env.`, so files under one become editable too (the preview already shows them); and like every `.codeman[^/]*` entry, the data-dir entry misses a data dir moved by `CODEMAN_DATA_DIR` to a folder not named `.codeman*`, or reached through a symlinked `~/.codeman`, which already left that dir's `users.json`/`settings.json` editable when it sits inside a workspace. ⚠️ **There is no `O_CREAT` anywhere in the handler** — that absence is what makes "edit-in-place only, never create" a structural property instead of a convention. Do not add a create path without treating it as a new security surface. - **A truncated buffer must never become an edit buffer.** The plain preview truncates to `lines` (default 500); saving such a buffer would silently delete everything past the cut, and the hash check cannot catch it (the loaded prefix hashes differently from the full file, which reads as an ordinary conflict at best). `edit=1` therefore never truncates — it 413s over `MAX_EDITABLE_BYTES` (512KB) instead — and the frontend always re-fetches with `edit=1` before swapping in the textarea, even though the preview already holds content. - **Concurrency is optimistic by content hash, not mtime.** The client echoes the sha256 it loaded (`baseHash`); mismatch → 409 CONFLICT (plain envelope — the error arm carries no data; the client re-fetches `edit=1` for fresh state) unless `force:true`. mtime alone is wrong: agents rewrite files within one timestamp tick. - **Writes are `wx` temp + `fchmod` + `fsync` + `rename` in the target's directory.** `wx` cannot follow a pre-existing symlink and `rename()` replaces (not follows) a symlink final component, which closes the validate-then-write TOCTOU window; `fchmod` because `open()`'s mode argument is masked by the umask; a symlink whose target is *inside* the workspace is deliberately written through (validation returns the realpath). Trade-off (same as vim): the inode changes, so hardlinks keep old content. diff --git a/docs/file-viewer-edit-plan.md b/docs/file-viewer-edit-plan.md index af17b744..decfe515 100644 --- a/docs/file-viewer-edit-plan.md +++ b/docs/file-viewer-edit-plan.md @@ -109,7 +109,9 @@ Decisions baked in: - `svg` and `env` are deliberately marked with `?` above: `svg` is served as an untrusted octet-stream on the read side (`file-routes.ts:118`) so allowing an edit is defensible, but I recommend **excluding both** in v1. `.env` files are matched by `isSensitivePath()` anyway and would be rejected downstream; - excluding them at the allowlist keeps a single obvious refusal. + excluding them at the allowlist keeps a single obvious refusal. Superseded for dotenv: the plain + preview serves a workspace `.env` in full, so the editor now opens dotenv files (`allowDotenv`, see + `docs/architecture-invariants.md#file-viewer-edit-mode`); `svg` stays excluded. - `isDeniedEditRelativePath` blocks the `.git/` subtree: `.git/hooks/*` is code execution and a corrupt index is unrecoverable-looking to a user who only wanted to fix a typo. Other dotfiles stay allowed but are not reachable from the tree UI anyway (`showHidden=false`). @@ -363,7 +365,8 @@ Route cases to assert explicitly: 4. symlink pointing inside is written through to the target 5. non-allowlisted extension gives 400 6. `.git/config` gives 403 -7. a `.env` in the workspace gives 403 (sensitive-path) +7. a `.env` in the workspace gives 403 (sensitive-path); superseded: it is now editable, while `.ssh/.env`, + the data dir's `.env` and the other credential files still give 403 8. a file with a NUL byte gives 400 9. a latin-1 file that fails the UTF-8 round-trip gives 400 10. stale `baseHash` gives 409 (`CONFLICT` envelope, no data); `force:true` then succeeds diff --git a/docs/wiki/Working-With-Files.md b/docs/wiki/Working-With-Files.md index c31fca51..79b194d3 100644 --- a/docs/wiki/Working-With-Files.md +++ b/docs/wiki/Working-With-Files.md @@ -69,6 +69,10 @@ convenient: - **`.git/` is denied outright.** Hooks are executable code, and a corrupted index looks unrecoverable to someone who wanted to fix a typo. - **Non-UTF-8 content is refused**, verified by a round-trip comparison. +- **Dotenv files are editable, other credential files are not.** `.env`, `.env.local` and + `prod.env` open in the editor like any config file, and a save keeps the file's permissions + (a `0600` file stays `0600`). `.npmrc`, `credentials.json`, anything under `.ssh/` and + Codeman's own `~/.codeman/.env` stay read-only here. ## Attachments diff --git a/src/config/attachment-guard.ts b/src/config/attachment-guard.ts index a3c536fa..6afea031 100644 --- a/src/config/attachment-guard.ts +++ b/src/config/attachment-guard.ts @@ -126,9 +126,15 @@ export async function loadAttachmentGuardConfig(): Promise isUnderTree(absPath, tree)); } diff --git a/src/config/file-editing.ts b/src/config/file-editing.ts index 67192dc8..b4d166cf 100644 --- a/src/config/file-editing.ts +++ b/src/config/file-editing.ts @@ -8,9 +8,13 @@ * * Design (docs/file-viewer-edit-plan.md): * - ALLOWLIST of text extensions/basenames, not a blocklist — matching the - * attachment-guard precedent. `svg` and `env` are deliberately absent: svg is - * treated as untrusted on the read side, and `.env` is sensitive-path blocked - * anyway; excluding them here keeps a single obvious refusal. + * attachment-guard precedent. `svg` and `env` are deliberately absent from + * the extension list: svg is treated as untrusted on the read side, and the + * list doubles as the attachment list, which must not gain `*.env`. Dotenv + * files (`.env`, `.env.*`, `*.env`) are editable by NAME in + * isEditableFileName instead: the plain preview already shows them, and the + * routes pass `allowDotenv` to the sensitive-path check, which keeps every + * other credential pattern. * - The `.git/` subtree is denied outright: `.git/hooks/*` is code execution and * a corrupted index looks unrecoverable to a user who wanted to fix a typo. * - EOL helpers exist because a browser