Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 19596004: Allow sites to enable 'window.onerror' handlers for cross-domain scripts. (Closed)

Created:
6 years ago by Mike West
Modified:
6 years ago
CC:
blink-reviews, Nils Barth (inactive), kojih, jsbell+bindings_chromium.org, eae+blinkwatch, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, haraken, do-not-use, Yang, japhet-do-not-use
Visibility:
Public.

Description

Allow sites to enable detailed 'window.onerror' handlers for cross-domain scripts. When triggering 'window.onerror', we currently sanitize the contents of the error if the script in which the error occurred isn't from the same origin as the document that loaded the script. Other major browsers (IE, Firefox[1], and WebKit[2]) bypass this sanitization step iff the script is served with appropriate 'Access-Control-Allow-Origin' headers that grant the loading document access to the script's contents. Clever developers agree[3] that this is a reasonable solution. This patch aligns our behavior with those browsers by passing the CORS state of a script through V8 so that it's available to us when exceptions are thrown. Note that this patch does not address the case of scripts imported into Workers. Our behavior there is already poor; it will require a bit more rework to correctly handle the basic case before moving on to implementing CORS support. Intent to Implement discussion at [4]. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=696301 [2]: https://bugs.webkit.org/show_bug.cgi?id=70574 [3]: http://www.schemehostport.com/2011/10/x-script-origin-we-hardly-knew-ye.html [4]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Li61lfcbWws/NuUUNofRciMJ BUG=159566 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155670

Patch Set 1 #

Patch Set 2 : WTF::HashSet FTW! #

Total comments: 1

Patch Set 3 : Rework. #

Total comments: 11

