refactor signature

[alexcos20]

BREAKING CHANGES !!!

Summary

This PR refactors signature validation across the Ocean Protocol node to use a standardized message format that includes the command being executed. This improves security by ensuring signatures are command-specific and prevents signature reuse across different operations.

Key Changes:

• Standardized signature message format: consumer + nonce + command
• Created reusable createHashForSignature() utility function for tests
• Refactored AdminCommand interface to SignedCommand with improved structure
• Updated all handlers and tests to use new signature format
• Nonce is now a float (real) value, instead of integer
• Remove hardcoded wallets in integration tests

Motivation

Security Improvements

• Command-Specific Signatures: Signatures now include the command being executed, preventing signature reuse across different operations
• Consistent Message Format: All signatures use the same format (consumer + nonce + command), reducing potential vulnerabilities from inconsistent implementations
• Better Replay Protection: The standardized format ensures nonces are properly tied to specific commands

Code Quality Improvements

• DRY Principle: Centralized signature generation logic in createHashForSignature() utility
• Type Safety: Better type definitions with SignedCommand interface
• Maintainability: Easier to update signature logic in one place

[alexcos20]

/run-security-scan

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: high

Summary:
This pull request introduces a significant refactoring of the signature validation mechanism, moving from expiryTimestamp to a nonce-based system where the command type is explicitly part of the signed message. This enhances security by preventing replay attacks and standardizes the validation logic across admin and consumer commands. The PR also includes various cleanups, test updates, and minor performance/logging adjustments.

Comments:
• [INFO][other] The renaming of AdminCommand to SignedCommand and the introduction of nonce instead of expiryTimestamp along with making address mandatory, represents a substantial and positive security improvement. This standardizes how commands requiring a signature are structured.
• [BUG][bug] Good catch! The IndexingCommand enum values STOP_THREAD and START_THREAD were previously inverted or incorrect (start/stop). This change corrects their mapping, preventing logical errors in indexing control.
• [WARNING][bug] The nonce is typically an integer (e.g., Date.now()). Using parseFloat(nonce) here could introduce floating-point precision issues if nonce were to unexpectedly contain decimal values, or is simply redundant if it's always an integer. Consider using parseInt(nonce) for clarity and to explicitly state the expected integer nature of a nonce, or ensure that nonces are always handled as strings if they can be floats and precision is critical. Given Date.now() is integer milliseconds, parseInt would be more accurate for that common use case.
• [WARNING][bug] Similar to src/components/Auth/index.ts, using parseFloat for a nonce that is expected to be an integer (like Date.now()) might be misleading or introduce precision issues. Recommend parseInt if nonces are strictly integers, or clarify why floating-point numbers are expected for nonces.
• [INFO][style] Changing logMessage to debug for decryptDDO flag logging is a good move to reduce noise in standard production logs while retaining information for debugging purposes.
• [ERROR][security] ### Potential Security Vulnerability: DecryptDDO Signature Message

Description: The message used for signing in decryptDDO operations has been changed from useTxIdOrContractAddress + ethAddress + chainId.toString() + nonce to String(ethAddress) + String(nonce) + String(PROTOCOL_COMMANDS.DECRYPT_DDO). This updated signed message no longer includes a specific identifier for the DDO being decrypted (useTxIdOrContractAddress which could be txId or contractAddress).

Vulnerability: An attacker could record a valid signature generated by a decrypterAddress for decrypting Asset A (e.g., identified by txId_A or dataNftAddress_A) with a specific nonce. If the attacker then wishes to decrypt Asset B (identified by txId_B or dataNftAddress_B) using the same decrypterAddress and nonce, they could potentially replay the signature obtained for Asset A. Since the asset identifier is not part of the signed message, the signature would appear valid for any DECRYPT_DDO command from that decrypterAddress with that nonce.

Impact: Unauthorized decryption of DDOs by replaying valid signatures for different assets.

Recommendation: Re-incorporate a unique identifier for the asset being decrypted (e.g., transactionId or dataNftAddress) into the signed message for PROTOCOL_COMMANDS.DECRYPT_DDO. The message should unambiguously specify which DDO the decrypterAddress is authorizing to decrypt with that nonce.
• [INFO][security] The introduction of validateTokenOrSignature in AdminCommandHandler is a significant security improvement. It centralizes and strengthens the authentication logic by validating the signature, nonce, and checking against both direct admin addresses and access lists. This makes the admin commands more robust against unauthorized access and replay attacks.
• [INFO][other] Adding the ability to retrieve a single log by logId is a useful enhancement for debugging and specific log auditing. The fallback to file logs if no database logs are found is a good resilience measure.
• [ERROR][security] ### Potential Security Vulnerability: DecryptDDO Signature Message

