Throw an exception when denying access to 'Frame's 'location' setter.

Throw an exception when denying access to 'Frame's 'location' setter.

Currently, we write an access-denied message to the console when we deny
a page's attempt to set a frame's location to a 'javascript:' URL. This
patch changes our behavior to throw an exception.


Firefox currently does not throw an exception, but silently denies
access to set the property cross-origin. I don't believe that's behavior
we should seek to replicate.

This patch removes the one-off
'BindingSecurity::allowSettingFrameSrcToJavascriptUrl', moving the guts of
the protocol check into the custom bindings, and delegating the security
aspects to 'allowAccessToFrame'. 'allowAccessToFrame' can now accept an
'ExceptionState' rather than a reporting enum, and that's piped through
to a new 'canAccessDocument' method. This has the happy effect of
beginning to put the pieces in place for future patches which will
migrate other 'allowAccessToFrame' calls to the new, exception-throwing
model.

The patch also adds 'ExceptionState::throwSecurityError', which
accepts two strings: a sanitized string, and an unsanitized optional string.
Those values get piped through V8ThrowException, and are stored on
'DOMException' which tunnels through V8 and pops out in 'V8Initializer'.
There, I set the unsanitized message on the 'ErrorEvent' object that's
handed off to the exception reporting code in 'ScriptExecutionContext'.

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

[Mike West, 2013-08-12 12:36:53]

Hi Adam, Jochen,

This patch adjusts 'Frame''s location setter to throw an exception when a
'javascript:' URL is set cross-origin. This is in-line with my general goal of
changing our cross-origin checks to throw exceptions rather than spamming the
console (generally, this is what Firefox does, though unfortunately not for this
_particular_ case).

Would you mind taking a look at the patch to see if it makes sense?

Future patches I have lined up in my head will add a similar 'ExceptionState'
variant to 'shouldAllowAccessToNode', and start migrating custom callsites over
one-by-one before adjusting the bindings code to catch the rest.

WDYT?

-mike

[jochen (gone - plz use gerrit), 2013-08-12 12:57:23]

I defer to Adam on this one

[abarth-chromium, 2013-08-12 19:57:03]

LGTM with some minor comments.

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
File Source/bindings/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.cpp:59: static bool
canAccessDocument(Document* targetDocument, ExceptionState& es)
What's the point of having this be a separate function?  There's exactly one
caller.

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.cpp:75:
frame->domWindow()->printErrorMessage(targetDocument->domWindow()->crossDomainAccessErrorMessage(activeDOMWindow()));
It's kine of lame that we call activeDOMWindow twice now.

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
File Source/bindings/v8/BindingSecurity.h (right):

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.h:52: static bool
shouldAllowAccessToFrame(Frame*, ExceptionState&);
How does this function differ from the one above it?

[abarth-chromium, 2013-08-12 19:58:25]

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
File Source/bindings/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.cpp:64: es.throwDOMException(SecurityError,
targetDocument->domWindow()->crossDomainAccessErrorMessage(activeDOMWindow()));
Wait a minute.  not lgtm.  This leaks the current URL of the frame, right?

[abarth-chromium, 2013-08-12 19:59:18]

https://codereview.chromium.org/22829002/diff/5001/LayoutTests/http/tests/sec...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt
(right):

https://codereview.chromium.org/22829002/diff/5001/LayoutTests/http/tests/sec...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt:3:
CONSOLE MESSAGE: line 34: Caught exception while setting frame's location to
'javascript	:"FAIL: this should not have been loaded."'. 'SecurityError: Blocked
a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin
"http://localhost:8000". Protocols, domains, and ports must match.'.
SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from
accessing a frame with origin "http://localhost:8000". Protocols, domains, and
ports must match.

Is the string "http://localhost:8000" contained in the exception in a way that's
accessible to the caller?

[Mike West, 2013-08-13 08:30:50]

Thanks Adam. That was dumb of me.

Mind taking another look?

-mike

https://codereview.chromium.org/22829002/diff/5001/LayoutTests/http/tests/sec...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt
(right):

