Skip to content

Commit aed1fc6

Browse files
data-douserdata-douser
committed
WIP fixes for CDS extractor rewrite
First attempt at fixing `Indirect uncontrolled command line` code scanning alerts for the `index-fils.js` script. Improves error handling and improves the reliability and security of code that creates child (exec/spawn) processes. Attempts to improve the passing of env vars to child processes, especially for the `LGTM_INDEX_FILTERS` env var. WIP because CDS extractor invocation is still failing to identify .cds.json files. Possible problem in the way env vars are passed within the javascript extractor autobuilder shell script (to the JVM launched by the javascript extractor autobuilder).
1 parent ea229d1 commit aed1fc6

File tree

7 files changed

+151
-46
lines changed

7 files changed

+151
-46
lines changed

‎extractors/cds/tools/autobuild.cmd‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
@echo off
22

3-
type NUL && "%CODEQL_DIST%\codeql" database index-files ^
3+
type NUL && "%CODEQL_DIST%\codeql.exe" database index-files ^
44
--include-extension=.cds ^
55
--language cds ^
66
--prune **\node_modules\**\* ^
@@ -9,4 +9,4 @@ type NUL && "%CODEQL_DIST%\codeql" database index-files ^
99
-- ^
1010
"%CODEQL_EXTRACTOR_CDS_WIP_DATABASE%"
1111

12-
exit /b %ERRORLEVEL%
12+
exit /b %ERRORLEVEL%

‎extractors/cds/tools/autobuild.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,4 @@ exec "${CODEQL_DIST}/codeql" database index-files \
1515
--prune **/.eslint/**/* \
1616
--total-size-limit=10m \
1717
-- \
18-
"$CODEQL_EXTRACTOR_CDS_WIP_DATABASE"
18+
"$CODEQL_EXTRACTOR_CDS_WIP_DATABASE"

‎extractors/cds/tools/index-files.js‎

Lines changed: 129 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
const { execSync, spawnSync } = require('child_process');
1+
const { execFileSync, execSync, spawnSync } = require('child_process');
22
const { existsSync, readFileSync, statSync } = require('fs');
33
const { arch, platform } = require('os');
44
const { dirname, join, resolve } = require('path');
5+
const { quote } = require('shell-quote');
56

67
console.log('Indexing CDS files');
78

@@ -11,32 +12,55 @@ const osPlatform = platform();
1112
const osPlatformArch = arch();
1213
console.log(`Detected OS platform=${osPlatform} : arch=${osPlatformArch}`);
1314
const autobuildScriptName = osPlatform === 'win32' ? 'autobuild.cmd' : 'autobuild.sh';
15+
const autobuildScriptPath = join(
16+
process.env.CODEQL_EXTRACTOR_JAVASCRIPT_ROOT, 'tools', autobuildScriptName
17+
);
1418
const codeqlExe = osPlatform === 'win32' ? 'codeql.exe' : 'codeql';
15-
const codeqlExePath = join(process.env.CODEQL_DIST, codeqlExe);
16-
const npmInstallCmdWithArgs = 'npm install --quiet --no-audit --no-fund --no-package-lock';
19+
const codeqlExePath = join(quote([process.env.CODEQL_DIST]), codeqlExe);
1720

18-
// If the response file does not exist, terminate.
21+
/**
22+
* Terminate early if:
23+
* - the javascript extractor autobuild script does not exist; or
24+
* - the codeql executable does not exist; or
25+
* - the input responseFile does not exist; or
26+
* - the input responseFile is empty or could not be parsed as a list of file paths.
27+
*/
28+
if (!existsSync(autobuildScriptPath)) {
29+
console.warn(`'${codeqlExe} database index-files --language cds' terminated early as autobuild script '${autobuildScriptPath}' does not exist.`);
30+
process.exit(0);
31+
}
32+
if (!existsSync(codeqlExePath)) {
33+
console.warn(`'${codeqlExe} database index-files --language cds' terminated early as codeql executable '${codeqlExePath}' does not exist.`);
34+
process.exit(0);
35+
}
1936
if (!existsSync(responseFile)) {
20-
console.log(`'codeql database index-files --language cds' terminated early as response file '${responseFile}' does not exist. This is because no CDS files were selected or found.`);
37+
console.warn(`'${codeqlExe} database index-files --language cds' terminated early as response file '${responseFile}' does not exist. This is because no CDS files were selected or found.`);
2138
process.exit(0);
2239
}
2340

