Disallow loads using HTTP 0.9 at the ResourceHandle/NetworkDataTask level

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Currently we disallow HTTP 0.9 loads at the ResourceLoader level. But this allows loads using HTTP 0.9 via other loaders (e.g. FrameLoader). We should disallow HTTP 0.9 loads at ResourceHandle/NetworkLoad level to prevent such circumvention.

[Daniel Bates]

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.

[Alex Christensen]

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.

[Michael Catanzaro]

(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.

[Brady Eidson]

(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!

[Brady Eidson]

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.

[Daniel Bates]

(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.

[Daniel Bates]

Created attachment 294706 [details]
Patch and layout tests

Updated patch based on feedback from Alex Christensen and Brady Eidson.

[John Wilander]

Web Sockets are restricted to HTTP 1.1 and above as of https://bugs.webkit.org/show_bug.cgi?id=82714.

[Brady Eidson]

Comment on attachment 294706 [details]
Patch and layout tests

LGTM, I think Alex should take a look too

[Alex Christensen]

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
> [phone]  Daniel Bates  <[e-mail]>

This has two ChangeLog entries.

[Radar WebKit Bug Importer]

<rdar://problem/29268514>

[Daniel Bates]

Committed r208732: <http://trac.webkit.org/changeset/208732>