Cross subdomain cookie sharing issue #63
Conversation
src/Plugin/CookiePlugin.php
Outdated
| @@ -69,7 +69,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |||
| } | |||
|
|
|||
| // Restrict setting cookie from another domain | |||
| if (false === strpos($cookie->getDomain(), $request->getUri()->getHost())) { | |||
| if (false === strpos($request->getUri()->getHost(), $cookie->getDomain())) { | |||
There was a problem hiding this comment.
is this correct. what about symfony.slack.com setting the cookie and then we do a request to hacksymfony.slack.com? its an edge case, but i think we should be correct about this.
There was a problem hiding this comment.
On the other hand, hack.symfony.slack.com should be correct again. Cookies.... 😕
There was a problem hiding this comment.
@dbu, @sagikazarmark:
What if we add a dot . to both cookie domain and request domain before the comparison?
strpos(
'.'.$request->getUri()->getHost(),
'.'.$cookie->getDomain()
)Here's what's happening in this case:
strpos(
'.hacksymfony.slack.com',
'.symfony.slack.com'
); // no match, cookie ignored.
strpos(
'.hack.symfony.slack.com',
'.symfony.slack.com'
); // match, cookie used.
strpos(
'.symfony.slack.com',
'.symfony.slack.com'
); // match, cookie used.I wonder if this workaround is appropriate and not counted as dirty code.
There was a problem hiding this comment.
i think that is the right approach. afaik the server could send the leading . already, we should handle that properly as well, not that we end up with "..symfony.slack.com".
are you sure that cookies should propagate down several levels? like test.com cookie being sent to foo.bar.baz.test.com ?
There was a problem hiding this comment.
afaik the server could send the leading . already, we should handle that properly as well, not that we end up with "..symfony.slack.com".
This is already handled by Http\Message\Cookie::normalizeDomain(), so we shouldn't care about it.
are you sure that cookies should propagate down several levels? like test.com cookie being sent to foo.bar.baz.test.com ?
Yes, that's what is said in RFC 6265. If Domain attribute is not specified, it defaults to the host portion of the current document location (but not including subdomains). If a domain is specified, subdomains are always included.
src/Plugin/CookiePlugin.php
Outdated
| if (false === strpos($cookie->getDomain(), $request->getUri()->getHost())) { | ||
| if (false === strpos( | ||
| '.'.$request->getUri()->getHost(), | ||
| '.'.$cookie->getDomain() |
There was a problem hiding this comment.
Isn't this check too weak (see https://3v4l.org/nXCMW)? You should here test that $request->getUri()->getHost() ends with $cookie->getDomain().
There was a problem hiding this comment.
i think you are right @fbourigault
@andrewkharook can you please adjust this accordingly and add a test that fails with this code but not with the new code? like comparing fake.domain.com.evil.org with domain.com?
src/Plugin/CookiePlugin.php
Outdated
| if (false === strpos($cookie->getDomain(), $request->getUri()->getHost())) { | ||
| if (false === strpos( | ||
| '.'.$request->getUri()->getHost(), | ||
| '.'.$cookie->getDomain() |
There was a problem hiding this comment.
i think you are right @fbourigault
@andrewkharook can you please adjust this accordingly and add a test that fails with this code but not with the new code? like comparing fake.domain.com.evil.org with domain.com?
|
Thank you @andrewkharook for this PR. Would you mind updating it according to the feedback? |
|
Hi, @Nyholm! |
There was a problem hiding this comment.
looks good to me.
@fbourigault do you want to check again as well?
|
Ping @fbourigault |
|
Thanks ! |
What's in this PR?
The changes make cookies set for main domain available for subdomains as well, as described in RFC 6265
Checklist