Bug 27639 – XSS Auditor interferes with -[WebView stringByEvaluatingJavaScriptFromString:]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Rowe (bdash)]

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.

[Adam Barth]

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.

[Adam Barth]

Assigned to myself to resolution in the morning.

[Daniel Bates]

Created attachment 33457 [details]
Patch to check that canonicalString is not empty

This patch does not include the white list functionality.

[Adam Barth]

Comment on attachment 33457 [details]
Patch to check that canonicalString is not empty

Thank.  We just need a test.  :)

[Adam Barth]

Comment on attachment 33457 [details]
Patch to check that canonicalString is not empty

Thanks.  We just need a test.  :)

[Daniel Bates]

Created attachment 33464 [details]
Patch with test

Added test.

[Mark Rowe (bdash)]

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?

[Adam Barth]

Comment on attachment 33464 [details]
Patch with test

A patch of beauty.

[Adam Barth]

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

[Daniel Bates]

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.

[Adam Barth]

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

[Adam Barth]

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

[Adam Barth]

I think we should keep this bug open to do the whitelisting part.  (Or we could file another bug, I guess.)

[Adam Barth]

Comment on attachment 33464 [details]
Patch with test

Removing from commit queue.

[Adam Barth]

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.