Summary: | AssociatedURLLoader should check HTTP method / headers for untrusted requests. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bill Budge <bbudge> | ||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, fishd, levin, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Bill Budge
2011-09-06 10:51:51 PDT
Created attachment 106444 [details]
Proposed Patch
Comment on attachment 106444 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106444&action=review Looks good to me. Since there is a chromium/public change, I'll leave the final r+ for Darin. > Source/WebKit/chromium/public/WebURLLoaderOptions.h:49 > + , crossOriginRequestPolicy(CrossOriginRequestPolicyDeny) { } Ideally put the {} on separate lines at this point. Created attachment 106607 [details]
Proposed Patch
*** Bug 66963 has been marked as a duplicate of this bug. *** Comment on attachment 106607 [details]
Proposed Patch
Adding some tests for invalid header values.
Created attachment 106631 [details]
Proposed Patch
Improved the tests to check for invalid header values. Improved helper method names.
Comment on attachment 106631 [details] Proposed Patch Attachment 106631 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9616952 Comment on attachment 106631 [details]
Proposed Patch
I believe this build failure is spurious.
Comment on attachment 106631 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106631&action=review > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:56 > +namespace { nit: put the anonymous namespace inside the WebKit namespace. then we shoudn't need the 'using namespace WebKit' up above. > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:289 > + m_clientAdapter->setDelayedError(ResourceError()); what are your plans for this? Once this lands, I can redo PPAPI and WebKit glue code that does all of these checks. As part of that CL I will have to modify URLRequest and URLLoader tests since these errors are reported asynchronously now. I also have FIXMEs to improve error reporting in the glue code. I will need to define a new error domain in WebKit so AssociatedURLLoader can report syntax vs. security errors in a way that WebKit glue understands. Currently it doesn't know about WebKit error domains, only our net:: one, and reports the former as PP_ERROR. It might make sense to modify this patch to generate ResourceErrors with enough information to make this possible. Quick question about the red bot. This is the first time I've seen a failure that isn't caused by the patch. Is there a way to re-run the bot? (In reply to comment #11) > Quick question about the red bot. This is the first time I've seen a failure that isn't caused by the patch. Is there a way to re-run the bot? Reupload the patch is the only way that I know of. (New attachment). Created attachment 107222 [details]
Proposed Patch
Moved header visitor into WebKit namespace. Added information on plans for fixing error reporting from this layer.
Comment on attachment 107222 [details]
Proposed Patch
R=me, sorry for the delayed response!
Comment on attachment 107222 [details] Proposed Patch Clearing flags on attachment: 107222 Committed r95490: <http://trac.webkit.org/changeset/95490> All reviewed patches have been landed. Closing bug. |