RPC: Tolerate unknown parameters, but with clear warning/errors #20548

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:soften_rpcauth changing 2 files +53 −7

luke-jr commented at 5:41 PM on December 2, 2020: member

By tolerating unknown extra rpcauth parameters (and ignoring the rpcauth), we can ensure a limited forward compatibility by not forcing users to downgrade their config file to switch back to older versions (perhaps temporarily).

To avoid any possible confusion, a message is still printed at startup, and returned as part of the error message if the user attempts to authenticate using the correct username/password.
RPC: Tolerate unknown parameters, but with clear warning/errors 3dab03e868

in src/httprpc.cpp:288 in 3dab03e868

 284 | +                if (fields.size() > MAX_RPCAUTH_VALUES) {
 285 | +                    LogPrintf("Unrecognised -rpcauth parameters for username '%s'. User will not be able to authenticate.\n", SanitizeString(fields[0]));
 286 | +                }
 287 |              } else {
 288 |                  LogPrintf("Invalid -rpcauth argument.\n");
 289 |                  return false;

MarcoFalke commented at 5:45 PM on December 2, 2020:

I think you are overcomplicating this. No need for a +53-7 patch when this line could be removed in a -1+0 patch to achieve the same user experience

luke-jr commented at 5:53 PM on December 2, 2020:

#20550

MarcoFalke changes_requested
DrahtBot added the label RPC/REST/ZMQ on Dec 2, 2020
jnewbery commented at 10:17 AM on December 3, 2020: member

NACK for the same reason as here: #20550 (comment).

We shouldn't make error handling worse in Bitcoin Core to make life easier for maintainers of downstream projects.
MarcoFalke commented at 10:40 AM on December 3, 2020: member

Closing for now due to controversy
MarcoFalke closed this on Dec 3, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#20550 RPC: Tolerate invalid rpcauth options