Bug 53664 - XSSFilter shouldn't bother to analyze pages without "injection" characters in the request
Summary: XSSFilter shouldn't bother to analyze pages without "injection" characters in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 49845
  Show dependency tree / graph
 
Reported: 2011-02-03 00:20 PST by Adam Barth
Modified: 2011-02-03 15:35 PST (History)
2 users (show)

See Also:

Attachments
Patch (5.24 KB, patch)
2011-02-03 00:26 PST, Adam Barth 		dbates: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-03 00:20:58 PST 
XSSFilter shouldn't bother to analyze pages without "injection" characters in the request
Comment 1 Adam Barth 2011-02-03 00:26:40 PST 
Created attachment 81037 [details]
Patch
Comment 2 Daniel Bates 2011-02-03 00:40:59 PST 
Comment on attachment 81037 [details]
Patch

r=me
Comment 3 Eric Seidel (no email) 2011-02-03 00:46:10 PST 
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?
Comment 4 Eric Seidel (no email) 2011-02-03 00:46:20 PST 
Oops.  Didn't mean to clear dan's r+.
Comment 5 Adam Barth 2011-02-03 00:47:55 PST 
(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.
Comment 6 Daniel Bates 2011-02-03 14:24:29 PST 
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.
Comment 7 Adam Barth 2011-02-03 15:12:48 PST 
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.
Comment 8 Adam Barth 2011-02-03 15:35:13 PST 
Committed r77545: <http://trac.webkit.org/changeset/77545>