fix(connector-quorum/ethereum): strengthen contract parameter validation #2854

shivam-Purohit · 2023-10-31T05:57:48Z

Peter's updates:

Made improvements to the test case verifying that the parameters with
incorrect types are indeed being rejected with useful error messaging
Added a new library (which I also had to re-publish with CJS exports)

Fixes #2760

Signed-off-by: Shivam Purohit [email protected]
Signed-off-by: Peter Somogyvari [email protected]

Pull Request Requirements

Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

shivam-Purohit · 2023-10-31T06:14:04Z

@petermetz I was not able to test the changes. I was following these two documentation
cacti/contribute
cacti/build for compiling and testing the packages. I complied the packages with no error but still the test test:all was failing. Also it was taking a ton of time for testing. I was looking to test for the package cactus-plugin-ledger-connector-quorum but that test was also failing without any changes made in the code.
Some part of the documentation is not working or lagging behind the current project , the test for integration or unit was not present.

How should I test these changes I made?

This is the steps i followed for compiling packages

installed OpenJDK sudo apt install openjdk-8-jdk-headless
nvm use 16.14.2
npm run configure

Regarding the code, the arguments are passed through the request body as it retrieved in the invokeRawWeb3EthContract
method. I also looked at the test for the method to get the idea of the total params send to the method. The methodName and MethodArgs are provided through the request which can also be obtained from the abi for cross validation. Is the approach right?, i think same will be applicable for the other endpoints

petermetz · 2023-11-06T22:37:30Z

@petermetz I was not able to test the changes. I was following these two documentation cacti/contribute cacti/build for compiling and testing the packages. I complied the packages with no error but still the test test:all was failing. Also it was taking a ton of time for testing. I was looking to test for the package cactus-plugin-ledger-connector-quorum but that test was also failing without any changes made in the code. Some part of the documentation is not working or lagging behind the current project , the test for integration or unit was not present.

How should I test these changes I made?

This is the steps i followed for compiling packages
1. installed OpenJDK  `sudo apt install openjdk-8-jdk-headless`

2. nvm use 16.14.2

3. npm run configure
Regarding the code, the arguments are passed through the request body as it retrieved in the invokeRawWeb3EthContract method. I also looked at the test for the method to get the idea of the total params send to the method. The methodName and MethodArgs are provided through the request which can also be obtained from the abi for cross validation. Is the approach right?, i think same will be applicable for the other endpoints

@shivam-Purohit Thank you very much for taking the time to read the documentation in detail!
You can verify that it is working with this test case. You might have to add additional assertions or edge cases to try out the specific code-paths you implemented but this is the test file that would make all of that the easiest: packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/v21.4.1-invoke-web3-contract-v1.test.ts

You can run that test file by doing yarn jest packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/v21.4.1-invoke-web3-contract-v1.test.ts

Now some pointers on how to get this done:

I sent a suggestion (see comment above with the diff).
Before you even look at the suggestion, what I would do is rebase your branch onto upstream/main because the build right now is broken on the old code)
Then you can apply the suggestions I made one by one. Here are some more details explaining the suggestions themselves:
3.1. Add the ethers package in the specific pakcage.json of the connector plugin not in the root package.json (this is a mono repo so there are dozens of packages in it and the root is only there to hold development tooling/ dev dependencies for the build itself)
3.2. Update the import of the ethers package so that it's import type { ... } instead of import { ... } because this way we don't need to actually have ethers as a dependency in the package.json, it is enough to have it as a devDependency which is better because it reduces the production code's footprint (bundle/install size in general). There is much more/better explanation provided on this link to the why: https://stackoverflow.com/a/64243357
3.3. Apply the automatic formatter (prettier/eslint)
With doing all the above you should arrive at a point where the PR is close to being done, but then I'll also ask that you update the ethereum connector plugin as well (exact same changes for the most part), which can be tested by executing this other test file: packages/cactus-plugin-ledger-connector-ethereum/src/test/typescript/integration/geth-invoke-web3-contract-v1.test.ts

shivam-Purohit · 2023-11-07T13:08:42Z

I sent a suggestion (see comment above with the diff).

Before you even look at the suggestion, what I would do is rebase your branch onto upstream/main because the build right now is broken on the old code)