https://codereview.chromium.org/22829002/diff/5001/LayoutTests/http/tests/sec...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt:3:
CONSOLE MESSAGE: line 34: Caught exception while setting frame's location to
'javascript	:"FAIL: this should not have been loaded."'. 'SecurityError: Blocked
a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin
"http://localhost:8000". Protocols, domains, and ports must match.'.
On 2013/08/12 19:59:18, abarth wrote:

> Is the string "http://localhost:8000" contained in the exception in a way
that's
> accessible to the caller?

It is. That was stupid. :/

I'm unsure which details are safe to hand to JavaScript. In this CL, I'd like to
introduce a 'sanitizedCrossDomainAccessErrorMessage' method that reverts back to
our old behavior of just saying "Hey, you're denied access.". As I work through
other examples of access violation exceptions, we can evaluate which bits of
information might be safe to report.

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
File Source/bindings/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.cpp:59: static bool
canAccessDocument(Document* targetDocument, ExceptionState& es)
On 2013/08/12 19:57:03, abarth wrote:
> What's the point of having this be a separate function?  There's exactly one
> caller.

This is the first patch that throws an exception on access violation, but my
goal is to move all the current usage over to the exception-throwing version.
So, there's exactly one caller _for now_. :)

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.cpp:75:
frame->domWindow()->printErrorMessage(targetDocument->domWindow()->crossDomainAccessErrorMessage(activeDOMWindow()));
On 2013/08/12 19:57:03, abarth wrote:
> It's kine of lame that we call activeDOMWindow twice now.

Good point. Fixed.

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
File Source/bindings/v8/BindingSecurity.h (right):

https://codereview.chromium.org/22829002/diff/5001/Source/bindings/v8/Binding...
Source/bindings/v8/BindingSecurity.h:52: static bool
shouldAllowAccessToFrame(Frame*, ExceptionState&);
On 2013/08/12 19:57:03, abarth wrote:
> How does this function differ from the one above it?

The new method accepts an 'ExceptionState&' object and always uses it to report
the access violation. My goal here is to slowly replace the code using the
current version of the method (which always writes an unsuppressable message to
the console) with the new version (which throws a catchable exception).

[abarth-chromium, 2013-08-13 20:09:30]

https://codereview.chromium.org/22829002/diff/14001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt
(left):

https://codereview.chromium.org/22829002/diff/14001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-expected.txt:3:
CONSOLE MESSAGE: Blocked a frame with origin "http://127.0.0.1:8000" from
accessing a frame with origin "http://localhost:8000". Protocols, domains, and
ports must match.
It's a shame that the console message itself is now less useful.  Is there some
way of including the target origin in the console message but not making it
available to JavaScript?

[Use mkwst_at_chromium.org plz., 2013-08-13 20:56:08]

On 2013/08/13 20:09:30, abarth wrote:
> It's a shame that the console message itself is now less useful.  Is there
some
> way of including the target origin in the console message but not making it
> available to JavaScript?

Hrm. When we originally discussed the topic on webkit-dev, the proposal was to
be able to pass two strings: one awesomely detailed for the console, and one
sanitized for JavaScript.

I was hoping to avoid that complexity, as I'm not sure it's going to be clear to
Blink developers. Having one and only one message would make the requirements
simpler and less likely that something unintentional would slip through.

That said, Erik's solution is better than what I'd originally played around
with. Perhaps adding an appropriately named method to ExceptionState might work.

+arv for comment.

[arv (Not doing code reviews), 2013-08-13 23:15:17]

On 2013/08/13 20:56:08, mkwst wrote:
> On 2013/08/13 20:09:30, abarth wrote:
> > It's a shame that the console message itself is now less useful.  Is there
> some
> > way of including the target origin in the console message but not making it
> > available to JavaScript?
> 
> Hrm. When we originally discussed the topic on webkit-dev, the proposal was to
> be able to pass two strings: one awesomely detailed for the console, and one
> sanitized for JavaScript.
> 
> I was hoping to avoid that complexity, as I'm not sure it's going to be clear
to
> Blink developers. Having one and only one message would make the requirements
> simpler and less likely that something unintentional would slip through.
> 
> That said, Erik's solution is better than what I'd originally played around
> with. Perhaps adding an appropriately named method to ExceptionState might
work.
> 
> +arv for comment.

