bitcoin-cli: Eliminate "Error parsing JSON" errors

ryanofsky commented at 3:17 pm on February 20, 2019: contributor

Just pass arguments that can’t be parsed as JSON as string arguments. If the RPC method isn’t expecting a string argument, this will result in more specific errors from the RPC. And if it allows an argument to be a string as one possible type (for example the hash_or_height argument to getblockstats), this avoids the need for bitcoin-cli users to use double JSON quoting+shell quoting to pass the string.

Just pass arguments that can't be parsed as JSON as string arguments. If the
RPC method isn't expecting a string argument, this will result in more specific
errors from the RPC. And if it allows an argument to be a string as one
possible type (for example the `hash_or_height` argument to `getblockstats`),
this avoids the need for bitcoin-cli users to use double JSON quoting+shell
quoting to pass the string.

45c7439fed

promag commented at 3:49 pm on February 20, 2019: member

I think this approach is fine, unless there is an use case where the end result would be really bad - which I don’t believe there is.

Concept ACK from me.

maflcko commented at 4:03 pm on February 20, 2019: member

If this is conceptually ACKed, the now unused ParseNonRFCJSONValue should be removed

in src/rpc/client.cpp:223 in 45c7439fed

218@@ -219,12 +219,13 @@ UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::s
219     for (unsigned int idx = 0; idx < strParams.size(); idx++) {
220         const std::string& strVal = strParams[idx];
221 
222-        if (!rpcCvtTable.convert(strMethod, idx)) {
223+        UniValue json_value;
224+        if (!rpcCvtTable.convert(strMethod, idx) || !json_value.read(strVal)) {

maflcko commented at 4:39 pm on February 20, 2019:

Seems odd to have two inversions in a condition. Would be easier to write

0if convert() and read():
1 # push back parsed value
2else:
3 # push back str

promag commented at 2:28 pm on June 25, 2019:

Keep diff small? 👼 But agree.

maflcko approved

maflcko commented at 4:46 pm on February 20, 2019: member

Tested ACK 45c743

This is effectively moving the type check from the client to the server (unparsable json will get send as string). I guess this is fine

 0# type check is moved to server
 1$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest getrawtransaction ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff True ffff  ; done
 2error: Error parsing JSON:True
 3
 4error code: -1
 5error message:
 6JSON value is not a boolean as expected
 7
 8
 9# test conversion
10$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echojson true 1 '"str"' '[1,2]' '{"a":1}'  ; done
11[
12  true,
13  1,
14  "str",
15  [
16    1,
17    2
18  ],
19  {
20    "a": 1
21  }
22]
23[
24  true,
25  1,
26  "str",
27  [
28    1,
29    2
30  ],
31  {
32    "a": 1
33  }
34]
35
36# test no conversion
37$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echo true 1 '"str"' '[1,2]' '{"a":1}'  ; done
38[
39  "true",
40  "1",
41  "\"str\"",
42  "[1,2]",
43  "{\"a\":1}"
44]
45[
46  "true",
47  "1",
48  "\"str\"",
49  "[1,2]",
50  "{\"a\":1}"
51]
52
53# test conversion with invalid json
54$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echojson True  ; done
55error: Error parsing JSON:True
56[
57  "True"
58]

jgarzik commented at 5:25 pm on February 20, 2019: contributor

This is effectively moving the type check from the client to the server (unparsable json will get send as string)

Agree. This matches a common frontend validation pattern:

The server must validate and reject garbage. (1) Thus one option is to lean on the remote server to perform that validation.

However, it is often better UX – and potentially saves users $$ in bandwidth, and saves server CPU cycles – to (2) avoid sending data the client can prove, via its own validation, is known garbage data.

Yes, this is double validation; but the 90+% common case is rejected on the client side, remaining efficient by sending no garbage over the network.

ryanofsky commented at 6:22 pm on February 20, 2019: contributor

Will wait for concept acks & nacks before working more on this. To summarize comments so far:

Advantages of this change

More usable bitcoin-cli getblockstats command, no longer requiring double json+shell quoting. Other rpc methods could benefit similarly since nothing about this change is specific to getblockstats.
Potentially improved error reporting. Server side error reporting can be more customized and provide context.
Simplified bitcoin-cli logic. Gets rid of thrown exception.

Disadvantages of this change

Increased latency and bandwidth (in error case only) since errors now handled server side.
Potentially worse error reporting in the future. bitcoin-cli error reporting is bad now, but if it’s removed entirely, there’s loss of opportunity to improve it in the future.

Potential followups

#15448 (review): logic should be inverted for clarity
#15448 (comment): unused ParseNonRFCJSONValue function should be removed
Should have python test for new bitcoin-cli behavior.

maflcko commented at 6:38 pm on February 20, 2019: member

I don’t think we should be optimizing for bandwidth in the error case, which can only happen when a developer debugs locally.

fanquake added the label RPC/REST/ZMQ on Feb 20, 2019

practicalswift commented at 11:49 pm on February 20, 2019: contributor

Concept ACK

In this case I think we should optimise for user-friendliness.

jonasschnelli commented at 5:48 am on February 21, 2019: contributor

Concept ACK

laanwj commented at 7:58 am on February 21, 2019: member

I’m kind of skeptical about this, to be honest. I prefer strong validation both client and server side, from my experience, lazy “anything goes” validation (in this case “let the server sort them out”) has resulted in quite some unexpected disasters. At the least it makes it harder to reason about what things will do, in advance.

Empact commented at 3:31 pm on March 7, 2019: member

Looks like this removed all remaining calls to ParseNonRFCJSONValue, so it can be removed.

in src/rpc/client.cpp:249 in 45c7439fed

244@@ -244,12 +245,13 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
245         std::string name = s.substr(0, pos);
246         std::string value = s.substr(pos+1);
247 
248-        if (!rpcCvtTable.convert(strMethod, name)) {
249+        UniValue json_value;
250+        if (!rpcCvtTable.convert(strMethod, name) || !json_value.read(value)) {

promag commented at 2:31 pm on June 25, 2019:

Same as above.

promag commented at 2:49 pm on June 25, 2019: member

ACK 45c7439fed93dc31c7bc79906ae6c2e9bf062f8c, above comment #15448 (comment) looks good.

Should have python test for new bitcoin-cli behavior.

👍

has resulted in quite some unexpected disasters. @laanwj like what? Preventing disasters and client validation sound like 2 different things and pretty much unrelated. But better safe than sorry? No strong opinion.

laanwj commented at 3:24 pm on June 25, 2019: member

@laanwj like what? Preventing disasters and client validation sound like 2 different things and pretty much unrelated. But better safe than sorry? No strong opinion.

Defense in depth, belt and suspenders, etc.

I mean the underlying issue is that there is an ambiguity, and resolving this ambiguity is here moved from the client to the server, which isn’t always better. It’s better if both the client and server know what the interpretation of some value is. SQL injection attacks are a well-known problem that is caused by trusting that the server will the right thing when you just pass the value as-is.

DrahtBot closed this on Mar 9, 2020

DrahtBot reopened this on Mar 9, 2020

maflcko referenced this in commit 5e5dd9918e on Apr 20, 2020

ftrader referenced this in commit b7c02a6617 on Aug 17, 2020

promag commented at 8:53 am on September 21, 2020: member

@laanwj this just pretends the client typed "" - it fallbacks the argument to string which seems natural considering this is a command line tool. @ryanofsky just wrote 4c19cf574e15b1bf0bd6142dfa55c2a0f637209e but then I’ve realised it’s pretty much the same as this.

DrahtBot commented at 4:33 am on September 22, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19949 (cli: Parse and allow hash value by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 12:20 pm on November 23, 2020: member

@laanwj this just pretends the client typed "" - it fallbacks the argument to string which seems natural considering this is a command line tool.

That’s my problem with it “pretends the client typed”. If it is purely a user-facing tool like a GUI it might be different (still, it’s often better not to guess at intent). The problem is that tools like this are also used in scripts and such, and they might deal with (partially) untrusted input. I think determinism and predictability is very important in input processing.

PastaPastaPasta referenced this in commit 7f631dbb14 on Dec 22, 2021

PastaPastaPasta referenced this in commit ce24ca5794 on Dec 22, 2021

PastaPastaPasta referenced this in commit c65f526ac1 on Dec 22, 2021

PastaPastaPasta referenced this in commit affb7a472f on Dec 28, 2021

kallewoof commented at 10:52 pm on January 31, 2022: member

Concept ACK

achow101 commented at 5:58 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

bitcoin deleted a comment on Oct 13, 2022

bitcoin locked this on Oct 13, 2023

bitcoin-cli: Eliminate “Error parsing JSON” errors #15448

Conflicts