Password manager internals page: Introduce logger in renderer

Password manager internals page: Introduce logger in renderer

A follow-up to https://codereview.chromium.org/216183008/

Password manager internals page serves as a debugging output from the process of observing submitted password forms and offering the user to save the password. The user will be able to grab the debugging info and pass it onto the Chrome developers to help investigate issues.

This CL introduces:
1) RendererSavePasswordProgressLogger, a specialization of the SavePasswordProgressLogger for the renderer part of the password management code.

2) Additional support in the PasswordManagementClient to use the Logger in the renderer code.

More context in the design doc: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit?usp=sharing

A follow-up CL will introduce actual logging calls.

TBR=inferno@chromium.org
BUG=347927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263002

[vabr (Chromium), 2014-04-09 14:28:34]

Hi Ilya!

Once you find time, please take a look at this follow-up CL to what you approved
yesterday.

Patch set 1 is what it says in the CL description.
Patch set 2 adds two little fixes related to the whole password internals
change. They do not need to be exactly here, but because they are small, I added
them.

Cheers,
Vaclav

[Ilya Sherman, 2014-04-09 20:53:10]

On 2014/04/09 14:28:34, vabr (Chromium) wrote:
> Patch set 2 adds two little fixes related to the whole password internals
> change. They do not need to be exactly here, but because they are small, I
added
> them.

Please do split these off to a separate CL.  I'll happily approve that CL and
tick the CQ box for you.  The benefit of splitting the changes off is (a)
reduced cognitive load for me when reviewing the CL, and (b) greater clarity
when digging through code history.

[Ilya Sherman, 2014-04-09 21:10:07]

Really nice!  Mostly just some nits:

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:186: bool
ChromePasswordManagerClient::IsLoggingActive() const { return logger_; }
nit: clang-format is being updated to format this as three lines, rather than
one.  Please do so here ahead of that change landing upstream.

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:186: bool
ChromePasswordManagerClient::IsLoggingActive() const { return logger_; }
nit: Please implement this as "return !!logger_", so that the returned value is
strictly |true| or |false|, rather than merely a bit pattern that is interpreted
as true.  In general, functions that promise to return a bool value should
return either 0 or 1.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:189:
IPC_MESSAGE_ROUTED1(AutofillHostMsg_SavePasswordProgressLogs,
nit: I'd name this message something like "LogSavePasswordProgress" or
"RecordSavePasswordProgress".  Definitely there should be some verb form in the
name :)

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
File
components/autofill/content/renderer/renderer_save_password_progress_logger.h
(right):

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:21:
// where the PasswordManagerClient can needs to be called over IPC.
nit: "where the PasswordManagerClient can needs to be called over IPC." ->
something more like "which sends logs to the browser process over IPC."

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:25:
RendererSavePasswordProgressLogger(IPC::Sender* sender, int routing_id);
nit: Docs, please.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:34:
IPC::Sender* const sender_;
nit: Please document lifetime expectations.

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.h:102:
PasswordManagerClient* client() const { return client_; }
This method returns a non-const pointer, so the method should also be non-const.
 A const method should not be able to lead to any externally observable
side-effects.

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:71: //
displayed. Return value false means they will be dropped.
nit: "Returns true if logs recorded via LogSavePasswordProgress will be
displayed, and false otherwise."

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:72: virtual
bool IsLoggingActive() const;
Should this be pure virtual?  Ditto for the two methods above, actually.

Actually actually, are these methods needed on the base class at all?  Can they
perhaps be moved into the ContentPasswordManagerClient class instead?

[vabr (Chromium), 2014-04-10 08:51:24]

Thanks, Ilya!

Sorry about squashing in the unrelated fixes, they are gone again (and in fact
in the CQ right now: https://codereview.chromium.org/232663002/).

I addressed all of your comments, except for the last one in
components/password_manager/core/browser/password_manager_client.h -- please let
me know what you think about my response there.

Thanks!
Vaclav

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:186: bool
ChromePasswordManagerClient::IsLoggingActive() const { return logger_; }
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: clang-format is being updated to format this as three lines, rather than
> one.  Please do so here ahead of that change landing upstream.

Thanks, that's good to know!
Done.

https://codereview.chromium.org/228263004/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:186: bool
ChromePasswordManagerClient::IsLoggingActive() const { return logger_; }
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: Please implement this as "return !!logger_", so that the returned value
is
> strictly |true| or |false|, rather than merely a bit pattern that is
interpreted
> as true.  In general, functions that promise to return a bool value should
> return either 0 or 1.

I agree, and even the compiler of the win_chromium_compile_dbg bot flagged this
as an error.
Instead of prefixing "!!", I appended "!= NULL". I find the former dangerously
near to just "!", whereas the latter reads clearly. Let me know if that's not
OK.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:189:
IPC_MESSAGE_ROUTED1(AutofillHostMsg_SavePasswordProgressLogs,
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: I'd name this message something like "LogSavePasswordProgress" or
> "RecordSavePasswordProgress".  Definitely there should be some verb form in
the
> name :)

Done.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
File
components/autofill/content/renderer/renderer_save_password_progress_logger.h
(right):

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:21:
// where the PasswordManagerClient can needs to be called over IPC.
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: "where the PasswordManagerClient can needs to be called over IPC." ->
> something more like "which sends logs to the browser process over IPC."

Done.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:25:
RendererSavePasswordProgressLogger(IPC::Sender* sender, int routing_id);
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/228263004/diff/110001/components/autofill/con...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:34:
IPC::Sender* const sender_;
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: Please document lifetime expectations.

Done.

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.h:102:
PasswordManagerClient* client() const { return client_; }
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> This method returns a non-const pointer, so the method should also be
non-const.
>  A const method should not be able to lead to any externally observable
> side-effects.

Done.
Personally, I would agree with you if the client was owned by the manager. But
that's not the case (the client actually owns the manager), so my personal
opinion is still in favour of const. But I don't feel strongly about it, hence I
removed it.

(I also split the line into three, following your note about changes in
clang-format earlier).

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:71: //
displayed. Return value false means they will be dropped.
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> nit: "Returns true if logs recorded via LogSavePasswordProgress will be
> displayed, and false otherwise."

Done.

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:72: virtual
bool IsLoggingActive() const;
On 2014/04/09 21:10:08, Ilya Sherman wrote:
> Should this be pure virtual?  Ditto for the two methods above, actually.

I'm not sure. My reason for providing default implementation (for all 3 of them)
was that a PasswordManagerClient works still reasonably if the logging feature
is off (whereas it cannot if it has no Prefs or PasswordStore, for example).

> 
> Actually actually, are these methods needed on the base class at all?  Can
they
> perhaps be moved into the ContentPasswordManagerClient class instead?

I'd like to keep them up here, because I expect we'll need this kind of logging
on the mobile platforms as well (note: there is no ContentPasswordManagerClient,
it's ChromePasswordManagerClient, so excluding also Android). Also, if I move it
to the embedder, I'd need to move the SavePasswordProgressLogger and children
there as well, as they use these methods.

[Ilya Sherman, 2014-04-10 09:23:07]

LGTM with two final change requests addressed.  Apologies for the small essay on
const-correctness, but I've actually seen this issue come up a fair bit in
Chromium code.  It's subtle, and so I think it's useful to explain in a bit more
detail :)

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.h:102:
PasswordManagerClient* client() const { return client_; }
On 2014/04/10 08:51:25, vabr (Chromium) wrote:
> On 2014/04/09 21:10:08, Ilya Sherman wrote:
> > This method returns a non-const pointer, so the method should also be
> non-const.
> >  A const method should not be able to lead to any externally observable
> > side-effects.
> 
> Done.
> Personally, I would agree with you if the client was owned by the manager. But
> that's not the case (the client actually owns the manager), so my personal
> opinion is still in favour of const. But I don't feel strongly about it, hence
I
> removed it.

You should really never use const on a method that returns a non-const pointer. 
This is actually a matter of correctness more so than it is of style.  For
example, if this method were non-const, you could write the following:

const PasswordManager password_manager = /* initialization code here */;
PasswordManager non_const_alias =
password_manager->client->GetDriver()->GetPasswordManager();
non_const_alias->ProvisionallySavePassword(/* some form */);

Tada! You've just transformed a const pointer into a non-const pointer by way of
an innocuous const method returning a non-const pointer.

In general, a good way to think about const is that calling a const method
should have no possible externally observable side-effects.  If you return a
non-const pointer from a const method, you can call a non-const method on the
returned pointer, which will tend to have externally observable side-effects,
which can lead to really surprising behavior.

> (I also split the line into three, following your note about changes in
> clang-format earlier).

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:72: virtual
bool IsLoggingActive() const;
On 2014/04/10 08:51:25, vabr (Chromium) wrote:
> On 2014/04/09 21:10:08, Ilya Sherman wrote:
> > Should this be pure virtual?  Ditto for the two methods above, actually.
> 
> I'm not sure. My reason for providing default implementation (for all 3 of
them)
> was that a PasswordManagerClient works still reasonably if the logging feature
> is off (whereas it cannot if it has no Prefs or PasswordStore, for example).

