Exit and show error if unrecognized command line args are present by john-moffett · Pull Request #742 · bitcoin-core/gui

john-moffett · 2023-06-29T16:37:50Z

Fixes #741

Starting bitcoin-qt with non-hyphen ("-") arguments causes it to silently ignore any later valid options. For instance, invoking bitcoin-qt -server=1 foo -regtest on a fresh install will run mainnet instead of regtest.

This change makes the client exit with an error message if any such "loose" arguments are encountered. This mirrors how bitcoind handles it:

https://github.com/bitcoin/bitcoin/blob/c6287faae4c0e705a9258a340dfcf548906f12af/src/bitcoind.cpp#L127-L132

However, BIP-21 bitcoin: payment URIs are still allowed, but only if they're not followed by any additional options.

DrahtBot · 2023-06-29T16:37:52Z

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
ACK	hernanmarino, pablomartin4btc, maflcko, hebasto

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

maflcko · 2023-06-29T19:16:27Z

I always wondered why the code wasn't the same. Was this needed for payment requests, when they were linked to be opened by the gui application?

john-moffett · 2023-06-29T20:27:04Z

I'm not entirely sure why they weren't the same. In the beginning, bitcoind would handle RPC commands. Then QT got special-cased so that you couldn't do that for bitcoin-qt -- though it wouldn't give any errors. It would just silently ignore the subsequent options.

Then the argument-handling code split entirely here. Now bitcoind and bitcoin-qt mostly did their own thing.

Subsequently, the ability to call RPC methods from bitcoind's command line was removed, and "loose" (non-hyphen) arguments would result in errors.

I can't see any good reason for "loose" arguments to remain in bitcoin-qt.

The "ignored options" issue was actually first raised here. The fix didn't address the specific issue exactly, as it just caught unrecognized options (ie - arguments with a hyphen). But by then, as I mentioned, the "loose" argument problem had already been fixed in bitcoind.

achow101 · 2023-06-29T20:45:35Z

I think this is needed to handle BIP 21 URIs (not to be confused with the now removed BIP 70 payment protocol). The "loose" argument is interpreted as the URI, although ignored if they could not be recognized as one. Anything that follows it is explicitly ignored to avoid argument injection, see https://achow101.com/2021/02/0.18-uri-vuln.

john-moffett · 2023-06-29T21:39:24Z

Ah, thanks @achow101. I think this may still be salvageable by making an exception for just those arguments.

gui/src/qt/paymentserver.cpp

Line 83 in 561915f

if (arg.startsWith(BITCOIN_IPC_PREFIX, Qt::CaseInsensitive)) // bitcoin: URI

Will revisit tomorrow.

hebasto · 2023-07-03T14:20:45Z

Is it safer and simpler to just document that a payment URi must follow all command line options? In bitcoin-qt -help?

john-moffett · 2023-07-03T14:41:23Z

I could modify this PR to enforce that behavior and add the documentation to bitcoin-qt -help. Right now, the PR only allows non-option arguments which must start with bitcoin: (case insensitive). It will still ignore any options after that.

I think leaving the current master behavior and just adding documentation to reflect it would be insufficient. My worry is that a user could be fooled by something like:

Scammer: "Can you send me some testnet coins? Start your client with bitcoin-qt select-chain -testnet. You should have the same number of testnet coins as you have on the real network. Just send me some, please."

It's also just an annoyance. I spent an embarrassingly long time trying to figure out why bitcoin-qt datadir=bitcoindatadir -regtest kept opening mainnet before I realized I omitted the hyphen on datadir.

Starting bitcoin-qt with non-dash ("-") arguments causes it to silently ignore any later valid options. This change makes the client exit with an error message if any such "loose" arguments are encountered. However, allow BIP-21 'bitcoin:' URIs only if no other options follow.

john-moffett · 2023-07-03T17:08:49Z

Modified to follow @hebasto 's idea of only allowing bitcoin: URIs if no other options follow.

hernanmarino

tested ACK 51e4dc4

pablomartin4btc

tACK 51e4dc4

I like both features the error shown when invalid args are passed and the error for options that are passed following a BIP21.

