Problem/Motivation

On sites served by https, the session cookie starts with "SSESS" because of Drupal\Core\Session\SessionConfiguration::getName().

drupalUserIsLoggedIn only checks for cookies starting with "SESS".

Proposed resolution

Allow cookies starting with SESS and SSESS.

Note we can't test this on drupal.org because DrupalCI does not offer https testing.

Remaining tasks

Fix it.

CommentFileSizeAuthor
#8 3078676-8.patch653 byteschr.fritsch
#2 3078676.patch657 byteschr.fritsch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Active » Needs review
FileSize
3078676.patch657 bytes

Here is a patch to fix it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes

This looks untestable on DrupalCI but the change makes sense.

mtodor’s picture

mtodor CreditAttribution: mtodor at Thunder commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. Regex is simple and easy to understand, so I would keep it as it is.
I have also tested with HTTPS and HTTP and it works in both cases.

Good job in finding this and fixing it!

Should we also fix this in 8.8.x?

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
CreditAttribution: justafish at Lullabot for DocuSign commented
Status: Reviewed & tested by the community » Needs work

The change also makes sense to me, but I think that regex will match for SSESS anywhere. How about ^S?SESS instead.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Great catch @justafish - https://regex101.com/r/gZPVSH/4 vs https://regex101.com/r/gZPVSH/3

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

And now with @justafish's suggested regex - https://regex101.com/r/gZPVSH/5 - works great.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review
FileSize
3078676-8.patch653 bytes

Nice. Here is the updated patch.

mtodor’s picture

mtodor CreditAttribution: mtodor at Thunder commented
Status: Needs review » Reviewed & tested by the community

Good catch @justafish, I didn't notice missing parentheses. :(

I have tested this again and it works.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed ffce54ecc7 to 8.8.x and b4b3f327ec to 8.7.x. Thanks!

  • alexpott committed ffce54e on 8.8.x 
    Issue #3078676 by chr.fritsch, justafish: drupalUserIsLoggedIn doesn't...

  • alexpott committed b4b3f32 on 8.7.x 
    Issue #3078676 by chr.fritsch, justafish: drupalUserIsLoggedIn doesn't...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.