Bug 66963

Summary:

[Chromium] Move untrusted HTTP method/header checking to AssociatedURLLoader.

Product:

WebKit

Reporter:

Bill Budge <bbudge>

Component:

Platform

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

darin, fishd, levin

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

66909

Bug Blocks:

Attachments:

Description	Flags
Proposed Patch	levin: review-

Bill Budge

Reported 2011-08-25 11:26:59 PDT

Move method and header checking to AssociatedURLLoader, so we can reuse code in XMLHttpRequest. Add an 'untrustedHttp' option to WebURLLoaderOptions, and if it's set, asynchronously return an error if the HTTP request is unsafe.

Attachments
Proposed Patch (10.60 KB, patch) 2011-08-25 11:30 PDT, Bill Budge	levin: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Bill Budge

Comment 1 2011-08-25 11:30:14 PDT

Created attachment 105224 [details] Proposed Patch

David Levin

Comment 2 2011-08-25 12:15:55 PDT

Comment on attachment 105224 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105224&action=review > Source/WebKit/chromium/public/WebURLLoaderOptions.h:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. In WebKit, we typically don't throw away years like this (but whatever). :) > Source/WebKit/chromium/public/WebURLLoaderOptions.h:45 > + : untrustedHttp(false) Need 4 space indent. > Source/WebKit/chromium/public/WebURLLoaderOptions.h:49 > + , crossOriginRequestPolicy(CrossOriginRequestPolicyDeny) { } Put {} on new lines. > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:73 > + m_isSafe = false; Alternately m_isSafe = m_isSafe && XMLHttpRequest::isSafeRequestHeader(name) && XMLHttpRequest::isValidHeaderValue(value); Also why isn't there a XMLHttpRequest::isValidToken(name) check here? > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:284 > m_clientAdapter->enableErrorNotifications(); Does the didFail go through since the enableErrorNotifications happens here? > Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:214 > + EXPECT_TRUE(m_didFail); Lines 204-214 seems like that repeat lines 184-194. Can we create a common function instead? Even the first three lines are dups and feel like they should also be a common function. > Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:343 > +} No tests for invalid header values or headers names with invalid tokens.

Darin Fisher (:fishd, Google)

Comment 3 2011-08-25 14:57:55 PDT

Comment on attachment 105224 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105224&action=review >> Source/WebKit/chromium/public/WebURLLoaderOptions.h:45 >> + : untrustedHttp(false) > > Need 4 space indent. nit: untrustedHttp -> untrustedHTTP see webkit style guide where it mentions capitalization rules for acronyms. > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:58 > +class SafeHttpHeaderValidator : public WebHTTPHeaderVisitor { nit: SafeHttp -> SafeHTTP > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:282 > + m_clientAdapter->didFail(ResourceError()); maybe we should have a setDelayedError method to make it more explicit what we are doing here?

Bill Budge

Comment 4 2011-09-07 11:56:14 PDT

*** This bug has been marked as a duplicate of bug 67655 ***

Note You need to log in before you can comment on or make changes to this bug.