wallet: Replace %w by wallet name in -walletnotify script

promag commented at 1:45 pm on May 29, 2018: member

Fixes #13237.

fanquake added the label Wallet on May 29, 2018

in src/wallet/wallet.cpp:991 in 608562276e outdated

987@@ -988,6 +988,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
988     if (!strCmd.empty())
989     {
990         boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
991+        boost::replace_all(strCmd, "%w", GetName());

laanwj commented at 3:15 pm on May 29, 2018:

As the wallet name can contain any character, we need shell-escaping here.

promag commented at 4:11 pm on May 29, 2018: member

@laanwj see 4fa29f8, what would be the solution in that case? only escape '? IIUC there is no function to do that as it depends on the shell.

jamesob commented at 4:27 pm on May 29, 2018: member

Concept ACK - seems like a nice consideration

laanwj commented at 5:17 pm on May 29, 2018: member

@laanwj see 4fa29f8, what would be the solution in that case? only escape ‘? IIUC there is no function to do that as it depends on the shell.

The best way would be to use an invocation (use one of the exec* functions) that passes arguments without involving the shell. That’s quite a big change, though, and existing usages of the arguments might assume a shell is involved.

Lacking that, you can escape for bash and most other shells by surrounding with ' and replacing single-quotes with '"'"'.

It’s extremely important to be careful about this, I have seen terrible things happen due to unproperly escaped shell input, and I will NACK this unless this is addressed.

promag commented at 5:29 pm on May 29, 2018: member

@laanwj exec* yes that would be breaking change. Agree on the NACK reasons. I think I’ll go with https://unix.stackexchange.com/a/30936. I’ll change the test to cover more cases.

laanwj commented at 5:52 pm on May 29, 2018: member

@laanwj exec* yes that would be breaking change. Agree on the NACK reasons. I think I’ll go with https://unix.stackexchange.com/a/30936. I’ll change the test to cover more cases.

Agree. Right - things to test would be $VAR and ${VAR} in the name, $(command) as well as various quote characters.

promag force-pushed on May 29, 2018

promag commented at 8:13 pm on May 29, 2018: member

Agree. Right - things to test would be $VAR and ${VAR} in the name, $(command) as well as various quote characters.

I don’t understand what you mean. You suggest the wallet name can have $VAR?

laanwj commented at 1:33 am on May 30, 2018: member

I don’t understand what you mean. You suggest the wallet name can have $VAR?

Yes, those sequences could be part of the wallet name. They are the typical ways to fool shell escaping and do arbitrary command execution, so I suggested them for testing whether shell escaping is working as expected.

jonasschnelli commented at 8:37 am on May 30, 2018: contributor

Not sure if we want to further extend walletnotify. A more future-proof way is probably the recently created RPC approach #13262. It would still be possible to ship a little python script that emulates the walletnotify “API”.

promag commented at 9:04 am on May 30, 2018: member

IMO we should consider adding this to fully support multi wallets without taking into account #13262. For existing systems that are based on -walletnotify changing to listsincetx would be more problematic. Beside that, currently listsincetx requires a connection for each wallet. I do consider listsincetx better than -walletnotify though.

laanwj commented at 10:06 am on May 30, 2018: member

I think it’s fine to add this to walletnotify, too. Agree that another notification system would be better, but this is useful to users in the short term. It’s is a small change to the code.

MarcoFalke commented at 1:58 pm on May 30, 2018: member

Needs rebase due to merge of #13341

promag force-pushed on May 30, 2018

in src/wallet/wallet.cpp:992 in cef0327afd outdated

987@@ -988,6 +988,9 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
988     if (!strCmd.empty())
989     {
990         boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
991+        std::string name = GetName();
992+        boost::replace_all(name, "'", "'\''");

laanwj commented at 6:01 am on May 31, 2018:

Please factor this out to a function ShellEscape which does boost::replace_all(name, "'", "'\''");; return "'" + name + "'". We might need that in other places at some point, and it makes it an easier to review unit. (also it’s possible that for WIN32/cmd this needs something else?)

promag commented at 10:34 am on May 31, 2018:

Will do.

in test/functional/feature_notifications.py:28 in cef0327afd outdated

23@@ -24,7 +24,8 @@ def setup_network(self):
24                             "-blocknotify=echo %%s >> %s" % self.block_filename],
25                            ["-blockversion=211",
26                             "-rescan",
27-                            "-walletnotify=echo %%s >> %s" % self.tx_filename]]
28+                            "-wallet=t- 1",
29+                            "-walletnotify=echo %%w\:%%s >> %s" % self.tx_filename]]

