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
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar, WebExposed
: 81438
:
  Show dependency treegraph
 
Reported: 2011-10-20 19:26 PST by
Modified: 2014-02-12 12:22 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-20 19:26:21 PST
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 From 2011-10-21 12:45:21 PST -------
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 From 2011-10-21 14:17:44 PST -------
Adam, good idea!  https://developer.mozilla.org/en/CORS_Enabled_Image, but for script tags looks like just the ticket. :)
------- Comment #3 From 2011-10-22 20:25:45 PST -------
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 From 2012-03-11 22:52:00 PST -------
The issue was solved in firefox, is there any chance to see it google chrome soon?
------- Comment #5 From 2012-03-11 22:54:30 PST -------
> 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 From 2012-03-15 17:27:01 PST -------
Created an attachment (id=132158) [details]
work-in-progress
------- Comment #7 From 2012-03-15 17:27:20 PST -------
(In reply to comment #6)
> Created an attachment (id=132158) [details] [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 From 2012-03-15 18:02:43 PST -------
(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.
------- Comment #9 From 2012-03-16 18:10:02 PST -------
(In reply to comment #8)
> (From update of attachment 132158 [details] [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 From 2012-03-20 14:03:44 PST -------
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 From 2012-07-25 16:27:59 PST -------
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 From 2012-07-25 16:35:30 PST -------
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 From 2012-07-25 17:59:26 PST -------
(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 From 2012-07-25 18:04:23 PST -------
(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 From 2012-07-26 11:13:30 PST -------
(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 From 2012-09-25 18:10:54 PST -------
Created an attachment (id=165709) [details]
Patch
------- Comment #17 From 2012-09-25 18:11:55 PST -------
Filed bug 97499 for V8.
------- Comment #18 From 2012-09-25 18:16:15 PST -------
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 From 2012-09-25 18:53:15 PST -------
(From update of attachment 165709 [details])
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 From 2012-09-26 01:27:10 PST -------
(From update of attachment 165709 [details])
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 From 2012-09-26 14:57:27 PST -------
(In reply to comment #19)
> (From update of attachment 165709 [details] [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 From 2012-09-26 17:22:20 PST -------
Created an attachment (id=165904) [details]
Patch
------- Comment #23 From 2012-09-26 17:24:38 PST -------
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 From 2012-10-11 16:45:22 PST -------
ping?
------- Comment #25 From 2012-11-07 12:02:31 PST -------
(From update of attachment 165904 [details])
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 From 2012-11-14 17:42:42 PST -------
Created an attachment (id=174298) [details]
Patch for landing
------- Comment #27 From 2012-11-16 11:10:22 PST -------
(From update of attachment 174298 [details])
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 From 2012-11-16 12:38:21 PST -------
Created an attachment (id=174745) [details]
Rebased patch for landing
------- Comment #29 From 2012-11-16 15:31:07 PST -------
(From update of attachment 174745 [details])
Clearing flags on attachment: 174745

Committed r135009: <http://trac.webkit.org/changeset/135009>
------- Comment #30 From 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 From 2012-12-07 13:50:29 PST -------
<rdar://problem/12838505>
------- Comment #32 From 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 From 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 From 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 From 2012-12-07 15:48:07 PST -------
Ok, I added it here: https://bugs.webkit.org/show_bug.cgi?id=104408
------- Comment #36 From 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 From 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.