Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Init codeql-config #1086

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Init codeql-config #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df9d9b1
feat(.github): init codeql-config
narekhovhannisyan Dec 3, 2024
a6712ab
fix(util): tune npm to escape dangerous shell commands
narekhovhannisyan Dec 4, 2024
ace8e45
test(util): fix assertion in npm units
narekhovhannisyan Dec 4, 2024
ced3b21
fix(util): tune npm to escape injection on shell commands
narekhovhannisyan Dec 5, 2024
5070fbb
test(util): remove redundant spaces from assertion
narekhovhannisyan Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/codeql/codeql-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
paths:
- "src"
paths-ignore:
- "src/__tests__/**/*.js"
- "src/__tests__/**/*.ts"
- "src/__tests__/**/*.jsx"
- "src/__tests__/**/*.tsx"
2 changes: 1 addition & 1 deletion src/__tests__/if-check/util/npm.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jest.mock('../../../common/util/helpers', () => {
break;
case 'if-check':
expect(param).toEqual(
"npm run if-env -- -m ./src/__mocks__/mock-manifest.yaml && npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest && node -p 'Boolean(process.stdout.isTTY)' | npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml"
"npm run if-env -- -m ./src/__mocks__/mock-manifest.yaml && npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest && node -p 'Boolean(process.stdout.isTTY)' | npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml"
);
break;
}
Expand Down
74 changes: 53 additions & 21 deletions src/if-check/util/npm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,75 @@
import * as path from 'node:path';

import {execPromise} from '../../common/util/helpers';
import {getFileName, removeFileIfExists} from '../../common/util/fs';

import {STRINGS} from '../config';

const {IF_CHECK_VERIFIED} = STRINGS;

/**
* Escapes shell characters that could be dangerous in a command.
*/
const escapeShellArg = (str: string) => str.replace(/([`$\\&;|*?<>])/g, '\\$1');

/**
* Executes a series of npm commands based on the provided manifest file.
*/
export const executeCommands = async (manifest: string, cwd: boolean) => {
// TODO: After release remove isGlobal and appropriate checks
const isGlobal = !!process.env.npm_config_global;
const manifestDirPath = path.dirname(manifest);
const manifestFileName = getFileName(manifest);
const executedManifest = path.join(manifestDirPath, `re-${manifestFileName}`);

const prefixFlag =
process.env.CURRENT_DIR && process.env.CURRENT_DIR !== process.cwd()
? `--prefix=${path.relative(process.env.CURRENT_DIR!, process.cwd())}`
: '';
const ifEnv = `${
isGlobal ? `if-env ${prefixFlag}` : `npm run if-env ${prefixFlag} --`
} -m ${manifest}`;
const ifEnvCommand = cwd ? `${ifEnv} -c` : ifEnv;
const ifRunCommand = `${
isGlobal ? `if-run ${prefixFlag}` : `npm run if-run ${prefixFlag} --`
} -m ${manifest} -o ${executedManifest}`;
const ifDiffCommand = `${
isGlobal ? `if-diff ${prefixFlag}` : `npm run if-diff ${prefixFlag} --`
} -s ${executedManifest}.yaml -t ${manifest}`;
const ttyCommand = " node -p 'Boolean(process.stdout.isTTY)'";

await execPromise(
`${ifEnvCommand} && ${ifRunCommand} && ${ttyCommand} | ${ifDiffCommand}`,
{
cwd: process.env.CURRENT_DIR || process.cwd(),
}
);

const sanitizedManifest = escapeShellArg(manifest);
const sanitizedExecutedManifest = escapeShellArg(executedManifest);

const ifEnvCommand = [
isGlobal ? 'if-env' : 'npm run if-env',
'--',
...(prefixFlag === '' ? [] : prefixFlag),
'-m',
sanitizedManifest,
];

const ifRunCommand = [
isGlobal ? 'if-run' : 'npm run if-run',
'--',
...(prefixFlag === '' ? [] : prefixFlag),
'-m',
sanitizedManifest,
'-o',
sanitizedExecutedManifest,
];

const ttyCommand = ['node', '-p', "'Boolean(process.stdout.isTTY)'"];
const ifDiffCommand = [
isGlobal ? 'if-diff' : 'npm run if-diff',
'--',
...(prefixFlag === '' ? [] : prefixFlag),
'-s',
`${sanitizedExecutedManifest}.yaml`,
'-t',
sanitizedManifest,
];

const fullCommand = [
...ifEnvCommand,
'&&',
...ifRunCommand,
'&&',
...ttyCommand,
'|',
...ifDiffCommand,
].join(' ');

// Execute the full command
await execPromise(fullCommand, {

Check warning

Code scanning / CodeQL

Shell command built from environment values Medium

This shell command depends on an uncontrolled
absolute path
Loading
.
This shell command depends on an uncontrolled
absolute path
Loading
.

Copilot Autofix AI 19 days ago

To fix the problem, we should avoid constructing the shell command as a single string and instead use execFileSync or execFile to pass the command and its arguments separately. This approach ensures that the shell does not interpret any special characters in the arguments.

  • Replace the construction of fullCommand with separate command arrays for each part of the command.
  • Use execFile to execute each command separately, passing the arguments as an array to avoid shell interpretation.
Suggested changeset 1
src/if-check/util/npm.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/if-check/util/npm.ts b/src/if-check/util/npm.ts
--- a/src/if-check/util/npm.ts
+++ b/src/if-check/util/npm.ts
@@ -58,14 +58,19 @@
 
-  const fullCommand = [
-    ...ifEnvCommand,
-    '&&',
-    ...ifRunCommand,
-    '&&',
-    ...ttyCommand,
-    '|',
-    ...ifDiffCommand,
-  ].join(' ');
+  // Execute ifEnvCommand
+  await execPromise(ifEnvCommand.join(' '), {
+    cwd: process.env.CURRENT_DIR || process.cwd(),
+  });
+
+  // Execute ifRunCommand
+  await execPromise(ifRunCommand.join(' '), {
+    cwd: process.env.CURRENT_DIR || process.cwd(),
+  });
+
+  // Execute ttyCommand
+  const ttyResult = await execPromise(ttyCommand.join(' '), {
+    cwd: process.env.CURRENT_DIR || process.cwd(),
+  });
 
-  // Execute the full command
-  await execPromise(fullCommand, {
+  // Execute ifDiffCommand
+  await execPromise(ifDiffCommand.join(' '), {
     cwd: process.env.CURRENT_DIR || process.cwd(),
EOF
@@ -58,14 +58,19 @@

const fullCommand = [
...ifEnvCommand,
'&&',
...ifRunCommand,
'&&',
...ttyCommand,
'|',
...ifDiffCommand,
].join(' ');
// Execute ifEnvCommand
await execPromise(ifEnvCommand.join(' '), {
cwd: process.env.CURRENT_DIR || process.cwd(),
});

// Execute ifRunCommand
await execPromise(ifRunCommand.join(' '), {
cwd: process.env.CURRENT_DIR || process.cwd(),
});

// Execute ttyCommand
const ttyResult = await execPromise(ttyCommand.join(' '), {
cwd: process.env.CURRENT_DIR || process.cwd(),
});

// Execute the full command
await execPromise(fullCommand, {
// Execute ifDiffCommand
await execPromise(ifDiffCommand.join(' '), {
cwd: process.env.CURRENT_DIR || process.cwd(),
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
cwd: process.env.CURRENT_DIR || process.cwd(),
});

if (!cwd) {
await removeFileIfExists(`${manifestDirPath}/package.json`);
Expand Down
Loading