Bug 33259 - Optionally set SameSite attribute of cookie to Strict
Summary: Optionally set SameSite attribute of cookie to Strict
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P3 enhancement
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-17 02:29 UTC by Aleisha Amohia
Modified: 2025-04-09 06:14 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 33259: Add SameSiteSessionCookie system preference (4.76 KB, patch)
2023-03-22 23:36 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (24.56 KB, patch)
2023-03-22 23:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (3.78 KB, patch)
2023-03-23 03:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (26.79 KB, patch)
2023-03-23 03:18 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (3.84 KB, patch)
2023-03-23 08:58 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (26.82 KB, patch)
2023-03-23 08:58 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (15.70 KB, patch)
2023-04-27 00:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.01 KB, patch)
2023-05-02 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (27.12 KB, patch)
2023-05-02 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (15.70 KB, patch)
2023-05-02 23:14 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: (follow-up) Add HTML filters to Cookies.set (15.53 KB, patch)
2023-05-02 23:14 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.01 KB, patch)
2023-08-01 23:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (27.08 KB, patch)
2023-08-01 23:38 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (15.66 KB, patch)
2023-08-01 23:38 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: (follow-up) Add HTML filters to Cookies.set (15.52 KB, patch)
2023-08-01 23:38 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 33259: (follow-up) perltidy (1.91 KB, patch)
2024-01-10 21:20 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.08 KB, patch)
2024-01-10 21:21 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (27.01 KB, patch)
2024-01-10 21:21 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (14.97 KB, patch)
2024-01-10 21:21 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: (follow-up) Add HTML filters to Cookies.set (15.45 KB, patch)
2024-01-10 21:21 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: (follow-up) perltidy (1.91 KB, patch)
2024-01-10 21:21 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.03 KB, patch)
2024-01-10 21:23 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (26.99 KB, patch)
2024-01-10 21:23 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.15 KB, patch)
2024-02-15 09:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (27.13 KB, patch)
2024-02-15 09:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (15.08 KB, patch)
2024-02-15 09:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: (follow-up) Add HTML filters to Cookies.set (15.53 KB, patch)
2024-02-15 09:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: (follow-up) perltidy (1.97 KB, patch)
2024-02-15 09:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: (QA follow-up) Default 'Strict' for new installs (1.95 KB, patch)
2024-02-15 09:22 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 33259: Add SameSiteSessionCookie system preference (4.15 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 33259: Optionally set SameSite attribute of session cookie to Strict (27.31 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 33259: (follow-up) Use different speechmarks to prevent errors (15.08 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 33259: (follow-up) Add HTML filters to Cookies.set (15.53 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 33259: (follow-up) perltidy (1.97 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 33259: (QA follow-up) Default 'Strict' for new installs (1.95 KB, patch)
2024-03-11 23:15 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2023-03-17 02:29:39 UTC
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

We should offer a system preference so that libraries who'd like to increase their security can set the SameSite attribute of the session cookie to Strict, rather than the default Lax.
Comment 1 Alex Buckley 2023-03-22 23:36:47 UTC
Created attachment 148577 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Comment 2 Alex Buckley 2023-03-22 23:37:03 UTC
Created attachment 148578 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Comment 3 Alex Buckley 2023-03-23 03:17:36 UTC
Created attachment 148579 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Comment 4 Alex Buckley 2023-03-23 03:18:09 UTC
Created attachment 148580 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Comment 5 Alex Buckley 2023-03-23 03:19:20 UTC
Ready for testing.

Note: I have not attached unit tests because no unit test files currently check the cookie values.
Comment 6 ByWater Sandboxes 2023-03-23 08:58:37 UTC
Created attachment 148589 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 7 ByWater Sandboxes 2023-03-23 08:58:39 UTC
Created attachment 148590 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 8 Sally 2023-03-23 08:59:05 UTC
Works as described, thanks for the patch.
Comment 9 Aleisha Amohia 2023-04-27 00:30:13 UTC
Created attachment 150259 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully
Comment 10 Lucas Gass (lukeg) 2023-05-01 22:37:55 UTC
Alex,

The QA tool complains about quite a few missing filters within Cookies.set. I think we can add the html filter in those cases, right? 

Also, should none be option along with lax and strict?
Comment 11 Alex Buckley 2023-05-02 23:13:47 UTC
Created attachment 150557 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 12 Alex Buckley 2023-05-02 23:13:59 UTC
Created attachment 150558 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 13 Alex Buckley 2023-05-02 23:14:14 UTC
Created attachment 150559 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully
Comment 14 Alex Buckley 2023-05-02 23:14:28 UTC
Created attachment 150560 [details] [review]
Bug 33259: (follow-up) Add HTML filters to Cookies.set

Test plan:
Run QA test tools and confirm the no complaints about filters
Comment 15 Alex Buckley 2023-05-02 23:15:11 UTC
(In reply to Lucas Gass from comment #10)
> Alex,
> 
> The QA tool complains about quite a few missing filters within Cookies.set.
> I think we can add the html filter in those cases, right? 
> 
> Also, should none be option along with lax and strict?

Hi Lucas, 

Thanks for your comments. 

Yes, adding the HTML filters does fix up the QA test tool failures, so I have added that as a followup patch.

Currently all Koha cookies have 'samesite' = 'Lax' which is why I made it the default syspref value. 

However, according to https://web.dev/samesite-cookies-explained/ there are three possible options for the 'samesite' attribute:
- Strict
- Lax 
- None  - means you intentionally want the cookie sent in a third-party context.

So I think yes we should probably add 'none' as a value along with lax and strict. 

I have amended the first patch 'Add SameSiteSessionCookie system preference' to add the 'none' option. 

Thanks,
Alex
Comment 16 Alex Buckley 2023-08-01 23:37:49 UTC
Created attachment 154142 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 17 Alex Buckley 2023-08-01 23:38:06 UTC
Created attachment 154143 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 18 Alex Buckley 2023-08-01 23:38:22 UTC
Created attachment 154144 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully
Comment 19 Alex Buckley 2023-08-01 23:38:33 UTC
Created attachment 154145 [details] [review]
Bug 33259: (follow-up) Add HTML filters to Cookies.set

Test plan:
Run QA test tools and confirm the no complaints about filters
Comment 20 Alex Buckley 2023-08-01 23:38:56 UTC
Rebased against master, applies cleanly now
Comment 21 David Cook 2023-08-02 00:59:26 UTC
It might be worth adding that "None" has security implications and is not recommended? 

Overall, I think only "Lax" and "Strict" makes sense for Koha with "Strict" being preferable overall, although it would need some testing, especially with identity providers...
Comment 22 Alex Buckley 2023-08-02 03:44:29 UTC
(In reply to David Cook from comment #21)
> It might be worth adding that "None" has security implications and is not
> recommended? 
> 
> Overall, I think only "Lax" and "Strict" makes sense for Koha with "Strict"
> being preferable overall, although it would need some testing, especially
> with identity providers...

That's a great idea to add that None has security implications and isn't recommended, thanks for suggesting that David, 

Will add that to the syspref description in the yaml file.
Comment 23 David Cook 2023-08-02 05:14:34 UTC
Another thing about "None" is that I think it can only be set with the Secure attribute set as well. 

Not sure what happens when you try to send a cookie with "None" that isn't secure... I imagine it would default back to Lax but don't know for sure.
Comment 24 Matthias Le Gac 2024-01-10 20:10:28 UTC
Maybe need a rebase I can't apply the patch.
Comment 25 Lucas Gass (lukeg) 2024-01-10 21:06:29 UTC
(In reply to Matthias Le Gac from comment #24)
> Maybe need a rebase I can't apply the patch.

Merge conflicts due to Bug 35651.
Comment 26 Lucas Gass (lukeg) 2024-01-10 21:20:13 UTC
Created attachment 160786 [details] [review]
Bug 33259: (follow-up) perltidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 27 Lucas Gass (lukeg) 2024-01-10 21:21:10 UTC
Created attachment 160787 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 28 Lucas Gass (lukeg) 2024-01-10 21:21:12 UTC
Created attachment 160788 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 29 Lucas Gass (lukeg) 2024-01-10 21:21:15 UTC
Created attachment 160789 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 30 Lucas Gass (lukeg) 2024-01-10 21:21:18 UTC
Created attachment 160790 [details] [review]
Bug 33259: (follow-up) Add HTML filters to Cookies.set

Test plan:
Run QA test tools and confirm the no complaints about filters

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 31 Lucas Gass (lukeg) 2024-01-10 21:21:20 UTC
Created attachment 160791 [details] [review]
Bug 33259: (follow-up) perltidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 32 Aleisha Amohia 2024-01-10 21:23:52 UTC
Created attachment 160792 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 33 Aleisha Amohia 2024-01-10 21:23:56 UTC
Created attachment 160793 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 34 Aleisha Amohia 2024-01-10 21:25:37 UTC
Oops Lucas and I were working on this at the same time. I'll obselete what I did and put his back.
Comment 35 Lucas Gass (lukeg) 2024-01-10 21:48:28 UTC
(In reply to Aleisha Amohia from comment #34)
> Oops Lucas and I were working on this at the same time. I'll obselete what I
> did and put his back.

Aleisha, I'm sorry. Since my bug caused the merge conflict I'd figure I would try the rebase. You're more than welcome to obsolete my rebase.
Comment 36 Martin Renvoize (ashimema) 2024-02-15 09:15:10 UTC
Created attachment 162181 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize (ashimema) 2024-02-15 09:15:13 UTC
Created attachment 162182 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize (ashimema) 2024-02-15 09:15:16 UTC
Created attachment 162183 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize (ashimema) 2024-02-15 09:15:19 UTC
Created attachment 162184 [details] [review]
Bug 33259: (follow-up) Add HTML filters to Cookies.set

Test plan:
Run QA test tools and confirm the no complaints about filters

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize (ashimema) 2024-02-15 09:15:21 UTC
Created attachment 162185 [details] [review]
Bug 33259: (follow-up) perltidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize (ashimema) 2024-02-15 09:21:28 UTC
No regressions found, this is a real improvement.

My one slight query though is the default, I'd be tempted to set it to Strict on new installs by default. It's great allowing people to downgrade their security a little as needed, but I think we should default to highest security level.
Comment 42 Martin Renvoize (ashimema) 2024-02-15 09:22:28 UTC
Created attachment 162186 [details] [review]
Bug 33259: (QA follow-up) Default 'Strict' for new installs
Comment 43 David Cook 2024-02-15 22:29:16 UTC
(In reply to Martin Renvoize from comment #41)
> No regressions found, this is a real improvement.
> 
> My one slight query though is the default, I'd be tempted to set it to
> Strict on new installs by default. It's great allowing people to downgrade
> their security a little as needed, but I think we should default to highest
> security level.

+1
Comment 44 David Cook 2024-02-15 22:30:16 UTC
Comment on attachment 162186 [details] [review]
Bug 33259: (QA follow-up) Default 'Strict' for new installs

Review of attachment 162186 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/mandatory/sysprefs.sql
@@ +668,4 @@
>  ('RoutingListAddReserves','0','','If ON the patrons on routing lists are automatically added to holds on the issue.','YesNo'),
>  ('RoutingListNote','To change this note edit <a href=\"/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RoutingListNote#jumped\">RoutingListNote</a> system preference.','70|10','Define a note to be shown on all routing lists','Textarea'),
>  ('RoutingSerials','1',NULL,'If ON, serials routing is enabled','YesNo'),
> +('SameSiteSessionCookie', 'Strict', 'None|Lax|Strict', 'If Strict, then the SameSite attribute of the session cookie will be set to Strict, rather than the default Lax. None means you want the cookie sent iun a third-party context.', 'Choice'),

There's a little typo here. "iun" instead of "in".
Comment 45 David Cook 2024-03-11 23:13:39 UTC
Rebasing this one as opac/sco-sco-main.pl wasn't applying anymore.
Comment 46 David Cook 2024-03-11 23:14:44 UTC
Technically, a lot of this was probably overkill. The key cookie that needed Strict is the CGISESSID cookie, because that's where there's security implications. 

However, it doesn't hurt doing it like we have here.
Comment 47 David Cook 2024-03-11 23:15:35 UTC
Created attachment 163066 [details] [review]
Bug 33259: Add SameSiteSessionCookie system preference

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 David Cook 2024-03-11 23:15:39 UTC
Created attachment 163067 [details] [review]
Bug 33259: Optionally set SameSite attribute of session cookie to Strict

Note: The below test plan is assuming you are running the Mozilla Firefox
browser.

Test plan:
1. Apply patches
2. Upgrade database
sudo koha-upgrade-schema <instance>
3. Confirm the new system preference 'SameSiteSessionCookie' is 'Lax'
4. Go to a staff client and OPAC page, right click, choose 'Inspect', go
   to the 'Storage' tab, click on 'Cookies', click on 'CGISESSID'
5. Confirm 'SameSite' = 'Lax'
6. Change the 'SameSiteSessionCookie' to 'Strict'
7. Open a new private browser window. Go to a staff client and OPAC
page, right click, choose 'Inspect, go to the 'Storage' tab, click on
'Cookies', click on 'CGISESSID'
8. Repeat step 4 and confirm 'SameSite' = 'Strict'
9. Go to: Cataloguing > New record
10. Right click, click 'Inspect', go to the 'Console' tab, confirm there
    are no JavaScript errors
11. Change 'SameSiteSessionCookie' to 'None'
12. Open a new private browser window, repeat step 4, and confirm the
'SameSite' = 'None'
13. Go to: Cataloguing > New record
14. Right click, click 'Inspect', go to the 'Console' tab, confirm there
are no JavaScript errors.

Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 David Cook 2024-03-11 23:15:42 UTC
Created attachment 163068 [details] [review]
Bug 33259: (follow-up) Use different speechmarks to prevent errors

To test: Confirm the checkouts table for a patron loads successfully

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 David Cook 2024-03-11 23:15:45 UTC
Created attachment 163069 [details] [review]
Bug 33259: (follow-up) Add HTML filters to Cookies.set

Test plan:
Run QA test tools and confirm the no complaints about filters

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 David Cook 2024-03-11 23:15:49 UTC
Created attachment 163070 [details] [review]
Bug 33259: (follow-up) perltidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 David Cook 2024-03-11 23:15:52 UTC
Created attachment 163071 [details] [review]
Bug 33259: (QA follow-up) Default 'Strict' for new installs
Comment 53 David Cook 2024-03-11 23:26:58 UTC
Actually, I'm momentarily going to move this to "In Discussion".

I think using a Strict SameSite attribute would break the SSO implementation especially in terms of CSRF.

You'd have an anonymous session, then you redirect to the IdP, and then you're redirected back.

Since you're being redirected back via an external site, your browser shouldn't send the Strict CGISESSID cookie, which means your CSRF validation will fail.

I'll test that in a minute.

We might need to always use a "Lax" SameSite attribute for anonymous sessions.

I'll try to think of any other problem scenarios...
Comment 54 David Cook 2024-03-11 23:55:40 UTC
Going to mark this as Failed QA in any case as it doesn't take into account the CGISESSID cookie created by the OIDC in Koha/REST/V1/OAuth/Client.pm.

--

Also, as I feared, with SameSite of "Strict", OIDC SSO is broken. 

--

Interestingly, when Keycloak POSTs a 302 to Koha it doesn't work, but there is a scenario with Keycloak where a Strict cookie is still sent after a redirect.

If I log into Keycloak first, and then I go to Koha and click "Log in with Keycloak", I'm redirected from Koha to Keycloak and redirected from Keycloak to Koha. In this situation where it's two GET 302s in a row, the Strict cookie is still sent, and I'm logged into Koha using Keycloak.
Comment 55 David Cook 2024-03-12 00:17:15 UTC
(In reply to David Cook from comment #54)
> Interestingly, when Keycloak POSTs a 302 to Koha it doesn't work, but there
> is a scenario with Keycloak where a Strict cookie is still sent after a
> redirect.
> 
> If I log into Keycloak first, and then I go to Koha and click "Log in with
> Keycloak", I'm redirected from Koha to Keycloak and redirected from Keycloak
> to Koha. In this situation where it's two GET 302s in a row, the Strict
> cookie is still sent, and I'm logged into Koha using Keycloak.

I'm not describing this right. With Strict, it's failing because Keycloak is doing a GET to Koha and that's not allowed. With Lax, it's succeeding because it's allowed for Keycloak to do a GET to Koha. 

And I think in the scenario where it goes Koha -> Keycloak -> Koha will all 302s, I think the browser is considering that to be user-initiated from the Koha side, and that's why it's allowing it to send the cookie.

Because we have a POST to Keycloak with the login, that's why it won't let us login with Keycloak when Strict is in use.

I think that brings me back to anonymous CGISESSID cookies needing to be Lax, even if an authenticated CGISESSID cookie is Strict.
Comment 56 David Cook 2024-03-12 00:28:17 UTC
Oh good. Chrome and Firefox appear to work differently. 

In Chrome, if you login to Keycloak first, and then do that Koha->Keycloak->Koha redirect you're fine.

In Firefox, if you login to Keycloak first, and then do that Koha->Keycloak->Koha redirect, you still will get an error. 

Of course, you should only be clicking "Login with Keycloak" from an anonymous session, which takes me back to that thing about the anonymous session not being Strict even when authenticated cookies are Strict.
Comment 57 David Cook 2024-03-12 00:39:16 UTC
I hate to say it now, but I think we should change tack, and just focus on the SameSiteSessionCookie being for the CGISESSID cookie and only for authenticated contexts. (Focusing only on the CGISESSID cookie is mostly just to help in terms of testability, although I think the best practice is to only set Strict for sensitive cookies.)

That should provide security for authenticated Koha users while also allowing usability for things like SSO.
Comment 58 David Cook 2025-04-09 03:15:49 UTC
(In reply to David Cook from comment #56)
> Oh good. Chrome and Firefox appear to work differently. 
> 
> In Chrome, if you login to Keycloak first, and then do that
> Koha->Keycloak->Koha redirect you're fine.
> 
> In Firefox, if you login to Keycloak first, and then do that
> Koha->Keycloak->Koha redirect, you still will get an error. 
 
I'm working on integrating another system with OIDC (testing with Keycloak), and I'm running into the same problem. 

My app session cookie is set to Strict and since it's going "302 App to Keycloak" -> "200 Keycloak to Keycloak" -> "302 Keycloak to App" Chrome prevents the original session cookie from being sent to the App.

But after a successful Keycloak to Keycloak login, then it's "302 App to Keycloak" -> "302 Keycloak to App", and then Chrome will send the session cookie to the App. 

Firefox will prevent the cookie from being sent in both scenarios. 

--

In my App, I don't think that I have the ability to use different SameSite attributes for anonymous sessions vs authenticated sessions. It's governed by config for a plugin to the MVC framework. I might have a little play, but I'm thinking I might just use a different cookie for managing the whole SSO interaction. 

I'll let you know how I go...
Comment 59 David Cook 2025-04-09 06:14:04 UTC
(In reply to David Cook from comment #58)
> In my App, I don't think that I have the ability to use different SameSite
> attributes for anonymous sessions vs authenticated sessions. It's governed
> by config for a plugin to the MVC framework. I might have a little play, but
> I'm thinking I might just use a different cookie for managing the whole SSO
> interaction. 
> 
> I'll let you know how I go...

I ended up saving the app sessionid in a "sso cookie" with a very short expiry and a SameSite of Lax. So when the SSO redirected back to the App, I used the "sso cookie" to get the sessionid, looked up the older anonymous session using the sessionid, and then I was able to access the relevant state data.

I used that to validate the "state" value, fetch the redirect_uri I used for the auth request, etc. 

It seemed like the least amount of change to get the desired result while still maintaining security. 

--

At some point with Koha it would be nice to be able to redirect back to the point where the SSO was initiated, but that's difficult with Plack since the request uri gets shortened to "/opac" and "/intranet", although I guess we can always have rewrites for those.