mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
fix(config): global skill symlink guard, tests, and empty-name handling (#1992)
Review feedback from @trek-e — three blocking issues and one style fix:
1. **Symlink escape guard** — Added validatePath() call on the resolved
global skill path with allowAbsolute: true. This routes the path
through the existing symlink-resolution and containment logic in
security.cjs, preventing a skill directory symlinked to an arbitrary
location from being injected. The name regex alone prevented
traversal in the literal name but not in the underlying directory.
2. **5 new tests** covering the global: code path:
- global:valid-skill resolves and appears in output
- global:invalid!name rejected by regex, skipped without crash
- global:missing-skill (directory absent) skipped gracefully
- Mix of global: and project-relative paths both resolve
- global: with empty name produces clear warning and skips
3. **Explicit empty-name guard** — Added before the regex check so
"global:" produces "empty skill name" instead of the confusing
'Invalid global skill name ""'.
4. **Style fix** — Hoisted require('os') and globalSkillsBase
calculation out of the loop, alongside the existing validatePath
import at the top of buildAgentSkillsBlock.
All 16 agent-skills tests pass.
This commit is contained in:
@@ -1456,6 +1456,8 @@ function cmdInitRemoveWorkspace(cwd, name, raw) {
|
||||
*/
|
||||
function buildAgentSkillsBlock(config, agentType, projectRoot) {
|
||||
const { validatePath } = require('./security.cjs');
|
||||
const os = require('os');
|
||||
const globalSkillsBase = path.join(os.homedir(), '.claude', 'skills');
|
||||
|
||||
if (!config || !config.agent_skills || !agentType) return '';
|
||||
|
||||
@@ -1473,17 +1475,30 @@ function buildAgentSkillsBlock(config, agentType, projectRoot) {
|
||||
// Support global: prefix for skills installed at ~/.claude/skills/ (#1992)
|
||||
if (skillPath.startsWith('global:')) {
|
||||
const skillName = skillPath.slice(7);
|
||||
// Explicit empty-name guard before regex for clearer error message
|
||||
if (!skillName) {
|
||||
process.stderr.write(`[agent-skills] WARNING: "global:" prefix with empty skill name — skipping\n`);
|
||||
continue;
|
||||
}
|
||||
// Sanitize: skill name must be alphanumeric, hyphens, or underscores only
|
||||
if (!/^[a-zA-Z0-9_-]+$/.test(skillName)) {
|
||||
process.stderr.write(`[agent-skills] WARNING: Invalid global skill name "${skillName}" — skipping\n`);
|
||||
continue;
|
||||
}
|
||||
const globalSkillDir = path.join(require('os').homedir(), '.claude', 'skills', skillName);
|
||||
const globalSkillDir = path.join(globalSkillsBase, skillName);
|
||||
const globalSkillMd = path.join(globalSkillDir, 'SKILL.md');
|
||||
if (!fs.existsSync(globalSkillMd)) {
|
||||
process.stderr.write(`[agent-skills] WARNING: Global skill not found at "~/.claude/skills/${skillName}/SKILL.md" — skipping\n`);
|
||||
continue;
|
||||
}
|
||||
// Symlink escape guard: validatePath resolves symlinks and enforces
|
||||
// containment within globalSkillsBase. Prevents a skill directory
|
||||
// symlinked to an arbitrary location from being injected (#1992).
|
||||
const pathCheck = validatePath(globalSkillMd, globalSkillsBase, { allowAbsolute: true });
|
||||
if (!pathCheck.safe) {
|
||||
process.stderr.write(`[agent-skills] WARNING: Global skill "${skillName}" failed path check (symlink escape?) — skipping\n`);
|
||||
continue;
|
||||
}
|
||||
validPaths.push({ ref: `${globalSkillDir}/SKILL.md`, display: `~/.claude/skills/${skillName}` });
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -205,3 +205,90 @@ describe('config-set agent_skills', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── global: prefix support (#1992) ──────────────────────────────────────────
|
||||
|
||||
describe('agent-skills global: prefix', () => {
|
||||
let tmpDir;
|
||||
let fakeHome;
|
||||
let globalSkillsDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
// Create a fake HOME with ~/.claude/skills/ structure
|
||||
fakeHome = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gsd-1992-home-'));
|
||||
globalSkillsDir = path.join(fakeHome, '.claude', 'skills');
|
||||
fs.mkdirSync(globalSkillsDir, { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
fs.rmSync(fakeHome, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
function createGlobalSkill(name) {
|
||||
const skillDir = path.join(globalSkillsDir, name);
|
||||
fs.mkdirSync(skillDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(skillDir, 'SKILL.md'), `# ${name}\nGlobal skill content.\n`);
|
||||
return skillDir;
|
||||
}
|
||||
|
||||
test('global:valid-skill resolves to $HOME/.claude/skills/valid-skill/SKILL.md', () => {
|
||||
createGlobalSkill('valid-skill');
|
||||
writeConfig(tmpDir, {
|
||||
agent_skills: { 'gsd-executor': ['global:valid-skill'] },
|
||||
});
|
||||
|
||||
const result = runGsdTools(['agent-skills', 'gsd-executor'], tmpDir, { HOME: fakeHome, USERPROFILE: fakeHome });
|
||||
assert.ok(result.output.includes('valid-skill/SKILL.md'), `should reference the global skill: ${result.output}`);
|
||||
assert.ok(result.output.includes('<agent_skills>'), 'should emit agent_skills block');
|
||||
});
|
||||
|
||||
test('global:invalid!name is rejected by regex and skipped', () => {
|
||||
writeConfig(tmpDir, {
|
||||
agent_skills: { 'gsd-executor': ['global:invalid!name'] },
|
||||
});
|
||||
|
||||
const result = runGsdTools(['agent-skills', 'gsd-executor'], tmpDir, { HOME: fakeHome, USERPROFILE: fakeHome });
|
||||
// No valid skills → empty output, command succeeds
|
||||
assert.strictEqual(result.output, '', 'should skip invalid name without crashing');
|
||||
});
|
||||
|
||||
test('global:missing-skill is skipped when directory is absent', () => {
|
||||
// Do NOT create the skill directory
|
||||
writeConfig(tmpDir, {
|
||||
agent_skills: { 'gsd-executor': ['global:missing-skill'] },
|
||||
});
|
||||
|
||||
const result = runGsdTools(['agent-skills', 'gsd-executor'], tmpDir, { HOME: fakeHome, USERPROFILE: fakeHome });
|
||||
assert.strictEqual(result.output, '', 'should skip missing skill gracefully');
|
||||
});
|
||||
|
||||
test('mix of global: and project-relative paths both resolve correctly', () => {
|
||||
createGlobalSkill('shadcn');
|
||||
|
||||
// Create a project-relative skill
|
||||
const projectSkillDir = path.join(tmpDir, 'skills', 'local-skill');
|
||||
fs.mkdirSync(projectSkillDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(projectSkillDir, 'SKILL.md'), '# local\n');
|
||||
|
||||
writeConfig(tmpDir, {
|
||||
agent_skills: { 'gsd-executor': ['global:shadcn', 'skills/local-skill'] },
|
||||
});
|
||||
|
||||
const result = runGsdTools(['agent-skills', 'gsd-executor'], tmpDir, { HOME: fakeHome, USERPROFILE: fakeHome });
|
||||
assert.ok(result.output.includes('shadcn/SKILL.md'), 'should include global shadcn');
|
||||
assert.ok(result.output.includes('skills/local-skill/SKILL.md'), 'should include project-relative skill');
|
||||
});
|
||||
|
||||
test('global: with empty name produces clear warning and skips', () => {
|
||||
writeConfig(tmpDir, {
|
||||
agent_skills: { 'gsd-executor': ['global:'] },
|
||||
});
|
||||
|
||||
const result = runGsdTools(['agent-skills', 'gsd-executor'], tmpDir, { HOME: fakeHome, USERPROFILE: fakeHome });
|
||||
assert.strictEqual(result.output, '', 'should skip empty global: prefix');
|
||||
// The warning goes to stderr — cannot assert on it through runGsdTools's output field,
|
||||
// but the command must not crash and must return empty.
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user