Repository navigation
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe destination authority now rejects workspace paths equal to home, filesystem roots, or ancestors of home. It accepts trusted worktrees outside home. The SSH relay resolves worktrees from the host Git worktree list and compares real paths instead of trusting client paths. Folder-scoped SSH installs now fail with Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description includes the required sections, linked issue, testing details, security considerations, and checklist. However, it materially states that workspace destinations must be inside the host home directory, while the implementation and objectives support listed worktrees outside home. It also checks off the full lint, typecheck, test, and build requirement even though the notes state that full lint and build were skipped. Resolution Correct the What Changed and Notes sections to state that host-listed Git worktrees outside the home directory remain supported. Update the checklist to distinguish completed commands from commands deferred to CI, and remove the claim that the full lint and build commands passed. 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two regressions to resolve before merging: folder-workspace skill installs over SSH now always fail closed, and local worktrees/folders outside $HOME are now rejected.
Reviewed changes
- SSH relay destination resolution —
SkillInstallHandler.authority()now ignores the caller-suppliedpathand instead resolves a host worktree by runninggit worktree listagainst a path decoded from the worktree id (viasplitWorktreeIdForFilesystem), realpath-matching the listing, with git-capability fallback (worktree-list-z) and fail-closednullon any error. - Shared containment check —
resolveSkillInstallDestinationnow requires the workspace directory to be strictly insidehomeDirectory(requireContained(home, workspace)), replacing the priorrequireContained(workspace, workspace)no-op. - Tests — five new relay-handler cases (outside-home, home-itself, unlisted subdir, listed worktree, host-lookup-wins) plus a desktop destination test for outside-home/home-itself rejection.
The host-driven resolution for worktrees is sound: the relay's own git worktree list is authoritative, symlinks are neutralized by realpath, and client-supplied paths cannot win. Two concrete fallouts remain, both anchored inline below.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 27316e71-1f35-4806-83a2-d2e42e24dbfd
📥 Commits
Reviewing files that changed from the base of the PR and between b52614c and c5ed0a7e35198d8df112c771c06371432677da8c.
📒 Files selected for processing (5)
src/main/runtime/runtime-skill-install-authority.test.tssrc/main/skills/skill-install-destinations.test.tssrc/main/skills/skill-install-destinations.tssrc/relay/skill-install-handler.test.tssrc/relay/skill-install-handler.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| resolveFolderWorkspace: async (id) => | ||
| workspace?.kind === 'folder' && workspace.id === id ? workspace : null | ||
| workspace?.kind === 'folder' && workspace.id === id ? { id, path: workspace.path } : null |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorization Bypass (CWE-73)
Reachability: External · Exploitability: Moderate
Resolve SSH folder paths from host authority.
workspace.path is client-controlled and becomes the installation directory after only home containment. Resolve folder IDs through trusted host state, or reject SSH folder installs until that mapping exists.
There was a problem hiding this comment.
Caution
The folder-workspace restore re-opens the exact TM-04 hole this PR is meant to close: resolveFolderWorkspace trusts the caller-supplied path with no host-side validation.
Reviewed changes
- Scoped the containment check — introduced
mustContainInHomeonSkillInstallDestinationAuthorityand gatedrequireContained(home, workspace)behind it, so the local/desktop authority (which resolves paths from trusted host inventory) again accepts worktrees and folders outside$HOME. - Restored SSH folder installs —
resolveFolderWorkspacenow returns{ id, path: workspace.path }verbatim instead of routing the folder id through thegit worktree listlookup, and the relay setsmustContainInHome: true. - Added coverage — new tests for local out-of-home worktrees, and folder install/reject (under home vs. outside home) cases; worktree cases now use real
repo::pathids.
The worktree branch is sound: the id encodes a host path, git worktree list is authoritative, and realpath neutralizes symlinks. The folder branch is not — see the inline comment. This is the same concern as the prior folder thread, now in the opposite direction: the author fixed "folders never install" by making the caller path the filesystem authority again, which the plan's STOP condition explicitly forbids.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Reviewed against the plan and the threat model. This is the stronger version of this fix — I had built an overlapping one at #19866 before triage pointed me here, and I'm closing mine in favour of this. Two things I'd raise: 1.
The plan's own STOP condition points the same way from the other side — "containing under home still allows writing into 2. Folder workspaces still take the caller's
Small note: deriving the candidate from |
c5ed0a7 to
18960f4
Compare
|
Follow-up for the home-containment and TM-04 notes:
|
18960f4 to
0124f3a
Compare
There was a problem hiding this comment.
ℹ️ No new code issues in this delta — one deferred follow-up on SSH folder targets is inline.
Reviewed changes
- Proved SSH worktrees by host listing —
authority().resolveWorktreenow decodes the path from the worktree id (splitWorktreeIdForFilesystem) and accepts it only whengit worktree liston the relay lists its realpath (readRelayWorktreeListwith theworktree-list-zcapability fallback), failing closed on any error; the clientpathno longer wins. - Replaced home containment with degenerate-path rejection —
rejectDegenerateWorkspacerejects a workspace equal to$HOME, a filesystem root, or an ancestor of$HOME, and otherwise accepts inside- and outside-home worktrees, restoring local out-of-home installs. - Failed SSH folder installs closed —
resolveFolderWorkspacereturnsnull, so a folder id resolves toskill-install-workspace-not-foundinstead of the caller path. - Coverage — relay cases for listed worktrees under and outside home, home-itself, an unlisted
~/.ssh-style subdir, a client path ignored in favour of the host path, and folder rejection; destination cases for home, root, ancestor, and trusted local out-of-home.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Resolve SSH skill-install destinations from the host git-worktree catalog and contain workspace directories under the relay home so a caller path cannot become remote filesystem authority.
Folder workspace ids are UUIDs, so the relay uses the host folder path instead of git worktree list. Home containment stays on the SSH relay and is not applied to trusted local worktree and folder destinations.
Home containment rejected legitimate checkouts at /opt or /srv and still left ~/.ssh reachable for folder installs. Keep git-worktree membership as the host proof, reject home/root/ancestor destinations, and fail closed on SSH folders until the relay has a folder registry.
Relay folder installs fail closed, so listing them only produces skill-install-workspace-not-found after dispatch.
0124f3a to
1af71b9
Compare
There was a problem hiding this comment.
Important
The renderer descope is correct, but it leaves the SSH-docker e2e spec still asserting that folder installs succeed — that lane will now fail.
Reviewed changes
- Descoped SSH folder install targets —
belongsToEnvironmentreturns!folder && hostId === machineIdforssh:environments, soskillInstallWorkspaceChoicesno longer offers SSH folder workspaces while SSH worktrees remain; the folder case in the existing test flipped from one choice to zero, and a new case proves the worktree is kept alongside an omitted SSH folder.
The wire path is unchanged and the code previously reviewed at 0124f3a36 is untouched by this delta. The filter itself is correct: for ssh: environments folders are dropped, and the local and paired-runtime branches are unaffected.
⚠️ SSH-docker e2e still asserts folder installs succeed
tests/e2e/ssh-skill-installation.spec.ts creates a remote folder workspace and calls window.api.skills.installPackageVersion with { scope: 'workspace', folderWorkspaceId }, expecting status: 'installed' (lines 116-123), then asserts three package downloads at the end (lines 125-137). With resolveFolderWorkspace returning null, that call resolves to skill-install-workspace-not-found, so the spec fails at the folder step and the trailing assertions never run. The descope removed the capability from the picker but left the spec asserting it; the folder scenario should be dropped (or inverted to assert the fail-closed error) so the lane stays green and the spec keeps exercising the global and worktree installs.
Technical details
# SSH folder installs removed but e2e still expects success
## Affected sites
- `tests/e2e/ssh-skill-installation.spec.ts:116-123` — creates a remote folder workspace and expects the install to succeed.
- `tests/e2e/ssh-skill-installation.spec.ts:125-137` — download-count assertions expect three installs (global, worktree, folder).
- `src/relay/skill-install-handler.ts:223` — `resolveFolderWorkspace: async () => null` makes the folder dispatch fail closed.
- `config/scripts/run-ssh-docker-e2e.mjs:80` — the spec is claimed by the Docker SSH lane, which sets `ORCA_E2E_SSH_DOCKER=1`.
## Required outcome
- The spec reflects that SSH folder installs are unsupported: drop the folder install/remove scenario (and adjust the three-download assertions to two), or assert the `skill-install-workspace-not-found` failure explicitly.
## Open questions for the human
- Is the folder scenario removal acceptable, or does this signal the capability should be restored with a host-side folder registry?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The relay has no folder registry, and the picker no longer lists SSH folders. Keep the docker lane green by expecting admission failure instead of a successful third copy.
|
Addressed in 306e4cb — the SSH-docker spec now expects the folder install to fail closed with |
There was a problem hiding this comment.
Important
The inverted SSH folder scenario still asserts three package downloads, but the refused folder never downloads — see the inline comment.
Reviewed changes
- Inverted the SSH folder e2e scenario —
tests/e2e/ssh-skill-installation.spec.tsnow callsinstallPackageVersiondirectly for a remote folder workspace and asserts theskill-install-workspace-not-foundadmission failure plus the absence of.agents, replacing the previous folder install/remove success path.
The relay, shared destination, and renderer code are unchanged by this delta (reviewed at 0124f3a36); the worktree host-listing proof and the folder fail-closed behavior remain sound.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…tall Admission fails before ingress, so the folder attempt never GETs /package.tar.gz. The three POSTs stay: the grant is minted in IPC first.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Corrected the SSH folder e2e download count — the
/package.tar.gzGET assertion intests/e2e/ssh-skill-installation.spec.tsnow expects 2 (global + worktree) instead of 3, since the refused folder install fails admission before ingress and never fetches the archive; the comment was reworded to scope to the 3-POST grant assertion, which remains correct.
All prior Pullfrog threads are resolved. The delta is a one-line test-count correction that matches the established control flow (resolveSkillInstallDestination at skill-install-request-service.ts:95 runs before resolveIngress at :101), and the relay, destination, and renderer code are unchanged since the previously reviewed 0124f3a36.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing as obsolete after the merged SSH relay retirement in #27214 and #27215. This PR concerns the SSH relay server and its relay-only handlers. #27214 removed the desktop relay connection/fallback path; #27215 deleted the SSH relay server, deployment assets, and relay-only tests. SSH hosts now use the managed Orca server (orcad), and unsupported hosts fail explicitly, so this relay-specific work is no longer planned. This closes the proposed relay implementation; it does not claim that any independent local, WSL, or managed-server behavior was fixed. |
|
Thanks @fettpl — on current main the host resolves a workspace skill install from its own worktree/folder record by id ( |
ELI5
When you install a skill onto an SSH machine, the remote host now picks the real workspace folder itself. A client can no longer say “put it in
/etcor~/.ssh” and have the host obey.What Changed
requireContained(home, workspace)).workspace.pathas the install root. It looks the path up on the host: decode a path from the worktree id when present, then accept it only ifgit worktree liston that host lists that realpath.path(schema unchanged). The relay ignores it; host lookup wins.Why
Issue #18273 / TM-04 / TM-11: destination paths must be resolved by the runtime that owns the host. The relay previously returned the RPC
workspaceobject as-is, sorequireContained(workspace, workspace)was a no-op and a packaged skill tree could be written outside the intended worktree.Linked Issue
Fixes #18273
Visual Proof
N/A — no visual or interaction change. This is host-side path resolution for SSH skill install.
Testing
Commands from the plan (all passed):
pnpm test src/relay/skill-install-handler.test.tspnpm test src/main/skills/skill-install-destinations.test.tspnpm tc:nodepnpm run check:code-quality:changedCoverage:
~/.sshanalogue) → rejected even when the workspace id matches.join(worktree, '.agents', 'skills', name).workspace.pathpoints outside home → host lookup wins; install goes to the listed worktree, not the caller path.AI Disclosure
Review
path+realpathrun on the relay (the execution host OS). Git listing usesreadRelayWorktreeListwithGitCapabilityCache(worktree-list-zfallback for Git <2.36). Child git isrunProcess, not rawchild_process. Path compares useareRelayWorktreePathsEqual(Windows case).pathcannot win. Listing failure is fail-closed (workspace-not-found), not evidence of process death.~/.ssh/~/.gnupgunder home) became filesystem authority. Folder workspaces that are not listed git worktrees fail closed.pathstays required on the Zod schema so old clients still parse.git worktree listper workspace-scoped install; capability cache is process-scoped on the relay.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-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Desktop
resolveSkillInstallDestinationnow also contains workspace dirs underauthority.homeDirectory. Worktrees outside$HOMEwould be rejected; that matches the plan’s shared contain check.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Plan verification ran
pnpm tc:node, the two test files, andpnpm run check:code-quality:changed. Fullpnpm lint/pnpm buildwere skipped per the issue plan.