Error if # is used in rpcpassword in conf by meshcollider · Pull Request #14494 · bitcoin/bitcoin

meshcollider · 2018-10-16T05:36:34Z

Fixes #13143 now #13482 was merged

laanwj · 2018-10-16T05:39:51Z

src/util.cpp

concept ACK—
though please reword this message, the # is not so much illegal, it starts a legal comment; it's just that probably the user made a mistake, by assuming the # will actually be part of the password

hebasto · 2018-10-16T06:33:13Z

src/util.cpp

Could # be mentioned as "reserved character"?

promag

utACK fa3569d, simple solution.

It's a bit awkward to have this limitation though. For instance :

#                  split here because of whitespace before of #
#                  v
rpcpassword=foo#bar # comment # more comments..

# allow quotes, however it should allow escaping quotes too
pcpassword="foo#bar\"bar"# comment

Maybe it should warn each comment in the middle of lines?

promag · 2018-10-16T08:52:32Z

src/util.cpp

Alternative:

pos = str.find('#'); const bool has_comment = pos != std::string::npos; if (has_comment) str = str.substr(0, pos);

meshcollider · 2018-10-16T09:45:16Z

I think allowing quotes makes the situation even more ambiguous and confusing, I mentioned it on IRC. The whitespace split seems ok but I'm not sure it's worth it, better safe than sorry?

promag · 2018-10-16T09:50:45Z

Maybe it should also error with command line -rpcpassword=foo#bar to make it consistent?

hebasto · 2018-10-16T20:09:13Z

There is another approach that allows # in the rpcpassword value:

static bool GetConfigOptions(std::istream& stream, std::string& error, std::vector<std::pair<std::string, std::string>> &options)
{
    std::string str, prefix;
    std::string::size_type pos;
    int linenr = 1;
    while (std::getline(stream, str)) {
        // A number sign (#) at the beginning of the line indicates a comment. Comment lines are ignored. 
        if ((pos = str.find('#')) != 0) {
            const static std::string pattern = " \t\r\n";
            str = TrimString(str, pattern);
            if (!str.empty()) {
                if (*str.begin() == '[' && *str.rbegin() == ']') {
                    prefix = str.substr(1, str.size() - 2) + '.';
                } else if (*str.begin() == '-') {
                    error = strprintf("parse error on line %i: %s, options in configuration file must be specified without leading -", linenr, str);
                    return false;
                } else if ((pos = str.find('=')) != std::string::npos) {
                    std::string name = prefix + TrimString(str.substr(0, pos), pattern);
                    std::string value = TrimString(str.substr(pos + 1), pattern);
                    options.emplace_back(name, value);
                } else {
                    error = strprintf("parse error on line %i: %s", linenr, str);
                    if (str.size() >= 2 && str.substr(0, 2) == "no") {
                        error += strprintf(", if you intended to specify a negated option, use %s=1 instead", str);
                    }
                    return false;
                }
            }
        }
        ++linenr;
    }
    return true;
}

promag · 2018-10-16T20:30:34Z

@hebasto unfortunately that is breaking change.

gmaxwell · 2018-10-16T22:22:58Z

The error message should probably recommend rpcauth over rpcpassword. I think just rejecting # in rpcpassword lines makes sense.

ryanofsky · 2018-10-19T18:51:06Z

utACK fa3569dbf16bc40c36f036e191a9de8a679d81f8. Erroring on # does seem like the simplest way to resolve this issue, if rpcpassword is considered deprecated in favor of rpcauth anyway.

If we ever decide we need to allow # values in the future, it seems like it would be straightforward to do this in a mostly-backwards compatible way by supporting simple quoting like

[section]
name = "value!@#$%^" # comment

1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov) Pull request description: From the IRC: > 2018-10-16T05:35:03 \<wumpus\> if something can be solved by better documentation, please work on documentation! > 2018-10-16T05:35:12 \<wumpus\> don't change the code instead Refs: - #14370 - #14427 - #14494 Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<[email protected]\> for the Debian system. Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4

DrahtBot · 2018-10-20T09:54:47Z

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

Conflicts

No conflicts as of last run.

meshcollider · 2018-11-05T23:36:54Z

Rebased

maflcko · 2018-11-06T15:57:40Z

utACK 0385109

