Error if # is used in rpcpassword in conf #14494

pull meshcollider wants to merge 2 commits into bitcoin:master from meshcollider:201810_hash_in_rpcpassword_error changing 2 files +10 −0

meshcollider commented at 5:36 AM on October 16, 2018: contributor

Fixes #13143 now #13482 was merged

in src/util.cpp:846 in ec998827dc outdated

 841 | @@ -840,6 +842,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
 842 |              } else if ((pos = str.find('=')) != std::string::npos) {
 843 |                  std::string name = prefix + TrimString(str.substr(0, pos), pattern);
 844 |                  std::string value = TrimString(str.substr(pos + 1), pattern);
 845 | +                if (used_hash && name == "rpcpassword") {
 846 | +                    error = strprintf("parse error on line %i, illegal hash character used in rpcpassword", linenr);

laanwj commented at 5:39 AM on October 16, 2018:

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

meshcollider commented at 5:55 AM on October 16, 2018:

Fixed :)

in src/util.cpp:846 in fa3569dbf1 outdated

 841 | @@ -840,6 +842,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
 842 |              } else if ((pos = str.find('=')) != std::string::npos) {
 843 |                  std::string name = prefix + TrimString(str.substr(0, pos), pattern);
 844 |                  std::string value = TrimString(str.substr(pos + 1), pattern);
 845 | +                if (used_hash && name == "rpcpassword") {
 846 | +                    error = strprintf("parse error on line %i, using # in rpcpassword can be ambiguous and should be avoided", linenr);

hebasto commented at 6:33 AM on October 16, 2018:

Could # be mentioned as "reserved character"?

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

in src/util.cpp:829 in fa3569dbf1 outdated

 825 | @@ -826,8 +826,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
 826 |      std::string::size_type pos;
 827 |      int linenr = 1;
 828 |      while (std::getline(stream, str)) {
 829 | +        bool used_hash = false;

promag commented at 8:52 AM on October 16, 2018:

Alternative:

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

promag commented at 9:04 AM on October 16, 2018: member

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?

meshcollider commented at 9:45 AM on October 16, 2018: contributor

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 commented at 9:50 AM on October 16, 2018: member

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

hebasto commented at 8:09 PM on October 16, 2018: member

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 commented at 8:30 PM on October 16, 2018: member

@hebasto unfortunately that is breaking change.
hebasto cross-referenced this on Oct 16, 2018 from issue docs: Add doc/bitcoin-conf.md by hebasto
gmaxwell commented at 10:22 PM on October 16, 2018: contributor

The error message should probably recommend rpcauth over rpcpassword. I think just rejecting # in rpcpassword lines makes sense.
hebasto cross-referenced this on Oct 18, 2018 from issue Version number for `bitcoin.conf` format by hebasto
ryanofsky commented at 6:51 PM on October 19, 2018: contributor
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
```
ryanofsky approved
laanwj referenced this in commit 6746a89519 on Oct 20, 2018
DrahtBot commented at 9:54 AM on October 20, 2018: contributor



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



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on Oct 20, 2018 from issue Use non-throwing type-safe ChainType where possible by MarcoFalke
practicalswift cross-referenced this on Oct 22, 2018 from issue Enable flake8 rule E225 which checks for missing whitespace around op… by jbampton
DrahtBot cross-referenced this on Oct 30, 2018 from issue Tests: Consistency changes in comments by fridokus
DrahtBot added the label Needs rebase on Nov 5, 2018
Error if rpcpassword in conf contains a hash character 13fe258e91
Add test for rpcpassword hash error 0385109444
meshcollider commented at 11:36 PM on November 5, 2018: contributor

Rebased
DrahtBot removed the label Needs rebase on Nov 5, 2018
MarcoFalke commented at 3:57 PM on November 6, 2018: member

utACK 0385109444646561a718f34ae437b7e0e4d4d5bc
MarcoFalke added this to the milestone 0.18.0 on Nov 6, 2018
MarcoFalke cross-referenced this on Nov 7, 2018 from issue bitcoin-cli console walletpassphrase can't unlock wallet Password with some special characters will not work by douglasdeodato
laanwj merged this on Nov 12, 2018
laanwj closed this on Nov 12, 2018
laanwj referenced this in commit 90c0b6aca2 on Nov 12, 2018
meshcollider deleted the branch on Nov 12, 2018
harding cross-referenced this on Jan 1, 2019 from issue Expected error not printed if rpcpassword contains hash character but is in a [section] by harding
PastaPastaPasta referenced this in commit 7644b0fa8e on Jun 27, 2021
PastaPastaPasta referenced this in commit 1454380ea9 on Jun 28, 2021
PastaPastaPasta referenced this in commit 931802b239 on Jun 29, 2021
PastaPastaPasta referenced this in commit 8a9b3ff5f1 on Jul 1, 2021
PastaPastaPasta referenced this in commit 868f071b9f on Jul 1, 2021
PastaPastaPasta referenced this in commit 30cdf9c6f7 on Jul 1, 2021
PastaPastaPasta referenced this in commit 787113a778 on Jul 3, 2021
knst referenced this in commit e74373879b on Aug 10, 2021
knst referenced this in commit a4062f57d9 on Aug 10, 2021
knst referenced this in commit 705d36cd39 on Aug 16, 2021
PastaPastaPasta referenced this in commit 42d1b0ba71 on Aug 23, 2021
christiancfifi referenced this in commit b33ca8474a on Aug 27, 2021
christiancfifi referenced this in commit 74858f06b4 on Aug 27, 2021
christiancfifi referenced this in commit 33876d2450 on Aug 28, 2021
christiancfifi referenced this in commit 1feb306462 on Aug 29, 2021
christiancfifi referenced this in commit bfd9852f90 on Aug 29, 2021
christiancfifi referenced this in commit b9d2e81286 on Aug 29, 2021
UdjinM6 referenced this in commit f38aa44bfd on Aug 29, 2021
bitcoin locked this on Sep 8, 2021