Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error if # is used in rpcpassword in conf #14494

Merged
merged 2 commits into from Nov 12, 2018

Conversation

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Oct 16, 2018

Fixes #13143 now #13482 was merged

src/util.cpp Outdated
@@ -840,6 +842,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
} 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);
if (used_hash && name == "rpcpassword") {
error = strprintf("parse error on line %i, illegal hash character used in rpcpassword", linenr);
Copy link
Member

@laanwj laanwj Oct 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

Copy link
Member Author

@meshcollider meshcollider Oct 16, 2018

Fixed :)

@meshcollider meshcollider force-pushed the 201810_hash_in_rpcpassword_error branch from ec99882 to fa3569d Oct 16, 2018
src/util.cpp Outdated
@@ -840,6 +842,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
} 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);
if (used_hash && name == "rpcpassword") {
error = strprintf("parse error on line %i, using # in rpcpassword can be ambiguous and should be avoided", linenr);
Copy link
Member

@hebasto hebasto Oct 16, 2018

Could # be mentioned as "reserved character"?

Copy link
Member

@promag promag left a comment

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?

src/util.cpp Outdated
@@ -826,8 +826,10 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
std::string::size_type pos;
int linenr = 1;
while (std::getline(stream, str)) {
bool used_hash = false;
Copy link
Member

@promag promag Oct 16, 2018

Alternative:

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

@meshcollider
Copy link
Member Author

@meshcollider meshcollider commented Oct 16, 2018

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
Copy link
Member

@promag promag commented Oct 16, 2018

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

@hebasto
Copy link
Member

@hebasto hebasto commented Oct 16, 2018

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
Copy link
Member

@promag promag commented Oct 16, 2018

@hebasto unfortunately that is breaking change.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Oct 16, 2018

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

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 19, 2018

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

laanwj added a commit that referenced this issue Oct 20, 2018
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 20, 2018

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

Conflicts

No conflicts as of last run.

@meshcollider meshcollider force-pushed the 201810_hash_in_rpcpassword_error branch from fa3569d to 0385109 Nov 5, 2018
@meshcollider
Copy link
Member Author

@meshcollider meshcollider commented Nov 5, 2018

Rebased

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Nov 6, 2018

utACK 0385109

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Nov 6, 2018
@laanwj laanwj merged commit 0385109 into bitcoin:master Nov 12, 2018
2 checks passed
laanwj added a commit that referenced this issue Nov 12, 2018
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
@meshcollider meshcollider deleted the 201810_hash_in_rpcpassword_error branch Nov 12, 2018
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 27, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 28, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 29, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 3, 2021
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 \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
knst added a commit to knst/dash that referenced this issue Aug 10, 2021
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
knst added a commit to knst/dash that referenced this issue Aug 10, 2021
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
knst added a commit to knst/dash that referenced this issue Aug 16, 2021
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Aug 23, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 27, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 27, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 28, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 29, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 29, 2021
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
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 29, 2021
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants