Bug 70574 - [JSC] Don't sanitize window.onerror information on crossorigin-enabled scripts
: [JSC] Don't sanitize window.onerror information on crossorigin-enabled scripts
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Pablo Flouret
: InRadar, WebExposed
Depends on: 81438
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 19:26 PDT by Robert Kieffer
Modified: 2014-05-15 09:57 PDT (History)
15 users (show)

See Also:


Attachments
work-in-progress (15.42 KB, patch)
2012-03-15 17:27 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (23.16 KB, patch)
2012-09-25 18:10 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (23.37 KB, patch)
2012-09-26 17:22 PDT, Pablo Flouret
ggaren: review+
ggaren: commit‑queue-
Details | Formatted Diff | Diff
Patch for landing (23.41 KB, patch)
2012-11-14 17:42 PST, Pablo Flouret
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Rebased patch for landing (23.48 KB, patch)
2012-11-16 12:38 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Kieffer 2011-10-20 19:26:21 PDT
WebKit and Mozilla browsers redact the information passed to window.onerror for exceptions that occur in scripts that originate from external domains.

Unfortunately this means that for large institutions (like us here at Facebook) that use CDNs to host static script resources, we are unable to collect useful information about errors that occur in production.  And public adoption of Mozilla and Webkit-based browsers has reached a point where the majority of script errors are completely unactionable for us.

I'd like to propose a "X-Script-Origin" header that can be sent back with a script resource that would specify the domain(s) for which error reporting should be allowed.  

I've provided a few more details in this Mozilla report: https://bugzilla.mozilla.org/show_bug.cgi?id=696301

Please let me know the best way to proceed with this so as to enable smooth (and hopefully consistent) progress between these two platforms.
Comment 1 Adam Barth 2011-10-21 12:45: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.
Comment 2 Robert Kieffer 2011-10-21 14:17:44 PDT
Adam, good idea!  https://developer.mozilla.org/en/CORS_Enabled_Image, but for script tags looks like just the ticket. :)
Comment 3 Adam Barth 2011-10-22 20:25:45 PDT
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
Comment 4 can3p 2012-03-11 22:52:00 PDT
The issue was solved in firefox, is there any chance to see it google chrome soon?
Comment 5 Adam Barth 2012-03-11 22:54:30 PDT
> 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.
Comment 6 Pablo Flouret 2012-03-15 17:27:01 PDT
Created attachment 132158 [details]
work-in-progress
Comment 7 Pablo Flouret 2012-03-15 17:27:20 PDT
(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 8 Adam Barth 2012-03-15 18:02:43 PDT
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.
Comment 9 Pablo Flouret 2012-03-16 18:10:02 PDT
(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.
Comment 10 Pablo Flouret 2012-03-20 14:03:44 PDT
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...
Comment 11 James Ide 2012-07-25 16:27:59 PDT
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.
Comment 12 Pablo Flouret 2012-07-25 16:35:30 PDT
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?
Comment 13 Sam Weinig 2012-07-25 17:59:26 PDT
(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.
Comment 14 Pablo Flouret 2012-07-25 18:04:23 PDT
(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.
Comment 15 Sam Weinig 2012-07-26 11:13:30 PDT
(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.
Comment 16 Pablo Flouret 2012-09-25 18:10:54 PDT
Created attachment 165709 [details]
Patch
Comment 17 Pablo Flouret 2012-09-25 18:11:55 PDT
Filed bug 97499 for V8.
Comment 18 WebKit Review Bot 2012-09-25 18:16:15 PDT
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 19 Adam Barth 2012-09-25 18:53:15 PDT
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 20 Build Bot 2012-09-26 01:27:10 PDT
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
Comment 21 Pablo Flouret 2012-09-26 14:57:27 PDT
(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.
Comment 22 Pablo Flouret 2012-09-26 17:22:20 PDT
Created attachment 165904 [details]
Patch
Comment 23 WebKit Review Bot 2012-09-26 17:24:38 PDT
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.
Comment 24 Pablo Flouret 2012-10-11 16:45:22 PDT
ping?
Comment 25 Geoffrey Garen 2012-11-07 12:02:31 PST
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.
Comment 26 Pablo Flouret 2012-11-14 17:42:42 PST
Created attachment 174298 [details]
Patch for landing
Comment 27 WebKit Review Bot 2012-11-16 11:10:22 PST
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
Comment 28 Pablo Flouret 2012-11-16 12:38:21 PST
Created attachment 174745 [details]
Rebased patch for landing
Comment 29 WebKit Review Bot 2012-11-16 15:31:07 PST
Comment on attachment 174745 [details]
Rebased patch for landing

Clearing flags on attachment: 174745

Committed r135009: <http://trac.webkit.org/changeset/135009>
Comment 30 Chris Cinelli 2012-11-19 15:17:15 PST
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.
Comment 31 Ryosuke Niwa 2012-12-07 13:50:29 PST
<rdar://problem/12838505>
Comment 32 Geoffrey Garen 2012-12-07 13:58:37 PST
> 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?
Comment 33 Pablo Flouret 2012-12-07 14:54:03 PST
(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
Comment 34 Chris Cinelli 2012-12-07 15:31:37 PST
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?
Comment 35 Chris Cinelli 2012-12-07 15:48:07 PST
Ok, I added it here: https://bugs.webkit.org/show_bug.cgi?id=104408
Comment 36 Todd H Gardner 2014-02-11 19:16:12 PST
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.
Comment 37 Alexey Proskuryakov 2014-02-12 12:22:25 PST
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.
Comment 38 Björn 2014-05-15 01:48:27 PDT
I just reproduced this with Safari7.
Comment 39 Alexey Proskuryakov 2014-05-15 09:23:19 PDT
Please file a new bug if you can reproduce this with Safari 7.
Comment 40 Björn 2014-05-15 09:57:35 PDT
I have, see here: https://bugs.webkit.org/show_bug.cgi?id=132945