feat: terminal enhancements, CI pipeline, and bug fixes - #137
Open
chihirokajiwara-AI wants to merge 64 commits into
Open
feat: terminal enhancements, CI pipeline, and bug fixes#137chihirokajiwara-AI wants to merge 64 commits into
chihirokajiwara-AI wants to merge 64 commits into
Conversation
0.1: Canvas RPC method name mismatch — strip canvas. prefix (DONE earlier) 0.2: Buffer canvas:rpc-request in shell preload (pendingRpcRequests) 0.3: Graceful error handling in image thumbnail worker Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
new URL() with npm package paths doesn't resolve in Vite's worker-import-meta-url plugin. Restored ?worker static imports which are effectively lazy since CodeEditorView is React.lazy loaded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The security agent created security.ts and added the calls but the import statement was missing from the built output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tiles that are outside the visible viewport at canvas restore time now show a lightweight placeholder div instead of immediately spawning a webview. The webview is created lazily when the tile scrolls into view, detected via a 200ms-debounced checkDeferredTiles call wired to canvas-viewport's new onViewportChange callback. Terminal tiles are always eagerly restored to preserve PTY session bindings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
will-attach-webview was stripping ALL preloads, breaking internal webviews (terminal, viewer, graph) that need the preload for IPC. Now only strips preloads outside our trusted preload directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
will-attach-webview now checks partition prefix (persist:ws-) to identify browser tiles. Only browser tiles get preload stripped. Internal webviews (terminal, viewer, graph) keep preloads intact. Previous approach (path matching) failed because preloads arrive as file:// URLs in packaged apps, not plain file paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tileAdd for type 'term' was falling through to createFileTile which doesn't handle terminals. Now calls createCanvasTile + spawnTerminalWebview. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously Cmd+=/- zoomed all webContents uniformly. Now zoom is routed to the active panel: canvas viewport zoom when the canvas has focus, nav webview zoom when the navigator has focus. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chromium's default Cmd+=/- zoom was firing alongside the menu accelerator, causing the entire BrowserWindow to zoom uniformly. Now before-input-event blocks those keys so only the menu handler (which routes to the active panel) takes effect. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three-layer fix to prevent the left nav and right canvas from zooming together: 1. registerAccelerator: false on zoom menu items — prevents Electron from handling Cmd+=/- before our code runs 2. before-input-event intercepts zoom keys and calls sendShortcut() directly — single code path, no duplication 3. setVisualZoomLevelLimits(1,1) + setZoomLevel(0) on main window and all webviews — locks out Chromium page zoom entirely Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Terminal webviews (and other internal webviews) must receive raw key events without interception. The before-input-event handler was attached to all webviews including terminals, which could interfere with escape sequences (e.g. arrow keys sending ESC[B arriving as just "B"). Now internal webviews (terminal, viewer, graph — identified by default session) are excluded from the shortcut listener and zoom lock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts all zoom-related changes in index.ts back to the upstream version. The before-input-event interception and setZoomLevel/ setVisualZoomLevelLimits calls were interfering with terminal webview key input (arrow keys, escape sequences). Per-panel zoom feature is deferred until it can be implemented without affecting terminal functionality. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dragging files from Finder into a terminal tile now inserts their shell-escaped absolute paths at the cursor position. Multiple files are space-separated. A blue outline highlights the drop target during a drag operation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cmd+= / Cmd++ increases font size (max 32px), Cmd+- decreases it (min 8px), Cmd+0 resets to the default 12px. Handled inside xterm's custom key event handler so the shortcuts only fire when the terminal has focus and cannot conflict with shell or OS-level bindings. fit.fit() is called after each change to recalculate columns/rows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a find/search bar to the terminal tile triggered by Cmd+F. The bar appears above the terminal content with live match highlighting, Enter/ Shift+Enter for next/previous navigation, and Escape to dismiss. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Webview elements cannot receive OS-level drop events from Finder. Move the drag-drop handler to the tile's contentArea (parent of the webview) in tile-manager.js. Add ptyWrite to shell preload so the shell renderer can send dropped file paths directly to the PTY. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Webviews swallow OS drag events, so the contentArea handlers never
fired. New approach: attach dragenter/dragleave/dragover/drop on the
canvas (#panel-viewer). On dragenter, disable pointer-events on all
webviews so the canvas can detect which terminal tile is under the
cursor via elementFromPoint + closest('.canvas-tile'). On drop,
shell-escape file paths and write to the tile's PTY session.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
elementFromPoint was called after pointer-events were restored on webviews, causing the webview to be the hit target instead of the underlying .canvas-tile div. Moved the restoration after hit-test so the drop correctly identifies the terminal tile. Found by code review audit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the search bar was closed via Cmd+F (toggle off), search highlight decorations were not cleared, unlike the Escape and close-button paths. Found by code review audit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. renderer.js: Merge dragRefCount into shared dragCounter; extract disable/restoreWebviewPointerEvents helpers; fix dnd:dragleave not restoring pointer-events (webviews could get permanently stuck). 2. TerminalTab.tsx: Remove unreachable Ctrl+Shift+C/V block — the earlier Ctrl+C/V checks always match first since key is lowercased. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous attempt broke terminal key input by attaching before-input-event to all webviews. This version: 1. Menu accelerators (registerAccelerator: true) capture Cmd+=/-/0 globally and route to renderer via sendShortcut 2. Chromium built-in zoom blocked ONLY on the main BrowserWindow's webContents — child webviews (terminal, nav, etc.) are untouched 3. Renderer routes zoom based on activeSurface: - "canvas": viewport zoom (tiles scale) - "canvas-tile": skip (xterm handles font zoom internally) - "nav"/other: nav webview zoom 4. No setZoomLevel/setVisualZoomLevelLimits on any webview 5. applyZoomToAll/globalZoomLevel removed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause found by Opus analysis: the canvas-level and window-level drop handlers were competing, and the window-level dragenter never called disableWebviewPointerEvents(), so elementFromPoint always returned the webview instead of the .canvas-tile div underneath. Fix: - Remove redundant canvas-level drag handlers entirely - Add disableWebviewPointerEvents() to window-level dragenter - Add restoreWebviewPointerEvents() to window-level dragleave - In window-level drop: hit-test with elementFromPoint BEFORE restoring pointer-events; if target is a terminal tile, insert shell-escaped paths via ptyWrite and return early; otherwise fall through to existing tile-creation logic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a note tile has no file path, the viewer would receive tilePath=undefined in the URL, triggering an ENOENT error on readFile. Now uses URLSearchParams and only sets tilePath when it's a valid non-empty string. Also strengthened the viewer-side guard to reject string literals "undefined" and "null". Upstream: collabs-inc#130 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… leak - Pin Bun version to 1.3.11 (1.3.13 has Set resolution issues) - Narrow CI test scope to pure-logic tests only (packages/shared, packages/components, select src/main files) - Fix files.test.ts mock that leaked incomplete IMAGE_EXTENSIONS Set into image.test.ts when running in same process Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Thank you for your submission! Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by posting the following comment on this PR: I have read the Contributor License Agreement (CLA) and hereby sign the CLA. 0 out of 2 committers have signed the CLA. |
…y audit advisory) Installed esbuild 0.25.12/0.27.7 (via electron-vite/tsx/vite) were flagged by bun audit. Top-level override pins the patched 0.28.1. bun audit count 36 to 34. Gated: electron-vite build OK (identical bundle sizes); CI test subset 60 pass 0 fail; tsc error count unchanged (173 pre-existing). esbuild is build-time only so the build fully validates it. Remaining collab-electron advisories tracked in product-dev task collabs-inc#19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ity advisories) bun audit flagged electron >=40.0.0 <40.8.0 with 5 high advisories (use-after-free in offscreen paint / permission callbacks / PowerMonitor, context-isolation bypass, renderer switch injection). 40.10.3 is the latest release in the current major (40), so no breaking API changes; it also pulls patched transitive deps. bun audit count: 34 to 16 (5 high + 12 moderate cleared). Gated (matches CI): electron-vite build OK; CI test subset 60 pass 0 fail; node-pty native rebuild OK against the new electron ABI. Remaining: critical form-data + the @postlight/parser cluster — see product-dev task collabs-inc#19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…visories) bun audit flagged dompurify 3.2.7 (>=3.1.3 <=3.3.1, via monaco-editor) for two moderate XSS issues (prototype-pollution + mutation-XSS). A second instance was already on 3.4.2. Override pins all dompurify to the latest 3.x (3.4.10) — same major, API-stable, newer-is-safer for a sanitizer. bun audit count: 16 to 8. Gated: electron-vite build OK (dompurify is bundled); CI test subset 60 pass 0 fail. Remaining: critical form-data + the @postlight/parser cluster — see product-dev task collabs-inc#19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…visory) bun audit flagged protobufjs 7.5.7 (<=7.5.7, via posthog-js opentelemetry export) for a moderate DoS (unbounded recursive descriptor expansion). 7.6.4 is the latest in the 7.x line and satisfies the consumer range ^7.3.0 (no major bump). bun audit count: 8 to 7. Gated: electron-vite build OK; CI test subset 60 pass 0 fail. Remaining are the critical form-data + @postlight/parser cluster (needs parser replacement) and tmp (packaging-time) — see product-dev task collabs-inc#19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…visory) bun audit flagged tmp 0.2.5 (<0.2.6, single instance via electron-builder -> @malept/flatpak-bundler) for a high path-traversal. Override pins the patched 0.2.7. Single instance + patch bump (API-stable); the flatpak path is not a build target (mac-zip/win-nsis/linux-AppImage), so the gated surface is unaffected. bun audit count: 7 to 6. Gated: electron-vite build OK; CI test subset 60 pass 0 fail. All 6 remaining vulns are now the @postlight/parser cluster (import-service.ts) — needs the maintained-extractor replacement decision. See product-dev task collabs-inc#19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-electron overrides) Add form-data 4.0.6 override: eliminates critical GHSA-fjxv-7rqg-78g4 (unsafe random boundary) and high GHSA-hmw2-7cc7-3qxx (CRLF injection). Both advisories affect form-data <2.5.4. postman-request (via @postlight/parser) uses form-data ~2.3.2 transitively; empirically verified compatible via import-service.test.ts (21/21 pass) which exercises the full Parser.parse() path without multipart upload surface. Bump dompurify override 3.4.10 → 3.4.11: patches GHSA-cmwh-pvxp-8882 (moderate, permanent ALLOWED_ATTR pollution via setConfig() — incomplete fix of 3.4.7 patch). Net: 27 → 23 vulns (1 critical → 0, 9 high → 7, 12 moderate → 11). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m-498p) GHSA-6rw7-vpxm-498p: qs < 6.14.1 allows DoS via memory exhaustion through arrayLimit bypass in bracket notation. Fix requires qs >= 6.14.1. Consumer chain: @postlight/parser → postman-request → qs (~6.5.2 declared). bun overrides hard-pins to 6.15.2, collapsing all consumers to a single patched resolution (same as bot-platform overrides). Verified compatible: import-service.test.ts 21/21 pass (exercises full Parser.parse() chain). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Patches GHSA-h67p-54hq-rp68 (js-yaml quadratic-complexity DoS via merge key handling with repeated aliases) and GHSA-vmf3-w455-68vh (tar PAX size override causing parser interpretation differential / file smuggling). Both packages are build-time only (electron-builder / node-gyp chains). Overrides satisfy existing semver constraints (^4.1.0 and ^7.5.7/^7.5.4). Vuln count: 11 → 8 (3 moderates removed). Existing 32 test failures in collab-electron are pre-existing DOM-env issues (confirmed identical before/after via git stash). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h-check/tough-cookie in collab-electron bun audit flagged 4 vulnerabilities (vite high, lodash.pick high, uuid moderate, launch-editor moderate via vite). Pinned fixed versions via package.json overrides and regenerated bun.lock (gitignored, not committed) — bun audit now reports only 1 residual: lodash.pick has no upstream fixed release (last publish 4.4.0, still vulnerable), deep transitive via @postlight/parser > cheerio, needs a package replacement rather than a version bump — tracked as follow-up. Verified no regression: bun test is 250 pass / 37 fail / 6 errors identically before and after (pre-existing tmux/DOM environment gaps, unrelated to this change). Independent adversarial audit (sonnet, default-REJECT): APPROVE.
…Dependencies GHSA-p6mc-m468-83gw (HIGH) — lodash.pick@4.4.0 in transitive chain @postlight/parser → cheerio → lodash.pick. No fixed version of lodash.pick exists; @postlight/parser cannot be removed (article import feature). Fix: add basePickBy key guard to skip __proto__, constructor, prototype. bun install will now apply patches/lodash.pick@4.4.0.patch automatically. bun audit still reports this advisory (version-based) but runtime code is safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ager.test.ts to CI panel-manager.test.ts failed under bun test with "ReferenceError: document is not defined" and was never in CI's test path whitelist, so it silently had zero coverage. Add @happy-dom/global-registrator as a devDependency, preload it via bunfig.toml's [test] section, and include the test file in the CI test job's run command.
The repo-wide js-yaml override (14f00d2, pinning to 4.3.0 for GHSA-h67p-54hq-rp68 / GHSA-vmf3-w455-68vh) broke front-matter@4.0.2, which calls js-yaml's `safeLoad` (removed in js-yaml 4.x — the method now exists only as a stub that throws "removed in js-yaml 4. Use yaml.load instead"). viewer-item.ts's try/catch silently swallowed that throw, so frontmatter parsing silently degraded to plain body text for every note, which is exactly the failure real GitHub Actions CI hit (run 28754730411, 5 failing assertions in viewer-item.test.ts). Bun's `overrides` field does not support npm/yarn-style nested/scoped overrides (confirmed against Bun docs), so pinning front-matter's own js-yaml to a different (3.x) version tree-wide isn't possible. Instead, patch front-matter itself (via the same patchedDependencies mechanism already used for lodash.pick) to detect the installed js-yaml major version and always call `load` on 4.x, where it is safe-by-default and the unsafe variant no longer exists. viewer-item.test.ts: 10 pass / 5 fail -> 15 pass / 0 fail. Full CI test-job command: 144 pass / 0 fail. js-yaml still resolves to a single 4.3.0 across the whole tree (satisfies electron-builder chain's ^4.1.0 requirement); bun audit shows no js-yaml vulnerability. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…der happy-dom canvas-viewport.js reads window.shellApi.getPlatform() at module top-level; happy-dom doesn't provide this Electron preload API, so importing the module crashed before any test ran (16 tests, 0 executed). Stub it before the dynamic import, mirroring the pattern in panel-manager.test.ts. Not yet in CI's test scope (.github/workflows/ci.yml) — this is a coverage fix, not a CI-gating one. 16 pass / 0 fail in isolation; 160 pass / 0 fail combined with the existing 144-test CI scope (no regression, order-independent — verified 3 orderings).
39 passing tests (pure-logic, no Electron/DOM/native deps) were never included in the CI test-path whitelist, so regressions in tile-renderer.js and webview-factory.js went undetected. Same coverage-gap class fixed for panel-manager.test.ts the day before (4eb8828). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…solation leak SidecarServer.sessionSocketPath() ignored the injected sessionSocketDir option and always resolved sockets via the hardcoded real ~/.collaborator path (protocol.ts buildSessionSocketPath). Production behavior is unchanged (entry.ts always configures sessionSocketDir to that same default), but server.test.ts's sandboxed temp dir was never actually used — tests were creating/listening on sockets in the real user directory, causing ENOENT/EACCES depending on environment (11-27 test failures depending on runner). Fix: POSIX path now joins this.opts.sessionSocketDir with the session id, matching the already-existing mkdirSync(this.opts.sessionSocketDir) in start(). Win32 (named pipe, OS-global namespace) untouched. Verified: npx tsx --test src/main/sidecar/server.test.ts now 13 pass/0 fail/1 skip (was 11 fail). CI-scoped `bun test` set unaffected (183 pass/0 fail, unchanged). No new tsc errors. Independent adversarial audit: APPROVE. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ests typecheck job (continue-on-error, silently masked) was failing TS2307 on tile-renderer.test.ts/webview-factory.test.ts's `bun:test` import — project never declared bun-types. Added bun-types@1.3.11 (pinned to CI's bun version) and explicit tsconfig.web.json `types` list (preserving existing react/d3 auto-includes). Verified: 14 TS2307 errors resolved, 0 new errors introduced (diffed tsc output pre/post), CI-mirror test suite unchanged at 183 pass/0 fail.
…manager.test.ts vitest is not a dependency of this repo (all other *.test.ts files already use bun:test); the file only worked at runtime because bun's test globals happened to tolerate the unused vi import, but tsc flagged "Cannot find module 'vitest'". Swapped to native bun:test APIs (describe/test/expect/mock/beforeEach) — mock() is functionally equivalent to vi.fn() (same .mockResolvedValue()/toHaveBeenCalledWith support). Verified: 4/4 tests pass standalone, 183/183 pass across full CI-mirror scope (unchanged), vitest-related typecheck error eliminated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wires bun audit --audit-level=high into CI so future new dependency vulnerabilities fail the build. Suppresses only GHSA-p6mc-m468-83gw (lodash.pick prototype pollution via cheerio@0.22.0, transitively required by unmaintained @postlight/parser@2.2.3) after verifying the vulnerable code path (cheerio's getCss() calling _.pick when the css() argument is an Array) is unreachable: every .css() call in @postlight/parser passes a string, and this app's own code never calls .css() at all. Independently re-verified and adversarially reviewed (APPROVE) before commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rename path-traversal guard fs:rename's inline filename-sanitization regex (strips illegal chars, trailing dot) was the only guard preventing a crafted title from escaping the target directory via path.join() in fsRename, and had zero test coverage. Extract it as a dependency-free pure function (file-title.ts) so it's testable without mocking electron, and add 8 unit tests covering illegal-char stripping, path-traversal neutralization, and edge cases. Wire the new test into ci.yml's curated test list so it actually runs in CI. An earlier version of this change mocked electron via bun's process-global mock.module() to test the ipc handler directly, which silently broke update-manager.test.ts's own electron mock when run together in the CI-mirror suite -- the pure-function extraction avoids that class of bug entirely. Verified via CI-mirror command (191 pass, up from 183) and full bun test (no new failures; pre-existing tmux/pty failures unchanged). Independently adversarially audited (APPROVE) before commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y check isPathWithinDirectory is a path-containment guard used at 4 call sites (workspace-graph.ts + workspace-graph-python.ts) to decide whether a resolved import path may be linked into the workspace dependency graph — the same class of security-relevant check as @collab/shared/path-utils's workspaceRootMatch (which already has tests), but this one had zero coverage. Add 8 unit tests including the trickiest case, a sibling directory that shares the target directory's name as a string prefix (e.g. "/workspace-other" vs "/workspace"), which a naive startsWith() check would wrongly treat as contained -- confirms the actual relative()-based implementation is not vulnerable to that. No production code changed; workspace-graph.ts has no electron dependency so this test carries none of the cross-file mock-pollution risk from an earlier iteration. Wired into ci.yml's curated test list. Verified via CI-mirror (199 pass, up from 191) and full bun test (no new failures). Independently adversarially audited (APPROVE) before commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exports the previously-private withOptionalFields (used at 6 call sites across createSession/reconnectSession/discoverSessions) and adds unit tests for its non-obvious semantics: only `undefined` fields are skipped, while null/false/0/'' are still assigned. A regression here would silently corrupt session metadata across multiple code paths. Wired into the CI test job's explicit file list. Independently adversarially audited (APPROVE) before commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
security.ts exported isNavigationAllowed (blocks javascript:/data:/file:/
blob: protocol navigation) but it was never imported anywhere in the repo.
index.ts's own will-navigate handler only handled http/https external
redirects, so those four protocols could navigate internal webContents
(terminal-tile/viewer-tile/graph-tile/shell/settings) that carry preload
scripts with privileged IPC bridge access unblocked.
Confirmed live vector: Markdown.tsx passes urlTransform={(url) => url} to
react-markdown, disabling its default link sanitizer, so markdown-authored
<a href="javascript:...">/file:/data: links in viewer/note tiles could
reach the DOM. Browser-tile webviews are intentionally excluded (they
legitimately encounter blob:/data: URLs during normal external browsing
and are already sandboxed+contextIsolated+no-preload).
Adds unit tests for isNavigationAllowed (zero coverage despite now being
load-bearing) and wires the new test file into CI. Independently
adversarially audited (APPROVE) — auditor built a real Electron 40.10.3
harness to confirm will-navigate never fires for the app's own file://
tile-loading (initial webview src / did-navigate only), so this cannot
regress legitimate internal navigation.
Follow-up tracked separately: setupPermissionHandler and
setupWebviewSecurity in the same file remain unwired dead code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ermission requests Electron auto-grants all permission requests (camera/mic/geolocation/ notifications) unless setPermissionRequestHandler is set explicitly. setupPermissionHandler() in security.ts implemented the deny-all handler but had zero call sites — untrusted browser-tile content (persist:browser / persist:ws-* sessions) could silently request and receive camera/mic access with no user prompt. Wired into web-contents-created, scoped to sessions that don't share session.defaultSession, so the main window and internal webviews (terminal/viewer/graph) keep Electron's default-allow behavior for clipboard read/write. An earlier unscoped version was caught by independent audit (Electron 40.10.3 runtime harness) breaking clipboard copy/paste app-wide; this version was re-verified by the same harness to preserve clipboard on defaultSession while still denying permission requests on persist:browser / persist:ws-* partitions. Added 3 unit tests for setupPermissionHandler in security.test.ts. CI-equivalent suite: 218 pass / 0 fail. No new typecheck errors.
setupWebviewSecurity locked down webview attachment for browser tiles (persist:ws-* partitions — strips preload, forces nodeIntegration=false/ contextIsolation=true/sandbox=true) but had zero call sites, same pattern as the two guards fixed earlier today (isNavigationAllowed, setupPermissionHandler in 6a8599e/c6eacf3). Browser-tile webviews could attach with default, unsandboxed webPreferences. Dropped the function's own setWindowOpenHandler(deny-all) call before wiring it in: index.ts's web-contents-created handler already implements a more precise window-open policy (allow browser-tile popups, forward external links, deny rest), and Electron keeps only the last-registered setWindowOpenHandler — calling both would have silently clobbered the existing, correct policy. Added 4 unit tests, including one asserting setWindowOpenHandler is NOT touched by this function. Verified: bun test src/main/security.test.ts (17 pass/0 fail) and full CI scope (222 pass/ 0 fail). Independent adversarial audit (sonnet, default-REJECT): APPROVE — only call site is the new one, no id/session/partition interaction bug. Local main now 16 commits ahead of origin/main; push still gated on CEO per D-62 (product-dev has no push permission on main).
isBrowserTileWebview() compared wc.session against session.fromPartition
("persist:browser") literally, but real browser tiles use partition
persist:ws-<hash> (tile-manager.js:289) — persist:browser is only used
for a small OAuth popup override and a one-time user-agent tweak. This
meant browser-tile keyboard shortcuts and canvas-forwarding never fired
for real tiles, and — the reverse gap — the persist:browser popup wrongly
skipped isNavigationAllowed()'s javascript:/data:/file:/blob: blocklist.
Session has no public .partition accessor, so add
isWorkspaceTileStoragePath() in security.ts, which infers browser-tile
identity from session.storagePath (Electron maps persist:<name> to
<userData>/Partitions/<name>, verified against Electron 40.10.3). Wire
isBrowserTileWebview() to use it; leave the persist:browser popup/UA
tweak untouched (still correct as-is).
CI-mirror 212 pass/0 fail (5 new), typecheck: 0 new errors (160
pre-existing, unrelated). Independent adversarial audit (sonnet,
default-reject): APPROVE.
server.test.ts and client.test.ts are documented (in their own header comments) to require `node --test` / `tsx --test` because node-pty's native addon needs node's libuv event loop — they were never meant to run under `bun test`. But the bare `test` script picked them up anyway, and Bun's node:test compat shim doesn't support describe() nested inside another test() (server.test.ts:209), throwing an unhandled error mid-run that corrupted results for every other file in the same process (14 fail + 2 errors framework-wide, none of them real). Excluded both files via --path-ignore-patterns and added a test:sidecar script that runs them the documented way (tsx --test). Verified: - bun run test: now 314 pass / 9 fail (down from 318 pass/15 fail/1 err) the remaining 9 are a separate, pre-existing bug: createSession() unconditionally calls ensureSidecar() even when terminalMode=tmux, so tmux.test.ts's Electron-free legacy path is unreachable. Not touched here — logged as follow-up, out of scope for this fix. - test:sidecar (tsx --test): 22 pass / 0 fail / 1 skip — confirms the sidecar server/client code itself is correct, it was purely a wrong-test-runner false alarm. - CI's actual test job (explicit file allowlist in ci.yml) does not reference either file and was never affected by this bug.
createSession() unconditionally routes through ensureSidecar(), which
throws when require("electron").app is unavailable. The old beforeAll
hook forcing terminalMode="tmux" was dead code (config.ts actively
strips terminalMode from loaded config), so these 9 tests were failing
outside Electron on every local bun test run. Gate them with
test.skipIf(!hasElectron) instead of letting them fail.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes (36 commits)
Test plan
🤖 Generated with Claude Code