Patch Set 4 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+212 lines, -106 lines) Patch
M LayoutTests/http/tests/security/resources/cors-script.php View 1 2 1 chunk +3 lines, -1 line 0 comments Download
D LayoutTests/http/tests/security/script-crossorigin-onerror-information.html View 1 2 1 chunk +0 lines, -21 lines 0 comments Download
D LayoutTests/http/tests/security/script-crossorigin-onerror-information-expected.txt View 1 2 3 1 chunk +0 lines, -13 lines 0 comments Download
D LayoutTests/http/tests/security/script-no-crossorigin-onerror-should-be-sanitized.html View 1 2 1 chunk +0 lines, -20 lines 0 comments Download
D LayoutTests/http/tests/security/script-no-crossorigin-onerror-should-be-sanitized-expected.txt View 1 2 3 1 chunk +0 lines, -13 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-crossorigin-cors-expected.txt View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-crossorigin-no-cors.html View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-crossorigin-no-cors-expected.txt View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-no-crossorigin-cors.html View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-no-crossorigin-cors-expected.txt View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-no-crossorigin-no-cors.html View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/script-onerror-no-crossorigin-no-cors-expected.txt View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M Source/bindings/v8/ScriptController.h View 1 2 3 3 chunks +4 lines, -3 lines 0 comments Download
M Source/bindings/v8/ScriptController.cpp View 1 2 3 4 chunks +4 lines, -4 lines 0 comments Download
M Source/bindings/v8/V8Initializer.cpp View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download
M Source/bindings/v8/V8ScriptRunner.h View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M Source/bindings/v8/V8ScriptRunner.cpp View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M Source/bindings/v8/WorkerScriptController.cpp View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/dom/ScriptExecutionContext.h View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M Source/core/dom/ScriptExecutionContext.cpp View 1 2 3 3 chunks +6 lines, -17 lines 0 comments Download
M Source/core/dom/ScriptLoader.cpp View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
M Source/core/loader/CrossOriginAccessControl.h View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/workers/WorkerMessagingProxy.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 28 (0 generated)
Mike West
Hi Jochen, this is the bug we briefly discussed at lunch. Would you mind taking ...
6 years ago (2013-07-23 11:49:33 UTC) #1
jochen (gone - plz use gerrit)
what about the worker version of the code? Wouldn't it be enough to store the ...
6 years ago (2013-07-23 14:15:08 UTC) #2
Mike West
On 2013/07/23 14:15:08, jochen wrote: > what about the worker version of the code? No ...
6 years ago (2013-07-23 14:35:00 UTC) #3
Mike West
On 2013/07/23 14:35:00, Mike West wrote: > On 2013/07/23 14:15:08, jochen wrote: > > what ...
6 years ago (2013-07-23 14:57:35 UTC) #4
Mike West
On 2013/07/23 14:57:35, Mike West wrote: > On 2013/07/23 14:35:00, Mike West wrote: > > ...
6 years ago (2013-07-24 18:16:45 UTC) #5
Mike West
On 2013/07/24 18:16:45, Mike West wrote: > On 2013/07/23 14:57:35, Mike West wrote: > > ...
6 years ago (2013-07-24 18:55:08 UTC) #6
jochen (gone - plz use gerrit)
Nate, do you know whether it's safe to assume that the CachedResource for a script ...
6 years ago (2013-07-24 19:19:08 UTC) #7
Nate Chapin
On 2013/07/24 19:19:08, jochen wrote: > Nate, do you know whether it's safe to assume ...
6 years ago (2013-07-24 20:43:34 UTC) #8
Mike West
Alright, so CachedScript is right out. The current patch: a) Removes the CachedScript* parameters from ...
6 years ago (2013-07-25 12:14:53 UTC) #9
jochen (gone - plz use gerrit)
i like it. lgtm.
6 years ago (2013-07-25 13:15:16 UTC) #10
Mike West
On 2013/07/25 13:15:16, jochen wrote: > i like it. lgtm. Cool. I'll wait for the ...
6 years ago (2013-07-25 13:17:20 UTC) #11
abarth-chromium
https://codereview.chromium.org/19596004/diff/11001/Source/core/dom/ScriptExecutionContext.cpp File Source/core/dom/ScriptExecutionContext.cpp (right): https://codereview.chromium.org/19596004/diff/11001/Source/core/dom/ScriptExecutionContext.cpp#newcode338 Source/core/dom/ScriptExecutionContext.cpp:338: return securityOrigin()->canRequest(url) || m_scriptsPassingAccessControlCheck.contains(url.string().impl()->hash()); not lgtm This is insecure. ...
6 years ago (2013-07-25 18:19:15 UTC) #12
jochen (gone - plz use gerrit)
On 2013/07/25 18:19:15, abarth wrote: > https://codereview.chromium.org/19596004/diff/11001/Source/core/dom/ScriptExecutionContext.cpp > File Source/core/dom/ScriptExecutionContext.cpp (right): > > https://codereview.chromium.org/19596004/diff/11001/Source/core/dom/ScriptExecutionContext.cpp#newcode338 > ...
6 years ago (2013-07-25 18:55:12 UTC) #13
Mike West
On 2013/07/25 18:55:12, jochen wrote: > On 2013/07/25 18:19:15, abarth wrote: > > > https://codereview.chromium.org/19596004/diff/11001/Source/core/dom/ScriptExecutionContext.cpp ...
6 years ago (2013-07-25 20:57:50 UTC) #14
abarth-chromium
On 2013/07/25 20:57:50, Mike West wrote: > The current code is more or less KURLHash. ...
6 years ago (2013-07-25 22:25:18 UTC) #15
abarth-chromium
In fact, even assuming a URL match implies that the access control check passed is ...
6 years ago (2013-07-25 22:26:19 UTC) #16
Mike West
On 2013/07/25 22:25:18, abarth wrote: > A better solution is to store this information on ...
6 years ago (2013-07-25 22:50:10 UTC) #17
Mike West
I thought about this a bit more today: I think checking the CORS status before ...
6 years ago (2013-07-26 16:49:13 UTC) #18
abarth-chromium
On 2013/07/26 16:49:13, Mike West wrote: > I thought about this a bit more today: ...
6 years ago (2013-07-26 17:28:36 UTC) #19
Mike West
The current patchset works locally, but relies on https://codereview.chromium.org/20646006/ landing in V8, and rolling into ...
6 years ago (2013-07-30 14:36:55 UTC) #20
Mike West
V8 rolled, so I'm optimistically throwing this to the bots. It might even compile, but ...
6 years ago (2013-08-05 20:43:20 UTC) #21
abarth-chromium
LGTM to unblock you, but I think this CL could use another iteration. https://chromiumcodereview.appspot.com/19596004/diff/26001/Source/bindings/v8/V8ScriptRunner.cpp File ...
6 years ago (2013-08-05 22:31:44 UTC) #22
Mike West
https://chromiumcodereview.appspot.com/19596004/diff/26001/Source/bindings/v8/V8ScriptRunner.cpp File Source/bindings/v8/V8ScriptRunner.cpp (right): https://chromiumcodereview.appspot.com/19596004/diff/26001/Source/bindings/v8/V8ScriptRunner.cpp#newcode74 Source/bindings/v8/V8ScriptRunner.cpp:74: printf("V8ScriptRunner::CompileScript: corsStatus == ScriptIsSharedCrossOrigin %s\n", corsStatus == ScriptIsSharedCrossOrigin? "true!" ...
6 years ago (2013-08-06 06:53:06 UTC) #23
Mike West
I've given it another pass. Things got quite a bit cleaner after rebasing on top ...
6 years ago (2013-08-06 07:54:03 UTC) #24
Mike West
Bots are happy, but this changed enough from the last patchset that I'd appreciate a ...
6 years ago (2013-08-06 13:03:54 UTC) #25
Mike West
On 2013/08/06 13:03:54, Mike West wrote: > Bots are happy, but this changed enough from ...
6 years ago (2013-08-07 07:33:03 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkwst@chromium.org/19596004/38001
6 years ago (2013-08-07 07:33:12 UTC) #27
commit-bot: I haz the power
6 years ago (2013-08-07 09:26:23 UTC) #28
Message was sent while issue was closed.
Change committed as 155670

Powered by Google App Engine
This is Rietveld 408576698