For the last one and regarding the -regtest being ignored in your original PR description, I detected a similar scenario in bitcoin-cli a few months ago (bitcoin/bitcoin#26990 - still in progress) where [options] passed after a command that receives arguments are not detected by the ArgsManager, they are being interpreted as another arg of the last command.

Last thing, not part of the scope of this PR, I think we could validate the BIP21 URI grammar before opening bitcoin-qt.

luke-jr · 2023-09-15T18:37:39Z

+        if (payment_server_token_seen && arg.startsWith("-")) {
+            InitError(Untranslated(strprintf("Options ('%s') cannot follow a BIP-21 payment URI", argv[i])));
+            QMessageBox::critical(nullptr, PACKAGE_NAME,
+                                  // message cannot be translated because translations have not been initialized


Should we just initialize them in this codepath?

luke-jr · 2023-09-15T18:39:58Z

+        }
+#endif
+        if (payment_server_token_seen && arg.startsWith("-")) {
+            InitError(Untranslated(strprintf("Options ('%s') cannot follow a BIP-21 payment URI", argv[i])));


Suggested change

InitError(Untranslated(strprintf("Options ('%s') cannot follow a BIP-21 payment URI", argv[i])));

InitError(Untranslated(strprintf("Options (eg, '%s') cannot follow a URI", argv[i])));

maflcko · 2023-10-20T10:34:24Z

lgtm ACK 51e4dc4

maflcko · 2023-10-25T11:11:47Z

Fuzz CI failure can be ignored

hebasto

ACK 51e4dc4, I have reviewed the code and it looks OK.

…for BIP21 URIs ede5014 Modify command line help to show support for BIP21 URIs (Hernan Marino) Pull request description: While reviewing a different PR (see bitcoin-core/gui#742 ) **hebasto** suggested that the help for bitcoin-qt should be updated to reflect the fact that bitcoin-qt supports an optional BIP21 URI parameter. Since this reflects actual behaviour of bitcoin-qt and is independent of whether or not the other PR gets merged, I created this simple PR to fix the help message. ACKs for top commit: kristapsk: utACK ede5014 pablomartin4btc: lgtm, re ACK ede5014 hebasto: ACK ede5014. Tree-SHA512: c456297c486bc5cc65e0e092e7ba9d51b0bd7a584d4fabca7f7ca1f8e58cbcc66e96226539c689ed0f5e7f40da220bbc4ea30b90e31e1aeeb8867a385a90209c

john-moffett force-pushed the 2023_06_ExitOnLooseArgument branch from 250b171 to 505c726 Compare June 30, 2023 19:59

john-moffett force-pushed the 2023_06_ExitOnLooseArgument branch from 505c726 to 51e4dc4 Compare July 3, 2023 17:05

hebasto requested a review from achow101 July 4, 2023 15:58

DrahtBot added CI failed and removed CI failed labels Aug 17, 2023

DrahtBot added CI failed and removed CI failed labels Aug 31, 2023

hernanmarino approved these changes Sep 8, 2023

View reviewed changes

hernanmarino mentioned this pull request Sep 8, 2023

Modify command line help to show support for BIP21 URIs #752

Merged

pablomartin4btc reviewed Sep 15, 2023

View reviewed changes

luke-jr reviewed Sep 15, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Oct 13, 2023

DrahtBot added the CI failed label Oct 25, 2023

hebasto approved these changes Oct 25, 2023

View reviewed changes

hebasto merged commit afa081a into bitcoin-core:master Oct 25, 2023

bitcoin-core locked and limited conversation to collaborators Oct 24, 2024

	InitError(Untranslated(strprintf("Options ('%s') cannot follow a BIP-21 payment URI", argv[i])));
	InitError(Untranslated(strprintf("Options (eg, '%s') cannot follow a URI", argv[i])));

Uh oh!

Conversation

john-moffett commented Jun 29, 2023 • edited by hebasto Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

maflcko commented Jun 29, 2023

Uh oh!

john-moffett commented Jun 29, 2023

Uh oh!

achow101 commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-moffett commented Jun 29, 2023

Uh oh!

hebasto commented Jul 3, 2023

Uh oh!

john-moffett commented Jul 3, 2023

Uh oh!

john-moffett commented Jul 3, 2023

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment • 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!

luke-jr Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 20, 2023

Uh oh!

maflcko commented Oct 25, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

john-moffett commented Jun 29, 2023 •

edited by hebasto

Loading

DrahtBot commented Jun 29, 2023 •

edited

Loading

achow101 commented Jun 29, 2023 •

edited

Loading

pablomartin4btc left a comment •

edited

Loading