feat(backend): add security check #163
Conversation
WalkthroughThis pull request introduces a new file for file upload validation and buffering. The new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as ProjectsResolver
participant Validator as validateAndBufferFile
Client->>Resolver: updateProjectPhoto(fileUpload)
Resolver->>Validator: validateAndBufferFile(fileUpload)
alt Validation Successful
Validator-->>Resolver: {buffer, mimetype}
Resolver-->>Client: Update Confirmation
else Validation Failure
Validator-->>Resolver: throw BadRequestException
Resolver-->>Client: Error Response
end
Possibly related PRs
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/project/project.resolver.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/common/security/file_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. ✨ Finishing Touches
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. 🪧 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: 1
🧹 Nitpick comments (3)
backend/src/common/security/file_check.ts (3)
14-19: Clear documentation for the function.The JSDoc comment provides good information about the function's purpose, parameters, return value, and potential exceptions.
Consider updating the
@returnsdocumentation to match the actual return type:- * @returns Promise<Buffer> - The file data in buffer format + * @returns Promise<{ buffer: Buffer; mimetype: string }> - The file data and MIME type
25-40: Add verification for MIME type and extension correlation.The current implementation validates MIME types and extensions separately but doesn't ensure they match (e.g., a JPEG file should have a .jpg/.jpeg extension).
Consider adding a map of valid MIME type to extension correlations:
// Validate MIME type if (!ALLOWED_MIME_TYPES.includes(mimetype)) { throw new BadRequestException( `Invalid file type: ${mimetype}. Only JPEG, PNG, and WebP are allowed.`, ); } // Validate file extension if (!ALLOWED_EXTENSIONS.includes(extension)) { throw new BadRequestException( `Invalid file extension: ${extension}. Only .jpg, .jpeg, .png, and .webp are allowed.`, ); } + +// Validate MIME type and extension correlation +const validMimeExtensionPairs = { + 'image/jpeg': ['.jpg', '.jpeg'], + 'image/png': ['.png'], + 'image/webp': ['.webp'] +}; + +if (!validMimeExtensionPairs[mimetype]?.includes(extension)) { + throw new BadRequestException( + `Mismatched file type (${mimetype}) and extension (${extension}).` + ); +}
1-57: Consider adding a logging mechanism for security-related events.For security features, it's beneficial to log validation failures to monitor potential abuse.
Add a logger to track validation failures:
-import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, Logger } from '@nestjs/common'; import { FileUpload } from 'graphql-upload-minimal'; import path from 'path'; +// Create a logger instance +const logger = new Logger('FileSecurityValidator'); + /** Maximum allowed file size (5MB) */ const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB // Then in the validation code: // Validate MIME type if (!ALLOWED_MIME_TYPES.includes(mimetype)) { + logger.warn(`Rejected file upload with invalid MIME type: ${mimetype}, filename: ${filename}`); throw new BadRequestException( `Invalid file type: ${mimetype}. Only JPEG, PNG, and WebP are allowed.`, ); }Also add similar logging for other validation failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/common/security/file_check.ts(1 hunks)backend/src/project/project.resolver.ts(3 hunks)
🔇 Additional comments (7)
backend/src/project/project.resolver.ts (2)
25-25: Good addition of security module import.The import is well-placed and appropriately named to reflect its security functionality.
102-103: Excellent refactoring of file handling logic.The code has been improved by replacing what was likely manual buffer handling with a dedicated security function. This centralization of file validation logic enhances security and reduces code duplication across the application.
backend/src/common/security/file_check.ts (5)
5-6: Good use of a constant for file size limit.Having the file size limit as a named constant makes the code more maintainable and easier to understand. The comment also helps clarify the unit (5MB).
8-9: Appropriate MIME type restrictions.Limiting acceptable file types to common image formats is a good security practice to prevent malicious file uploads.
11-12: Well-defined extension whitelist.Maintaining a separate list of allowed extensions provides an additional layer of validation beyond just MIME types.
20-23: Function signature matches intended use case.The function signature is well-designed, accepting a FileUpload object and returning both the buffer and MIME type, which are needed for the project photo update.
1-57:✅ Verification successful
Verify against potential path traversal attacks.
While the current implementation extracts file extensions, it should also sanitize filenames to prevent path traversal attacks if the filename is used for storage.
While the current code only uses the extension for validation, if you're going to use the filename elsewhere for storage, ensure you have proper sanitization:
🏁 Script executed:
#!/bin/bash # Check if there's any code using the filename directly for file operations # Look for any uses of the 'filename' variable from file uploads that might be used in file operations rg -A 3 -B 3 "filename.*createReadStream|createReadStream.*filename" # Also check for any code that might be storing files using the uploaded filename rg -A 3 -B 3 "writeFile.*filename|filename.*writeFile|fs\..*filename"Length of output: 706
Path Traversal Check Confirmed – No Additional Sanitization Needed
After verifying with the repository searches, the only use of the uploaded filename inbackend/src/common/security/file_check.tsis to extract its extension. There’s no evidence that the raw filename is used directly in any file operations (e.g., storage or write operations). While the current usage is safe, if you eventually use the filename for storage or file path construction, please ensure that proper sanitization is implemented to mitigate any potential path traversal risks.
| const chunks: Buffer[] = []; | ||
| let fileSize = 0; | ||
|
|
||
| // Read file stream and check size | ||
| for await (const chunk of createReadStream()) { | ||
| fileSize += chunk.length; | ||
| if (fileSize > MAX_FILE_SIZE) { | ||
| throw new BadRequestException( | ||
| 'File size exceeds the maximum allowed limit (5MB).', | ||
| ); | ||
| } | ||
| chunks.push(chunk); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for stream operations.
The file stream reading code should have appropriate error handling to prevent unhandled exceptions.
Add a try/catch block around the stream processing:
const chunks: Buffer[] = [];
let fileSize = 0;
// Read file stream and check size
+try {
for await (const chunk of createReadStream()) {
fileSize += chunk.length;
if (fileSize > MAX_FILE_SIZE) {
throw new BadRequestException(
'File size exceeds the maximum allowed limit (5MB).',
);
}
chunks.push(chunk);
}
+} catch (error) {
+ if (error instanceof BadRequestException) {
+ throw error;
+ }
+ throw new BadRequestException(
+ 'File upload failed: ' + (error.message || 'Unknown error')
+ );
+}📝 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 chunks: Buffer[] = []; | |
| let fileSize = 0; | |
| // Read file stream and check size | |
| for await (const chunk of createReadStream()) { | |
| fileSize += chunk.length; | |
| if (fileSize > MAX_FILE_SIZE) { | |
| throw new BadRequestException( | |
| 'File size exceeds the maximum allowed limit (5MB).', | |
| ); | |
| } | |
| chunks.push(chunk); | |
| } | |
| const chunks: Buffer[] = []; | |
| let fileSize = 0; | |
| // Read file stream and check size | |
| try { | |
| for await (const chunk of createReadStream()) { | |
| fileSize += chunk.length; | |
| if (fileSize > MAX_FILE_SIZE) { | |
| throw new BadRequestException( | |
| 'File size exceeds the maximum allowed limit (5MB).', | |
| ); | |
| } | |
| chunks.push(chunk); | |
| } | |
| } catch (error) { | |
| if (error instanceof BadRequestException) { | |
| throw error; | |
| } | |
| throw new BadRequestException( | |
| 'File upload failed: ' + (error.message || 'Unknown error') | |
| ); | |
| } |
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced file upload validation for project photos, ensuring only allowed image formats (JPEG, PNG, WebP) are accepted and that files do not exceed 5MB. - **Refactor** - Streamlined file handling during photo updates by centralizing validation logic, resulting in clearer error handling and improved reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit
New Features
Refactor