Sanitizing ports of -rpcconnect and -rpcport. #27820

1. 
   Brotcrunsher commented at 3:34 pm on June 4, 2023: contributor

   Previously, if they contained malformed ports they were silently interpreted as value%0xffff. Illegal ports now lead to an error. Additionally, if rpcconnect has a port and rpcport is set, a useful warning is now printed.
2. Sanitizing ports of -rpcconnect and -rpcport. Previously, if they contained malformed ports they were silently interpreted as value%0xffff. Illegal ports now lead to an error. Additionally, if rpcconnect has a port and rpcport is set, a useful warning is now printed. 6027388939
3. 
   DrahtBot commented at 3:34 pm on June 4, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	luke-jr, S3RK

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

4. 
   luke-jr commented at 1:56 am on June 24, 2023: member

   Concept ACK
5. achow101 requested review from S3RK on Sep 20, 2023
6. achow101 requested review from achow101 on Sep 20, 2023
7. in src/bitcoin-cli.cpp:749 in 6027388939

   742@@ -743,9 +743,23 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
   743     //     2. port in -rpcconnect (ie following : in ipv4 or ]: in ipv6)
   744     //     3. default port for chain
   745     uint16_t port{BaseParams().RPCPort()};
   746-    SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host);
   747-    port = static_cast<uint16_t>(gArgs.GetIntArg("-rpcport", port));
   748+    {
   749+        bool containsPort;
   750+        if (!SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host, &containsPort)) {
   751+            throw std::runtime_error(strprintf("Malformed Port in -rpcconnect: %s", gArgs.GetArg("-rpcconnect", "")));
   

   ---

   ---

   

   S3RK commented at 7:22 am on September 25, 2023:

   @hebasto we can reuse InvalidPortErrMsg() here, but I don’t see any other users of bilingual strings. What do you recommend for bitcoin-cli?
8. in src/bitcoin-cli.cpp:754 in 6027388939

   751+            throw std::runtime_error(strprintf("Malformed Port in -rpcconnect: %s", gArgs.GetArg("-rpcconnect", "")));
   752+        }
   753+        if (gArgs.IsArgSet("-rpcport")) {
   754+            int64_t desiredRpcPort = gArgs.GetIntArg("-rpcport", port);
   755+            if (desiredRpcPort <= 0 || desiredRpcPort > 0xFFFF) {
   756+                throw std::runtime_error(strprintf("Malformed Port in -rpcport: %lld", desiredRpcPort));
   

   ---

   ---

   

   S3RK commented at 7:25 am on September 25, 2023:

   nit: better to read port as a string to show it back to the user exactly as it was written.

   this is how src/init.cpp does it

   0const std::string port = args.GetArg(port_option, "");
   1uint16_t n;
   2if (!ParseUInt16(port, &n) || n == 0) {
   3    return InitError(InvalidPortErrMsg(port_option, port));
   4}
   

9. in src/util/strencodings.cpp:101 in 6027388939

    97@@ -98,20 +98,22 @@ std::optional<std::vector<Byte>> TryParseHex(std::string_view str)
    98 template std::optional<std::vector<std::byte>> TryParseHex(std::string_view);
    99 template std::optional<std::vector<uint8_t>> TryParseHex(std::string_view);
   100 
   101-bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut)
   102+bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut, bool* cPortOut)
   

   ---

   ---

   

   S3RK commented at 7:30 am on September 25, 2023:

   Not a huge fan of this new parameter. The return value has almost exactly what we need, can we reuse it somehow?
10. 
    S3RK commented at 7:34 am on September 25, 2023: contributor

    Concept ACK.

    Could you separate the PR into two commits? PR does two separate things, each of them could be placed in a separate commit:

    1. Handling of incorrect port values
    2. Warning in case both params have a port.

    Also, could you add a functional test for the first? You can see similar examples in test/functional/interface_bitcoin_cli.py

11. DrahtBot added the label CI failed on Oct 25, 2023
12. 
    DrahtBot commented at 7:22 pm on October 25, 2023: contributor

    Are you still working on this?
13. maflcko removed the label CI failed on Nov 16, 2023
14. maflcko added the label Up for grabs on Nov 16, 2023
15. maflcko closed this on Nov 16, 2023

    ---

16. fanquake removed the label Up for grabs on Jun 6, 2024
17. 
    fanquake commented at 12:39 pm on June 6, 2024: member

    Picked up in #29521.

Contributors
Brotcrunsher DrahtBot luke-jr S3RK fanquake

Review Requested
achow101