feat(backend): improve path protection when writing file #128
Conversation
WalkthroughThis change refactors file path security validation in the backend. The previously used Changes
Sequence Diagram(s)sequenceDiagram
participant FOM as FileOperationManager
participant FPS as filePathSafetyChecks
participant FS as File System
FOM->>FPS: Validate filePath with { projectRoot }
alt Valid path
FPS-->>FOM: Return success
FOM->>FS: Execute file operation (write/read/rename)
else Invalid path
FPS-->>FOM: Throw error ("Unauthorized file access")
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/build-system/utils/security/path-check.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
backend/src/build-system/utils/security/Path_protect.ts (1)
1-1: Rename file to follow TypeScript naming conventions.The file name should use kebab-case instead of snake_case with underscores.
Rename
Path_protect.tstopath-protect.ts.backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts (1)
95-103: Add error handling for FilePathSafetyChecks.The file operation methods should catch and handle errors from FilePathSafetyChecks separately from file operation errors.
Example for handleWrite (apply similar pattern to handleRead and handleRename):
private async handleWrite(op: FileOperation): Promise<void> { const originalPath = path.resolve(this.projectRoot, op.originalPath); const securityOptions = { projectRoot: this.projectRoot, allowedPaths: this.allowedPaths }; - FilePathSafetyChecks(originalPath, securityOptions); + try { + FilePathSafetyChecks(originalPath, securityOptions); + } catch (error) { + this.logger.error(`Security check failed for ${op.originalPath}: ${error}`); + throw new Error(`Security violation: ${error.message}`); + } this.logger.debug('start update file to: ' + originalPath); const parseCode = removeCodeBlockFences(op.code); await writeFile(originalPath, parseCode, 'utf-8'); }Also applies to: 111-131, 138-150
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts(4 hunks)backend/src/build-system/utils/security/Path_protect.ts(1 hunks)
| export interface SecurityCheckOptions { | ||
| projectRoot: string; | ||
| allowedPaths?: string[]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation requirements to SecurityCheckOptions.
The interface should enforce that projectRoot is an absolute, normalized path and that allowedPaths contains valid paths.
export interface SecurityCheckOptions {
- projectRoot: string;
+ projectRoot: string & { __brand: 'AbsolutePath' }; // Type brand to ensure absolute path
allowedPaths?: string[];
+ // Consider adding:
+ // allowSymlinks?: boolean; // Whether to follow symbolic links
+ // maxDepth?: number; // Maximum directory depth allowed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SecurityCheckOptions { | |
| projectRoot: string; | |
| allowedPaths?: string[]; | |
| } | |
| export interface SecurityCheckOptions { | |
| projectRoot: string & { __brand: 'AbsolutePath' }; // Type brand to ensure absolute path | |
| allowedPaths?: string[]; | |
| // Consider adding: | |
| // allowSymlinks?: boolean; // Whether to follow symbolic links | |
| // maxDepth?: number; // Maximum directory depth allowed | |
| } |
| export function FilePathSafetyChecks( | ||
| filePath: string, | ||
| options: SecurityCheckOptions, | ||
| ) { | ||
| const { projectRoot, allowedPaths } = options; | ||
|
|
||
| const targetPath = path.resolve(projectRoot, filePath); | ||
| const relativePath = path.relative(projectRoot, targetPath); | ||
| // Prevent path traversal attacks | ||
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | ||
| throw new Error('Unauthorized file access detected'); | ||
| } | ||
|
|
||
| // To do white list check | ||
| } |
There was a problem hiding this comment.
Critical security improvements needed in FilePathSafetyChecks.
Several security concerns need to be addressed:
- The whitelist check is incomplete (TODO comment)
- No validation of projectRoot
- No handling of symbolic links
- No sanitization of input paths
Apply this diff to enhance security:
export function FilePathSafetyChecks(
filePath: string,
options: SecurityCheckOptions,
) {
const { projectRoot, allowedPaths } = options;
+
+ // Validate projectRoot
+ if (!path.isAbsolute(projectRoot)) {
+ throw new Error('Project root must be an absolute path');
+ }
+
+ // Sanitize input path
+ const sanitizedPath = path.normalize(filePath).replace(/\0/g, '');
- const targetPath = path.resolve(projectRoot, filePath);
+ const targetPath = path.resolve(projectRoot, sanitizedPath);
const relativePath = path.relative(projectRoot, targetPath);
+
// Prevent path traversal attacks
if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) {
throw new Error('Unauthorized file access detected');
}
- // To do white list check
+ // Implement whitelist check
+ if (allowedPaths?.length) {
+ const isAllowed = allowedPaths.some(allowedPath => {
+ const normalizedAllowedPath = path.normalize(allowedPath);
+ return relativePath.startsWith(normalizedAllowedPath);
+ });
+ if (!isAllowed) {
+ throw new Error('Path not in allowed paths list');
+ }
+ }
+
+ // Handle symbolic links
+ try {
+ const realPath = await fs.realpath(targetPath);
+ const realRelativePath = path.relative(projectRoot, realPath);
+ if (realRelativePath.startsWith('..') || path.isAbsolute(realRelativePath)) {
+ throw new Error('Symbolic link points outside project root');
+ }
+ } catch (error) {
+ if (error.code !== 'ENOENT') {
+ throw new Error(`Failed to resolve symbolic link: ${error.message}`);
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const securityOptions = { projectRoot: this.projectRoot }; | ||
| FilePathSafetyChecks(originalPath, securityOptions); |
There was a problem hiding this comment.
Pass allowedPaths to security checks.
The security options object should include the allowedPaths array that's initialized in the constructor.
- const securityOptions = { projectRoot: this.projectRoot };
+ const securityOptions = {
+ projectRoot: this.projectRoot,
+ allowedPaths: this.allowedPaths
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const securityOptions = { projectRoot: this.projectRoot }; | |
| FilePathSafetyChecks(originalPath, securityOptions); | |
| const securityOptions = { | |
| projectRoot: this.projectRoot, | |
| allowedPaths: this.allowedPaths | |
| }; | |
| FilePathSafetyChecks(originalPath, securityOptions); |
| const securityOptions = { projectRoot: this.projectRoot }; | ||
| FilePathSafetyChecks(originalPath, securityOptions); |
There was a problem hiding this comment.
Pass allowedPaths to security checks in handleRead.
Same issue as in handleWrite - the security options should include allowedPaths.
- const securityOptions = { projectRoot: this.projectRoot };
+ const securityOptions = {
+ projectRoot: this.projectRoot,
+ allowedPaths: this.allowedPaths
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const securityOptions = { projectRoot: this.projectRoot }; | |
| FilePathSafetyChecks(originalPath, securityOptions); | |
| const securityOptions = { | |
| projectRoot: this.projectRoot, | |
| allowedPaths: this.allowedPaths | |
| }; | |
| FilePathSafetyChecks(originalPath, securityOptions); |
| const RenamePath = path.resolve(this.projectRoot, op.renamePath); | ||
| const securityOptions = { projectRoot: this.projectRoot }; | ||
|
|
||
| this.safetyChecks(originalPath); | ||
| this.safetyChecks(RenamePath); | ||
| FilePathSafetyChecks(originalPath, securityOptions); | ||
| FilePathSafetyChecks(RenamePath, securityOptions); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable naming and security options in handleRename.
Issues to address:
- Inconsistent variable naming (RenamePath vs renamePath)
- Security options missing allowedPaths
- Reusing same options object for both paths
- const RenamePath = path.resolve(this.projectRoot, op.renamePath);
- const securityOptions = { projectRoot: this.projectRoot };
+ const renamePath = path.resolve(this.projectRoot, op.renamePath);
+ const securityOptions = {
+ projectRoot: this.projectRoot,
+ allowedPaths: this.allowedPaths
+ };
FilePathSafetyChecks(originalPath, securityOptions);
- FilePathSafetyChecks(RenamePath, securityOptions);
+ FilePathSafetyChecks(renamePath, securityOptions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RenamePath = path.resolve(this.projectRoot, op.renamePath); | |
| const securityOptions = { projectRoot: this.projectRoot }; | |
| this.safetyChecks(originalPath); | |
| this.safetyChecks(RenamePath); | |
| FilePathSafetyChecks(originalPath, securityOptions); | |
| FilePathSafetyChecks(RenamePath, securityOptions); | |
| const renamePath = path.resolve(this.projectRoot, op.renamePath); | |
| const securityOptions = { | |
| projectRoot: this.projectRoot, | |
| allowedPaths: this.allowedPaths | |
| }; | |
| FilePathSafetyChecks(originalPath, securityOptions); | |
| FilePathSafetyChecks(renamePath, securityOptions); |
Sma1lboy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts (1)
149-149:⚠️ Potential issueUpdate variable name in rename function call.
Ensure you're using the renamed variable here as well.
- await rename(originalPath, RenamePath); + await rename(originalPath, renamePath);
🧹 Nitpick comments (2)
backend/src/build-system/utils/security/path-check.ts (1)
17-20: Consider using camelCase for function name.TypeScript/JavaScript conventions typically use camelCase for functions and PascalCase for classes/interfaces.
-export function FilePathSafetyChecks( +export function filePathSafetyChecks( filePath: string, options: SecurityCheckOptions, )backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts (1)
146-148: Update variable name in debug logs.For consistency, update the debug logs to use the same variable name.
this.logger.debug('start rename: ' + originalPath); - this.logger.debug('change to name: ' + RenamePath); + this.logger.debug('change to name: ' + renamePath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts(4 hunks)backend/src/build-system/utils/security/path-check.ts(1 hunks)
🔇 Additional comments (7)
backend/src/build-system/utils/security/path-check.ts (3)
4-7: Interface looks good!The
SecurityCheckOptionsinterface is well-defined with clear parameters for the security checks.
9-16: Well-documented function!The JSDoc comments are thorough and clearly explain the purpose, parameters, and potential errors.
21-28: Security check implementation looks solid.The path traversal protection logic is correctly implemented to prevent directory traversal attacks.
backend/src/build-system/handlers/frontend-code-generate/FileOperationManager.ts (4)
16-17: Import looks good!The import statement for the new security utility function is correctly implemented.
97-98: Pass allowedPaths to security checks.The security options object should include the
allowedPathsarray that's initialized in the constructor.- const securityOptions = { projectRoot: this.projectRoot }; + const securityOptions = { + projectRoot: this.projectRoot, + allowedPaths: this.allowedPaths + };
114-115: Pass allowedPaths to security checks in handleRead.Same issue as in handleWrite - the security options should include allowedPaths.
- const securityOptions = { projectRoot: this.projectRoot }; + const securityOptions = { + projectRoot: this.projectRoot, + allowedPaths: this.allowedPaths + };
139-144: Fix variable naming and security options in handleRename.Issues to address:
- Inconsistent variable naming (RenamePath vs renamePath)
- Security options missing allowedPaths
- Reusing same options object for both paths
- const RenamePath = path.resolve(this.projectRoot, op.renamePath); - const securityOptions = { projectRoot: this.projectRoot }; + const renamePath = path.resolve(this.projectRoot, op.renamePath); + const securityOptions = { + projectRoot: this.projectRoot, + allowedPaths: this.allowedPaths + }; FilePathSafetyChecks(originalPath, securityOptions); - FilePathSafetyChecks(RenamePath, securityOptions); + FilePathSafetyChecks(renamePath, securityOptions);
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced file operation security with a unified validation process, ensuring robust handling of file actions. - **Refactor** - Streamlined file validation by consolidating multiple checks into one efficient routine for improved consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Refactor