[webkit-reviews] review denied: [Bug 34289] WebSocket ignores HttpOnly cookies, but should use in Handshake. : [Attachment 47825] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 34289: WebSocket ignores HttpOnly cookies, but should use in Handshake.
https://bugs.webkit.org/show_bug.cgi?id=34289

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+# Copyright (C) 2009 Google Inc. All rights reserved.

2010.

+Should say PASS:
+
+WebSocket open
+WebSocket closed
+PASS cookie is "WK-test=1; WK-test-httponly=1"
+PASS successfullyParsed is true

The "should say pass" phrase is slightly misguiding - it's a series of PASS
messages, followed by "TEST COMPLETE".

+	 "-x", "/websocket/tests/cookies",

It's confusing to have .html files that are processed as CGIs, and it's
confusing to have directories that are magically different from others. The
run-webkit-tests script has a built-in list of extensions for tests, which
includes .pl and .php, among others.

+    shouldBe("cookie", '"WK-test=1; WK-test-httponly=1"');

I'm always nervous when cookie names are reused. We already have WK-test used
by xmlhttprequest/cookies.html test, and I don't see how this new test can not
make it fail (other than if it's more than a 1000 tests away, and gets a new
DRT instance).

Please use unique cookie names, ideally ones that clearly correspond to the
test (I should have set a good example of the latter with
xmlhttprequest/cookies.html!)

+static String getCookiesForWebSocket(Document *document, KURL& url)

WebSocket code should not attempt to build Cookie headers itself. For example,
we should use +[NSHTTPCookie requestHeaderFieldsWithCookies:] on Mac OS X, just
like cookies() function in CookieJar does.

This should be trivial to implement by adapting cookie() code, the only minor
challenge is coming up with a nice name for the new CookieJar function. I'm
thinking of cookieRequestHeaderFieldValue().

+SECURITY WARNING: This uses CGIHTTPServer and CGIHTTPServer is not secure.

I had some difficulty parsing this phrase. And a more detailed explanation of
how this is insecure would help - Apache also executes CGIs, but this doesn't
automatically make it insecure. 

+It may execute arbitrary Python code or external programs. It should not be
+used outside a firewall.

I think it's actually more common that insecure servers run outside of firewall
(more precisely, in "DMZ"). That way, one can't use them as trampoline for
bypassing firewall.