Refactor sanitization middleware

[BelfordZ]

Development & PR Process

1. Follow MetaMask Mobile Coding Standards
2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
3. Add needs-dev-review label when work is completed
4. Add needs-qa label when dev review is completed
5. Add QA Passed label when QA has signed off

Description

Add sanitiziation RPC middleware to process requests just before they get sent into our internal network provider. This is meant to be a refactor with no functional changes; this middleware is identical to the middleware already used today within web3-provider-engine.

This is being added so that we can remove web3-provider-engine with fewer functional changes.

Test Instructions

This should have no functional impact. This middleware is expected to process any RPC request not intercepted earlier in the pipeline however (essentially any request that gets sent to the RPC endpoint). It should ignore most requests (as the unit tests demonstrate), but it will process any that have an object as the first parameter. Known examples are eth_call, eth_estimateGas, and eth_getLogs.

To manually test this, you could try invoking these methods with parameters that the middleware should be expected to filter out. Or invoke them with parameters that are allowed, to confirm they remain unaffected. You could also try testing requests that we anticipate are unaffected. In all cases, we should see the same behavior before and after.

The test dapp has a new "Request" page that might be useful in testing this. See the README Usage section for details: https://github.com/MetaMask/test-dapp

Issue

Closes #5846

Checklist

• There is a related GitHub issue
• Tests are included if applicable
• Any added code is fully documented

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[socket-security]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

---

👍 No dependency changes detected in pull request

Pull request alert summary

Issue	Status
Critical CVE	✅ 0 issues
CVE	✅ 0 issues
Mild CVE	✅ 0 issues
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script confusion	✅ 0 issues
Bin script shell injection	✅ 0 issues
Filesystem access	✅ 0 issues
Network access	✅ 0 issues
Shell access	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
GitHub dependency	✅ 0 issues
No bug tracker	✅ 0 issues
No contributors or author data	✅ 0 issues
No README	✅ 0 issues
Deprecated	✅ 0 issues
New author	✅ 0 issues
Unstable ownership	✅ 0 issues
Non-existent author	✅ 0 issues
Unmaintained	✅ 0 issues
Unpublished package	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues
AI detected security risk	✅ 0 issues
AI warning	✅ 0 issues

[Gudahtt]

This generally looks correct to me! Both the functionality and the way the middleware was integrated. I left a bunch of suggestions on improving the types.

The main question I have left is about whether we should restrict this just to eth_call. From reading the code, it still seems like any requests bound for the network (e.g. anything not intercepted by the RPCMethodMiddleware module) would be processed by this middleware.

I'll keep looking into that, but if you could share your method of testing that, that might be helpful. I recall you saying that you did some testing already.

[Gudahtt]

I am investigating this:

The main question I have left is about whether we should restrict this just to eth_call. From reading the code, it still seems like any requests bound for the network (e.g. anything not intercepted by the RPCMethodMiddleware module) would be processed by this middleware.

Once that question is answered, this will be ready for review

Edit: Also would be nice to add JSDocs

[Gudahtt]

I have reviewed all supported RPC methods to see which might be affected by the santization middleware today in web3-provider-engine. I can't anticipate all hypothetical RPC methods supported by custom networks, so I restricted my search just to Infura-supported methods.

This middleware only affects requests with an object as the first parameter. The Infura methods with an object as the first parameter are:

• eth_call
• eth_estimateGas
• eth_getLogs
• eth_newFilter

eth_newFilter gets intercepted by mobile's RPC middleware, so the first three are all that we need to consider.

[Gudahtt]

The only first-parameter property for eth_getLogs getting filtered out is blockhash. I have confirmed that eth_getLogs works on both extension and mobile, but on mobile the blockhash parameter is filtered out.

For eth_call and eth_estimateGas, the only properties getting filtered out are maxPriorityFeePerGas and maxFeePerGas.

It seems that the santization middleware was written to account for all known Infura/Ethereum RPC methods at the time, but doesn't count for parameters added in later EIPs.

This seems to confirm that this affects all methods that reach the network middleware, not just eth_call

[Gudahtt]

I have marked this as "Spot Check on the Release Build" because this PR is anticipated to have no functional change, but the affected functionality isn't covered by any e2e tests, and adding e2e coverage would be challenging and potentially unsuited for the e2e test suite anyway.

[mcmire]

@Gudahtt I've double-checked your work against the Infura docs, and you are correct about eth_call, eth_estimateGas, eth_getLogs, and eth_getFilterLogs.

I also looked at the Ethereum execution API docs, and it appears that some new RPC methods have been added since I last looked. These are the ones affected, with the parameters that would be excluded (skipping eth_sendTransaction since I think we override that one?):

• eth_createAccessList
  • type
  • input
  • maxPriorityFeePerGas
  • maxFeePerGas
  • accessList
  • chainId
• engine_forkchoiceUpdatedV1
  • headBlockHash
  • safeBlockHash
  • finalizedBlockHash
• engine_forkchoiceUpdatedV2
  • headBlockHash
  • safeBlockHash
  • finalizedBlockHash
• engine_newPayloadV1
  • parentHash
  • feeRecipient
  • stateRoot
  • receiptsRoot
  • logsBloom
  • prevRandao
  • blockNumber
  • gasLimit
  • gasUsed
  • timestamp
  • extraData
  • baseFeePerGas
  • blockHash
  • transactions
• engine_newPayloadV2
  • parentHash
  • feeRecipient
  • stateRoot
  • receiptsRoot
  • logsBloom
  • prevRandao
  • blockNumber
  • gasLimit
  • gasUsed
  • timestamp
  • extraData
  • baseFeePerGas
  • blockHash
  • transactions
  • withdrawals
• engine_exchangeTransitionConfigurationV1
  • terminalTotalDifficulty
  • terminalBlockHash
  • terminalBlockNumber

Not sure it is worth considering these, but I've noted them for completeness.

[Gudahtt]

@mcmire Thanks for the review! All comments have been addressed.

Good find re: those new methods too. I've noted them in a comment in 64485a3

[sethkfman]

LGTM. Just one clarifying question.

[cortisiko]

I tested the test dapp as well as other dapp interactions. Nothing to report here. ✅