[webkit-reviews] review denied: [Bug 66963] [Chromium] Move untrusted HTTP method/header checking to AssociatedURLLoader. : [Attachment 105224] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Bill Budge <bbudge at gmail.com>'s
request for review:
Bug 66963: [Chromium] Move untrusted HTTP method/header checking to
AssociatedURLLoader.
https://bugs.webkit.org/show_bug.cgi?id=66963

Attachment 105224: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=105224&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.