Let's make them pure virtual for now.  We can always revisit the decision later
if need be; but I think it's nice to have PasswordManagerClient be a pure
interface if it can be.

> > Actually actually, are these methods needed on the base class at all?  Can
> they
> > perhaps be moved into the ContentPasswordManagerClient class instead?
> 
> I'd like to keep them up here, because I expect we'll need this kind of
logging
> on the mobile platforms as well (note: there is no
ContentPasswordManagerClient,
> it's ChromePasswordManagerClient, so excluding also Android). Also, if I move
it
> to the embedder, I'd need to move the SavePasswordProgressLogger and children
> there as well, as they use these methods. 

Fair enough :)

https://codereview.chromium.org/228263004/diff/130001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/130001/components/password_man...
components/password_manager/core/browser/password_manager.h:104: }
nit: This should still be on a single line.  Per the recent clang format email
thread on chromium-dev, it's fine, and encouraged, even, to collapse (as well as
to inline) hacker_case methods; but not other methods.

[vabr (Chromium), 2014-04-10 12:15:41]

Colin, could you please take a look at components/components_tests.gyp?

inferno@: could you please rubber-stamp
components/autofill/content/common/autofill_messages.h?

Thanks a lot Ilya!
(And I hope you get some sleep!)
I addressed both your comments. Introducing the pure abstract methods triggered:
* extending the StubPasswordManagerClient, and
* making some more test client classes derive from the stub rather than the
PasswordManagerClient (no production code involved).
Given that that's what you asked me to introduce the stub for, and given your
approval for the rest, I will proceed with landing after Colin approves the GYPI
change. If you have objections, I will do my best to correct those in a
follow-up CL.

Thanks!
Vaclav

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.h:102:
PasswordManagerClient* client() const { return client_; }
On 2014/04/10 09:23:07, Ilya Sherman wrote:
> On 2014/04/10 08:51:25, vabr (Chromium) wrote:
> > On 2014/04/09 21:10:08, Ilya Sherman wrote:
> > > This method returns a non-const pointer, so the method should also be
> > non-const.
> > >  A const method should not be able to lead to any externally observable
> > > side-effects.
> > 
> > Done.
> > Personally, I would agree with you if the client was owned by the manager.
But
> > that's not the case (the client actually owns the manager), so my personal
> > opinion is still in favour of const. But I don't feel strongly about it,
hence
> I
> > removed it.
> 
> You should really never use const on a method that returns a non-const
pointer. 
> This is actually a matter of correctness more so than it is of style.  For
> example, if this method were non-const, you could write the following:
> 
> const PasswordManager password_manager = /* initialization code here */;
> PasswordManager non_const_alias =
> password_manager->client->GetDriver()->GetPasswordManager();
> non_const_alias->ProvisionallySavePassword(/* some form */);
> 
> Tada! You've just transformed a const pointer into a non-const pointer by way
of
> an innocuous const method returning a non-const pointer.
> 
> In general, a good way to think about const is that calling a const method
> should have no possible externally observable side-effects.  If you return a
> non-const pointer from a const method, you can call a non-const method on the
> returned pointer, which will tend to have externally observable side-effects,
> which can lead to really surprising behavior.
> 
> > (I also split the line into three, following your note about changes in
> > clang-format earlier).
> 

Thanks, Ilya, that's a very good point, and I agree now.

https://codereview.chromium.org/228263004/diff/130001/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/228263004/diff/130001/components/password_man...
components/password_manager/core/browser/password_manager.h:104: }
On 2014/04/10 09:23:07, Ilya Sherman wrote:
> nit: This should still be on a single line.  Per the recent clang format email
> thread on chromium-dev, it's fine, and encouraged, even, to collapse (as well
as
> to inline) hacker_case methods; but not other methods.

Done.

[blundell, 2014-04-10 13:15:56]

components_tests.gyp LGTM

[vabr (Chromium), 2014-04-10 13:33:30]

Thanks, Colin.

TBR-ing inferno@ for
components/autofill/content/common/autofill_messages.h (already reviewed by
Ilya) and sending to CQ.

Cheers,
Vaclav

[vabr (Chromium), 2014-04-10 13:33:38]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-04-10 13:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/228263004/170001

[commit-bot: I haz the power, 2014-04-10 15:33:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:33:56]

List of reviewers changed. tsepez@chromium.org did a drive-by without LGTM'ing!

[blundell, 2014-04-10 15:36:23]

On 2014/04/10 15:33:56, I haz the power (commit-bot) wrote:
> List of reviewers changed. mailto:tsepez@chromium.org did a drive-by without
LGTM'ing!

whoa...cq runs amok!

[vabr (Chromium), 2014-04-10 16:03:59]

On 2014/04/10 15:36:23, blundell wrote:
> On 2014/04/10 15:33:56, I haz the power (commit-bot) wrote:
> > List of reviewers changed. mailto:tsepez@chromium.org did a drive-by without
> LGTM'ing!
> 
> whoa...cq runs amok!

Filed http://crbug.com/362127, and checking the CQ bit again, as tsepez is not
currently reachable and I don't see any new comments.

[vabr (Chromium), 2014-04-10 16:04:10]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-04-10 16:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/228263004/170001

[commit-bot: I haz the power, 2014-04-10 16:05:37]

Change committed as 263002

[Ilya Sherman, 2014-04-10 18:19:41]

Václav, please don't TBR IPC message changes.  There is a reason that the
security team wants to review these changes: The IPC system can potentially
escalate a renderer security bug into a sandbox escape and a browser security
bug.

[Tom Sepez, 2014-04-10 19:11:13]

On 2014/04/10 18:19:41, Ilya Sherman wrote:
> Václav, please don't TBR IPC message changes.  There is a reason that the
> security team wants to review these changes: The IPC system can potentially
> escalate a renderer security bug into a sandbox escape and a browser security
> bug.
The question I have is whether there is sensitive information in the strings
being logged.

[Tom Sepez, 2014-04-10 19:16:42]

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:188: // Notification that
logs during saving the password have been gathered.
We'd be a lot happier if the log wasn't a free-form string; there's really no
way to audit what information might be leaked in that case.  From the design
doc, it looks like you are recording an URL plus a set of events which might
have happened to it.  If so, it would be much better to pass the URL, plus some
enums/flags to describe what's going on.

[Tom Sepez, 2014-04-10 19:20:57]

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:188: // Notification that
logs during saving the password have been gathered.
nit: I'm having a hard time reading that sentence.

[Ilya Sherman, 2014-04-10 19:32:15]

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:188: // Notification that
logs during saving the password have been gathered.
On 2014/04/10 19:16:42, Tom Sepez wrote:
> We'd be a lot happier if the log wasn't a free-form string; there's really no
> way to audit what information might be leaked in that case.  From the design
> doc, it looks like you are recording an URL plus a set of events which might
> have happened to it.  If so, it would be much better to pass the URL, plus
some
> enums/flags to describe what's going on.

There are actually methods that build these strings from more structured data,
sanitizing along the way: [
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
].  Those methods internally sanitize the data, then call SendLog().  Does that
address your concern, or do you think it's important to perform the sanitization
at the IPC layer?

[Tom Sepez, 2014-04-10 19:36:47]

> There are actually methods that build these strings from more structured data,
> sanitizing along the way: [
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> ].  Those methods internally sanitize the data, then call SendLog().  Does
that
> address your concern, or do you think it's important to perform the
sanitization
> at the IPC layer?
It depends where the sanitization is taking place, since this code is in
/common, I can't tell if its being applied browser-side (where it should be).

I guess my larger concern was that although your design doc says "No passwords
will be sent", there's no way to catch someone putting a password into your
string (say for debugging).  On the other hand, when someone add a field to a
message:

[vabr (Chromium), 2014-04-10 19:37:05]

I'm sorry about the TBR, it did not occur to me until after the commit, that all
the OWNERS are from the security team, and that IPC messages from renderer are
indeed crucial for security. That was particularly stupid from me, I'll make
sure not to TBR on this in the future.

I tried to answer Tom's questions here, and will be preparing a CL with fix once
we settle on a good way to do so.
Please feel free to revert this CL until the fix is ready, if you feel it's
better (note though, that all this functionality is behind a flag).

Thanks, and apologies.

Vaclav

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:188: // Notification that
logs during saving the password have been gathered.
On 2014/04/10 19:16:42, Tom Sepez wrote:
> We'd be a lot happier if the log wasn't a free-form string; there's really no
> way to audit what information might be leaked in that case.  From the design
> doc, it looks like you are recording an URL plus a set of events which might
> have happened to it.  If so, it would be much better to pass the URL, plus
some
> enums/flags to describe what's going on.

Does it help, that the URLs are being scrubbed to port and host?
(https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...)
This ScrubURL is used on everything which can be a URL, free form strings logged
through the logger are compile-time constants.

If it does not help, I can change the code to send structured messages + a
compile-time known set of strings.

https://codereview.chromium.org/228263004/diff/170001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:188: // Notification that
logs during saving the password have been gathered.
On 2014/04/10 19:20:57, Tom Sepez wrote:
> nit: I'm having a hard time reading that sentence.

Would
"Sends the collected log to browser for displaying to the user"
be better?

[Tom Sepez, 2014-04-10 19:39:27]

(hit send too soon).
I guess my larger concern was that although your design doc says "No passwords
will be sent", there's no way to catch someone putting a password into your
string (say for debugging).  On the other hand, when someone adds a field to a
message along the lines of:

   string /* password */

we get an obvious red flag.  Similarly, you say that no query params will be
part of the URL, if you passed this as a GURL apart from the string, you could
enforce this.

[vabr (Chromium), 2014-04-10 19:40:33]

On 2014/04/10 19:36:47, Tom Sepez wrote:
> > There are actually methods that build these strings from more structured
data,
> > sanitizing along the way: [
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> > ].  Those methods internally sanitize the data, then call SendLog().  Does
> that
> > address your concern, or do you think it's important to perform the
> sanitization
> > at the IPC layer?
> It depends where the sanitization is taking place, since this code is in
> /common, I can't tell if its being applied browser-side (where it should be).
> 
> I guess my larger concern was that although your design doc says "No passwords
> will be sent", there's no way to catch someone putting a password into your
> string (say for debugging).  On the other hand, when someone add a field to a
> message:

The sanitization in this case happens unfortunately in the renderer.

What did you mean with putting the password in the string? Did you mean if
somebody executes in the renderer context and wants to send the password to the
browser for further display on the internals page?

[Tom Sepez, 2014-04-10 19:44:19]

On 2014/04/10 19:40:33, vabr (Chromium) wrote:
> On 2014/04/10 19:36:47, Tom Sepez wrote:
> > > There are actually methods that build these strings from more structured
> data,
> > > sanitizing along the way: [
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> > > ].  Those methods internally sanitize the data, then call SendLog().  Does
> > that
> > > address your concern, or do you think it's important to perform the
> > sanitization
> > > at the IPC layer?
> > It depends where the sanitization is taking place, since this code is in
> > /common, I can't tell if its being applied browser-side (where it should
be).
> > 
> > I guess my larger concern was that although your design doc says "No
passwords
> > will be sent", there's no way to catch someone putting a password into your
> > string (say for debugging).  On the other hand, when someone add a field to
a
> > message:
> 
> The sanitization in this case happens unfortunately in the renderer.
Not great, but OK for now, given your HTML escaping takes place browser-side,
right (for your UI).
> 
> What did you mean with putting the password in the string? Did you mean if
> somebody executes in the renderer context and wants to send the password to
the
> browser for further display on the internals page?
No, I mean if a chrome developer does:

    sprintf(msg, "Password was: ", password);
    Log(msg);

and then some users sees their password in plain text.  At which point you'll
have to explain to all the naive people in the world why there isn't an issue
here (there isn't).

[Tom Sepez, 2014-04-10 19:45:44]

In other words, we prefer "Its not possible to do that" to "we're never going to
do that"

[vabr (Chromium), 2014-04-10 19:53:55]

On 2014/04/10 19:44:19, Tom Sepez wrote:
> On 2014/04/10 19:40:33, vabr (Chromium) wrote:
> > On 2014/04/10 19:36:47, Tom Sepez wrote:
> > > > There are actually methods that build these strings from more structured
> > data,
> > > > sanitizing along the way: [
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> > > > ].  Those methods internally sanitize the data, then call SendLog(). 
Does
> > > that
> > > > address your concern, or do you think it's important to perform the
> > > sanitization
> > > > at the IPC layer?
> > > It depends where the sanitization is taking place, since this code is in
> > > /common, I can't tell if its being applied browser-side (where it should
> be).
> > > 
> > > I guess my larger concern was that although your design doc says "No
> passwords
> > > will be sent", there's no way to catch someone putting a password into
your
> > > string (say for debugging).  On the other hand, when someone add a field
to
> a
> > > message:
> > 
> > The sanitization in this case happens unfortunately in the renderer.
> Not great, but OK for now, given your HTML escaping takes place browser-side,
> right (for your UI).
I'm afraid I do it in the renderer (even in JavaScript) currently, by assigning
the string as .innerText
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...).
This should be easy to fix, is there a preferred standard function in Chromium
code to escape the string? I would call that in
PasswordManagerInternalsUI::LogSavePasswordProgress.

