Repository navigation
Conversation
…from the request The relay's `authority()` returned the RPC `workspace` object verbatim when the id matched, caller `path` included, so whatever directory a request named became the install root on the execution host. The only containment check was `requireContained(workspaceDirectory, workspaceDirectory)`, true by construction and therefore asserting nothing (threat model TM-04/TM-11). Two checks replace it, both answered by the machine that owns the filesystem: - `resolveSkillInstallDestination` refuses a workspace directory that is a filesystem root, is the host's home directory, or contains it. A checkout under the home tree — the normal case — still installs. - For a worktree destination the relay now asks Git: `rev-parse --show-toplevel` inside the candidate must name the candidate itself, so only a real checkout root is accepted. `~/.ssh`, `~/.gnupg` and `/etc` are not. Mixed-version safe: `path` stays on the schema and old clients keep sending it, but it is a hint the host verifies rather than authority it grants. No new required field and no new stream opcode. Refs #18273
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Structural workspace rules —
requireContained(dir, dir)(a no-op) replaced byrequireWorkspaceDirectory, which rejects a filesystem root, the home tree itself, and any directory that contains the home tree, for both worktree and folder destinations on every host. - Host-side Git proof — the relay's
authority().resolveWorktreenow gates onisHostGitWorktreeRoot(git -C <candidate> rev-parse --show-toplevelmust name the candidate), so~/.ssh, a non-checkout directory, and a path merely inside a checkout are refused asskill-install-workspace-not-found. - Tests — structural escape cases in
skill-install-destinations.test.tsplus an end-to-end relay test covering refuse/install/refuse-nested against a realgit initcheckout, with the mutation table verifying each rule is load-bearing.
The fix is well-scoped and sound: the threat-model references (TM-04/TM-11) are accurate, --show-toplevel is Git 1.7-era so no capability probe is needed, and the shared resolveSkillInstallDestination change does not regress the local authority (legitimate worktrees sit under home and pass; WSL UNC paths resolve to a different root and also pass). The wire contract is unchanged — path stays required on .strict() schemas and becomes a host-verified hint, which old clients keep sending harmlessly.
ℹ️ Worktree proof verifies "a checkout root", not "the requested worktree"
The Git proof establishes that the candidate is some checkout root on the host, not that it is the worktree the session actually opened. Any git checkout the SSH user owns is accepted as a destination. This is bounded (the install root always has .agents/skills/<validated-name>/ appended) and stays within the same user's authority, so it is a precision limitation rather than an escalation — and it is the exact symmetric of the folder-workspace residual already documented in the PR body, which the author deferred to a host-side workspace registry. No change requested; noting it only so the follow-up issue covers worktrees as well as folders.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
📝 WalkthroughWalkthroughThe change validates local skill-install workspace destinations against filesystem roots and the execution host home directory. It adds Git-based validation for SSH relay destinations so only checkout roots are accepted. New tests cover rejected plain directories, accepted checkout roots, rejected nested checkout paths, and valid local workspaces. Merge Risk: 🟡 Moderate · up to Workspace installation may still target a WSL home directory or a directory substituted after Git validation. These destination-boundary issues should be resolved before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: b6231cb1-007a-4698-907d-510e9d676c4f
📒 Files selected for processing (5)
src/main/skills/skill-install-destinations.test.tssrc/main/skills/skill-install-destinations.tssrc/relay/skill-install-handler.tssrc/relay/skill-install-workspace-authority.test.tssrc/relay/skill-install-workspace-proof.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| 'skill-install-workspace-unavailable' | ||
| ) | ||
| requireContained(workspaceDirectory, workspaceDirectory) | ||
| requireWorkspaceDirectory(homeDirectory, workspaceDirectory) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the producer contract for WSL workspace identities and the filesystem consumer.
ast-grep outline src/main src/relay --items all --type function \
--match 'resolveWorktree|resolveFolderWorkspace|resolveWsl|createWslSkillInstallFilesystem'
rg -n -C 8 --type ts \
'\b(resolveWorktree|resolveFolderWorkspace|resolveWsl|wslDistro|createWslSkillInstallFilesystem)\b' \
src/main src/relayRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- destination implementation ---'
sed -n '1,180p' src/main/skills/skill-install-destinations.ts
printf '%s\n' '--- workspace resolver definitions ---'
rg -n -l --type ts 'resolveWorktree|resolveFolderWorkspace|type SkillInstallDestinationAuthority|interface SkillInstallDestinationAuthority' src/main src/relay \
| head -80Repository: stablyai/orca
Length of output: 9001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- skill-install authority wiring ---'
rg -n -C 12 --type ts \
'resolveSkillInstallDestination|resolveWorktree:|resolveFolderWorkspace:|resolveWsl:' \
src/main/skills src/relay
printf '%s\n' '--- workspace identity construction ---'
rg -n -C 8 --type ts \
'wslDistro:\s|wslDistro\?|path:\s|resolveWorktree\s*\(|resolveFolderWorkspace\s*\(' \
src/main/skills src/relay src/main/project-groups src/main/runtime \
| head -500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all production authority call sites ---'
rg -n -C 10 --type ts \
'SkillInstallDestinationAuthority|resolveWorktree:|resolveFolderWorkspace:|resolveWsl:' \
src/main src/relay \
| rg -v '(\.test\.ts|__tests__)' \
| head -500
printf '%s\n' '--- WSL workspace producers ---'
rg -n -C 12 --type ts \
'wslDistro.*workspace|workspace.*wslDistro|wslDistro.*path|path.*wslDistro|parseWslUncPath|wslHookRelayConnectionId' \
src/main src/relay \
| head -500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime authority ---'
sed -n '131,188p' src/main/runtime/runtime-skill-install-authority.ts
printf '%s\n' '--- WSL path and home helpers ---'
rg -n -C 8 --type ts 'function getWslHome|getWslHome\s*=|export .*getWslHome|parseWslUncPath' \
src/main src/shared \
| head -300
printf '%s\n' '--- WSL filesystem consumer ---'
fd -i 'skill-wsl-install-filesystem' src/main/skills --type f -x sh -c 'cat -n "$1"' sh {}Repository: stablyai/orca
Length of output: 48531
Path Traversal
Reachability: External
Exploitability: Moderate
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability path
● Entry
src/main/skills/skill-install-destinations.test.ts:130
│
▼
● Sink
src/main/skills/skill-install-destinations.ts
Validate WSL workspaces against the WSL home directory.
When workspace.wslDistro is set, resolve its WSL home with resolveWsl and pass that home to requireWorkspaceDirectory. Keep workspaceDirectory as the installation target. Add a test that rejects a WSL workspace equal to its distro home.
| if (workspace?.kind !== 'worktree' || workspace.id !== id) { | ||
| return null | ||
| } | ||
| return (await isHostGitWorktreeRoot(workspace.path)) ? workspace : null |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' src/relay/skill-install-workspace-proof.ts
sed -n '1,150p' src/main/skills/skill-install-destinations.ts
sed -n '1,150p' src/relay/skill-install-workspace-authority.test.ts
rg -n "requireWorkspaceDirectory|workspaceDirectory|install.*workspace|symlink|realpath" src/main/skills src/relay/skill-install-workspace-authority.test.tsRepository: stablyai/orca
Length of output: 42447
🏁 Script executed:
sed -n '1,125p' src/main/skills/skill-install-service.ts
sed -n '1,125p' src/main/skills/skill-install-request-service.ts
sed -n '1,125p' src/main/skills/skill-install-filesystem.ts
sed -n '1,220p' src/main/skills/skill-install-destinations.test.tsRepository: stablyai/orca
Length of output: 21414
Authorization Bypass
Reachability: External
Exploitability: Difficult
CWE: CWE-367 — Time-of-check Time-of-use (TOCTOU) Race Condition
Bind the destination to the Git-proven path.
requireDirectory rejects symlink entries, but it does not prevent replacement with a regular directory after Git validation. Return the canonical path from isHostGitWorktreeRoot and use it as workspace.path. Add a regression test for this replacement race.
|
Closing as a duplicate of #18357, which is the stronger version of this fix — it proves the destination against the host's own |
Refs #18273
ELI5
When you install a skill onto an SSH host, the request says "put it in worktree
worktree_1, which lives at<path>". The relay took that path at face valueand used it as the install root. The one containment check it ran was:
— a directory is always inside itself, so that assertion could never fail. Any
directory the request named became the destination on the host.
Now the host decides. It refuses places that obviously aren't a workspace, and
for a worktree it asks Git whether the directory is really a checkout.
What changed
1. Structural rules, in
resolveSkillInstallDestination(both kinds).The workspace directory may not be a filesystem root, may not be the host's home
directory, and may not contain the home directory. A checkout under the home
tree — the normal case — still installs. This replaces the vacuous call; the
plan's Step 1 "contain under home" was deliberately not used, because the
plan's own STOP condition rules it out: "any subdirectory of home" still permits
~/.sshand~/.gnupg.2. A Git proof, for worktree destinations, on the relay.
git -C <candidate> rev-parse --show-toplevelmust name the candidate itself.Only a checkout root answers with itself, so
~/.ssh,~/.gnupg,/etcand apath merely inside a checkout are all rejected as
skill-install-workspace-not-found.--show-toplevelis Git 1.7-era, so nocapability probe or fallback is needed.
The relay has no worktree catalog to look an id up in — an Orca worktree id is
client metadata, and
splitWorktreeIdForFilesystemderives the path from theid, so it is exactly as caller-controlled as
workspace.path. Verificationagainst the host's own filesystem is the only authority available here.
Mixed-version:
pathstays required on the schema and old clients keepsending it. It is now a hint the host verifies rather than authority it grants —
no new required field, no new stream opcode
(
docs/reference/remote-wire-compatibility.mdRules 1–2).Tests
src/main/skills/skill-install-destinations.test.ts— home directory, adirectory containing home, and a filesystem root all throw
skill-install-destination-escape; a real workspace under the home tree stillresolves.
src/relay/skill-install-workspace-authority.test.ts— a full staged-uploadinstall through the relay handler against three claimed paths:
~/.ssh(refused), a real
git initcheckout (installed,SKILL.mdwritten under.agents/skills/), and a subdirectory of that checkout (refused).Mutation results (5/5 killed)
workspacewith no host check (the bug)refuses a client path that is not a checkout on this host,refuses a path merely inside a checkout rather than its rootrefuses a path merely inside a checkout rather than its rootrejects the execution host home directory as a workspace,rejects a directory that contains the execution host homestill accepts a real workspace directory that sits under the home tree+ 1Known residual — folder workspaces
A folder workspace is not necessarily a Git repository, so the Git proof
cannot apply to it without breaking legitimate plain-folder workspaces, and the
relay has no other host-side registry to check it against. Folder destinations
therefore get the structural rules only. Blast radius is bounded — the install
root always has
.agents/skills/<validated-name>appended, so this creates adirectory rather than overwriting a named file — but a caller can still place
one in any writable directory on the host. Closing it properly needs a host-side
workspace registry (the relay learning which folder roots a session actually
opened), which is larger than this change and worth its own issue. Flagging
rather than shipping a guess.
Gates
pnpm tc,pnpm test src/relay src/main/skills …(2327 passed),oxlint,pnpm run check:code-quality:changed,pnpm run check:max-lines-ratchet,pnpm format.