luke-jr commented at 9:28 pm on June 12, 2018:

This would work even without %w properly escaped.

MarcoFalke commented at 6:54 am on July 8, 2018:

Could just use '...%w\:%s >> {}'.format(self.tx_filename) to avoid this confusion.

MarcoFalke added the label Needs rebase on Jun 16, 2018

laanwj commented at 9:30 am on September 12, 2018: member

Needs rebase and comments addressed.

laanwj added this to the milestone 0.18.0 on Oct 11, 2018

meshcollider commented at 2:43 pm on November 12, 2018: contributor

LGTM https://github.com/bitcoin/bitcoin/pull/13339/commits/cef0327afd220950142ec9d4d051bb94f5210be1 but yeah needs rebase + comments addressed 👍

MarcoFalke added the label Up for grabs on Nov 12, 2018

promag force-pushed on Nov 12, 2018

DrahtBot removed the label Needs rebase on Nov 12, 2018

practicalswift commented at 12:36 pm on November 13, 2018: contributor

@promag This no longer compiles:

0error: no member named 'replace_all' in namespace 'boost'

Perhaps a good opportunity to get rid of boost::replace_all? :-)

promag force-pushed on Nov 13, 2018

MarcoFalke removed the label Up for grabs on Nov 13, 2018

in test/functional/feature_notifications.py:32 in f43c5e2221 outdated

28@@ -29,7 +29,8 @@ def setup_network(self):
29                             "-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s'))],
30                            ["-blockversion=211",
31                             "-rescan",
32-                            "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%s'))]]
33+                            "-wallet=t- 1",

MarcoFalke commented at 4:55 pm on November 13, 2018:

This test fails when the wallet name has a ' in it. Probably need to quote it below?

promag force-pushed on Nov 13, 2018

DrahtBot commented at 7:50 pm on November 13, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag force-pushed on Nov 14, 2018

promag commented at 2:00 am on November 15, 2018: member

@ken2812221 do you mind checking the last commit?

ken2812221 commented at 8:24 am on November 15, 2018: contributor

Sure, will test

ken2812221 commented at 0:02 am on November 17, 2018: contributor

ACK ac771808910dab16e7d0c413a1077464407c66e2. I think it’s good to test all available ascii characters. like 2f16218b658aecf798a77cb5e5ecae5f000e1729

in src/util/system.cpp:1165 in 2330bb4f62 outdated

1205+{
1206+    using convert_typeX = std::codecvt_utf8<wchar_t>;
1207+    std::wstring_convert<convert_typeX, wchar_t> converterX;
1208+
1209+    return converterX.to_bytes(wstr);
1210+}

ken2812221 commented at 2:14 am on November 19, 2018:

~~Can you explain that why ws2s and s2ws is required? I think it’s fine to use std::string to add quotes.~~

Edit: Oh I know. It’s unicode problem. But maybe it’s ok if #13884 merged.

ken2812221 commented at 4:12 am on January 10, 2019:

#13884 has been merged, you could use std::string instead of std::wstring and drop ws2s, s2ws now.

DrahtBot added the label Needs rebase on Nov 19, 2018

promag commented at 1:00 pm on November 21, 2018: member

Rebased.

promag force-pushed on Nov 21, 2018

DrahtBot removed the label Needs rebase on Nov 21, 2018

in src/util/system.cpp:1119 in 71d70632ee outdated

1159@@ -1158,6 +1160,68 @@ fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
1160 }
1161 #endif
1162 
1163+#ifdef WIN32
1164+std::wstring ArgvQuote(const std::wstring& arg)

luke-jr commented at 2:22 am on November 25, 2018:

This appears to be copied out of this MSDN article, at least some derivative thereof. There’s no indication that it is licensed for free use. Please clarify the licensing terms (and move it to a separate commit with proper attribution to the true author), or rewrite it.

Furthermore, this article mentions that the quoting done herein is not sufficient if the commandline passes through cmd.exe, which it probably does in our case. Someone should test to confirm whether ^ works (if it does, we need it).

