diff --git a/.github/workflows/pull_requests.yml b/.github/workflows/pull_requests.yml index 6735dff0..8771b7b0 100644 --- a/.github/workflows/pull_requests.yml +++ b/.github/workflows/pull_requests.yml @@ -5,9 +5,18 @@ on: push: branches: [ master ] +# Default for every job below: read this repository's code and nothing else. +permissions: + contents: read + jobs: pre_job: runs-on: ubuntu-latest + # skip-duplicate-actions lists this workflow's previous runs to decide whether the + # GUI paths changed, which needs read access to the Actions API on top of the default. + permissions: + contents: read + actions: read outputs: should_skip: ${{ steps.skip_check.outputs.should_skip }} steps: diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 52fa2981..eef78058 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -6,6 +6,10 @@ on: pull_request: branches: [ master ] +# The token only ever needs to read this repository's code. +permissions: + contents: read + jobs: test: runs-on: ${{ matrix.os }} diff --git a/lib/dns/cliHelpers.js b/lib/dns/cliHelpers.js index 85b187ce..7edbe2f8 100644 --- a/lib/dns/cliHelpers.js +++ b/lib/dns/cliHelpers.js @@ -14,7 +14,37 @@ const collect = (value, previous) => previous.concat([value]); // --drop-value flags compile to case-insensitive regexes. All commands must compile // them identically, or compare's "pass the same patterns the migration dropped" // contract (TASK-1.11) silently stops matching. -const dropValuePatterns = (values) => values.map(pattern => new RegExp(pattern, 'i')); +// +// The pattern is the operator's own input, but it still reaches a regex engine mid-migration, +// so it is bounded before compiling. DNS record values are printable ASCII (international +// names arrive as punycode), which makes that the honest limit: anything outside it cannot +// match a record anyway, and a control character in a flag is a shell-quoting accident, not a +// pattern. The length cap keeps the compiled program small. Deliberately NOT restricted +// further: a narrower charset would reject legitimate patterns over TXT values, and rejecting +// nested quantifiers would also reject bounded ones like ([0-9]{1,3}\.){3} that never blow up. +const MAX_DROP_VALUE_PATTERN_LENGTH = 200; +const PRINTABLE_ASCII = /^[\x20-\x7e]+$/; + +const compileDropValuePattern = (pattern) => { + if (typeof pattern !== 'string' || pattern.length === 0) { + throw new Error('--drop-value needs a pattern, got an empty value'); + } + if (pattern.length > MAX_DROP_VALUE_PATTERN_LENGTH) { + throw new Error(`--drop-value pattern is ${pattern.length} characters, over the ${MAX_DROP_VALUE_PATTERN_LENGTH} character limit`); + } + if (!PRINTABLE_ASCII.test(pattern)) { + throw new Error(`--drop-value pattern "${pattern}" contains characters outside printable ASCII, which no DNS record value can match`); + } + + try { + return new RegExp(pattern, 'i'); + } catch (error) { + // Without this the operator gets a bare SyntaxError stack and no hint which flag caused it. + throw new Error(`--drop-value pattern "${pattern}" is not a valid regular expression: ${error.message}`); + } +}; + +const dropValuePatterns = (values) => (values || []).map(compileDropValuePattern); const filterByDomains = (items, domains, getName = (item) => item.domainName) => { if (!domains.length) return items; diff --git a/lib/server.js b/lib/server.js index 7e8f4847..755f25fa 100644 --- a/lib/server.js +++ b/lib/server.js @@ -2,6 +2,7 @@ import path from 'path'; import express from 'express'; import bodyParser from 'body-parser'; import multer from 'multer'; +import rateLimit from 'express-rate-limit'; import { fileURLToPath } from 'url'; import pkg from '../package.json' with { type: 'json' }; @@ -26,21 +27,43 @@ const start = (env, client) => { email: env.MARKETPLACE_EMAIL }, client); + // Gateway rejections are Error instances, so `res.send(error)` serialised them to `{}` + // under a 200 — the GUI saw a successful but empty response and then threw on the fields + // it expected. Forward the upstream status with the upstream body instead, and always as + // JSON so no error text is ever handed back for a browser to parse as HTML. + const sendError = (res, error) => { + const status = error?.statusCode || error?.response?.statusCode || 502; + const body = error?.response?.body; + + if (body && typeof body === 'object') return res.status(status).json(body); + return res.status(status).json({ error: (typeof body === 'string' && body) || error?.message || 'Request failed' }); + }; + const graphqlRouting = (req, res) => { gateway .graph(req.body) .then(body => res.send(body)) - .catch(error => res.send(error)); + .catch(error => sendError(res, error)); }; const liquidRouting = (req, res) => { gateway .liquid(req.body) .then(body => res.send(body)) - .catch(error => res.send(error)); + .catch(error => sendError(res, error)); }; app.use(bodyParser.json()); + // A ceiling on abuse, not a throttle on normal use: one GUI page load pulls dozens of + // static assets through the catch-all route, so the limit has to sit far above anything + // a developer clicking around can reach. + app.use(rateLimit({ + windowMs: 60 * 1000, + limit: 2000, + standardHeaders: true, + legacyHeaders: false + })); + app.use((req, res, next) => { res.header('Access-Control-Allow-Origin', '*'); res.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept'); @@ -65,7 +88,7 @@ const start = (env, client) => { gateway .logs({ lastId: req.query.lastId }) .then(body => res.send(body)) - .catch(error => res.send(error)); + .catch(error => sendError(res, error)); }); app.get('/api/logsv2', (req, res) => { @@ -75,7 +98,7 @@ const start = (env, client) => { res.send(body); }) .catch(error => { - logger.Debug(error); res.send(error); + logger.Debug(error); sendError(res, error); }); }); @@ -86,7 +109,7 @@ const start = (env, client) => { res.send(body); }) .catch(error => { - logger.Debug(error); res.send(error); + logger.Debug(error); sendError(res, error); }); }); @@ -106,7 +129,7 @@ const start = (env, client) => { gateway .sync(formData) .then(body => res.send(body)) - .catch(error => res.send(error)); + .catch(error => sendError(res, error)); } ); diff --git a/mcp-min/http-server.js b/mcp-min/http-server.js index d90a9580..86e430b2 100644 --- a/mcp-min/http-server.js +++ b/mcp-min/http-server.js @@ -1,5 +1,6 @@ import express from 'express'; import bodyParser from 'body-parser'; +import { randomUUID } from 'crypto'; import tools from './tools.js'; import { sseHandler, writeSSE } from './sse.js'; import { DEBUG } from './config.js'; @@ -8,8 +9,12 @@ import log from './log.js'; // SSE sessions keyed by Mcp-Session-Id. Supports multiple concurrent clients. const sseSessions = new Map(); +// A session id is the only thing separating one SSE client's stream from another's, so it +// has to be unguessable: Math.random() is seeded per process and its output is predictable +// from a couple of prior samples, which would let a local caller attach to someone else's +// session by supplying the Mcp-Session-Id it derived. function generateSessionId() { - return `mcpmin-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + return `mcpmin-${randomUUID()}`; } export default async function startHttp({ port = 5910 } = {}) { diff --git a/package-lock.json b/package-lock.json index 788c9eb8..d59b6876 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,6 +29,7 @@ "email-validator": "^2.0.4", "execa": "^10.0.1", "express": "^5.2.1", + "express-rate-limit": "^8.6.2", "fast-glob": "^3.3.3", "ignore": "^7.0.5", "inquirer": "^14.2.0", @@ -3683,9 +3684,9 @@ } }, "node_modules/express-rate-limit": { - "version": "8.6.0", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.6.0.tgz", - "integrity": "sha512-XKJXDsASUOo0LLtFwW5hCcQGH0N4WQc/Rn8/Pvoia+TJFOkkFPvrtW9lZOeeNcxQJspvOIERMwiRLsVFlhHEkA==", + "version": "8.6.2", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.6.2.tgz", + "integrity": "sha512-YH4ru+eOJxQABscKFfRCy9R7x9QFGdezclVMwwgFFndzS2Xnm0uo6B0ABZsLhcpeptGv2qvuJVWlQr9gQZoC3A==", "license": "MIT", "dependencies": { "debug": "^4.4.3", diff --git a/package.json b/package.json index e3cebab7..037d948b 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "email-validator": "^2.0.4", "execa": "^10.0.1", "express": "^5.2.1", + "express-rate-limit": "^8.6.2", "fast-glob": "^3.3.3", "ignore": "^7.0.5", "inquirer": "^14.2.0", diff --git a/test/global-setup.js b/test/global-setup.js index 6c8e221a..623d3ae3 100644 --- a/test/global-setup.js +++ b/test/global-setup.js @@ -1,24 +1,27 @@ -import { execSync } from 'child_process'; +import { execFileSync } from 'child_process'; import path from 'path'; import { fileURLToPath } from 'url'; import dotenv from 'dotenv'; +import { isExampleUrl } from './utils/credentials.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); -const cliPath = `node ${path.join(__dirname, '../bin/pos-cli.js')}`; +const cliScript = path.join(__dirname, '../bin/pos-cli.js'); export async function setup() { dotenv.config(); const { MPKIT_URL, MPKIT_TOKEN, MPKIT_EMAIL } = process.env; - if (!MPKIT_URL || !MPKIT_TOKEN || !MPKIT_EMAIL || MPKIT_URL.includes('example.com')) { + if (!MPKIT_URL || !MPKIT_TOKEN || !MPKIT_EMAIL || isExampleUrl(MPKIT_URL)) { console.log('[Global Setup] No real credentials found, skipping instance cleanup'); return; } console.log(`[Global Setup] Cleaning instance: ${MPKIT_URL}`); - execSync(`${cliPath} data clean --include-schema --auto-confirm`, { + // execFileSync, not execSync: the repo path goes into argv, and a shell would split it on + // spaces and re-read any metacharacter in it before node ever saw the script name. + execFileSync(process.execPath, [cliScript, 'data', 'clean', '--include-schema', '--auto-confirm'], { env: process.env, stdio: 'inherit' }); diff --git a/test/integration/ai.test.js b/test/integration/ai.test.js index 2e6cb31e..a34226fc 100644 --- a/test/integration/ai.test.js +++ b/test/integration/ai.test.js @@ -2,8 +2,7 @@ import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest'; import fs from 'fs'; import os from 'os'; import path from 'path'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; vi.setConfig({ testTimeout: 60000 }); @@ -17,7 +16,7 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); -const run = (options = '') => exec(`${cliPath} ai init ${options}`, { cwd: tmpDir }); +const run = (options = '') => cli(`ai init ${options}`, { cwd: tmpDir }); describe('pos-cli ai init', () => { test('--tool claude creates .mcp.json with both servers', async () => { @@ -48,12 +47,12 @@ describe('pos-cli ai init', () => { }); test('help lists the ai command and its init subcommand', async () => { - const aiHelp = await exec(`${cliPath} ai --help`); + const aiHelp = await cli('ai --help'); expect(aiHelp.stdout).toMatch('Usage: pos-cli ai'); expect(aiHelp.stdout).toMatch('init'); expect(aiHelp.stdout).toMatch('register platformOS MCP servers'); - const rootHelp = await exec(`${cliPath} --help`); + const rootHelp = await cli('--help'); expect(rootHelp.stdout).toMatch('configure AI tools'); }); }); diff --git a/test/integration/check.test.js b/test/integration/check.test.js index ca880ccf..3589b312 100644 --- a/test/integration/check.test.js +++ b/test/integration/check.test.js @@ -1,13 +1,12 @@ import { describe, test, expect, vi } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import { path as checkPaths } from '@platformos/platformos-check-node'; import path from 'path'; vi.setConfig({ testTimeout: 60000 }); const cwd = name => path.join(process.cwd(), 'test', 'fixtures', 'check', name); -const run = (fixtureName, options = '') => exec(`${cliPath} check run ${options}`, { cwd: cwd(fixtureName) }); +const run = (fixtureName, options = '') => cli(`check run ${options}`, { cwd: cwd(fixtureName) }); describe('pos-cli check run', () => { describe('Happy path', () => { @@ -147,7 +146,7 @@ describe('pos-cli check run', () => { // is. `app/` is the only marker here, so the message reports an inference, not a fact. const root = cwd('with-issues'); const specificPath = path.join(root, 'app/views/pages'); - const { stdout, stderr } = await exec(`${cliPath} check run ${specificPath}`); + const { stdout, stderr } = await cli(['check', 'run', specificPath]); // The refusal is platformos-check's own message, and it names a path the way it keys one: // through a URI, which lowercases a Windows drive letter. So `D:\…` comes back as `d:\…`, @@ -194,7 +193,7 @@ describe('pos-cli check run', () => { describe('Error handling', () => { test('Non-existent path', async () => { const nonExistentPath = path.join(process.cwd(), 'test', 'fixtures', 'check', 'nonexistent'); - const { stderr, code } = await exec(`${cliPath} check run ${nonExistentPath}`); + const { stderr, code } = await cli(['check', 'run', nonExistentPath]); expect(code).toEqual(1); expect(stderr).toMatch('Path does not exist'); @@ -202,7 +201,7 @@ describe('pos-cli check run', () => { test('File instead of directory', async () => { const filePath = path.join(cwd('clean'), 'app/views/pages/index.liquid'); - const { stderr, code } = await exec(`${cliPath} check run ${filePath}`); + const { stderr, code } = await cli(['check', 'run', filePath]); expect(code).toEqual(1); expect(stderr).toMatch('Path is not a directory'); @@ -219,7 +218,7 @@ describe('pos-cli check run', () => { fs.writeFileSync(path.join(tempDir, '.pos'), '{}'); try { - const { stdout, code } = await exec(`${cliPath} check init ${tempDir}`); + const { stdout, code } = await cli(['check', 'init', tempDir]); expect(code).toEqual(0); expect(stdout).toMatch('Created .platformos-check.yml'); @@ -250,7 +249,7 @@ describe('pos-cli check run', () => { fs.writeFileSync(path.join(tempDir, '.platformos-check.yml'), 'existing config'); try { - const { stdout, code } = await exec(`${cliPath} check init ${tempDir}`); + const { stdout, code } = await cli(['check', 'init', tempDir]); expect(code).toEqual(0); expect(stdout).toMatch('.platformos-check.yml already exists'); @@ -267,7 +266,7 @@ describe('pos-cli check run', () => { describe('Help text', () => { test('Check command help', async () => { - const { stdout } = await exec(`${cliPath} check --help`); + const { stdout } = await cli('check --help'); expect(stdout).toMatch('Usage: pos-cli check'); expect(stdout).toMatch('run [path]'); @@ -277,7 +276,7 @@ describe('pos-cli check run', () => { }); test('Check run command help', async () => { - const { stdout } = await exec(`${cliPath} check run --help`); + const { stdout } = await cli('check run --help'); expect(stdout).toMatch('Usage: pos-cli check run'); expect(stdout).toMatch('-a'); @@ -290,7 +289,7 @@ describe('pos-cli check run', () => { }); test('Check init command help', async () => { - const { stdout } = await exec(`${cliPath} check init --help`); + const { stdout } = await cli('check init --help'); expect(stdout).toMatch('Usage: pos-cli check init'); expect(stdout).toMatch('initialize .platformos-check.yml configuration file'); diff --git a/test/integration/deploy.test.js b/test/integration/deploy.test.js index fcc03c20..5e7083e9 100644 --- a/test/integration/deploy.test.js +++ b/test/integration/deploy.test.js @@ -1,7 +1,6 @@ import 'dotenv/config'; import { describe, test, expect, vi, beforeAll } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import unzip from 'unzipper'; import fs from 'fs'; import path from 'path'; @@ -23,7 +22,7 @@ beforeAll(() => { cleanupTmp(); }); -const run = (fixtureName, options, env = process.env) => exec(`${cliPath} deploy ${options || ''}`, { cwd: cwd(fixtureName), env }); +const run = (fixtureName, options, env = process.env) => cli(`deploy ${options || ''}`, { cwd: cwd(fixtureName), env }); const extract = async (inputPath, outputPath) => { return unzip.Open.file(inputPath).then(d => d.extract({ path: outputPath, concurrency: 5 })); diff --git a/test/integration/dns.test.js b/test/integration/dns.test.js index 4c14dfd1..29762310 100644 --- a/test/integration/dns.test.js +++ b/test/integration/dns.test.js @@ -13,8 +13,7 @@ import { describe, test, expect, vi } from 'vitest'; import fs from 'fs'; import os from 'os'; import path from 'path'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; vi.setConfig({ testTimeout: 120000 }); @@ -37,9 +36,11 @@ describe.skipIf(!hasSource)('pos-cli dns (integration)', () => { const exportFile = path.join(tmpDir, 'export.json'); test('export produces a valid envelope from the live portal', async () => { - const { code } = await exec( - `${cliPath} dns export --portal-url ${SOURCE.portalUrl} --token ${SOURCE.token} --instance-uuid ${SOURCE.uuid} -o ${exportFile}` - ); + const { code } = await cli([ + 'dns', 'export', + '--portal-url', SOURCE.portalUrl, '--token', SOURCE.token, '--instance-uuid', SOURCE.uuid, + '-o', exportFile + ]); expect(code).toEqual(0); const envelope = JSON.parse(fs.readFileSync(exportFile, 'utf8')); @@ -50,28 +51,28 @@ describe.skipIf(!hasSource)('pos-cli dns (integration)', () => { }); test('offline dry-run import renders a plan without touching any portal', async () => { - const { stdout, code } = await exec( - `${cliPath} dns import --file ${exportFile} --instance-uuid fake-target-uuid --dry-run` - ); + const { stdout, code } = await cli([ + 'dns', 'import', '--file', exportFile, '--instance-uuid', 'fake-target-uuid', '--dry-run' + ]); expect(code).toEqual(0); expect(stdout).toMatch(/domain\(s\) to apply|skipped/); }); test('offline self-compare is clean', async () => { - const { stdout, code } = await exec( - `${cliPath} dns compare --source-file ${exportFile} --target-file ${exportFile}` - ); + const { stdout, code } = await cli([ + 'dns', 'compare', '--source-file', exportFile, '--target-file', exportFile + ]); expect(code).toEqual(0); expect(stdout).toMatch(/Critical: 0/); }); test.skipIf(!hasTarget)('cross-portal compare runs against both live portals', async () => { - const { stdout } = await exec( - `${cliPath} dns compare ` + - `--source-portal-url ${SOURCE.portalUrl} --source-token ${SOURCE.token} --source-instance-uuid ${SOURCE.uuid} ` + - `--target-portal-url ${TARGET.portalUrl} --target-token ${TARGET.token} --target-instance-uuid ${TARGET.uuid} ` + + const { stdout } = await cli([ + 'dns', 'compare', + '--source-portal-url', SOURCE.portalUrl, '--source-token', SOURCE.token, '--source-instance-uuid', SOURCE.uuid, + '--target-portal-url', TARGET.portalUrl, '--target-token', TARGET.token, '--target-instance-uuid', TARGET.uuid, '--ignore-status' - ); + ]); expect(stdout).toMatch(/OK: \d+ {2}Advisory: \d+ {2}Critical: \d+/); }); }); diff --git a/test/integration/modules-install.test.js b/test/integration/modules-install.test.js index fa514acf..bfa8f674 100644 --- a/test/integration/modules-install.test.js +++ b/test/integration/modules-install.test.js @@ -1,7 +1,6 @@ import 'dotenv/config'; import { describe, test, expect } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import fs from 'fs'; import path from 'path'; import { requireRealCredentials, noCredentials, applyCredentials, saveCredentials } from '#test/utils/credentials'; @@ -9,7 +8,7 @@ import { plainMessages } from '#test/utils/parseOutput'; const cwd = name => path.join(process.cwd(), 'test', 'fixtures', name); const run = async (fixtureName, options = '') => - exec(`${cliPath} modules install ${options}`, { cwd: cwd(fixtureName), env: process.env }); + cli(`modules install ${options}`, { cwd: cwd(fixtureName), env: process.env }); describe('modules install', () => { test('downloads module with transitive dependencies, skipping what is already on disk', async () => { diff --git a/test/integration/modules-push.test.js b/test/integration/modules-push.test.js index b7b63cd4..84abbfd4 100644 --- a/test/integration/modules-push.test.js +++ b/test/integration/modules-push.test.js @@ -1,7 +1,6 @@ import 'dotenv/config'; import { describe, test, expect } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import path from 'path'; import { requireRealCredentials } from '#test/utils/credentials'; @@ -11,7 +10,7 @@ Object.assign(process.env, { const cwd = name => path.join(process.cwd(), 'test', 'fixtures', 'modules', name); -const run = (fixtureName, options) => exec(`${cliPath} modules push ${options}`, { cwd: cwd(fixtureName), env: process.env }); +const run = (fixtureName, options) => cli(`modules push ${options}`, { cwd: cwd(fixtureName), env: process.env }); describe('Server errors', () => { test('Empty directory', async () => { diff --git a/test/integration/modules-update.test.js b/test/integration/modules-update.test.js index f4bbd4a6..0e2d7bfc 100644 --- a/test/integration/modules-update.test.js +++ b/test/integration/modules-update.test.js @@ -1,7 +1,6 @@ import 'dotenv/config'; import { describe, test, expect } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import fs from 'fs'; import path from 'path'; import { requireRealCredentials } from '#test/utils/credentials'; @@ -9,7 +8,7 @@ import { plainMessages } from '#test/utils/parseOutput'; const cwd = name => path.join(process.cwd(), 'test', 'fixtures', name); const run = async (fixtureName, options = '') => - exec(`${cliPath} modules update ${options}`, { cwd: cwd(fixtureName), env: process.env }); + cli(`modules update ${options}`, { cwd: cwd(fixtureName), env: process.env }); describe('modules update', () => { test('updates module to latest and downloads it', async () => { diff --git a/test/integration/sync.test.js b/test/integration/sync.test.js index 1797c8f1..41d8aa2e 100644 --- a/test/integration/sync.test.js +++ b/test/integration/sync.test.js @@ -1,7 +1,6 @@ import 'dotenv/config'; import { describe, test, expect, afterAll, afterEach, vi } from 'vitest'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import waitForOutput from '#test/utils/waitForOutput'; import path from 'path'; import fs from 'fs'; @@ -11,8 +10,8 @@ vi.setConfig({ testTimeout: 30000 }); const cwd = name => path.join(process.cwd(), 'test', 'fixtures', 'deploy', name); const run = (fixtureName, options, callback) => { - return exec( - `${cliPath} sync ${options || ''}`, + return cli( + `sync ${options || ''}`, { cwd: cwd(fixtureName), env: process.env }, callback ); @@ -51,7 +50,7 @@ describe('Happy path', () => { const steps = async (child) => { await waitForOutput(child, /Synchronizing changes to/); - exec('echo "x" >> app/assets/bar.js', { cwd: cwd('correct_with_assets') }); + fs.appendFileSync(barJsPath, 'x\n'); await waitForOutput(child, /\[Sync\] Synced asset: app\/assets\/bar\.js/); kill(child); }; @@ -65,7 +64,7 @@ describe('Happy path', () => { test('sync with direct assets upload', { retry: 2 }, async () => { const steps = async (child) => { await waitForOutput(child, /Synchronizing changes to/); - exec('echo "x" >> app/assets/bar.js', { cwd: cwd('correct_with_assets') }); + fs.appendFileSync(barJsPath, 'x\n'); await waitForOutput(child, /\[Sync\] Synced asset: app\/assets\/bar\.js/); kill(child); }; @@ -119,8 +118,8 @@ properties: fs.writeFileSync(fullTestPath, testContent); try { - const { stdout, code } = await exec( - `${cliPath} sync -f ${testFilePath}`, + const { stdout, code } = await cli( + ['sync', '-f', testFilePath], { cwd: cwd('correct_with_assets'), env: process.env } ); @@ -143,8 +142,8 @@ properties: fs.writeFileSync(fullTestPath, testContent); try { - const { stdout, code } = await exec( - `${cliPath} sync -f ${testFilePath}`, + const { stdout, code } = await cli( + ['sync', '-f', testFilePath], { cwd: cwd('correct_with_assets'), env: process.env } ); @@ -162,8 +161,8 @@ properties: const testFilePath = 'app/schema/invalid-property-type.yml'; - const { stderr, code } = await exec( - `${cliPath} sync -f ${testFilePath}`, + const { stderr, code } = await cli( + ['sync', '-f', testFilePath], { cwd: cwd('invalid_schema'), env: process.env } ); diff --git a/test/integration/test-run.test.js b/test/integration/test-run.test.js index 55c8dd3d..38caf553 100644 --- a/test/integration/test-run.test.js +++ b/test/integration/test-run.test.js @@ -1,17 +1,16 @@ import 'dotenv/config'; import { describe, test, expect, vi } from 'vitest'; -import { spawn, exec as cpExec } from 'child_process'; -import path from 'path'; +import { spawn } from 'child_process'; +import cli from '#test/utils/exec'; +import cliScript from '#test/utils/cliPath'; import { requireRealCredentials } from '#test/utils/credentials'; import { TestLogStream } from '#lib/test-runner/logStream.js'; import { formatDuration, formatTestLog, transformTestResponse } from '#lib/test-runner/formatters.js'; vi.setConfig({ testTimeout: 30000 }); -const cliPath = path.join(process.cwd(), 'bin', 'pos-cli.js'); - const startCommand = (args, env = process.env) => { - const child = spawn('node', [cliPath, ...args], { + const child = spawn(process.execPath, [cliScript, ...args], { env: { ...process.env, ...env }, stdio: ['pipe', 'pipe', 'pipe'] }); @@ -38,18 +37,6 @@ const startCommand = (args, env = process.env) => { }; }; -const exec = (command, options = {}) => { - return new Promise((resolve) => { - // Use child_process.exec instead of spawn for better cross-platform compatibility - // This is the same approach used in test/utils/exec.js - cpExec(command, options, (err, stdout, stderr) => { - // err.code contains the exit code when process exits with error - const code = err ? (err.code ?? 1) : 0; - resolve({ stdout, stderr, code }); - }); - }); -}; - describe('pos-cli test-run command', () => { describe('Unit tests', () => { describe('formatDuration', () => { @@ -633,20 +620,20 @@ describe('pos-cli test-run command', () => { const CLI_TIMEOUT = 5000; test('requires environment argument', async () => { - const { code, stderr } = await exec(`node "${cliPath}" test run`, { env, timeout: CLI_TIMEOUT }); + const { code, stderr } = await cli(['test', 'run'], { env, timeout: CLI_TIMEOUT }); expect(code).toBe(1); expect(stderr).toMatch("error: missing required argument 'environment'"); }); test('accepts test name argument', async () => { - const { stderr } = await exec(`node "${cliPath}" test run staging my_test_name`, { env, timeout: CLI_TIMEOUT }); + const { stderr } = await cli(['test', 'run', 'staging', 'my_test_name'], { env, timeout: CLI_TIMEOUT }); expect(stderr).not.toMatch('error: missing required argument'); }); test('accepts test name with path', async () => { - const { stderr } = await exec(`node "${cliPath}" test run staging test/examples/assertions_test`, { env, timeout: CLI_TIMEOUT }); + const { stderr } = await cli(['test', 'run', 'staging', 'test/examples/assertions_test'], { env, timeout: CLI_TIMEOUT }); expect(stderr).not.toMatch('error: missing required argument'); }); @@ -660,7 +647,7 @@ describe('pos-cli test-run command', () => { MPKIT_EMAIL: 'test@example.com' }; - const { code, stderr } = await exec(`node "${cliPath}" test run staging`, { env: badEnv, timeout: CLI_TIMEOUT }); + const { code, stderr } = await cli(['test', 'run', 'staging'], { env: badEnv, timeout: CLI_TIMEOUT }); expect(code).toBe(1); expect(stderr).toMatch(/Could not connect|Request to( the)? server failed/); @@ -675,7 +662,7 @@ describe('pos-cli test-run command', () => { MPKIT_EMAIL: 'test@example.com' }; - const { code } = await exec(`node "${cliPath}" test run staging`, { env: badEnv, timeout: CLI_TIMEOUT }); + const { code } = await cli(['test', 'run', 'staging'], { env: badEnv, timeout: CLI_TIMEOUT }); expect(code).toBe(1); }); diff --git a/test/unit/audit.test.js b/test/unit/audit.test.js index ea02f7ec..feff1c7a 100644 --- a/test/unit/audit.test.js +++ b/test/unit/audit.test.js @@ -1,11 +1,10 @@ -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; import path from 'path'; import normalize from 'normalize-path'; const cwd = name => path.join(process.cwd(), 'test', 'fixtures', 'audit', name); -const run = fixtureName => exec(`${cliPath} audit`, { cwd: cwd(fixtureName) }); +const run = fixtureName => cli('audit', { cwd: cwd(fixtureName) }); test('Reports no errors with empty directory', async () => { const { stderr } = await run('empty'); diff --git a/test/unit/data.test.js b/test/unit/data.test.js index bf4c3dd0..5a3feecb 100644 --- a/test/unit/data.test.js +++ b/test/unit/data.test.js @@ -1,5 +1,4 @@ -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli, { feedStdin } from '#test/utils/exec'; const env = Object.assign(process.env, { CI: true, @@ -10,7 +9,7 @@ const env = Object.assign(process.env, { describe('Data clean', () => { test('shows message when wrong confirmation passed inline', async () => { - const {code, stderr} = await exec(`echo "wrong confirm" | ${cliPath} data clean`, { env }); + const {code, stderr} = await cli('data clean', { env }, feedStdin('wrong confirm\n')); expect(stderr).toMatch('Wrong confirmation. Closed without cleaning instance data.'); expect(code).toEqual(1); }); @@ -18,10 +17,7 @@ describe('Data clean', () => { describe('Data clean real', () => { test('shows message when wrong confirmation passed inline', async () => { - const {code, stderr, stdout} = await exec(`${cliPath} data clean`, { env }, (child) => { - child.stdin.write('CLEAN DATA\n'); - child.stdin.end(); - }); + const {code, stderr, stdout} = await cli('data clean', { env }, feedStdin('CLEAN DATA\n')); expect(stderr).toMatch('WARNING!!! You are going to REMOVE your data from instance: http://google.com') expect(stderr).toMatch('There is no coming back.') expect(stderr).toMatch('data_clean') @@ -31,7 +27,7 @@ describe('Data clean real', () => { describe('Data import', () => { test('should show message when wrong file for data import', async () => { - const {code, stderr} = await exec(`echo "wrong confirm" | ${cliPath} data import foo -p ./test/fixtures/wrong_json.json`, { env }); + const {code, stderr} = await cli('data import foo -p ./test/fixtures/wrong_json.json', { env }, feedStdin('wrong confirm\n')); expect(stderr).toMatch('Invalid format of ./test/fixtures/wrong_json.json. Must be a valid json file. Check file using one of JSON validators online: https://jsonlint.com'); expect(code).toEqual(1); }); diff --git a/test/unit/dns/transform.test.js b/test/unit/dns/transform.test.js index 6bd71dac..918d64e7 100644 --- a/test/unit/dns/transform.test.js +++ b/test/unit/dns/transform.test.js @@ -109,7 +109,9 @@ describe('transformDomain', () => { ); expect(result.payload.extra_dns_records).toHaveLength(1); - expect(result.warnings.some(w => w.includes('elb.amazonaws.com'))).toBe(true); + // Assert on the warning the transform owns, not on the record value it echoes back: + // the value is only there because the fixture put it there. + expect(result.warnings.some(w => w.includes('old-stack infrastructure'))).toBe(true); }); test('--drop-value patterns drop matching records with a reason', () => { diff --git a/test/unit/env-add.test.js b/test/unit/env-add.test.js index a58d3b89..56ecfe02 100644 --- a/test/unit/env-add.test.js +++ b/test/unit/env-add.test.js @@ -1,15 +1,16 @@ import process from 'process'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import fs from 'fs'; +import cli from '#test/utils/exec'; import { settingsFromDotPos } from '#lib/settings.js'; process.env['CI'] = 'true'; -const run = (options) => exec(`${cliPath} env add ${options}`); +const run = (options) => cli(`env add ${options}`); describe('commander env add', () => { - afterEach(() => exec('rm -f .pos')); + // fs, not `rm -f`: nothing here needs a shell, and the suite spawns no shell at all. + afterEach(() => fs.rmSync('.pos', { force: true })); test('adding with email and token', async () => { const { stdout } = await run('--url https://example.com --email pos-cli-ci@platformos.com --token 12345 e1'); diff --git a/test/unit/generators.test.js b/test/unit/generators.test.js index 98defcaf..377b015b 100644 --- a/test/unit/generators.test.js +++ b/test/unit/generators.test.js @@ -1,14 +1,13 @@ import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest'; -import { exec } from 'child_process'; +import { execFile } from 'child_process'; import path from 'path'; import fs from 'fs/promises'; import { fileURLToPath } from 'url'; +import cliScript from '#test/utils/cliPath'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); -const cliPath = path.join(process.cwd(), 'bin', 'pos-cli.js'); - // Every generator run spawns a real node process. In isolation each finishes in // well under a second, but the suite runs test files in parallel, so a run can // take several seconds under load. One generous shared ceiling keeps these tests @@ -19,11 +18,13 @@ const CLI_TIMEOUT = 20000; // execCommand gets a chance to report what the CLI actually printed. vi.setConfig({ testTimeout: CLI_TIMEOUT + 10000 }); -const execCommand = (cmd, opts = {}) => { +// execFile with an argv array, not exec with a command string: the generator paths below are +// absolute, and a shell would re-split them on any space in the checkout path. +const execCommand = (file, args, opts = {}) => { return new Promise((resolve) => { - // exec's own `timeout` option kills the child for us and still passes the + // execFile's own `timeout` option kills the child for us and still passes the // output collected so far to the callback, which a manual timer would lose. - const child = exec(cmd, opts, (err, stdout, stderr) => { + const child = execFile(file, args, opts, (err, stdout, stderr) => { // Extract exit code from error or default to 0 // Different environments might use err.code, err.exitCode, or err.signal let code = 0; @@ -44,18 +45,19 @@ const execCommand = (cmd, opts = {}) => { }); }; +const GENERATOR_FIXTURE = /^test\/fixtures\/yeoman\/(modules\/core|custom)\/generators\/\w+$/; + const run = (args, opts = {}) => { - // Convert relative generator paths to absolute paths - // __dirname is test/unit, so we need to go up two levels to reach project root - const absoluteArgs = args - .replace(/(test\/fixtures\/yeoman\/modules\/core\/generators\/\w+)(?=\s|$)/g, (match) => { - return path.resolve(__dirname, '../..', match); - }) - .replace(/(test\/fixtures\/yeoman\/custom\/generators\/\w+)(?=\s|$)/g, (match) => { - return path.resolve(__dirname, '../..', match); - }); - // Use process.execPath to ensure we use the same node executable - return execCommand(`"${process.execPath}" "${cliPath}" generate run ${absoluteArgs}`, { + // Split into argv first, then resolve each generator path on its own. Substituting before + // splitting would tear an absolute path apart at the first space inside it. + // __dirname is test/unit, so we go up two levels to reach the project root. + const argv = args + .split(/\s+/) + .filter(Boolean) + .map(token => (GENERATOR_FIXTURE.test(token) ? path.resolve(__dirname, '../..', token) : token)); + + // process.execPath so the child runs on the same node as the suite. + return execCommand(process.execPath, [cliScript, 'generate', 'run', ...argv], { ...opts, cwd: opts.cwd || process.cwd() }); diff --git a/test/unit/lib/commands.test.js b/test/unit/lib/commands.test.js index 39644734..a9877f54 100644 --- a/test/unit/lib/commands.test.js +++ b/test/unit/lib/commands.test.js @@ -1,5 +1,4 @@ -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; const getEnvs = () => { const env = Object.assign({}, process.env, { CI: true }); @@ -9,7 +8,7 @@ const getEnvs = () => { delete env.MPKIT_PASSWORD; return env; }; -const run = async args => exec(`${cliPath} ${args}`, { env: getEnvs() }); +const run = async args => cli(args, { env: getEnvs() }); test('should return error for missing command on stdout', async () => { let { stderr, code } = await run('missing'); diff --git a/test/unit/lsp.test.js b/test/unit/lsp.test.js index 9435668e..0c6e4fb5 100644 --- a/test/unit/lsp.test.js +++ b/test/unit/lsp.test.js @@ -1,14 +1,12 @@ import { describe, test, expect, vi } from 'vitest'; import { spawn } from 'child_process'; -import path from 'path'; -import exec from '#test/utils/exec'; -import cliPath from '#test/utils/cliPath'; +import cli from '#test/utils/exec'; +import cliScript from '#test/utils/cliPath'; vi.setConfig({ testTimeout: 15000 }); const spawnLsp = () => { - const binPath = path.join(process.cwd(), 'bin', 'pos-cli.js'); - const child = spawn('node', [binPath, 'lsp'], { + const child = spawn(process.execPath, [cliScript, 'lsp'], { stdio: ['pipe', 'pipe', 'pipe'] }); @@ -35,14 +33,14 @@ const spawnLsp = () => { describe('pos-cli lsp', () => { describe('Help text', () => { test('shows correct usage and description', async () => { - const { stdout } = await exec(`${cliPath} lsp --help`); + const { stdout } = await cli('lsp --help'); expect(stdout).toMatch('Usage: pos-cli lsp'); expect(stdout).toMatch('Language Server Protocol'); }); test('lsp is listed in main help', async () => { - const { stdout } = await exec(`${cliPath} --help`); + const { stdout } = await cli('--help'); expect(stdout).toMatch('lsp'); expect(stdout).toMatch('Language Server Protocol'); diff --git a/test/utils/cliPath.js b/test/utils/cliPath.js index b27d3c76..0119a3f2 100644 --- a/test/utils/cliPath.js +++ b/test/utils/cliPath.js @@ -4,6 +4,9 @@ import { fileURLToPath } from 'url'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); -const bin = `node ${path.join(__dirname, '../../bin', 'pos-cli.js')}`; +// The pos-cli entry script as a plain filesystem path. It is handed to execFile as an argv +// element (see ./exec.js), never interpolated into a command line, so it carries neither a +// `node ` prefix nor quoting of its own. +const cliScript = path.join(__dirname, '../../bin', 'pos-cli.js'); -export default bin; +export default cliScript; diff --git a/test/utils/commands.js b/test/utils/commands.js index cad8909a..810574d9 100644 --- a/test/utils/commands.js +++ b/test/utils/commands.js @@ -1,8 +1,7 @@ -import exec from './exec'; -import cliPath from './cliPath'; +import cli from './exec'; const cleanInstance = async (cwd) => { - const result = await exec(`${cliPath} data clean --auto-confirm --include-schema`, { cwd, env: process.env }); + const result = await cli('data clean --auto-confirm --include-schema', { cwd, env: process.env }); if (result.code !== 0) { throw new Error(`Failed to clean instance: ${result.stderr}`); } diff --git a/test/utils/credentials.js b/test/utils/credentials.js index 234f2578..113feecc 100644 --- a/test/utils/credentials.js +++ b/test/utils/credentials.js @@ -30,8 +30,22 @@ const applyCredentials = (creds) => { } }; +// Tells the placeholder URL above apart from a real instance. Compares the parsed hostname +// instead of searching the raw string: `includes('example.com')` also matches +// https://example.com.attacker.test and https://real-instance.io/?ref=example.com, either of +// which would silently skip every test that needs real credentials. +const isExampleUrl = (url) => { + if (!url) return false; + try { + const host = new URL(url).hostname.toLowerCase(); + return host === 'example.com' || host.endsWith('.example.com'); + } catch { + return false; + } +}; + const hasRealCredentials = () => { - return !!(process.env.MPKIT_URL && process.env.MPKIT_TOKEN && process.env.MPKIT_EMAIL && !process.env.MPKIT_URL.includes('example.com')); + return !!(process.env.MPKIT_URL && process.env.MPKIT_TOKEN && process.env.MPKIT_EMAIL && !isExampleUrl(process.env.MPKIT_URL)); }; const requireRealCredentials = () => { @@ -52,6 +66,6 @@ const restoreCredentials = (saved) => { }; export { - exampleCredentials, noCredentials, applyCredentials, + exampleCredentials, noCredentials, applyCredentials, isExampleUrl, hasRealCredentials, requireRealCredentials, saveCredentials, restoreCredentials }; diff --git a/test/utils/exec.js b/test/utils/exec.js index ee371c0e..4273aa48 100644 --- a/test/utils/exec.js +++ b/test/utils/exec.js @@ -1,12 +1,19 @@ -import { exec } from 'child_process'; +import { execFile } from 'child_process'; +import cliScript from './cliPath.js'; -const execCommand = (cmd, opts, callback) => { +// Every child process in the suite is spawned through execFile with an argv array, never +// through a shell. The repo's absolute path is part of every command here, so a shell would +// re-split it on spaces and re-read any metacharacter in it — the suite would break on a +// checkout under a path like "C:\My Projects\pos-cli". +const run = (file, args, opts = {}, callback) => { let stepError = null; return new Promise((resolve, reject) => { - const child = exec(cmd, opts, (err, stdout, stderr) => { + const child = execFile(file, args, opts, (err, stdout, stderr) => { if (stepError) return reject(stepError); - resolve({ stdout, stderr, code: err ? err.code : 0, child }); + // A child killed by opts.timeout reports code null; surface it as a failure rather than + // as "no exit code", which reads as success at the call sites that only check `code`. + resolve({ stdout, stderr, code: err ? (err.code ?? 1) : 0, child }); }); if (callback) { @@ -20,4 +27,29 @@ const execCommand = (cmd, opts, callback) => { }); }; -export default execCommand; +// `args` is the pos-cli argument list: an array, or — for the many call sites where every +// argument is a single token — a whitespace-separated string. Pass an array whenever an +// argument can contain a space, a filesystem path above all. +const toArgv = (args) => { + if (Array.isArray(args)) return args; + if (args === undefined || args === null) return []; + return String(args).split(/\s+/).filter(Boolean); +}; + +const cli = (args, opts, callback) => run(process.execPath, [cliScript, ...toArgv(args)], opts, callback); + +// Writes to a CLI that prompts, for the tests that used to pipe `echo` into it. The child can +// exit before it ever reads stdin (on a validation error, say), which turns the write into +// EPIPE — not a test failure, so it is swallowed. +const feedStdin = (text) => (child) => { + child.stdin.on('error', () => {}); + try { + child.stdin.write(text); + child.stdin.end(); + } catch { + // child already gone + } +}; + +export default cli; +export { cli, run, feedStdin };