Bug 26784 - Enable XSSAuditor by default
Summary: Enable XSSAuditor by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 26807
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-28 12:35 PDT by Adam Barth
Modified: 2009-07-10 18:22 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.84 KB, patch)
2009-06-28 12:35 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-06-28 12:35:02 PDT
We should try enabling the XSSAuditor by default in the nightly to get a sense for the false positive rate.  Sam said we should do this once we have decent test coverage, and we now have 29 tests.

Please CC me and Dan on any regressions / false positives we find.  If we get a bunch of them, we can turn off the auditor again while we think about how to reduce them.

We still have one known false negative (HTML entities), but we can work on fixing that in parallel.  Also, we should support the "turn off XSS filtering" header that IE8 supports, but I'll file a separate bug about that.
Comment 1 Adam Barth 2009-06-28 12:35:51 PDT
Created attachment 31993 [details]
patch
Comment 2 Daniel Bates 2009-06-28 19:30:46 PDT
Another known false negative is HTTP header injection.

(In reply to comment #0)
> We should try enabling the XSSAuditor by default in the nightly to get a sense
> for the false positive rate.  Sam said we should do this once we have decent
> test coverage, and we now have 29 tests.
> 
> Please CC me and Dan on any regressions / false positives we find.  If we get a
> bunch of them, we can turn off the auditor again while we think about how to
> reduce them.
> 
> We still have one known false negative (HTML entities), but we can work on
> fixing that in parallel.  Also, we should support the "turn off XSS filtering"
> header that IE8 supports, but I'll file a separate bug about that.
> 

Comment 3 Adam Barth 2009-06-28 23:05:08 PDT
(In reply to comment #2)
> Another known false negative is HTTP header injection.

True.  I suppose we could try to stop that too.  I was thinking of that as more of a non-goal for the first version.
Comment 4 Eric Seidel (no email) 2009-06-29 00:04:25 PDT
I would think it would be annoying that this would break plexode.com and hixie's live dom viewer in the nightlies.  Not a deal breaker.  But definitely important that we give sites a way to work around this.
Comment 5 Eric Seidel (no email) 2009-06-29 00:05:57 PDT
FYI, neither plexode.com or Hixie's dom viewer send the IE8 header at this time:

% curl -I http://www.plexode.com/cgi-bin/main.py
HTTP/1.1 200 OK
Date: Mon, 29 Jun 2009 07:03:40 GMT
Server: Apache/2.2.8 (Ubuntu) DAV/2 SVN/1.4.6 mod_fastcgi/2.4.6 PHP/5.2.4-2ubuntu5.6 with Suhosin-Patch mod_ssl/2.2.8 OpenSSL/0.9.8g
Content-Type: text/html

% curl -I http://software.hixie.ch/utilities/js/live-dom-viewer/
HTTP/1.1 200 OK
Date: Mon, 29 Jun 2009 07:04:41 GMT
Server: Apache/2.0.63 (Unix) PHP/5.2.6 mod_ssl/2.0.63 OpenSSL/0.9.7e mod_fastcgi/2.4.2 DAV/2 SVN/1.4.2
Accept-Ranges: bytes
Vary: Accept-Encoding
X-Pingback: http://tracking.damowmow.com/
Content-Language: en-GB-x-Hixie
Content-Length: 13791
Content-Type: text/html; charset=utf-8
Comment 6 Eric Seidel (no email) 2009-06-29 00:08:49 PDT
I emailed Aaron Whyte (plexode) and Hixie to notify them of this pending change.  BTW, I'm totally in favor of this change.  Although I would prefer we added HTTP Header opt-in support before.... or at least really really soon after making this change.
Comment 7 Ian 'Hixie' Hickson 2009-06-29 01:25:17 PDT
I'm not adding anything that's explicitly asking the browsers to follow the specs.
Comment 8 Adam Barth 2009-06-29 09:16:17 PDT
Eric, do you have examples of where plexode.com and hixie's live dom viewer do the wrong thing with the auditor turned on?  I played with them for a few minutes and they seemed to work fine.

(You can test the auditor by enabling the pref in the above patch or by grabbing a trunk build of Chromium and passing the --enable-xss-auditor command line flag.)
Comment 9 Adam Barth 2009-06-29 11:23:58 PDT
We need to fix Bug 26807 first.
Comment 10 Eric Seidel (no email) 2009-06-30 02:58:13 PDT
Comment on attachment 31993 [details]
patch

No need for this to be in the review queue while blocked by a crasher.  Removing r=? for now.
Comment 11 Adam Barth 2009-07-06 01:33:27 PDT
Comment on attachment 31993 [details]
patch

Marking for review again now that this is actionable again.
Comment 12 Sam Weinig 2009-07-08 21:42:42 PDT
Comment on attachment 31993 [details]
patch

By the power of Grayskull!  r=me
Comment 13 Adam Barth 2009-07-08 21:58:26 PDT
Comment on attachment 31993 [details]
patch

Clearing the review flag.  We're going to hold off landing this for a few days.
Comment 14 Adam Barth 2009-07-10 18:22:42 PDT
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebPreferences.mm
Sending        WebKit/win/ChangeLog
Sending        WebKit/win/WebPreferences.cpp
Transmitting file data ....
Committed revision 45740.