Summary: | [JSC] Don't sanitize window.onerror information on crossorigin-enabled scripts | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Kieffer <broofa> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | Pablo Flouret <pf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, ap, browser-bugs, chris.cinelli, doochik, dpetroff, ggaren, ian, ide+webkit, pf, rniwa, sam, todd, webkit.review.bot, wickedjack | ||||||||||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=132945 | ||||||||||||||
Bug Depends on: | 81438 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Robert Kieffer
2011-10-20 19:26:21 PDT
We should just use CORS for this. The script tag should have a crossorigin attribute like the img tag does. If the CORS check passes, then we can feel confident in forwarding the errors to the embedding page. Adam, good idea! https://developer.mozilla.org/en/CORS_Enabled_Image, but for script tags looks like just the ticket. :) I wrote a blog post with some background information about this bug: http://www.schemehostport.com/2011/10/x-script-origin-we-hardly-knew-ye.html The issue was solved in firefox, is there any chance to see it google chrome soon? > The issue was solved in firefox, is there any chance to see it google chrome soon?
We have a good idea what the API should look like. We just need someone to write a patch that implements it.
Created attachment 132158 [details]
work-in-progress
(In reply to comment #6) > Created an attachment (id=132158) [details] > work-in-progress I've been looking at this on-and-off the past weeks, and the plumbing is a bit tricky. Uploaded a wip patch with what i have so far for JSC, couldn't find a way to do it in v8 at a glance. It'd be nice to hear from someone more experienced with v8 bindings if there's a viable way to do it with existing infrastructure or if bigger changes are needed on the whole. Hopefully i'm missing something obvious ;-) Comment on attachment 132158 [details]
work-in-progress
This looks like a nice approach. You might want to split this patch up into two pieces. The first might just add support for the crossorigin attribute to the <script> element. The second could use that information when sanitizing the error reporting. I'm not sure how to do that latter in V8 off the top of my head, but I'm sure we can figure out a way.
(In reply to comment #8) > (From update of attachment 132158 [details]) > This looks like a nice approach. You might want to split this patch up into two pieces. The first might just add support for the crossorigin attribute to the <script> element. The second could use that information when sanitizing the error reporting. I'm not sure how to do that latter in V8 off the top of my head, but I'm sure we can figure out a way. Split off the attribute part to bug 81438. It would've been nice if Script::SetData had taken a void* instead of v8::String, but anyway, would it be wise to use V8BindingPerIsolateData to store a ScriptSourceCode or CachedScript while the script is executing? Is only one evaluated script ever active per isolate at a given time? Not very pretty, but it might do the job... Firefox now supports this in release builds and IE doesn't implement the "Script error." part of the HTML spec so now it's largely WebKit errors that are opaque. Can this feature be prioritized accordingly? Being able to rely on window.onerror is very helpful for debugging at scale. I'm not sure anything changed in v8 to make this easier to accomplish since last time i tried, but maybe in the meantime we can land the part that deals with JSC, which is pretty straightforward; and i can take another look at v8 when i get some time? (In reply to comment #12) > I'm not sure anything changed in v8 to make this easier to accomplish since last time i tried, but maybe in the meantime we can land the part that deals with JSC, which is pretty straightforward; and i can take another look at v8 when i get some time? In order to land anything here, the patch needs to be cleaned up and tests added. (In reply to comment #13) > (In reply to comment #12) > > I'm not sure anything changed in v8 to make this easier to accomplish since last time i tried, but maybe in the meantime we can land the part that deals with JSC, which is pretty straightforward; and i can take another look at v8 when i get some time? > > In order to land anything here, the patch needs to be cleaned up and tests added. Sure, that's why it says work-in-progress :-) I was just wondering if it's a good idea to go ahead with the JSC part first, and look into fixing the v8 side at an unspecified future time, or if it's better to have it working in both at the same time. (In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > I'm not sure anything changed in v8 to make this easier to accomplish since last time i tried, but maybe in the meantime we can land the part that deals with JSC, which is pretty straightforward; and i can take another look at v8 when i get some time? > > > > In order to land anything here, the patch needs to be cleaned up and tests added. > > Sure, that's why it says work-in-progress :-) > > I was just wondering if it's a good idea to go ahead with the JSC part first, and look into fixing the v8 side at an unspecified future time, or if it's better to have it working in both at the same time. That's fine. JavaScriptCore is the javascript engine of the WebKit project, and Google has agreed to keep their V8 bindings up to date, but there is not a compelling reason to wait. Created attachment 165709 [details]
Patch
Filed bug 97499 for V8. Attachment 165709 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
Source/WebCore/bindings/js/JSDOMBinding.h:58: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 165709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165709&action=review > Source/WebCore/bindings/js/ScriptSourceCode.h:54 > + ScriptSourceCode(CachedScript* cachedScript) Should this constructor be marked explicit? > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:78 > + m_crossOriginMode = attributeValue; There's no whitespace stripping? > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:135 > + request.setAllowCookies(equalIgnoringCase(m_crossOriginMode, "use-credentials")); It feels odd to be doing a string comparison here. Maybe we should have a helper function that can give us back a cross origin mode enum value? > LayoutTests/http/tests/security/resources/cors-script.php:5 > + echo "ohhaiconga;"; ohhaiconga -> ? Comment on attachment 165709 [details] Patch Attachment 165709 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14018441 New failing tests: http/tests/workers/terminate-during-sync-operation.html (In reply to comment #19) > (From update of attachment 165709 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165709&action=review > > > LayoutTests/http/tests/security/resources/cors-script.php:5 > > + echo "ohhaiconga;"; > > ohhaiconga -> ? Heh, quick way to get an exception, forgot to change it to a more serious/explicit variant. I'll fix it and the rest of the issues. Created attachment 165904 [details]
Patch
Attachment 165904 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/js/JSDOMBinding.h:58: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
ping? Comment on attachment 165904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165904&action=review It looks like all of Adam's prior comments have been addressed, and this patch looks good to me, so I'll say r+. Please fix the initializer before landing. > Source/WebCore/bindings/js/ScriptSourceCode.h:49 > + , m_cachedScript(0) Explicit initialization is not required here. The class default-initializes to null. > Source/WebCore/bindings/js/ScriptSourceCode.h:78 > RefPtr<JSC::SourceProvider> m_provider; > > JSC::SourceCode m_code; > - > + > + CachedResourceHandle<CachedScript> m_cachedScript; It's a little disappointing to have both a SourceProvider and a CachedScript pointer, since these will point to the same thing. But I don't have an immediate suggestion for how to fix this. Created attachment 174298 [details]
Patch for landing
Comment on attachment 174298 [details] Patch for landing Rejecting attachment 174298 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rity/script-crossorigin-onerror-information.html patching file LayoutTests/http/tests/security/script-no-crossorigin-onerror-should-be-sanitized-expected.txt patching file LayoutTests/http/tests/security/script-no-crossorigin-onerror-should-be-sanitized.html patching file LayoutTests/platform/chromium/TestExpectations Hunk #1 succeeded at 4112 (offset -17 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/14846931 Created attachment 174745 [details]
Rebased patch for landing
Comment on attachment 174745 [details] Rebased patch for landing Clearing flags on attachment: 174745 Committed r135009: <http://trac.webkit.org/changeset/135009> Please move the status to completed and add the patch. We host the static resource on Amazon and our logs of frontend errors are useless with line: 0 and error "Script error". Please consider expanding onerror and add a 4th parameter that can give more info on what happened (ex: exception). Adding also the stack trace would be great, but if we have at least the exception and the context in the meanwhile we can still use: https://github.com/eriwen/javascript-stacktrace to find out what happened. > Please consider expanding onerror and add a 4th parameter
This sounds like a separate feature request. Can you file a feature request bug for it?
(In reply to comment #32) > > Please consider expanding onerror and add a 4th parameter > > This sounds like a separate feature request. Can you file a feature request bug for it? I seem to recall some threads with similar requests in the whatwg list as well. At least this one, http://lists.w3.org/Archives/Public/public-whatwg-archive/2012May/thread.html#msg124 Yes, that conversation is aligned with what I am asking. Did it have any follow-up ? Should add a new enhancement request referencing that message? Ok, I added it here: https://bugs.webkit.org/show_bug.cgi?id=104408 I think this may have regressed. I am not seeing this behavior in Chrome 32.0.1700.107 or Safari 5.1.7. I put together a test case at: http://trackjs.com/demo/bug.html Scripts are both loaded from an AWS domain and have the crossorigin attribute. The error that occurs on button click is santitized before being given to the window.onerror handler. Safari 5.1.7 is very old. Please file a new bg if you can reproduce this with Safari 7. Google has forked WebKit, so Chrome code is separate now. I just reproduced this with Safari7. Please file a new bug if you can reproduce this with Safari 7. I have, see here: https://bugs.webkit.org/show_bug.cgi?id=132945 |