fix(opencode): reject path traversal in remote skill discovery#5
Conversation
Ported from upstream anomalyco#36364.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSkill discovery now validates remote skill names, file paths, filesystem roots, and URL origins before downloading or caching files. Tests add hostile metadata cases covering traversal, absolute paths, and cross-origin URLs. ChangesSecure skill discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery
participant RemoteIndex
participant Validator
participant SkillServer
participant Cache
Discovery->>RemoteIndex: Fetch index.json
RemoteIndex-->>Discovery: Return skill metadata
Discovery->>Validator: Validate names, paths, roots, and origins
Validator-->>Discovery: Return safe download mappings
Discovery->>SkillServer: Download mapped files
SkillServer-->>Discovery: Return file contents
Discovery->>Cache: Stage and refresh validated skill files
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 23 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 4 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 22 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 7 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/test/skill/discovery.test.ts (1)
196-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood regression coverage; consider adding a case for the bare drive-letter name gap.
The four traversal/absolute/cross-origin cases are well-targeted and match the implementation's rejection paths. Given the
isSafeSegmentgap flagged indiscovery.ts(bare"C:"-style skill names), once that's hardened, a correspondinghostileSkills = [{ name: "C:", files: ["SKILL.md"] }]case would be a useful addition to lock in the fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/skill/discovery.test.ts` around lines 196 - 246, Add a live regression test alongside the existing traversal cases in discovery.test.ts for a bare drive-letter skill name: set hostileSkills to [{ name: "C:", files: ["SKILL.md"] }], reset hostileDownloads, call Discovery.Service.pull, and assert it returns no directories without downloading files.packages/opencode/src/skill/discovery.ts (1)
143-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo diagnostic logging when a skill is dropped due to unsafe
files[]entries.Skills missing
SKILL.mdget a warning (Lines 155-162), but skills rejected becausefiles.some(item => item === undefined)(Line 143-145) — i.e. containing a traversal/absolute/cross-origin file entry — are silently dropped with no log. This makes it hard for legitimate skill authors to debug why their skill vanished, unlike the missing-SKILL.mdcase which does warn.♻️ Suggested addition
if (files.some((item) => item === undefined)) { + yield* Effect.logWarning("skill entry has unsafe file path", { url: index, skill: skill.name }) return [] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/skill/discovery.ts` around lines 143 - 162, Add diagnostic warning logging in the skill mapping flow when files.some((item) => item === undefined) rejects a skill, including the skill identity and relevant index or file context. Preserve the existing empty-result behavior, and keep the warning consistent with the missing-SKILL.md logging in the surrounding discovery logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/skill/discovery.ts`:
- Around line 143-162: Add diagnostic warning logging in the skill mapping flow
when files.some((item) => item === undefined) rejects a skill, including the
skill identity and relevant index or file context. Preserve the existing
empty-result behavior, and keep the warning consistent with the missing-SKILL.md
logging in the surrounding discovery logic.
In `@packages/opencode/test/skill/discovery.test.ts`:
- Around line 196-246: Add a live regression test alongside the existing
traversal cases in discovery.test.ts for a bare drive-letter skill name: set
hostileSkills to [{ name: "C:", files: ["SKILL.md"] }], reset hostileDownloads,
call Discovery.Service.pull, and assert it returns no directories without
downloading files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87143fc4-e123-4cb2-b44b-e967b68aaec1
📒 Files selected for processing (2)
packages/opencode/src/skill/discovery.tspackages/opencode/test/skill/discovery.test.ts
…e entries Addresses CodeRabbit review findings on the anomalyco#36364 port.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Port of upstream anomalyco/opencode#36364.
Validates
skill.nameand everyfiles[]entry from remote skill indexes before any filesystem write: rejects traversal segments, absolute paths, URL-encoded escapes, cross-origin resources, and destinations outside the per-skill cache root.Verified:
bun test test/skill/discovery.test.ts(11 pass, includes new traversal regression tests) andbun typecheckfrom packages/opencode. Applied cleanly (no fork adaptations needed).Summary by CodeRabbit
Bug Fixes
Tests