Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two minor suggestions inline and one scope consideration.
Reviewed changes
This run reviewed the authorization gates the PR adds over the shared RPC dispatcher: computer.* now rejects any paired clientKind, the skills install/remove/commit-upload mutations reject paired callers with the existing unsupported-environment code, settings.update strips agentDefaultEnv/agentDefaultArgs for paired clients, and files.browseServerDir/browseDirectory bounds listing to os.homedir(). Coverage is solid — the computer gate is exercised across all 15 methods for both mobile and runtime, and every new assertion checks an exact rejection string/code plus that the sidecar/runtime method was never called.
computer.*host-only gate — a newassertHostOnlyClientrejects every computer method (input and observe alike) for paired clients; local callers still reach the sidecar.- Skill-mutation gate —
rejectPairedSkillMutationthrowsagent_skill_sharing_unsupported_environmentfrominstall/installBundle/removeInstall/commitUploadand reuses it forskills.share. - Settings env/args strip — paired
settings.updatedropsagentDefaultEnv/agentDefaultArgswhile persisting the rest. - Homedir browse bound —
browseDirectoryresolves againsthomedir()and rejects paths outside it before anystat/readdir.
ℹ️ The browse bound is universal, not paired-only
The files.browseServerDir gate lives inside RuntimeServerEnvironmentCommands.browseDirectory with no clientKind check, so it restricts local unix-socket and in-process callers just as much as paired clients. The PR frames the threat as a paired-client escape, but the fix also removes the host's own ability to browse /srv, /opt, /mnt, /tmp, or external drives — a potential regression for headless orca serve hosts whose repos commonly live outside $HOME. If that is intended, fine; if not, gate the bound on clientKind !== undefined and leave local browsing unbounded.
Technical details
# Browse homedir bound scope
## Affected sites
- src/main/runtime/runtime-server-environment-commands.ts:27-32 — `assertAllowedServerBrowsePath` runs for every caller, with no `clientKind` discriminator.
- src/main/runtime/rpc/methods/files.ts:218-220 — `files.browseServerDir` forwards to `runtime.browseServerDir` without a `clientKind` check.
## Required outcome
- Decide whether local host browsing outside `$HOME` must stay available, and if so scope the deny to paired `clientKind` (or exempt local callers).
## Open questions for the human
- Is the directory browser expected to reach repos outside `$HOME` on headless hosts (e.g. `/srv`, `/opt`)?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR restricts computer RPC methods and skill mutations to the host runtime. It removes agent launch environment and argument settings from paired-client updates. It limits server directory browsing to paths inside or equal to the home directory. Skill deletion and preview requests on non-local targets are rejected before any RPC call. Tests cover paired-client rejection, local request behavior, and valid and invalid browse paths. Priority: ➖ Normal Merge Risk: 🔵 Low · up to On Windows, selecting a drive from the server browser now fails. This is a bounded navigation issue; the previously reported paired-client skill-operation gaps have been closed. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Allow an already-allowed worktree root outside the home directory in 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/rpc/methods/skills.ts (1)
97-104: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-862): Missing Authorization
Reachability: External · Exploitability: Moderate
Guard
skills.deletefor paired callers.Pass
clientKindto the handler and callrejectPairedSkillMutation(clientKind, 'Deleting skills')beforerunSkillDeleteRequest. Addskills.deleteto the paired-client test matrix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: fe4c6d69-af7f-4b2d-8d58-557194d3ec71
📥 Commits
Reviewing files that changed from the base of the PR and between 720c329 and 29770736154f58430205c71dbc39e6d12a88bcb4.
📒 Files selected for processing (9)
src/main/runtime/orca-runtime-tests/repository-project-operations.spec.tssrc/main/runtime/rpc/methods/client-ui.test.tssrc/main/runtime/rpc/methods/client-ui.tssrc/main/runtime/rpc/methods/computer.test.tssrc/main/runtime/rpc/methods/computer.tssrc/main/runtime/rpc/methods/skills.test.tssrc/main/runtime/rpc/methods/skills.tssrc/main/runtime/runtime-server-environment-commands.test.tssrc/main/runtime/runtime-server-environment-commands.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered the two follow-up commits since the prior review, both closing open threads:
- Gated
skills.beginUpload/skills.uploadChunk— paired callers now hitrejectPairedSkillMutationbefore opening an upload session or appending chunk bytes, closing the staged-but-never-committed upload hole. The paired-caller test matrix now includes both methods. - Realpath browse bound —
resolveAllowedServerBrowsePathresolvesrealpathon bothhomedir()and the target beforeisPathInsideOrEqual, closing the symlink escape; a regression test covers a home symlink pointing outside home.
ℹ️ Realpath now also denies home symlinks to external storage
Resolving realpath on the target means a directory that is lexically inside $HOME but symlinks outside it (e.g. ~/projects -> /mnt/data/projects) is now rejected, even though the earlier lexical check would have listed it. That is the correct tightening for the paired-client threat, but it applies just as much to the local host browser and headless orca serve hosts whose large repos commonly live on a symlinked external drive. Worth confirming this is the intended reading of "browse limited to the home directory."
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…unded dir browse Paired mobile and runtime sockets could drive computer-use, install skills, persist agentDefaultEnv, and readdir any absolute path through the shared RPC dispatcher. Gate computer.* and skill install/remove/commit to local unix-socket callers, strip launch env/args from paired settings.update, and limit files.browseServerDir to the home directory. Fixes stablyai#18269
files.browseServerDir used a lexical home check, so a symlink inside $HOME that pointed outside (e.g. ~/link -> /etc) still listed the target. Resolve realpath of the requested path and of homedir before isPathInsideOrEqual. skills.beginUpload and skills.uploadChunk were still reachable from paired clients after commitUpload was gated, so a client could fill disk with staged chunks. Apply the same host-only rejectPairedSkillMutation gate.
0dc136c to
6bf19ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Reject paired runtime callers from both cancellation methods. · skills.ts:162-163
src/main/runtime/rpc/methods/skills.ts:162-163
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick winAuthorization Bypass
Reachability: External
Exploitability: Moderate
CWE: CWE-862 — Missing AuthorizationReject paired runtime callers from both cancellation methods.
The mobile allowlist blocks these method names, but paired runtime callers can reach them. Both handlers omit
clientKind, and both runtime methods cancel host-owned state using only an ID. Add the same host-only guard used by the other skill mutations.Proposed fix
- handler: (params, { runtime }) => ({ - cancelled: runtime.cancelSharedSkillInstall(params.operationId) - }) + handler: (params, { runtime, clientKind }) => { + rejectPairedSkillMutation(clientKind, 'Cancelling skill installation') + return { cancelled: runtime.cancelSharedSkillInstall(params.operationId) } + } ... - handler: (params, { runtime }) => runtime.cancelSkillUpload(params.uploadId) + handler: (params, { runtime, clientKind }) => { + rejectPairedSkillMutation(clientKind, 'Cancelling skill upload') + return runtime.cancelSkillUpload(params.uploadId) + }
🟠 Major · Reject paired callers before deleting skills. · skills.ts:97-104
src/main/runtime/rpc/methods/skills.ts:97-104
🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject paired callers before deleting skills.
skills.deleteis callable throughcallRuntimeRpcfor capable remote runtimes. Its handler omitsrejectPairedSkillMutation, unlike the adjacent skill mutations. The request reachesrunSkillDeleteRequest, which binds non-WSL targets tonativeSkillInstallFilesystemand removes staged skill paths. A paired mobile or runtime caller can therefore delete skills on the executing host.- handler: async (params, { runtime }) => - runSkillDeleteRequest( + handler: async (params, { runtime, clientKind }) => { + rejectPairedSkillMutation(clientKind, 'Deleting skills') + return runSkillDeleteRequest( params, resolveDiscoveryTarget(params.target ?? {}, runtime), skillDeleteDependencies(runtime) ) + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d8a97c8b-a7e4-48b8-9efa-d2e54e4de8a8
📥 Commits
Reviewing files that changed from the base of the PR and between 29770736154f58430205c71dbc39e6d12a88bcb4 and 6bf19ed.
📒 Files selected for processing (8)
src/main/runtime/rpc/methods/client-ui.test.tssrc/main/runtime/rpc/methods/client-ui.tssrc/main/runtime/rpc/methods/computer.test.tssrc/main/runtime/rpc/methods/computer.tssrc/main/runtime/rpc/methods/skills.test.tssrc/main/runtime/rpc/methods/skills.tssrc/main/runtime/runtime-server-environment-commands.test.tssrc/main/runtime/runtime-server-environment-commands.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
Paired mobile/runtime clients could still delete host skills or cancel host-owned install/upload state. Gate those RPCs the same way as the other skill mutations.
|
Paired callers can no longer reach |
There was a problem hiding this comment.
Important
The new paired-client gate on skills.delete also denies Orca's own desktop-to-remote-runtime skill management, which the renderer explicitly implements — see the inline note.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review (commit c9afaaf), which extends the paired-client deny to three more skill methods:
skills.deletegated —rejectPairedSkillMutation(clientKind, 'Deleting skills')now runs beforerunSkillDeleteRequest.skills.cancelInstall/skills.cancelUploadgated — both cancellation handlers now reject paired callers;skills.sharewas refactored onto the shared helper with its message preserved.- Test matrix extended —
cancelInstall/cancelUploadjoin the paired-caller rejection table, and a newskills.deletetest asserts the deny precedeslistRepos.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Runtime-scoped pairing is the same WebSocket as web/phone. Keep delete host-local so the UI does not offer an action the RPC already denies.
There was a problem hiding this comment.
Important
The renderer half of the fix hides the delete affordance for {kind:'environment'} targets but not for the paired web client, whose active skill target resolves to {kind:'local'} — see the inline note.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review (commit c9afaaf), which the author answered by choosing to hide rather than exempt first-party clients:
- Hid skill delete for paired targets —
runtimeTargetSupportsSkillDeletenow returnsfalsefor any target whosekind !== 'local', andassertSkillDeleteSupportedthrowsPAIRED_SKILL_DELETE_UNSUPPORTED_MESSAGEbefore any RPC, so the desktop renderer no longer sendsskills.delete/skills.previewDeleteover a runtime-scoped pairing. - Reconciled delete tests — the environment-target tests now assert the deny path and that the remote capability probe is never consulted.
ℹ️ "Update Orca" is shown for a paired target that no update can enable
useSkillDeleteFlow's unsupportedReason treats "unsupported" as either a stale host or an unresolved target, so a focused paired runtime now renders "Update Orca on the selected machine to delete skills." Since runtimeTargetSupportsSkillDelete returns false for every non-local target, updating Orca on the selected machine cannot turn the action back on; the flow needs a third reason that names the host-only restriction instead.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Web always rides a pairing token, so deleteSupported is false and preview/delete reject locally. Host previewDelete now fail-closes the same way as delete. Browse tests compare realpath.
There was a problem hiding this comment.
ℹ️ No critical issues — the paired-web delete gap is closed. One minor test-robustness nit inline.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review (commit aecb7b9), which closes the paired-web delete gap:
- Host gates
skills.previewDelete— the handler now runsrejectPairedSkillMutation(clientKind, 'Deleting skills')before preview work, matchingskills.delete, so a paired caller cannot even probe the delete plan. - Web delete affordance removed —
createSkillsApi().deleteSupportedis now unconditionallyfalseand the webpreviewDelete/deletereject locally, so a paired web/phone client no longer offers an action the host always rejects. - Tests —
skills.test.tsparameterizes the paired-rejection case overpreviewDeleteanddelete; a newweb-host-capability-api.test.tsasserts the web probe staysfalseeven when the host advertisesskills.delete.v1; the browse suite now comparesrealpath.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Keep the home-bound guard and disable drive-root… · runtime-server-environment-commands.ts:56
src/main/runtime/runtime-server-environment-commands.ts:56
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the home-bound guard and disable drive-root navigation.
The
/response only enumerates Windows drives. A drive row is then treated as a directory, and clicking it callsbrowseServerDir('C:\\'). The server rejects that path because it is outside the home directory. Do not bypass the guard. Prevent drive-root rows from entering the directory-navigation path.Suggested fix
diff --git a/src/renderer/src/components/sidebar/remote-file-browser-helpers.ts b/src/renderer/src/components/sidebar/remote-file-browser-helpers.ts @@ export function joinPath( resolvedPath: string, name: string, pathFlavor: FilesystemPathFlavor = 'posix' ): string { @@ return resolvedPath === '/' ? `/${name}` : `${resolvedPath}/${name}` } +export function isDriveRootEntry( + resolvedPath: string, + name: string, + pathFlavor: FilesystemPathFlavor = 'posix' +): boolean { + return pathFlavor === 'win32' && resolvedPath === '/' && isDrivePath(name) +} + export function parentPath(p: string, pathFlavor: FilesystemPathFlavor = 'posix'): string {diff --git a/src/renderer/src/components/sidebar/RemoteFileBrowser.tsx b/src/renderer/src/components/sidebar/RemoteFileBrowser.tsx @@ -import { filterEntries, joinPath, parentPath, type DirEntry } from './remote-file-browser-helpers' +import { + filterEntries, + isDriveRootEntry, + joinPath, + parentPath, + type DirEntry +} from './remote-file-browser-helpers' @@ const navigateInto = useCallback( (name: string) => { + if (isDriveRootEntry(resolvedPath, name, pathFlavor)) { + return + } navigate(joinPath(resolvedPath, name, pathFlavor)) }, @@ - if (entry.isDirectory) { - navigate(joinPath(listParentPath, entry.name, pathFlavor)) + if (entry.isDirectory) { + if (!isDriveRootEntry(listParentPath, entry.name, pathFlavor)) { + navigate(joinPath(listParentPath, entry.name, pathFlavor)) + } } else {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d438763e-ed89-4c5c-a9a3-4c7c955264b4
📒 Files selected for processing (5)
src/main/runtime/rpc/methods/skills.test.tssrc/main/runtime/rpc/methods/skills.tssrc/main/runtime/runtime-server-environment-commands.test.tssrc/renderer/src/web/preload-api/web-host-capability-api.test.tssrc/renderer/src/web/preload-api/web-host-capability-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/runtime/runtime-server-environment-commands.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
The command returns the canonical path; a symlinked $HOME made the unresolved mkdtemp assertion host-dependent.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review (commit 75ae5f3), a single test-only follow-up:
- Realpath browse assertion —
repository-project-operations.spec.tsnow comparesbrowseServerDir'sresolvedPathagainstawait realpath(tempRoot)instead of the unresolvedmkdtemppath, so the assertion no longer depends on$HOMEbeing canonical on the runner host. This was the one open Pullfrog thread and it is now resolved; no production behavior changed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
ELI5
Paired web/phone clients were allowed to click the host mouse, install skill packages, set agent environment variables like PATH, and list any folder on the machine. Those actions now only work from the Orca host itself; directory browsing is limited to the home folder.
What Changed
computer.*RPC methods reject pairedclientKind(mobile/runtime). Local unix-socket callers (clientKind === undefined) still reach the sidecar.skills.install,skills.installBundle,skills.removeInstall, andskills.commitUploadthrow the same unsupported-environment errorskills.sharealready used for paired clients.settings.updatestill persists other settings, butagentDefaultEnvandagentDefaultArgsare stripped. Host settings types are unchanged.files.browseServerDir/browseDirectoryrefuses paths outsideos.homedir()(null bytes still rejected; Windows/still lists drive roots only).Why
The RPC dispatcher is shared by the local unix-socket CLI and paired WebSocket clients. Mobile tokens have an allowlist; runtime-scoped pairing did not. A paired client could drive host input, install skills, persist launch env, and
readdirarbitrary absolute paths.accounts.addClaudeFromConfigDiralready documented the local-socket-only rule; this applies that gate to the privileged methods above.Removing paired access is a visible behavior change for old clients (remote-wire Rule 1/3). Fail with an error string, not silence: old paired clients must not keep OS-input.
Linked Issue
Fixes #18269
Visual Proof
N/A — no visual or interaction change. This is a host RPC authorization bound.
Testing
Verification run:
pnpm tc:node— exit 0pnpm test src/main/runtime/rpc/methods— passpnpm test src/main/runtime/runtime-rpc/runtime-rpc-mobile-method-allowlist.test.ts— passpnpm test src/main/runtime/orca-runtime.test.ts src/main/runtime/rpc/methods/files.test.ts— pass (repository-project-operations.spec.tsis loaded viaorca-runtime.test.ts)pnpm test src/main/runtime/runtime-server-environment-commands.test.ts— passpnpm run check:code-quality:changed— passPlatforms covered by tests: macOS (this run). Browse deny uses
/etcon POSIX andC:\Windowson Windows; Windows/drive listing is unchanged (it.runIf(win32)).Review
PATH/LD_PRELOAD-style launch env, or list arbitrary host directories. Runtime-scoped pairing hits the same denies as mobile; this does not rely onMOBILE_RPC_METHOD_ALLOWLIST.isPathInsideOrEqual(Windows case/UNC-aware). Drive listing on win32/still returns drive roots only; walkingD:\srcvia browse is denied rather than re-opening/.orca serve/ paired runtime), including headless. Loss of contact is not involved; this is request-timeclientKinddenial. Browse uses the host'sos.homedir().stat/readdir).files.browseServerDirremains on the mobile allowlist but is now homedir-bound.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)