Skip to content

Commit 0ba67d5

Browse files
committed
Fix code-scanning alerts for insecure tmp files
Fixes newly introduced code-scanning alerts due to insecure use of files created under the system `/tmp/` directory in some recently implemented unit tests.
1 parent af066d7 commit 0ba67d5

File tree

7 files changed

+96
-62
lines changed

7 files changed

+96
-62
lines changed

extractors/cds/tools/cds-extractor.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ if (typedProjectMap.__debugParserSuccess) {
8080
}
8181

8282
// Install dependencies of discovered CAP/CDS projects
83-
console.log('Ensuring depencencies are installed in cache for required CDS compiler versions...');
83+
console.log('Ensuring dependencies are installed in cache for required CDS compiler versions...');
8484
const projectCacheDirMap = installDependencies(projectMap, sourceRoot, codeqlExePath);
8585

8686
const cdsFilePathsToProcess: string[] = [];

extractors/cds/tools/eslint.config.mjs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ export default defineConfig([
4040
// @ts-expect-error - typescript-eslint is a valid plugin
4141
'@typescript-eslint': fixupPluginRules(typescriptEslint),
4242
// @ts-expect-error - import is a valid plugin
43-
'import': fixupPluginRules(_import),
43+
import: fixupPluginRules(_import),
4444
// @ts-expect-error - prettier is a valid plugin
4545
prettier: fixupPluginRules(prettier),
4646
},
@@ -55,7 +55,7 @@ export default defineConfig([
5555
sourceType: 'module',
5656
parserOptions: {
5757
project: './tsconfig.json',
58-
tsconfigRootDir: '/Users/data-douser/Git/data-douser/codeql-sap-js/extractors/cds/tools',
58+
tsconfigRootDir: __dirname,
5959
createDefaultProgram: true,
6060
},
6161
},

extractors/cds/tools/package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@
1616
"test:coverage": "jest --coverage --collectCoverageFrom='src/**/*.ts'"
1717
},
1818
"dependencies": {
19+
"@types/tmp": "^0.2.6",
1920
"child_process": "^1.0.2",
2021
"fs": "^0.0.1-security",
2122
"glob": "^11.0.2",
2223
"os": "^0.1.2",
2324
"path": "^0.12.7",
24-
"shell-quote": "^1.8.2"
25+
"shell-quote": "^1.8.2",
26+
"tmp": "^0.2.3"
2527
},
2628
"devDependencies": {
2729
"@eslint/compat": "^1.2.9",

extractors/cds/tools/src/cds/compiler/compile.ts

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -173,19 +173,6 @@ function compileIndividualFile(
173173
'warn',
174174
];
175175

176-
/**
177-
* using the `--parse` flag changes how the data is shown in the generated .cds.json file,
178-
* where annotations and other metadata will show up under the `extensions` property instead
179-
* of the `definitions` property, as used in previous versions of the CDS extractor.
180-
*
181-
* DELETE this comment if the `--parse` flag is definitely not needed in the future.
182-
*
183-
* // For root CDS files in project-aware mode, add --parse flag to ensure complete model
184-
*if (projectBasedCompilation) {
185-
* compileArgs.push('--parse');
186-
*}
187-
*/
188-
189176
// CRITICAL: Use the provided spawnOptions which has sourceRoot as cwd
190177
const result = spawnSync(cdsCommand, compileArgs, spawnOptions);
191178

extractors/cds/tools/src/cds/compiler/project.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { relative } from 'path';
2+
13
/**
24
* Helper functions for mapping CDS files to their projects and cache directories
35
*/
@@ -15,15 +17,20 @@ export function findProjectForCdsFile(
1517
projectMap: Map<string, { cdsFiles: string[] }>,
1618
): string | undefined {
1719
// Get the relative path to the project directory for this CDS file
18-
const relativeCdsFilePath = cdsFilePath.startsWith(sourceRoot)
19-
? cdsFilePath.substring(sourceRoot.length + 1)
20-
: cdsFilePath;
20+
const relativeCdsFilePath = relative(sourceRoot, cdsFilePath);
21+
22+
// If the file is outside the source root, path.relative() will start with '../'
23+
// In this case, we should also check against the absolute path
24+
const isOutsideSourceRoot = relativeCdsFilePath.startsWith('../');
2125

2226
// Find the project this file belongs to
2327
for (const [projectDir, project] of projectMap.entries()) {
2428
if (
2529
project.cdsFiles.some(
26-
cdsFile => cdsFile === relativeCdsFilePath || relativeCdsFilePath.startsWith(projectDir),
30+
cdsFile =>
31+
cdsFile === relativeCdsFilePath ||
32+
relativeCdsFilePath.startsWith(projectDir) ||
33+
(isOutsideSourceRoot && cdsFile === cdsFilePath),
2734
)
2835
) {
2936
return projectDir;

extractors/cds/tools/test/src/cds/parser/files-to-compile.test.ts

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,44 @@
1-
import { mkdirSync, writeFileSync, rmSync } from 'fs';
2-
import { tmpdir } from 'os';
3-
import { join } from 'path';
1+
import { mkdirSync, writeFileSync } from 'fs';
2+
import { resolve, relative } from 'path';
3+
4+
import * as tmp from 'tmp';
45

56
import { determineCdsFilesToCompile } from '../../../../src/cds/parser/functions';
67

8+
/**
9+
* Validates that a path is safe to use within a base directory.
10+
* Prevents path traversal attacks by ensuring the resolved path stays within the base directory.
11+
*/
12+
function validateSafePath(basePath: string, ...pathSegments: string[]): string {
13+
const resolvedBase = resolve(basePath);
14+
const targetPath = resolve(basePath, ...pathSegments);
15+
16+
// Check if the resolved target path is within the base directory
17+
const relativePath = relative(resolvedBase, targetPath);
18+
if (relativePath.startsWith('..') || relativePath.includes('..')) {
19+
throw new Error(`Path traversal detected: ${pathSegments.join('/')}`);
20+
}
21+
22+
return targetPath;
23+
}
24+
725
describe('determineCdsFilesToCompile', () => {
826
let tempDir: string;
927
let sourceRoot: string;
28+
let tmpCleanup: (() => void) | undefined;
1029

1130
beforeEach(() => {
12-
// Create a unique temporary directory for each test
13-
tempDir = join(tmpdir(), `test-${Date.now()}-${Math.random().toString(36).substring(7)}`);
31+
// Create a secure temporary directory for each test
32+
const tmpObj = tmp.dirSync({ unsafeCleanup: true });
33+
tempDir = tmpObj.name;
1434
sourceRoot = tempDir;
15-
mkdirSync(tempDir, { recursive: true });
35+
tmpCleanup = tmpObj.removeCallback;
1636
});
1737

1838
afterEach(() => {
19-
// Clean up temporary directory
20-
if (tempDir) {
21-
rmSync(tempDir, { recursive: true, force: true });
39+
// Clean up temporary directory using tmp library's cleanup function
40+
if (tmpCleanup) {
41+
tmpCleanup();
2242
}
2343
});
2444

@@ -48,8 +68,8 @@ describe('determineCdsFilesToCompile', () => {
4868

4969
it('should return only root files for non-CAP projects with imports', () => {
5070
// Create test CDS files in a flat structure (not CAP-like)
51-
writeFileSync(join(sourceRoot, 'service.cds'), 'using from "./schema";');
52-
writeFileSync(join(sourceRoot, 'schema.cds'), 'entity Test {}');
71+
writeFileSync(validateSafePath(sourceRoot, 'service.cds'), 'using from "./schema";');
72+
writeFileSync(validateSafePath(sourceRoot, 'schema.cds'), 'entity Test {}');
5373

5474
const project = {
5575
projectDir: '.',
@@ -79,10 +99,10 @@ describe('determineCdsFilesToCompile', () => {
7999

80100
it('should use project-level compilation for CAP projects with srv and db directories', () => {
81101
// Create test CDS files
82-
mkdirSync(join(sourceRoot, 'srv'), { recursive: true });
83-
mkdirSync(join(sourceRoot, 'db'), { recursive: true });
84-
writeFileSync(join(sourceRoot, 'srv/service.cds'), 'using from "../db/schema";');
85-
writeFileSync(join(sourceRoot, 'db/schema.cds'), 'entity Test {}');
102+
mkdirSync(validateSafePath(sourceRoot, 'srv'), { recursive: true });
103+
mkdirSync(validateSafePath(sourceRoot, 'db'), { recursive: true });
104+
writeFileSync(validateSafePath(sourceRoot, 'srv/service.cds'), 'using from "../db/schema";');
105+
writeFileSync(validateSafePath(sourceRoot, 'db/schema.cds'), 'entity Test {}');
86106

87107
const project = {
88108
projectDir: '.',
@@ -111,11 +131,11 @@ describe('determineCdsFilesToCompile', () => {
111131

112132
it('should use project-level compilation for CAP projects with multiple services', () => {
113133
// Create test CDS files
114-
mkdirSync(join(sourceRoot, 'srv'), { recursive: true });
115-
mkdirSync(join(sourceRoot, 'db'), { recursive: true });
116-
writeFileSync(join(sourceRoot, 'srv/service1.cds'), 'using from "../db/schema";');
117-
writeFileSync(join(sourceRoot, 'srv/service2.cds'), 'using from "../db/schema";');
118-
writeFileSync(join(sourceRoot, 'db/schema.cds'), 'entity Test {}');
134+
mkdirSync(validateSafePath(sourceRoot, 'srv'), { recursive: true });
135+
mkdirSync(validateSafePath(sourceRoot, 'db'), { recursive: true });
136+
writeFileSync(validateSafePath(sourceRoot, 'srv/service1.cds'), 'using from "../db/schema";');
137+
writeFileSync(validateSafePath(sourceRoot, 'srv/service2.cds'), 'using from "../db/schema";');
138+
writeFileSync(validateSafePath(sourceRoot, 'db/schema.cds'), 'entity Test {}');
119139

120140
const project = {
121141
projectDir: '.',

extractors/cds/tools/test/src/cds/parser/project-aware-compilation.test.ts

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,55 @@
1-
import { mkdirSync, rmSync, writeFileSync } from 'fs';
2-
import { tmpdir } from 'os';
3-
import { join } from 'path';
1+
import { mkdirSync, writeFileSync } from 'fs';
2+
import { resolve, relative } from 'path';
3+
4+
import * as tmp from 'tmp';
45

56
import { determineCdsFilesToCompile } from '../../../../src/cds/parser/functions';
67

8+
/**
9+
* Validates that a path is safe to use within a base directory.
10+
* Prevents path traversal attacks by ensuring the resolved path stays within the base directory.
11+
*/
12+
function validateSafePath(basePath: string, ...pathSegments: string[]): string {
13+
const resolvedBase = resolve(basePath);
14+
const targetPath = resolve(basePath, ...pathSegments);
15+
16+
// Check if the resolved target path is within the base directory
17+
const relativePath = relative(resolvedBase, targetPath);
18+
if (relativePath.startsWith('..') || relativePath.includes('..')) {
19+
throw new Error(`Path traversal detected: ${pathSegments.join('/')}`);
20+
}
21+
22+
return targetPath;
23+
}
24+
725
describe('Project-aware compilation for CAP projects', () => {
826
let tempDir: string;
27+
let tmpCleanup: (() => void) | undefined;
928

1029
beforeEach(() => {
11-
// Create a temporary directory for each test
12-
tempDir = join(tmpdir(), `cds-test-${Date.now()}-${Math.random().toString(36).substring(7)}`);
13-
mkdirSync(tempDir, { recursive: true });
30+
// Create a secure temporary directory for each test
31+
const tmpObj = tmp.dirSync({ unsafeCleanup: true });
32+
tempDir = tmpObj.name;
33+
tmpCleanup = tmpObj.removeCallback;
1434
});
1535

1636
afterEach(() => {
17-
// Clean up temporary directory
18-
try {
19-
rmSync(tempDir, { recursive: true, force: true });
20-
} catch (error: unknown) {
21-
console.warn(`Warning: Could not clean up temp directory ${tempDir}: ${String(error)}`);
37+
// Clean up temporary directory using tmp library's cleanup function
38+
if (tmpCleanup) {
39+
tmpCleanup();
2240
}
2341
});
2442

2543
it('should use project-level compilation for log injection test case structure', () => {
2644
// Create the log injection test case structure
27-
const projectPath = join(tempDir, 'log-injection-without-protocol-none');
45+
const projectPath = validateSafePath(tempDir, 'log-injection-without-protocol-none');
2846
mkdirSync(projectPath, { recursive: true });
29-
mkdirSync(join(projectPath, 'db'), { recursive: true });
30-
mkdirSync(join(projectPath, 'srv'), { recursive: true });
47+
mkdirSync(validateSafePath(projectPath, 'db'), { recursive: true });
48+
mkdirSync(validateSafePath(projectPath, 'srv'), { recursive: true });
3149

3250
// Create package.json with CAP dependencies
3351
writeFileSync(
34-
join(projectPath, 'package.json'),
52+
validateSafePath(projectPath, 'package.json'),
3553
JSON.stringify({
3654
name: 'log-injection-test',
3755
dependencies: {
@@ -42,7 +60,7 @@ describe('Project-aware compilation for CAP projects', () => {
4260

4361
// Create CDS files
4462
writeFileSync(
45-
join(projectPath, 'db', 'schema.cds'),
63+
validateSafePath(projectPath, 'db', 'schema.cds'),
4664
`namespace advanced_security.log_injection.sample_entities;
4765
4866
entity Entity1 {
@@ -57,7 +75,7 @@ entity Entity2 {
5775
);
5876

5977
writeFileSync(
60-
join(projectPath, 'srv', 'service1.cds'),
78+
validateSafePath(projectPath, 'srv', 'service1.cds'),
6179
`using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema';
6280
6381
service Service1 @(path: '/service-1') {
@@ -70,7 +88,7 @@ service Service1 @(path: '/service-1') {
7088
);
7189

7290
writeFileSync(
73-
join(projectPath, 'srv', 'service2.cds'),
91+
validateSafePath(projectPath, 'srv', 'service2.cds'),
7492
`using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema';
7593
7694
service Service2 @(path: '/service-2') {
@@ -128,12 +146,12 @@ service Service2 @(path: '/service-2') {
128146

129147
it('should still use individual file compilation for simple projects without CAP structure', () => {
130148
// Create a simple project without typical CAP structure
131-
const simpleProjectPath = join(tempDir, 'simple-project');
149+
const simpleProjectPath = validateSafePath(tempDir, 'simple-project');
132150
mkdirSync(simpleProjectPath, { recursive: true });
133151

134152
// Create CDS files in flat structure
135153
writeFileSync(
136-
join(simpleProjectPath, 'main.cds'),
154+
validateSafePath(simpleProjectPath, 'main.cds'),
137155
`using from "./model";
138156
139157
service MainService {
@@ -142,7 +160,7 @@ service MainService {
142160
);
143161

144162
writeFileSync(
145-
join(simpleProjectPath, 'model.cds'),
163+
validateSafePath(simpleProjectPath, 'model.cds'),
146164
`namespace model;
147165
148166
entity Item {

0 commit comments

Comments
 (0)