0385109 Add test for rpcpassword hash error (MeshCollider) 13fe258 Error if rpcpassword in conf contains a hash character (MeshCollider) Pull request description: Fixes #13143 now #13482 was merged Tree-SHA512: e7d00c8df1657f6b8d0eee1e06b9ce2b1b0a2de487377699382c1b057836e1571dac313ca878b5877c862f0461ba789a50b239d2a9f34accd8a6321f126e3d2a

1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov) Pull request description: From the IRC: > 2018-10-16T05:35:03 \<wumpus\> if something can be solved by better documentation, please work on documentation! > 2018-10-16T05:35:12 \<wumpus\> don't change the code instead Refs: - bitcoin#14370 - bitcoin#14427 - bitcoin#14494 Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<[email protected]\> for the Debian system. Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4

0385109 Add test for rpcpassword hash error (MeshCollider) 13fe258 Error if rpcpassword in conf contains a hash character (MeshCollider) Pull request description: Fixes bitcoin#13143 now bitcoin#13482 was merged Tree-SHA512: e7d00c8df1657f6b8d0eee1e06b9ce2b1b0a2de487377699382c1b057836e1571dac313ca878b5877c862f0461ba789a50b239d2a9f34accd8a6321f126e3d2a

backport 0.18 PR bitcoin#14522 bitcoin#14672 bitcoin#14812 bitcoin#14494 bitcoin#14690 bitcoin#14700 bitcoin#14705 bitcoin#14478 bitcoin#14247

laanwj reviewed Oct 16, 2018

View reviewed changes

hebasto reviewed Oct 16, 2018

View reviewed changes

src/util.cpp Outdated

Copy link

Member

hebasto Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could # be mentioned as "reserved character"?

fanquake added the Utils/log/libs label Oct 16, 2018

promag reviewed Oct 16, 2018

View reviewed changes

hebasto mentioned this pull request Oct 16, 2018

docs: Add doc/bitcoin-conf.md #14497

Merged

hebasto mentioned this pull request Oct 18, 2018

Version number for bitcoin.conf format #14514

Closed

ryanofsky approved these changes Oct 19, 2018

View reviewed changes

DrahtBot mentioned this pull request Oct 20, 2018

Use non-throwing type-safe ChainType where possible #14309

Closed

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

DrahtBot mentioned this pull request Oct 30, 2018

Tests: Consistency changes in comments #14606

Closed

DrahtBot added the Needs rebase label Nov 5, 2018

meshcollider added 2 commits November 6, 2018 12:35

Error if rpcpassword in conf contains a hash character

13fe258

Add test for rpcpassword hash error

0385109

DrahtBot removed the Needs rebase label Nov 5, 2018

maflcko added this to the 0.18.0 milestone Nov 6, 2018

maflcko mentioned this pull request Nov 7, 2018

bitcoin-cli console walletpassphrase can't unlock wallet Password with some special characters will not work #14681

Closed

laanwj merged commit 0385109 into bitcoin:master Nov 12, 2018

meshcollider deleted the 201810_hash_in_rpcpassword_error branch November 12, 2018 14:25

harding mentioned this pull request Jan 1, 2019

Expected error not printed if rpcpassword contains hash character but is in a [section] #15075

Closed

UdjinM6 added a commit to dashpay/dash that referenced this pull request Aug 29, 2021

Merge pull request #4375 from PastaPastaPasta/pr_4330

f38aa44

backport 0.18 PR bitcoin#14522 bitcoin#14672 bitcoin#14812 bitcoin#14494 bitcoin#14690 bitcoin#14700 bitcoin#14705 bitcoin#14478 bitcoin#14247

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

meshcollider commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Oct 16, 2018

Uh oh!

promag commented Oct 16, 2018

Uh oh!

hebasto commented Oct 16, 2018

Uh oh!

promag commented Oct 16, 2018

Uh oh!

gmaxwell commented Oct 16, 2018

Uh oh!

ryanofsky commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider commented Nov 5, 2018

Uh oh!

maflcko commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

meshcollider commented Oct 16, 2018 •

edited

Loading

laanwj Oct 16, 2018 •

edited

Loading

ryanofsky commented Oct 19, 2018 •

edited

Loading

DrahtBot commented Oct 20, 2018 •

edited

Loading