Skip to content

fix(editor): restored tabs for files outside your projects no longer fail with Access denied - #24489

Merged
brennanb2025 merged 35 commits into
mainfrom
brennanb2025/editor-reads-files-outside-projects
Oct 4, 2026
Merged

brennanb2025 merged 35 commits into
mainfrom
brennanb2025/editor-reads-files-outside-projects

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 72 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3373 $\color{#cf222e}{\Huge{\mathbf{−}}}$​652 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2721
Prod 78 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1251 $\color{#cf222e}{\Huge{\mathbf{−}}}$​501 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​750

ELI5

If you opened a file that isn't inside one of your projects (for example ~/notes.txt from the floating workspace), Orca could show it until you restarted. After a restart the tab came back with "Unable to load file — Access denied: path resolves outside allowed directories", and Retry failed the same way every time.

Orca used to remember, in memory only, which outside files you had opened, and it often forgot them after a restart. Now every file request says what kind of access it is: "a file the user opened", "a file a document shows", "an image in a chat", "a write beside a document the user opened", or the default, "a file inside a project". Orca's main process checks each kind on the spot and remembers nothing, so a restart can't break a tab any more.

What Changed

The problem

Orca's main process only lets the app touch files inside known folders: your repos, worktrees and folder workspaces. Any other file needed a one-off permission, granted when you opened it (Cmd-click in a terminal, drag-and-drop, typing a path, and so on). Those permissions lived only in memory, so a restart wiped them.

When tabs came back, the editor tried to grant them again, but only for tabs that had saved a full path. A floating-workspace tab saves its path relative to the floating folder (~ by default), so it looked like a project file and was never granted again. Every read was refused, Retry included. Markdown files opened from Finder ("Open With") or the floating markdown picker, and keybindings.json, had the same problem.

Before → after, as you see it

  • A restored tab for a file outside every project (floating workspace, a file opened by full path, an OS-opened markdown file, keybindings.json): before, "Access denied" after a restart, and Retry never worked. After, it loads, and saving, autosave and Compare / Keep My Edits on a changed-on-disk banner all work.
  • A notebook outside every project: before, after a restart the kernel failed to start with "Access denied". After, it starts.
  • A project file that is a symlink to something outside the project, opened from the file explorer, a terminal Cmd-click, a typed path, a tab-strip drop or a browser file:// notebook: before, it worked until restart and then showed "Access denied". After, it opens by its full path and keeps working after a restart. These tabs show the full path and have no git-status decoration, because they are no longer project-relative.
  • A project folder that is a symlink to somewhere outside the project, clicked in the file explorer: before, it expanded until restart. After, the explorer doesn't follow it and says "This folder links outside the project, so it can't be opened here."
  • Images in chat transcripts: shown for any role when the file they really point to is an image Orca can display (PNG, JPEG, GIF, WebP, SVG, BMP, ICO, AVIF). Pasted screenshots keep showing after a restart, and agent images saved outside the project now render. A link without an image extension that points at a real image now shows too. A file with no image extension at all still shows as a filename chip, as it did before this PR (the editor never returned image data for one).
  • Images on a Windows network share: in markdown previews and chat, they load only when they are inside one of your projects. Anywhere else they are not loaded automatically. An image dragged into the chat composer from a share outside every project shows as a filename chip.
  • Images and other files a markdown document shows: any file type loads, as long as it is inside your projects (for a project document) or inside the document's own folder (for a document outside every project). Before, the same folder rule applied through the remembered permissions.
  • Renaming a file you opened outside every project, or inserting an image into it (tab double-click, editor header rename, image paste, drop or picker in the rich markdown editor): before, both failed with "Access denied" even before a restart, unless the file was in Orca's own floating folder. After, both work wherever the file lives. The rename goes wherever the name you type points, relative to the file's folder: a new name in the same folder, a subfolder (archive/todo.md), another folder (../other/todo.md) or a project folder. Undo and redo move it back and forth from wherever it went. Undo is Cmd/Ctrl+Z in the file explorer, and it only works if the explorer was already showing that workspace before the rename; a floating-workspace file has no explorer, so its rename can't be undone from the app yet (the same reachability gap project renames have on main). Afterwards the tab keeps its access, so it still saves and can be renamed again, and the path it restores after a restart still gets that access. The same holds for a tab opened by full path in a project workspace (for example a Cmd-click in a project terminal), including when you move the project folder that holds it. The inserted image is saved next to the document.
  • AI Vault "View Log": the "path not authorized" toast is gone, since there is nothing to authorize. A log that can't be read shows the editor's normal load error.
  • Files inside a project: no change. Containment, including symlinks that lead outside the project, works as before.

How it works

Each local file request from the desktop window now declares what kind of file access it is, in one optional field. The main process checks each kind and keeps no per-file state.

One rule covers every kind, in the one function that handles them all: a declared kind never refuses what the default project check would allow. Main runs the project check first, and the kind's own rule only adds paths outside your projects. So a kind can't make anything inside a project stricter, and a future kind gets this for free.

  • No field (the default): a file inside a project. This is the existing project check, unchanged: the path must be inside a project folder, symlinks included. Desktop requests also accept Orca's own floating-workspace folder. A request that doesn't say what it is gets only this.
  • A file the user opened (a click, a drop, a typed path, or a saved tab that was opened that way). It must be a full path, and it is read where it is. A saved tab counts only if it is a floating-workspace tab, or was saved with its full path, and its owner is known to be this machine. SSH and remote-server tabs never get it. Only file reads, file info, existence checks and saving an open file accept this kind.
  • A file a document shows (markdown preview, rich editor, image preloading). Beyond your projects, it adds the document's own folder, symlinks included. Any file type, like the common pattern.
  • An image in a chat (transcript or composer preview, whoever's message named it). Beyond your projects, it must be a full path, and the file it really points to must be an image Orca can display. Only the real target's type counts, so shot.png pointing at ~/.ssh/id_rsa is refused, and an extensionless link to a real PNG loads.
  • A write beside a document the user opened (renaming it, or saving an inserted image next to it). The renderer sends it only for a tab that already counts as "a file the user opened", and never for read-only tabs, SSH or remote-server tabs. Beyond your projects, main allows renaming only that document, to any absolute path, by the same rule as a file the user opened. Its Undo declares the moved file, so it is allowed back from wherever the file landed, inside a project or not. Renaming any other file gets the project check only. Copying new files is allowed into that document's folder or a folder under it, checked by path text before any filesystem call and again after resolving symlinks.

A network-share path outside every project is refused by the project check's path text, before Orca touches the disk or the network, for every kind; the user-opened kind then reads a share the user named, as today. For the document and chat kinds, which load automatically as you scroll, a Windows device path (\\?\, \\.\) or device name (NUL.png, COM1.jpg) is refused by its text before anything else, even inside a project, and a share is reached only inside a project. Writes beside a document refuse device paths and names outside projects the same way.

Other changes:

  • The in-memory permission system is deleted: the remembered set, the fs:authorizeExternalPath channel, and every place that granted a path, including the restore-time re-grant.
  • Local reads open the file once, without blocking, and refuse anything that isn't a regular file (devices like /dev/zero, pipes, sockets, folders). They then read a capped amount from that same open file. Saves refuse to overwrite a device or pipe.
  • Notebook kernels and AI Vault log tails check their file as one the user opened, through the same resolver, so inside a project they resolve as before. A notebook's kernel starts in the real file's folder, as on main, whether it was opened through a link inside or outside a project.
  • The file explorer, terminal Cmd-click, typed paths, tab-strip drops and browser notebooks first check a path inside the project as a project file. If only the "file the user opened" check passes, the path leads outside the project, and Orca opens it by its full path.

Why

What broke first. The first version of this PR let any full path outside a project be read in place, for every caller. Review showed that automatic readers then reached it too, because Markdown preview images and chat images load without a click:

  • On Windows, a README image pointing at a network share (//attacker/share/x.png) would make Orca connect to that host and send the user's Windows login hash.
  • An image pointing at /dev/zero would make the main process buffer data until it ran out of memory.

The fix is not to widen reads for everyone, but to let each request say what it is, and to keep the default as strict as before.

Alternative: just re-grant the right tabs at restart. Three rounds of review on that approach each found another reader, or another way of spelling a path, that it missed. It keeps a remembered state that every new feature has to restore correctly. Declaring the access kind on each request leaves nothing to restore.

Differences from the common pattern (a file named by full path is read where it is, a path relative to a project stays inside it, and content-referenced loads are limited to the document's folders):

  • Intended: a request that declares nothing gets only the project check, not an in-place read. Every caller that wasn't updated therefore fails closed, which is what keeps automatic loads away from arbitrary files.
  • Intended: the "file the user opened" kind is chosen by an explicit field. Orca's file calls always send full paths, so the path's form can't carry that meaning.
  • Intended: saving an open tab for a file outside every project is allowed, and so is renaming that file to wherever the user types, as the common pattern does for a file the user opened. Saving an inserted image next to it stays inside the file's own folder. File-explorer actions (create, delete, copy, import into a project tree) stay inside projects.
  • Intended: outside your projects, chat images are limited to image files Orca can display, judged by the real target. The common pattern shows chat images through the same image renderer, which can't show other file types either.
  • Intended: the one exception to "never stricter than the project check": automatic loads (document and chat) refuse a Windows device path or device name even inside a project. A device is never a file to show, and opening one can block.
  • Intended: Orca's floating folder is an extra desktop-only project folder; paired clients never see it.
  • Intended: a Windows network-share path clicked or typed by the user stays readable, as today. Automatic loads reach a share only inside one of your projects.
  • Temporary: a test lists every renderer file that can create the "file the user opened" kind, or open a tab that is read that way. It can't see a decision passed through props, so it is a review tripwire, not a guarantee. Follow-up: a dedicated user-opened path type that only gesture handlers can create.
  • Temporary, pre-existing and unchanged: terminal link detection checks whether any path printed in a terminal exists, with no restriction. On Windows a printed share path can make Orca contact that host. Follow-up: refuse non-WSL share paths there.
  • Temporary:
    • when a folder is moved, the editor's own-move check reads without the tab's access kind, so a dirty tab opened through an outside-pointing link could show a false "changed on disk";
    • one module name follows its caller rather than its content;
    • Quick Open and file-path search don't include the floating folder;
    • a rename typed with .. keeps those segments in the tab's path, as project renames already do, so opening the same file by its plain path can open a second tab;
    • document images resolve their real path on every load, with no cache.
      All are small follow-ups.

Linked Issue

None. Reported directly.

Visual Proof

Live checks ran on a remote Mac (M4Air) and a Windows machine: a hidden development build of origin/main (before) and of this branch at bbe1afe (after), each with its own throwaway home folder and profile. The final head adds only a merge of main on top of that.

A file opened outside every project, after a restart. Before: "Access denied" and Retry fails. After: it loads.

Before After
notes.txt after restart on main: Access denied notes.txt after restart on this branch: loads

A notebook outside every project, after a restart. After: it loads and its kernel runs (prints 42).

Before After
notebook after restart on main: Access denied notebook after restart on this branch: kernel ran

A project file link that points outside the project, after a restart. After: it opens by full path and still loads.

Before After
outfile.txt after restart on main: Access denied outfile.txt after restart on this branch: loads

A project folder link that points outside shows a message instead of being followed (main followed it). The same shot shows an image pasted into a markdown file outside every project, saved next to it and shown in the editor.

folder link message and pasted image

Rename anywhere: the outside notes.txt renamed into a subfolder, another folder and then into a project, saved there and restored after a restart. The path keeps the typed .. segments (listed under Temporary).

notes.txt renamed into a project, restored after restart

On Windows, markdown images pointing at network shares (//host/share/x.png, file://host/share/x.png) stay broken and Orca's file reads never touch the share, inside or outside a project, and a normal image next to an outside document now shows (on main it didn't). One gap remains and is the same on main: see Known gaps below.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated

What the tests cover:

  • Real-filesystem tests (main process):
    • A file outside every project is read in place when the user opened it, and refused when the request declares nothing.
    • Relative and drive-relative paths are refused.
    • Folders can be checked but not read.
    • /dev/zero, /dev/urandom and a symlink to a device are refused without reading, and saves onto /dev/null are refused.
    • Reads are capped even when a file grows past its reported size.
    • A project symlink that leads outside, under nine different spellings of the project path (aliases, .., a /var-style parent alias, a case variant on a case-insensitive disk), is refused for reads and writes when the request declares nothing. When the user opened it, it reads in place.
    • Document files load in the right folders whatever their type, links included, and are refused outside them or through a link that leads out.
    • Chat images: any local image works, including AVIF, BMP, ICO, SVG and WebP, and an extensionless link to a real PNG; text files, PDFs and image-named links to text or devices are refused.
    • A declared kind never refuses what the default allows: for every kind and operation, an in-project file, folder, new name, link and the floating folder resolve exactly as with no declared access, and a full-path project tab renames into another folder of the same project.
    • Resolving any kind never touches a network share outside every project (recorded filesystem calls stay empty).
    • Writes beside a document: renaming an outside file inside its folder, into a subfolder, into another outside folder and into a project all work, and so does each Undo. A full-path project file can be moved out of its project and back. Renaming a different file (outside, or a project file named with the document's access), a relative new name, and the same rename with no declared access are refused. An SSH rename still goes to the remote host unchanged. An image import into the document's folder or a subfolder of it works; into its parent, through a linked subfolder that leads out, or with no declared access, it is refused.
    • A notebook opened through a link, inside or outside a project, starts its kernel in the real file's folder.
    • In the editor, after a rename and after its Undo, a floating-workspace tab and a full-path tab in a project workspace keep their full path. They still read and save as a file the user opened (checked on the save request), and the stored fields a restart restores still get that access. Moving a project folder that holds a full-path tab keeps it too. A project tab still gets its project-relative path.
  • Windows path rules, tested with Windows path semantics and a recorded filesystem, so they run on any machine:
    • network shares, \\?\ and \\.\ paths, device names and dot-segment spellings like NUL.png\. are refused with zero filesystem calls, for automatic loads and for writes beside a document;
    • share images inside a project load; outside every project, share images are refused with zero filesystem calls even beside a document opened from that share, including look-alike host spellings (a KELVIN SIGN K, a decomposed accent);
    • WSL paths are allowed.
  • Renderer tests:
    • which access kind each reader sends: editor load, save, restored-tab conflict check, change banner, compare dialog, phone markdown bridge, markdown preview, image preloading, chat images;
    • the image cache never shares an image across access kinds;
    • floating tabs restore and save as user-opened files, and project tabs stay inside their project;
    • SSH, remote-server and unloaded owners get no special access;
    • project links leading outside the project open by full path from the explorer, terminal, typed path, tab-strip drop and browser notebook;
    • renaming a document (and the rollback when the editor can't follow it) and inserting an image send the write-beside-a-document kind only for a local user-opened tab, never for a project, SSH or read-only tab;
    • the explorer refuses folder links that leave the project;
    • the ratchet lists.
  • Ablations: each check was removed in turn to confirm its tests fail. That covers running the project check first (and running it before any filesystem call), the device checks before and after it, the regular-file check, the path-text checks for shares and device names, the chat real-target type check, the AVIF entry, each check on writes beside a document (only that document, the folder check by path text and by real path, device names), the notebook path going through the project check, the share refusal for document images (including look-alike hosts), the rename rule (putting back the old same-folder rule, and letting a new path through when the renamed file isn't the opened document), the editor keeping a full-path tab's path after a move, which file a move, its Undo and its rollback declare, the notebook's real-folder kernel, each renderer site that sends that kind, putting back the old type checks (document type, chat requested name), "no declared access means project only", each site's handling of links leaving the project, the access kind at each reader, and treating every tab as user-opened.
  • Typecheck and lint: node and web typechecks, oxlint, the anti-slop check, the max-lines ratchet and the changed-code quality gate pass locally. Related suites pass locally.
  • Failures seen during development, unrelated to this PR:
    • Under heavy machine load a few unrelated suites timed out; they passed when re-run alone.
    • One CI run had a timing failure in profile-state-backup-worker.test.ts. That file and its imports are untouched by this PR.

Not verified: a real restart in the app, any run on Windows (Windows behaviour is checked with Windows path rules only), and a manual Linux run (CI covers Linux tests).

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Known gaps found in live testing, all the same on main and not changed by this PR (follow-ups):

    • A file in the floating workspace can't be saved until a project has been opened since the app started: Cmd+S shows "Failed to save the file" and autosave doesn't run. The editor's save handler only exists inside the project workspace area, which isn't built until a project is active. On main this was hidden, because the restored file never loaded. Opening any project makes saving work.
    • On Windows, switching away from a rich-editor tab makes Windows look up the host of share-style images in that document (//host/share/x.png, file://host/share/x.png). When the editor view is torn down, its custom image renderer is removed first and the images are redrawn from the stock markup in the live page, so the browser engine fetches them itself. The preview and Orca's main process are not involved.
    • Undo of a rename started from a tab or the editor header is only reachable from the file explorer, as described above.
  • Known limitation: a markdown file opened from a raw network-share path outside your projects (\\nas\notes\todo.md) can't show the images beside it. That is the same as before this PR. Mapped drive letters and mounted volumes aren't affected.

  • Security:

    • Automatic loads (markdown files and chat images) stay inside their folders or, for chat, are limited to image files by real target. Orca's file reads for them can't reach Windows network shares outside your projects or device paths; the path text is refused before any filesystem call. (The rich markdown editor still has a separate, pre-existing way to contact a share: see Known gaps.)
    • Writes outside every project are limited to renaming the opened file (to the path the user typed) and adding new files under its folder, and only for local tabs the user opened.
    • Reads refuse devices and pipes and are capped.
    • There is no in-memory permission any more for the app to grant itself.
    • One residual case: a local symlink that leads onto a network share is resolved once before being refused.
  • SSH: requests with an SSH connection go to the remote host as before. The access field is never sent to SSH or remote servers. SSH-owned tabs never read local disk as user-opened files.

  • Cross-platform:

    • The path checks use Node's path functions.
    • The Windows-only rules (shares, devices, device names, WSL paths) are tested with Windows path semantics; markdown share images and restore after restart were also checked live on Windows.
    • O_NONBLOCK doesn't exist on Windows and is skipped there.
  • Paired mobile and web clients: they use runtime file calls, which this PR doesn't change. They are stricter now, because a desktop permission no longer leaks into their checks. A phone can only read or save tabs that the desktop opened.

  • Compatibility:

    • The new access field is optional and exists only on the desktop window's own file calls, which always ship with main.
    • The fs:authorizeExternalPath channel is removed; nothing outside the app used it.
    • Nothing sent between a paired client and a host changes.
  • Performance:

    • Every request runs the project-folder check first; it is cached, and only a refusal there runs the kind's own rule.
    • Reads size their buffer from the file's size instead of always allocating about 2 MiB.
    • Document images resolve their real path on each load, with no cache yet.

Checklist

  • This PR is small and focused (it isn't: it replaces the permission system end to end; most of the file count is test updates)
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred). Node and web typechecks, scoped lint, the anti-slop check and related suites pass locally; CI covers the full runs.

A file opened from outside every project (e.g. ~/notes.txt from the floating
workspace) read through an in-memory grant. After a restart the restored tab
only renewed that grant when it stored a full path, so a tab saved relative to
the floating workspace folder failed with "Access denied" and Retry repeated it.

Single-file reads (read, stat, exists) and open-editor-tab saves now resolve a
path outside every project in place. Paths inside a project keep the full
containment check, so a project's symlinks still cannot escape it, and every
other write stays inside projects.
@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

[High risk] Removes external path authorization checks from filesystem operations.

The PR does not appear safe to merge while supported older Git versions can miss prepared-checkout fallback and automatic image loading can contact a network share before rejection.

Findings

  1. P1 Security Network share contacted before rejection ▶

Summary

The PR replaces transient external-path grants with declared access modes for desktop file operations and adds supporting path, read, and save handling. Changes since the previous review also add prepared-worktree fetch barriers and locking, idle pack-index maintenance, terminal-routing updates, and E2E fixture isolation.

  • The prepared-worktree fallback does not recognize every unsupported-option response on older supported Git versions.

Reviews (9) · Last reviewed commit: "fix(fs): refuse every Windows device-nam..."

Comment thread src/main/ipc/filesystem-auth.ts Outdated
Comment thread src/main/ipc/filesystem/filesystem-read-handlers.ts Outdated
Comment thread src/main/ipc/filesystem/filesystem-write-handlers.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Path resolver for user-named files — adds resolveUserNamedPath in src/main/ipc/filesystem-auth.ts:149. It tries the existing containment check and, only when the candidate is outside every root, returns the resolved path with no realpath and no in-memory grant; non-absolute paths are refused.
  • Read handlers switch resolvers — fs:readFile, fs:stat, fs:pathExists, and fs:pathsExist now use resolveUserNamedPath instead of resolveAuthorizedPath (filesystem-read-handlers.ts).
  • Editor-save write flag — fs:writeFile accepts savesOpenEditorFile and routes only editor saves through resolveUserNamedPath; every other write keeps resolveAuthorizedPath (filesystem-write-handlers.ts:45).
  • Renderer plumbing — writeRuntimeFile gains an options bag and strips the flag when context.connectionId is set, so SSH writes never carry it (runtime-file-mutation-client.ts); editor-save-queue.ts:113 sets it on local saves.
  • Tests — new src/main/ipc/filesystem-user-named-path.test.ts (6 tests, verified green locally) plus renderer call-shape updates.

ℹ️ Outside-project access now depends on fs:authorizeExternalPath staying ungated

The change is not a widening today because any renderer can already grant itself any absolute path: fs:authorizeExternalPath is an unauthenticated IPC that records the path straight into authorizedExternalPaths. The new fallthrough returns exactly resolve(targetPath), which is the same value that handler stores, so every target it permits was already reachable. This is load-bearing — if fs:authorizeExternalPath is ever given a sender check or a user-gesture requirement, resolveUserNamedPath becomes a genuine bypass for symlink escapes and for a renderer-set savesOpenEditorFile.

Technical details
# The invariant that makes this change safe

## Affected sites
- `src/main/ipc/filesystem/filesystem-write-handlers.ts:106` — `fs:authorizeExternalPath` handler, no sender/trust/gesture check.
- `src/main/ipc/filesystem-auth.ts:38` — `authorizeExternalPath` records `resolve(targetPath)` (and best-effort `realpathSync`) with no validation.
- `src/main/ipc/filesystem-auth.ts:149` — `resolveUserNamedPath` fallthrough returns `resolve(targetPath)` for outside-root paths.
- `src/main/ipc/filesystem/filesystem-write-handlers.ts:45` — `savesOpenEditorFile` is taken directly from IPC args.

## Required outcome
- Either keep `fs:authorizeExternalPath` ungated (current state, so no action needed), or, if it is ever gated, move the outside-root allowance behind an equivalent check.

## Suggested approach (optional)
- No change requested for this PR. Carrying this note forward so a future gate on `authorizeExternalPath` is recognized as security-relevant to `resolveUserNamedPath`.

## Open questions for the human (optional)
- The PR discloses that a remote path misrouted to the local read path would now read a same-named local file instead of being denied. The loader's owner-not-ready guard covers the restored-tab path; is any other entry point able to send a remote path without a `connectionId`?

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: cb24e002-a47a-4e88-a6bc-40aff1e26136

📥 Commits

Reviewing files that changed from the base of the PR and between 7c8350c and af65b5a.

📒 Files selected for processing (127)
  • src/main/ai-vault/local-log-tail-reader.ts
  • src/main/ipc/app.test.ts
  • src/main/ipc/app.ts
  • src/main/ipc/filesystem-allowed-roots.test.ts
  • src/main/ipc/filesystem-auth.test.ts
  • src/main/ipc/filesystem-auth.ts
  • src/main/ipc/filesystem-import-local-rollback.test.ts
  • src/main/ipc/filesystem-import-local.ts
  • src/main/ipc/filesystem-import-ssh-path-safety.test.ts
  • src/main/ipc/filesystem-import-ssh.test.ts
  • src/main/ipc/filesystem-import-ssh.ts
  • src/main/ipc/filesystem-import.test.ts
  • src/main/ipc/filesystem-mutations.test.ts
  • src/main/ipc/filesystem-mutations.ts
  • src/main/ipc/filesystem-provider-continuations.test.ts
  • src/main/ipc/filesystem-request-shape-network-path.test.ts
  • src/main/ipc/filesystem-request-shape.test.ts
  • src/main/ipc/filesystem-request-shape.ts
  • src/main/ipc/filesystem-runtime-upload-staging.test.ts
  • src/main/ipc/filesystem-runtime-upload-staging.ts
  • src/main/ipc/filesystem-search-rg-timeout.test.ts
  • src/main/ipc/filesystem-test-harness.ts
  • src/main/ipc/filesystem.test.ts
  • src/main/ipc/filesystem.ts
  • src/main/ipc/filesystem/filesystem-file-content-inspection.ts
  • src/main/ipc/filesystem/filesystem-read-handlers.ts
  • src/main/ipc/filesystem/filesystem-search-handlers.ts
  • src/main/ipc/filesystem/filesystem-write-handlers.ts
  • src/main/ipc/filesystem/local-regular-file-read.test.ts
  • src/main/ipc/filesystem/local-regular-file-read.ts
  • src/main/ipc/floating-workspace-directory.test.ts
  • src/main/ipc/floating-workspace-directory.ts
  • src/main/ipc/keybindings.test.ts
  • src/main/ipc/keybindings.ts
  • src/main/ipc/local-log-tail-lifetime.test.ts
  • src/main/ipc/local-log-tail.test.ts
  • src/main/ipc/local-log-tail.ts
  • src/main/ipc/notebook.test.ts
  • src/main/ipc/notebook.ts
  • src/main/ipc/runtime-upload-file-stream.test.ts
  • src/main/ipc/runtime-upload-file-stream.ts
  • src/main/ipc/runtime-upload-slice-boundaries.test.ts
  • src/main/startup/os-opened-markdown-files.test.ts
  • src/main/startup/os-opened-markdown-files.ts
  • src/main/window/clipboard-image-temp-file.test.ts
  • src/main/window/clipboard-image-temp-file.ts
  • src/main/window/clipboard-ipc-handlers.test.ts
  • src/main/window/clipboard-ipc-handlers.ts
  • src/main/window/clipboard-runtime-owned-ssh-paste.test.ts
  • src/main/worktree-root-preparation.test.ts
  • src/preload/api/filesystem-api.ts
  • src/preload/api/fs-bridge.ts
  • src/renderer/src/components/browser-pane/describe-page/browser-artifact-upload.ts
  • src/renderer/src/components/browser-pane/navigate/navigate-browser-page-url.ts
  • src/renderer/src/components/editor/ExternalFileChangeBanner.tsx
  • src/renderer/src/components/editor/ExternalFileChangeCompareDialog.tsx
  • src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts
  • src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts
  • src/renderer/src/components/editor/editor-save-queue.ts
  • src/renderer/src/components/editor/editor-save-user-named-tab.test.ts
  • src/renderer/src/components/editor/local-image-src-reader.ts
  • src/renderer/src/components/editor/markdown-preview-local-images.ts
  • src/renderer/src/components/editor/rich-markdown-extensions.ts
  • src/renderer/src/components/editor/rich-markdown-local-image.test.ts
  • src/renderer/src/components/editor/use-markdown-preview-components.tsx
  • src/renderer/src/components/editor/useEditorPanelContentState.test.tsx
  • src/renderer/src/components/editor/useEditorPanelFileContentLoader.ts
  • src/renderer/src/components/editor/useEditorPanelFloatingTabContentState.test.tsx
  • src/renderer/src/components/editor/useEditorPanelHostUnresolvedContentState.test.tsx
  • src/renderer/src/components/editor/useEditorPanelRemoteSiblingContentState.test.tsx
  • src/renderer/src/components/editor/useLocalImageSrc.test.ts
  • src/renderer/src/components/editor/useLocalImageSrc.ts
  • src/renderer/src/components/native-chat/NativeChatImageAttachmentPreview.test.tsx
  • src/renderer/src/components/native-chat/NativeChatImageAttachmentPreview.tsx
  • src/renderer/src/components/native-chat/NativeChatMessageRow.tsx
  • src/renderer/src/components/native-chat/NativeChatTranscriptChrome.test.tsx
  • src/renderer/src/components/native-chat/NativeChatTranscriptChrome.tsx
  • src/renderer/src/components/native-chat/native-chat-composer-drop-scope.test.tsx
  • src/renderer/src/components/native-chat/use-native-chat-composer-attachments.ts
  • src/renderer/src/components/native-chat/use-native-chat-external-attachments.test.tsx
  • src/renderer/src/components/native-chat/use-native-chat-external-attachments.ts
  • src/renderer/src/components/right-sidebar/ai-vault-session-log-open.test.ts
  • src/renderer/src/components/right-sidebar/ai-vault-session-log-open.ts
  • src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx
  • src/renderer/src/components/right-sidebar/file-explorer-instant-dir-toggle.test.ts
  • src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts
  • src/renderer/src/components/right-sidebar/useFileExplorerHandlers.test.ts
  • src/renderer/src/components/right-sidebar/useFileExplorerHandlers.ts
  • src/renderer/src/components/right-sidebar/useFileExplorerTree.ts
  • src/renderer/src/components/sidebar/useSidebarProjectDrop.ts
  • src/renderer/src/components/tab-bar/tab-create-entry-absolute-file.ts
  • src/renderer/src/components/tab-bar/tab-create-entry-action.test.ts
  • src/renderer/src/components/tab-bar/tab-create-entry-action.ts
  • src/renderer/src/components/terminal-pane/terminal-file-open-routing.ts
  • src/renderer/src/components/terminal-pane/terminal-link-click-fallback.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-escaping-file-open.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-handlers-test-fixtures.ts
  • src/renderer/src/components/terminal-pane/terminal-link-handlers-test-harness.ts
  • src/renderer/src/components/terminal-pane/terminal-link-missing-file-open.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-osc-file-targets.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-remote-runtime-ssh-open.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-worktree-root-activation.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-wsl-path-mapping.test.ts
  • src/renderer/src/hooks/composer-state/attachment-drop-failure.test.tsx
  • src/renderer/src/hooks/composer-state/attachment-drop-lifetime.test.tsx
  • src/renderer/src/hooks/composer-state/attachment-drop-state.ts
  • src/renderer/src/hooks/useGlobalFileDrop.ts
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/i18n/locales/es.json
  • src/renderer/src/i18n/locales/fr.json
  • src/renderer/src/i18n/locales/ja.json
  • src/renderer/src/i18n/locales/ko.json
  • src/renderer/src/i18n/locales/zh.json
  • src/renderer/src/lib/local-file-access.test.ts
  • src/renderer/src/lib/local-file-access.ts
  • src/renderer/src/lib/user-opened-local-path.test.ts
  • src/renderer/src/lib/user-opened-local-path.ts
  • src/renderer/src/runtime/mobile-markdown-bridge.test.ts
  • src/renderer/src/runtime/mobile-markdown-bridge.ts
  • src/renderer/src/runtime/runtime-file-client-types.ts
  • src/renderer/src/runtime/runtime-file-metadata-client.ts
  • src/renderer/src/runtime/runtime-file-mutation-client.ts
  • src/renderer/src/runtime/runtime-file-read-client.ts
  • src/renderer/src/store/slices/editor-markdown-link-activation.test.ts
  • src/renderer/src/store/slices/editor/actions/markdown-link-action.ts
  • src/renderer/src/web/preload-api/web-filesystem-api.ts
  • src/shared/local-file-access.ts
💤 Files with no reviewable changes (19)
  • src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts
  • src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx
  • src/renderer/src/components/right-sidebar/file-explorer-instant-dir-toggle.test.ts
  • src/renderer/src/components/tab-bar/tab-create-entry-action.ts
  • src/main/ipc/filesystem-import-ssh-path-safety.test.ts
  • src/main/ipc/runtime-upload-slice-boundaries.test.ts
  • src/main/ipc/filesystem-runtime-upload-staging.test.ts
  • src/renderer/src/web/preload-api/web-filesystem-api.ts
  • src/renderer/src/hooks/composer-state/attachment-drop-failure.test.tsx
  • src/renderer/src/store/slices/editor/actions/markdown-link-action.ts
  • src/renderer/src/components/terminal-pane/terminal-link-handlers-test-harness.ts
  • src/main/ipc/filesystem-import-local-rollback.test.ts
  • src/main/ipc/filesystem-runtime-upload-staging.ts
  • src/main/ipc/filesystem-import-ssh.ts
  • src/renderer/src/components/terminal-pane/terminal-link-remote-runtime-ssh-open.test.ts
  • src/main/ipc/keybindings.ts
  • src/renderer/src/components/terminal-pane/terminal-link-handlers-test-fixtures.ts
  • src/main/window/clipboard-image-temp-file.ts
  • src/main/ipc/runtime-upload-file-stream.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The pull request replaces external-path authorization grants with typed local access shapes. Main-process handlers now resolve desktop, user-named, and document-resource paths and validate regular files. Runtime clients and renderer features pass access metadata through reads, writes, stats, previews, image caches, editor tabs, attachments, drops, and link handling. Tests cover path boundaries, symlinks, bounded reads, restored tabs, and access-aware image caching.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to af65b

The change lets restored editor tabs for files outside a project load and save without "Access denied", and it keeps containment checks for project files. No actionable merge-blocking issue remains; the author's reported gaps (a real app restart, Windows, Linux) are normal verification items.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to af65b

The permission model changes across file opening, saving, and previews. A compromised application interface can still request writes outside projects, but that capability predates this change. Several controls improve; the wider migration remains only partially verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — An attacker controlling the privileged renderer filesystem bridge can request local reads and writes outside projects, subject to filesystem permissions and operation-specific checks. This maximum arbitrary-path authority already existed through the base external-grant API; the inspected change does not establish new remote-host or elevated operating-system authority.

Security Findings and Attack Paths

  • observed — The retained authorization-bypass finding identifies renderer-selected user-file access reaching unrestricted absolute-path resolution and the local write sink without proof of a user gesture or owned tab. Base exposed equivalent arbitrary-path grants to that renderer, so this condition predates the PR rather than demonstrating a newly introduced attack scope.

Trust Boundaries and Controls

  • observed — The image cache distinguishes rooted, user-file, and document-resource requests, including documentPath, runtime identity, and connection identity. Loading and lease cleanup use this access-aware key, preventing a less restricted cache entry from directly satisfying a differently authorized request.
  • observed — Local access metadata is omitted for SSH connections and is not added to the inspected remote stat/read RPC arguments. Existing remote relative-path and ownership checks remain in those client branches.

Resilience and Maintainability Implications

  • observed — The new regular-file write probe closes its handle before the subsequent path-based write. Target replacement can therefore race the preflight, and interrupted writes have no handler-level filesystem rollback. Base also used a separate path check followed by direct writeFile, so these are residual limitations rather than demonstrated PR regressions.

Hardening Proposals

  • proposed — If user-named access is intended to contain renderer compromise, replace the renderer's authority assertion with main-owned, path-scoped authorization tied to an approved gesture or persisted tab identity, with explicit operation and lifetime limits.
  • proposed — For stronger target-type and recovery guarantees, bind validation to the actual write operation and define atomic replacement and interruption behavior, rather than relying solely on a separate preflight probe.
🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ❓ Inconclusive Docstring coverage is 39.39% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 59 files. (58 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: fixing restored tabs for files outside project folders.
Description check ✅ Passed The description is detailed and covers the problem, changes, rationale, visual proof, testing, security, compatibility, and known gaps. It lists no linked issue; the template requires one for outside …
Full details: Docstring Coverage

Explanation

Docstring coverage is 39.39% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 59 files. (58 skipped: 6 unsupported, 52 over the file limit.)

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f47e8b1a-9a0b-4163-82d9-a8c3cda7f8b8

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2300c and 19fdcf6.

📒 Files selected for processing (13)
  • src/main/ipc/filesystem-auth.ts
  • src/main/ipc/filesystem-user-named-path.test.ts
  • src/main/ipc/filesystem/filesystem-read-handlers.ts
  • src/main/ipc/filesystem/filesystem-write-handlers.ts
  • src/preload/api/fs-bridge.ts
  • src/renderer/src/components/editor/editor-autosave-conflict-flow.test.ts
  • src/renderer/src/components/editor/editor-autosave-controller.test.ts
  • src/renderer/src/components/editor/editor-save-queue.ts
  • src/renderer/src/components/editor/restored-editor-owner-save-lifecycle.test.ts
  • src/renderer/src/components/editor/useEditorPanelFileContentLoader.ts
  • src/renderer/src/runtime/mobile-markdown-bridge.test.ts
  • src/renderer/src/runtime/runtime-file-client-mutation-ownership.test.ts
  • src/renderer/src/runtime/runtime-file-mutation-client.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Comment thread src/main/ipc/filesystem-auth.ts Outdated
…path shape

Problem: a file opened from the floating workspace (e.g. ~/notes.txt via
Cmd-click in the floating terminal, the floating markdown picker, or a .md
opened from the OS) loads until restart, then shows "Access denied: path
resolves outside allowed directories". Main's external-path grants live only
in memory. On restore the editor re-granted only tabs that stored an absolute
path, but floating tabs store a path relative to the floating root (~ by
default), which is deliberately not an authorized root, so they were never
re-granted. Restored floating notebooks also failed to start a kernel.

The previous commit on this branch let main read and save any path outside a
project without a grant. That widened fs:readFile/stat/pathExists for every
caller, including automatic reads of untrusted content (markdown preview
images), which opened a Windows UNC credential leak and a /dev/zero
main-process memory blowup. This reverts that model entirely.

Fix: one helper decides which client-local path a tab needs re-granted by
ownership: a floating-workspace tab, or a tab stored outside its own project.
It never grants paths a local project root covers (a grant would also
authorize a project symlink's outside target), and skips SSH-owned,
runtime-owned and not-yet-hydrated owners. Every reader that can touch a
restored tab before or without the editor loader uses it: the loader, the
restored dirty-tab conflict scan, and the paired-mobile markdown bridge.
Comment thread src/renderer/src/runtime/mobile-markdown-bridge.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (since the prior pullfrog review at 19fdcf6)

  • Reverted the main-process approach entirely — resolveUserNamedPath, the read-handler swaps (fs:readFile/stat/pathExists/pathsExist), the fs:writeFile savesOpenEditorFile flag, and the preload/mutation-client plumbing are all gone; reads and writes are back to grant-required. This closes the outside-root widening the earlier commit introduced (automatic reads of untrusted content, e.g. Windows UNC credential leak and /dev/zero blowup).
  • Added a renderer-side grant helper — getEditorTabExternalPathGrantTarget/refreshEditorTabExternalPathGrant in src/renderer/src/lib/editor-tab-external-path-grant.ts re-derive the client-local path a tab needs re-granted: always for floating-workspace tabs, only for absolute-stored tabs whose owner is a resolved local worktree, and never when a local project/folder root, SSH target, or runtime environment already owns the path.
  • Wired it into the readers that can touch a restored tab before or without the editor loader — useEditorPanelFileContentLoader.ts (new root-relative branch), editor-restored-tab-conflict-scan.ts (before the dirty-tab verify read), and runtime/mobile-markdown-bridge.ts.
  • Tests — helper unit tests for floating/local/SSH/runtime/unhydrated owners, grant-before-read ordering in the conflict scan and mobile bridge, and the floating loader path; the helper and conflict-scan suites pass locally.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…dy covers

Problem: the restore re-grant helper decided "already inside a project" in
the renderer from its worktree list. At startup that list only holds repos
the session references, so a floating tab inside an unlisted repo was granted
(including a symlink's outside target), and the renderer's path matcher
disagrees with main's on WSL \\wsl$ vs \\wsl.localhost, which stranded a
folder-workspace tab with "Access denied" after restart. The helper also
treated a folder workspace with a missing or ambiguous host as local.

Fix: the renderer now decides only by owner (a floating-workspace tab, or a
tab stored outside a project whose owner is explicitly local) and asks main
with `skipIfInsideAllowedRoots`. Main checks the path against its own allowed
and registered roots, in both the named and canonical-parent spelling against
both root spellings: a path a project covers gets no grant, an alias spelling
of a project path gets only that spelling, and a project symlink's outside
target is never granted. Explicit-open grants (Cmd-click, drag, explorer) are
unchanged. Tests now prove each reader waits for the grant before reading.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Re-review of 303b7ab found one security-guard regression in the new main-process grant helper: an alias-named path whose parent traverses an inner directory symlink out of the project falls through to a plain grant, recording the outside target in authorizedExternalPaths. I reproduced it locally (temporary vitest case, since removed). One inline comment with the trace, required outcome, and a suggested fix.

Reviewed changes (since the prior pullfrog review at 9e47d99)

  • Moved the "does a project root already cover this path?" decision from the renderer into main — the renderer helper now only identifies the owner, and fs:authorizeExternalPath accepts skipIfInsideAllowedRoots to run authorizeExternalPathOutsideAllowedRoots.
  • Added authorizeExternalPathOutsideAllowedRoots — a textually-in-root target gets no grant; an alias-spelled project path gets only the alias spelling (never its realpath); everything else falls through to the existing grant.
  • Simplified the renderer owner predicate — replaced the isWorktreeConnectionResolved / findWorkspaceFileRoute checks with getConnectionIdForFile(...) === null, so a folder workspace with an unknown or ambiguous owner now fails closed instead of guessing local.
  • Tests — new main grant suite (outside file, leaf/directory symlink denial, two-spelling alias matrix) plus renderer helper, floating-tab loader, conflict-scan, and mobile-bridge ordering tests.

I independently checked the renderer owner predicate change for lost grants: the only behavior removed is the folder-workspace unknown/ambiguous-owner local guess, which the loader's WORKTREE_OWNER_NOT_READY guard and main's folder allowed-roots both make harmless. No regression for a provably-local restored tab.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/filesystem-auth.ts Outdated
…or's real path

Problem: the restore re-grant decided "inside a project" from the named path
and the real path of its parent only. When a tab path crossed a project
directory symlink and named the project through a spelling that was neither
the registered root nor its realpath (a second alias, a `..` segment, a case
variant on a case-insensitive disk, a /var-style alias of an ancestor), no
check matched, the path took the full grant, and the symlink's outside target
became readable and writable.

Fix: a path is inside a project when the named path is inside a root, or the
real path of any ancestor folder is inside a root in its registered or real
spelling. Such a path gets at most its named spelling, never its realpath. An
ancestor that fails to resolve for any reason other than "missing" now fails
closed to the named-spelling grant instead of falling through to the full one.
Tests cover each spelling; the non-symlink case also runs on Windows.
Problem: fs:readFile stat'ed a path and then read it to EOF. A character
device such as /dev/zero reports size 0, passes the size limit and never ends,
so the main process buffers until memory runs out; a FIFO hangs the open.
Writes could also target an existing device or FIFO.

Fix: every local fs:readFile (editor and log snapshot) opens the path once,
non-blocking, refuses anything but a regular file, and reads the size check,
binary probe and content from that same handle, capped at the limit even if
the file lies about its size. The AI Vault log tail opens non-blocking too,
and fs:writeFile refuses an existing non-regular target.
Problem: main decided every local file request against one allow-list plus a
set of in-memory grants the renderer had to recreate after every restart, so
a file the user opened outside a project (for example from the floating
workspace) was denied once Orca restarted.

This adds the request shape the common pattern uses, alongside the grants for
now:
- no shape (the default): the path must be inside a project root main
  recognises, symlinks included. Desktop requests also accept the app-owned
  floating-workspace folder; paired-client RPC never does.
- user-file: a single file the user named by absolute path, used in place.
  Only fs:readFile/stat/pathExists and saving (fs:writeFile) accept it.
- document-resource: an image or PDF a document references, limited to every
  project root when the document is in one, else to the document's folder,
  and refused by path text before any disk or network access.
Notebook kernels and AI Vault log tails check their open file as user-named.
Problem: after a restart, a tab opened outside every project (a floating
workspace file, a file opened by absolute path, an OS-opened markdown) could
only be read if the renderer first re-granted its path, and readers that ran
before the editor loaded the tab had no grant at all.

The renderer now says what kind of request it is making, and main checks that:
- A persisted tab opened outside its owner's root (floating workspace, or an
  absolute stored path) whose owner is explicitly local reads and saves as a
  user-named file, from every reader: the editor loader, the restored-tab
  conflict scan, the change banner and compare dialog, the paired-phone
  markdown bridge and the save queue. Project tabs stay inside their root.
- Clicks, drops, typed paths and browser-opened notebooks stat as user-named.
- Markdown preview and rich-editor images are document resources, limited to
  the document's roots or folder. Images the user pasted or attached into a
  chat show as user-named; agent images stay inside the project.
- The image cache keys on the shape, so one shape's image never answers
  another's request.
A ratchet test lists every renderer file allowed to create a user-named
request.
Problem: main kept a set of paths the renderer had asked it to allow
(fs:authorizeExternalPath). The set lived only in memory, so a file the user
opened outside every project could be read until Orca restarted and was then
denied, and every new reader of a restored tab had to remember to recreate
the grant first. Three rounds of re-deriving grants at restore each found
another reader or path spelling it missed.

Now that every desktop request declares its shape, nothing needs a grant:
- delete the grant set, authorizeExternalPath, the restore re-grant from the
  earlier commits on this branch, the fs:authorizeExternalPath channel and its
  preload and web-client entries;
- delete every renderer grant call (terminal and markdown link clicks, drops,
  typed paths, the file explorer, AI Vault logs, chat attachments, browser
  notebooks) and every main one (floating markdown picker and folder, OS-opened
  markdown, keybindings.json, pasted images, import and upload sources);
- the floating workspace's picker-approved folders stay a terminal-cwd
  allowlist only.
Main now holds no per-path permission, so a restart can't change any answer.
…ed files

Problem: a file inside a project that is a symlink to something outside it
opened fine from the file explorer or a terminal Cmd-click, then showed
"Access denied" after a restart: its tab was stored as a project file, and a
project request is refused when it resolves out of the project. A folder link
out of the project expanded in the explorer until restart and then failed with
a raw access error.

Now the click decides and the tab keeps that decision. Both gestures stat the
path inside the project first; if only the user-named check passes, the path
leads out of the project:
- a file opens by its absolute path, so it reads and saves as a file the user
  named, the same before and after a restart;
- the explorer does not follow a folder link out of the project and says so
  ("This folder links outside the project, so it can't be opened here.").
Paths that stay inside the project still open as contained project tabs. Also
drops the AI Vault "path not authorized" message, which nothing shows now.
The FileContent casts in the editor loader and the paired-phone markdown
bridge were never needed (the read result is already assignable). Tests stub
window.api through vi.stubGlobal and pass narrow stores without casting; the
one test store that still needs a cast states why.
@greptile-apps

This comment has been minimized.

…adable

Problems found in review:
- Chat transcript images were trusted by message role: any user-role
  "[Image: source: <path>]" (an injected Claude record, `orca terminal send`,
  a paired client's image-ref) became an automatic user-named read as the row
  scrolled into view, of any file type, and on Windows a network-share path
  would have opened an SMB connection to that host.
- A document image named like an image but linking to a text file
  (logo.png -> .env) was read as text.
- Windows device names (NUL.png, COM1.jpg) passed the path-text check of the
  automatic image loads.
- A project symlink leading out of the project, opened by a typed path, a
  tab-strip drop or a browser file:// notebook, was stored as a project tab
  and immediately refused.

Fix:
- New chat-image request shape for every transcript image and the composer
  preview, whoever's turn named it: an absolute local path whose requested and
  real targets are image files, a regular file, size-capped; network-share and
  device-namespace paths and Windows device names are refused by path text
  before any filesystem call. Pasted screenshots still show after a restart,
  and agent images outside the project now render.
- Document resources check the real target's type too, and refuse Windows
  device names by path text.
- Typed paths, tab-strip drops and browser notebooks stat through the same
  check as the explorer and terminal, and open an escaping link by its
  absolute path.
- Tests pin the shape at the change banner, compare dialog, markdown preview
  and image prewarm; a second ratchet lists every file that can open a tab the
  tab rule reads as user-named, and its comment says what it can't see.
- Stale grant wording removed.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (since the prior pullfrog review at 303b7ab)

  • Deleted the in-memory external-path grant system — authorizeExternalPath, authorizedExternalPaths, the fs:authorizeExternalPath IPC channel and every call site are gone; resolveAuthorizedPath no longer consults remembered grants.
  • Added renderer-declared request shapes — src/shared/local-file-access.ts defines user-file / document-resource; main dispatches them in the new filesystem-request-shape.ts (resolveLocalFileRequestPath, resolveLocalWriteRequestPath, resolveDesktopAuthorizedPath, which adds the app-owned floating-workspace folder as a desktop-only root).
  • Hardened local reads — the new local-regular-file-read.ts opens one bounded handle and refuses non-regular files (devices/FIFOs/dirs), closing the device-file blowup; document-resource refuses network paths by path text before any disk access.
  • Migrated every former grant site — markdown/rich-markdown images use document-resource; drops, pastes, link clicks, clipboard temp images and the floating picker use user-file; editor tabs (including the restored floating tab this PR targets) resolve through editorTabFileAccess; notebook and log-tail reads use the user-named regular-file resolver.
  • Threaded the shape through the runtime clients — read/stat/write carry access, and localAccess() strips it whenever a connectionId is set so SSH stays the remote host's call; the local image cache key now includes the shape.
  • Added regression coverage — new request-shape, network-path, regular-file-read, floating-tab loader, user-named save and a source-ratchet suite pinning the user-file importers.

I re-derived the two concerns a prior round raised and both stay closed: content-referenced images cannot reach user-file (they use document-resource, which refuses network targets before touching disk), and the deleted authorizeExternalPathOutsideAllowedRoots fallthrough no longer exists. Two focused sub-reviews (content→user-file bypass; lost-grant regressions) found no new exploitable gap — link-target clicks are gestures equivalent to the old grant, and the newly-refused case (expanding a file-explorer directory symlink that resolves outside the project) is deliberate and test-covered.

ℹ️ PR description drifted from the final code

The "Why / Temporary" section still says directory listing, search, notebooks and log tails "still use the in-memory permission", but that permission and the fs:authorizeExternalPath channel no longer exist — those surfaces now resolve roots-only or via resolveUserNamedRegularFile. Worth a quick edit so reviewers are not looking for a mechanism that is gone.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

throw new Error(CHAT_IMAGE_TYPE_MESSAGE)
}
// Why the real target too: `shot.png -> ~/.ssh/id_rsa` must not be read as an image.
const realTarget = resolve(await realpath(resolvedTarget))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Network share contacted before rejection If a chat transcript references a local shot.png symlink to a Windows network share, scrolling the image into view triggers a read without a click. The local path passes the first check, but realpath dereferences it before the resolved network path is rejected. This can initiate an unintended network connection.

How this was verified: Transcript images load automatically, and the resolver dereferences the local path before checking whether its target is a network path.

Problems found in review:
- Two unit tests went red on this branch: the browser-share test still
  expected reads without a shape, and the rename test's electron mock had no
  app, which the desktop root check now needs.
- The device-name check ran on the raw path, so `NUL.png\.` or
  `COM1.png\x\..` (reachable from markdown `![](NUL.png%2F.)`) reached the
  filesystem; a document image whose real target was a device name passed.
- Chat images in a project that lives on a Windows network share no longer
  rendered, though the markdown preview showed them.
- Any failed project check (a missing file, a dropped connection) was taken
  as "this link leads out of the project" and opened as an absolute tab.
- Every local read allocated about 2 MiB, even for a tiny image.

Fix:
- Device names and device-namespace paths are checked on the resolved path
  and on the real target, for chat images and document resources alike.
- A network-share path in an automatic load is read only inside a project
  root (the user chose that share when adding the project); anywhere else it
  is still refused by path text before any filesystem call.
- Only main's "outside allowed directories" refusal marks a project path as
  leading out of the project; other errors surface as before. The message now
  lives in shared code so both sides agree on it.
- Reads size their first buffer from fstat and confirm EOF with a 1-byte
  probe; a file that grows past its reported size is still read in bounded
  chunks up to the cap.
- Fixed the two red tests.
…heck allows

A full-path tab for a file inside a project (for example a link that
leads out, opened by its absolute path) was renamed under the
document-folder rule, which limited the new name to the file's own
folder, although the same rename with no declared access could move it
anywhere in the project. Any declared kind could be stricter than the
default in the same way.

Every desktop local file request now goes through one resolver,
resolveLocalRequestPath: it runs the default project check first (roots,
Orca's floating folder, symlink containment, outside-root path text
refused before any filesystem call) and only on a refusal applies the
declared kind's rule, which adds paths outside projects. Reads, saves,
rename source and target, and import destinations all use it, so a new
kind gets the rule for free. Automatic loads (document and chat) still
refuse Windows device paths and names by text first, even inside a
project; a device is never a file to show.

The document-resource rule no longer needs its own project branch, and
chat images no longer re-run the roots check for shares.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (since the prior pullfrog review at f0d898cc)

  • Unified desktop file resolution behind resolveLocalRequestPath — every desktop local read, save, rename and import now runs the default project/root check first and only applies the declared kind's rule when that check refuses, so a declared kind can no longer be stricter than the default project check. LocalRequestOperation names the five path roles.
  • Simplified the document-resource and chat-image rules — resolveDocumentResourcePath dropped its own project-roots branch (the default check now covers in-project targets), and resolveChatImagePath no longer re-runs the roots check for a network-share target. Both still refuse Windows device paths/names by path text before any filesystem call.
  • Moved rename/import onto the shared resolver — fs:rename and fs:importExternalPaths replace the documentFolderAccessPath guard with resolveLocalRequestPath calls; resolveDocumentRenameSource keeps the exact-document constraint for the outside-project rename source.
  • Regression coverage — an in-project file renamed with document-folder access is now allowed to move anywhere in its project, matching a request with no declared access; a matrix pins "a declared kind never refuses what the default project check allows" across every kind × operation; and a "default check runs first" test confirms no network share outside every project is contacted while resolving any declared kind. I ran the three touched main-process suites here (117 tests, all pass).

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…e-share images

- Renaming a file opened outside every project accepted a name in a
  subfolder (`archive/todo.md`), but the Undo and the rollback rename,
  declared from the moved file, were then refused and the file stayed
  moved. A rename under document-folder access must now land directly in
  the document's own folder (checked by path text before any filesystem
  call), so rename, Undo and rollback are symmetric. Image import still
  accepts the folder or a folder under it. Inside projects the default
  check still allows any in-project target.
- A notebook opened through a link inside a project started its kernel in
  the link's folder instead of the real file's folder (main's behaviour),
  because notebook and AI Vault log-tail paths skipped the project check.
  Both now resolve through resolveLocalRequestPath (project check first,
  then the user-file rule).
- A markdown file opened from a Windows share outside every project could
  not show the images beside it. Document images on a share are now
  allowed inside the document's own folder; the folder text check refuses
  every other host and share before any filesystem call.
- resolveDesktopAuthorizedPath is async, so a synchronous failure in the
  default check rejects like any other refusal.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ The new commits are a clean fix for the outside-project rename/Undo asymmetry and the notebook real-folder case. No new critical or must-address issues — one security-residual observation and two wording nitpicks below.

Reviewed changes

Since the prior pullfrog review at 02f0290 (this delta is commit be3f9bb5e0 plus the origin/main merge 41450dac):

  • Made outside-project renames symmetric — a document-folder rename now has to land directly in the document's own folder (isSameFolder(dirname(target), documentFolder)), so a forward rename, its Undo declared from the new name, and a rollback all pass; project files, whose default check runs first, still move anywhere inside the project.
  • Resolved notebook and log-tail paths through the project check first — resolveUserNamedRegularFile now takes the store and delegates to resolveLocalRequestPath(..., {kind:'user-file'}, 'read'), so a notebook reached through an in-project symlink starts its kernel in the real file's folder; local-log-tail threads the store for the same reason.
  • Allowed document images on the document's own network share — resolveDocumentResourcePath dropped its unconditional network-share refusal; a target must still be inside the document's own folder by path text and by realpath, and every other host and share is refused by path text before any filesystem call.
  • Made resolveDesktopAuthorizedPath async — a synchronous throw while building the floating-workspace root now rejects and is swallowed like any other refusal instead of escaping resolveLocalRequestPath.
  • Tests — undo/rollback rename coverage, subfolder-rename and linked-subfolder-import refusals, a same-share read/refusal matrix, and a notebook project-link test asserting the real folder is the kernel cwd.

ℹ️ Same-share document images now realpath a share before the containment re-check can refuse a reparse point

Allowing a document image only inside the document's own folder is the right shape, and every spelling of a foreign host or share is still refused by path text. The one case the removed isNetworkSharePath(resolvedTarget) guard used to reject outright is a reparse point that lives inside the document's own folder but resolves to a different host: the folder gate passes, realpath follows the link (Windows may authenticate to the second host here), and only the realpath containment check refuses afterwards. This mirrors the already-disclosed local-symlink-to-share residual, now on the share side, and its exploitability could not be confirmed here without a Windows capture.

Technical details
# A reparse point inside the document's own share is realpath'd before the containment re-check

## Affected sites
- `src/main/ipc/local-file-access-resolution.ts:98` — the folder path-text gate passes for any target under the document's own folder, including a share path (the guard removed in `be3f9bb`).
- `src/main/ipc/local-file-access-resolution.ts:101` — `realpath(resolvedTarget)` now runs on a share target the previous unconditional `isNetworkSharePath(resolvedTarget)` refusal would have rejected outright.
- `src/main/ipc/local-file-access-resolution.ts:102` — the realpath containment re-check refuses a target that resolves to another host, but only after the call returned.

## Scenario
`documentPath = \\nas\notes\doc.md`, `targetPath = \\nas\notes\link.png`, where `link.png` is a
reparse point to `\\attacker.example\share\secret.png`. Gate passes at `:98`; `realpath` at `:101`
resolves off-share; the check at `:102` refuses — after the second host may have been contacted.

## Required outcome
- Decide whether a share reparse point whose real target leaves the document's share is acceptable
  (as the disclosed local-symlink residual is), or should be refused before `realpath`.
- If accepted as-is, extend the PR body's residual note to cover it and add a test that a same-share
  reparse point resolving off-share is refused.

## Open questions for the human
- Is the attached-document residual acceptable for a share the user already opened, given opening the
  document already contacted that host?
- Does `fs.realpath` follow cross-server SMB reparse points on Windows? This could not be verified
  here without a Windows capture.

ℹ️ Nitpicks

  • src/main/ipc/local-file-access-resolution.ts:96 — the comment says the folder text check refuses every other host and share "before a filesystem call could reach one", but isDescendantOrEqual can itself statSync on non-ASCII paths (filesystem-path-containment.ts:88) and the following realpath is a filesystem call by design. Reword so the guarantee matches what the code does.
  • The PR body's "Images on a Windows network share" bullet still says they load only inside one of your projects; same-share document images outside every project now load as well. A short update keeps the body from contradicting the code.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

… kernels as on main

- Reverts the same-share document image rule from the previous commit.
  Its folder check compared hosts case-insensitively, so a host spelled
  with U+212A KELVIN SIGN (or a decomposed accent) passed as the
  document's own share and was contacted, reopening the network
  credential leak. Document and chat images on a share outside every
  project are again refused by path text before any filesystem call;
  tests now cover the look-alike hosts with zero filesystem calls.
- Renaming a file opened outside every project into a project folder
  passed the project check, but its Undo (declared from the new path)
  was refused and the file stayed moved. When the rename source is
  allowed only as the opened document, the new name must now land
  directly in the document's folder even if a project would accept it
  (resolveLocalRenamePaths).
- A notebook opened through a link outside every project started its
  kernel in the link's folder; main used the real file's folder. The
  kernel cwd is now the real file's folder in every case.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at 41450dac, this delta is commit c2e253d7 plus the origin/main merge 99b1aa4:

  • Refused share images outside every project again — resolveDocumentResourcePath restored its unconditional isNetworkSharePath refusal, closing the same-share reparse-point residual from the prior round: a document's own network share no longer reaches automatic image loads, and look-alike hosts (U+212A KELVIN SIGN, decomposed accents) are refused by path text with zero filesystem calls.
  • Made an outside-project rename into a project undo-consistent — the new resolveLocalRenamePaths resolves the target through the document-folder rule directly when the source was allowed only as the opened document, so a rename can no longer move a file into a project whose Undo (declared from the new name) would be refused.
  • Started a linked notebook's kernel in the real file's folder — notebook:startKernel now realpaths the resolved user-named file, matching main's real-folder cwd for a notebook opened through a link outside every project.
  • Tests — added the look-alike-host share refusals, the rename-into-a-project undo case, and both linked-notebook cwds.

I ran the five touched suites (142 tests) and pnpm tc:node; all pass.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…eads-files-outside-projects

# Conflicts:
#	src/renderer/src/components/editor/MarkdownPreviewBody.tsx

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at 99b1aa44, the only commit is the origin/main merge 0d3db6dc (second parent 58cf72d48e); it had one conflict, in MarkdownPreviewBody.tsx.

  • Merged origin/main — brings the large-Markdown-preview rewrite (#24880) and other upstream work into the branch.
  • Resolved the MarkdownPreviewBody.tsx conflict by keeping main's extraction — the sanitize schema moved to the new markdown-preview-plugins.ts, where the branch's file:// protocol allowance and its "so the click handler can open the target" wording are preserved; both the static and virtualized preview bodies consume MARKDOWN_REHYPE_PLUGINS, so preview file:// handling is unchanged.
  • Confirmed the PR's access threading survives the renderer rewrite — useMarkdownPreviewComponents still passes the document-aware image component to VirtualMarkdownPreviewBody, and rich-markdown-extensions.ts still threads documentResourceAccess; the four touched Markdown suites (31 tests) pass.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

… keeps its access

Renaming a document the user opened (floating workspace or full-path tab) now
follows the user-file rule: the source must be the opened document, and the
new path can be any absolute path, so a rename into another folder, a
subfolder or a project works, and its Undo (declared from the moved file)
comes back from there. Any other rename keeps the project check only. Remove
the same-folder rename rule and its tests; image import stays in the
document's own folder.

After a move, a tab stored by its full path keeps its full path instead of
being recomputed project-relative, so it keeps user-file access for save,
the next rename, image insert and restore after restart. Folder moves go
through the same remap.

Also un-export unused resolver exports and avoid a copy for single-chunk reads.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at 0d3db6dc, the delta is the single commit bbe1afe:

  • Allowed a user-opened document to rename to any path — resolveLocalRenamePaths now resolves the opened document's rename as user-file (both source and target), so the target is no longer confined to the document's own folder; this deliberately reverses the r11 folder-confinement rule.
  • Moved Undo symmetry to the renderer — the moved file is declared as the document on Undo and rollback (documentFolderAccess(fromPath) / documentFolderAccess(toPath)), so the reverse rename is allowed back out of a project it landed in; the main-process write suite now asserts the forward/undo/redo moves, and a new renderer suite pins the exact rename calls.
  • Preserved user-named status across a move — getUpdatedRelativePath short-circuits when relativePath === filePath, so a full-path tab keeps a full path instead of being recomputed project-relative and silently losing its user-file access.
  • Simplified the resolver — removed resolveDocumentRenameSource and isSameFolder, folded resolveLocalRequest back into resolveLocalRequestPath, un-exported the two IPC-internal error messages, and scoped document-folder to import-into only.
  • Added a single-chunk fast path in readLocalFileBounded, avoiding a copy for a one-chunk read.

The widening is not a renderer capability change: the renderer is trusted, user-file already permits arbitrary absolute read/write, a rename is an explicit gesture, and assertNoClobberRenameDestinationAvailable still prevents overwriting the target. The automatic-load kinds (document-resource / chat-image) are untouched, so the network-share and device-path refusals remain.

Re-ran the seven touched suites (199 tests), the remap/move suites (20), and pnpm tc:node + pnpm tc:web; all pass. No open pullfrog threads remain (r3's is resolved).

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…eads-files-outside-projects

# Conflicts:
#	src/main/ipc/notebook.test.ts
#	src/main/ipc/notebook.ts
#	src/main/startup/os-opened-documents.ts
#	src/main/startup/os-opened-markdown-files.test.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at bbe1afe, the only commit is the origin/main merge b00383c, which had four conflicts. The PR's own resolution files are byte-identical across the merge, so the access-kind behavior cannot have changed.

  • Merged origin/main — brings in the notebook kernel-start cancellation (#24858) and the OS-opened CSV/TSV documents feature, among the upstream commits on main.
  • Composed the notebook.ts conflict — main's createSenderScopedRequestCancellations guard now wraps the branch's resolveUserNamedRegularFile + realpath cwd, so a kernel still starts in the notebook's real folder and is still cancelled when the notebook closes.
  • Kept the branch's grant removal in os-opened-documents.ts — main's renamed and generalized resolver no longer calls the deleted authorizeExternalPath; the floating tab reads each OS-opened document as a user-named file.
  • Realigned the renamed tests — the deleted os-opened-markdown-files.test.ts is replaced by main's split suites, os-opened-document-resolution.test.ts drops its authorizeExternalPath mocks and assertions, and the clipboard-image-temp-file.test.ts source ratchet now names lib/open-document-in-floating-workspace.ts.

Verified pnpm tc:node and the conflicted plus ratchet suites (149 tests) green. No open pullfrog threads remain.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review summary

This is the final readiness and QA pass for this PR. It covers the code at head b00383c4774c (a merge of current main on top of bbe1afe79244).

The problem

If you opened a file that lives outside every project (for example ~/notes.txt from the floating workspace), it worked until you restarted Orca. After a restart the restored tab showed "Unable to load file — Access denied: path resolves outside allowed directories", and Retry failed the same way. The same happened to notebooks outside projects, and to project files reached through a link that points outside the project.

The cause: permission for those files was granted once, in memory, when you opened them, and nothing granted it again when the tab came back after a restart.

What changes for you

  • Files, notebooks and project file links outside your projects now reload after a restart. Notebook kernels start.
  • You can rename a file you opened outside every project to anywhere you type: a subfolder, another folder, or into a project. Images pasted into an outside markdown file are saved next to it.
  • A project folder link that points outside the project now shows "This folder links outside the project, so it can't be opened here." instead of being followed.
  • Images in markdown and chat that load on their own still can't reach Windows network shares through Orca's file reads.

How it works: the in-memory permission system is gone. Every file request from the window now says what kind of access it needs, and the main process checks that on every request, so there is nothing to lose at restart.

Fixed during this final pass

  • Renaming a tab that was opened by its full path inside a project workspace used to break the tab: save, the next rename, and restore after a restart all failed with "Access denied". The tab now keeps its full path.
  • Rename now works for any destination you type, and the reverse rename that Undo performs is allowed from wherever the file landed. This follows your decision to allow rename anywhere.
  • A small memory waste on every file read: it copied the whole file once more than needed.
  • Main was merged twice. Main had brought back the old permission call in the code that opens documents handed over by the OS (now including CSV and TSV); that call is removed again. Main's new notebook-start cancellation is kept, together with this PR's checks.

Known gaps, all the same on main (follow-ups, not fixed here)

Live testing found three gaps that this PR exposes but doesn't cause. Each is listed in the PR body.

  1. Saving with no project open. A floating-workspace file can't be saved until a project has been opened since launch. Cmd+S says "Failed to save the file" and autosave doesn't run. This was confirmed on main too, with and without a restart. Opening any project fixes it. The save handler only exists inside the project workspace area. The proposed follow-up creates it for the whole window.
  2. Windows: switching away from a rich-editor tab contacts network shares named in its images. This happens for //host/share/x.png and file://host/share/x.png. A live test found it: no lookup while the tab is open, a lookup within seconds of leaving it. When the editor shuts its view down, the editor library removes Orca's image renderer first. Images are then redrawn from the stock markup in the live page, and the browser engine fetches them itself. The preview and Orca's main process are not involved. The proposed follow-up registers the image renderer so it survives that teardown.
  3. Undo of a tab rename is hard to reach. Undo is Cmd/Ctrl+Z in the file explorer, and opening the explorer after a rename clears its undo history. A floating-workspace file has no explorer, so its rename can't be undone from the app. The reverse rename itself works, as tests show.

Other smaller items:

  • A rename typed with .. keeps those segments in the tab's path.
  • Renaming an opened file to a Windows device name like NUL isn't refused up front; the operating system should refuse it.
  • A helper that keeps the access field off SSH requests sits in the read client but is used by every local file client.
  • Every file read makes one extra memory copy. This is fixed in the branch.

What was verified

  • Readiness checklist. Run on the full diff and again on the final changes, with no blocking (P0 or P1) findings. Two fixes came out of it, listed above.
  • Live tests. A non-Claude agent drove hidden development builds of main and this branch. Each build ran with its own throwaway home folder and profile, so nothing touched real settings.
    • Mac (M4Air):
      • restore after restart for a file, a notebook and a project file link;
      • rename to a subfolder, a sibling folder and into a project, with save and restore after a restart;
      • image paste into an outside document;
      • the folder-link message;
      • save checks with and without a project open, on both builds.
    • Windows:
      • network-share images in previews, with a local listener, DNS-cache checks and instrumentation of both the window and the main process;
      • restore after restart.
    • Screenshots are in the PR body.
  • Tests and checks. Typechecks (node, web, CLI) and the targeted tests pass after the final merge. CI on the final head b00383c4774c: all green (20 passed, 16 skipped).

What wasn't verified

  • The live tests ran on bbe1afe79244. The final head adds only a merge of main, which was checked with typechecks and targeted tests, not re-run live.
  • On Windows, the outside file in the restore test was opened through the tab bar instead of a terminal Ctrl+click. A Ctrl+click on a terminal path opened nothing there, and that wasn't checked on main.
  • Undo from the explorer wasn't exercised live, because of gap 3.
  • WSL and SSH weren't exercised live. Tests cover them.
  • The case where a local link points onto a network share and is resolved once before being refused remains as disclosed; no planted-link test was run.

Decisions for you

  • Whether the three known gaps go into follow-up PRs, as recommended, or into this PR.
  • The file explorer is now stricter than main for project folder links that point outside the project. Main followed them on every click; this PR shows a message instead. That matches the earlier "do what is common" decision, but cuts against "don't be stricter than users expect". It's kept as is unless you want it changed.

@brennanb2025
brennanb2025 merged commit 51fe6f3 into main Oct 4, 2026
36 checks passed
brennanb2025 added a commit that referenced this pull request Oct 5, 2026
…astes

Main's #24489 deletes the in-memory path grant system: every desktop file
request now declares its access kind, and the composer preview reads a paste
as a chat image. The paste code this branch carries from #24905 still granted
its files, so:
- a composer paste is written without a grant, and a restored paste is kept
  only when its real path is a file inside the paste folder, as before, with
  no grant for either spelling;
- the tests keep every containment case and drop the grant assertions; a
  restore still never makes an outside file readable.
The composer attachment cache keeps this branch's draft-store reader.
brennanb2025 added a commit that referenced this pull request Oct 5, 2026
Main's #24489 removed the in-memory path grants (authorizeExternalPath and
fs:authorizeExternalPath): a chat image is now read with chat-image file
access, which needs no grant. The paste code is resolved onto that:
- a composer paste is written into the paste folder with no grant;
- the restore call no longer grants anything. It still reports a paste as
  kept only when its real path is a file inside Orca's paste folder, and
  now also only when the stored spelling names that same file (the preview
  and the send read by that spelling). Anything else is a placeholder.
Jinwoo-H added a commit that referenced this pull request Oct 5, 2026
…5351)

#25176 replaced insertPos with getInsertionRange and #24489 added a test
using insertPos; together they broke main's typecheck.
Jinwoo-H added a commit that referenced this pull request Oct 5, 2026
…5357)

#25176 and #24489 landed together and collided in
insertRichMarkdownImageFromPath. #25176 added three test files whose
@/store mock returns { settings, folderWorkspaces, worktreesByRepo }.
#24489 made the same function look up the open document with
state.openFiles.find(...) to pass document-folder access to the import.
Under the mock, openFiles is undefined, so the lookup throws, the catch
shows "Failed to insert image", and no image is inserted -- 38 tests
fail on main.

The real store always carries openFiles, so production behavior of both
PRs is intact; the mocks were just written against the pre-#24489 shape.
Add openFiles: [] to the three mocks.
sj900211 added a commit to sj900211/orcinus that referenced this pull request Oct 13, 2026
Measured from the post-stage-4 main: 827 commits, 33 conflicting files, 3 of
them in the PTY delivery subsystem. Four analysts planned the clusters
(installer/packaging/docs, main process, sidebar/worktree, app-shell/tabs/
store/i18n); one implementer applied all four, and an independent critic
reviewed the result.

## The two decisions this merge turned on

**orcad must not contain Electron.** This merge's Phase 3 deploys orcad to
every SSH host, and the runtime-electron ratchet had one entry on main:
`src/main/ipc/ui.ts`, pulled into the bundle by `pty/ipc/write-input.ts` for
`isTrustedUIRenderer`. That trust predicate and its id state moved to
`src/main/ipc/trusted-ui-renderer-ids.ts`, whose only Electron import is
`import type { WebContents }`; `ui.ts` re-exports the same five names, so all
15 importers and 35 `vi.mock('./ui')` sites are unchanged. The move is pure:
there is exactly one declaration site for the trust ids. Bundle
`require("electron")` goes 1 -> 0 and the ratchet reports `0 entries`.

**An SFTP preview tab must not resolve to a local file.** Upstream stablyai#24489 made
`editorTabFileAccess` grant a `user-file` entry per open tab. The fork's SFTP
preview tabs carry a REMOTE path in the same field, so the grant would have
named a local path that does not exist, or worse, one that does.
`local-file-access.ts:65-68` now returns `undefined` when `file.sftpTargetId`
is set, which leaves main's default root containment in force — the behaviour
main already has. The fork's two openers are registered in
`USER_NAMED_TAB_OPENERS`. Tests pin both halves: an SFTP tab gets no grant, and
a read-only SSH-workspace tab of the identical shape without `sftpTargetId`
still gets `'user-file'`.

## Defects found that no conflict marker could show

Nine files merged cleanly and were still wrong — the recurring shape of this
sync, now at eighteen instances across five stages:

- `readDirViaSftp` was deleted by upstream with no marker; `sftp-transfer.ts`
  and `sftp-download-batch.ts` are migrated to the generator form.
- `clipboard-ipc-handlers.ts` and `use-app-session-persistence.ts` breached
  `max-lines` at 303/300 from both sides adding lines, and no cluster owned
  them. Resolved by extraction, with no disable: 297 and 271.
- Upstream's new `child-workspaces-toggle-target.ts` consumes the fallback
  worktree-visibility builder, which lacked the fork's multi-window filter;
  `projectKeysInOtherWindows` is restored there.
- Upstream's new profile-save-delay toast has no timeout and clears only on a
  push the main process sends to one window, so it could stick forever in a
  fork project window. It is gated to the main window — the same class as the
  satellite focus defect fixed in stage 4.
- Upstream's new `vitest-sqlite-database-runtime-files.mjs` allowlist did not
  know about the fork's `cache-write-tokens.test.ts`, which opens a real SQLite
  fixture.
- Upstream's new runtime member `noteRemoteDesktopHostInput` is called
  unguarded, leaving a fork test stub one member short.
- `daemon-host-profile-isolation.test.ts:148` was passing *vacuously*: it
  seeded a legacy mirror under a directory the fork's pruner never visits.

`verify-orcinus-identity.mjs` is blind to upstream's second uninstall root
(`daemon-host-profiles`): a resolution keeping upstream's `Orca\...` line passes
the verifier. The kept associations test is the only thing that catches it, so
`orcinus-ci.yml` now runs it, with a pass/fail control recorded.

The Windows CLI launcher crate is unchanged by this merge and still carries
upstream's version resource (`build.rs` `ProductName "Orca"`). Noted, not fixed:
the identity verifier does not read it, and the three pieces move together.

## Review

An independent critic returned ACCEPT with 0 blocking defects, re-measuring
every gate and auditing all 27 decisions on the staged index rather than the
working tree. It found one process defect worth recording: the regression
gate's scope had a hole. CI's own unit list has 12,285 files; the sweep covered
6,650, and 5,635 (46%) were never run — a directory-shaped scope missing about
half the suite for the second stage running. The critic closed it by running
the whole complement on the merge and rechecking its 334 failing files on both
parents: `same=2059 diff=2 mergeOnly=0`, with the merge's totals on that set
equal to upstream's exactly. For stage 6 the scope is `discoverUnitFiles()`
minus nothing, or a reported `|universe - swept|`.

Twelve fork-side test files ship red. Every one is red identically on main and
is fork debt, not merge-caused: stale doubles (the fork's 4-argument
`fastGetViaSftp` against upstream's 3-argument stubs), fixtures missing
upstream's new `muse` provider, and tests pinning upstream identity where the
fork differs by ratified decision. One of them is a known production gap, not
debt: runtime notifier events still reach only the main window, which this
merge adds two more members to; the fork's own comment dates that to the
dungeon-3 re-implant.

Two test mocks were completed so that this merge's own change is covered:
`EditorFileTab.tsx` folds the SFTP icon into `FileIcon` to fit `max-lines` at
392/400, and its only test was red on main for an unrelated missing store key.
8 tests go green; the fix is `unifiedTabsByWorktree: {}` in two mocks.

## Gates

Cold typecheck (after deleting `config/*.tsbuildinfo`): 0 errors, measured
independently three times, the last after the mock fix. oxlint: 0 warnings,
0 errors across 32,832 files; oxfmt clean. verify-orcinus-identity: OK,
`upstreamRefs=0`. Unsigned smoke: OK. Fork contract tests: 32/32. Windows CLI
launcher: the Rust build runs here and passes. Localization catalog: OK. The
five repo ratchets (owner-routing, max-lines, ts-nocheck, process-host-imports,
reliability-gates) all match a parent exactly, with no new bypass.

One merge-only test failure remains, attributed: `mobile-web-source-line-
endings` fails on 871 working-copy files whose index blobs are LF in all three
trees. Exporting the index with `git checkout-index` passes on every tree. They
are stale bytes in this long-lived checkout; `.gitattributes` has pinned
`mobile/**` to `eol=lf` since before this merge, so a fresh checkout — which is
what CI does — is LF.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant