Potential fix for code scanning alert no. 184: Type confusion through parameter tampering#205
Potential fix for code scanning alert no. 184: Type confusion through parameter tampering#205perinst wants to merge 1 commit intofeature/mergefrom
Conversation
… parameter tampering Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
WalkthroughAdds runtime validation for req.files, ensuring it is a non-empty array before processing. On invalid input, throws a BadRequest with a specific message. The service call now casts at the call site while leaving other logic unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant UC as UploadFileController
participant S as UploadFileService
C->>UC: POST /uploads (multipart files)
UC->>UC: Validate req.files is non-empty array
alt Invalid files
UC-->>C: 400 BadRequest ("No files uploaded or malformed files parameter")
else Valid files
UC->>S: processMultipleFiles(files as Express.Multer.File[])
S-->>UC: Result
UC-->>C: 200 OK (result)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (2)
src/controllers/uploads/upload.file.controller.ts (2)
103-117: Make cleanup resilient to memoryStorage (no path).fs.existsSync(undefined) throws; you catch it, but it spams logs. Guard path presence first.
- req.files.forEach((file: Express.Multer.File) => { + req.files.forEach((file: Express.Multer.File) => { try { - if (fs.existsSync(file.path)) { + if (typeof (file as any).path === 'string' && fs.existsSync((file as any).path)) { - fs.unlinkSync(file.path); + fs.unlinkSync((file as any).path); } } catch (cleanupError) {
1-1: Use a type-only import for Express to avoid emitting a runtime import.Small TS hygiene improvement.
-import { Express } from 'express'; +import type { Express } from 'express';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/controllers/uploads/upload.file.controller.ts(1 hunks)
🔇 Additional comments (1)
src/controllers/uploads/upload.file.controller.ts (1)
88-95: Ignore this comment – the middleware already usesmulter.array, soreq.filesis guaranteed to be an array.Likely an incorrect or invalid review comment.
| const files = req.files; | ||
| if (!Array.isArray(files) || files.length === 0) { | ||
| throw new BadRequest('No files uploaded or malformed files parameter'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden req.files validation with a proper type guard (array + element shape).
Array-only check still permits type confusion via arrays of non-file objects. Validate each element has expected Multer fields, then narrow to Express.Multer.File[].
Apply within this hunk:
- const files = req.files;
- if (!Array.isArray(files) || files.length === 0) {
- throw new BadRequest('No files uploaded or malformed files parameter');
- }
+ const files = req.files;
+ if (!isMulterFileArray(files) || files.length === 0) {
+ throw new BadRequest('No files uploaded or malformed files parameter');
+ }Add these type guards near the top of the file (after imports):
// Runtime type guards for Multer file(s)
const isMulterLikeFile = (f: unknown): f is Express.Multer.File =>
!!f &&
typeof f === 'object' &&
'originalname' in (f as any) &&
'mimetype' in (f as any) &&
('path' in (f as any) || 'buffer' in (f as any));
const isMulterFileArray = (x: unknown): x is Express.Multer.File[] =>
Array.isArray(x) && x.every(isMulterLikeFile);🤖 Prompt for AI Agents
In src/controllers/uploads/upload.file.controller.ts around lines 88 to 91, the
current check only verifies req.files is an array but allows non-file objects;
add runtime type guards after imports: isMulterLikeFile (checks object,
originalname, mimetype and path or buffer) and isMulterFileArray (Array.isArray
+ every(isMulterLikeFile)), then replace the existing Array.isArray(files) check
with isMulterFileArray(req.files), throwing BadRequest if it fails and narrowing
files to Express.Multer.File[] when it passes so downstream code has the correct
type.
|
|
||
| // Process the uploaded files | ||
| const result = await uploadFileService.processMultipleFiles(files); | ||
| const result = await uploadFileService.processMultipleFiles(files as Express.Multer.File[]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unsafe type assertion; rely on narrowed type.
Once narrowed with a type guard, you can pass files without casting.
- const result = await uploadFileService.processMultipleFiles(files as Express.Multer.File[]);
+ const result = await uploadFileService.processMultipleFiles(files);📝 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 result = await uploadFileService.processMultipleFiles(files as Express.Multer.File[]); | |
| const result = await uploadFileService.processMultipleFiles(files); |
🤖 Prompt for AI Agents
In src/controllers/uploads/upload.file.controller.ts around line 94, the call
currently uses an unsafe type assertion "files as Express.Multer.File[]";
instead narrow the files variable with a proper type guard or refined check
before calling uploadFileService.processMultipleFiles so you can pass files
directly without casting. Add or use a predicate like isMulterFileArray(files):
files is Express.Multer.File[] (or refine the existing conditional that ensures
files is an array of Multer.File), and then call processMultipleFiles(files)
using the narrowed type.
|
1 participant
Potential fix for https://github.com/perinst/dozu-api-service/security/code-scanning/184
The correct fix is to perform a runtime type check on
filesimmediately after assignment fromreq.filesin the controller (uploadMultipleFiles). Specifically, before using any array operations (like.lengthorfor ... of), ensure thatfilesis an array and not a string or other unexpected type. If the check fails, throw aBadRequesterror.Change the following in
src/controllers/uploads/upload.file.controller.ts:uploadMultipleFiles, after assigningconst files = req.files as Express.Multer.File[];, check thatfilesis an array usingArray.isArray(files).BadRequestwith a message like 'Malformed files parameter'.as Express.Multer.File[]from the assignment; cast after the check for type safety).This ensures the downstream code (
processMultipleFiles) receives a real array only, preventing type confusion and abuse.No changes are required in the service; the controller will now guarantee the type for the service.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by CodeRabbit