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

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

Created:
5 years, 7 months ago by Mike West
Modified:
5 years, 6 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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: > > ...
5 years, 7 months 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: > > ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months ago (2013-07-25 12:14:53 UTC) #9
jochen (gone - plz use gerrit)
i like it. lgtm.
5 years, 7 months 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 ...
5 years, 7 months 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. ...
5 years, 7 months 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 > ...
5 years, 7 months 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 ...
5 years, 7 months 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. ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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 ...
5 years, 7 months 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: ...
5 years, 7 months 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 ...
5 years, 6 months 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 ...
5 years, 6 months 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 ...
5 years, 6 months 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!" ...
5 years, 6 months 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 ...
5 years, 6 months 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 ...
5 years, 6 months 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 ...
5 years, 6 months 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
5 years, 6 months ago (2013-08-07 07:33:12 UTC) #27
commit-bot: I haz the power
5 years, 6 months 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