Bug 70574 – [JSC] Don't sanitize window.onerror information on crossorigin-enabled scripts

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Robert Kieffer]

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.

[Adam Barth]

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.

[Robert Kieffer]

Adam, good idea!  https://developer.mozilla.org/en/CORS_Enabled_Image, but for script tags looks like just the ticket. :)

[Adam Barth]

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

[can3p]

The issue was solved in firefox, is there any chance to see it google chrome soon?

[Adam Barth]

> 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.

[Pablo Flouret]

Created attachment 132158 [details]
work-in-progress

[Pablo Flouret]

(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 ;-)

[Adam Barth]

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.

[Pablo Flouret]

(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.

[Pablo Flouret]

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...

[James Ide]

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.

[Pablo Flouret]

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?

[Sam Weinig]

(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.

[Pablo Flouret]

(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.

[Sam Weinig]

(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.

[Pablo Flouret]

Created attachment 165709 [details]
Patch

[Pablo Flouret]

Filed bug 97499 for V8.

[WebKit Review Bot]

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.

[Adam Barth]

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 -> ?

[Build Bot]

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

[Pablo Flouret]

(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.

[Pablo Flouret]

Created attachment 165904 [details]
Patch

[WebKit Review Bot]

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.

[Pablo Flouret]

ping?

[Geoffrey Garen]

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.

[Pablo Flouret]

Created attachment 174298 [details]
Patch for landing

[WebKit Review Bot]

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

[Pablo Flouret]

Created attachment 174745 [details]
Rebased patch for landing

[WebKit Review Bot]

Comment on attachment 174745 [details]
Rebased patch for landing

Clearing flags on attachment: 174745

Committed r135009: <http://trac.webkit.org/changeset/135009>

[Chris Cinelli]

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.

[Ryosuke Niwa]

<rdar://problem/12838505>

[Geoffrey Garen]

> 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?

[Pablo Flouret]

(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

[Chris Cinelli]

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?

[Chris Cinelli]

Ok, I added it here: https://bugs.webkit.org/show_bug.cgi?id=104408

[Todd H Gardner]

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.

[Alexey Proskuryakov]

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.

[Walt]

I just reproduced this with Safari7.

[Alexey Proskuryakov]

Please file a new bug if you can reproduce this with Safari 7.

[Walt]

I have, see here: https://bugs.webkit.org/show_bug.cgi?id=132945