Bugfix: RPC: Check for blank rpcauth on a per-param basis #29141
Conversation
Without this, a blank rpcauth will error if there is a non-blank following it, and a non-blank rpcauth will get ignored if the last one is blank
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I'm not entirely sure what the expected behaviour here is... |
Are you still working on this? Not sure something that is unsupported does need fixing. |
|
Not sure what more work there is to be done here. The current behaviour is broken |
|
I can't recall that a blank rpcauth was ever supported, not in the docs, or in the tests, nor in the written code or code comments. Absent of that, changing the behavior and claiming that the previous is a bug and the new behavior is a fix, seems odd. If this was the behavior since the code was written, and someone relied on that, it would be a breaking change for them. So unless there is a good reason to support per-param blank auths, and discard the previous behavior, I'd say to not change it. When proposing a change, the burden to motivate and explain the change is on the pull request author. Simply making a change, then stating that you don't know the expected behavior (#29141 (comment)), while claiming your fix is a "bugfix" does not seem consequential to me. |
IIUC the "previous" behavior, i.e. the current behavior just doesn't make any sense. I made a similar bugfix in 8701603 from #17493, but it made specifying I think this PR (as of 6b79de4) would be an definite improvement it it had two tweaks:
|
|
Sure, turning it into an error makes more sense. At least users (in the unlikely case) relying on the previous behavior will be notified. But silently ignoring empty auths doesn't seem desirable, even if it is more "consistent". |
|
Are you still working on this? |
|
Converted to draft due to failing CI and inactivity |
|
See #30401. |
27c976d fix: increase consistency of rpcauth parsing (tdb3) 2ad3689 test: add norpcauth test (tdb3) 67df0de test: blank rpcauth CLI interaction (tdb3) ecc98cc test: add cases for blank rpcauth (tdb3) Pull request description: The current `rpcauth` parsing behavior is inconsistent and unintuitive (see #29141 (comment) and additional details below). The current behavior inconsistently treats empty `rpcauth` as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid `rpcauth` params. Empty `rpcauth` is now consistently treated as an error and prevents bitcoind from starting. Continuation of the upforgrabs PR #29141. ### Additional details: Current `rpcauth` behavior is nonsensical: - If an empty `rpcauth` argument was specified as the last command line argument, it would cause all other `rpcauth` arguments to be ignored. - If an empty `rpcauth` argument was specified on the command line followed by any nonempty `rpcauth` argument, it would cause an error. - If an empty `rpcauth=` line was specified after non-empty rpcauth line in the config file it would cause an error. - If an empty `rpcauth=` line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were `-rpcauth` command line arguments and the last one was nonempty, in which case it would cause an error. New behavior is simple: - If an empty rpcauth config line or command line argument is used it will cause an error ACKs for top commit: naiyoma: Tested ACK [https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586](https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586) achow101: ACK 27c976d ryanofsky: Code review ACK 27c976d. Since last review commit message was just tweaked to clarify previous behavior. Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
Without this, a blank rpcauth will error if there is a non-blank following it, and a non-blank rpcauth will get ignored if the last one is blank
Includes test updates to detect issues
(It might make sense to support
-norpcauth/-rpcauth=0to disable all defined-rpcauthoptions, but that isn't currently supported, so out of scope here)