Bug 144174 – [WK2] Cookies set during media loading are not added to cookie store

• Component: New Bugs
• Status: NEW

[Jer Noble]

[WK2] Cookies set during media loading are not added to cookie store

[Jer Noble]

Created attachment 251597 [details]
Patch

[Jer Noble]

Created attachment 251598 [details]
Patch

[Jer Noble]

Created attachment 251600 [details]
Patch

[Jer Noble]

Created attachment 251636 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 251636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251636&action=review

> Source/WebKit2/ChangeLog:12
> +        To do so, WebKit will add a new shim onto the CFConnectionCreateWithProperties() function,

CF_URL_Connection.

Just for the record, this is quite fragile, because WebKit also makes requests from WebContent (ping requests, application cache). I don't know how to improve this, but we should at least have a comment in the code, to easier find the problem when it inevitably occurs.

> Source/WebKit2/Shared/mac/CookieStorageShim.mm:52
>  SOFT_LINK(CFNetwork, CFURLRequestGetURL, CFURLRef, (CFURLRequestRef request), (request))
>  
> +extern "C" CFURLConnectionRef CFURLConnectionCreateWithProperties(CFAllocatorRef alloc, CFURLRequestRef request, CFURLConnectionClient* client, CFDictionaryRef properties);

I don't know why the above is SOF_LINK'ed, but if there is a reason, it probably applies to this function too.

> Source/WebKit2/Shared/mac/CookieStorageShim.mm:79
> +#if USE(CFNETWORK)

As discussed in person, this is false on Mac.

> Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h:34
> +typedef struct CFURLConnectionClient_V1 CFURLConnectionClient;

V1?

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.h:47
> +    static CFIndex maximumSupportedClientVersion();

I'd just use a constant in the class, not a function.

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:50
> +    case 0:

Maybe we don't need the older versions? As you'll have to redefine these SPI structures in CFNetworkSPI.h, that's a lot of structure definitions to have.

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:158
> +        WebProcess::singleton().networkConnection()->connection()->send(Messages::NetworkConnectionToWebProcess::SetCookiesFromDOM(SessionID::defaultSessionID(), firstPartyForCookiesURL, requestURL, cookies), 0);

I don't think that "FromDOM" is appropriate here, it's decisively not from DOM.