Then you can apply the suggestions I made one by one. Here are some more details explaining the suggestions themselves:
3.1. Add the ethers package in the specific pakcage.json of the connector plugin not in the root package.json (this is a mono repo so there are dozens of packages in it and the root is only there to hold development tooling/ dev dependencies for the build itself)
3.2. Update the import of the ethers package so that it's import type { ... } instead of import { ... } because this way we don't need to actually have ethers as a dependency in the package.json, it is enough to have it as a devDependency which is better because it reduces the production code's footprint (bundle/install size in general). There is much more/better explanation provided on this link to the why: https://stackoverflow.com/a/64243357
3.3. Apply the automatic formatter (prettier/eslint)

With doing all the above you should arrive at a point where the PR is close to being done, but then I'll also ask that you update the ethereum connector plugin as well (exact same changes for the most part), which can be tested by executing this other test file: packages/cactus-plugin-ledger-connector-ethereum/src/test/typescript/integration/geth-invoke-web3-contract-v1.test.ts

Thanks @petermetz for that detailed explanation. Sorry though I messed up with the other branch in merge conflicts. So I created new and forced push. I did the following things like you suggested

rebased onto remote/main
added ethers dev dependencies for the cactus-plugin-ledger-connector-ethereum and cactus-plugin-ledger-connector-quorum packages. I added using --dev flag so its added in devDependencies
used typed import
tested both the files.
ran lint and prettier

A CI test is failing though and I don't know the reason why. Am I missing something?

petermetz

I sent a suggestion (see comment above with the diff).

Before you even look at the suggestion, what I would do is rebase your branch onto upstream/main because the build right now is broken on the old code)

Then you can apply the suggestions I made one by one. Here are some more details explaining the suggestions themselves:
3.1. Add the ethers package in the specific pakcage.json of the connector plugin not in the root package.json (this is a mono repo so there are dozens of packages in it and the root is only there to hold development tooling/ dev dependencies for the build itself)
3.2. Update the import of the ethers package so that it's import type { ... } instead of import { ... } because this way we don't need to actually have ethers as a dependency in the package.json, it is enough to have it as a devDependency which is better because it reduces the production code's footprint (bundle/install size in general). There is much more/better explanation provided on this link to the why: https://stackoverflow.com/a/64243357
3.3. Apply the automatic formatter (prettier/eslint)

With doing all the above you should arrive at a point where the PR is close to being done, but then I'll also ask that you update the ethereum connector plugin as well (exact same changes for the most part), which can be tested by executing this other test file: packages/cactus-plugin-ledger-connector-ethereum/src/test/typescript/integration/geth-invoke-web3-contract-v1.test.ts

Thanks @petermetz for that detailed explanation. Sorry though I messed up with the other branch in merge conflicts. So I created new and forced push. I did the following things like you suggested
1. rebased onto remote/main

2. added ethers dev dependencies for the `cactus-plugin-ledger-connector-ethereum` and `cactus-plugin-ledger-connector-quorum ` packages. I added using `--dev `flag so its added in devDependencies

3. used typed import

4. tested both the files.

5. ran lint and prettier
A CI test is failing though and I don't know the reason why. Am I missing something?

