Qt RPC console: history sensitive-data filter, and saving input line when browsing history

luke-jr commented at 5:10 am on October 4, 2016: member

Rescuing uncontroversial parts of #5891

Alternative to #8746

luke-jr force-pushed on Oct 4, 2016

laanwj commented at 9:11 am on October 4, 2016: member

Concept ACK

MarcoFalke added the label GUI on Oct 4, 2016

sipa commented at 12:35 pm on October 4, 2016: member

Concept ACK. Maybe add a comment that signrawtransaction can be removed from this list if it’s ever split into a wallet call and a utility call.

achow101 commented at 12:38 pm on October 4, 2016: member

If we’re adding privkey stuff here, then signmessagewithprivkey needs to be added.

Won’t those commands not show up in history? I think it should at least show that the command happened.

paveljanik commented at 1:55 pm on October 4, 2016: contributor

Concept ACK

luke-jr commented at 2:10 pm on October 4, 2016: member

@sipa If signrawtransaction is split, there will likely still be users trying to use the old usage for at least one release, so I’m not sure it makes sense to remove it from the filter, at least not at the same time? @achow101 I agree it would be better to add dummy history items, but comments on previous PRs seem to suggest that is a source of disagreement.

Added signmessagewithprivkey to the filter.

achow101 commented at 2:22 pm on October 4, 2016: member

@luke-jr instead of masking out the arguments as I did in my PR, what if you just added the command name to the history. Since all the commands pass through that filter method, you could have it return the string that goes into the history. For non-filtered commands, it returns the command itself. For filtered ones, it just returns the command name.

laanwj commented at 3:30 pm on October 4, 2016: member

Instead of masking out the arguments as I did in my PR, what if you just added the command name to the history.

Would work for me. My main comment on doing it @achow101 ’s way with masking individual arguments is just that that is too brittle and hard to maintain. If having just the command name to the history is useful in any way, which I doubt a bit with the incredible autocompletion that the debug console has these days, you could do that.

luke-jr commented at 3:38 pm on October 4, 2016: member

I consider it useful to have any placeholder in history, since otherwise one might <up>-<enter> and get the second-to-last execution instead. At least with a dummy placeholder, they get an error/help.

jonasschnelli commented at 8:19 am on October 9, 2016: contributor

The passphrase is still visible in the console as executing command when calling walletpassphrase (should this be done in a different pull request?)
Nested commands makes this a little bit more complex. Executing “a dump” getnewaddress(walletpassphrase(test)) results in executing walletpassphrase(test) but actually not “hiding” the passphrase from the history.

luke-jr force-pushed on Nov 16, 2016

luke-jr commented at 12:40 pm on November 16, 2016: member

Rebased and taught it to handle nested commands. When a filtered command is encountered, all its parameters are replaced with “(…)” in the command history. Unit tests now check this.

luke-jr force-pushed on Nov 16, 2016

in src/qt/rpcconsole.cpp: in 84eabc9af0 outdated

72+    << "signmessagewithprivkey"
73+    << "signrawtransaction"
74+    << "walletpassphrase"
75+    << "walletpassphrasechange"
76+    << "encryptwallet";
77+

jonasschnelli commented at 2:34 pm on November 22, 2016:

nit: newline

luke-jr commented at 11:36 pm on November 22, 2016:

?

jonasschnelli commented at 2:42 pm on November 22, 2016: contributor

Tested a bit. Needs coverage for importmulti now.

luke-jr referenced this in commit 19321e129f on Dec 21, 2016

Qt/RPCConsole: Save current command entry when browsing history

Shell-like, but doesn't store changed history commands until executing it.

fc95daa97f

Qt/RPCConsole: Don't store commands with potentially sensitive information in the history

Filters importprivkey, signrawtransaction, walletpassphrase, walletpassphrasechange, and encryptwallet

9044908636

Bugfix: Do not add sensitive information to history for real

Original code was missing braces, and short-circuited before checking everything after importprivkey

de8980df9d

Qt/RPCConsole: Refactor command_may_contain_sensitive_data function out of RPCConsole::on_lineEdit_returnPressed afde12f265

Qt/RPCConsole: Add signmessagewithprivkey to list of commands filtered from history d80a00660f

Qt/RPCConsole: Truncate filtered commands to just the command name, rather than skip it entirely in history 1755c04576

Qt/RPCConsole: Make it possible to parse a command without executing it e2d9213c32

Qt/RPCConsole: Teach RPCParseCommandLine how to filter out arguments to sensitive commands 629cd42364

Qt/Test: Make sure filtering sensitive data works correctly in nested commands a79598ddf4

Qt/RPCConsole: Use RPCParseCommandLine to perform command filtering ff77faf480

GUI/RPCConsole: Include importmulti in history sensitive-command filter 8562792095

luke-jr force-pushed on Dec 29, 2016

jonasschnelli commented at 8:35 am on January 3, 2017: contributor

Looks good. Squash/combine some commits?

luke-jr commented at 2:39 pm on January 3, 2017: member

Rather not squash… They look reasonably logical progression, and squashing is a bad practice anyway.

jonasschnelli commented at 3:19 pm on January 3, 2017: contributor

Tested ACK 8562792095a78fdef2dacc3c5e32c41206df4c87

btcdrak commented at 3:55 pm on January 3, 2017: contributor

Tested ACK 8562792095a78fdef2dacc3c5e32c41206df4c87

jonasschnelli merged this on Jan 3, 2017

jonasschnelli closed this on Jan 3, 2017

jonasschnelli referenced this in commit 6dc4c43d32 on Jan 3, 2017

paveljanik commented at 9:55 pm on January 3, 2017: contributor

Wshadow statistics: 1 qt/rpcconsole.cpp:173:56: warning: declaration shadows a local variable [-Wshadow]

0qt/rpcconsole.cpp:173:56: warning: declaration shadows a local variable [-Wshadow]
1    auto add_to_current_stack = [&](const std::string& curarg) {
2                                                       ^
3qt/rpcconsole.cpp:167:17: note: previous declaration is here
4    std::string curarg;
5                ^
61 warning generated.

luke-jr commented at 2:48 pm on January 4, 2017: member

@paveljanik That looks like a bug in the compiler?

paveljanik commented at 7:48 pm on January 5, 2017: contributor

Why do you think so? It is clear that curarg inside the block shadows curarg outside.

jonasschnelli commented at 7:57 pm on January 5, 2017: contributor

@paveljanik: Maybe write a fix PR?

luke-jr commented at 9:38 pm on January 5, 2017: member

@paveljanik It’s a different scope, and lambda functions do not inherit the scope of the calling function beyond what you tell it to.

sipa commented at 9:41 pm on January 5, 2017: member

@luke-jr Not by default, but your lambda has capture list [&], which means it does capture everything.

luke-jr commented at 0:43 am on January 6, 2017: member

I thought [&] was only supposed to capture stuff actually referenced?

sipa commented at 0:47 am on January 6, 2017: member

@luke-jr Well, yes. But if -Wshadow wants to catch potential errors resulting from variables being identically named, it should absolutely catch this.

paveljanik commented at 1:23 pm on January 9, 2017: contributor

@luke-jr What name do you want me to use inside lambda instead of curarg? strArg?

luke-jr commented at 5:22 pm on January 9, 2017: member

Sure

codablock referenced this in commit 1d83352dad on Jan 18, 2018

andvgal referenced this in commit be09450837 on Jan 6, 2019

CryptoCentric referenced this in commit 1b53750c4a on Feb 26, 2019

MarcoFalke locked this on Sep 8, 2021

Qt RPC console: history sensitive-data filter, and saving input line when browsing history #8877