Fix RPC cross-contract event-filter leak - #1372
Conversation
Non-wildcard selections are pooled and queried against every contract's addresses (addressesByContractNameGetAll), so one contract's unfiltered query fetches a sibling contract's logs, which route back by address and bypass that contract's `where` filter (routing never re-applies it). Reproduces end-to-end through getItemsOrThrow with a mock that honors the `address` filter: ContractA filters on topic1, ContractB is unfiltered and shares topic0, and ContractB's pooled query leaks a ContractA log past ContractA's filter. Uses `it_fails` asserting the correct (no-leak) behavior, so it stays green while the bug is present and flips red once selections are scoped per contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
📝 WalkthroughWalkthroughRpcSource.res's log selection construction was refactored to partition event filters into address-independent and per-contract (static, dynamic, dynamic wildcard) topic selection groups, replacing the prior wildcard/address decomposition and buildLogSelections implementation. A new test verifies per-contract address scoping prevents cross-contract log leakage. ChangesRpcSource log selection scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
getSelectionConfig pooled all non-wildcard topic selections and queried each against every contract's addresses, so one contract's query could fetch a sibling contract's logs sharing the same topic0. Those routed back to the sibling by address and bypassed its `where` filter, since routing never re-applies the topic filter. Group address-bound events by contract and scope each contract's selections to its own addresses (mirroring HyperSyncSource). Pure-wildcard events carry no address constraint, so they stay pooled and are resolved once. Converts the reproduction test to a passing regression guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/RpcSource_test.res (1)
1911-1914: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the scoped request shape, not only the empty result.
This regression would still pass if
getLogSelectionsOrThrowaccidentally returned no log selections. Also assert that twoeth_getLogsrequests were made and scoped to[addrA]/[addrB]without an unscoped address query.Suggested test strengthening
- t.expect(result.parsedQueueItems->Array.length).toEqual(0) + let getLogsAddressSets = + mock.requests->Array.filterMap(body => + switch body->JSON.parseOrThrow->JSON.Decode.object { + | Some(obj) if obj->Dict.get("method")->Option.flatMap(JSON.Decode.string) == Some("eth_getLogs") => + obj + ->Dict.get("params") + ->Option.flatMap(JSON.Decode.array) + ->Option.flatMap(a => a->Array.get(0)) + ->Option.flatMap(JSON.Decode.object) + ->Option.map(filter => + switch filter->Dict.get("address") { + | Some(JSON.Array(addrs)) => addrs->Array.filterMap(JSON.Decode.string) + | Some(JSON.String(addr)) => [addr] + | _ => [] + } + ) + | _ => None + } + ) + let addrAText = addrA->Address.toString + let addrBText = addrB->Address.toString + t.expect({ + "parsedItems": result.parsedQueueItems->Array.length, + "requestCount": getLogsAddressSets->Array.length, + "hasContractAQuery": getLogsAddressSets->Array.some(addrs => addrs == [addrAText]), + "hasContractBQuery": getLogsAddressSets->Array.some(addrs => addrs == [addrBText]), + "hasUnscopedQuery": getLogsAddressSets->Array.some(addrs => addrs == []), + }).toEqual({ + "parsedItems": 0, + "requestCount": 2, + "hasContractAQuery": true, + "hasContractBQuery": true, + "hasUnscopedQuery": false, + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/RpcSource_test.res` around lines 1911 - 1914, Strengthen the RpcSource_test.res regression by asserting the request shape in addition to the empty parsed result. In the test around the addrA case, verify that getLogSelectionsOrThrow produces two eth_getLogs calls, and that the emitted requests are scoped to [addrA] and [addrB] rather than making any unscoped address query. Use the existing result.parsedQueueItems assertion as a secondary check, but add direct assertions on the captured log request payloads so the test fails if log selections are missing or incorrectly scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scenarios/test_codegen/test/RpcSource_test.res`:
- Around line 1911-1914: Strengthen the RpcSource_test.res regression by
asserting the request shape in addition to the empty parsed result. In the test
around the addrA case, verify that getLogSelectionsOrThrow produces two
eth_getLogs calls, and that the emitted requests are scoped to [addrA] and
[addrB] rather than making any unscoped address query. Use the existing
result.parsedQueueItems assertion as a secondary check, but add direct
assertions on the captured log request payloads so the test fails if log
selections are missing or incorrectly scoped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 636c15a9-0e8e-4175-8640-d9b130aa4556
📒 Files selected for processing (2)
packages/envio/src/sources/RpcSource.resscenarios/test_codegen/test/RpcSource_test.res
Summary
Fixes a filter-bypass bug in the RPC data source (surfaced by CodeRabbit on #1368), with a regression test.
The bug
RpcSource.getSelectionConfigpooled all non-wildcard events' topic selections and queried each againstaddressesByContractNameGetAll— the union of every contract's addresses. When two non-wildcard contracts share atopic0(same event signature) but have differentwherefilters, one contract's query fetches the other's logs:ContractA—Transferwithwhere: { to: … }→ selection[transferSig, [to]]ContractB—Transferunfiltered → selection[transferSig]ContractB's unfiltered query ran against{ContractA, ContractB}addresses, so it fetchedContractA's transfers. Those routed back toContractAby address (getItemsOrThrowroutes ontopic0+topicCount+ address and never re-applies the topic filter), soContractA's handler received logs itswherefilter should have excluded. It only affected literal-value filters — address-param filters bound tochain.Contract.addressesare re-checked byclientAddressFilter.The fix
Group address-bound events by contract and scope each contract's selections to its own addresses (
addressesByContractName->Dict.get(contractName)), mirroring howHyperSyncSourcealready groups per contract. NowContractB's query only coversContractB's addresses and can't reachContractA's logs.Pure-wildcard events (
dependsOnAddresses == false) carry no address constraint, so they stay pooled and are resolved once — the wildcard partition still fans out to a single request.Trade-off: distinct contracts sharing an event signature now issue one
eth_getLogseach instead of a single pooled request. That's inherent toeth_getLogstaking one address list per call; a single contract (even a factory with many addresses) is still one request, and per-contractcompressTopicSelectionsstill folds its filter-less events into one topic0 OR-set.Test
scenarios/test_codegen/test/RpcSource_test.res— an e2e regression test throughgetItemsOrThrowwith aMockRpcServerthat honors the query'saddressfilter.ContractAfilters ontopic1,ContractBis unfiltered and sharestopic0; the test assertsContractB's query no longer leaks aContractAlog pastContractA's filter. It started as ait_failsreproduction and is now a plain passing assertion. All existinggetSelectionConfigcases are unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
Summary by CodeRabbit