Bug 53664 – XSSFilter shouldn't bother to analyze pages without "injection" characters in the request

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

XSSFilter shouldn't bother to analyze pages without "injection" characters in the request

[Adam Barth]

Created attachment 81037 [details]
Patch

[Daniel Bates]

Comment on attachment 81037 [details]
Patch

r=me

[Eric Seidel (no email)]

Comment on attachment 81037 [details]
Patch

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

> Source/WebCore/html/parser/XSSFilter.h:73
> +    bool m_isInitialized;

I'm always suspicious of these types of bools.  should this just be part of the state machine?  Is there a better bool name than "initialzed"?  m_hasParsedURL?

[Eric Seidel (no email)]

Oops.  Didn't mean to clear dan's r+.

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 81037 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81037&action=review
> 
> > Source/WebCore/html/parser/XSSFilter.h:73
> > +    bool m_isInitialized;
> 
> I'm always suspicious of these types of bools.  should this just be part of the state machine?  Is there a better bool name than "initialzed"?  m_hasParsedURL?

We could move it into the state machine.  I originally thought the state machine would have more states, but didn't turn out to need very many.

[Daniel Bates]

Comment on attachment 81037 [details]
Patch

Thank you Eric for looking over this patch. I was also not very satisfied with the m_isInitialized, but its presence isn't terrible. Moreover, I envisioned that we will perform some clean up iterations on this code once all the major pieces have been moved into place.

If you see any correctness issues with this patch then feel free to override my review and help improve the code.

[Adam Barth]

Generally speaking, I'm happy to do things in the most clean way the first time around.  I'll try to fix this issue on landing.

[Adam Barth]

Committed r77545: <http://trac.webkit.org/changeset/77545>