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

Merged
merged 11 commits into from Jan 3, 2017

Projects

None yet

8 participants

@luke-jr
Member
luke-jr commented Oct 4, 2016

Rescuing uncontroversial parts of #5891

Alternative to #8746

@laanwj
Member
laanwj commented Oct 4, 2016

Concept ACK

@MarcoFalke MarcoFalke added the GUI label Oct 4, 2016
@sipa
Member
sipa commented Oct 4, 2016

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
Contributor
achow101 commented Oct 4, 2016 edited

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
Contributor

Concept ACK

@luke-jr
Member
luke-jr commented Oct 4, 2016

@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
Contributor
achow101 commented Oct 4, 2016

@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
Member
laanwj commented Oct 4, 2016

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
Member
luke-jr commented Oct 4, 2016 edited

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
Member
  1. The passphrase is still visible in the console as executing command when calling walletpassphrase (should this be done in a different pull request?)
  2. 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.

bildschirmfoto 2016-10-09 um 10 15 55

@luke-jr
Member
luke-jr commented Nov 16, 2016

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.

+ << "walletpassphrase"
+ << "walletpassphrasechange"
+ << "encryptwallet";
+
@jonasschnelli
Member

Tested a bit.
Needs coverage for importmulti now.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@luke-jr luke-jr GUI/RPCConsole: Include importmulti in history sensitive-command filter
Github-Pull: #8877
Rebased-From: 5c7fc22
19321e1
jonasschnelli and others added some commits Mar 13, 2015
@jonasschnelli
Member

Looks good. Squash/combine some commits?

@luke-jr
Member
luke-jr commented Jan 3, 2017

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

@jonasschnelli
Member

Tested ACK 8562792

@btcdrak
Member
btcdrak commented Jan 3, 2017

Tested ACK 8562792

@jonasschnelli jonasschnelli merged commit 8562792 into bitcoin:master Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasschnelli jonasschnelli added a commit that referenced this pull request Jan 3, 2017
@jonasschnelli jonasschnelli Merge #8877: Qt RPC console: history sensitive-data filter, and savin…
…g input line when browsing history


8562792 GUI/RPCConsole: Include importmulti in history sensitive-command filter (Luke Dashjr)
ff77faf Qt/RPCConsole: Use RPCParseCommandLine to perform command filtering (Luke Dashjr)
a79598d Qt/Test: Make sure filtering sensitive data works correctly in nested commands (Luke Dashjr)
629cd42 Qt/RPCConsole: Teach RPCParseCommandLine how to filter out arguments to sensitive commands (Luke Dashjr)
e2d9213 Qt/RPCConsole: Make it possible to parse a command without executing it (Luke Dashjr)
1755c04 Qt/RPCConsole: Truncate filtered commands to just the command name, rather than skip it entirely in history (Luke Dashjr)
d80a006 Qt/RPCConsole: Add signmessagewithprivkey to list of commands filtered from history (Luke Dashjr)
afde12f Qt/RPCConsole: Refactor command_may_contain_sensitive_data function out of RPCConsole::on_lineEdit_returnPressed (Luke Dashjr)
de8980d Bugfix: Do not add sensitive information to history for real (Luke Dashjr)
9044908 Qt/RPCConsole: Don't store commands with potentially sensitive information in the history (Jonas Schnelli)
fc95daa Qt/RPCConsole: Save current command entry when browsing history (Jonas Schnelli)
6dc4c43
@paveljanik
Contributor
paveljanik commented Jan 3, 2017 edited

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

qt/rpcconsole.cpp:173:56: warning: declaration shadows a local variable [-Wshadow]
    auto add_to_current_stack = [&](const std::string& curarg) {
                                                       ^
qt/rpcconsole.cpp:167:17: note: previous declaration is here
    std::string curarg;
                ^
1 warning generated.
@luke-jr
Member
luke-jr commented Jan 4, 2017

@paveljanik That looks like a bug in the compiler?

@paveljanik
Contributor

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

@jonasschnelli
Member

@paveljanik: Maybe write a fix PR?

@luke-jr
Member
luke-jr commented Jan 5, 2017

@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
Member
sipa commented Jan 5, 2017

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

@luke-jr
Member
luke-jr commented Jan 6, 2017

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

@sipa
Member
sipa commented Jan 6, 2017

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

@paveljanik
Contributor

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

@luke-jr
Member
luke-jr commented Jan 9, 2017

Sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment