Open Bug 1269350 Opened 5 years ago Updated 2 years ago
Submitting passwords in extension panels causes errors
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.
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?
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...
(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.
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?
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.
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.
Clearing ni since we've been in touch over IRC. Let me know when you have some time to look at it again. :)
Whiteboard: [fxprivacy] [good first bug] → [fxprivacy]
You need to log in before you can comment on or make changes to this bug.