promag commented at 3:23 pm on November 26, 2018:

Yes it’s borrowed from there. I’ll update accordingly.

MarcoFalke commented at 8:11 pm on November 26, 2018:

Rewriting it doesn’t make the creators authorship disappear, just like translations don’t make the copyright disappear. Though, very straightforward algorithms can’t be licensed due to their simplicity. I suggest we at least put a link and author in a comment inside this function.

luke-jr commented at 9:40 pm on November 28, 2018:

“Rewrite” does not mean “obfuscate”.

Again, AFAICT, this code is not licensed freely. (I don’t think there’s a strong argument that it is too simple for copyright.)

ryanofsky commented at 11:12 pm on January 16, 2019:

re: #13339 (review)

I don’t think we should use this code. As Luke points out it’s unlicensed, and it doesn’t handle cmd.exe escaping, so it will break if the argument contains quotes.

But the problem is actually worse than that. There is no reliable way to escape argv values on windows because the windows OS doesn’t have a concept of argument arrays. When you create a process on windows, you only pass a single command line string, not an array of strings like on unix. Parsing of the command line string into C main() arguments can happen different ways. The MSDN article describes one way implemented by the visual c++ runtime, but other runtimes like cygwin may parse arguments differently. And while cmd.exe will normally interpret the command, this only happens because COMSPEC is set by default to cmd.exe (it could also be unset or set to something like bash or powershell).

I agree with @laanwj in #13339 (review) that using base64 is here probably overkill, but I don’t think using evhttp_uriencode on windows like bitcoin-cli does would be a bad option:

https://github.com/bitcoin/bitcoin/blob/fcb6694a9945d2a02f40587e18bd395ef64048e0/src/bitcoin-cli.cpp#L359

But you will also have to replace % with ^% for cmd.exe. And for non-windows builds it would be safest to stick with your current ShellEscape (evhttp_uriencode won’t escape ~).

luke-jr changes_requested

in src/util/system.cpp:66 in 71d70632ee outdated

72@@ -73,6 +73,8 @@
73 #include <malloc.h>
74 #endif
75 
76+#include <boost/algorithm/string/replace.hpp>

practicalswift commented at 8:21 pm on November 26, 2018:

Consider adding a project local ReplaceAll(…) instead of bringing in boost::replace_all.

luke-jr commented at 10:30 am on January 10, 2019:

Using dependencies is better than rewriting them.

practicalswift commented at 11:23 am on January 10, 2019:

@luke-jr Sometimes, but not always.

This is new code and we’re mostly using our own string handling helpers already.

Personally I think it is a mistake to add new code depending on Boost: I want to see declining Boost usage without unnecessary increases like this one.

promag commented at 11:30 am on January 10, 2019:

@practicalswift boost::replace_all is already a dependency.

Beside that, I’m considering EncodeBase64 the wallet name/path. WDYT?

laanwj commented at 9:56 am on January 16, 2019:

replace_all is already used in four places. I’d suggest to simply use it here, too. Rewriting it could always be done, it’s irrelevant to his PR.

Edit: Regarding EncodeBase64, no I don’t think that’s useful here. It’d add another hoop to client scripts to decode it again, adding complexity and potential bugs. It should be possible to quote correctly. It’s not trivail but also we’re not the first software to do this.

DrahtBot added the label Needs rebase on Jan 21, 2019

laanwj commented at 3:22 pm on February 6, 2019: member

What is the status here? This is still tagged for 0.18.

laanwj removed this from the milestone 0.18.0 on Feb 6, 2019

ryanofsky commented at 4:08 pm on February 6, 2019: member

What is the status here? This is still tagged for 0.18.

I don’t think this this can merged currently due to concerns in #13339 (review).

I think the current shell escaping in this PR looks good when WIN32 isn’t defined. But the windows shell escaping is cumbersome and probably broken and uses unlicensed, undocumented, and uncredited(!) code. I think encoding is the better way to go on windows, and good options would be

URI encoding with evhttp_uriencode and ^% replacement for cmd.exe. Example: My Wallet! as My^%20Wallet^%21.
C encoding. Example: My Wallet! as My\x20Wallet\x21
Base64 encoding. My Wallet! as TXkgV2FsbGV0IQ==

Again on UNIX I think it would be most natural to stick with plain shell escaping and not require any decoding on the receiving end.

