Skip to content

Commit

Permalink
resources: replace execSync with spawnSync
Browse files Browse the repository at this point in the history
Extracted from #3700

CodeQL correctly reported that we using user supplied data in our scripts
that can lead to shell injection.
Running those scripts on untrusted PRs both locally and on CI can be problematic
Note I reviewed all places and none of them can be exploited but it good practice
to switch to spawnSync if we can.
Aditional benefit it automatically solves all the issues with command arguments
being misenterpritade by the shell.
  • Loading branch information
IvanGoncharov committed Aug 18, 2022
1 parent 9a494d9 commit 900f3ba
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 30 deletions.
12 changes: 6 additions & 6 deletions integrationTests/integration-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import * as path from 'node:path';

import { describe, it } from 'mocha';

function exec(command: string, options = {}) {
const output = childProcess.execSync(command, {
function npm(args: ReadonlyArray<string>, options = {}): string {
const result = childProcess.spawnSync('npm', ['--quite', ...args], {
encoding: 'utf-8',
...options,
});
return output?.trimEnd();
return result.stdout.toString().trimEnd();
}

describe('Integration Tests', () => {
Expand All @@ -19,7 +19,7 @@ describe('Integration Tests', () => {
fs.mkdirSync(tmpDir);

const distDir = path.resolve('./npmDist');
const archiveName = exec(`npm --quiet pack ${distDir}`, { cwd: tmpDir });
const archiveName = npm(['pack', distDir], { cwd: tmpDir });
fs.renameSync(
path.join(tmpDir, archiveName),
path.join(tmpDir, 'graphql.tgz'),
Expand All @@ -38,8 +38,8 @@ describe('Integration Tests', () => {

const cwd = path.join(tmpDir, projectName);
// TODO: figure out a way to run it with --ignore-scripts
exec('npm --quiet install', { cwd, stdio: 'inherit' });
exec('npm --quiet test', { cwd, stdio: 'inherit' });
npm(['install'], { cwd, stdio: 'inherit' });
npm(['test'], { cwd, stdio: 'inherit' });
}).timeout(120000);
}

Expand Down
16 changes: 8 additions & 8 deletions resources/benchmark.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as os from 'node:os';
import * as path from 'node:path';
import * as url from 'node:url';

import { exec, localRepoPath } from './utils';
import { git, localRepoPath, npm } from './utils';

const NS_PER_SEC = 1e9;
const LOCAL = 'local';
Expand Down Expand Up @@ -63,7 +63,7 @@ function prepareBenchmarkProjects(
path.join(projectPath, 'package.json'),
JSON.stringify(packageJSON, null, 2),
);
exec('npm --quiet install --ignore-scripts ', { cwd: projectPath });
npm(['--quite', 'install', '--ignore-scripts'], { cwd: projectPath });

return { revision, projectPath };
});
Expand All @@ -77,7 +77,7 @@ function prepareBenchmarkProjects(
}

// Returns the complete git hash for a given git revision reference.
const hash = exec(`git rev-parse "${revision}"`);
const hash = git(['rev-parse', revision]);

const archivePath = path.join(tmpDir, `graphql-${hash}.tgz`);
if (fs.existsSync(archivePath)) {
Expand All @@ -87,19 +87,19 @@ function prepareBenchmarkProjects(
const repoDir = path.join(tmpDir, hash);
fs.rmSync(repoDir, { recursive: true, force: true });
fs.mkdirSync(repoDir);
exec(`git clone --quiet "${localRepoPath()}" "${repoDir}"`);
exec(`git checkout --quiet --detach "${hash}"`, { cwd: repoDir });
exec('npm --quiet ci --ignore-scripts', { cwd: repoDir });
git(['clone', '--quiet', localRepoPath(), repoDir]);
git(['checkout', '--quiet', '--detach', hash], { cwd: repoDir });
npm(['--quiet', 'ci', '--ignore-scripts'], { cwd: repoDir });
fs.renameSync(buildNPMArchive(repoDir), archivePath);
fs.rmSync(repoDir, { recursive: true });
return archivePath;
}

function buildNPMArchive(repoDir: string) {
exec('npm --quiet run build:npm', { cwd: repoDir });
npm(['--quiet', 'run', 'build:npm'], { cwd: repoDir });

const distDir = path.join(repoDir, 'npmDist');
const archiveName = exec(`npm --quiet pack ${distDir}`, {
const archiveName = npm(['--quiet', 'pack', distDir], {
cwd: repoDir,
});
return path.join(repoDir, archiveName);
Expand Down
15 changes: 8 additions & 7 deletions resources/diff-npm-package.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import * as assert from 'node:assert';
import * as childProcess from 'node:child_process';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';

import { exec, localRepoPath, writeGeneratedFile } from './utils';
import { git, localRepoPath, npm, writeGeneratedFile } from './utils';

const LOCAL = 'local';
const tmpDir = path.join(os.tmpdir(), 'graphql-js-npm-diff');
Expand All @@ -27,7 +28,7 @@ console.log(`📦 Building NPM package for ${toRevision}...`);
const toPackage = prepareNPMPackage(toRevision);

console.log('➖➕ Generating diff...');
const diff = exec(`npm diff --diff=${fromPackage} --diff=${toPackage}`);
const diff = npm(['diff', '--diff', fromPackage, '--diff', toPackage]);

if (diff === '') {
console.log('No changes found!');
Expand Down Expand Up @@ -78,19 +79,19 @@ function generateReport(diffString: string): string {

function prepareNPMPackage(revision: string): string {
if (revision === LOCAL) {
exec('npm --quiet run build:npm', { cwd: localRepoPath() });
npm(['--quiet', 'run', 'build:npm'], { cwd: localRepoPath() });
return localRepoPath('npmDist');
}

// Returns the complete git hash for a given git revision reference.
const hash = exec(`git rev-parse "${revision}"`);
const hash = git(['rev-parse', revision]);
assert(hash != null);

const repoDir = path.join(tmpDir, hash);
fs.rmSync(repoDir, { recursive: true, force: true });
fs.mkdirSync(repoDir);
exec(`git archive "${hash}" | tar -xC "${repoDir}"`);
exec('npm --quiet ci --ignore-scripts', { cwd: repoDir });
exec('npm --quiet run build:npm', { cwd: repoDir });
childProcess.execSync(`git archive "${hash}" | tar -xC "${repoDir}"`);
npm(['--quiet', 'ci', '--ignore-scripts'], { cwd: repoDir });
npm(['--quiet', 'run', 'build:npm'], { cwd: repoDir });
return path.join(repoDir, 'npmDist');
}
10 changes: 5 additions & 5 deletions resources/gen-changelog.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { exec, readPackageJSON } from './utils';
import { git, readPackageJSON } from './utils';

const packageJSON = readPackageJSON();
const labelsConfig: { [label: string]: { section: string; fold?: boolean } } = {
Expand Down Expand Up @@ -64,15 +64,15 @@ function getChangeLog(): Promise<string> {
const { version } = packageJSON;

let tag: string | null = null;
let commitsList = exec(`git rev-list --reverse v${version}..`);
let commitsList = git(['rev-list', '--reverse', `v${version}..`]);
if (commitsList === '') {
const parentPackageJSON = exec('git cat-file blob HEAD~1:package.json');
const parentPackageJSON = git(['cat-file', 'blob', 'HEAD~1:package.json']);
const parentVersion = JSON.parse(parentPackageJSON).version;
commitsList = exec(`git rev-list --reverse v${parentVersion}..HEAD~1`);
commitsList = git(['rev-list', '--reverse', `v${parentVersion}..HEAD~1`]);
tag = `v${version}`;
}

const date = exec('git log -1 --format=%cd --date=short');
const date = git(['log', '-1', '--format=%cd', '--date=short']);
return getCommitsInfo(commitsList.split('\n'))
.then((commitsInfo) => getPRsInfo(commitsInfoToPRs(commitsInfo)))
.then((prsInfo) => genChangeLog(tag, date, prsInfo));
Expand Down
29 changes: 25 additions & 4 deletions resources/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,36 @@ export function localRepoPath(...paths: ReadonlyArray<string>): string {
return path.join(__dirname, '..', ...paths);
}

type ExecOptions = Parameters<typeof childProcess.execSync>[1];
export function exec(command: string, options?: ExecOptions): string {
const output = childProcess.execSync(command, {
export function npm(
args: ReadonlyArray<string>,
options?: SpawnOptions,
): string {
return spawn('npm', args, options);
}

export function git(
args: ReadonlyArray<string>,
options?: SpawnOptions,
): string {
return spawn('git', args, options);
}

interface SpawnOptions {
cwd?: string;
}

function spawn(
command: string,
args: ReadonlyArray<string>,
options?: SpawnOptions,
): string {
const result = childProcess.spawnSync(command, args, {
maxBuffer: 10 * 1024 * 1024, // 10MB
stdio: ['inherit', 'pipe', 'inherit'],
encoding: 'utf-8',
...options,
});
return output.toString().trimEnd();
return result.stdout.toString().trimEnd();
}

export function readdirRecursive(
Expand Down

0 comments on commit 900f3ba

Please sign in to comment.