@shivam-Purohit Sorry for the slow response! Some of the tests are unfortunately in a failing state currently as we are working our way through clearing the backlog for the v2.0.0 release. Please let me know which specific tests you are talking about and also you can compare the main branches CI execution results to the ones on your PR and see if they were failing to begin with on the main branch (in which case you can know for sure that you didn't break the those specific tests and then it should be OK)

If it turns out that you did break some test cases then I need to know exactly which ones and then I can give more targeted help.

In the meantime, please make sure not to delete your branch and not to close the PR because we already have the review history here so new branches won't work. If you struggle with the git operations to get all this done let me know and I can help with that too.

Right now it is looking like you still need to squash 4 different commits into one but that can be done later too if you prefer to do it that way (everybody's git workflows are slightly different)

petermetz

@shivam-Purohit Please also make sure not to use any auto-upgrade syntax when specifying npm dependency versions. It is a security risk (software supply chain attacks)

...nnector-ethereum/src/main/typescript/web-services/invoke-raw-web3eth-contract-v1-endpoint.ts

shivam-Purohit · 2023-12-07T04:15:05Z

@shivam-Purohit Sorry for the slow response! Some of the tests are unfortunately in a failing state currently as we are working our way through clearing the backlog for the v2.0.0 release. Please let me know which specific tests you are talking about and also you can compare the main branches CI execution results to the ones on your PR and see if they were failing to begin with on the main branch (in which case you can know for sure that you didn't break the those specific tests and then it should be OK)

If it turns out that you did break some test cases then I need to know exactly which ones and then I can give more targeted help.

In the meantime, please make sure not to delete your branch and not to close the PR because we already have the review history here so new branches won't work. If you struggle with the git operations to get all this done let me know and I can help with that too.

Right now it is looking like you still need to squash 4 different commits into one but that can be done later too if you prefer to do it that way (everybody's git workflows are slightly different)

sure I was also busy with my college exams. I will check with the tests thing you mentioned and I will squash the commits when we are done with the PR.

shivam-Purohit · 2023-12-07T04:15:27Z

@shivam-Purohit Please also make sure not to use any auto-upgrade syntax when specifying npm dependency versions. It is a security risk (software supply chain attacks)

noted!

shivam-Purohit · 2023-12-08T13:08:00Z

hey @petermetz @outSH
I am a bit confused since I moved my code to the invokeRawWeb3EthContract of the ethereum connector. We want to validate the invocationParams or the contractMethodArgs ?. We are using abi to get the inputs name and type and validate it against that.
either way when i try to test the changes it fails as the mock values we are using in invocationParams or contractMethodArgs not having the same inputs as abi.
this is the mock arguments we are sending to the invokeRawWeb3EthContract

{
  abi: [
    {
      inputs: [],
      name: 'getName',
      outputs: [Array],
      stateMutability: 'view',
      type: 'function'
    },
    {
      inputs: [],
      name: 'sayHello',
      outputs: [Array],
      stateMutability: 'pure',
      type: 'function'
    },
    {
     inputs: [Array],
      name: 'setName',
     outputs: [],
      stateMutability: 'nonpayable',
      type: 'function'
    }
  ],
  address: '0xa12bde8577fb58e4453f8dab09e47144950ec840',
  invocationType: 'send',
  invocationParams: { from: '0x6a2ec8c50ba1a9ce47c52d1cb5b7136ee9d0ccc0' },
  contractMethod: 'setName',
  contractMethodArgs: [ 'EthereumCactus' ]
}

I am wrong somewhere or we need to modify the tests or add new for checking this.

petermetz · 2023-12-11T21:13:48Z

hey @petermetz @outSH I am a bit confused since I moved my code to the invokeRawWeb3EthContract of the ethereum connector. We want to validate the invocationParams or the contractMethodArgs ?. We are using abi to get the inputs name and type and validate it against that. either way when i try to test the changes it fails as the mock values we are using in invocationParams or contractMethodArgs not having the same inputs as abi. this is the mock arguments we are sending to the invokeRawWeb3EthContract
{
  abi: [
    {
      inputs: [],
      name: 'getName',
      outputs: [Array],
      stateMutability: 'view',
      type: 'function'
    },
    {
      inputs: [],
      name: 'sayHello',
      outputs: [Array],
      stateMutability: 'pure',
      type: 'function'
    },
    {
     inputs: [Array],
      name: 'setName',
     outputs: [],
      stateMutability: 'nonpayable',
      type: 'function'
    }
  ],
  address: '0xa12bde8577fb58e4453f8dab09e47144950ec840',
  invocationType: 'send',
  invocationParams: { from: '0x6a2ec8c50ba1a9ce47c52d1cb5b7136ee9d0ccc0' },
  contractMethod: 'setName',
  contractMethodArgs: [ 'EthereumCactus' ]
}
I am wrong somewhere or we need to modify the tests or add new for checking this.

@shivam-Purohit Thank you for working on this and also for being receptive to feedback/change requests!
The specific issue at hand is aboutinvocationParams only, not contractMethodArgs.

For the failing tests: Please let make it so that we can reproduce the test case failure locally on our own machines as well and then we can offer more targeted help. E.g., push your latest changes to your branch, send us a link of that branch and then the instructions of how were you exactly running the test case to get what exact log output.

shivam-Purohit · 2023-12-15T14:38:31Z

@petermetz I think I got what the problem was. From abi we suppose we get the inputs and types for the contractMethodArgs from where we can validate it using functionFragments
but for the invocationParams we are not getting any of the information ( i suppose). So I am manually added some type checks for invocationParams.
I have build my alleged hypothesis based on the mocked tests we are using and the inputs we are getting. Correct me if I am wrong here.

outSH · 2023-12-28T12:13:59Z

@shivam-Purohit Do you have any blocking issues / more questions to be addressed regarding this PR?

shivam-Purohit · 2023-12-29T05:34:23Z

@shivam-Purohit Do you have any blocking issues / more questions to be addressed regarding this PR?

currently one workflow is failing docs/readthedocs.org:hyperledger-cactus. I tested things on my side with the integration tests. I wanted to make sure I do not add any breaking changes.

Other than that I don't think I have any other questions.

petermetz

@shivam-Purohit I've added a test case to verify that the validation works as expected and also did some refactoring on the error handling itself so that it isn't a security issue anymore (it was reporting HTTP 500 statuses for user errors which should respond with HTTP 4xx)

With all that and with your improvements together: LGTM

.../cactus-plugin-ledger-connector-quorum/src/main/typescript/plugin-ledger-connector-quorum.ts

-      args.invocationType
-    ](args.invocationParams);
+    const txObjectFactory = contract.methods[args.contractMethod];
+    const txObject = txObjectFactory(...contractMethodArgs);


shivam-Purohit · 2024-01-04T05:17:05Z

@shivam-Purohit I've added a test case to verify that the validation works as expected and also did some refactoring on the error handling itself so that it isn't a security issue anymore (it was reporting HTTP 500 statuses for user errors which should respond with HTTP 4xx)

With all that and with your improvements together: LGTM

Thanks a lot. Hope this works!

petermetz · 2024-01-24T03:07:13Z

@shivam-Purohit Have you hit the 're-request review' button to ping @outSH to check on this again? If not, I recommend doing it so that he has a chance to take another look at the chances you've made.

outSH · 2024-01-24T12:36:09Z

@shivam-Purohit Please rebase with main and re-request the review as Peter pointed out, and we can merge it right away. Everything looks good, thank you very much for this contribution! :)

shivam-Purohit · 2024-01-24T14:15:33Z

@shivam-Purohit Have you hit the 're-request review' button to ping @outSH to check on this again? If not, I recommend doing it so that he has a chance to take another look at the chances you've made.

sorry It slipped my mind I thought the repo was busy.

shivam-Purohit · 2024-01-24T14:15:57Z

@shivam-Purohit Please rebase with main and re-request the review as Peter pointed out, and we can merge it right away. Everything looks good, thank you very much for this contribution! :)

sure!

petermetz · 2024-01-24T19:38:33Z

@shivam-Purohit FYI: That didn't work but I rebased it for you, no worries.
The trick I usually use to fix merge conflicts in lock files: I hit 'accept all current' in VSCode's conflict resolution panel on the lock file and then run yarn install while still in the paused state of the rebase. Once yarn updates the lock file then I add those changes to the git stage and then hit git rebase --continue

petermetz · 2024-01-24T19:40:27Z

@shivam-Purohit Have you hit the 're-request review' button to ping @outSH to check on this again? If not, I recommend doing it so that he has a chance to take another look at the chances you've made.

sorry It slipped my mind I thought the repo was busy.

@shivam-Purohit No worries! It gets complicated sometimes but we do want to make sure that your PR gets merged as much as all the other ones! I recommend hitting that re-request review button for @outSH once again to pass the proverbial ball back .

shivam-Purohit · 2024-01-25T12:24:46Z

The trick I usually use to fix merge conflicts in lock files: I hit 'accept all current' in VSCode's conflict resolution panel on the lock file and then run yarn install while still in the paused state of the rebase. Once yarn updates the lock file then I add those changes to the git stage and then hit git rebase --continue

Thats a very handy trick, that sure will be very helpful for me in future, thanks a lot!

shivam-Purohit · 2024-01-25T12:26:37Z

@petermetz Was the rebase successful, it says that this branch still has conflicts? should i try to rebase?

outSH · 2024-01-25T14:03:15Z

@petermetz Was the rebase successful, it says that this branch still has conflicts? should i try to rebase?

@shivam-Purohit
Yes, please rebase, I think more commits were merged into main and turned this one out of sync again :) After resolving conflicts in yarn.lock run yarn install to make sure that it's updated correctly