promag commented at 4:23 pm on February 6, 2019: member

If boost process was an option I would expose an env var with the plain wallet name, after all %w is a bit awkward.

luke-jr commented at 5:30 pm on February 6, 2019: member

I think encoding is the better way to go on windows,

Ugh, let’s not have different interfaces for *nix vs Windows please…

Considering how Windows works (application just gets a long string and parses it how it likes), maybe we should just use *nix quoting and make the target application deal with it?

ryanofsky commented at 6:32 pm on February 6, 2019: member

maybe we should just use *nix quoting and make the target application deal with it?

This won’t work for two reasons:

cmd.exe is script interpreter that uses special characters. The target application will have no way of accessing the original string if cmd.exe has interpreted and executed it.
In many scripting environments it is not straightforward to get access to the command line string even if it hasn’t been mangled by the COMSPEC interpreter. You need to bypass the argv parsing and call the GetCommandLine API, which is easy to do from a C program, but awkward to do from other languages. In python you either need to use ctypes, or install win32 packages which aren’t bundled with python. I think the situation is similar with other languages like ruby, java, javascript. By contrast, it is trivial to add: if sys.platform == "win32": wallet = urllib.unquote(wallet) in python if you need your hook script to have windows compatibility.

If boost process was an option I would expose an env var with the plain wallet name, after all %w is a bit awkward.

This is a nice idea, and you don’t need boost to implement it since you can rely on the script interpreter.

It would make Luke happy I think because it would avoid creating any differences between platforms on the receiving end and avoid the need for decoding on the receiving end on windows. It would require adding some tricky code that hasn’t been written yet to escape for cmd.exe, but basically you could say:

0#ifdef WIN32
1runCommand(strprintf("SET BITCOIN_WALLET=%s & %s", CmdExeEscape(wallet_name), script_command));
2#else
3runCommand(strprintf("BITCOIN_WALLET=%s %s", ShellEscape(wallet_name), script_command));
4#endif

I might slightly prefer one of my previous suggestions from #13339 (comment) over this just avoid having a CmdExeEscape function in bitcoin. But I think at this point there are number of reasonable options to choose from.

luke-jr commented at 2:58 am on February 7, 2019: member

Passing the environment on the command line would need to be escaped too.

What’s wrong with putenv?

promag commented at 3:05 am on February 7, 2019: member

It’s not thread safe, we would have to serialize command executions.

ryanofsky commented at 4:54 am on February 7, 2019: member

Passing the environment on the command line would need to be escaped too.

It would only need to be escaped for cmd.exe interpreter, not for the argv parsing that happens in msvcrt. Escaping only for cmd.exe is simpler because msvcrt escaping is crazy by comparison and can also confuse cmd.exe (see the msdn article or the cmd.exe+msvcrt escaping code I wrote years ago). Also IMO, it is much more reasonable to require COMSPEC=cmd.exe than to require client program to use a particular C runtime.

To summarize alternatives for escaping:

Pass as shell escaped argv, implementing cmd.exe + msvcrt escaping on windows. This requires ugliest, most fragile escaping and will be broken with a different comspec, or with a different c library.
Pass as shell escaped environment variable, #13339 (comment). This requires simpler escaping, just prefixing special characters with ^, and will be broken with a different comspec but not a different c library.
Pass as url-encoded (%21) or c-string escaped (\x21) or base64 encoded argument, #13339 (comment). This is probably the simplest thing for bitcoin to implement, but would a put small burden on client programs, which would have to call urllib.unquote or a similar function, on windows only.

My preference would probably be 3, then 2, then 1. And of course it is possible to avoid escaping entirely if you are willing to call CreateProcess directly, or use a library which does this like boost. I think all of these options have some drawbacks, but they mostly aren’t terrible and it’s not like you don’t have options to pick from.

jb55 commented at 0:49 am on May 24, 2019: member

Concept ACK. I ran into this issue recently with an emailer walletnotify script. I needed the wallet name for the subsequent gettransaction call inside the script.

jb55 commented at 9:25 am on May 24, 2019: member

Hmm after reading the comments, is walletnotify deprecated in favor of listsincetx + https://github.com/bitcoin/bitcoin/pull/13262/commits/2dadbc883ecf37c42ec29c904b19cc2e5fe39b30 ?

promag commented at 10:34 am on June 22, 2019: member

I still think this makes sense regardless of other options.

I’m kind of waiting for boost process support.

I wonder if we can ignore windows for now?

jb55 commented at 8:40 pm on June 22, 2019: member

FWIW I’ve been using this patch on my linux machine in the meantime, seems to be working.

promag force-pushed on Jun 27, 2019

DrahtBot removed the label Needs rebase on Jun 27, 2019

DrahtBot added the label Needs rebase on Jul 7, 2019

promag force-pushed on Jul 7, 2019

DrahtBot removed the label Needs rebase on Jul 7, 2019

DrahtBot added the label Needs rebase on Aug 2, 2019

laanwj deleted a comment on Sep 30, 2019

laanwj commented at 12:09 pm on September 30, 2019: member

I wonder if we can ignore windows for now?

Yes, please, this has been held up for long enough now. Let’s do this for UNIX only for now. Windows support can be added later.

promag commented at 12:13 pm on September 30, 2019: member

For 0.19?

laanwj commented at 2:39 pm on September 30, 2019: member

It’s too late for that, and unfair with regard to the feature freeze. But the way this was going it was going to miss 0.20 too :woman_shrugging:

promag commented at 2:44 pm on September 30, 2019: member

It’s too late for that, and unfair with regard to the feature freeze

As I thought.. I’ll get back to this after branching off 0.19, thanks.

jb55 commented at 4:35 pm on September 30, 2019: member

If I’m understanding correctly, this should still work on windows except in the case where the walletname has certain characters?

practicalswift commented at 4:47 pm on September 30, 2019: contributor

The potential licensing issue which @luke-jr commented on needs to be resolved as well?

promag force-pushed on Oct 1, 2019

promag commented at 2:23 pm on October 1, 2019: member

Rebased and dropped shell escape support for windows.

DrahtBot removed the label Needs rebase on Oct 1, 2019

jb55 commented at 5:25 pm on October 1, 2019: member

perhaps we should log a warning/error if the walletname contains those characters on windows?

promag commented at 9:18 pm on October 1, 2019: member

@jb55 honestly I was tempted to simply not suport %w in windows.

jb55 commented at 11:49 pm on October 1, 2019: member

honestly I was tempted to simply not suport %w in windows.

really? as long as they don’t have a weird wallet name it should work fine, no?

laanwj commented at 8:07 am on October 2, 2019: member

@jb55 honestly I was tempted to simply not suport %w in windows.

I think that’s for the best for now. Simply not escaping could be dangerous, and anything else will bring in lots of logic and bikeshedding around it again, just on how to cripple the functionality.

(make sure to mention in the –help that it’s only supported on UNIX)

Hopefully someone else will pick it up for windows later.

promag commented at 8:48 am on October 2, 2019: member

@laanwj will updated accordingly, thanks.

ryanofsky commented at 1:33 pm on October 2, 2019: member

Hopefully someone else will pick it up for windows later.

If someone does want to pick this up later, the various solutions that were discussed are listed in #13339 (comment). Any of these would work and be safe and at least get some acks.

Another alternative that would work would be to build on #15382 and use boost process.

in src/util/system.cpp:1030 in 826718490f outdated

