From 59d4bbca2d6c39bac932df1d2c7192ea326b51b7 Mon Sep 17 00:00:00 2001
From: Codeman maintainer <noreply@anthropic.com>
Date: Wed, 11 Nov 2026 20:25:37 +0100
Subject: [PATCH] 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>
---
 docs/architecture-invariants.md               |  2 +-
 docs/wiki/Working-With-Files.md               |  7 ++--
 src/web/routes/file-routes.ts                 | 13 ++++++++
 src/web/sensitive-path.ts                     |  5 ++-
 .../file-routes-outside-worktree.test.ts      | 32 +++++++++++++++++++
 test/routes/file-write-routes.test.ts         | 27 +++++++++++++++-
 test/sensitive-path.test.ts                   |  9 ++++++
 7 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md
index 09d418c1..0fe95023 100644
--- a/docs/architecture-invariants.md
+++ b/docs/architecture-invariants.md
@@ -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.
 
diff --git a/docs/wiki/Working-With-Files.md b/docs/wiki/Working-With-Files.md
index cb988131..8ea81756 100644
--- a/docs/wiki/Working-With-Files.md
+++ b/docs/wiki/Working-With-Files.md
@@ -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
 
diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts
index 473cf6c6..580dd0c2 100644
--- a/src/web/routes/file-routes.ts
+++ b/src/web/routes/file-routes.ts
@@ -2455,6 +2455,13 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
       // masked by the process umask; fsync so the rename never publishes a
       // partially-durable file. Trade-off (same as vim's default): the inode
       // changes, so hardlinks keep the old content.