Peter's updates: 1. Made improvements to the test case verifying that the parameters with incorrect types are indeed being rejected with useful error messaging 2. Added a new library (which I also had to re-publish with CJS exports) Fixes hyperledger-cacti#2760 Signed-off-by: Shivam Purohit <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>

petermetz · 2024-01-25T18:11:54Z

@outSH @shivam-Purohit FYI I just rebased it!

This comment was marked as outdated.

Sign in to view

shivam-Purohit force-pushed the valid_params branch from 82d017d to b2c7d0d Compare November 7, 2023 08:09

shivam-Purohit marked this pull request as ready for review November 7, 2023 12:55

shivam-Purohit requested review from petermetz, takeutak, izuru0, jagpreetsinghsasan, VRamakrishna and sandeepnRES as code owners November 7, 2023 12:55

petermetz requested changes Dec 1, 2023

View reviewed changes

outSH requested changes Dec 4, 2023

View reviewed changes

...nnector-ethereum/src/main/typescript/web-services/invoke-raw-web3eth-contract-v1-endpoint.ts Outdated Show resolved Hide resolved

shivam-Purohit force-pushed the valid_params branch from f417cbb to 3c3a233 Compare December 15, 2023 14:18

shivam-Purohit force-pushed the valid_params branch from 3c3a233 to da7ef0a Compare December 29, 2023 06:08

