Disabling JavaScript in MHTML and limiting MHTML loading to top frames.

Restricts MHTML loading to top frames.
Also disabling JavaScript for MHTML documents.
Because JavaScipt is now disabled, MHTML layout tests had to be changed
not to use JavaScript anymore.

BUG=330663
TEST=Run MHTML layout tests.
R=abarth@chromium.org, jschuh@chromium.org

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

[abarth-chromium, 2014-02-12 22:09:15]

https://codereview.chromium.org/161383002/diff/40001/Source/core/loader/Docum...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/161383002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentLoader.cpp:629:
m_frame->settings()->setScriptEnabled(false);
Won't this setting persist until after the MHTML has left the frame?  Another
way of doing this is to call enforceSandboxFlags(...) on the document you
create.  Using the sandbox flags, you can disable JavaScript (and other features
if you wish), and they will propagate down to child documents.  They also won't
persist when the next Document is loaded into the frame.

[Jay Civelli, 2014-02-12 23:26:35]

Now using the sandbox flags.

(It seems my patch is in a weird state on codereview. I got couple of 500s when
uploading, then it would not show the page. Not sure if the server is having
issues or if my CL is cursed)

https://codereview.chromium.org/161383002/diff/40001/Source/core/loader/Docum...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/161383002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentLoader.cpp:629:
m_frame->settings()->setScriptEnabled(false);
On 2014/02/12 22:09:15, abarth wrote:
> Won't this setting persist until after the MHTML has left the frame?  Another
> way of doing this is to call enforceSandboxFlags(...) on the document you
> create.  Using the sandbox flags, you can disable JavaScript (and other
features
> if you wish), and they will propagate down to child documents.  They also
won't
> persist when the next Document is loaded into the frame.

Ah! Thanks for catching this, I did not realize the settings would persist.
I changed to use enforceSandboxFlags() with all sandbox flags, as I don't think
we need any of the features that we can sandbox (layout tests pass, trying a few
actual actual saved MHTML pages worked as well).

[abarth-chromium, 2014-02-12 23:35:38]

https://codereview.chromium.org/161383002/diff/180001/Source/core/loader/Docu...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/161383002/diff/180001/Source/core/loader/Docu...
Source/core/loader/DocumentLoader.cpp:284:
document()->enforceSandboxFlags(SandboxAll);
This is probably too late.  You want to enforce these flags from the beginning
of the load.  Isn't this the end of the load?

This code should probably be inside the MHTML code, right before it starts
adding content to the document...

[Jay Civelli, 2014-02-13 00:37:58]

On 2014/02/12 23:35:38, abarth wrote:
>
https://codereview.chromium.org/161383002/diff/180001/Source/core/loader/Docu...
> File Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/161383002/diff/180001/Source/core/loader/Docu...
> Source/core/loader/DocumentLoader.cpp:284:
> document()->enforceSandboxFlags(SandboxAll);
> This is probably too late.  You want to enforce these flags from the beginning
> of the load.  Isn't this the end of the load?
> 
> This code should probably be inside the MHTML code, right before it starts
> adding content to the document...

The way the MHTML code works is that it reads the main frame code and associated
resources and stuff them in the DocumentLoader.m_archiveResourceCollection, so
the MHTML code is not directly creating that Document.
From my (limited) understanding of DocumentLoader, in the archive case (MHTML
only now in blink), DocumentLoader::finishedLoading is called when the entire
MHTML file was loaded and at that point the Document has not yet been created.
The Document creation is triggered by DocumentLoader::maybeCreateArchive(),
which itself is called from DocumentLoader::finishedLoading().
So no JS code should have been able to run at this point I think.
I could move that code do maybeCreateArchive() if you prefer.

[abarth-chromium, 2014-02-13 01:22:37]

On 2014/02/13 00:37:58, Jay Civelli wrote:
> I could move that code do maybeCreateArchive() if you prefer.

Can we do this in DocumentLoader::responseReceived?

    if (isArchiveMIMEType(m_response.mimeType()) &&
m_mainResource->dataBufferingPolicy() != BufferData)
        m_mainResource->setDataBufferingPolicy(BufferData);

That looks like the point at which we decide that we're going to load an
archive.  Seems like we could set the sandbox bits on the document at that
point.  Notice that this function already calls enforceSandboxFlags a dozen or
so lines earlier for a different condition.

[Jay Civelli, 2014-02-13 01:51:39]

On 2014/02/13 01:22:37, abarth wrote:
> On 2014/02/13 00:37:58, Jay Civelli wrote:
> > I could move that code do maybeCreateArchive() if you prefer.
> 
> Can we do this in DocumentLoader::responseReceived?
> 
>     if (isArchiveMIMEType(m_response.mimeType()) &&
> m_mainResource->dataBufferingPolicy() != BufferData)
>         m_mainResource->setDataBufferingPolicy(BufferData);
> 
> That looks like the point at which we decide that we're going to load an
> archive.  Seems like we could set the sandbox bits on the document at that
> point.  Notice that this function already calls enforceSandboxFlags a dozen or
> so lines earlier for a different condition.

It does not work because we don't have a document at that point in the MHTML
case.
The document is creation is triggered in DocumentLoader::maybeCreateArchive()
which itself is called from DocumentLoader::finishedLoading().

[abarth-chromium, 2014-02-13 02:03:05]

On 2014/02/13 01:51:39, Jay Civelli wrote:
> It does not work because we don't have a document at that point in the MHTML
> case.
> The document is creation is triggered in DocumentLoader::maybeCreateArchive()
> which itself is called from DocumentLoader::finishedLoading().

Ok, sounds like we should do this in maybeCreateArchive right after triggering
creation of the document.

[Jay Civelli, 2014-02-13 02:12:22]

On 2014/02/13 02:03:05, abarth wrote:
> On 2014/02/13 01:51:39, Jay Civelli wrote:
> > It does not work because we don't have a document at that point in the MHTML
> > case.
> > The document is creation is triggered in
DocumentLoader::maybeCreateArchive()
> > which itself is called from DocumentLoader::finishedLoading().
> 
> Ok, sounds like we should do this in maybeCreateArchive right after triggering
> creation of the document.

Good idea. Done.

[abarth-chromium, 2014-02-13 02:14:16]

LGTM

Thanks for iterating on this CL.

[jschuh, 2014-02-13 04:01:10]

I don't know the code, but lgtm to whatever @abarth agreed to. And thanks for
taking care of this.

[jschuh, 2014-03-14 13:59:45]

Was there any particular hold-up on getting this landed?

[Jay Civelli, 2014-03-14 17:07:11]

I kept running into problems trying to land this.
Trybots would fail because the EOL was not CRLF in the svn repo.
Had to check-out an svn repo, got a read-only by mistake, gave me some
obscure errors when trying to land... and other issues.
Long story short the patch is now running through the try bots again and
will hopefully land shortly.



On Fri, Mar 14, 2014 at 6:59 AM, <jschuh@chromium.org> wrote:

> Was there any particular hold-up on getting this landed?
>
> https://codereview.chromium.org/161383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Jay Civelli, 2014-03-18 23:26:31]

Committed patchset #12 manually as r169499 (presubmit successful).