- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Removing change record tag, as we have one of those.
Hiding patches because now there's an MR.
Adding cookie compliance table link to the issue summary.
We still need manual testing across browsers here, added table to track that to the issue summary.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Left a review, but leaving it as needs review so we can get some comments from the security team and some manual testing
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Added manual testing steps to the issue summary and pinged in #security-discussions to try and solicit some help
- πͺπΈSpain fjgarlin
My tests:
URL for all: https://8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
Set up:
CHROME
* SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
FIREFOX
* SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
SAFARI
* SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
- πͺπΈSpain fjgarlin
The remaining tests:
URL for all: https://8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
EDGE
* SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
OPERA
* SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io
I think this makes all the listed browsers.
- Status changed to Needs work
almost 2 years ago 1:47pm 22 February 2023 The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to Needs review
almost 2 years ago 2:04pm 22 February 2023 - πͺπΈSpain fjgarlin
The above error seems to be reported against the patch for 9.5, not the MR. Setting back to "Needs review".
Note that I tested the remaining browsers as well, but not sure if further tests or review is needed. - π¬π§United Kingdom alexpott πͺπΊπ
I did some stuff to the MR:
- rebased on to 10.1.x
- Updated mentions to 9.2.0 to 10.1.0
- changed str_pos() to str_contains()
- Status changed to Needs work
almost 2 years ago 7:57pm 13 March 2023 - πΊπΈUnited States smustgrave
Oh wow MR 173 not even a 4 digit one.
There are some open threads in the MR from @larowlan that should be looked at.
- πͺπΈSpain fjgarlin
Updating status of manual tests to add the ones done at #53.
- First commit to issue fork.
- Status changed to Needs review
almost 2 years ago 3:53am 15 March 2023 - πΊπΈUnited States dww
MR had a bunch of merge conflicts since π Set SameSite on session cookies Fixed and π SessionConfiguration::getCookieDomain() return value doc is incorrect. Fixed landed. Manually rebased. Attaching the conflicts.
I'm not sure I resolved the conflicts correctly in
core/lib/Drupal/Core/Session/SessionConfiguration.php
. Definitely needs another pair of eyes on what I did. - Status changed to Needs work
almost 2 years ago 3:49pm 15 March 2023 - πΊπΈUnited States smustgrave
Think the merge conflicts look good!
Only moving to NW as there is a comment in the MR
Should we have a todo for when we plan to phase this out?
Should we emit a deprecation warning here so sites know they need to update their services.yml?That (I can't tell at least) has been answered
- π¬π§United Kingdom mcdruid π¬π§πͺπΊ
Updated IS to show manual testing in the 5 targeted browsers is done (between #50/#53).
- π§πͺBelgium kriboogh
Maybe this isn't the ticket to mention this, but when
SessionConfiguration::getOptions is called we do:
$options['cookie_domain'] = $this->getCookieDomain($request) ?: '';
A few lines after we do:
$options['name'] = $this->getName($request);
=> getName calls getUnprefixedName which in turn depends on $this->options['cookie_domain']:
elseif (isset($this->options['cookie_domain'])) {
but at this point $this->options doesn't have cookie_domain key, since we are using a locally scoped variable $options in the getOptions call.