fix: add path traversal validation and safer file operations#53
fix: add path traversal validation and safer file operations#53
Conversation
WalkthroughAdds a canonical path validation helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Test Runner
participant Wrapper as wrap-performance-test.ts
participant Validator as validateFilePath
participant Setup as setup
participant FS as File System
Runner->>Wrapper: invoke(filePath)
Wrapper->>Validator: validateFilePath(filePath, baseDir)
Validator-->>Wrapper: canonicalPath OR error
alt valid
Wrapper->>FS: copyFile(canonicalPath -> inputDir)
Wrapper->>Setup: setup(canonicalPath)
Setup-->>Wrapper: ready
Wrapper->>FS: rmSync(inputDir, { recursive: true })
Wrapper->>FS: rmSync(outputDir, { recursive: true })
else invalid
Wrapper-->>Runner: throw validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
performance-tests/wrap-performance-test.ts(5 hunks)tradetrust-cli(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Linters / Code Lint
performance-tests/wrap-performance-test.ts
[failure] 32-32:
Replace '..' with ".."
[failure] 24-24:
Delete ····
[failure] 86-86:
Replace '..' with ".."
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests / Run Tests
🔇 Additional comments (3)
tradetrust-cli (1)
1-1: Submodule update acknowledged.The submodule pointer has been updated to commit
0e49230bb41cb4b7fc74cec001d4f0677616a548. The referenced commit contains a gas station URL fix that appears reasonable.performance-tests/wrap-performance-test.ts (2)
64-65: Good modernization of filesystem cleanup.The switch from
rmdirSynctormSyncis appropriate.rmSyncis the recommended API for recursive directory removal and provides better error handling.
51-54: Correct usage of validated paths.The file operations correctly use
resolvedFilePathfor both access checking and copying. This ensures that the validated path is consistently used throughout the function.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
performance-tests/wrap-performance-test.ts (1)
79-82: Remove redundant validation check.This validation at lines 79-82 is redundant with the more comprehensive validation performed at lines 84-88. The second check includes both the
startsWithvalidation and additional normalization checks.Apply this diff to remove the redundant check:
- // Ensure the file path is within the allowed directory - if (!absoluteFilePath.startsWith(__dirname)) { - throw new Error("Unsafe file path detected."); - } -
♻️ Duplicate comments (2)
performance-tests/wrap-performance-test.ts (2)
22-39: Path validation issues already flagged in previous reviews.The validation logic here has been identified in previous reviews as having:
- Redundant normalization (line 31)
- False positive risk with the
.includes('..')check (line 32)- Symlink vulnerability with the
startsWith(__dirname)approach (line 26)- Linting issues (lines 24, 32)
Please refer to the previous review comments for the recommended
realpath-based approach and shared validation helper.
84-88: Duplicate validation logic with issues flagged in previous reviews.This validation logic duplicates the problematic checks from the
setupfunction (lines 22-39) and has been flagged in previous reviews for:
- Redundant normalization
- False positive risk with
.includes('..')check- Linting issue at line 86 (single quotes)
Please refer to the previous review comments for the recommended refactored approach using a shared validation helper with
realpath.
🧹 Nitpick comments (1)
performance-tests/wrap-performance-test.ts (1)
27-27: Improve error message consistency.The error messages for path validation failures are inconsistent across the file:
- "Source file path is outside the allowed directory" (line 27)
- "Path traversal attempt detected in source file path" (line 33)
- "Unsafe file path detected" (lines 47, 81)
- "Path traversal attempt detected in input file path" (line 87)
Consider standardizing the error messages and including the actual path value for easier debugging.
Example:
throw new Error(`Path validation failed: "${filePath}" is outside the allowed directory`);Also applies to: 33-33, 47-47, 81-81, 87-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
performance-tests/wrap-performance-test.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests / Run Tests
🔇 Additional comments (2)
performance-tests/wrap-performance-test.ts (2)
4-4: LGTM: Imports support the new validation and cleanup logic.The added imports (
rmSync,promises,constants,normalize) are correctly used throughout the file for enhanced path validation and safer file operations.Also applies to: 7-7
64-65: LGTM: Correct migration tormSync.The replacement of
rmdirSyncwithrmSyncis the proper approach for recursive directory removal in modern Node.js.
rongquan1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
performance-tests/wrap-performance-test.ts (1)
44-47: Fix unsafe output path validation.The
startsWithcheck on line 45 has the same prefix-matching vulnerability as the helper function. IfINPUT_UNWRAPPED_FILE_FOLDERis/home/user/setup/raw-documents, a path like/home/user/setup/raw-documents-backup/file.jsonwould incorrectly pass validation.Apply this diff to fix the output path validation:
- // Sanitize the file path to prevent directory traversal - if (!outputPath.startsWith(INPUT_UNWRAPPED_FILE_FOLDER)) { - throw new Error("Unsafe file path detected."); - } + // Sanitize the file path to prevent directory traversal + const { relative, isAbsolute } = require("path"); + const relativeOutputPath = relative(INPUT_UNWRAPPED_FILE_FOLDER, outputPath); + if (relativeOutputPath.startsWith("..") || isAbsolute(relativeOutputPath)) { + throw new Error("Unsafe file path detected."); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
performance-tests/wrap-performance-test.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests / Run Tests
🔇 Additional comments (2)
performance-tests/wrap-performance-test.ts (2)
36-50: Good use of the validation helper.The canonical path is properly obtained and propagated from
monitorWrapFeaturetosetup, and the validated path is used for file operations. Once the helper function's validation logic is fixed, this usage will be secure.Also applies to: 72-76
58-62: LGTM: Proper use ofrmSync.Correctly replaced the deprecated
rmdirSyncwithrmSync. Therecursive: trueoption is appropriate for directory removal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
performance-tests/wrap-performance-test.ts (1)
46-51: Consider usingrelative()for consistency.The safety check on line 49 uses
startsWith(), which could theoretically have false positives when directory names share a prefix. While unlikely given howoutputPathis constructed, using the samerelative()approach asvalidateFilePathwould be more consistent and robust.Optional refactor:
const outputPath = resolve(INPUT_UNWRAPPED_FILE_FOLDER, `${fileName + (index + 1)}${fileExtension}`); // Sanitize the file path to prevent directory traversal - if (!outputPath.startsWith(INPUT_UNWRAPPED_FILE_FOLDER)) { + const relPath = relative(INPUT_UNWRAPPED_FILE_FOLDER, outputPath); + if (relPath.startsWith("..") || isAbsolute(relPath)) { throw new Error("Unsafe file path detected."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
performance-tests/wrap-performance-test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests / Run Tests
🔇 Additional comments (3)
performance-tests/wrap-performance-test.ts (3)
14-32: LGTM! Path validation is now secure.The
validateFilePathhelper correctly addresses the security concerns raised in previous reviews:
- ✅ Base directory is canonicalized (line 23) to handle symlinks
- ✅ Uses
relative()instead ofstartsWith()to prevent false positives- ✅ Checks for
".."prefix to detect parent directory escapes- ✅ Checks for absolute paths to catch different drive/root scenarios (Windows)
This implementation properly defends against symlink attacks and directory traversal.
64-65: LGTM! Using modernrmSyncAPI.Correctly replaces the deprecated
rmdirSyncwithrmSyncfor recursive directory removal.
76-80: LGTM! Input validation properly integrated.The monitor flow correctly validates the input file path and passes the canonical path to
setup, ensuring consistent security checks throughout the codebase.
|
🎉 This PR is included in version 3.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor