Closed Bug 1257757 Opened 4 years ago Closed 4 years ago

Include filenames in InsecurePasswordUtils errors

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

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.
There are no tests for this module yet but Sean will add some in bug 1191092.
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?
https://hg.mozilla.org/mozilla-central/rev/5ed6d4e80064
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Iteration: --- → 48.2 - Apr 4
Priority: -- → P1
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Verified fixed FX 48.0a1 (2016-04-06) Win 7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.