1257757 - Include filenames in InsecurePasswordUtils errors

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

Currently the errors say "(unknown)" where the filename is supposed to show which makes it harder for developers to figure out what's causing the warning.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1257757 - Add filenames to insecure password warnings. r=bgrins

Review commit: https://reviewboard.mozilla.org/r/40973/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40973/

Comment 2

[Matthew N. [:MattN]]

There are no tests for this module yet but Sean will add some in bug 1191092.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8732027 [details]
MozReview Request: Bug 1257757 - Add filenames to insecure password warnings. r=bgrins

https://reviewboard.mozilla.org/r/40973/#review37647

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:32
(Diff revision 1)
>      let category = "Insecure Password Field";
>      // All web console messages are warnings for now.
>      let flag = Ci.nsIScriptError.warningFlag;
>      let message = l10n.getStr(messageTag);
>      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> -    consoleMsg.initWithWindowID(message, "", 0, 0, 0, flag, category, windowId);
> +    consoleMsg.initWithWindowID(message, domDoc.location.href, 0, 0, 0, flag, category, windowId);

It looks like most places use the file name only: https://dxr.mozilla.org/mozilla-central/search?q=initWithWindowID&redirect=true&case=false

But it seems fine to use href (any string will do)

Comment 4

[Matthew N. [:MattN]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> It looks like most places use the file name only:
> https://dxr.mozilla.org/mozilla-central/
> search?q=initWithWindowID&redirect=true&case=false
> 
> But it seems fine to use href (any string will do)

I thought I checked and e.fileName has the whole URL. I want it so that clicking on the file name opens to the source which it currently does with this patch. I guessed that only the filename wouldn't be able to do that.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > It looks like most places use the file name only:
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=initWithWindowID&redirect=true&case=false
> > 
> > But it seems fine to use href (any string will do)
> 
> I thought I checked and e.fileName has the whole URL. I want it so that
> clicking on the file name opens to the source which it currently does with
> this patch. I guessed that only the filename wouldn't be able to do that.

Do you have a good test page / STR for this error to show up?

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5ed6d4e80064

Comment 7

[Brian Grinstead [:bgrins]]

Disregard comment 5.. I have a test page at http://bgrins.github.io/devtools-demos/inspector/form.html

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ed6d4e80064

Comment 9

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 48.0a1 (2016-04-06) Win 7