Skip to content

Commit b52db7b

Browse files
authored
Security fixes for TOCTOU & OS tmp files (#18)
* Fixes for TOCTOU & OS tmp files * Changes to address PR review comments
1 parent 2bb3df6 commit b52db7b

26 files changed

+545
-287
lines changed

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
.DS_Store
33
.cache/
44
.env
5+
6+
# Project-local temporary data (never use OS tmpdir)
7+
.tmp/
58
.env.*
69
!.env.example*
710
.idea/

client/src/lib/integration-test-runner.js

Lines changed: 75 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -454,20 +454,40 @@ export class IntegrationTestRunner {
454454
*/
455455
validateCodeQLQueryRunOutput(interpretedOutput, expectedOutputPath, toolName, testCase) {
456456
try {
457-
// Check if expected output exists
458-
if (!fs.existsSync(expectedOutputPath)) {
459-
// Expected output doesn't exist - validate non-empty content only
460-
this.logger.log(
461-
` Note: No expected output found at ${expectedOutputPath}, validating non-empty content only`
462-
);
463-
return this.validateNonEmptyOutput(interpretedOutput, toolName, testCase);
457+
// Read both paths directly to avoid TOCTOU race (CWE-367).
458+
// If a path is a directory, readFileSync throws EISDIR.
459+
// If a path doesn't exist, readFileSync throws ENOENT.
460+
let actualContent, expectedContent;
461+
let actualIsDir = false,
462+
expectedIsDir = false;
463+
464+
try {
465+
actualContent = fs.readFileSync(interpretedOutput, "utf8");
466+
} catch (readErr) {
467+
if (readErr.code === "EISDIR") {
468+
actualIsDir = true;
469+
} else {
470+
throw readErr;
471+
}
464472
}
465473

466-
// Compare actual output against expected output
467-
const actualStats = fs.statSync(interpretedOutput);
468-
const expectedStats = fs.statSync(expectedOutputPath);
474+
try {
475+
expectedContent = fs.readFileSync(expectedOutputPath, "utf8");
476+
} catch (readErr) {
477+
if (readErr.code === "EISDIR") {
478+
expectedIsDir = true;
479+
} else if (readErr.code === "ENOENT") {
480+
// Expected output doesn't exist - validate non-empty content only
481+
this.logger.log(
482+
` Note: No expected output found at ${expectedOutputPath}, validating non-empty content only`
483+
);
484+
return this.validateNonEmptyOutput(interpretedOutput, toolName, testCase);
485+
} else {
486+
throw readErr;
487+
}
488+
}
469489

470-
if (actualStats.isDirectory() && expectedStats.isDirectory()) {
490+
if (actualIsDir && expectedIsDir) {
471491
// Compare directory structures
472492
const comparisonResult = compareDirectories(interpretedOutput, expectedOutputPath);
473493
if (!comparisonResult) {
@@ -479,11 +499,8 @@ export class IntegrationTestRunner {
479499
this.logger.log(` ✓ Output files match expected output`);
480500
return true;
481501
}
482-
} else if (actualStats.isFile() && expectedStats.isFile()) {
483-
// Compare file contents
484-
const actualContent = fs.readFileSync(interpretedOutput, "utf8");
485-
const expectedContent = fs.readFileSync(expectedOutputPath, "utf8");
486-
502+
} else if (!actualIsDir && !expectedIsDir) {
503+
// Compare file contents (already read above)
487504
if (actualContent !== expectedContent) {
488505
this.logger.log(
489506
` Validation Failed: Output content does not match expected content for ${toolName}/${testCase}`
@@ -519,52 +536,55 @@ export class IntegrationTestRunner {
519536
*/
520537
validateNonEmptyOutput(outputPath, toolName, testCase) {
521538
try {
522-
const stats = fs.statSync(outputPath);
523-
524-
if (stats.isDirectory()) {
525-
// Find all output files in the directory using getDirectoryFiles
526-
const allFiles = getDirectoryFiles(outputPath);
527-
528-
// Filter for relevant output file extensions
529-
const outputExtensions = [".txt", ".dgml", ".dot", ".sarif", ".csv", ".json"];
530-
const outputFiles = allFiles.filter((file) =>
531-
outputExtensions.some((ext) => file.endsWith(ext))
532-
);
533-
534-
if (outputFiles.length === 0) {
535-
this.logger.log(
536-
` Validation Failed: No output files found in ${outputPath} for ${toolName}/${testCase}`
537-
);
538-
return false;
539-
}
540-
541-
// Check that at least one file has non-empty content
542-
let hasNonEmptyContent = false;
543-
for (const file of outputFiles) {
544-
const content = fs.readFileSync(file, "utf8");
545-
if (content.trim().length > 0) {
546-
hasNonEmptyContent = true;
547-
break;
548-
}
549-
}
550-
551-
if (!hasNonEmptyContent) {
552-
this.logger.log(
553-
` Validation Failed: All output files are empty for ${toolName}/${testCase}`
554-
);
555-
return false;
556-
}
557-
558-
return true;
559-
} else {
560-
// File - check if non-empty
539+
// Try reading as a file first to avoid TOCTOU race (CWE-367).
540+
// If the path is a directory, readFileSync throws EISDIR.
541+
try {
561542
const content = fs.readFileSync(outputPath, "utf8");
562543
if (content.trim().length === 0) {
563544
this.logger.log(` Validation Failed: Output file is empty for ${toolName}/${testCase}`);
564545
return false;
565546
}
566547
return true;
548+
} catch (readErr) {
549+
if (readErr.code !== "EISDIR") {
550+
throw readErr;
551+
}
552+
}
553+
554+
// Path is a directory - find and check output files
555+
const allFiles = getDirectoryFiles(outputPath);
556+
557+
// Filter for relevant output file extensions
558+
const outputExtensions = [".txt", ".dgml", ".dot", ".sarif", ".csv", ".json"];
559+
const outputFiles = allFiles.filter((file) =>
560+
outputExtensions.some((ext) => file.endsWith(ext))
561+
);
562+
563+
if (outputFiles.length === 0) {
564+
this.logger.log(
565+
` Validation Failed: No output files found in ${outputPath} for ${toolName}/${testCase}`
566+
);
567+
return false;
567568
}
569+
570+
// Check that at least one file has non-empty content
571+
let hasNonEmptyContent = false;
572+
for (const file of outputFiles) {
573+
const content = fs.readFileSync(file, "utf8");
574+
if (content.trim().length > 0) {
575+
hasNonEmptyContent = true;
576+
break;
577+
}
578+
}
579+
580+
if (!hasNonEmptyContent) {
581+
this.logger.log(
582+
` Validation Failed: All output files are empty for ${toolName}/${testCase}`
583+
);
584+
return false;
585+
}
586+
587+
return true;
568588
} catch (error) {
569589
this.logger.log(
570590
` Validation Error: Failed to check output at ${outputPath} for ${toolName}/${testCase}: ${error.message}`

0 commit comments

Comments
 (0)