Json RPC method parameter validation #2097

asoto-iov · 2023-08-09T16:20:54Z

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

… blockHash, Tx Hash and Hex Index parameters and deserializers

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexIndexParam.java

rskj-core/src/test/java/co/rsk/rpc/netty/JsonRPCParamValidationTest.java

+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class JsonRPCParamValidationTest {


….. related methods having parameter validation

rskj-core/src/test/java/co/rsk/rpc/netty/JsonRPCParamValidationTest.java

+
+    @Test
+    void eth_getTransactionByHash_invalidHash_returnsError() throws Exception {
+        TransactionResultDTO resultDTO = mock(TransactionResultDTO.class);


rskj-core/src/main/java/co/rsk/util/HexUtils.java

+
+        String preResult = removeHexPrefix(param);
+
+        return Integer.parseInt(preResult, 16);


casiojapi · 2023-08-17T14:28:29Z

pipeline:run

…fier

Vovchyk · 2023-08-31T07:20:32Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/BlockRefParam.java

+        public BlockRefParam deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException {
+            BlockRef blockRef = mapper.readValue(jp, BlockRef.class);
+            return new BlockRefParam(blockRef);
+        }


here we'd need to support two cases and validate them:

value is a blockId, eg. number "0x123" or tag "latest" (String)

value is an JSON object with all supported fields, eg. blockNumber or requireCanonical (Map<String, String>)

…nd eth_getFilterLogs. Fixing failing test from previous commit

Adding new FilterResquest Parameter class for JsonPRC

asoto-iov · 2023-09-20T08:49:03Z

rskj-core/src/main/java/co/rsk/rpc/modules/eth/EthModuleWallet.java

 public interface EthModuleWallet {

    String[] accounts();

-    String sign(String addr, String data);
+    String sign(HexAddressParam addr, HexDataParam data);


About this change, I think we shouldn't add the specific JsonRPC parameters here, we could keep the string or directly update to the address and data classes.
The Json PRC parameter classes should be used only under the jsonRPC domain.

I understand, I will revert the changes for this one.

…ccount_method Check null value in newAccount method

Vovchyk · 2023-09-21T07:23:07Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexDataParam.java

+        try {
+            this.rawDataBytes = HexUtils.stringHexToByteArray(rawData);
+        } catch (Exception e) {
+            throw RskJsonRpcRequestException.invalidParamError("Invalid data format. " + e.getMessage());


I guess we should not expose an Exception's message to a user, I'd rather do it this way:

throw RskJsonRpcRequestException.invalidParamError("Invalid data format", e);

Messages are returned in JSON-RPC responses

Please also check other param classes, eg HashParam32, HexAddressParam etc

@Vovchyk @asoto-iov I overlooked that using the e object will still expose the exception message, so I updated these errors to "Invalid data format: invalid hex value". I think this also will keep it consistent with the rest of the code since we don't use the exception message for any of the other validation errors. Let me know what you think.

@rmoreliovlabs, you can still pass exception as the last parameter in the list, so it'd be printed in logs later - like in the example above

Vovchyk · 2023-09-21T07:28:47Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/BlockIdentifierParam.java

@@ -0,0 +1,51 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:01Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/BlockRefParam.java

@@ -0,0 +1,102 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:16Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/CallArgumentsParam.java

@@ -0,0 +1,127 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:26Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexAddressParam.java

@@ -0,0 +1,48 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:30Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexDataParam.java

@@ -0,0 +1,49 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:36Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexDurationParam.java

@@ -0,0 +1,41 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:43Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexKeyParam.java

@@ -0,0 +1,45 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:47Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexNumberParam.java

@@ -0,0 +1,50 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

Vovchyk · 2023-09-21T07:31:50Z

rskj-core/src/main/java/org/ethereum/rpc/parameters/HexStringParam.java

@@ -0,0 +1,16 @@
+package org.ethereum.rpc.parameters;


(C) header is missing

I've added the (C) header to all new files that were missing it, thanks for catching that.

rmoreliovlabs · 2023-09-21T14:26:42Z

pipeline:run

…equest_parameter Adding support for null topics

…dation

rskj-core/src/test/java/org/ethereum/rpc/parameters/FilterRequestParamTest.java

+                "            \"topics\":[\"0x000000000000000000000000000000006d696e696e675f6665655f746f706963\", null, [\"0x000000000000000000000000000000006d696e696e675f6665655f746f706963\",null]]}";
+        JsonNode jsonNode = objectMapper.readTree(filterRequestInput);
+        FilterRequestParam filterRequestParam = objectMapper.convertValue(jsonNode, FilterRequestParam.class);
+        FilterRequest fr = objectMapper.convertValue(jsonNode, FilterRequest.class);


Added proper handling of number parsing

Vovchyk · 2023-10-12T14:20:44Z

pipeline:run

sonarcloud · 2023-10-12T14:26:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
59 Code Smells

82.3% Coverage
0.0% Duplication

Updating BlockHash related methods having validated parameters. Added…

af96058

… blockHash, Tx Hash and Hex Index parameters and deserializers

asoto-iov requested review from Vovchyk, casiojapi, rmoreliovlabs and illuque-iov August 9, 2023 16:20

github-advanced-security bot found potential problems Aug 9, 2023

View reviewed changes

Updated deserializers parent class and updated new eth_getTransaction…

22cc178

….. related methods having parameter validation

github-advanced-security bot found potential problems Aug 10, 2023

View reviewed changes

asoto-iov marked this pull request as ready for review August 10, 2023 15:29

casiojapi added 2 commits August 15, 2023 20:53

fix hex input validation

f1baf19

fix unit test exception message

d45a14f

github-advanced-security bot found potential problems Aug 16, 2023

View reviewed changes

rskj-core/src/main/java/co/rsk/util/HexUtils.java

String preResult = removeHexPrefix(param);

return Integer.parseInt(preResult, 16);

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException Note

Potential uncaught 'java.lang.NumberFormatException'.

casiojapi added 2 commits August 16, 2023 12:28

fix sonarqube warning

1722017

add serialVersionUID to deserializer

50df9f9

rmoreliovlabs added 4 commits August 26, 2023 09:48

Add deserializers for CallArguments, RawData, Address and BlockIdenti…

bc6baa5

…fier

Fix sonarcloud issues

ba11d0f

Refactor deserializers

e9f27a0

Add BlockRef and BlockRefParam classes

9e04a89

Vovchyk requested changes Aug 31, 2023

View reviewed changes

rmoreliovlabs and others added 3 commits August 31, 2023 09:14

Refactor BlockRefParam

63e7dd6

Adding new Parameter classes

85c2fe0

Refactor BlockRefParam deserialize to check for the param's type

acbde9e

rmoreliovlabs mentioned this pull request Aug 31, 2023

Implement json param validation #2118

Closed

8 tasks

asoto-iov and others added 6 commits August 31, 2023 19:17

Updating eth_newFilter method with FilterRequestParam

8c5affe

Updating eth_getLogs method with FilterRequestParam

27cc17d

Adding param validation to eth_unistallFilter, eth_getFilterChanges a…

7a9a5d4

…nd eth_getFilterLogs. Fixing failing test from previous commit

Merge pull request #2117 from rsksmart/json_rpc_param_validation_pt1

95d61d6

Adding new FilterResquest Parameter class for JsonPRC

Refactor CallArgumentsParam and add HexNumberParam

eda346b

Fix SonarCloud issues

5c00e1e

rmoreliovlabs previously approved these changes Sep 19, 2023

View reviewed changes

asoto-iov commented Sep 20, 2023

View reviewed changes

asoto-iov and others added 2 commits September 20, 2023 12:48

Check null value in newAccount method

bdc6dd1

Merge pull request #2136 from rsksmart/check_null_parameter_for_new_a…

82079f9

…ccount_method Check null value in newAccount method

Vovchyk dismissed rmoreliovlabs’s stale review via 82079f9 September 20, 2023 12:17

rmoreliovlabs added 2 commits September 20, 2023 08:56

Revert changes on EthModuleWallet sign method

aabccdd

Remove unused imports

61c0687

Vovchyk requested changes Sep 21, 2023

View reviewed changes

rmoreliovlabs added 3 commits September 21, 2023 08:23

Add (C) header to new files

0dd8a3e

Update invalid hex error messages

5ca790a

Send error object to invalidParamError for logging

8d27c79

asoto-iov and others added 4 commits September 21, 2023 18:57

Adding support for null topics

24fe940

minor code improvement

73909be

Merge pull request #2139 from rsksmart/support_null_topics_in_FilterR…

f05a209

…equest_parameter Adding support for null topics

Merge remote-tracking branch 'origin/master' into json_rpc_param_vali…

66549ed

…dation

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

Fixed tests after the merge from master

55b42e8

Vovchyk force-pushed the json_rpc_param_validation branch from 2ad2c49 to 55b42e8 Compare September 22, 2023 11:17

Vovchyk added 3 commits September 22, 2023 15:06

Added proper handling of number parsing

3b3bcc6

Added check for empty string in HexIndexParamTest

5d0a327

Merge pull request #2140 from rsksmart/hex-num-parsing

4e64426

Added proper handling of number parsing

Vovchyk approved these changes Sep 22, 2023

View reviewed changes

Vovchyk added 2 commits October 3, 2023 16:35

Merge branch 'master' into json_rpc_param_validation

59173f6

Merge branch 'master' into json_rpc_param_validation

bdc22a1

Vovchyk merged commit 5c5fe3d into master Oct 12, 2023
11 checks passed

Vovchyk deleted the json_rpc_param_validation branch October 12, 2023 15:38

aeidelman added this to the Fingerroot 5.4.0 milestone Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Json RPC method parameter validation #2097

Json RPC method parameter validation #2097

asoto-iov commented Aug 9, 2023

casiojapi commented Aug 17, 2023

Vovchyk Aug 31, 2023

asoto-iov Sep 20, 2023

rmoreliovlabs Sep 20, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

rmoreliovlabs Sep 21, 2023

rmoreliovlabs Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

Vovchyk Sep 21, 2023

rmoreliovlabs Sep 21, 2023

rmoreliovlabs commented Sep 21, 2023

Vovchyk commented Oct 12, 2023

sonarcloud bot commented Oct 12, 2023


		String preResult = removeHexPrefix(param);

		return Integer.parseInt(preResult, 16);

Json RPC method parameter validation #2097

Json RPC method parameter validation #2097

Conversation

asoto-iov commented Aug 9, 2023

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

casiojapi commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmoreliovlabs commented Sep 21, 2023

Vovchyk commented Oct 12, 2023

sonarcloud bot commented Oct 12, 2023