Fixing the case where mixed content checking for insecure form submission in secure origins is brea…

Fixing the case where mixed content checking for insecure form submission in secure origins is breaking when using invisible DOMs.

An invisible DOM (i.e. DOM without a frame) causes the mixed content checking for insecure form submission to crash the current tab. This is because the DOM is not associated with any frame.

A follow up bug (https://crbug.com/399810) was filed to address another corner case where mixed content checking can be bypassed when you replace the current DOM with this invisible DOM.

BUG=398066
R=jww
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179530

[mhm, 2014-08-01 23:16:07]

Hey jww@ can you please have a look at this for me.

[jww, 2014-08-01 23:20:52]

lgtm modulo nits. Obviously wait for adamk, though.

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
File
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html
(right):

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html:20:
window.open("https://127.0.0.1:8443/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html");
Why does this need to be a "window.open" and not an iframe?

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
File
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html
(right):

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html:3:
var inv_doc = document.implementation.createHTMLDocument("");
nit: make this a more explicit variable name, like invisible_doc or
invisible_document

https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
Source/core/html/HTMLFormElement.cpp:506: // Check if the DOM has a frame before
checking mixed content (crbug/398066).
It's clear that you're checking if the DOM has a frame from the code. Make this
comment more about the "why". Something like, "Blink needs to check if the DOM
has a frame because the mixed content checker assumes it has a frame. See bug
xxx."

[mhm, 2014-08-01 23:28:45]

adamk@ can you please owner review this for me.

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
File
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html
(right):

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html:20:
window.open("https://127.0.0.1:8443/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html");
On 2014/08/01 23:20:52, jww wrote:
> Why does this need to be a "window.open" and not an iframe?

just following the pattern with all other mixed content checks.

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
File
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html
(right):

https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html:3:
var inv_doc = document.implementation.createHTMLDocument("");
On 2014/08/01 23:20:52, jww wrote:
> nit: make this a more explicit variable name, like invisible_doc or
> invisible_document

Done.

https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
Source/core/html/HTMLFormElement.cpp:506: // Check if the DOM has a frame before
checking mixed content (crbug/398066).
On 2014/08/01 23:20:52, jww wrote:
> It's clear that you're checking if the DOM has a frame from the code. Make
this
> comment more about the "why". Something like, "Blink needs to check if the DOM
> has a frame because the mixed content checker assumes it has a frame. See bug
> xxx."

Done.

[jww, 2014-08-01 23:33:42]

On 2014/08/01 23:28:45, mhm wrote:
> adamk@ can you please owner review this for me.
> 
>
https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
> File
>
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html
> (right):
> 
>
https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
>
LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-invisible-DOM.html:20:
>
window.open("https://127.0.0.1:8443/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html");
> On 2014/08/01 23:20:52, jww wrote:
> > Why does this need to be a "window.open" and not an iframe?
> 
> just following the pattern with all other mixed content checks.
> 
>
https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
> File
>
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html
> (right):
> 
>
https://codereview.chromium.org/431273006/diff/1/LayoutTests/http/tests/secur...
>
LayoutTests/http/tests/security/mixedContent/resources/frame-with-invisible-DOM-with-insecure-form.html:3:
> var inv_doc = document.implementation.createHTMLDocument("");
> On 2014/08/01 23:20:52, jww wrote:
> > nit: make this a more explicit variable name, like invisible_doc or
> > invisible_document
> 
> Done.
> 
>
https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
> File Source/core/html/HTMLFormElement.cpp (right):
> 
>
https://codereview.chromium.org/431273006/diff/1/Source/core/html/HTMLFormEle...
> Source/core/html/HTMLFormElement.cpp:506: // Check if the DOM has a frame
before
> checking mixed content (crbug/398066).
> On 2014/08/01 23:20:52, jww wrote:
> > It's clear that you're checking if the DOM has a frame from the code. Make
> this
> > comment more about the "why". Something like, "Blink needs to check if the
DOM
> > has a frame because the mixed content checker assumes it has a frame. See
bug
> > xxx."
> 
> Done.

I think Adam should more than owner review this one :-) He's much more of a DOM
wizard then we are; I consider myself a sanity check on this.

[jww, 2014-08-01 23:35:43]

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:506: // Blink needs to check if the form is
associted with a frame because mixed content checker
Can you please wrap this comment to 80 characters? Also, should be "...because
the mixed content checker..."

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:507: // assumes it has a fram. See bug
398066.
fram -> frame

[adamk, 2014-08-01 23:43:29]

I think there's a typo for the followup bug in your CL description...

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:506: // Blink needs to check if the form is
associted with a frame because mixed content checker
I don't think this comment is useful, the very next line accesses
document().frame(), so checking that it's non-null is a clear prerequisite for
that. The bug information will remain in the history through the BUG= line in
your commit message.

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:508: if (document().frame()) {
Nit: if { if looks funny to me, please make this:

if (document().frame() && ...)

[mhm, 2014-08-02 00:08:52]

adamk@ I think it is time for DOM wizard skills jww@ is referring to :-)

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:506: // Blink needs to check if the form is
associted with a frame because mixed content checker
On 2014/08/01 23:35:42, jww wrote:
> Can you please wrap this comment to 80 characters? Also, should be "...because
> the mixed content checker..."

Done.

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:507: // assumes it has a fram. See bug
398066.
On 2014/08/01 23:35:42, jww wrote:
> fram -> frame

Done.

[adamk, 2014-08-02 00:16:18]

On 2014/08/02 at 00:08:52, mohammed wrote:
> adamk@ I think it is time for DOM wizard skills jww@ is referring to :-)

I responded on the last patchset...did you not see that message?

[mhm, 2014-08-04 15:15:44]

Apologies adamk@ I saw them after sending the reply.

I addressed them in this patch. Can you please have a look at it.

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:506: // Blink needs to check if the form is
associted with a frame because mixed content checker
On 2014/08/01 23:43:29, adamk wrote:
> I don't think this comment is useful, the very next line accesses
> document().frame(), so checking that it's non-null is a clear prerequisite for
> that. The bug information will remain in the history through the BUG= line in
> your commit message.

Done.

https://codereview.chromium.org/431273006/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:508: if (document().frame()) {
On 2014/08/01 23:43:28, adamk wrote:
> Nit: if { if looks funny to me, please make this:
> 
> if (document().frame() && ...)

Done.

[adamk, 2014-08-04 16:15:38]

lgtm

[mhm, 2014-08-04 16:55:13]

The CQ bit was checked by mohammed@chromium.org

[commit-bot: I haz the power, 2014-08-04 16:55:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohammed@chromium.org/431273006/60001

[commit-bot: I haz the power, 2014-08-04 18:02:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20569)

[commit-bot: I haz the power, 2014-08-04 18:45:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:45:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20579)

[mhm, 2014-08-05 21:31:42]

The CQ bit was checked by mohammed@chromium.org

[commit-bot: I haz the power, 2014-08-05 21:33:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohammed@chromium.org/431273006/60001

[commit-bot: I haz the power, 2014-08-05 21:35:28]

Change committed as 179530