Skip to content

Commit 821fc1d

Browse files
committed
fix(security): address path traversal and CRLF vulnerabilities
- agent.ts: Sanitize skill.name when building default filename - publish.ts: Add sanitizeSkillName() to validate skill names before using in file paths, verify resolved paths stay within target dir - skill-converter.ts: Normalize CRLF to LF before extracting body content to handle Windows-style line endings - wellknown.ts: Add sanitizeSkillName() to validate remote skill names, verify resolved paths stay within temp dir, encode URL components
1 parent 8d70df6 commit 821fc1d

File tree

3 files changed

+74
-13
lines changed

3 files changed

+74
-13
lines changed

packages/cli/src/commands/publish.ts

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,21 @@
11
import { existsSync, readFileSync, mkdirSync, writeFileSync, readdirSync, statSync } from 'node:fs';
2-
import { join, basename, dirname } from 'node:path';
2+
import { join, basename, dirname, resolve } from 'node:path';
33
import chalk from 'chalk';
44
import { Command, Option } from 'clipanion';
55
import { generateWellKnownIndex, type WellKnownSkill } from '@skillkit/core';
66

7+
function sanitizeSkillName(name: string): string | null {
8+
if (!name || typeof name !== 'string') return null;
9+
const base = basename(name);
10+
if (base !== name || name.includes('..') || name.includes('/') || name.includes('\\')) {
11+
return null;
12+
}
13+
if (!/^[a-zA-Z0-9._-]+$/.test(name)) {
14+
return null;
15+
}
16+
return name;
17+
}
18+
719
interface SkillFrontmatter {
820
name?: string;
921
description?: string;
@@ -61,19 +73,33 @@ export class PublishCommand extends Command {
6173

6274
const wellKnownSkills: WellKnownSkill[] = [];
6375

76+
const validSkills: Array<{ name: string; safeName: string; description?: string; path: string }> = [];
77+
6478
for (const skill of discoveredSkills) {
79+
const safeName = sanitizeSkillName(skill.name);
80+
if (!safeName) {
81+
console.log(chalk.yellow(` ${chalk.yellow('⚠')} Skipping "${skill.name}" (invalid name - must be alphanumeric with hyphens/underscores)`));
82+
continue;
83+
}
84+
6585
const files = this.getSkillFiles(skill.path);
66-
console.log(chalk.dim(` ${chalk.green('●')} ${skill.name}`));
86+
console.log(chalk.dim(` ${chalk.green('●')} ${safeName}`));
6787
console.log(chalk.dim(` Description: ${skill.description || 'No description'}`));
6888
console.log(chalk.dim(` Files: ${files.join(', ')}`));
6989

90+
validSkills.push({ name: skill.name, safeName, description: skill.description, path: skill.path });
7091
wellKnownSkills.push({
71-
name: skill.name,
92+
name: safeName,
7293
description: skill.description,
7394
files,
7495
});
7596
}
7697

98+
if (validSkills.length === 0) {
99+
console.error(chalk.red('\nNo valid skills to publish'));
100+
return 1;
101+
}
102+
77103
console.log('');
78104

79105
if (this.dryRun) {
@@ -94,14 +120,23 @@ export class PublishCommand extends Command {
94120
const wellKnownDir = join(outputDir, '.well-known', 'skills');
95121
mkdirSync(wellKnownDir, { recursive: true });
96122

97-
for (const skill of discoveredSkills) {
98-
const skillDir = join(wellKnownDir, skill.name);
123+
for (const skill of validSkills) {
124+
const skillDir = join(wellKnownDir, skill.safeName);
125+
const resolvedSkillDir = resolve(skillDir);
126+
const resolvedWellKnownDir = resolve(wellKnownDir);
127+
128+
if (!resolvedSkillDir.startsWith(resolvedWellKnownDir)) {
129+
console.log(chalk.yellow(` Skipping "${skill.name}" (path traversal detected)`));
130+
continue;
131+
}
132+
99133
mkdirSync(skillDir, { recursive: true });
100134

101135
const files = this.getSkillFiles(skill.path);
102136
for (const file of files) {
137+
const safeFile = basename(file);
103138
const sourcePath = join(skill.path, file);
104-
const destPath = join(skillDir, file);
139+
const destPath = join(skillDir, safeFile);
105140
const content = readFileSync(sourcePath, 'utf-8');
106141
writeFileSync(destPath, content);
107142
}

packages/core/src/agents/skill-converter.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ function appendYamlList(lines: string[], key: string, items?: string[]): void {
209209

210210
/**
211211
* Extract body content from skill markdown (without frontmatter)
212+
* Handles both Unix (LF) and Windows (CRLF) line endings
212213
*/
213214
function extractBodyContent(content: string): string {
214215
const normalized = content.replace(/\r\n/g, '\n');

packages/core/src/providers/wellknown.ts

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,23 @@
11
import { existsSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
2-
import { join, basename } from 'node:path';
2+
import { join, basename, resolve } from 'node:path';
33
import { tmpdir } from 'node:os';
44
import { randomUUID } from 'node:crypto';
55
import type { GitProviderAdapter, CloneOptions } from './base.js';
66
import { isGitUrl, isLocalPath } from './base.js';
77
import type { GitProvider, CloneResult } from '../types.js';
88

9+
function sanitizeSkillName(name: string): string | null {
10+
if (!name || typeof name !== 'string') return null;
11+
const base = basename(name);
12+
if (base !== name || name.includes('..') || name.includes('/') || name.includes('\\')) {
13+
return null;
14+
}
15+
if (!/^[a-zA-Z0-9._-]+$/.test(name)) {
16+
return null;
17+
}
18+
return name;
19+
}
20+
921
export interface WellKnownSkill {
1022
name: string;
1123
description?: string;
@@ -94,18 +106,31 @@ export class WellKnownProvider implements GitProviderAdapter {
94106
const baseSkillsUrl = calculateBaseSkillsUrl(foundUrl);
95107

96108
for (const skill of index.skills) {
97-
const skillDir = join(tempDir, skill.name);
109+
const safeName = sanitizeSkillName(skill.name);
110+
if (!safeName) {
111+
continue;
112+
}
113+
114+
const skillDir = join(tempDir, safeName);
115+
const resolvedSkillDir = resolve(skillDir);
116+
const resolvedTempDir = resolve(tempDir);
117+
118+
if (!resolvedSkillDir.startsWith(resolvedTempDir + '/') && resolvedSkillDir !== resolvedTempDir) {
119+
continue;
120+
}
121+
98122
mkdirSync(skillDir, { recursive: true });
99123

100124
let hasSkillMd = false;
101125

102126
for (const file of skill.files) {
103-
const fileUrl = `${baseSkillsUrl}/${skill.name}/${file}`;
127+
const fileUrl = `${baseSkillsUrl}/${encodeURIComponent(skill.name)}/${encodeURIComponent(file)}`;
104128
try {
105129
const response = await fetch(fileUrl);
106130
if (response.ok) {
107131
const content = await response.text();
108-
writeFileSync(join(skillDir, basename(file)), content);
132+
const safeFileName = basename(file);
133+
writeFileSync(join(skillDir, safeFileName), content);
109134

110135
if (file === 'SKILL.md' || file.endsWith('/SKILL.md')) {
111136
hasSkillMd = true;
@@ -117,10 +142,10 @@ export class WellKnownProvider implements GitProviderAdapter {
117142
}
118143

119144
if (hasSkillMd) {
120-
skills.push(skill.name);
145+
skills.push(safeName);
121146
discoveredSkills.push({
122-
name: skill.name,
123-
dirName: skill.name,
147+
name: safeName,
148+
dirName: safeName,
124149
path: skillDir,
125150
});
126151
}

0 commit comments

Comments
 (0)