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

• Component: Platform
• Status: RESOLVED DUPLICATE

[Bill Budge]

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.

[Bill Budge]

Created attachment 105224 [details]
Proposed Patch

[David Levin]

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 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]

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