Description: The message used for signing DECRYPT_DDO commands via this.validateTokenOrSignature in DecryptDdoHandler also suffers from the same vulnerability described in BaseProcessor.ts.

Vulnerability: The validateTokenOrSignature (which calls checkNonce and validateNonceAndSignature) uses the message String(String(consumer) + String(nonce) + String(command)). In the context of DecryptDdoHandler, consumer is task.decrypterAddress, nonce is task.nonce, and command is PROTOCOL_COMMANDS.DECRYPT_DDO. Crucially, the signed message does not include task.dataNftAddress or task.transactionId, which identify the specific DDO being decrypted.

Impact: This could allow an attacker to replay a valid decryption signature for one DDO to decrypt a different DDO, as long as it's from the same decrypterAddress and nonce.

Recommendation: The command parameter passed to validateTokenOrSignature for PROTOCOL_COMMANDS.DECRYPT_DDO must incorporate the unique DDO identifier (e.g., dataNftAddress or transactionId) to prevent replay attacks across different assets. This might require updating DecryptDDOCommand to include a field that forms the unique part of the signed command message.
• [INFO][other] Changing PROTOCOL_COMMANDS.DOWNLOAD_URL to PROTOCOL_COMMANDS.DOWNLOAD in the DownloadHandler is a good simplification if DOWNLOAD_URL is being deprecated or merged. However, it's important to confirm if any external clients or internal components still rely on DOWNLOAD_URL as a distinct command, as this could be a breaking change.
• [INFO][security] The new canonical message construction for signature validation String(String(consumer) + String(nonce) + String(command)) is a very positive change. By explicitly including the command in the signed message, it effectively prevents cross-command replay attacks (e.g., replaying a valid CREATE_AUTH_TOKEN signature for a DOWNLOAD command). This significantly strengthens the overall security posture related to signed messages.
• [INFO][performance] Changing refresh: 'wait_for' to refresh: true forces an immediate index refresh after write operations. While true implies wait_for, this could marginally impact write performance for very high-volume operations, as it guarantees visibility immediately. This is usually acceptable for ensuring data consistency in reads immediately following writes, but it's a minor trade-off to be aware of.
• [WARNING][bug] Changing the nonce column type from INTEGER to REAL in SQLite. If nonces are exclusively timestamps (like Date.now() which are integers) or sequential integers, REAL is not ideal due to potential floating-point precision issues that could lead to subtle bugs or security vulnerabilities if comparisons are not exact. If nonces are indeed always integers, INTEGER should be preferred. If nonces can be non-integer numbers, this should be explicitly documented and the implications of floating-point comparison carefully considered.
• [INFO][style] The refactoring of the request body parsing and command parameter extraction for VALIDATE_DDO is cleaner and more robust. Removing JSON.parse(req.body) is correct as express.json() middleware should handle this. Delegating the ddo.version check to the handler is also a good separation of concerns.
• [INFO][refactor] The removal of the validateRequest middleware and moving signature validation into GetLogsHandler is a good architectural choice. It centralizes the command-specific validation logic within its respective handler, improving maintainability and consistency.
• [INFO][other] Adding nodePublicKey to the root endpoint (/) response is beneficial. It provides clients with a crucial piece of information for secure communication and validation against this specific Ocean Node.
• [INFO][other] The deletion of clientExample.ts implies that this helper script is no longer needed or maintained. If this functionality is still desired for developers, ensure there's an alternative or updated example available.
• [INFO][refactor] Replacing direct ethers.Wallet instantiation with provider.getSigner() for test accounts is a good practice. It makes tests more robust and less reliant on hardcoded private keys, which improves security and flexibility, especially when running against various RPC environments.
• [INFO][refactor] The removal of addresses.ts (which contained ganachePrivateKeys) aligns with the move towards using provider.getSigner() in tests, eliminating hardcoded private keys and improving test setup.
• [INFO][refactor] The introduction of createHashForSignature and safeSign as utility functions for tests is excellent. This centralizes the logic for generating consistent signed messages and handles potential differences in signing methods (e.g., _legacySignMessage for Ganache), making test code cleaner and more reliable.
• [INFO][refactor] The complete removal of validateAdminSignature from utils/auth.ts is justified given that its logic has been refactored and moved into AdminCommandHandler's validateTokenOrSignature method, leading to better encapsulation and separation of concerns.
• [INFO][refactor] The removal of getPeerIdFromPrivateKey suggests that the process of generating peer IDs from private keys might be handled differently or is no longer directly exposed in this utility. Ensure that peer ID generation is still correctly managed elsewhere if this was a necessary utility function.
• [INFO][refactor] Removing DOWNLOAD_URL as a distinct protocol command and likely consolidating it under DOWNLOAD simplifies the command set. Confirm that all internal and external usages have been updated to prevent breaking changes for existing integrations.