Bug 110733 – XSSAuditor should send only one console error when blocking a page.

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

[Mike West]

Currently, we send two console errors when XSSAuditor blocks a page: "Refused to execute a JavaScript script. Source code of script found within request.\n", and "Entire page will be blocked.".

We should only send one message, tuning it properly for the context. It would also be nice to include the URL of the page whose script is being altered, as it's entirely possible that multiple IFrames on a page could be effected.

[Mike West]

Created attachment 190060 [details]
Patch

[Mike West]

Comment on attachment 190060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190060&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:287
> +    m_originalURL = m_documentURL.string().isolatedCopy();

This bit is a drive-by: rather than copying the KURL and relying on an implicit cast to String, it seems more direct (and more performant?) to make an isolatedCopy of the string itself.

> Source/WebCore/html/parser/XSSAuditor.cpp:288
> +    m_originalHTTPBody = httpBodyAsString;

If the size here is a concern, I'd be happy to wrap this in the "are we reporting?" conditional.

[Build Bot]

Comment on attachment 190060 [details]
Patch

Attachment 190060 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16752263

New failing tests:
fast/frames/xss-auditor-handles-file-urls.html

[WebKit Review Bot]

Comment on attachment 190060 [details]
Patch

Attachment 190060 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16745337

New failing tests:
fast/frames/xss-auditor-handles-file-urls.html

[Adam Barth]

Comment on attachment 190060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190060&action=review

>> Source/WebCore/html/parser/XSSAuditor.cpp:288
>> +    m_originalHTTPBody = httpBodyAsString;
> 
> If the size here is a concern, I'd be happy to wrap this in the "are we reporting?" conditional.

Why did you remove the isEmpty branch?

[Build Bot]

Comment on attachment 190060 [details]
Patch

Attachment 190060 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16749419

New failing tests:
fast/frames/xss-auditor-handles-file-urls.html

[Build Bot]

Comment on attachment 190060 [details]
Patch

Attachment 190060 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16746515

New failing tests:
fast/frames/xss-auditor-handles-file-urls.html

[Daniel Bates]

Comment on attachment 190060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190060&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:289
> +    // We may need these for reporting and console logs later on.
> +    m_originalURL = m_documentURL.string().isolatedCopy();
> +    m_originalHTTPBody = httpBodyAsString;
>  }

I prefer that we defer this change to a separate patch as this change isn't related to the bug (as you acknowledge by your use the of phrase "drive-by") and the patch is large without this change.

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:62
> +    m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Potential cross-site scripting attack detected against '" + xssInfo.m_originalURL + "'; the page's source code appears to directly reflect content found in the request. " + (xssInfo.m_didBlockEntirePage ? "The entire page will be blocked." : "The reflected script will not be executed."));

I'm not happy with the proposed change to the console message. In particular, I'm not happy that the proposed console message sounds passive and conveys uncertainty because of its use of the word "potential" and phrase "appears to". Notice the current console message is concise and assertive. One way to phrase the console message so as to include the URL of the effected page when we don't block the entire page is:

"Refused to execute a JavaScript script. Source code of script found within request to '" + xssInfo.m_originalURL + "'."

OR

"Refused to execute a JavaScript script. Source code of script found within request, '" + xssInfo.m_originalURL + "'."

For the case when we block the entire page, we could write:

"Refused to load page. Source code of script found within request to '" + xssInfo.m_originalURL + "'."

OR

"Refused to show page content. Source code of script found within request to " + xssInfo.m_originalURL + "."

(Can we find a better word than "load"? Or a better way to describe this refusal to show the page content/execute script? We may want to consider elaborating on how we decided to perform a full page block (e.g. server requested it via the HTTP request header X-XSS-Protection.)

Additional remarks:

Although the XSS Auditor console message will most likely only be seen by developers (*), I find the current message we use to be understandable to a layman (or a person whom is only familiar with basic web terminology such as "script" and "request") because it clearly states that we refused to execute a script. It may be reasonable to assume that our audience has a richer vocabulary and using more XSS-specific terminology may help developers more quickly diagnose the cause of such a console warning. Clearly we should look to more effectively communicate the refusal to execute script/block the entire page as indicated by the various Stack Overflow questions and blog articles that have been written with regards to this matter. It would be great if we can come up with a concise and assertive message that balances usage of common and XSS-specific terminology so as to be understandable to an audience with backgrounds in various areas of web development. Maybe this is infeasible?

(*) A person must explicitly enable developer tools to view the console.

[Daniel Bates]

Comment on attachment 190060 [details]
Patch

I'm marking this patch r- since I prefer that the changes to XSSAuditor::init() be in a separate bug as they aren't related to the purpose described in the bug description. Additionally, the Mac, Mac WebKit2, and Chromium Linux EWS bots report that test fast/frames/xss-auditor-handles-file-urls.html fails when this patch is applied. We'll need to rebase its expected results.

[Mike West]

Comment on attachment 190060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190060&action=review

>>> Source/WebCore/html/parser/XSSAuditor.cpp:288
>>> +    m_originalHTTPBody = httpBodyAsString;
>> 
>> If the size here is a concern, I'd be happy to wrap this in the "are we reporting?" conditional.
> 
> Why did you remove the isEmpty branch?

If I don't remove the branch, we'll only store the original URL when we're in reporting mode. I'd like to add the URL to the delegate's message regardless of reporting state.

>> Source/WebCore/html/parser/XSSAuditor.cpp:289
>>  }
> 
> I prefer that we defer this change to a separate patch as this change isn't related to the bug (as you acknowledge by your use the of phrase "drive-by") and the patch is large without this change.

I can drop the change from `m_documentURL.copy()` into another patch, no problem.

Removing the isEmpty branch, however, is important: if it isn't removed, then we won't have a document URL to report unless a report URL has been set. I'd like to be able to add the document URL to the console message regardless of reporting state.

>> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:62
>> +    m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Potential cross-site scripting attack detected against '" + xssInfo.m_originalURL + "'; the page's source code appears to directly reflect content found in the request. " + (xssInfo.m_didBlockEntirePage ? "The entire page will be blocked." : "The reflected script will not be executed."));
> 
> I'm not happy with the proposed change to the console message. In particular, I'm not happy that the proposed console message sounds passive and conveys uncertainty because of its use of the word "potential" and phrase "appears to". Notice the current console message is concise and assertive. One way to phrase the console message so as to include the URL of the effected page when we don't block the entire page is:
> 
> "Refused to execute a JavaScript script. Source code of script found within request to '" + xssInfo.m_originalURL + "'."
> 
> OR
> 
> "Refused to execute a JavaScript script. Source code of script found within request, '" + xssInfo.m_originalURL + "'."
> 
> For the case when we block the entire page, we could write:
> 
> "Refused to load page. Source code of script found within request to '" + xssInfo.m_originalURL + "'."
> 
> OR
> 
> "Refused to show page content. Source code of script found within request to " + xssInfo.m_originalURL + "."
> 
> (Can we find a better word than "load"? Or a better way to describe this refusal to show the page content/execute script? We may want to consider elaborating on how we decided to perform a full page block (e.g. server requested it via the HTTP request header X-XSS-Protection.)
> 
> Additional remarks:
> 
> Although the XSS Auditor console message will most likely only be seen by developers (*), I find the current message we use to be understandable to a layman (or a person whom is only familiar with basic web terminology such as "script" and "request") because it clearly states that we refused to execute a script. It may be reasonable to assume that our audience has a richer vocabulary and using more XSS-specific terminology may help developers more quickly diagnose the cause of such a console warning. Clearly we should look to more effectively communicate the refusal to execute script/block the entire page as indicated by the various Stack Overflow questions and blog articles that have been written with regards to this matter. It would be great if we can come up with a concise and assertive message that balances usage of common and XSS-specific terminology so as to be understandable to an audience with backgrounds in various areas of web development. Maybe this is infeasible?
> 
> (*) A person must explicitly enable developer tools to view the console.

I'm happy to change the message, and I agree that putting the action first is probably the right way to go. I'll poke at it a bit today in the hopes of coming up with something you'll be happier with.

[Mike West]

Moved the implicit casting bit to http://wkbug.com/110859

[Mike West]

How about:

"The XSS Auditor has blocked access to '[URL]' because its source reflected code found within the page's URL."

and

"The XSS Auditor has blocked a script in '[URL]' because its source reflected code found within the page's URL."

I'm not sure how to incorporate the detail about how XSSAuditor has been enabled, because we're not currently storing that information. There are three possibilities:

1. The server did nothing, in which case the Auditor is enabled in filtering mode by default.
2. The server sent an 'X-XSS-Protection' header with either '1' or '1; mode=block[; report=...]'.
3. The server sent an 'X-WebKit-CSP' header with a value containing 'reflected-xss [filter|block]'.

We can ignore the third for the moment, since it's locked away behind a flag, and the implementation might change completely if the working group changes its mind. I suppose we could store a bool for the first two.

If we did that, how about adding one of the following to the above messages:

"The auditor was implicitly enabled, as the server did not send an 'X-XSS-Protection' header."

or

"The server sent an 'X-XSS-Protection' header requesting this behavior."

WDYT?

[Mike West]

Created attachment 190265 [details]
Patch

[Adam Barth]

Comment on attachment 190265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190265&action=review

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:62
> +    m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, String::format("The XSS Auditor has blocked %s '%s' because its source reflected code found within the page's URL. %s", (xssInfo.m_didBlockEntirePage ? "access to" : "a script in"), xssInfo.m_originalURL.ascii().data(), (xssInfo.m_didSendHeader ? "The server sent an 'X-XSS-Protection' header requesting this behavior." : "The auditor was implicitly enabled, as the server did not send an 'X-XSS-Protection' header.")));

Can this message arise from a CSP header?  If so, the mention of X-XSS-Protection seems wrong...

[Mike West]

(In reply to comment #14)
> (From update of attachment 190265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190265&action=review
> 
> > Source/WebCore/html/parser/XSSAuditorDelegate.cpp:62
> > +    m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, String::format("The XSS Auditor has blocked %s '%s' because its source reflected code found within the page's URL. %s", (xssInfo.m_didBlockEntirePage ? "access to" : "a script in"), xssInfo.m_originalURL.ascii().data(), (xssInfo.m_didSendHeader ? "The server sent an 'X-XSS-Protection' header requesting this behavior." : "The auditor was implicitly enabled, as the server did not send an 'X-XSS-Protection' header.")));
> 
> Can this message arise from a CSP header?  If so, the mention of X-XSS-Protection seems wrong...

Hrm. Yes. Given that 'reflected-xss' is still a bit up in the air, however, I didn't think that this fudge was that bad. :)

In an ideal world, we'd note exactly what the user did to trigger the behavior. This requires maintaining a few more bits of state, and I'm not sure we'd get to complete accuracy without some more rework. I don't think ContentSecurityPolicy currently exposes whether the user sent a 'Content-Security-Policy' or 'X-WebKit-CSP' header, for example. Likewise, if both headers are sent, then the state gets complex quickly.

I'll put a bit more detail in to get us a bit closer to that state.

For the future, I wonder whether it would be better to expose things like this state somewhere else in the inspector. On the resource panel for the page, perhaps?

[Mike West]

Created attachment 190497 [details]
Patch

[Mike West]

Created attachment 190947 [details]
Rebased.

[Daniel Bates]

Comment on attachment 190947 [details]
Rebased.

View in context: https://bugs.webkit.org/attachment.cgi?id=190947&action=review

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:65
> +    message.append("' because its source reflected code found within the page's URL. ");

This is factually incorrect as we may have found some substring of the source code in the HTTP POST data. Also, is there another way to describe "source reflected code"? One idea is to use phrasing similar to the original console message:

"... because the source code of a script was found within the request."

I need to get some sleep. I'll think of some more ideas on how to best to phrase this message and comment later today (03/03), at latest Monday (03/04).

[Mike West]

(In reply to comment #18)
> This is factually incorrect as we may have found some substring of the source code in the HTTP POST data. Also, is there another way to describe "source reflected code"? One idea is to use phrasing similar to the original console message:

Fair enough. GET reflection is much more common, I suppose, but you're entirely correct.

> "... because the source code of a script was found within the request."

I could certainly live with this, but I'll wait for your feedback on Monday just in case something even clearer occurs to you in the meantime.

There's no rush on this, by the way. It's really just cleanup, so don't feel like you need to spend any piece of your Sunday on this. :)

Thanks!

[Daniel Bates]

Here are some more variations on the error message:

== Full page block ==

Refused access to %URL%. Source code of script found within request.
Refused access to %URL% because the source code of a script was found within the request.
The XSS Auditor refused access to %URL%. Source code of script found within request.
The XSS Auditor refused access to %URL% because the source code of a script was found within the request.
Blocked access to %URL%. Source code of script found within request.
Blocked access to %URL% because the source code of a script was found within the request.
The XSS Auditor blocked access to %URL%. Source code of script found within request.
The XSS Auditor blocked access to %URL% because the source code of a script was found within the request.

== Refused to execute a JavaScript script ==

Refused to execute a JavaScript script in %URL%. Source code of script found within request.
Refused to execute a JavaScript script in %URL% because the source code of the script was found within the request.
The XSS Auditor refused to execute a JavaScript script in %URL%. Source code of script found within request.
The XSS Auditor refused to execute a JavaScript script in %URL% because the source code of the script found within request.

[Mike West]

Ok.

How about:

"The XSS Auditor has refused to execute a script in '[URL]' because its source code was found within the request. The server sent a 'Content-Security-Policy' header requesting this behavior."

and

"The XSS Auditor has blocked access to '[URL]' because the source code of a script was found within the request. The server sent a 'Content-Security-Policy' header requesting this behavior."?

That's up as the current patch.

[Mike West]

Created attachment 191493 [details]
Patch

[Daniel Bates]

Comment on attachment 191493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191493&action=review

Thank you Mike for updating the patch. When the URL is long, which is likely for a GET-based XSS attack, the console message may be long and be difficult to read. See my remarks (below) for more details. We may want to consider moving the URL to the end of the sentence similar the proposed messages strings in comment 8. Regardless, the message text proposed in this patch is reasonable.

> Source/WebCore/html/parser/XSSAuditor.cpp:285
> +        m_didSendValidXSSProtectionHeader = (xssProtectionHeader != ContentSecurityPolicy::ReflectedXSSUnset && xssProtectionHeader != ContentSecurityPolicy::ReflectedXSSInvalid);

Please remove the outer-most parenthesis as I don't feel they improve the readability of this line.

> Source/WebCore/html/parser/XSSAuditor.cpp:299
> +        m_didSendValidCSPHeader = (document->contentSecurityPolicy()->reflectedXSSDisposition() != ContentSecurityPolicy::ReflectedXSSUnset && document->contentSecurityPolicy()->reflectedXSSDisposition() != ContentSecurityPolicy::ReflectedXSSInvalid);

I suggest we store the value of the expression "document->contentSecurityPolicy()->reflectedXSSDisposition()" in a local variable and then reference it twice. This will save dereferencing two pointers and making a function call as well as improve the readability of this line. Also, please remove the outer-most parenthesis as I don't feel they improve the readability of this line.

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:61
> +    message.append("The XSS Auditor has ");

I suggest we remove the word "has".

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:64
> +    message.append(xssInfo.m_originalURL);

The URL will tend to be long.  And a long URL may affect the readability of this sentence since it breaks up the explanation of what was blocked/refused and why it was blocked/refused.

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:66
> +    message.append(xssInfo.m_didBlockEntirePage ? " the source code of a script" : " its source code");

Nit: I suggest we remove the space character from each of the string literals in this line and append a space character to the end of the string literal on the previous line (line 65).

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:74
> +        message.append(" The auditor was implicitly enabled, as the server did not send an 'X-XSS-Protection' or 'Content-Security-Policy' header to control its state.");

I wish there was a better way to state this. Maybe:

The auditor was enabled as the server neither sent an 'X-XSS-Protection' nor 'Content-Security-Policy' header.

[Mike West]

Created attachment 191962 [details]
Patch

[Mike West]

(In reply to comment #23)
> (From update of attachment 191493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191493&action=review
> 
> Thank you Mike for updating the patch. When the URL is long, which is likely for a GET-based XSS attack, the console message may be long and be difficult to read. See my remarks (below) for more details. We may want to consider moving the URL to the end of the sentence similar the proposed messages strings in comment 8. Regardless, the message text proposed in this patch is reasonable.

Thanks, Daniel. I've updated the patch with your latest comments.

As discussed on IRC, the URL is currently ellipsized when rendered in the console. I believe this mitigates the impact of your concerns about its current position, but if that turns out to be incorrect I'll happily come back and move the URL whenever makes the most sense.

I'll throw this patch to the bots to make sure I rebaselined everything correctly, then CQ it if all is well.

Thanks again for your attention to detail.

[WebKit Review Bot]

Comment on attachment 191962 [details]
Patch

Clearing flags on attachment: 191962

Committed r145083: <http://trac.webkit.org/changeset/145083>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Weinstein]

Reverted r145083 for reason:

caused lots crashes in http/tests/security/xssAuditor/* tests

Committed r145115: <http://trac.webkit.org/changeset/145115>

[Rafael Weinstein]

FYI. A selection of crash outputs that I looked at all seemed to hit the assertion:

crash log for DumpRenderTree (pid 2520):
STDOUT: <empty>
STDERR: ASSERTION FAILED: xssInfos[i]->isSafeToSendToAnotherThread()
STDERR: ../../third_party/WebKit/Source/WebCore/html/parser/BackgroundHTMLParser.cpp(65) : void WebCore::checkThatXSSInfosAreSafeToSendToAnotherThread(const WebCore::XSSInfoStream&)
STDERR: 1   0x7f14a69b8f32
STDERR: 2   0x7f14a69ba7eb
STDERR: 3   0x7f14a69ba767
STDERR: 4   0x7f14a69b9baa
STDERR: 5   0x7f14a69d4ea3
STDERR: 6   0x7f14a69d49b6
STDERR: 7   0x7f14a55163f0
STDERR: 8   0x7f14a69e0383
STDERR: 9   0x7f14a69e02ea
STDERR: 10  0x7f14a551254d
STDERR: 11  0x7f14a5512b79
STDERR: 12  0x7f149f0149ca
STDERR: 13  0x7f149ed71cdd clone
STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
STDERR:  [0x7f14aa15779c] base::debug::StackTrace::StackTrace()
STDERR:  [0x7f14aa157046] base::debug::(anonymous namespace)::StackDumpSignalHandler()
STDERR:  [0x7f149f01d8f0] <unknown>
STDERR:  [0x7f14a69b8f3c] WebCore::checkThatXSSInfosAreSafeToSendToAnotherThread()
STDERR:  [0x7f14a69ba7eb] WebCore::BackgroundHTMLParser::sendTokensToMainThread()
STDERR:  [0x7f14a69ba767] WebCore::BackgroundHTMLParser::pumpTokenizer()
STDERR:  [0x7f14a69b9baa] WebCore::BackgroundHTMLParser::append()
STDERR:  [0x7f14a69d4ea3] WTF::FunctionWrapper<>::operator()()
STDERR:  [0x7f14a69d49b6] WTF::BoundFunctionImpl<>::operator()()
STDERR:  [0x7f14a55163f0] WTF::Function<>::operator()()
STDERR:  [0x7f14a69e0383] WebCore::HTMLParserThread::runLoop()
STDERR:  [0x7f14a69e02ea] WebCore::HTMLParserThread::threadStart()
STDERR:  [0x7f14a551254d] WTF::threadEntryPoint()
STDERR:  [0x7f14a5512b79] WTF::wtfThreadEntryPoint()
STDERR:  [0x7f149f0149ca] start_thread
STDERR:  [0x7f149ed71cdd] clone
STDERR:   r8: 00007f1494edd700  r9: 00007f14a7191415 r10: 0000000000000000 r11: 0000000000000000
STDERR:  r12: 0000000000000000 r13: 00001ec99b0f04f8 r14: 0000000000000000 r15: 0000000000000000
STDERR:   di: 0000000000000000  si: 00000000efcdab90  bp: 00007f1494edca80  bx: 00001ec99b0f0628
STDERR:   dx: 00007f149f009e00  ax: 00000000bbadbeef  cx: 00007f149ed63acd  sp: 00007f1494edca60
STDERR:   ip: 00007f14a69b8f3c efl: 0000000000010246 cgf: 0000000000000033 erf: 0000000000000006
STDERR:  trp: 000000000000000e msk: 0000000000000000 cr2: 00000000bbadbeef

[Mike West]

(In reply to comment #29)
> FYI. A selection of crash outputs that I looked at all seemed to hit the assertion:

Ugh. Sorry about that. I shouldn't have only tested in Release. :(

I'll fix this up and reland.

[Mike West]

Comment on attachment 191962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191962&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:349
>              m_originalURL = String();

Since we moved the original URL out of the `if` block on line 324, we need to move it out here as well to ensure it's cleared correctly.

[Mike West]

Created attachment 192211 [details]
Patch

[Mike West]

(In reply to comment #31)
> (From update of attachment 191962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191962&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:349
> >              m_originalURL = String();
> 
> Since we moved the original URL out of the `if` block on line 324, we need to move it out here as well to ensure it's cleared correctly.

That revealed another problem: the current code only carries a URL for the first XSSInfo object created. If multiple scripts are blocked on the same page, they should all contain the relevant URL.

The current patch drops the clearing code completely, instead creating a new copy of the URL for each XSSInfo object on a page. Reports are still only sent once.

[Build Bot]

Comment on attachment 192211 [details]
Patch

Attachment 192211 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17027430

New failing tests:
editing/selection/selection-modify-crash.html

[Mike West]

(In reply to comment #33)
> (In reply to comment #31)
> > (From update of attachment 191962 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191962&action=review
> > 
> > > Source/WebCore/html/parser/XSSAuditor.cpp:349
> > >              m_originalURL = String();
> > 
> > Since we moved the original URL out of the `if` block on line 324, we need to move it out here as well to ensure it's cleared correctly.
> 
> That revealed another problem: the current code only carries a URL for the first XSSInfo object created. If multiple scripts are blocked on the same page, they should all contain the relevant URL.
> 
> The current patch drops the clearing code completely, instead creating a new copy of the URL for each XSSInfo object on a page. Reports are still only sent once.

Adam, Daniel, Tom: The bots seem happy with the patch, and I've verified locally that it doesn't ASSERT in debug mode. Before relanding it, I'd appreciate a quick comment on whether or not you folks will accept making more copies of the URL string. Any objections?

[Adam Barth]

Comment on attachment 192211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192211&action=review

Something is confused about this patch.  We shouldn't need to send the document's URL back to the main thread in the XSSInfo.  Whoever receives the XSSInfo on the main thread will have access to the document and therefore its URL and HTTPBody.

> Source/WebCore/html/parser/XSSAuditor.cpp:321
> +    // If we discover XSS, we'll need this for reporting and console messages later on.
> +    m_originalURL = m_documentURL.string().isolatedCopy();

Why do we need to make another copy of this string?

[Mike West]

(In reply to comment #36)
> (From update of attachment 192211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192211&action=review
> 
> Something is confused about this patch.

I think you're right.

> > Source/WebCore/html/parser/XSSAuditor.cpp:321
> > +    // If we discover XSS, we'll need this for reporting and console messages later on.
> > +    m_originalURL = m_documentURL.string().isolatedCopy();
> 
> Why do we need to make another copy of this string?

It doesn't look like we do. We only use it in XSSAuditor to populate an XSSInfo object. The only consumer of XSSInfo objects that I've found is XSSAuditorDelegate, which is called from HTMLDocumentParser::processParsedChunkFromBackgroundParser after moving back to the main thread. We should indeed be able to read the URL of the document straight off the Document object.

I've thrown out a patch for review at https://bugs.webkit.org/show_bug.cgi?id=111944.

[Mike West]

(In reply to comment #37)
> It doesn't look like we do. We only use it in XSSAuditor to populate an XSSInfo object. The only consumer of XSSInfo objects that I've found is XSSAuditorDelegate, which is called from HTMLDocumentParser::processParsedChunkFromBackgroundParser after moving back to the main thread. We should indeed be able to read the URL of the document straight off the Document object.
> 
> I've thrown out a patch for review at https://bugs.webkit.org/show_bug.cgi?id=111944.

Body too, same reasons. Verified it didn't blow up in a debug build, put a patch up for review at https://bugs.webkit.org/show_bug.cgi?id=111946.

I plan to rebase this patch on top of those two and re-land it, assuming nothing else looks fishy?

[Mike West]

Created attachment 192440 [details]
Patch

[Mike West]

(In reply to comment #39)
> Created an attachment (id=192440) [details]
> Patch

Assuming the EWS bots are happy, I'll reland this via the CQ tonight.

[WebKit Review Bot]

Comment on attachment 192440 [details]
Patch

Attachment 192440 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17169114

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-empty.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-allow-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-block-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-filter-allow.html
http/tests/cache/subresource-failover-to-network.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-filter-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-invalid-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-invalid-allow.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-block-unset.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-unset-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-unset-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-invalid.html
fast/forms/datalist/update-range-with-datalist.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-unset-invalid.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-unset-unset.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-invalid-unset.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-allow-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-invalid-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-filter-unset.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-block-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-block-allow.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-block-invalid.html
fast/hidpi/video-controls-in-hidpi.html
fast/layers/no-clipping-overflow-hidden-added-after-transform.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-filter.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-filter-invalid.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-invalid-invalid.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-filter-block.html
http/tests/security/contentSecurityPolicy/1.1/reflected-xss-and-xss-protection-allow-invalid.html

[Mike West]

Created attachment 192458 [details]
Patch

[Mike West]

(In reply to comment #41)
> (From update of attachment 192440 [details])
> Attachment 192440 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://webkit-commit-queue.appspot.com/results/17169114

*sigh* I forgot that Mac hasn't enable CSP 1.1; I needed to rebaseline in both ports. So. That's fun. :)

[Mike West]

Comment on attachment 192458 [details]
Patch

Alright. Bots are happy enough. Relanding.

[WebKit Review Bot]

Comment on attachment 192458 [details]
Patch

Clearing flags on attachment: 192458

Committed r145503: <http://trac.webkit.org/changeset/145503>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.