Summary: | WebSockets: first-party for cookies not set in handshake request when using platform APIs | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, webkit-bug-importer, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 199943 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2019-07-26 08:47:26 PDT
Created attachment 374966 [details]
Patch
Created attachment 375310 [details]
Patch
Alex, could you review this one too, please? Comment on attachment 375310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375310&action=review > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:347 > + m_document->page()->cookieJar().setCookies(*m_document, m_httpURLForCookies, serverSetCookie); Cannot it be handled directly in NetworkProcess, we prefer to not send cookies back to web process these days. Comment on attachment 375310 [details]
Patch
I agree with Youenn. It should go straight from WebSocketTask::didConnect to the cookie store.
Sure, that makes sense. The thing is, do we have all the info we need in the network process? first party URL, url, etc.? We do or we should. NetworkResourceLoadParameters has origins. We can similarly pass such origins where we need them here. Created attachment 375338 [details]
Patch
I hope this is the right way.
Comment on attachment 375338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375338&action=review Much better. > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:109 > + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), You should probably check m_session before using it, since it's weak. > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:87 > + // FIXME: support extensions and server cookie. Actually, you should probably make an accessor for m_session on NetworkSocketChannel then move your cookie setting code from NetworkSocketChannel::didConnect to WebSocketTaskSoup.cpp because the cocoa implementation won't be setting cookies like this. Comment on attachment 375338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375338&action=review >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:109 >> + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > You should probably check m_session before using it, since it's weak. I'm doing it, see the condition above: if (!serverSetCookie.isEmpty() && m_session) { >> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:87 >> + // FIXME: support extensions and server cookie. > > Actually, you should probably make an accessor for m_session on NetworkSocketChannel then move your cookie setting code from NetworkSocketChannel::didConnect to WebSocketTaskSoup.cpp because the cocoa implementation won't be setting cookies like this. Ok, isn't it possible to use the same approach to set the cookies? hmm, we would also need the request, frameID and pageID. It's a friend class, so we don't need to add accessors for everything, but it looks weird anyway. What I can do is adding a method to set the cookies that is only called from libsoup (and maybe curl in the future). Created attachment 375402 [details]
Patch
Created attachment 375404 [details]
Patch
Created attachment 375407 [details]
Patch
Comment on attachment 375407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375407&action=review > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:54 > + , m_request(request) Could be an r-value and moved here. > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), The call to setCookiesFromDOM seems strange to me since this is a regular HTTP response processing code path. Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except that we are doing client-side cookie checks to validate the duration. If we compare to the WebProcess handshake implementation, this is status quo but I wonder if there is something better we could do here. Comment on attachment 375407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375407&action=review >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 >> + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > The call to setCookiesFromDOM seems strange to me since this is a regular HTTP response processing code path. > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except that we are doing client-side cookie checks to validate the duration. > If we compare to the WebProcess handshake implementation, this is status quo but I wonder if there is something better we could do here. setCookiesFromDOM is for document.cookie=something. That should not be used here because an HTTP-only cookie sent to a web socket handshake should not be accessible from document.cookie. Could you add a test that verifies that? Also, your code in NetworkSocketChannel::setCookies should be in a soup-specific file because the NSURLSession-based implementation will not have CFNetwork give WebKit cookies to tell CFNetwork to store. It will just do that for us. Comment on attachment 375407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375407&action=review >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:54 >> + , m_request(request) > > Could be an r-value and moved here. I'll check, it's an existing parameter. (In reply to youenn fablet from comment #16) > Comment on attachment 375407 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375407&action=review > > > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:54 > > + , m_request(request) > > Could be an r-value and moved here. > > > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > > + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > The call to setCookiesFromDOM seems strange to me since this is a regular > HTTP response processing code path. > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except > that we are doing client-side cookie checks to validate the duration. > If we compare to the WebProcess handshake implementation, this is status quo > but I wonder if there is something better we could do here. I just followed what current code does, what's the alternative? NetworkStorageSession::setCookies()? (In reply to Alex Christensen from comment #17) > Comment on attachment 375407 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375407&action=review > > >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > >> + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > > > The call to setCookiesFromDOM seems strange to me since this is a regular HTTP response processing code path. > > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except that we are doing client-side cookie checks to validate the duration. > > If we compare to the WebProcess handshake implementation, this is status quo but I wonder if there is something better we could do here. > > setCookiesFromDOM is for document.cookie=something. That should not be used > here because an HTTP-only cookie sent to a web socket handshake should not > be accessible from document.cookie. Could you add a test that verifies that? hmm, isn't this what imported/w3c/web-platform-tests/websockets/cookies/007.html does? It's checking that document.cookie is indeed set after the handshake, no? > Also, your code in NetworkSocketChannel::setCookies should be in a > soup-specific file because the NSURLSession-based implementation will not > have CFNetwork give WebKit cookies to tell CFNetwork to store. It will just > do that for us. It's only called from soup implementation, it could also be used by curl as well eventually, I think. (In reply to Carlos Garcia Campos from comment #19) > (In reply to youenn fablet from comment #16) > > Comment on attachment 375407 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=375407&action=review > > > > > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:54 > > > + , m_request(request) > > > > Could be an r-value and moved here. > > > > > Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > > > + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > > > The call to setCookiesFromDOM seems strange to me since this is a regular > > HTTP response processing code path. > > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except > > that we are doing client-side cookie checks to validate the duration. > > If we compare to the WebProcess handshake implementation, this is status quo > > but I wonder if there is something better we could do here. > > I just followed what current code does, what's the alternative? > NetworkStorageSession::setCookies()? hmm, I wonder if I'll end up re-implementing setCookiesFromDOM to build the Vector<Cookie>, at least in the soup case. Alex? (In reply to Carlos Garcia Campos from comment #20) > (In reply to Alex Christensen from comment #17) > > Comment on attachment 375407 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=375407&action=review > > > > >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > > >> + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > > > > > The call to setCookiesFromDOM seems strange to me since this is a regular HTTP response processing code path. > > > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except that we are doing client-side cookie checks to validate the duration. > > > If we compare to the WebProcess handshake implementation, this is status quo but I wonder if there is something better we could do here. > > > > setCookiesFromDOM is for document.cookie=something. That should not be used > > here because an HTTP-only cookie sent to a web socket handshake should not > > be accessible from document.cookie. Could you add a test that verifies that? > > hmm, isn't this what > imported/w3c/web-platform-tests/websockets/cookies/007.html does? It's > checking that document.cookie is indeed set after the handshake, no? Is the cookie HttpOnly? If so, document.cookie should not allow accessing it. It would be a good test to add if there is no HttpOnly cookie in ws handshake. > > Also, your code in NetworkSocketChannel::setCookies should be in a > > soup-specific file because the NSURLSession-based implementation will not > > have CFNetwork give WebKit cookies to tell CFNetwork to store. It will just > > do that for us. > > It's only called from soup implementation, it could also be used by curl as > well eventually, I think. How are cookies handled in soup and curl? If soup is handling cookies internally for regular http responses, it should also do so for the handshake response. (In reply to youenn fablet from comment #23) > (In reply to Carlos Garcia Campos from comment #20) > > (In reply to Alex Christensen from comment #17) > > > Comment on attachment 375407 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=375407&action=review > > > > > > >> Source/WebKit/NetworkProcess/NetworkSocketChannel.cpp:111 > > > >> + m_session->networkStorageSession()->setCookiesFromDOM(m_request.firstPartyForCookies(), SameSiteInfo::create(m_request), > > > > > > > > The call to setCookiesFromDOM seems strange to me since this is a regular HTTP response processing code path. > > > > Looking at MacOS setCookiesFromDOM implementation, this is mostly ok except that we are doing client-side cookie checks to validate the duration. > > > > If we compare to the WebProcess handshake implementation, this is status quo but I wonder if there is something better we could do here. > > > > > > setCookiesFromDOM is for document.cookie=something. That should not be used > > > here because an HTTP-only cookie sent to a web socket handshake should not > > > be accessible from document.cookie. Could you add a test that verifies that? > > > > hmm, isn't this what > > imported/w3c/web-platform-tests/websockets/cookies/007.html does? It's > > checking that document.cookie is indeed set after the handshake, no? > > Is the cookie HttpOnly? If so, document.cookie should not allow accessing it. > It would be a good test to add if there is no HttpOnly cookie in ws > handshake. I don't know, I'll have to look in detail to understand the test. > > > Also, your code in NetworkSocketChannel::setCookies should be in a > > > soup-specific file because the NSURLSession-based implementation will not > > > have CFNetwork give WebKit cookies to tell CFNetwork to store. It will just > > > do that for us. > > > > It's only called from soup implementation, it could also be used by curl as > > well eventually, I think. > > How are cookies handled in soup and curl? > If soup is handling cookies internally for regular http responses, it should > also do so for the handshake response. hmm, good point, I'll investigate. Thanks for your help! You are indeed right, cookies should be handled by libsoup, it's not doing it because of a bug. I'll leave this bug open for now because I'm not sure if we will need something else from WebKit side. We still need to set FirstPartyForCookies in the handshake request. That fixes http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html and http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html Created attachment 377562 [details]
Patch
Committed r249252: <https://trac.webkit.org/changeset/249252> |