Bugfix: RPC: Check for blank rpcauth on a per-param basis #29141

1. 
   luke-jr commented at 6:50 pm on December 23, 2023: member

   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=0 to disable all defined -rpcauth options, but that isn’t currently supported, so out of scope here)

2. Bugfix: RPC: Check for blank rpcauth on a per-param basis

   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

   d91d0227ae
3. QA: rpc_users: Test blank rpcauth in combination with non-blank 415734ebaf
4. 
   DrahtBot commented at 6:50 pm on December 23, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30401 (fix: increase consistency of rpcauth parsing by tdb3)
   • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

   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.

5. 
   luke-jr commented at 7:06 pm on December 23, 2023: member

   I’m not entirely sure what the expected behaviour here is…
6. QA: rpc_users: Test behaviour of -norpcauth 6b79de4f9a
7. DrahtBot added the label CI failed on Jan 15, 2024
8. 
   maflcko commented at 6:20 pm on February 22, 2024: member

   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.

9. 
   luke-jr commented at 4:45 pm on February 29, 2024: member

   Not sure what more work there is to be done here. The current behaviour is broken
10. 
    maflcko commented at 5:19 pm on February 29, 2024: member

    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 (https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1868353002), while claiming your fix is a “bugfix” does not seem consequential to me.

11. 
    ryanofsky commented at 10:34 pm on February 29, 2024: contributor

    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.

    IIUC the “previous” behavior, i.e. the current behavior just doesn’t make any sense. I made a similar bugfix in 870160359e7cec9f66c7c26f01f4e65b40f35e1b from #17493, but it made specifying -rpcauth="" an error instead of an ignored parameter, which I think would be a better thing to do here. You can read 870160359e7cec9f66c7c26f01f4e65b40f35e1b for a description of the current behavior, it is pretty crazy.

    I think this PR (as of 6b79de4f9aed365adda0a99d6522e83ccb6c593f) would be an definite improvement it it had two tweaks:

    • It would be great if it dropped the if (rpcauth.empty()) continue; line and treated -rpcauth="" as an error consistently.
    • It would be great if the new python tests were added as a first commit, and the bugfix was added as a second commit, that way it would be a lot more obvious what current behavior is and how the bugfix changes it.
12. 
    maflcko commented at 7:27 am on March 1, 2024: member

    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”.
13. 
    maflcko commented at 5:31 pm on April 17, 2024: member

    Are you still working on this?
14. DrahtBot marked this as a draft on Apr 23, 2024
15. 
    DrahtBot commented at 6:45 am on April 23, 2024: contributor

    Converted to draft due to failing CI and inactivity
16. DrahtBot added the label Up for grabs on Jun 18, 2024
17. fanquake closed this on Jul 6, 2024

    ---

18. fanquake removed the label Up for grabs on Jul 6, 2024
19. 
    fanquake commented at 1:45 pm on July 6, 2024: member

    See #30401.

Contributors
luke-jr DrahtBot maflcko ryanofsky fanquake

Labels
CI failed