24-
// Read the response file and split it into lines, removing (filter(Boolean)) empty lines.
25-
const responseFiles = readFileSync(responseFile, 'utf-8').split('\n').filter(Boolean);
26-
// If the response file is empty, terminate.
27-
if (statSync(responseFile).size === 0 || !responseFiles) {
28-
console.log(`'codeql database index-files --language cds' terminated early as response file '${responseFile}' is empty. This is because no CDS files were selected or found.`);
41+
let responseFiles = [];
42+
try {
43+
// Read the response file and split it into lines, removing (filter(Boolean)) empty lines.
44+
responseFiles = readFileSync(responseFile, 'utf-8').split('\n').filter(Boolean);
45+
if (statSync(responseFile).size === 0 || responseFiles.length === 0) {
46+
console.warn(`'${codeqlExe} database index-files --language cds' terminated early as response file '${responseFile}' is empty. This is because no CDS files were selected or found.`);
47+
process.exit(0);
48+
}
49+
} catch (err) {
50+
console.warn(`'${codeqlExe} database index-files --language cds' terminated early as response file '${responseFile}' could not be read due to an error: ${err}`);
2951
process.exit(0);
3052
}
3153

3254
// Determine if we have the cds commands available. If not, install the cds develpment kit
3355
// (cds-dk) in the appropriate directories and use npx to run the cds command from there.
3456
let cdsCommand = 'cds';
3557
try {
36-
execSync('cds --version', { stdio: 'ignore' });
58+
execFileSync('cds', ['--version'], { stdio: 'ignore' });
3759
} catch {
3860
console.log('Pre-installing cds compiler');
3961

62+
// Use a JS `Set` to avoid duplicate processing of the same directory.
63+
const packageJsonDirs = new Set();
4064
/**
4165
* Find all the directories containing a package.json with a dependency on `@sap/cds`,
4266
* where the directory contains at least one of the files listed in the response file
@@ -52,22 +76,51 @@ try {
5276
*
5377
* We also ensure we skip node_modules, as we can end up in a recursive loop.
5478
*/
55-
const packageJsonDirs = new Set();
5679
responseFiles.forEach(file => {
57-
let dir = dirname(file);
80+
let dir = dirname(quote([file]));
5881
while (dir !== resolve(dir, '..')) {
59-
if (existsSync(join(dir, 'package.json')) && readFileSync(join(dir, 'package.json'), 'utf-8').includes('@sap/cds')) {
60-
packageJsonDirs.add(dir);
61-
break;
82+
const packageJsonPath = join(dir, 'package.json');
83+
if (existsSync(packageJsonPath)) {
84+
const rawData = readFileSync(packageJsonPath, 'utf-8');
85+
const packageJsonData = JSON.parse(rawData);
86+
// Check if the 'name' and 'dependencies' properties are present in the
87+
// package.json file at packageJsonPath.
88+
if (
89+
packageJsonData.name &&
90+
packageJsonData.dependencies &&
91+
typeof packageJsonData.dependencies === 'object'
92+
) {
93+
const dependencyNames = Object.keys(packageJsonData.dependencies);
94+
if (
95+
dependencyNames.includes('@sap/cds')
96+
&&
97+
dependencyNames.includes('@sap/cds-dk')
98+
) {
99+
packageJsonDirs.add(dir);
100+
break;
101+
}
102+
}
62103
}
104+
// Move up one directory level and try again to find a package.json file
105+
// for the response file.
63106
dir = resolve(dir, '..');
64107
}
65108
});
66109

67-
packageJsonDirs.forEach(dir => {
68-
console.log(`Installing @sap/cds-dk into ${dir} to enable CDS compilation.`);
69-
execSync(`${npmInstallCmdWithArgs} @sap/cds-dk`, { cwd: dir });
70-
execSync(npmInstallCmdWithArgs, { cwd: dir });
110+
// TODO : revise this check as the equality is probably not guaranteed.
111+
if (responseFiles.length !== packageJsonDirs.length) {
112+
console.warn(
113+
`WARN: mismatch between number of response files (${responseFiles.length}) and package.json directories (${packageJsonDirs.length})`
114+
);
115+
}
116+
117+
packageJsonDirs.forEach((dir) => {
118+
console.log(`Installing node packages into ${dir} to enable CDS compilation.`);
119+
execFileSync(
120+
'npm',
121+
['install', '--quiet', '--no-audit', '--no-fund'],
122+
{ cwd: dir, stdio: 'inherit' }
123+
);
71124
});
72125

73126
/**
@@ -84,22 +137,46 @@ console.log('Processing CDS files to JSON');
84137
* Run the cds compile command on each file in the response files list, outputting the
85138
* compiled JSON to a file with the same name but with a .json extension appended.
86139
*/
87-
responseFiles.forEach(cdsFile => {
88-
const cdsJsonFile = `${cdsFile}.json`;
89-
console.log(`Processing CDS file ${cdsFile} to: ${cdsJsonFile}`);
90-
const result = spawnSync(cdsCommand, ['compile', cdsFile, '-2', 'json', '-o', cdsJsonFile, '--locations'], { shell: true });
140+
responseFiles.forEach(rawCdsFilePath => {
141+
const cdsFilePath = quote([rawCdsFilePath]);
142+
const cdsJsonFilePath = `${cdsFilePath}.json`;
143+
console.log(`Processing CDS file ${cdsFilePath} to: ${cdsJsonFilePath}`);
144+
const result = spawnSync(
145+
cdsCommand,
146+
['compile', cdsFilePath, '-2', 'json', '-o', cdsJsonFilePath, '--locations'],
147+
{ shell: true }
148+
);
91149
if (result.error || result.status !== 0) {
92-
const stderrTruncated = result.stderr.toString().split('\n').filter(line => line.startsWith('[ERROR]')).slice(-4).join('\n');
93-
const errorMessage = `Could not compile the file ${cdsFile}.\nReported error(s):\n\`\`\`\n${stderrTruncated}\n\`\`\``;
150+
const stderrTruncated = quote(
151+
result.stderr.toString().split('\n').filter(line => line.startsWith('[ERROR]')).slice(-4).join('\n'));
152+
const errorMessage = `Could not compile the file ${cdsFilePath}.\nReported error(s):\n\`\`\`\n${stderrTruncated}\n\`\`\``;
94153
console.log(errorMessage);
95-
execSync(`${codeqlExePath} database add-diagnostic --extractor-name cds --ready-for-status-page --source-id=cds/compilation-failure --source-name="Failure to compile one or more SAP CAP CDS files" --severity=error --markdown-message="${errorMessage}" --file-path="${cdsFile}" -- "${process.env.CODEQL_EXTRACTOR_CDS_WIP_DATABASE}"`);
154+
execFileSync(
155+
codeqlExePath,
156+
[
157+
'database',
158+
'add-diagnostic',
159+
'--extractor-name=cds',
160+
'--ready-for-status-page',
161+
'--source-id=cds/compilation-failure',
162+
'--source-name="Failure to compile one or more SAP CAP CDS files"',
163+
'--severity=error',
164+
`--markdown-message="${errorMessage}"`,
165+
`--file-path="${cdsFilePath}"`,
166+
'--',
167+
`${process.env.CODEQL_EXTRACTOR_CDS_WIP_DATABASE}`
168+
],
169+
);
96170
}
97171
});
98172

99173
// Check if the (JavaScript) JS extractor variables are set, and set them if not.
100174
if (!process.env.CODEQL_EXTRACTOR_JAVASCRIPT_ROOT) {
101175
// Find the JS extractor location.
102-
process.env.CODEQL_EXTRACTOR_JAVASCRIPT_ROOT = execSync(`${codeqlExePath} resolve extractor --language=javascript`).toString().trim();
176+
process.env.CODEQL_EXTRACTOR_JAVASCRIPT_ROOT = execFileSync(
177+
codeqlExePath,
178+
['resolve', 'extractor', '--language=javascript']
179+
).toString().trim();
103180
// Set the JAVASCRIPT extractor environment variables to the same as the CDS
104181
// extractor environment variables so that the JS extractor will write to the
105182
// CDS database.
@@ -134,25 +211,39 @@ if (process.env.LGTM_INDEX_FILTERS) {
134211
.filter(line =>
135212
line.startsWith('exclude')
136213
&&
137-
!allowedExcludePatterns.includes(line)
214+
!allowedExcludePatterns.some(pattern => line.includes(pattern))
138215
).join('\n');
139216
}
140217

141218
// Enable extraction of the .cds.json files only.
142-
const lgtmIndexFiltersPatterns = join(
143-
'exclude:**', '*.*\ninclude:**', '*.cds.json\ninclude:**', '*.cds\nexclude:**', 'node_modules', '**', '*.*'
144-
);
145-
process.env.LGTM_INDEX_FILTERS = `${lgtmIndexFiltersPatterns}${excludeFilters}`;
146-
console.log(`Setting $LGTM_INDEX_FILTERS to:\n${process.env.LGTM_INDEX_FILTERS}`);
219+
const lgtmIndexFiltersPatterns = [
220+
join('exclude:**', '*.*'),
221+
join('include:**', '*.cds.json'),
222+
join('include:**', '*.cds'),
223+
join('exclude:**', 'node_modules', '**', '*.*')
224+
].join('\n');;
225+
process.env.LGTM_INDEX_FILTERS = lgtmIndexFiltersPatterns + excludeFilters;
226+
console.log(`Set $LGTM_INDEX_FILTERS to:\n${process.env.LGTM_INDEX_FILTERS}`);
147227
process.env.LGTM_INDEX_TYPESCRIPT = 'NONE';
148228
// Configure to copy over the .cds files as well, by pretending they are JSON.
149229
process.env.LGTM_INDEX_FILETYPES = '.cds:JSON';
150230
// Ignore the LGTM_INDEX_INCLUDE variable for this purpose as it may explicitly
151231
// refer to .js or .ts files.
152232
delete process.env.LGTM_INDEX_INCLUDE;
153233

154-
console.log('Extracting the cds.json files');
234+
console.log('Extracting the .cds.json files');
155235

156-
// Invoke the JS autobuilder to index the .cds.json files only.
157-
const autobuildScriptPath = join(process.env.CODEQL_EXTRACTOR_JAVASCRIPT_ROOT, 'tools', autobuildScriptName);
158-
execSync(autobuildScriptPath, { stdio: 'inherit' });
236+
console.log(`Running 'javascript' extractor autobuild script: ${autobuildScriptPath}`);
237+
/**
238+
* Invoke the javascript autobuilder to index the .cds.json files only.
239+
*
240+
* Environment variables must be passed from this script's process to the
241+
* process that invokes the autobuild script, otherwise the CDS autobuild.sh
242+
* script will not be invoked by the autobuild script built into the
243+
* 'javascript' extractor.
244+
*/
245+
spawnSync(
246+
autobuildScriptPath,
247+
[],
248+
{ env: process.env, shell: true, stdio: 'inherit' }
249+
);

‎extractors/cds/tools/index-files.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,6 @@ fi
4141
# dependencies (i.e. node_modules) relative to this directory.
4242
cd "$_script_dir" && \
4343
echo "Installing node package dependencies" && \
44-
npm install --quiet --no-audit --no-fund --no-package-json && \
44+
npm install --quiet --no-audit --no-fund && \
4545
echo "Running the 'index-files.js' script" && \
46-
node "$(dirname "$0")/index-files.js" "$_response_file_path"
46+
node "$(dirname "$0")/index-files.js" "$_response_file_path"

‎extractors/cds/tools/package-lock.json‎

Lines changed: 14 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎extractors/cds/tools/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"child_process": "^1.0.2",
99
"fs": "^0.0.1-security",
1010
"os": "^0.1.2",
11-
"path": "^0.12.7"
11+
"path": "^0.12.7",
12+
"shell-quote": "^1.8.2"
1213
}
1314
}

‎extractors/javascript/tools/pre-finalize.cmd‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
@echo off
22

33
if not defined CODEQL_EXTRACTOR_CDS_SKIP_EXTRACTION (
4-
type NUL && "%CODEQL_DIST%\codeql" database index-files ^
4+
type NUL && "%CODEQL_DIST%\codeql.exe" database index-files ^
55
--include-extension=.cds ^
66
--language cds ^
77
--prune **\node_modules\**\* ^

0 commit comments

Comments
 (0)