- Issue created by @fago
- 🇦🇹Austria fago Vienna
This is handled by Drupal session configuration like this;
protected function getCookieDomain(Request $request) { // $this->options is what is set as session configuration via drupal services.yml config if (isset($this->options['cookie_domain'])) { $cookie_domain = $this->options['cookie_domain']; } else { $host = $request->getHost(); // To maximize compatibility and normalize the behavior across user // agents, the cookie domain should start with a dot. $cookie_domain = '.' . $host; } // Cookies for domains without an embedded dot will be rejected by user // agents in order to defeat malicious websites attempting to set cookies // for top-level domains. Also IP addresses may not be used in the domain // attribute of a Set-Cookie header. IPv6 addresses will not pass the first // test, so it's acceptable to bias the second test to IPv4. if (count(explode('.', $cookie_domain)) > 2 && !is_numeric(str_replace('.', '', $cookie_domain))) { return $cookie_domain; }
When nothing is returned, the request is returned with a Domain parameter set. Without it being set, browsers implement the safe default:
Section 5.2.3 of RFC 6265, which defines the HTTP State Management Mechanism (including cookies), states:
If the server omits the Domain attribute, the user agent will return the cookie only to the origin server.
For example, if the value of the Host header field is "example.com", a Set-Cookie header field containing a cookie without a Domain attribute will set a cookie with a domain attribute of "example.com".
Full RFC: https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.3
Thus,when we drop the else clause
$host = $request->getHost();
we get a working cookie on the frontend and backend, while staying safe. It's even better than the drupal default ".$host" since that allows also other sub-domains.Let's provide a new alternative session configuration that decorates the existing one, which does not use the host default. Then, inject that into the session manager. Finally, we should add a test-case to ensure this does not break during major upgrades.
- Status changed to Needs review
3 months ago 6:27am 5 September 2024 - 🇦🇹Austria fago Vienna
ok, tests where a bit tricky but got it working now. Turns out this is not properly doable with kernel tests, so moved it to a functional test case.
- Status changed to Needs work
3 months ago 9:25am 5 September 2024 - 🇸🇮Slovenia useernamee Ljubljana
@fago I added my remarks to the PR:
- I'm wondering what happens when IPv4 and IPv6 checks are skipped.
- We only override session_configuration service in one place (inside session configuration). The service is used on other places as well which might cause some confusion.
- Should we properly decorate the session_configuration service?
- Service provider should be in /src folder. - Status changed to Needs review
3 months ago 5:53am 6 September 2024 - 🇦🇹Austria fago Vienna
> - We only override session_configuration service in one place (inside session configuration). The service is used on other places as well which might cause some confusion.
The only goal is to change how Session-Manager works, so this seems to be fine. I cannot think of any other usages of cookie_domain besides sending this header, this is the one and only purpose. So this seems fine to me.
> - I'm wondering what happens when IPv4 and IPv6 checks are skipped.
It does not skip anything. When the value is set, it calls the parent and all the parent logic is run as normal.> - Should we properly decorate the session_configuration service?
I thought it's less intrusive when avoiding to do so, since only once module can do that. Less chance for conflics.> - Service provider should be in /src folder.
good point, updated! strange it still works! fixed. - Status changed to Fixed
3 months ago 5:24am 10 September 2024 Automatically closed - issue fixed for 2 weeks with no activity.