From 24ed43935c2e0a1669263c9c9b3e50c088080b98 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 16 Aug 2026 20:34:34 +0200 Subject: [PATCH] fix: file-link and session-sidebar review follow-ups from 1.19.0 Five post-merge review items from PRs #306 (clickable file paths) and #307 (session sidebar): - constants.js FILE_PREVIEW_EXTENSIONS gains the media extensions it was missing vs the single-source sets in attachment-registry.ts (m4v ogv ogg oga m4a aac flac opus), so an in-workspace .m4a opens the preview player instead of the log viewer; new test/media-extension-parity.test.ts pins all three copies (constants.js, panels-ui.js, attachment-registry.ts) against each other. - FILE_PATH_LINK_PATTERN drops `etc` from its root alternation: /etc is unconditionally in DEFAULT_BLOCKED_TREES, so every /etc link 403'd. Negative cases added to the link-provider and response-viewer tests. - updateSidebarCount() counts the rows actually on the sidebar list (session rows + web-tab rows, minus filtered-out ones) instead of this.sessions.size, and applySidebarFilter() refreshes it so the count follows the filter box per keystroke. - The incremental-render connection-line gate now also fires in sidebar layout (this._lineageEdgeCount is permanently 0 there), matching the strip-scroll listener widened in #307, so a badge changing row heights redraws subagent/ultracode connectors. - isSensitivePath() blocks ~/.claude.json, ~/.claude/settings.json and ~/.claude/settings.local.json (credential-bearing by schema), anchored to homedir() read at check time so case-level .claude/settings*.json files stay servable in the File Viewer. Co-Authored-By: Claude Fable 5 --- src/web/public/app.js | 28 +++++++-- src/web/public/constants.js | 14 ++++- src/web/public/panels-ui.js | 3 + src/web/sensitive-path.ts | 21 ++++++- test/link-provider-regex.test.ts | 17 ++++++ test/media-extension-parity.test.ts | 75 +++++++++++++++++++++++++ test/response-viewer-file-links.test.ts | 10 ++++ test/sensitive-path.test.ts | 31 ++++++++++ test/session-list-layout.test.ts | 23 +++++++- 9 files changed, 209 insertions(+), 13 deletions(-) create mode 100644 test/media-extension-parity.test.ts diff --git a/src/web/public/app.js b/src/web/public/app.js index f3b502b93..96b8cf51e 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -3826,7 +3826,6 @@ class CodemanApp { // Collapse/expand changes whether the filter is reachable, so re-evaluate it // here too — not only at the render tails. this.applySidebarFilter(this._sidebarFilter); - this.updateSidebarCount(); this.updateConnectionLines(); // The desktop home rail defers to the sidebar (both dock the session list // flush left), so a layout flip while the welcome screen is up has to @@ -3871,9 +3870,22 @@ class CodemanApp { this.toggleSessionSidebar(); } + /** + * The count is what is actually ON the list: session rows plus web-tab rows, + * minus whatever the sidebar filter is hiding. `this.sessions.size` was the + * original source and disagreed with the screen twice over — web tabs render + * in the same list but are not sessions (3 sessions + 2 dashboards read "3" + * above 5 rows), and a filter hides rows without touching the map. Counting + * the rendered rows keeps one source of truth: the list itself. + */ updateSidebarCount() { const el = document.getElementById('sessionSidebarCount'); - if (el) el.textContent = String(this.sessions?.size ?? 0); + if (!el) return; + const container = this.$('sessionTabs'); + const count = container + ? container.querySelectorAll('.session-tab:not(.tab-filtered-out)').length + : (this.sessions?.size ?? 0); + el.textContent = String(count); } /** @@ -3906,6 +3918,9 @@ class CodemanApp { const haystack = `${tab.getAttribute('aria-label') || ''} ${tab.getAttribute('title') || ''}`.toLowerCase(); tab.classList.toggle('tab-filtered-out', !haystack.includes(needle)); } + // The count shows visible rows, so it moves with every filter change — + // including keystrokes in the filter box, which call this directly. + this.updateSidebarCount(); } // ═══════════════════════════════════════════════════════════════ @@ -4262,11 +4277,13 @@ class CodemanApp { // The full-render path already redraws the connection SVG; this incremental // one does not, and a badge appearing widens a tab and shifts every tab after // it, sliding the lineage arcs off their anchors. Only pay for it when there - // is an arc to keep anchored. - if (this._lineageEdgeCount > 0) this.updateConnectionLines(); + // is something anchored to tab rects: lineage arcs, or — in sidebar layout, + // where lineage is skipped and the edge count stays 0 — the subagent/ + // ultracode connectors, whose rows a badge changes the HEIGHT of. Same + // widening as the strip-scroll listener in session-lineage.js. + if (this._lineageEdgeCount > 0 || this.isSessionSidebarActive()) this.updateConnectionLines(); this.applySidebarFilter(this._sidebarFilter); - this.updateSidebarCount(); } // Auto-wrap desktop session tabs to a second row when they overflow one row, @@ -4467,7 +4484,6 @@ class CodemanApp { // innerHTML was rebuilt wholesale, so the sidebar filter classes are gone — // re-apply them or filtered-out sessions flicker back on every SSE tick. this.applySidebarFilter(this._sidebarFilter); - this.updateSidebarCount(); } // Set up arrow key navigation for session tabs (accessibility) diff --git a/src/web/public/constants.js b/src/web/public/constants.js index ab3844e32..4b97711e2 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -997,13 +997,16 @@ function computeRewriteScrollLine(input) { * never match) and terminated by a known extension (so the end of the path is * unambiguous — a trailing `)` or `.` after the extension stays out). Longer * extensions come first in each family (`tsx|ts`), so the trailing `\b` cannot - * be satisfied by the shorter branch mid-word. + * be satisfied by the shorter branch mid-word. `/etc` is deliberately NOT a + * root: DEFAULT_BLOCKED_TREES (config/attachment-guard.ts) refuses the whole + * tree server-side, so every `/etc/...` link was a guaranteed 403 — a link + * that renders clickable and then dies is worse than plain text. * * ⚠ Consumers must never share one instance: `lastIndex` is per-object state on * a `/g` regex, so {@link absoluteFilePathPattern} mints a fresh one per call. */ const FILE_PATH_LINK_PATTERN = - /(\/(?:home|Users|tmp|var|private|etc|opt|mnt|srv|media|data|workspace)\/[^\s"'<>|;&\n\x00-\x1f]*\.(?:log|txt|json|md|ya?ml|csv|xml|sh|py|tsx|ts|jsx|js|mjs|cjs|css|html|toml|ini|sql|png|jpe?g|gif|webp|bmp|svg|pdf|docx|pptx|mp4|webm|mov|mp3|wav))\b/g; + /(\/(?:home|Users|tmp|var|private|opt|mnt|srv|media|data|workspace)\/[^\s"'<>|;&\n\x00-\x1f]*\.(?:log|txt|json|md|ya?ml|csv|xml|sh|py|tsx|ts|jsx|js|mjs|cjs|css|html|toml|ini|sql|png|jpe?g|gif|webp|bmp|svg|pdf|docx|pptx|mp4|webm|mov|mp3|wav))\b/g; /** A fresh, zero-state instance of {@link FILE_PATH_LINK_PATTERN}. */ function absoluteFilePathPattern() { @@ -1014,9 +1017,14 @@ function absoluteFilePathPattern() { * Extensions the file-preview overlay renders itself. Everything else a link * points at goes to the tail/log viewer, which is the right home for a growing * text file and the wrong one for bytes (tailing a PNG shows binary noise). + * + * The media entries mirror VIDEO_ATTACHMENT_EXTENSIONS/AUDIO_ATTACHMENT_EXTENSIONS + * (src/attachment-registry.ts, the single source) — they diverged once and an + * in-workspace `.m4a` opened as binary noise in the log viewer while the same + * file in /tmp played fine. test/media-extension-parity.test.ts pins the sync. */ const FILE_PREVIEW_EXTENSIONS = new Set( - ('png jpg jpeg gif webp bmp svg pdf docx pptx mp4 webm mov mp3 wav').split(' ') + ('png jpg jpeg gif webp bmp svg pdf docx pptx mp4 webm mov m4v ogv mp3 wav ogg oga m4a aac flac opus').split(' ') ); /** Whether a path's extension is one {@link FILE_PREVIEW_EXTENSIONS} covers. */ diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 2750e94d8..1676a541e 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -3346,6 +3346,9 @@ Object.assign(CodemanApp.prototype, { if (attachmentId) { const base = `/api/sessions/${sessionId}/attachments/${encodeURIComponent(attachmentId)}`; const IMAGE_EXTS = new Set(['png', 'jpg', 'jpeg', 'gif', 'webp', 'bmp', 'svg']); + // VIDEO/AUDIO mirror VIDEO_ATTACHMENT_EXTENSIONS/AUDIO_ATTACHMENT_EXTENSIONS + // (src/attachment-registry.ts, the single source); the frontend cannot import + // it, so test/media-extension-parity.test.ts pins the copies equal. const VIDEO_EXTS = new Set(['mp4', 'webm', 'mov', 'm4v', 'ogv']); const AUDIO_EXTS = new Set(['mp3', 'wav', 'ogg', 'oga', 'm4a', 'aac', 'flac', 'opus']); // Size when we just registered the file ourselves, so a path opened from a diff --git a/src/web/sensitive-path.ts b/src/web/sensitive-path.ts index 29caede80..ecaaa1aa3 100644 --- a/src/web/sensitive-path.ts +++ b/src/web/sensitive-path.ts @@ -29,6 +29,9 @@ * symlink pointing at a sensitive target is also caught. */ +import { homedir } from 'node:os'; +import { join } from 'node:path'; + const SENSITIVE_PATTERNS: RegExp[] = [ // System account databases. /^\/etc\/shadow$/, @@ -99,10 +102,26 @@ const SENSITIVE_PATTERNS: RegExp[] = [ /\/\.codeman[^/]*\/intents\.json$/, ]; +/** + * Claude config members that are credential-bearing ONLY under the user's real + * home directory: `~/.claude/settings.json` can hold `env.ANTHROPIC_API_KEY` + * and `apiKeyHelper` by schema (settings.local.json shares that schema), and + * `~/.claude.json` holds account/OAuth-adjacent state. A blanket + * `/\.claude\/settings\.json$/` would also block every CASE-level + * `.claude/settings.json`, which users legitimately view and edit in the File + * Viewer (model override, hooks) — so these are anchored to homedir(), read at + * CHECK time inside isSensitivePath, never captured at module load (wrong for + * anything that changes HOME later, e.g. per-file test fixtures — same + * reasoning as the `.ssh/` note above). + */ +const HOME_SENSITIVE_MEMBERS = ['.claude.json', '.claude/settings.json', '.claude/settings.local.json']; + /** * Returns true if the given ABSOLUTE, symlink-resolved path matches the * sensitive-file blocklist and must not be served to the browser. */ export function isSensitivePath(absPath: string): boolean { - return SENSITIVE_PATTERNS.some((pattern) => pattern.test(absPath)); + if (SENSITIVE_PATTERNS.some((pattern) => pattern.test(absPath))) return true; + const home = homedir(); + return HOME_SENSITIVE_MEMBERS.some((member) => absPath === join(home, member)); } diff --git a/test/link-provider-regex.test.ts b/test/link-provider-regex.test.ts index ed556337a..5aa9f97a5 100644 --- a/test/link-provider-regex.test.ts +++ b/test/link-provider-regex.test.ts @@ -150,6 +150,23 @@ describe('terminal link-provider regexes (shipped source)', () => { } }); + it('the file-path pattern refuses /etc roots (blocked server-side, so the link could only 403)', () => { + // `/etc` sits in DEFAULT_BLOCKED_TREES (config/attachment-guard.ts), so an + // /etc link is guaranteed dead: it renders clickable, then the preview 403s. + // It used to be in the root alternation, which linked exactly those paths. + const ext = shippedPattern('FILE_PATH_LINK_PATTERN'); + const cases = [ + 'see /etc/hosts here', + // Extension-bearing, so only the root removal keeps it out. + 'see /etc/app/config.json here', + 'cat /etc/nginx/nginx.conf.txt', + ]; + for (const line of cases) { + ext.lastIndex = 0; + expect(ext.exec(line), line).toBeNull(); + } + }); + it('terminal-ui builds its path pattern from the shared factory', () => { // Structural guard: a local literal here would drift from the response // viewer's linkifier, which is the divergence the move exists to prevent. diff --git a/test/media-extension-parity.test.ts b/test/media-extension-parity.test.ts new file mode 100644 index 000000000..924eb2fd9 --- /dev/null +++ b/test/media-extension-parity.test.ts @@ -0,0 +1,75 @@ +/** + * @fileoverview Media-extension parity — attachment registry ⇄ frontend copies. + * + * CLAUDE.md single-sources playable media extensions in + * `VIDEO_ATTACHMENT_EXTENSIONS`/`AUDIO_ATTACHMENT_EXTENSIONS` + * (src/attachment-registry.ts): the workspace preview and the out-of-workspace + * attachment path must agree on what plays. The frontend cannot import that + * module, so two hand-maintained copies exist and BOTH have drifted: + * + * - `FILE_PREVIEW_EXTENSIONS` (constants.js) decides whether a clicked + * terminal/chat path opens the preview overlay or the tail/log viewer. It + * was missing `m4v ogv ogg oga m4a aac flac opus`, so an in-workspace + * `.m4a` routed to the log viewer and rendered as binary noise while the + * same file in /tmp played fine. + * - `VIDEO_EXTS`/`AUDIO_EXTS` (panels-ui.js) pick the