index escrow events#1378
Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR successfully introduces indexing and querying for Ocean Protocol Escrow events. The implementation is well-architected, cleanly extending the existing Command Handler, Indexer, and Database patterns (both Elasticsearch and Typesense). Excellent use of data normalization and robust parsing logic for blockchain events.
Comments:
• [INFO][performance] When constructing Typesense filter_by queries, it's a recommended best practice to wrap string values in backticks to prevent syntax errors if the string ever contains spaces, hyphens, or other special characters. While the fields currently being filtered (addresses, hashes) are typically safe hex strings, adding backticks improves query robustness.
- .map(([field, value]) => `${field}:=${value}`)
+ .map(([field, value]) => `${field}:=${typeof value === 'string' ? `\`${value}\`` : value}`)• [WARNING][other] By default, Typesense indexes all string fields for searching and enforces a maximum length limit (often 2048 bytes per field) on indexed strings. The proof field from the Escrow Claimed event could potentially contain a large hex string exceeding this limit, which would cause record insertion to fail. Since it is highly unlikely you will need to search/filter by the proof value itself, consider disabling indexing for this field to avoid length restrictions.
- { name: 'proof', type: 'string', optional: true },
+ { name: 'proof', type: 'string', optional: true, index: false },• [INFO][style] Great job utilizing safe parsing helper functions (addr and num) to consistently handle missing arguments, enforce lowercase addresses, and prevent precision loss with uint256 values. The switch-case mapping for different escrow events is also very clean.
|
/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 PR implements escrow event indexing and querying, which cleanly extends the current architecture. It appropriately segregates database interactions and properly implements pagination. However, there is a NoSQL injection risk in the Typesense query builder and potential runtime crashes in the command handler when processing untrusted inputs.
Comments:
• [ERROR][security] User inputs (e.g., query string parameters) are passed directly into backticks within Typesense's filter_by syntax. If an attacker includes backticks in their payload, it breaks the string literal boundaries, resulting in a NoSQL injection vulnerability. Backticks should be stripped or escaped from the input values.
- typeof value === 'string' ? `${field}:=\`${value}\`` : `${field}:=${value}`
+ typeof value === 'string' ? `${field}:=\`${value.replace(/`/g, '')}\`` : `${field}:=${value}`• [WARNING][bug] P2P commands can send loosely structured JSON. If task.payer, task.payee, or task.token are provided as objects or arrays instead of strings, calling .toLowerCase() will throw a TypeError and cause an unhandled runtime error. Defensively check the types before calling string prototype methods.
- payer: task.payer ? task.payer.toLowerCase() : undefined,
- payee: task.payee ? task.payee.toLowerCase() : undefined,
- token: task.token ? task.token.toLowerCase() : undefined,
+ payer: typeof task.payer === 'string' ? task.payer.toLowerCase() : undefined,
+ payee: typeof task.payee === 'string' ? task.payee.toLowerCase() : undefined,
+ token: typeof task.token === 'string' ? task.token.toLowerCase() : undefined,• [INFO][performance] Good call using index: false for the proof field. This successfully avoids hitting Typesense's indexed-field length limitation for large cryptographic hex strings and preserves memory space.
Fixes #1355
Changes proposed in this PR: