Include filenames in InsecurePasswordUtils errors

VERIFIED FIXED in Firefox 48

Status

()

P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox48 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment)

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.
Created attachment 8732027 [details]
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/
Attachment #8732027 - Flags: review?(bgrinstead)
There are no tests for this module yet but Sean will add some in bug 1191092.

Updated

3 years ago
Flags: qe-verify?
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Attachment #8732027 - Flags: review?(bgrinstead) → review+
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)
(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.
(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 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ed6d4e80064
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

3 years ago
Iteration: --- → 48.2 - Apr 4
Priority: -- → P1

Updated

3 years ago
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Verified fixed FX 48.0a1 (2016-04-06) Win 7
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.