[webkit-reviews] review requested: [Bug 38424] add support for text/html-sandboxed on sandboxed iframes : [Attachment 66053] Revised patch

[bugzilla-daemon at webkit.org]

Patrik Persson <patrik.j.persson at ericsson.com> has asked  for review:
Bug 38424: add support for text/html-sandboxed on sandboxed iframes
https://bugs.webkit.org/show_bug.cgi?id=38424

Attachment 66053: Revised patch
https://bugs.webkit.org/attachment.cgi?id=66053&action=review

------- Additional Comments from Patrik Persson <patrik.j.persson at ericsson.com>
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.