petermetz force-pushed the valid_params branch from da7ef0a to 407bcf4 Compare January 3, 2024 00:11

petermetz self-requested a review January 3, 2024 00:11

petermetz approved these changes Jan 3, 2024

View reviewed changes

github-advanced-security bot found potential problems Jan 3, 2024

View reviewed changes

petermetz force-pushed the valid_params branch from 407bcf4 to 32e87d0 Compare January 24, 2024 19:36

petermetz enabled auto-merge (rebase) January 24, 2024 19:38

outSH approved these changes Jan 25, 2024

View reviewed changes

petermetz force-pushed the valid_params branch from 32e87d0 to f299b48 Compare January 25, 2024 18:11

petermetz merged commit 779bb7e into hyperledger-cacti:main Jan 25, 2024
128 of 146 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(connector-quorum/ethereum): strengthen contract parameter validation #2854

fix(connector-quorum/ethereum): strengthen contract parameter validation #2854

shivam-Purohit commented Oct 31, 2023 •

edited by petermetz

Loading

shivam-Purohit commented Oct 31, 2023

This comment was marked as outdated.

petermetz commented Nov 6, 2023

shivam-Purohit commented Nov 7, 2023 •

edited

Loading

petermetz left a comment

petermetz left a comment

shivam-Purohit commented Dec 7, 2023

shivam-Purohit commented Dec 7, 2023

shivam-Purohit commented Dec 8, 2023

petermetz commented Dec 11, 2023

shivam-Purohit commented Dec 15, 2023

outSH commented Dec 28, 2023

shivam-Purohit commented Dec 29, 2023

petermetz left a comment

shivam-Purohit commented Jan 4, 2024

petermetz commented Jan 24, 2024

outSH commented Jan 24, 2024

shivam-Purohit commented Jan 24, 2024

shivam-Purohit commented Jan 24, 2024

petermetz commented Jan 24, 2024

petermetz commented Jan 24, 2024

shivam-Purohit commented Jan 25, 2024

shivam-Purohit commented Jan 25, 2024

outSH commented Jan 25, 2024

petermetz commented Jan 25, 2024

fix(connector-quorum/ethereum): strengthen contract parameter validation #2854

fix(connector-quorum/ethereum): strengthen contract parameter validation #2854

Conversation

shivam-Purohit commented Oct 31, 2023 • edited by petermetz Loading

shivam-Purohit commented Oct 31, 2023

This comment was marked as outdated.

petermetz commented Nov 6, 2023

shivam-Purohit commented Nov 7, 2023 • edited Loading

petermetz left a comment

Choose a reason for hiding this comment

petermetz left a comment

Choose a reason for hiding this comment

shivam-Purohit commented Dec 7, 2023

shivam-Purohit commented Dec 7, 2023

shivam-Purohit commented Dec 8, 2023

petermetz commented Dec 11, 2023

shivam-Purohit commented Dec 15, 2023

outSH commented Dec 28, 2023

shivam-Purohit commented Dec 29, 2023

petermetz left a comment

Choose a reason for hiding this comment

shivam-Purohit commented Jan 4, 2024

petermetz commented Jan 24, 2024

outSH commented Jan 24, 2024

shivam-Purohit commented Jan 24, 2024

shivam-Purohit commented Jan 24, 2024

petermetz commented Jan 24, 2024

petermetz commented Jan 24, 2024

shivam-Purohit commented Jan 25, 2024

shivam-Purohit commented Jan 25, 2024

outSH commented Jan 25, 2024

petermetz commented Jan 25, 2024

shivam-Purohit commented Oct 31, 2023 •

edited by petermetz

Loading

shivam-Purohit commented Nov 7, 2023 •

edited

Loading