Bug 27639 - XSS Auditor interferes with -[WebView stringByEvaluatingJavaScriptFromString:]
Summary: XSS Auditor interferes with -[WebView stringByEvaluatingJavaScriptFromString:]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords: XSSAuditor
Depends on:
Blocks:
 
Reported: 2009-07-23 22:36 PDT by Mark Rowe (bdash)
Modified: 2011-05-27 16:49 PDT (History)
2 users (show)

See Also:


Attachments
Patch to check that canonicalString is not empty (1.85 KB, patch)
2009-07-24 11:52 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test (3.48 KB, patch)
2009-07-24 12:52 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2009-07-23 22:36:43 PDT
I tracked down an assertion failure in DumpRenderTree this afternoon that is caused by the XSS auditor causing the following API invocation to fail: [webView stringByEvaluatingJavaScriptFromString:@"0"];  With the XSS auditor enabled, this returns the empty string rather than "0" as expected.  This was being tripped over by DumpRenderTree on machines that had never run the layout tests before due to the default preferences being incorrectly configured (<http://trac.webkit.org/changeset/46304>), but it seems undesirable for the XSS auditor to have any effect on these sorts of APIs.
Comment 1 Adam Barth 2009-07-23 22:41:50 PDT
We should actually do two things here:

1) If the canonical string is empty, we should allow the script.
2) We should white list the API to never be blocked.
Comment 2 Adam Barth 2009-07-24 02:26:44 PDT
Assigned to myself to resolution in the morning.
Comment 3 Daniel Bates 2009-07-24 11:52:00 PDT
Created attachment 33457 [details]
Patch to check that canonicalString is not empty

This patch does not include the white list functionality.
Comment 4 Adam Barth 2009-07-24 12:27:54 PDT
Comment on attachment 33457 [details]
Patch to check that canonicalString is not empty

Thank.  We just need a test.  :)
Comment 5 Adam Barth 2009-07-24 12:27:56 PDT
Comment on attachment 33457 [details]
Patch to check that canonicalString is not empty

Thanks.  We just need a test.  :)
Comment 6 Daniel Bates 2009-07-24 12:52:21 PDT
Created attachment 33464 [details]
Patch with test

Added test.
Comment 7 Mark Rowe (bdash) 2009-07-24 13:02:52 PDT
Comment on attachment 33464 [details]
Patch with test

Do we also plan on adding a more explicit mechanism for preventing legitimate JavaScript-related WebKit API calls from being touched by the XSS auditor?
Comment 8 Adam Barth 2009-07-24 13:02:54 PDT
Comment on attachment 33464 [details]
Patch with test

A patch of beauty.
Comment 9 Adam Barth 2009-07-24 13:03:48 PDT
(In reply to comment #7)
> (From update of attachment 33464 [details])
> Do we also plan on adding a more explicit mechanism for preventing legitimate
> JavaScript-related WebKit API calls from being touched by the XSS auditor?

Yes.  We also want to do this patch and it's easier.
Comment 10 Daniel Bates 2009-07-24 13:09:51 PDT
I have not looked into this, but we may need to be mindful of plugins which use the API to evaluate JavaScript scripts. In which case, we wouldn't want to disable the filter.

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 33464 [details] [details])
> > Do we also plan on adding a more explicit mechanism for preventing legitimate
> > JavaScript-related WebKit API calls from being touched by the XSS auditor?
> 
> Yes.  We also want to do this patch and it's easier.
Comment 11 Adam Barth 2009-07-24 13:13:16 PDT
(In reply to comment #10)
> I have not looked into this, but we may need to be mindful of plugins which use
> the API to evaluate JavaScript scripts. In which case, we wouldn't want to
> disable the filter.

We should look at all the clients of evaluateScript and think about whether they need XSS filtering.
Comment 12 Adam Barth 2009-07-24 13:59:56 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/security/xssAuditor/script-tag-safe-expected.txt
	A	LayoutTests/http/tests/security/xssAuditor/script-tag-safe.html
	M	WebCore/ChangeLog
	M	WebCore/page/XSSAuditor.cpp
Committed r46372
	M	WebCore/ChangeLog
	M	WebCore/page/XSSAuditor.cpp
	A	LayoutTests/http/tests/security/xssAuditor/script-tag-safe-expected.txt
	A	LayoutTests/http/tests/security/xssAuditor/script-tag-safe.html
	M	LayoutTests/ChangeLog
r46372 = 20701f1ff156a5bec23db64671d4cd7556a2cde8 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46372
Comment 13 Adam Barth 2009-07-24 14:41:12 PDT
I think we should keep this bug open to do the whitelisting part.  (Or we could file another bug, I guess.)
Comment 14 Adam Barth 2009-07-24 14:41:31 PDT
Comment on attachment 33464 [details]
Patch with test

Removing from commit queue.
Comment 15 Adam Barth 2011-05-27 16:49:45 PDT
I'd be surprised of this was still an issue given the new XSS auditor design.  If there's something to fix here, please re-open with an example!  Thx.