1269568 - Remove LoginManagerContent's isDocumentSecure function in favor of Window.isSecureContext

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Jonathan Watt [:jwatt]]

LoginManagerContent has an isDocumentSecure function that tries to provide a rough approximation of whether or not a document creates a Secure Context, aka:

https://w3c.github.io/webappsec-secure-contexts/

Now that bug 1162772 has landed this code should be removed in favor of Window.isSecureContext.

Note that the hasInsecureLoginForms function in LoginManagerContent.jsm no longer needs the parentIsInsecure argument since Window.isSecureContext takes that into account.

Similarly _checkForInsecureNestedDocuments is not needed since all it does is walk the parent chain checking if each document is secure.

Note that Window.isSecureContext also takes into account the security of opener windows, so switching to Window.isSecureContext does involve a change in behavior to become more restrict in that regard.

Comment 1

[Jonathan Watt [:jwatt]]

(gScriptSecurityManager can also be removed from LoginManagerContent.jsm since isDocumentSecure is the only consumer.)

Comment 2

[Matthew N. [:MattN]]

Yeah, I was planning on switching this out after bug 1261234 lands. It seems like we would still need isOriginPotentiallyTrustworthy in InsecurePasswordUtils for the form @action attribute.

Comment 3

[Jonathan Watt [:jwatt]]

MattN: Should this be assigned to you?

Comment 4

[Matthew N. [:MattN]]

No as I have no time to work on it this quarter and it doesn't seem like a high priority. If Sean or you want to figure it out I can review but I'm not sure what the urgency is since I don't think we'll ever be able to remove isOriginPotentiallyTrustworthy.

Comment 5

[Matthew N. [:MattN]]

This mess of differing security checks was getting in the way of bug 1302474 so I'm putting up a patch. Bug 1302474 will clean up the security checks even more.

Comment 6

[Matthew N. [:MattN]]

Attachment: Bug 1269568 - Remove LoginManagerContent's isDocumentSecure function in favor of Window.isSecureContext.

Review commit: https://reviewboard.mozilla.org/r/84114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84114/

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8798695 [details]
Bug 1269568 - Remove LoginManagerContent's isDocumentSecure function in favor of Window.isSecureContext.

https://reviewboard.mozilla.org/r/84114/#review83114

Looks good, thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/084f217c4d49
Remove LoginManagerContent's isDocumentSecure function in favor of Window.isSecureContext. r=johannh

Comment 9

[Matthew N. [:MattN]]

Attachment: Bug 1269568 - Update browser_webconsole_bug_762593_... to check for the more accurate frame message.

Review commit: https://reviewboard.mozilla.org/r/84686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84686/

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8799540 [details]
Bug 1269568 - Update browser_webconsole_bug_762593_... to check for the more accurate frame message.

https://reviewboard.mozilla.org/r/84686/#review83276

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2919296c48c0
Update browser_webconsole_bug_762593_... to check for the more accurate frame message. r=me

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/084f217c4d49
https://hg.mozilla.org/mozilla-central/rev/2919296c48c0