What does the inspector get? Does it have access to the DOMException (C++
object) or just the wrapper of it? If it has access to the DOMException object
maybe we can add another field to that?

[Use mkwst_at_chromium.org plz., 2013-08-13 23:28:59]

On 2013/08/13 23:15:17, arv wrote:

> What does the inspector get? Does it have access to the DOMException (C++
> object) or just the wrapper of it? If it has access to the DOMException object
> maybe we can add another field to that?

I'll have to look at the code again tomorrow, but I'm fairly sure that we end up
creating a v8::Exception object in bindings (somewhere in V8ThrowException),
that gets piped through V8, potentially handled, and then pops out the other in
as a v8::Message. That gets piped through
ScriptExecutionContext::reportException, and ends up in some
"logExceptionToConsole" method (on PageConsole?) after we throw it at
`window.onerror`.

I think we'd need to teach V8 about the additional string in order to make one
available to JavaScript, and another available to the console. That certainly
seems possible.

I'm more concerned about how this would look to the Blink developer, really. The
current API is simple: `es.throwDOMException(ExceptionType, "message");`. Adding
another string to the call (e.g. `es.throwDOMException(ExceptionType, "message",
"sanitizedMessage");` seems ripe for confusion. Which message is which?

Perhaps we should add some sort of scarily-named
'throwDOMExceptionWithSecurityImplication' method, and ASSERT that all
'SecurityError' exceptions use it?

Adam, are the cross-origin messages the only ones you're concerned about?

[abarth-chromium, 2013-08-14 06:08:55]

On 2013/08/13 23:28:59, mkwst wrote:
> Adam, are the cross-origin messages the only ones you're concerned about?

Yeah, just the ones where we've stripped out useful information about the thing
the script is trying to access.

[Mike West, 2013-08-14 12:52:20]

The latest patch adds 'ExceptionState::throwSecurityError', which accepts two
strings: a sanitized string, and an unsanitized optional string. Those values
get piped through V8ThrowException, and are stored on DOMException which tunnels
through V8 and pops out in V8Initializer. There, I set the unsanitized message
on the ErrorEvent object that's handed off to the exception reporting code in
ScriptExecutionContext.

This turned out to be a little simpler than I thought it'd be last night: we
actually do pass the DOMException object all the way through V8, so we can do
this all of this work in Blink.

Would you mind taking another look at these, Adam and Erik?

[arv (Not doing code reviews), 2013-08-14 13:43:02]

Overall this looks like a good approach. A few minor issues.

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt
(right):

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt:8:
This test passes as long as the onerror handler logs PASS to the console, and
the uncaught exception reported to the console has unsanitized detail.
According to this text this test is failing. It should log PASS if it passes.

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html
(right):

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html:4:
if (window.testRunner) {
Can you just use js-test-pre/post instead?

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html:29:
runTest = function()
function runTest() {

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.h:86: virtual void throwTypeError(const
String& message = String()) OVERRIDE FINAL;
Please add it here too... and in ExceptionStatePlaceholder

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
File Source/bindings/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
Source/bindings/v8/V8ThrowException.cpp:54: // Unsanitized messages are only
relevant for SecurityError exceptions.
Maybe add an assert outside this if.

ASSERT(ec == SecurityError || unsanitizedMessage.isEmpty());

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
Source/bindings/v8/V8ThrowException.cpp:76: v8::Handle<v8::Value> exception =
createDOMException(ec, sanitizedMessage, unsanitizedMessage, isolate);
Assert here too?

[Use mkwst_at_chromium.org plz., 2013-08-14 13:57:20]

Thank you, latest patchset addresses these comments.

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt
(right):

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt:8:
This test passes as long as the onerror handler logs PASS to the console, and
the uncaught exception reported to the console has unsanitized detail.
On 2013/08/14 13:43:03, arv wrote:
> According to this text this test is failing. It should log PASS if it passes.

The first line of the expectation is the "PASS" log message.

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html
(right):

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html:4:
if (window.testRunner) {
On 2013/08/14 13:43:03, arv wrote:
> Can you just use js-test-pre/post instead?

Not without modifying it to support a document without a body. It doesn't like
running in a frameset. :(

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught.html:29:
runTest = function()
On 2013/08/14 13:43:03, arv wrote:
> function runTest() {

Done.

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/Except...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/Except...
Source/bindings/v8/ExceptionState.h:86: virtual void throwTypeError(const
String& message = String()) OVERRIDE FINAL;
On 2013/08/14 13:43:03, arv wrote:
> Please add it here too... and in ExceptionStatePlaceholder

Very good catch, thank you.

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
File Source/bindings/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
Source/bindings/v8/V8ThrowException.cpp:54: // Unsanitized messages are only
relevant for SecurityError exceptions.
On 2013/08/14 13:43:03, arv wrote:
> Maybe add an assert outside this if.
> 
> ASSERT(ec == SecurityError || unsanitizedMessage.isEmpty());

Done.

https://codereview.chromium.org/22829002/diff/27001/Source/bindings/v8/V8Thro...
Source/bindings/v8/V8ThrowException.cpp:76: v8::Handle<v8::Value> exception =
createDOMException(ec, sanitizedMessage, unsanitizedMessage, isolate);
On 2013/08/14 13:43:03, arv wrote:
> Assert here too?

Done.

[arv (Not doing code reviews), 2013-08-14 15:14:09]

LGTM but please let Adam have a final look.

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt
(right):

https://codereview.chromium.org/22829002/diff/27001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom-uncaught-expected.txt:8:
This test passes as long as the onerror handler logs PASS to the console, and
the uncaught exception reported to the console has unsanitized detail.
On 2013/08/14 13:57:21, mkwst wrote:
> On 2013/08/14 13:43:03, arv wrote:
> > According to this text this test is failing. It should log PASS if it
passes.
> 
> The first line of the expectation is the "PASS" log message.

I can't believe I could not find that... I stared at this for a long time.

[abarth-chromium, 2013-08-14 19:42:07]

LGTM modulo the issue below.

https://codereview.chromium.org/22829002/diff/39001/Source/core/dom/ErrorEvent.h
File Source/core/dom/ErrorEvent.h (right):

https://codereview.chromium.org/22829002/diff/39001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:69: const String& message() const { return
!sanitizedMessage().isEmpty() ? sanitizedMessage() : unsanitizedMessage(); }
Woah, I don't think we should fall back to the sanitized message.  Can't we just
ASSERT that sanitizedMessage is always non-empty if there is a
unsanitizedMessage?

https://codereview.chromium.org/22829002/diff/39001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:75: const String& messageForConsole() const {
return !unsanitizedMessage().isEmpty() ? unsanitizedMessage() :
sanitizedMessage(); }
Fallback here makes sense.

https://codereview.chromium.org/22829002/diff/39001/Source/core/dom/ErrorEven...
Source/core/dom/ErrorEvent.h:87: const String& sanitizedMessage() const { return
m_sanitizedMessage; }
What's the point of these private accessors?  Code that can see these can see
the member variables too.

[commit-bot: I haz the power, 2013-08-15 12:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/22829002/52001

[commit-bot: I haz the power, 2013-08-15 12:42:24]

Change committed as 156151

[Use mkwst_at_chromium.org plz., 2013-08-15 19:25:47]

On 2013/08/15 12:42:24, I haz the power (commit-bot) wrote:
> Change committed as 156151

Reverted here: https://codereview.chromium.org/22887017

Seeing renderer crashes on Windows, and only on Windows. *sigh*
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%2...

[Use mkwst_at_chromium.org plz., 2013-08-15 19:27:22]

On 2013/08/15 19:25:47, mkwst wrote:
> On 2013/08/15 12:42:24, I haz the power (commit-bot) wrote:
> > Change committed as 156151
> 
> Reverted here: https://codereview.chromium.org/22887017
> 
> Seeing renderer crashes on Windows, and only on Windows. *sigh*
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Win7%2520%252...

Hrm. And these failures were visible on the win_layout bot here. I'm a bit
surprised that it landed with those failures. Ah well.