Closed Bug 1191092 Opened 9 years ago Closed 9 years ago

InsecurePasswordUtils should handle <input type=password> outside of a <form>

Categories

(Core :: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: MattN, Assigned: selee, Mentored)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 3 obsolete files)

Some sites (e.g. hulu.com's homepage) have insecure logins pages without using a <form> element and those get missed by InsecurePasswordUtils. InsecurePasswordUtils should use DOMInputPasswordAdded to catch those cases. Since that event doesn't do batching we should probably implement batching with a WeakMap (e.g. using the document as the key) so we don't show the same error more than once per document.
Blocks: 1174333
We want to have (some/all) of checkForInsecurePasswords work from DOMInputPasswordAdded. We should probably use FormLikeFactory.createFromField(event.target) then use the "FormLike" instead of the real HTMLFormElement in the helper functions where it makes sense. See https://mxr.mozilla.org/mozilla-central/search?string=checkForInsecurePasswords and https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm?rev=1249401fb8b0#1300
Mentor: MattN+bmo
Whiteboard: [lang=js][good first bug]
Hey Matthew, I am interested in this bug, so I take it. Thanks :)
Assignee: nobody → selee
Hi Matthew, This is the patch to check for insecure passwords which is not wrapper in form element. Could you help to give it a feedback? I suppose that I have to give it a correspond test. Is this a good place[1] where the test should be? Should I create another html file to test the fields not in form element? This is my first patch for Firefox Browser. Thanks a lot for mentoring me. :) [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_insecureLoginForms.js#16 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/form_basic.html
Attachment #8731593 - Flags: feedback?(MattN+bmo)
Blocks: 1217142
Comment on attachment 8731593 [details] [diff] [review] 0001-Bug-1191092-Check-password-box-not-inside-a-form-ele.patch Review of attachment 8731593 [details] [diff] [review]: ----------------------------------------------------------------- Hi Sean, good job on getting the basics working! I think you may missed this comment though: (Quoting Matthew N. [:MattN] from comment #0) > Since that event doesn't do batching we should probably implement batching > with a WeakMap (e.g. using the document as the key) so we don't show the > same error more than once per document. With your patch we will warn in the console for each insecure <input type=password> (+ each insecure form from before – see below on that). I think one of each warning per FormLike would be better. Note that the problem of running more times will also skew the PWMGR_LOGIN_PAGE_SAFETY telemetry probe and I think a per-form count will continue to align with before (it was just missing non-<form>) Note that this new event overlaps with the previous so the batching below will also help with that. Here's what I think we should do (a little more involved than I originall expected for a "good first bug"): 1) Change the "DOMFormHasPassword" listener in content.js to create a FormLike from the real <form> so we always use a FormLike and will then have the rootElement property we will use for (2): `FormLikeFactory.createFromForm(event.target)` 2) Add a WeakMap property on the InsecurePasswordUtils (e.g. _formRootsWarned) which maps from FormLike.rootElement to a boolean indicating whether we've warned for the FormLike already. We use FormLike.rootElement as the key instead of FormLike itself since you won't get the same FormLike for the same thing that looks like a form. 3) At the beginning of `checkForInsecurePasswords`, return without any warning/logging if the rootElement of the argument already has a true value in the WeakMap. By returning at the beginning we avoid logging and accumulating more than once. 4) At the three places where we call `_sendWebConsoleMessage`, set the Map value to true for the rootElement. (In reply to Sean Lee [:seanlee][:weilonge] from comment #3) > I suppose that I have to give it a correspond test. Is this a good place[1] > where the test should be? Yes, we should have a simple test. That existing test is testing the lock icon UI which already handled form-less passwords field since it doesn't use InsecurePasswordUtils. What this bug is changing is the console warnings for developers and the associated telemetry for it. Before your fix there would be no console warnings on an insecure login form without a real <form>. With your patch we now get the warnings (just too many). What we would want is a test of the console output. Unfortunately the console warnings don't already have a test to simply add to but there are other tests which do similar things and it's not too hard. Since testing this requires content.js in browser/, we will need a mochitest-browser-chrome test [1] and we should use the new style where we use add_task. An example (somewhat more complicated) is browser/components/extensions/test/browser/browser_ext_popup_api_injection.js. See promiseConsoleMessage there. I think we probably don't want to use a promise since I think we want to continue listening after the expected number of messages to see if we log too many (like your current patch does). We can add the console listener and keep track of how many of each InsecurePasswordUtils messages are received. When we get the right number of each type for the test task use ContentTask.spawn to console.log some message to indicate we're done the test. When that message is received by our console listener we can unregister it and finish the test.. > Should I create another html file to test the fields not in form element? Yes, I think that would be easiest as our existing form-less tests are dynamically created in mochitest-plain. A copy of form_basic.html with the <form> removed would be good (e.g. formless_basic.html). Don't forget to add the HTML file and test to browser.ini. Note that the Public Domain comment is no longer necessary for tests as they are PD by default now. Please add a separate task to your new test file for form_basic.html as it will be very similar and it would be nice to have coverage for both cases. Don't worry about the iframe/subframe case for the test though as that's even more orthogonal to this bug. Let me know if you have any questions as I know there is a lot of information above since I know it's your first desktop bug. Thanks, Matthew [1] https://developer.mozilla.org/en-US/docs/Browser_chrome_tests
Attachment #8731593 - Flags: feedback?(MattN+bmo) → feedback+
Hey Matthew, Sorry that I was blocked by other stuff, so the response is late. I've followed your 4 suggestions and fixed them. I added one more thing to avoid showing "InsecureFormActionPasswordsPresent" message when aForm.rootElement is an input field rather than a form. Could you help to give a feedback again when I am implementing the test? Thank you!
Attachment #8731593 - Attachment is obsolete: true
Attachment #8733813 - Flags: feedback?(MattN+bmo)
Comment on attachment 8733813 [details] [diff] [review] 0001-Bug-1191092-Check-password-box-not-inside-a-form-ele.patch Review of attachment 8733813 [details] [diff] [review]: ----------------------------------------------------------------- Great! Looks like we just need to finish the test now. ::: toolkit/components/passwordmgr/test/browser/input_basic.html @@ +1,1 @@ > +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body> Nit: Can you call this formless_basic.html as it's more clear that it's for testing formless stuff. @@ +1,3 @@ > +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body> > +<!-- Any copyright is dedicated to the Public Domain. > + - http://creativecommons.org/publicdomain/zero/1.0/ --> Nit: Don't need to include public domain license in tests anymore as it's PD by default.
Attachment #8733813 - Flags: feedback?(MattN+bmo) → feedback+
Hey Matthew, The test is in the latest patch. Could you help to review it? Thank you!
Attachment #8733813 - Attachment is obsolete: true
Attachment #8735143 - Flags: review?(MattN+bmo)
Apologies for the delay. I've taken some PTO for Easter.
Comment on attachment 8735143 [details] [diff] [review] 0001-Bug-1191092-Check-password-box-not-inside-a-form-ele.patch Review of attachment 8735143 [details] [diff] [review]: ----------------------------------------------------------------- Great job! I can take another look at the test if you would like but I'm fine without that too. Perhaps you'd be interested in bug 1261234 too? ::: toolkit/components/passwordmgr/moz.build @@ +15,4 @@ > TESTING_JS_MODULES += [ > # Make this file available from the "resource:" URI of the test environment. > 'test/browser/form_basic.html', > + 'test/browser/formless_basic.html', I don't think you need this as I believe that was just used for https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js#11 ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js @@ +1,2 @@ > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set sts=2 sw=2 et tw=80: */ Nit: No modelines in new JS files please. @@ +12,5 @@ > + '[JavaScript Warning: "Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen."]': "INSECURE_PAGE" > +}; > + > +add_task(function* testInsecurePasswordWarning() { > + for (let [origin, testPattern, expectWarnings] of [ `testPattern` doesn't seem like an accurate variable name. Maybe `testFile`? @@ +13,5 @@ > +}; > + > +add_task(function* testInsecurePasswordWarning() { > + for (let [origin, testPattern, expectWarnings] of [ > + ["http://127.0.0.1", "form_basic.html", ["INSECURE_FORM_ACTION"]], This is actually a bug in the existing logic and shouldn't be reported. I noticed that the code was using a regex for the scheme the other day but thought it wasn't so bad. I didn't think about the fact that it doesn't treat localhost/127.0.0.1/etc. as "secure" though. I filed bug 1261234 on this issue so please add a comment with that bug number and mention that it should be `[]` @@ +15,5 @@ > +add_task(function* testInsecurePasswordWarning() { > + for (let [origin, testPattern, expectWarnings] of [ > + ["http://127.0.0.1", "form_basic.html", ["INSECURE_FORM_ACTION"]], > + ["http://127.0.0.1", "formless_basic.html", []], > + ["http://example.com", "form_basic.html", ["INSECURE_FORM_ACTION","INSECURE_PAGE"]], Nit: space after `,` in the array literal @@ +22,5 @@ > + ["https://example.com", "formless_basic.html", []], > + ]) { > + let testUrlPath = origin + TEST_URL_PATH + testPattern; > + let tab = gBrowser.addTab(testUrlPath); > + let browser = tab.linkedBrowser; I think this for loop body could be simplified with BrowserTestUtils.withNewTab to get rid of all the other tab method calls. If not, at least use BrowserTestUtils.openNewForegroundTab and BrowserTestUtils.removeTab instead of the gBrowser methods directly. @@ +26,5 @@ > + let browser = tab.linkedBrowser; > + function listener(msg) { > + // Only handle the insecure password related warning messages. > + if (WARNING_PATTERN[msg.message]) { > + var index = expectWarnings.indexOf(WARNING_PATTERN[msg.message]); Nit: use `let` in new code @@ +28,5 @@ > + // Only handle the insecure password related warning messages. > + if (WARNING_PATTERN[msg.message]) { > + var index = expectWarnings.indexOf(WARNING_PATTERN[msg.message]); > + > + // Warning message found. Nit: This comment seems a bit redundant with the line below so please remove it. @@ +29,5 @@ > + if (WARNING_PATTERN[msg.message]) { > + var index = expectWarnings.indexOf(WARNING_PATTERN[msg.message]); > + > + // Warning message found. > + ok(index !== -1, "Warning message hits!"); To make this test easier to debug if it starts to fail, could you include `WARNING_PATTERN[msg.message]` appended to the message in the 2nd argument to ok(). Example output: Found warning: INSECURE_FORM_ACTION @@ +45,5 @@ > + ]); > + > + // Verify if all warnings are shown. > + for (let i = 0; i < expectWarnings.length; i++) { > + ok(expectWarnings[i] === null, "All warnings are shown."); You could use splice[1] above instead of setting to `null` then you can just check the length of expectWarnings [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice with deleteCount=1 ::: toolkit/components/passwordmgr/test/browser/formless_basic.html @@ +10,5 @@ > + <script> > + document.getElementById('add').addEventListener('click', function () { > + var node = document.createElement("input"); > + node.setAttribute('type', 'password'); > + document.querySelector('body').appendChild(node); Nit: Please use double-quotes in all new JS code (I know some old password manager tests use single quotes).
Attachment #8735143 - Flags: review?(MattN+bmo) → review+
Hey Matthew, The latest patch has been modified with your suggestion at Comment 9. For the simplified loop with BrowserTestUtils.withNewTab, I am not sure if I did the right way. Could you help to review it again? This patch is in MozReview format because I would like to practice it. Although I know some reviewers prefer to use the patch attachement, may I use MozReview for the further review? Thank you!
Status: NEW → ASSIGNED
Blocks: 1261234
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN https://reviewboard.mozilla.org/r/43999/#review40569 > > This patch is in MozReview format because I would like to practice it. > Although I know some reviewers prefer to use the patch attachement, may I > use MozReview for the further review? Sure, I prefer MozReview. ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:40 (Diff revision 1) > + } > + } > + } > + Services.console.registerListener(listener); > + > + let tab = yield BrowserTestUtils.withNewTab({ No need to assign this to a variable ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:40 (Diff revision 1) > + let tab = yield BrowserTestUtils.withNewTab({ > + gBrowser, > + url: testUrlPath > + }, function*() {}); > + > + // Verify if all warnings are shown. > + ok(expectWarnings.length === 0, "All warnings are shown."); I think this approach with the empty function is fine assuming the console listener sends messages synchronously. Please double-check this passes on Try. ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:46 (Diff revision 1) > + gBrowser, > + url: testUrlPath > + }, function*() {}); > + > + // Verify if all warnings are shown. > + ok(expectWarnings.length === 0, "All warnings are shown."); `is(expectWarnings.length, 0, "All warnings are shown.")` is preferred as expectWarnings.length will get output if it fails. Thanks!
Attachment #8737521 - Flags: review?(MattN+bmo) → review+
Hi Matthew, The new mochitest for e10s is failed. (but non-e10s is fine.) I suppose that this is caused by the console listener sends messages asynchronously. I try to use setTimeout to wait console message event for 5 secs, and the test is PASS for e10s. However, I suppose setTimeout is a bad practice. Could you give me a suggestion here? Thank you!
Flags: needinfo?(MattN+bmo)
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43999/diff/1-2/
Hi Matthew, After rebasing codes and pushing to try-server, the result [1] shows that e10s mode is sometimes failed (bc2, bc3, bc5), but non-e10s mode is all PASS. I think setTimeout is a way to wait for all console warning coming. Although I know I should find an event to wait for all warnings shown, I still add this code to fix the failed case in e10s mode. I found the warning messages seem been modified, so I change WARNING_PATTERN to satisfy the new style. Could you help to review this patch again? Thank you! [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2930fe0ef3a
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN https://reviewboard.mozilla.org/r/43999/#review41659 ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:52 (Diff revision 2) > + setTimeout(() => { > + // Verify if all warnings are shown. > + is(expectWarnings.length, 0, "All warnings are shown. URL:" + testUrlPath); > + Services.console.unregisterListener(listener); > + resolve(); > + }, 3000); Hi Sean, I would rather avoid the setTimeout by having a helper that returns a promise (e.g. promiseConsoleMessages) and resolves when all of the expected messages were seen. It would replace the ```is(expectWarnings.length, 0, "All warnings are shown. URL:" + testUrlPath);``` which could be switched to an `info()`.
Attachment #8737521 - Flags: review+
Flags: needinfo?(MattN+bmo)
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43999/diff/2-3/
Attachment #8737521 - Flags: review?(MattN+bmo)
Hi Matthew, I use a promise to wait for console warning messages in the latest patch. messageHandler is a warning message handler for the whole task, and warningPatternHandler is for each little test case. This design is for monitoring all messages come lately, and the late message should not match the insecure password pattern that we defined. tree-herder link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de324d20fd0a (ES error is fixed in the latest patch) Thanks for reviewing this good-first-bug couple times.
Attachment #8737521 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN https://reviewboard.mozilla.org/r/43999/#review42207 ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:13 (Diff revisions 2 - 3) > add_task(function* testInsecurePasswordWarning() { > + var warningPatternHandler; Nit: please use `let` ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:27 (Diff revisions 2 - 3) > + // Prevent any unexpected or redundant matched warning message comes after > + // the test case is ended. Nit: Replace "comes" with "coming" ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:77 (Diff revisions 2 - 3) > + // Remove warningPatternHandler to stop handing the matched warning pattern > + // and the task should not get any warning anymore. > + warningPatternHandler = null; typo: "handing" ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:50 (Diff revision 3) > + let messageFullMatched = originMessage === `[${warning.msg} {file: "${testUrlPath}" line: 0 column: 0 source: "0"}]`; > + ok(messageFullMatched, "Message full matched:" + originMessage); Make this an is(…) so both the expected and actual get output upon failure ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:54 (Diff revision 3) > + ok(warning, "Handling a warning pattern"); > + let messageFullMatched = originMessage === `[${warning.msg} {file: "${testUrlPath}" line: 0 column: 0 source: "0"}]`; > + ok(messageFullMatched, "Message full matched:" + originMessage); > + > + let index = expectWarnings.indexOf(warning.key); > + ok(index !== -1, "Found warning: " + warning.key + " for URL:" + testUrlPath); Make this isnot(…) ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:59 (Diff revision 3) > + if (expectWarnings.length === 0) { > + console.info("All warnings are shown. URL:" + testUrlPath); > + resolve(); Unless I understand, I think you should be using just info(…) instead of console.info ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:71 (Diff revision 3) > + yield BrowserTestUtils.withNewTab({ > + gBrowser, > + url: testUrlPath > + }, function*() { > + if (expectWarnings.length === 0) { > + console.info("All warnings are shown. URL:" + testUrlPath); same here ::: toolkit/components/passwordmgr/test/browser/browser_insecurePasswordWarning.js:81 (Diff revision 3) > + > + // Remove warningPatternHandler to stop handing the matched warning pattern > + // and the task should not get any warning anymore. > + warningPatternHandler = null; > + } > + Services.console.unregisterListener(messageHandler); Please put this in a SimpleTest.registerCallbackFunction callback on the line after the registerListener call. This ensures it will get cleaned up if the test throws in the middle.
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43999/diff/3-4/
Comment on attachment 8737521 [details] MozReview Request: Bug 1191092 - Check password box not inside a form element.; r?MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43999/diff/4-5/
Keywords: checkin-needed
needs rebasing it seems applying 27d841661cad patching file toolkit/components/passwordmgr/test/browser/browser.ini Hunk #1 FAILED at 8 1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/passwordmgr/test/browser/browser.ini.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(selee)
Keywords: checkin-needed
Attachment #8735143 - Attachment is obsolete: true
I improved the commit message, rebased and pushed. Thanks Sean!
Flags: needinfo?(selee)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1265201
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: