Bug 53664

Summary: XSSFilter shouldn't bother to analyze pages without "injection" characters in the request
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 49845    
Attachments:
Description Flags
Patch dbates: review+

Adam Barth
Reported 2011-02-03 00:20:58 PST
XSSFilter shouldn't bother to analyze pages without "injection" characters in the request
Attachments
Patch (5.24 KB, patch)
2011-02-03 00:26 PST, Adam Barth 		dbates: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2011-02-03 00:26:40 PST
Created attachment 81037 [details] Patch
Daniel Bates
Comment 2 2011-02-03 00:40:59 PST
Comment on attachment 81037 [details] Patch r=me
Eric Seidel (no email)
Comment 3 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?
Eric Seidel (no email)
Comment 4 2011-02-03 00:46:20 PST
Oops. Didn't mean to clear dan's r+.
Adam Barth
Comment 5 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.
Daniel Bates
Comment 6 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.
Adam Barth
Comment 7 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.
Adam Barth
Comment 8 2011-02-03 15:35:13 PST
Committed r77545: <http://trac.webkit.org/changeset/77545>
Note You need to log in before you can comment on or make changes to this bug.