1126@@ -1126,6 +1127,15 @@ fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
1127 }
1128 #endif
1129 
1130+#ifndef WIN32
1131+std::string ShellEscape(const std::string& arg)
1132+{
1133+    std::string escaped = arg;
1134+    boost::replace_all(escaped, "'", "'\"'\"'");

luke-jr commented at 6:56 pm on October 21, 2019:

I wonder if "'\\''" would be better

in src/wallet/wallet.cpp:845 in 826718490f outdated

1155@@ -1156,6 +1156,11 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
1156     if (!strCmd.empty())
1157     {
1158         boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
1159+#ifdef WIN32
1160+        boost::replace_all(strCmd, "%w", GetName());
1161+#else
1162+        boost::replace_all(strCmd, "%w", ShellEscape(GetName()));
1163+#endif

luke-jr commented at 8:32 pm on October 21, 2019:

Prefer making ShellEscape a noop on WIN32 over conditionals everywhere…

promag commented at 12:13 pm on December 13, 2019:

You mean identity.

ryanofsky commented at 1:55 pm on December 13, 2019:

Please don’t do this is. Please leave behavior unchanged on windows in this PR, and do not perform any substitution, as laanwj suggested #13339 (review) and #13339 (comment)

We are talking about an obscure new feature. Better to not support it on windows, than to implement it an a half baked way where it will sometimes work, sometimes mangle the string it is trying to pass, and sometimes turn on your webcam, upload your private keys, and reset your gmail password.

jb55 commented at 4:33 pm on December 13, 2019:

sometimes mangle the string it is trying to pass, and sometimes turn on your webcam, upload your private keys, and reset your gmail password

if this is the case we should probably have tests that cover these broken scenarios?

ryanofsky commented at 5:21 pm on December 13, 2019:

re: #13339 (review)

sometimes mangle the string it is trying to pass, and sometimes turn on your webcam, upload your private keys, and reset your gmail password

if this is the case we should probably have tests that cover these broken scenarios?

I’m confused figuring out if this is a joke or a serious suggestion. But to take this straightforwardly, writing a python test to verify that lack of escaping on windows allows running arbitrary shell commands over RPC would just require writing a python test that:

Starts bitcoind with -walletnotify="echo %w"
Calls createwallet with wallet name & echo It is fun to run shell commands from RPC > funfile
Triggering a wallet notification
Checking that a new file funfile exists with contents It is fun to run shell commands from RPC

This would (I hope obviously) not be my preferred approach. My preferred approach would be the one suggested by laanwj #13339 (review) and #13339 (comment) implemented like:

 0diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
 1index 57b9fdd4c1f..bb18efc9412 100644
 2--- a/src/wallet/init.cpp
 3+++ b/src/wallet/init.cpp
 4@@ -59,7 +59,7 @@ void WalletInit::AddWalletOptions() const
 5     gArgs.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 6     gArgs.AddArg("-walletdir=<dir>", "Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 7 #if HAVE_SYSTEM
 8-    gArgs.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes (%s in cmd is replaced by TxID and %w is replaced by wallet name)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 9+    gArgs.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes. %s in cmd is replaced by TxID and %w is replaced by wallet name. %w is not currently implmented on windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
10 #endif
11     gArgs.AddArg("-walletrbf", strprintf("Send transactions with full-RBF opt-in enabled (RPC only, default: %u)", DEFAULT_WALLET_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
12     gArgs.AddArg("-zapwallettxes=<mode>", "Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup"
13diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
14index a8ae5ef62c4..ad0a918d7aa 100644
15--- a/src/wallet/wallet.cpp
16+++ b/src/wallet/wallet.cpp
17@@ -1156,9 +1156,12 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
18     if (!strCmd.empty())
19     {
20         boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
21-#ifdef WIN32
22-        boost::replace_all(strCmd, "%w", GetName());
23-#else
24+#ifndef WIN32
25+        // Substituting the wallet name isn't currently supported on windows
26+        // because windows shell escaping has not been implemented yet:
27+        // [#13339 (comment)](/bitcoin-bitcoin/13339/#issuecomment-537384875)
28+        // A few ways it could be implemented in the future are described in:
29+        // [#13339 (comment)](/bitcoin-bitcoin/13339/#issuecomment-461288094)
30         boost::replace_all(strCmd, "%w", ShellEscape(GetName()));
31 #endif
32         std::thread t(runCommand, strCmd);

jb55 commented at 6:30 pm on December 13, 2019:

you’re describing a different issue than what I was thinking of, could you have a wallet name that would get passed incorrectly if it was not escaped properly? for instance with a wallet named wallet, wallet 1, wallet 2. I’m just trying to understand all the different ways the current code would break, and suggested tests that document this, so no I was not joking.

jb55 commented at 7:00 pm on December 13, 2019:

Regardless, I believe that you’re right that this should be a noop if we’re not supporting it on windows…

ryanofsky commented at 7:52 pm on December 13, 2019:

Ah I see. I hope that comment didn’t come off wrong. I believe an example of a name that’d be passed incorrectly would be a wallet named eggs & bacon. With cacc63bed101ecb56679695e1b490fd76f83bd50 and walletnotify=walletnotify.exe %w, I think the notifications script would see command line string walletnotify.exe 'eggs, because the cmd.exe command interpreter on windows treats & as a separator between commands, and doesn’t treat single quotes specially (only double quotes).

in test/functional/feature_notifications.py:16 in 826718490f outdated

12@@ -13,13 +13,19 @@
13     connect_nodes,
14 )
15 
16+# Linux allow all characters other than \x00

luke-jr commented at 8:39 pm on October 21, 2019:

Even with various locales? How is Unicode handled?

ryanofsky commented at 4:35 pm on January 9, 2020:

Even with various locales? How is Unicode handled?

Should be no issue here. This remains in unicode and utf8 formats until it hits bitcoin filesystem code which uses utf8 directly on non-windows systems, and converts to wide characters for windows apis.

in test/functional/feature_notifications.py:20 in 826718490f outdated

12@@ -13,13 +13,19 @@
13     connect_nodes,
14 )
15 
16+# Linux allow all characters other than \x00
17+# Windows disallow control characters (0-31) and /\?%:|"<>
18+FILE_CHAR_START = 32 if os.name == 'nt' else 1
19+FILE_CHAR_END = 128
20+FILE_CHAR_BLACKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'

luke-jr commented at 8:39 pm on October 21, 2019:

Would be nice to autodetect this in the test.

luke-jr commented at 8:42 pm on October 21, 2019:

Until escaping works on Windows, we should exclude spaces, tabs, and single quotes here too…

ryanofsky commented at 5:02 pm on January 9, 2020:

What about macOS, BSD, Amiga, etc?

Would be nice to autodetect this in the test.

It shouldn’t be too hard to autodetect by just writing to files and seeing what succeeds, but in the worst case this test just doesn’t work on an obscure platform and the fix is trivial, so I don’t think it’s is a big deal

in test/functional/feature_notifications.py:17 in 826718490f outdated

12@@ -13,13 +13,19 @@
13     connect_nodes,
14 )
15 
16+# Linux allow all characters other than \x00
17+# Windows disallow control characters (0-31) and /\?%:|"<>

luke-jr commented at 8:40 pm on October 21, 2019:

What about macOS, BSD, Amiga, etc?

in test/functional/feature_notifications.py:33 in 826718490f outdated

23     def set_test_params(self):
24         self.num_nodes = 2
25         self.setup_clean_chain = True
26 
27     def setup_network(self):
28+        self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHAR_BLACKLIST)

luke-jr commented at 8:41 pm on October 21, 2019:

This looks potentially dangerous…

luke-jr commented at 9:23 pm on October 21, 2019:

Maybe prefix it with “wallet” so you can tab-complete if you ever need to touch it…

ryanofsky commented at 4:23 pm on January 9, 2020:

This looks potentially dangerous…

Maybe prefix it with “wallet” so you can tab-complete if you ever need to touch it…

I like luke’s suggestion to prefix with something. Maybe a prefix like “feature_notifications” to give a hint where the file comes from, and to help with tab completion

promag commented at 7:23 pm on January 9, 2020:

~~Just prefixed with wallet.~~ not yet

in test/functional/feature_notifications.py:19 in 826718490f outdated

12@@ -13,13 +13,19 @@
13     connect_nodes,
14 )
15 
16+# Linux allow all characters other than \x00
17+# Windows disallow control characters (0-31) and /\?%:|"<>
18+FILE_CHAR_START = 32 if os.name == 'nt' else 1
19+FILE_CHAR_END = 128

luke-jr commented at 8:43 pm on October 21, 2019:

Why 128? If we’re aiming for only ASCII, it should end at 127…? (Yes, I see that the range excludes the end char, but the name for this implies inclusive)

in src/util/system.cpp:1026 in 826718490f outdated

1126@@ -1126,6 +1127,15 @@ fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
1127 }
1128 #endif
1129 
1130+#ifndef WIN32

luke-jr commented at 9:27 pm on October 21, 2019:

Maybe leaving the UNIX quoting in place is better than nothing at all?

ryanofsky commented at 5:20 pm on October 22, 2019:

Maybe leaving the UNIX quoting in place is better than nothing at all?

I think there’s some agreement to not support %w on windows, and leave windows escaping for a future PR: #13339 (comment), instead of implementing something broken. This is the simplest and least dangerous approach.

laanwj commented at 11:24 am on October 23, 2019:

Right. Let’s please not repeat this argument. If we do, this will never make any progress. This makes the explicit choice to not support %w on windows for now.

laanwj commented at 2:25 pm on December 13, 2019: member

ACK 826718490fb22d7908ebf23ffc789d2949bb44d6

promag commented at 3:42 am on January 6, 2020: member

BTW #17878 adds support for wallet ZMQ notifications which include the wallet name.

in src/wallet/wallet.cpp:1160 in cacc63bed1 outdated

1155@@ -1156,6 +1156,11 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
1156     if (!strCmd.empty())
1157     {
1158         boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
1159+#ifdef WIN32
1160+        boost::replace_all(strCmd, "%w", GetName());

ryanofsky commented at 9:36 pm on January 8, 2020:

This line is unsafe since it can corrupt wallet names and trigger arbitrary shell commands to run from an RPC. Suggested fix from previous thread #13339 (review) is 26e0017bcfca722892eaf79d459d80f57a599863

ryanofsky changes_requested

promag force-pushed on Jan 9, 2020

promag commented at 4:14 pm on January 9, 2020: member

Thanks @ryanofsky, rebased, squashed and added you as co author. Updated test to take into account behavior difference in windows.

in test/functional/feature_notifications.py:43 in 1c335d5828 outdated

38@@ -33,7 +39,8 @@ def setup_network(self):
39                             "-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s'))],
40                            ["-blockversion=211",
41                             "-rescan",
42-                            "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%s'))]]
43+                            "-wallet={}".format(self.wallet),
44+                            "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%w_%s' if os.name != 'nt' else '%s'))]]

ryanofsky commented at 5:10 pm on January 9, 2020:

Instead of repeating the os.name != 'nt' checks and "{}_{}" formatting throughout the test, maybe just define a helper function and call it here and below:

0def notify_outputname(walletname, txid):
1    return txid if os == "nt" else "{}_{}".format(walletname, txid)

ryanofsky commented at 5:34 pm on January 9, 2020:

I misunderstood what this code was doing when I wrote the suggestion above. But I guess you could still call the function here as notify_outputname("%w", "%s")

promag commented at 7:05 pm on January 9, 2020:

Yes that’s a neat suggestion, will also add a comment.

ryanofsky commented at 10:50 pm on January 31, 2020:

Pushed amended commit https://github.com/ryanofsky/bitcoin/commit/4e9efac678a9c0ea4e4c7dd956ea036ae6cf17ec (diff)

ryanofsky commented at 5:27 pm on January 9, 2020: member

Code review ACK a89a1701ed1f9d8de26a83776003d2ff41cb21b5

ryanofsky approved

jb55 commented at 3:42 am on January 10, 2020: member

Linux Tested ACK 1c335d5828e27833d4afd5e23cca9c28e6ccf1cc

in src/wallet/init.cpp:65 in ff93682917 outdated

61@@ -62,7 +62,7 @@ void WalletInit::AddWalletOptions() const
62     gArgs.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
63     gArgs.AddArg("-walletdir=<dir>", "Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET);
64 #if HAVE_SYSTEM
65-    gArgs.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
66+    gArgs.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes. %s in cmd is replaced by TxID and %w is replaced by wallet name. %w is not currently implmented on windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

meshcollider commented at 10:35 am on January 15, 2020:

Typo in “implemented”

promag commented at 11:48 am on January 15, 2020:

ops, fixed.

wallet: Replace %w by wallet name in -walletnotify script

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

9a5b5ee81f

promag force-pushed on Jan 15, 2020

fanquake added this to the "In progress" column in a project

test: Check wallet name in -walletnotify script 4e9efac678

promag force-pushed on Feb 17, 2020

promag commented at 0:40 am on February 17, 2020: member

Replaced with @ryanofsky amended commit.

laanwj commented at 10:58 am on February 17, 2020: member

ACK 4e9efac678a9c0ea4e4c7dd956ea036ae6cf17ec

laanwj referenced this in commit 051439813e on Feb 17, 2020

laanwj merged this on Feb 17, 2020

laanwj closed this on Feb 17, 2020

promag deleted the branch on Feb 17, 2020

sidhujag referenced this in commit 0f86b36c7e on Feb 18, 2020

fanquake moved this from the "In progress" to the "Done" column in a project

jasonbcox referenced this in commit a8eab50048 on Oct 26, 2020

sidhujag referenced this in commit d43bf96cab on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

wallet: Replace %w by wallet name in -walletnotify script #13339

Conflicts