[Webkit-unassigned] [Bug 38424] add support for text/html-sandboxed on sandboxed iframes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38424


Patrik Persson <patrik.j.persson at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #65708|0                           |1
        is obsolete|                            |
  Attachment #65708|review?, commit-queue?      |
               Flag|                            |
  Attachment #66053|                            |review?, commit-queue?
               Flag|                            |




--- Comment #24 from Patrik Persson <patrik.j.persson at ericsson.com>  2010-08-31 08:00:59 PST ---
Created an attachment (id=66053)
 --> (https://bugs.webkit.org/attachment.cgi?id=66053)
Revised patch

Thanks for your comments, all very valid.

(In reply to comment #22)

> By the way, do we want to add an inspector message explaining why the load failed?  Previously, we've had problems with authors getting confused when we block loads like this for security.

Added.  I also added an explicit error message (see below) for the user.

(In reply to comment #23)

> Why are some of the special cases for text/html-sandboxed inside #if but others not?

I originally attempted to use as few #if's as possible, but I realize that's probably confusing.  I have now added #if's more consistenly.  However, one check remains when ENABLE(SANDBOX) is disabled, to ensure that html-sandboxed content is still rejected in that case.

> Can this possibly be right? It seems we treat this as an eternally uncommitted load rather than a failure. I don't think that's what we want. Is this really the right bottleneck?

Good point.  I moved the check to DocumentLoader::commitLoad, to make sure the decision is made as early as possible.  I also added an explicit error.

The cannotShowURLError is the least surprising error I could come up with without introducing a new one.  The console message (above) provides developers more hints on what happened.

> It seems quite error prone that in all these places we have separate text/html-sandboxed strings, and any could have a typo. Do we have sufficient test coverage so we'd get a failure if any one of these strings was wrong?

I agree.  I could come up with two potential errors from a typo:

* text/html-sandboxed content could sneak past the check in
  DocumentLoader. This is checked for in the tests that verify the
  difference between html and html-sandboxed content.

* text/html-sandboxed content would be rendered as text/plain (as it
  is done without this patch).  I have added a section break to the
  untrusted parts of the test cases, which should be rendered
  differently if the content is (incorrectly) handled as text/plain.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.