Bug 69050

Summary:

Fix assertion failure in XSS Auditor

Product:

WebKit

Reporter:

Ilya Sherman <isherman>

Component:

New Bugs

Assignee:

Ilya Sherman <isherman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dbates, isherman, webkit.review.bot

Priority:

Keywords:

XSSAuditor

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Ilya Sherman

Reported 2011-09-28 18:01:45 PDT

Fix assertion failure in XSS Auditor

Attachments
Patch (4.17 KB, patch) 2011-09-28 18:13 PDT, Ilya Sherman	no flags	Details Formatted Diff Diff
Patch (4.61 KB, patch) 2011-09-30 15:51 PDT, Ilya Sherman	no flags	Details Formatted Diff Diff
Patch (4.66 KB, patch) 2011-09-30 16:10 PDT, Ilya Sherman	no flags	Details Formatted Diff Diff
Patch (4.81 KB, patch) 2011-09-30 16:16 PDT, Ilya Sherman	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ilya Sherman

Comment 1 2011-09-28 18:13:31 PDT

Created attachment 109108 [details] Patch

Ilya Sherman

Comment 2 2011-09-28 18:14:36 PDT

Note: the included test doesn't actually work, but I'm not sure why. If I open the .html page in a Debug build of Safari or Chromium, it trips the assertion, as expected. But when run as a test (also in Debug), no dice.

Adam Barth

Comment 3 2011-09-28 19:48:05 PDT

Your change looks fine. A test that triggers the error in the testing harness would be better, of course.

Ilya Sherman

Comment 4 2011-09-28 19:48:51 PDT

(In reply to comment #3) > Your change looks fine. A test that triggers the error in the testing harness would be better, of course. If you could help me arrive at such a test, I would appreciate it :)

Adam Barth

Comment 5 2011-09-28 19:52:58 PDT

I'm sorry that I don't have time to help you find such a test. I'd probably start out by understanding what's different about the test harness and in-browser.

Adam Barth

Comment 6 2011-09-28 19:53:38 PDT

Comment on attachment 109108 [details] Patch Have you tried using an HTTP test? That might behave differently than a test like this one that uses the file system.

Ilya Sherman

Comment 7 2011-09-30 15:51:22 PDT

Created attachment 109361 [details] Patch

Ilya Sherman

Comment 8 2011-09-30 15:52:24 PDT

(In reply to comment #5) > I'm sorry that I don't have time to help you find such a test. I'd probably start out by understanding what's different about the test harness and in-browser. Thanks to a bunch of help from dbates@ (dydx), the test now properly tests the assertion. Hooray!

Daniel Bates

Comment 9 2011-09-30 15:58:30 PDT

Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review Thanks for taking the time to add a test case that works. > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:1 > +<html> Nit: We should add the HTML5 doctype, <!DOCTYPE html>, since this page doesn't need quirks mode. > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:3 > + <script src="../js/resources/js-test-pre.js"></script> As far as I can tell we don't use any functionality that is provided by this external script. Please remove it.

Daniel Bates

Comment 10 2011-09-30 16:09:32 PDT

Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review > LayoutTests/ChangeLog:10 > + * fast/forms/xss-auditor-doesnt-crash.html: Added. This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:24 > + This tests that no assertions fire when submitting a form. This text is a bit vague. Maybe "This tests that no assertions is thrown when POST submitting an empty form."?

Ilya Sherman

Comment 11 2011-09-30 16:10:28 PDT

Created attachment 109365 [details] Patch

Ilya Sherman

Comment 12 2011-09-30 16:16:17 PDT

Created attachment 109367 [details] Patch

Ilya Sherman

Comment 13 2011-09-30 16:17:28 PDT

Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review >> LayoutTests/ChangeLog:10 >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:1 >> +<html> > > Nit: We should add the HTML5 doctype, <!DOCTYPE html>, since this page doesn't need quirks mode. Done. >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:3 >> + <script src="../js/resources/js-test-pre.js"></script> > > As far as I can tell we don't use any functionality that is provided by this external script. Please remove it. Added testPassed() call. >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:24 >> + This tests that no assertions fire when submitting a form. > > This text is a bit vague. Maybe "This tests that no assertions is thrown when POST submitting an empty form."? Done.

Daniel Bates

Comment 14 2011-09-30 16:19:19 PDT

Comment on attachment 109367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109367&action=review > LayoutTests/fast/forms/xss-auditor-doesnt-crash-on-post-submit.html:4 > + <script src="../js/resources/js-test-pre.js"></script> This test is good enough as-is. Nit: You're under utilizing js-test-pre.js and the other JS unit test scripts (i.e. js-test-post.js) which could be taken advantage of to reduce the amount of code in this test.

Daniel Bates

Comment 15 2011-09-30 16:26:35 PDT

(In reply to comment #13) > >> LayoutTests/ChangeLog:10 > >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > > This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit > We should further investigate this.

WebKit Review Bot

Comment 16 2011-09-30 17:24:17 PDT

Comment on attachment 109367 [details] Patch Clearing flags on attachment: 109367 Committed r96440: <http://trac.webkit.org/changeset/96440>

WebKit Review Bot

Comment 17 2011-09-30 17:24:22 PDT

All reviewed patches have been landed. Closing bug.

Ilya Sherman

Comment 18 2011-10-03 12:54:51 PDT

(In reply to comment #15) > (In reply to comment #13) > > >> LayoutTests/ChangeLog:10 > > >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > > > > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > > > > This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit > > > > We should further investigate this. I probably won't have time to investigate further; but if you're interested in doing so, it might help to start from the repro case in https://code.google.com/p/chromium/issues/detail?id=97346.

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