+      //
+      // The new file would also take this process's own user and group, so a
+      // `.env` shared by group (you:www-data, 0640) came back you:you and the
+      // web server lost it. fchown puts both back where the account may: the
+      // owner can give a file to a group they belong to, and only root can set
+      // the user. Best effort, since a save that cannot keep the group is still
+      // the save the user asked for.
       const fileMode = stat.mode & 0o777;
       const tmpPath = join(
         dirname(resolvedPath),
@@ -2463,6 +2470,12 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
       let handle;
       try {
         handle = await fs.open(tmpPath, 'wx', fileMode);
+        const tmpHandle = handle;
+        await tmpHandle
+          .chown(stat.uid, stat.gid)
+          .catch(() => tmpHandle.chown(-1, stat.gid))
+          .catch(() => {});
+        // After the chown: changing the owner can clear mode bits.
         await handle.chmod(fileMode);
         await handle.writeFile(outBuf);
         await handle.sync();
diff --git a/src/web/sensitive-path.ts b/src/web/sensitive-path.ts
index f3c97d42..a39f3cfb 100644
--- a/src/web/sensitive-path.ts
+++ b/src/web/sensitive-path.ts
@@ -85,7 +85,10 @@ const SENSITIVE_PATTERNS: RegExp[] = [
   // The data dir's `.env` is the CODEMAN_USERNAME/PASSWORD fallback `codeman attach`
   // reads. Named here as well as in DOTENV_PATTERNS so `allowDotenv` (the File
   // Viewer editor) cannot reach it from a session whose workspace is $HOME.
-  /\/\.codeman[^/]*\/\.env$/,
+  // Case-insensitive, unlike its neighbours: the editor's name rule lowercases
+  // (`isEditableFileName`), so `.codeman/.ENV` passes it, and on a case-insensitive
+  // filesystem (the macOS default) that IS this file.
+  /\/\.codeman[^/]*\/\.env$/i,
   // Codeman's own state files. Named once `.json` became previewable outside
   // the workspace: `SessionState.envOverrides` persists whatever the user set
   // for a session, and the env allowlist admits key-shaped names
diff --git a/test/routes/file-routes-outside-worktree.test.ts b/test/routes/file-routes-outside-worktree.test.ts
index 53ce7868..fdca9999 100644
--- a/test/routes/file-routes-outside-worktree.test.ts
+++ b/test/routes/file-routes-outside-worktree.test.ts
@@ -137,6 +137,38 @@ describe('file routes for a worktree outside the session folder', () => {
       expect(plainRaw.body).toBe('plain');
     });
 
+    // The editor's own gate lets a dotenv file through (#796), so for a file in an outside
+    // worktree `validateLocalSessionFilePath()` is the ONLY refusal on the edit path.
+    it('refuses to open or save a dotenv file in the worktree through the editor', async () => {
+      const target = join(sibling, '.env');
+      writeFileSync(target, 'TOKEN=abc');
+      const edit = await harness.app.inject({
+        method: 'GET',
+        url: sessionUrl(`file-content?edit=1&path=${encodeURIComponent(target)}`),
+      });
+      expect(edit.json().success).toBe(false);
+      expect(edit.body).not.toContain('abc');
+
+      const put = await harness.app.inject({
+        method: 'PUT',
+        url: sessionUrl('file-content'),
+        payload: { path: target, content: 'TOKEN=changed', baseHash: sha256('TOKEN=abc') },
+      });
+      expect(put.statusCode).not.toBe(200);
+      expect(put.json().success).toBe(false);
+      expect(readFileSync(target, 'utf8')).toBe('TOKEN=abc');
+
+      // The same file in the session's own folder is the one #796 opened.
+      writeFileSync(join(repo, '.env'), 'TOKEN=abc');
+      const own = await harness.app.inject({
+        method: 'PUT',
+        url: sessionUrl('file-content'),
+        payload: { path: '.env', content: 'TOKEN=changed', baseHash: sha256('TOKEN=abc') },
+      });
+      expect(own.statusCode).toBe(200);
+      expect(readFileSync(join(repo, '.env'), 'utf8')).toBe('TOKEN=changed');
+    });
+
     it('still resolves a relative path against the session folder only', async () => {
       expect((await readContent('src/main-only.ts')).statusCode).toBe(200);
       const escaped = await readContent('../myapp-feature/src/feature-only.ts');
diff --git a/test/routes/file-write-routes.test.ts b/test/routes/file-write-routes.test.ts
index b2dea92d..6a06db94 100644
--- a/test/routes/file-write-routes.test.ts
+++ b/test/routes/file-write-routes.test.ts
@@ -19,6 +19,7 @@ import {
   readFileSync,
   symlinkSync,
   chmodSync,
+  chownSync,
   statSync,
   lstatSync,
   readlinkSync,
@@ -323,7 +324,16 @@ describe('file viewer edit mode (real fs)', () => {
     const original = 'TEST_VALUE=original\nTEST_LABEL=café';
     const updated = 'TEST_VALUE=updated\nTEST_LABEL=été';
     const editablePaths = ['.env', '.env.local', '.env.example', 'config.env', 'app/.env'];
-    const forbiddenPaths = ['.ssh/.env', '.codeman/.env', '.codeman-dev/.env', '.npmrc', 'credentials.json'];
+    const forbiddenPaths = [
+      '.ssh/.env',
+      '.codeman/.env',
+      '.codeman-dev/.env',
+      // Spelled in capitals: the same login file on a case-insensitive filesystem.
+      '.codeman/.ENV',
+      '.CODEMAN/.env',
+      '.npmrc',
+      'credentials.json',
+    ];
 
     const getPreview = (path: string) =>
       harness.app.inject({
@@ -388,6 +398,21 @@ describe('file viewer edit mode (real fs)', () => {
       expect(statSync(path).mode & 0o777).toBe(mode);
     });
 
+    // The save publishes a NEW file. Without help it takes this process's own group, so a
+    // `.env` shared with a web server by group (you:www-data, 0640) came back you:you.
+    const otherGroup = process.getgroups?.().find((gid) => gid !== process.getgid?.());
+    it.skipIf(otherGroup === undefined)('keeps the group of a dotenv file across a save', async () => {
+      const path = join(workDir, '.env');
+      writeFileSync(path, original);
+      chownSync(path, -1, otherGroup!);
+      chmodSync(path, 0o640);
+      const res = await putFile('.env', { content: updated, baseHash: sha256(original) });
+      expect.soft(res.statusCode).toBe(200);
+      expect.soft(readFileSync(path)).toEqual(Buffer.from(updated));
+      expect.soft(statSync(path).mode & 0o777).toBe(0o640);
+      expect(statSync(path).gid).toBe(otherGroup);
+    });
+
     it('rejects a stale dotenv baseHash with 409 and leaves the bytes unchanged', async () => {
       writeFileSync(join(workDir, '.env'), original);
       const res = await putFile('.env', { content: updated, baseHash: sha256('TEST_VALUE=older') });
diff --git a/test/sensitive-path.test.ts b/test/sensitive-path.test.ts
index cc9cbc0e..518f8539 100644
--- a/test/sensitive-path.test.ts
+++ b/test/sensitive-path.test.ts
@@ -25,6 +25,10 @@ const dotenvPaths: Array<[string, string]> = [
   ['dotenv', '/srv/app/.env'],
   ['local dotenv', '/srv/app/.env.local'],
   ['suffixed dotenv', '/srv/app/.env.production'],
+  // Deliberate: `.env.*` is blocked wholesale for downloads and attachments, so even a
+  // committed `.env.example` is refused rather than risking the one repo whose "example"
+  // holds a live key. Only the File Viewer editor lifts it (`allowDotenv`), and only for
+  // a file the same user already reads in the preview.
   ['a dotenv example', '/srv/app/.env.example'],
   ['nested dotenv', '/srv/app/sub/dir/.env'],
 ];
@@ -151,6 +155,11 @@ describe('isSensitivePath', () => {
         '/home/u/.codeman-dev/.env',
         '/home/u/.codeman-beta/.env',
         '/srv/x/.codeman/.env',
+        // The editor's name rule lowercases, and on a case-insensitive filesystem (the macOS
+        // default) these two ARE the file above.
+        '/home/u/.codeman/.ENV',
+        '/home/u/.CODEMAN/.env',
+        '/home/u/.Codeman-Dev/.Env',
       ])('blocks %s', (path) => {
         expect(isSensitivePath(path, options)).toBe(true);
       });