Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request refactors the FileInfoCommand and EncryptFileCommand to use a unified StorageObject type, simplifying the internal representation of various file storage methods (URL, IPFS, Arweave, FTP). The FileInfoHttpRequest type has also been updated to be more flexible, allowing direct StorageObject input or did/serviceId based requests. A key change involves the relocation and modification of input validation logic for file info requests. Basic checks for did format and FileObjectType enum are now handled in the core FileInfoHandler. However, a more comprehensive validation function previously present in httpRoutes/fileInfo.ts has been removed, replaced by a much simpler check, leading to potential gaps in input validation at the API entry point.
Comments:
• [ERROR][security] The removal of the comprehensive validateFileInfoRequest function without fully replicating its checks in FileInfoHandler introduces a significant security and stability risk. For example, requests for type: 'ipfs' without a hash, or type: 'url' without a url, will now pass the initial HTTP validation (if (!hasType && !hasDid) is too permissive), potentially leading to errors or unexpected behavior downstream. It's crucial to ensure all necessary input validations are performed at the API entry point or the command handler to prevent malformed requests from being processed effectively.
• [WARNING][security] The new validation check if (!hasType && !hasDid) is insufficient. It only ensures that either type or did is present, but it does not validate the completeness of the StorageObject itself. For instance, if type: 'ipfs' is provided, a hash is still required. This check should either be expanded to cover all necessary StorageObject properties based on type, or the FileInfoHandler must be guaranteed to handle these missing parameters gracefully and securely (e.g., by consistently returning 400 Bad Request).
• [WARNING][security] While moving the did format and FileObjectType enum validation to the core handler is a good step towards centralization, the handler currently lacks detailed validation for the specific properties required by each FileObjectType. For example, it does not check for the presence of hash when type is 'ipfs', url for 'url' or 'ftp', or transactionId for 'arweave'. Without these checks, the handler might receive incomplete StorageObject instances, leading to downstream errors. Consider adding these specific property validations here to ensure robust input validation at the command processing level, regardless of the command's origin.
• [INFO][style] Changing FileInfoHttpRequest to a type alias using a union of StorageObject and a did/serviceId object is a good improvement. It clearly reflects the two primary ways a file info request can be structured and enhances type safety and flexibility in the application.
Checks are now done in handler when calling |
AdriGeorge
left a comment
There was a problem hiding this comment.
LGTM, tested and worked as expecting
Changes proposed in this PR: