Highlight relfected XSS vectors in view-source page.

Make HTMLViewSourceParser also run the XSSAuditor when it is creating a view source
page, and mark the tokens that it thinks are reflected XSS vectors using a
red highlight.

This happens to work at the moment in both "rewrite" and "block" xss protection
modes, because we don't use the modified token when constructing the view
source page.  Ideally, the logic about what mode to use should move out of
XSSAuditor, so that view-source can pass in a value suitable for this specific
case.

Tangentially related to the "XSS Auditor is silent" bug.
BUG=93976
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175320

[Tom Sepez, 2014-05-28 20:50:52]

Adam, Mike, let me know what you think.  Thanks.

[Mike West, 2014-05-29 09:29:48]

Adam is OOO for a while, moving him to CC; Eric might be a good replacement
reviewer for the parser, adding him (Hi Eric!).

I like this idea! Since the performance impact of running the XSSAuditor over
the code is fairly well contained to `view-source://`, I think this is a pretty
reasonable approach. A nit and a question, inline.

LGTM, but please do add a test verifying the behavior, and please also wait for
someone more familiar with the parser to take a look before landing it.

https://codereview.chromium.org/301813002/diff/1/Source/core/css/view-source.css
File Source/core/css/view-source.css (right):

https://codereview.chromium.org/301813002/diff/1/Source/core/css/view-source....
Source/core/css/view-source.css:85: .webkit-highlight {
Hrm. At some point (not in this CL), we need to drop the 'webkit-' prefixes
here. They're just class names, after all.

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
File Source/core/html/HTMLViewSourceDocument.h (right):

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
Source/core/html/HTMLViewSourceDocument.h:43: void addSource(const String&,
HTMLToken&, bool dangerous);
Nit: Would you mind changing |dangerous| here and |highlight| below into an enum
so that the purpose is clear at the callsite? Perhaps something like
TOKEN_CONSIDERED_XSS and TOKEN_CONSIDERED_SAFE?

[eseidel, 2014-05-29 14:28:11]

My impression is that view-source is pretty slow anyway due to counters... but
lgtm.

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
File Source/core/html/HTMLViewSourceDocument.h (right):

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
Source/core/html/HTMLViewSourceDocument.h:54: void processTagToken(const String&
source, HTMLToken&, bool highlight);
All of these bools are hard to read at callsites:

void processTagToken(source, token, true);  // What does true mean??

Hence, as Mike recommends above, we try to use enums instead.  Even if they're
only used in one place once.

enum ViewSourceHighlight {
   HightlightToken,
   DoNotHighlightToken,
}

or whatever the appropriate choices are.  You can define it inside the class or
not, up to you.

[Tom Sepez, 2014-05-29 17:17:24]

Two things to to note:
1. Testing is a bear because there doesn't appear to be an easy way to get a
view-source page apart from magic logic in content_shell (some guy removed
view-source iframes because they were dangerous and not very useful (yes, that
would be me)).
2. Do I still have the post body when I do a view-source on a page that was the
result of a POST? May need to test for that as well.
I will see what I can come up with.  Thanks.

[jww, 2014-05-29 18:35:01]

Any chance we could add a hover note to the highlighted elements to explain why
they're red? I can imagine that developers might be confused by all this red
suddenly appearing in the source for unknown reasons. Otherwise, lgtm.

https://codereview.chromium.org/301813002/diff/1/Source/core/css/view-source.css
File Source/core/css/view-source.css (right):

https://codereview.chromium.org/301813002/diff/1/Source/core/css/view-source....
Source/core/css/view-source.css:85: .webkit-highlight {
On 2014/05/29 09:29:48, Mike West wrote:
> Hrm. At some point (not in this CL), we need to drop the 'webkit-' prefixes
> here. They're just class names, after all.

I just filed https://crbug.com/378867 for this.

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
File Source/core/html/HTMLViewSourceDocument.h (right):

https://codereview.chromium.org/301813002/diff/1/Source/core/html/HTMLViewSou...
Source/core/html/HTMLViewSourceDocument.h:54: void processTagToken(const String&
source, HTMLToken&, bool highlight);
On 2014/05/29 14:28:11, eseidel wrote:
> All of these bools are hard to read at callsites:
> 
> void processTagToken(source, token, true);  // What does true mean??
> 
> Hence, as Mike recommends above, we try to use enums instead.  Even if they're
> only used in one place once.
> 
> enum ViewSourceHighlight {
>    HightlightToken,
>    DoNotHighlightToken,
> }
> 
> or whatever the appropriate choices are.  You can define it inside the class
or
> not, up to you.

Just as a further thought that I could see future uses for this where we might
want to highlight different colors for different purposes, and such an enum
would be more flexible for that.

[Tom Sepez, 2014-05-29 20:27:56]

On 2014/05/29 18:35:01, jww wrote:
> Any chance we could add a hover note to the highlighted elements to explain
why
> they're red? I can imagine that developers might be confused by all this red
> suddenly appearing in the source for unknown reasons. Otherwise, lgtm.
> 
Yeah, I was thinking the same thing once I got the basic framework to work.

[jasvir1, 2014-05-29 20:40:10]

+1 on the change especially with tooltip highlighting to explain why a filespan
was highlighted.

Would the expected way for a user to get to this page be for them hit "View
Source" on a page where the page did not execute correctly/there was an error in
the console?  What happens in the bug 93976 case where if you visit a
view-source:http://someurl where http://someurl happens to have an xss that
causes the auditor to redirect to a blank sandboxed page?  Does the view-source:
still do the highlighting?

[Tom Sepez, 2014-05-29 20:57:11]

> What happens in the bug 93976 case where if you visit a
> view-source:http://someurl where http://someurl happens to have an xss that
> causes the auditor to redirect to a blank sandboxed page?  Does the
view-source:
> still do the highlighting?
Not quite.  But I'm working on it.

[Tom Sepez, 2014-05-29 20:59:19]

Testing blocked on https://codereview.chromium.org/302043002/

[apavlov, 2014-05-30 08:43:38]

Folks, should this perhaps be a part of the DevTools, too? That could
drastically increase the discoverability (and the usability, since a richer UI
can be made use of) of the feature, unless I'm missing something (not really
familiar with the XSSAuditor)...

[Tom Sepez, 2014-05-30 17:49:03]

@apavlov - such integration sounds good, but isn't straightforward since
XSSAuditor removes the offending source.  There may be nothing to mark when
using the elements tab, for example.

[apavlov, 2014-05-31 18:57:43]

On 2014/05/30 17:49:03, Tom Sepez wrote:
> @apavlov - such integration sounds good, but isn't straightforward since
> XSSAuditor removes the offending source.  There may be nothing to mark when
> using the elements tab, for example.

Aha, that makes it a bit more clear. Anyway, we could link the console message
to the related HTML range and mark it accordingly in the Sources panel. Just
thinking out loud :)

[Tom Sepez, 2014-06-02 19:39:48]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-06-02 19:40:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/301813002/100001

[commit-bot: I haz the power, 2014-06-02 21:43:35]

Change committed as 175320