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

luke-jr · 2016-10-04T05:10:26Z

Rescuing uncontroversial parts of #5891

Alternative to #8746

laanwj · 2016-10-04T09:11:32Z

Concept ACK

sipa · 2016-10-04T12:35:56Z

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 · 2016-10-04T12:38:01Z

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 · 2016-10-04T13:55:22Z

Concept ACK

luke-jr · 2016-10-04T14:10:01Z

@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 · 2016-10-04T14:22:42Z

@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 · 2016-10-04T15:30:27Z

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 · 2016-10-04T15:38:26Z

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 · 2016-10-09T08:19:51Z

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 · 2016-11-16T12:40:53Z

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.

jonasschnelli · 2016-11-22T14:34:15Z

src/qt/rpcconsole.cpp

nit: newline

jonasschnelli · 2016-11-22T14:42:42Z

Tested a bit.
Needs coverage for importmulti now.

Github-Pull: bitcoin#8877 Rebased-From: 5c7fc2237aa68675858ae17a5b825140f72487da

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

…ation in the history Filters importprivkey, signrawtransaction, walletpassphrase, walletpassphrasechange, and encryptwallet

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

…ut of RPCConsole::on_lineEdit_returnPressed

…d from history

…ather than skip it entirely in history

…to sensitive commands

… commands

jonasschnelli · 2017-01-03T08:35:13Z

Looks good. Squash/combine some commits?

luke-jr · 2017-01-03T14:39:37Z

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

jonasschnelli · 2017-01-03T15:19:44Z

Tested ACK 8562792

btcdrak · 2017-01-03T15:55:21Z

Tested ACK 8562792

…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)

paveljanik · 2017-01-03T21:55:45Z

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 · 2017-01-04T14:48:33Z

@paveljanik That looks like a bug in the compiler?

paveljanik · 2017-01-05T19:48:40Z

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

jonasschnelli · 2017-01-05T19:57:32Z

@paveljanik: Maybe write a fix PR?

luke-jr · 2017-01-05T21:38:53Z

@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 · 2017-01-05T21:41:41Z

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

luke-jr · 2017-01-06T00:43:02Z

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

sipa · 2017-01-06T00:47:36Z

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

paveljanik · 2017-01-09T13:23:03Z

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

luke-jr · 2017-01-09T17:22:34Z

Sure

…d saving 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)

luke-jr force-pushed the qt_console_history_filter branch from 0c83c7d to 313c0b5 Compare October 4, 2016 05:10

maflcko added the GUI label Oct 4, 2016

laanwj mentioned this pull request Oct 4, 2016

[Qt][RPC] Hide passphrases in debug console history #8746

Closed

luke-jr force-pushed the qt_console_history_filter branch 2 times, most recently from 858efb8 to 965bef8 Compare November 16, 2016 12:37

luke-jr force-pushed the qt_console_history_filter branch from 965bef8 to 84eabc9 Compare November 16, 2016 13:09

jonasschnelli reviewed Nov 22, 2016

View reviewed changes

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016

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

19321e1

Github-Pull: bitcoin#8877 Rebased-From: 5c7fc2237aa68675858ae17a5b825140f72487da

jonasschnelli and others added 11 commits December 29, 2016 11:43

Qt/RPCConsole: Save current command entry when browsing history

fc95daa

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

Qt/RPCConsole: Don't store commands with potentially sensitive inform…

9044908

…ation in the history Filters importprivkey, signrawtransaction, walletpassphrase, walletpassphrasechange, and encryptwallet

Bugfix: Do not add sensitive information to history for real

de8980d

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

Qt/RPCConsole: Refactor command_may_contain_sensitive_data function o…

afde12f

…ut of RPCConsole::on_lineEdit_returnPressed

Qt/RPCConsole: Add signmessagewithprivkey to list of commands filtere…

d80a006

…d from history

Qt/RPCConsole: Truncate filtered commands to just the command name, r…

1755c04

…ather than skip it entirely in history

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

e2d9213

Qt/RPCConsole: Teach RPCParseCommandLine how to filter out arguments …

629cd42

…to sensitive commands

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

a79598d

… commands

Qt/RPCConsole: Use RPCParseCommandLine to perform command filtering

ff77faf

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

8562792

luke-jr force-pushed the qt_console_history_filter branch from 5c7fc22 to 8562792 Compare December 29, 2016 11:49

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

paveljanik mentioned this pull request Jan 9, 2017

Rename lambda argument name to prevent shadowing #9496

Merged

jonasschnelli mentioned this pull request Jan 10, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

luke-jr commented Oct 4, 2016

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

sipa commented Oct 4, 2016

Uh oh!

achow101 commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Oct 4, 2016

Uh oh!

luke-jr commented Oct 4, 2016

Uh oh!

achow101 commented Oct 4, 2016

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

luke-jr commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Oct 9, 2016

Uh oh!

luke-jr commented Nov 16, 2016

Uh oh!

jonasschnelli Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 22, 2016

Uh oh!

jonasschnelli commented Jan 3, 2017

Uh oh!

luke-jr commented Jan 3, 2017

Uh oh!

jonasschnelli commented Jan 3, 2017

Uh oh!

btcdrak commented Jan 3, 2017

Uh oh!

paveljanik commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Jan 4, 2017

Uh oh!

paveljanik commented Jan 5, 2017

Uh oh!

jonasschnelli commented Jan 5, 2017

Uh oh!

luke-jr commented Jan 5, 2017

Uh oh!

sipa commented Jan 5, 2017

Uh oh!

luke-jr commented Jan 6, 2017

Uh oh!

sipa commented Jan 6, 2017

Uh oh!

paveljanik commented Jan 9, 2017

Uh oh!

luke-jr commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

achow101 commented Oct 4, 2016 •

edited

Loading

luke-jr commented Oct 4, 2016 •

edited

Loading

paveljanik commented Jan 3, 2017 •

edited

Loading