Open Bug 1269350 Opened 5 years ago Updated 2 years ago

Submitting passwords in extension panels causes errors

Categories

(Toolkit :: Password Manager, defect, P5)

defect

Tracking

()

Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- affected
firefox53 --- affected

People

(Reporter: ecfbugzilla, Assigned: matrixisreal, Mentored)

References

Details

(Whiteboard: [fxprivacy] )

Attachments

(1 file)

577 bytes, application/x-xpinstall
Details
Attached file Minimal test extension
Steps to reproduce:

1. Download attached extension.
2. Go to about:config and set xpinstall.signatures.required to false.
3. Open Add-ons Manager and install the extension from file.
4. Click the extension's icon in the toolbar (generic puzzle icon).
5. Enter something into the password field and click Submit.
6. Press Ctrl-Shift-J (Cmd-Shift-J on OS X) to open the Browser Console.

Expected results:

No errors in Browser Console.

Actual results:

The following error messages appear:

> TypeError: browser is null  (resource://gre/components/nsLoginManagerPrompter.js line 808)
> NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "browser is null" {file: "resource://gre/components/nsLoginManagerPrompter.js" line: 808}]'[JavaScript Error: "browser is null" {file: "resource://gre/components/nsLoginManagerPrompter.js" line: 808}]' when calling method: [nsILoginManagerPrompter::promptToSavePassword] (resource://gre/modules/LoginManagerParent.jsm line 419)

This affects panels created by both WebExtensions (like this test extension) and Add-on SDK (like Easy Passwords). From the source code, I could not find any way to stop login manager from trying - other than stopping to submit forms altogether which would degrade accessibility however.
LoginManagerContent's onFormSubmit() should probably ignore form submissions from non-browser contexts. If we don't have a browser to attach a "do you want to save this" notification to, we should just bail out early.
Mentor: MattN+bmo
Priority: -- → P5
i'd like to work on this, could you assign this to me?
Assignee: nobody → julia.friesel
Status: NEW → ASSIGNED
It seems that WebExtensions have a browser (see http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/browser/components/extensions/ext-utils.js#228). We should probably also return early if the browser is a webextension popup browser. I'd suggest something like

if (!browser || browser.className == "webextension-popup-browser") return;

Kris, does that sound like a good way to detect popup browsers?
Flags: needinfo?(kmaglione+bmo)
Mentor: jhofmann
Whiteboard: [fxprivacy] [good first bug]
That sounds OK to me, but it's probably worth nothing that those browsers will become remote in the near future. And either way, we should make sure to add a test.

But, does that mean that we won't support saved logins in extension popups? I'm not sure whether or not that's desirable...
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #4)
> But, does that mean that we won't support saved logins in extension popups?
> I'm not sure whether or not that's desirable...

At least classic extensions typically use nsLoginManager directly and save the passwords themselves if necessary. Not sure what the idea is for WebExtensions.
I'm sorry, I meant to comment on this 2 days ago: Can you please unassign me again? It turns out I won't have the time to do this one. Thanks!
Assignee: julia.friesel → nobody
Status: ASSIGNED → NEW
> But, does that mean that we won't support saved logins in extension popups? I'm not sure whether or not that's desirable...

If we come up with a solid plan/UX on how to support this we certainly can. This bug is only about preventing crashes and/or undefined behavior in the short term.
For anyone who would like to tackle this bug:

In http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#772 you will need to add something along the lines of

if (!browser || browser.className == "webextension-popup-browser") return;

to exit the function early in case there is no browser or the browser is from a webextension.

You should try it out by installing the extension that is attached to this bug. Before your patch, it would probably throw an error in the browser console (https://developer.mozilla.org/en-US/docs/Tools/Browser_Console) and after applying your patch it should just do nothing at all.

If you need help getting started check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide or ask me questions on this bug or in IRC.
Hi, 
I need some help
I have installed the extension and submitted the form but i couldnt reproduce the error in the Browser Console, what do you think might be the problem.
I have tried setting up a breaking point at the func _showLoginCaptureDoorhanger(login, type) but it turns out the func is never executed.
Hm, I can't reproduce this on the fly, either. I haven't really looked into why the function is never called, but I can do that after the holidays. Maybe this was resolved elsewhere already.

Sorry. Feel free to play around with this a little further, if you have the time. :)

Thanks!
Yes, I can no longer reproduce this issue in 2016-12-19 nightly - maybe some unrelated change fixed this already. Assuming that login manager isn't simply broken in nightly of course.
Assignee: nobody → pavankarthikboddeda
Whiteboard: [fxprivacy] [good first bug] → [fxprivacy] [good first bug] [triage]
Hi Johann,
Shall we mark this fixed as the bug is nomore reproducable?
So I finally found time to look into this. The error is not happening anymore because there's another error in _getShortDisplayHost at this line (http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#1572). It seems that it's unable to get a base domain from a moz:// url (that error is caught, however).

Now we could either just be happy about this and resolve this bug or we add my proposed if-clause anyway, to explicitly guard against addons using this in the future (maybe someone else removes _getShortDisplayHost and the error starts to appear again). I'd say we should add it, but I don't feel strongly here.

Matt, do you have an opinion on this?
Flags: needinfo?(MattN+bmo)
I would rather not tightly couple pwmgr with webextensions so I would prefer we just write a test for this and leave the implementation as-is.
Flags: needinfo?(MattN+bmo)
Whiteboard: [fxprivacy] [good first bug] [triage] → [fxprivacy] [good first bug]
Hi Johann,
I have never worked with tests before, Could you guide me in writing a test for this if thats what it needs in here.
Thanks.
Flags: needinfo?(jhofmann)
Clearing ni since we've been in touch over IRC. Let me know when you have some time to look at it again. :)
Flags: needinfo?(jhofmann)
See Also: → 887337
Keywords: good-first-bug
Whiteboard: [fxprivacy] [good first bug] → [fxprivacy]
You need to log in before you can comment on or make changes to this bug.