Error if # is used in rpcpassword in conf

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:

0pos = str.find('#');
1const bool has_comment = pos != std::string::npos;
2if (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 :

0#                  split here because of whitespace before of #
1#                  v
2rpcpassword=foo#bar # comment # more comments..
3
4# allow quotes, however it should allow escaping quotes too
5pcpassword="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:

 0static bool GetConfigOptions(std::istream& stream, std::string& error, std::vector<std::pair<std::string, std::string>> &options)
 1{
 2    std::string str, prefix;
 3    std::string::size_type pos;
 4    int linenr = 1;
 5    while (std::getline(stream, str)) {
 6        // A number sign (#) at the beginning of the line indicates a comment. Comment lines are ignored. 
 7        if ((pos = str.find('#')) != 0) {
 8            const static std::string pattern = " \t\r\n";
 9            str = TrimString(str, pattern);
10            if (!str.empty()) {
11                if (*str.begin() == '[' && *str.rbegin() == ']') {
12                    prefix = str.substr(1, str.size() - 2) + '.';
13                } else if (*str.begin() == '-') {
14                    error = strprintf("parse error on line %i: %s, options in configuration file must be specified without leading -", linenr, str);
15                    return false;
16                } else if ((pos = str.find('=')) != std::string::npos) {
17                    std::string name = prefix + TrimString(str.substr(0, pos), pattern);
18                    std::string value = TrimString(str.substr(pos + 1), pattern);
19                    options.emplace_back(name, value);
20                } else {
21                    error = strprintf("parse error on line %i: %s", linenr, str);
22                    if (str.size() >= 2 && str.substr(0, 2) == "no") {
23                        error += strprintf(", if you intended to specify a negated option, use %s=1 instead", str);
24                    }
25                    return false;
26                }
27            }
28        }
29        ++linenr;
30    }
31    return true;
32}

promag commented at 8:30 pm on October 16, 2018: member

@hebasto unfortunately that is breaking change.

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.

ryanofsky commented at 6:51 pm on October 19, 2018: member

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

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

ryanofsky approved

laanwj referenced this in commit 6746a89519 on Oct 20, 2018

DrahtBot commented at 9:54 am on October 20, 2018: member

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

Conflicts

No conflicts as of last run.

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

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

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

DrahtBot locked this on Sep 8, 2021

Error if # is used in rpcpassword in conf #14494

Conflicts