Also, defaultSession :(

[Jer Noble]

(In reply to comment #5)
> Comment on attachment 251636 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251636&action=review
> 
> > Source/WebKit2/ChangeLog:12
> > +        To do so, WebKit will add a new shim onto the CFConnectionCreateWithProperties() function,
> 
> CF_URL_Connection.

Whoops, will fix.

> Just for the record, this is quite fragile, because WebKit also makes
> requests from WebContent (ping requests, application cache). I don't know
> how to improve this, but we should at least have a comment in the code, to
> easier find the problem when it inevitably occurs.

Okay, I'll look into this.

> > Source/WebKit2/Shared/mac/CookieStorageShim.mm:52
> >  SOFT_LINK(CFNetwork, CFURLRequestGetURL, CFURLRef, (CFURLRequestRef request), (request))
> >  
> > +extern "C" CFURLConnectionRef CFURLConnectionCreateWithProperties(CFAllocatorRef alloc, CFURLRequestRef request, CFURLConnectionClient* client, CFDictionaryRef properties);
> 
> I don't know why the above is SOF_LINK'ed, but if there is a reason, it
> probably applies to this function too.

We can't soft-link and dynamically interpose at the same time, which is why the first is SOFT_LINK'd and the second is not.

> > Source/WebKit2/Shared/mac/CookieStorageShim.mm:79
> > +#if USE(CFNETWORK)
> 
> As discussed in person, this is false on Mac.

Okay, I'll work around this.

> > Source/WebKit2/Shared/mac/CookieStorageShimLibrary.h:34
> > +typedef struct CFURLConnectionClient_V1 CFURLConnectionClient;
> 
> V1?

Yes; this is verbatim.

> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.h:47
> > +    static CFIndex maximumSupportedClientVersion();
> 
> I'd just use a constant in the class, not a function.

Ok.

> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:50
> > +    case 0:
> 
> Maybe we don't need the older versions? As you'll have to redefine these SPI
> structures in CFNetworkSPI.h, that's a lot of structure definitions to have.

That's very true. I'll look into using only the V6 version.

> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:158
> > +        WebProcess::singleton().networkConnection()->connection()->send(Messages::NetworkConnectionToWebProcess::SetCookiesFromDOM(SessionID::defaultSessionID(), firstPartyForCookiesURL, requestURL, cookies), 0);
> 
> I don't think that "FromDOM" is appropriate here, it's decisively not from
> DOM.

I looked into this, and there is no difference in /setting/ cookies from the DOM vs. from HTTP; there is when retrieving cookies. I.e., the DOM can set cookies that it cannot read.

We could define a new message, SetCookiesFromHTTP, but the implementation would be identical to the DOM version.

> Also, defaultSession :(

:(

[Alexey Proskuryakov]

> We can't soft-link and dynamically interpose at the same time, which is why the first is SOFT_LINK'd and the second is not.

Why do we need to SOFT_LINK the CFURLRequestGetURL function?

> I looked into this, and there is no difference in /setting/ cookies from the DOM vs. from HTTP

That's a bug, httponly cookies should not be overwritable from DOM.

[Alexey Proskuryakov]

(see rdar://problem/12427027)

[David Kilzer (:ddkilzer)]

<rdar://problem/20525970>

[Jer Noble]

Created attachment 256917 [details]
Patch

[Brady Eidson]

Comment on attachment 256917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256917&action=review

This approach is clever, but I'm concerned there's pitfalls.

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:70
> +    if (m_wrappedClient.copyDescription)
> +        m_client.copyDescription = &copyDescription;
> +    if (m_wrappedClient.didReceiveData)
> +        m_client.didReceiveData = &didReceiveData;
> +    if (m_wrappedClient.didStopBuffering)
> +        m_client.didStopBuffering = &didStopBuffering;

When setting up the proxy client, you only include functions that actually exist in the wrapped client. This is good!  But then...

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:185
> +void WebCFURLConnectionClientWrapper::didReceiveData(CFURLConnectionRef conn, CFDataRef data, CFIndex originalLength, const void *info)
> +{
> +    const WebCFURLConnectionClientWrapper* thisInfo = static_cast<const WebCFURLConnectionClientWrapper*>(info);
> +    if (thisInfo->m_wrappedClient.didReceiveData)
> +        thisInfo->m_wrappedClient.didReceiveData(conn, data, originalLength, thisInfo->m_wrappedClient.clientInfo);
> +}

In each of the trivial callbacks, you verify that the wrapped client implements the method.

It implemented the method at construct time, which is all you should worry about. You can assert here if you're super paranoid, but please don't add branches.

[Brady Eidson]

(I have more comments coming, too)

[Brady Eidson]

Comment on attachment 256917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256917&action=review

There's more details I didn't look into closely, will wait until the next rev that takes into account the feedback so far.

> Source/WebCore/platform/spi/cf/CFURLConnectionSPI.h:27
> +#ifndef CFURLConnectionSPI_h
> +#define CFURLConnectionSPI_h

There's already a CFNetworkSPI.h that does some CFURLConnection stuff.

No real need to add this one as long as we already have the dumping ground.

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:127
> +    RetainPtr<CFMutableURLRequestRef> mutableRequest = adoptCF(CFURLRequestCreateMutableCopy(kCFAllocatorDefault, request));

You always create the mutable request...

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:160
> +    if (thisInfo->m_wrappedClient.willSendRequest)
> +        return thisInfo->m_wrappedClient.willSendRequest(conn, mutableRequest.get(), response, thisInfo->m_wrappedClient.clientInfo);

...and always pass that one into the wrapped client, or...

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:161
> +    return mutableRequest.leakRef();

... then always return it.

We've seen more than once that:
1 - Identity of the request passed in is important
2 - Churning through copies of CFURLRequests when not necessary is a perf hit.

Please take care to use the original request when there was no need to create the mutable one.

> Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:173
> +        WebProcess::singleton().networkConnection()->connection()->send(Messages::NetworkConnectionToWebProcess::SetCookiesFromDOM(thisInfo->m_sessionID, firstPartyForCookiesURL, requestURL, cookies), 0);

As of VERY recently, setCookiesFromDOM no longer behaves like receiving a set-cookie header from a resource response.

If we go through with this you'll need to add a new message for "setting cookies from response set-cookie header".

[Brady Eidson]

(In reply to comment #6)
> I looked into this, and there is no difference in /setting/ cookies from the
> DOM vs. from HTTP; there is when retrieving cookies. I.e., the DOM can set
> cookies that it cannot read.
> 
> We could define a new message, SetCookiesFromHTTP, but the implementation
> would be identical to the DOM version.

As I mentioned in my review feedback, this is not true, so you are once again established as a known liar. :)

[Jer Noble]

(In reply to comment #11)
> Comment on attachment 256917 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256917&action=review
> 
> This approach is clever, but I'm concerned there's pitfalls.
> 
> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:70
> > +    if (m_wrappedClient.copyDescription)
> > +        m_client.copyDescription = &copyDescription;
> > +    if (m_wrappedClient.didReceiveData)
> > +        m_client.didReceiveData = &didReceiveData;
> > +    if (m_wrappedClient.didStopBuffering)
> > +        m_client.didStopBuffering = &didStopBuffering;
> 
> When setting up the proxy client, you only include functions that actually
> exist in the wrapped client. This is good!  But then...
> 
> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:185
> > +void WebCFURLConnectionClientWrapper::didReceiveData(CFURLConnectionRef conn, CFDataRef data, CFIndex originalLength, const void *info)
> > +{
> > +    const WebCFURLConnectionClientWrapper* thisInfo = static_cast<const WebCFURLConnectionClientWrapper*>(info);
> > +    if (thisInfo->m_wrappedClient.didReceiveData)
> > +        thisInfo->m_wrappedClient.didReceiveData(conn, data, originalLength, thisInfo->m_wrappedClient.clientInfo);
> > +}
> 
> In each of the trivial callbacks, you verify that the wrapped client
> implements the method.
> 
> It implemented the method at construct time, which is all you should worry
> about. You can assert here if you're super paranoid, but please don't add
> branches.

Sure thing.

(In reply to comment #13)
> Comment on attachment 256917 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256917&action=review
> 
> There's more details I didn't look into closely, will wait until the next
> rev that takes into account the feedback so far.
> 
> > Source/WebCore/platform/spi/cf/CFURLConnectionSPI.h:27
> > +#ifndef CFURLConnectionSPI_h
> > +#define CFURLConnectionSPI_h
> 
> There's already a CFNetworkSPI.h that does some CFURLConnection stuff.
> 
> No real need to add this one as long as we already have the dumping ground.

OK.

> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:127
> > +    RetainPtr<CFMutableURLRequestRef> mutableRequest = adoptCF(CFURLRequestCreateMutableCopy(kCFAllocatorDefault, request));
> 
> You always create the mutable request...
> 
> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:160
> > +    if (thisInfo->m_wrappedClient.willSendRequest)
> > +        return thisInfo->m_wrappedClient.willSendRequest(conn, mutableRequest.get(), response, thisInfo->m_wrappedClient.clientInfo);
> 
> ...and always pass that one into the wrapped client, or...
> 
> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:161
> > +    return mutableRequest.leakRef();
> 
> ... then always return it.
> 
> We've seen more than once that:
> 1 - Identity of the request passed in is important
> 2 - Churning through copies of CFURLRequests when not necessary is a perf
> hit.
> 
> Please take care to use the original request when there was no need to
> create the mutable one.

Sure thing.

> > Source/WebKit2/Shared/mac/WebCFURLConnectionClientWrapper.mm:173
> > +        WebProcess::singleton().networkConnection()->connection()->send(Messages::NetworkConnectionToWebProcess::SetCookiesFromDOM(thisInfo->m_sessionID, firstPartyForCookiesURL, requestURL, cookies), 0);
> 
> As of VERY recently, setCookiesFromDOM no longer behaves like receiving a
> set-cookie header from a resource response.
> 
> If we go through with this you'll need to add a new message for "setting
> cookies from response set-cookie header".

Ok.

(In reply to comment #14)
> (In reply to comment #6)
> > I looked into this, and there is no difference in /setting/ cookies from the
> > DOM vs. from HTTP; there is when retrieving cookies. I.e., the DOM can set
> > cookies that it cannot read.
> > 
> > We could define a new message, SetCookiesFromHTTP, but the implementation
> > would be identical to the DOM version.
> 
> As I mentioned in my review feedback, this is not true, so you are once
> again established as a known liar. :)

It was true at the time!  If the passage of time made old liars out of the young, then... :)