Summary: | Disallow loads using HTTP 0.9 at the ResourceHandle/NetworkDataTask level | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | beidson, berto, bfulgham, cdumez, cgarcia, commit-queue, danw, gustavo, japhet, mcatanzaro, mrobinson, webkit-bug-importer, wilander | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=82714 | ||||||||
Attachments: |
|
Description
Daniel Bates
2016-11-11 16:00:27 PST
Created attachment 294563 [details]
Patch and layout test
I am not a loader/ResourceHandle/NetworkLoad expert. I am open to suggestions on the approach of this patch.
Comment on attachment 294563 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=294563&action=review Why do we want to move this to the NetworkProcess? Do we care about WebSockets here? > Source/WebCore/platform/network/ResourceHandle.cpp:153 > +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) If we do something with ResourceHandle, we should do it with NetworkDataTask, too. I'm not sure we should do this here, though. > LayoutTests/platform/wk2/TestExpectations:551 > +# Internals.registerDefaultPortForProtocol() does not affect NetworkProcess We shouldn't expand this. (In reply to comment #2) > Comment on attachment 294563 [details] > Patch and layout test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294563&action=review > > Why do we want to move this to the NetworkProcess? Do we care about > WebSockets here? I believe we already have been blocking WebSockets that use HTTP 0.9 (at least in soup ports). I have seen several websites broken as a result, but I don't remember any off the top of my head. But I might be remembering wrongly; maybe they used HTTP 1.0. I wish I could remember which websites were affected. (In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 294563 [details] > > Patch and layout test > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=294563&action=review > > > > Why do we want to move this to the NetworkProcess? Do we care about > > WebSockets here? > > I believe we already have been blocking WebSockets that use HTTP 0.9 (at > least in soup ports). I have seen several websites broken as a result, but I > don't remember any off the top of my head. But I might be remembering > wrongly; maybe they used HTTP 1.0. I wish I could remember which websites > were affected. It is not only okay to break legacy websites that use WebSockets over obsolete comm channels - It is encouraged! Comment on attachment 294563 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=294563&action=review >> Source/WebCore/platform/network/ResourceHandle.cpp:153 >> +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) > > If we do something with ResourceHandle, we should do it with NetworkDataTask, too. I'm not sure we should do this here, though. Yes: The NetworkLoad parts of this should be NetworkDataTask. Sorry for telling you the wrong one in person, Dan. But as for Alex's other point; Yes, doing this at the lowest level of the networking abstractions *is* where we should do it. (In reply to comment #2) > Why do we want to move this to the NetworkProcess? We want to do this blocking of HTTP 0.9 at the lowest network abstraction level to to prevent circumvention. > Do we care about WebSockets here? > We do not allow an HTTP 0.9 response for the WebSocket handshake by <http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketHandshake.cpp?rev=208601#L481> and <http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketHandshake.cpp?rev=208601#L389>. > > Source/WebCore/platform/network/ResourceHandle.cpp:153 > > +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) > > If we do something with ResourceHandle, we should do it with > NetworkDataTask, too. [...] > You're right! > > LayoutTests/platform/wk2/TestExpectations:551 > > +# Internals.registerDefaultPortForProtocol() does not affect NetworkProcess > > We shouldn't expand this. I will update the comment to emphasize that we should remove this function. Created attachment 294706 [details]
Patch and layout tests
Updated patch based on feedback from Alex Christensen and Brady Eidson.
Web Sockets are restricted to HTTP 1.1 and above as of https://bugs.webkit.org/show_bug.cgi?id=82714. Comment on attachment 294706 [details]
Patch and layout tests
LGTM, I think Alex should take a look too
Comment on attachment 294706 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=294706&action=review elegant > Source/WebKit2/ChangeLog:36 > +2016-11-11 Daniel Bates <dabates@apple.com> This has two ChangeLog entries. Committed r208732: <http://trac.webkit.org/changeset/208732> |