mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 02:29:33 +01:00
Merge #797: the File Viewer edits workspace dotenv files
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
commit
18b309fc02
13 files changed
+417
-34
No files matched your search
@@ -482,7 +482,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 file contents** (the Files panel's delete, [below](#files-panel-delete), removes whole files and borrows these confinement rules), 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.<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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -76,6 +76,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
|
||||
|
||||
|
||||
Reference in new issue
Block a user