Conversation
|
depends on oceanprotocol/ocean-node#1329 and it's hardcoded in ci.yml, so before merging this, we need to wait for node and then clean up |
|
/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 introduces comprehensive support for persistent storage features within ocean.js, allowing interaction with provider nodes for bucket and file management. It includes new type definitions, API methods in both HTTP and P2P providers, and integration tests to cover the core functionality. The implementation correctly handles authentication using signatures and nonces, and provides cross-environment streaming for file uploads.
Comments:
• [INFO][other] The NODE_VERSION: pr-1329 for Barge is very specific. While this might be a temporary or internal tag for CI, it's worth documenting or explaining its purpose if it's not self-evident. Does pr-1329 refer to a specific branch, commit, or Docker image version?
• [INFO][other] The Node.js version is bumped from 20 to 22. This is a significant update. Ensure all dependencies and underlying systems are compatible with Node.js 22. Given this is a client library, it mainly impacts the build/dev environment, but it's a good change for keeping up-to-date.
• [WARNING][style] The type PersistentStorageAccessList is defined as [key: string]: unknown. This is overly generic. In src/@types/Provider.ts, NodeStatus.persistentStorage.accessLists uses AccessList[]. If PersistentStorageAccessList is meant to represent an item within an access list (e.g., specific chain ID and addresses), it should be more strictly defined, potentially mirroring the AccessList interface or a more specific structure like interface PersistentStorageAccessListItem { chainId: string; addresses: string[] }. Currently, its definition as [key: string]: unknown makes it very permissive and could lead to runtime errors if the expected structure isn't unknown.
• [INFO][security] The nonce + 1 logic is crucial for preventing replay attacks for signed requests. This is correctly implemented in both HttpProvider and P2pProvider for persistent storage operations, which is good.
• [INFO][performance] The uploadPersistentStorageFile method includes robust cross-environment streaming logic (Node.js stream vs. Browser ReadableStream). This is well-implemented and handles various input chunk types, which is excellent for usability and compatibility.
• [INFO][other] Error handling typically involves throw new Error(await response.text()). While useful for debugging, in a production environment, exposing raw server error messages might be undesirable. Consider wrapping generic HTTP errors with more user-friendly messages, or only exposing the full server response for specific debugging scenarios, while logging full details internally.
• [INFO][security] Similar to HttpProvider, the nonce + 1 logic for P2P persistent storage operations ensures replay protection, which is a good security measure.
• [INFO][other] The list methods (getPersistentStorageBuckets, listPersistentStorageFiles) defensively return Array.isArray(result) ? result : []. This is a good practice to ensure type safety and prevent errors if the backend returns a non-array value for a list endpoint.
• [INFO][other] The test denies a non-owner not in bucket ACL is a critical security test case and is well-implemented, ensuring access control mechanisms are functioning as expected.
Fixes # .
Changes proposed in this PR: