fix(files): the editor never reaches Codeman's own .env by another spelling, and a save keeps the file's group (#797)

The four review items of #797, applied at merge.

The editor's name rule lowercases, the data dir's .env pattern did not:
.codeman/.ENV passed both, and on a case-insensitive filesystem (the macOS
default) that is ~/.codeman/.env, the login file codeman attach reads. A
session whose workspace is the home folder could open and save it. The
pattern is case-insensitive now, pinned in the blocklist table and on the
three editor routes.

A save writes a new file and renames it over the old one, so the file took
the server's own group: a 0640 .env owned you:www-data came back you:you
and the web server lost read access. The save now puts the old user and
group back where the account may (best effort), and the wiki says mode
and group instead of permissions.

Tests: an .env in an outside worktree is refused on edit=1 and PUT too,
now that the editor's own gate lets dotenv through; the comment on why
.env.example is blocked for downloads and attachments is back.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer committed 2026-11-11 20:25:37 +01:00
1 parent 18b309fc02
commit 59d4bbca2d
7 files changed
+89 -6

No files matched your search

+1 -1
View File
@@ -485,7 +485,7 @@ The general rule: **any new endpoint that turns a caller-supplied `sessionId` in
- **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.<x>`, 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.
- **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; before it a best-effort `fchown` back to the old file's user and group (group alone when the user cannot be set), because the rename publishes a NEW file that would take the server's own group, and a `0640` `.env` owned `you:www-data` came back `you:you`; 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.
- **Corruption guards**: NUL-sniff + UTF-8 **round-trip compare** (`Buffer.from(buf.toString('utf8'), 'utf8').equals(buf)`) refuse binary and non-UTF-8 files — decoding latin-1 yields U+FFFD replacements and writing those back destroys the original bytes. EOL is detected server-side and re-applied on save because a `<textarea>` normalizes to LF (a two-line edit of a CRLF file must not become a whole-file diff).
- **Two size caps on the wire**: the Zod `.max()` counts UTF-16 code units (coarse pre-filter, 400) while the handler's `Buffer.byteLength` check enforces the real byte cap (413); the route sets `bodyLimit: 4MB` because JSON escaping can expand 512KB of content past Fastify's 1MB default. Error paths **throw** structured `{statusCode, body}` errors (`throwFileEditError`) rather than returning envelopes — the central preSerialization status-mapping hook is absent from the route-test harness, and 413 has no errorCode mapping at all.
+4 -3
View File
@@ -77,9 +77,10 @@ convenient:
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.
`prod.env` open in the editor like any config file, and a save keeps the file's mode (a
`0600` file stays `0600`) and, where your account may set it, its group, so a `.env` a web
server reads through its group stays readable. `.npmrc`, `credentials.json`, anything under
`.ssh/` and Codeman's own `~/.codeman/.env` stay read-only here.
## Attachments