> > 
> > What did you mean with putting the password in the string? Did you mean if
> > somebody executes in the renderer context and wants to send the password to
> the
> > browser for further display on the internals page?
> No, I mean if a chrome developer does:
> 
>     sprintf(msg, "Password was: ", password);
>     Log(msg);
> 
> and then some users sees their password in plain text.  At which point you'll
> have to explain to all the naive people in the world why there isn't an issue
> here (there isn't).

I understand.
I'm not sure what to improve to stop that. We did not really want to expect
sending passwords at all, so even if I make the logs structured, there will not
be a part of the structure for saving passwords, so the developer would be
likely putting it as a simple message (which I would still needed). Or do you
suggest I introduce a password part in the structure?

Or I could put the compile-time messages in resources, and send only resource
IDs. Would that help?

[Tom Sepez, 2014-04-10 20:02:57]

> Or I could put the compile-time messages in resources, and send only resource
> IDs. Would that help?
Much better. No strings to leak.

[vabr (Chromium), 2014-04-10 20:10:58]

On 2014/04/10 20:02:57, Tom Sepez wrote:
> > Or I could put the compile-time messages in resources, and send only
resource
> > IDs. Would that help?
> Much better. No strings to leak.

OK, so I'll try to put the following structure on the messages being handed over
to the browser. They will be either:
* URLs
* numbers
* booleans
* resource IDs for fixed strings
* ideally also HTML element IDs or names -- would that be acceptable? If not, I
can drop those, but would prefer to keep them. Browser-side I'm happy to
sanitize them, and through out everything which does not consist of, say
alphanumerics and -, _.

Also, I will add browser-side escaping into
PasswordManagerInternalsUI::LogSavePasswordProgress -- would the stuff from
net/base/escape.h be the right thing to use?

I'll work on this CL first thing in the morning, which is approx. 10 hour from
now for me. Is that OK?

Cheers,
Vaclav

[Tom Sepez, 2014-04-10 20:12:59]

Sounds good to me.  thanks!

[blundell, 2014-04-11 14:39:15]

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/228263004/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:72: virtual
bool IsLoggingActive() const;
PasswordClient is already not a pure interface FWIW (see e.g.
PasswordWasAutofilled). I think that there's value in having "optional" client
methods have default implementations, compared to "mandatory" client methods
that are pure virtual (e.g., GetPrefs()).

On 2014/04/10 09:23:07, Ilya Sherman wrote:
> On 2014/04/10 08:51:25, vabr (Chromium) wrote:
> > On 2014/04/09 21:10:08, Ilya Sherman wrote:
> > > Should this be pure virtual?  Ditto for the two methods above, actually.
> > 
> > I'm not sure. My reason for providing default implementation (for all 3 of
> them)
> > was that a PasswordManagerClient works still reasonably if the logging
feature
> > is off (whereas it cannot if it has no Prefs or PasswordStore, for example).
> 
> Let's make them pure virtual for now.  We can always revisit the decision
later
> if need be; but I think it's nice to have PasswordManagerClient be a pure
> interface if it can be.
> 
> > > Actually actually, are these methods needed on the base class at all?  Can
> > they
> > > perhaps be moved into the ContentPasswordManagerClient class instead?
> > 
> > I'd like to keep them up here, because I expect we'll need this kind of
> logging
> > on the mobile platforms as well (note: there is no
> ContentPasswordManagerClient,
> > it's ChromePasswordManagerClient, so excluding also Android). Also, if I
move
> it
> > to the embedder, I'd need to move the SavePasswordProgressLogger and
children
> > there as well, as they use these methods. 
> 
> Fair enough :)

[vabr (Chromium), 2014-04-11 18:42:11]

For everyone possibly following this: the new CL is
https://codereview.chromium.org/235623002/. Discussion is preferred there, this
Cl is already closed.

Thanks!
Vaclav