X Tutup
Skip to content

feat: add samesite cookie attribute to session cookie#38477

Merged
DeepDiver1975 merged 1 commit intomasterfrom
relax-samesite-session-cookie
Mar 9, 2021
Merged

feat: add samesite cookie attribute to session cookie#38477
DeepDiver1975 merged 1 commit intomasterfrom
relax-samesite-session-cookie

Conversation

@fschade
Copy link
Contributor

@fschade fschade commented Mar 8, 2021 •
edited
Loading

Description

to decrypt oc_sessionPassphrase we need to have the session cookie samesite attribute to be set the same

Related Issue

Motivation and Context

the passphrase cookie should have the same samesite attribute as the oc_sessionPassphrase cookie. If they are different it's not possible to decrypt oc_sessionPassphrase in cases like a iframe because the passphrase then is unknown.

How Has This Been Tested?

  • manual in teams

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@fschade fschade added this to the development milestone Mar 8, 2021
@fschade fschade self-assigned this Mar 8, 2021
IljaN
IljaN approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@IljaN IljaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

DeepDiver1975
DeepDiver1975 requested changes Mar 8, 2021
View reviewed changes
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. conflict
  2. drone :red:
  3. please explain why this is necessary - usage scenario- THX

@fschade fschade force-pushed the relax-samesite-session-cookie branch from bcc872e to fea786a Compare March 9, 2021 09:01
@mmattel
Copy link
Contributor

mmattel commented Mar 9, 2021

strict --> Strict
in this case you also have to fix: https://github.com/owncloud/core/pull/38458/files#diff-3bbe91e1f85eec5dbd0031642dfb0ad6749b550fc3b94af7aa68a98210b78738R277
(config/config.sample.php: 'http.cookie.samesite' => 'strict',)

@fschade
Copy link
Contributor Author

fschade commented Mar 9, 2021

strict --> Strict
in this case you also have to fix: https://github.com/owncloud/core/pull/38458/files#diff-3bbe91e1f85eec5dbd0031642dfb0ad6749b550fc3b94af7aa68a98210b78738R277
(config/config.sample.php: 'http.cookie.samesite' => 'strict',)

thanks @mmattel, i updated the pr

@fschade fschade requested review from DeepDiver1975 and IljaN March 9, 2021 13:22
@fschade
Copy link
Contributor Author

fschade commented Mar 9, 2021

  1. conflict
  2. drone :red:
  3. please explain why this is necessary - usage scenario- THX

i updated "Motivation and Context"

DeepDiver1975
DeepDiver1975 requested changes Mar 9, 2021
View reviewed changes
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

squash plz

@fschade
Copy link
Contributor Author

fschade commented Mar 9, 2021 •
edited
Loading

squash

i like to keep the history, anyway done

@fschade
add samesite cookie attribute to session cookie too
c30ec4b 
update config.sample.php to use uppercase http.cookie.samesite attribute
reference pr in changelog
@fschade fschade force-pushed the relax-samesite-session-cookie branch from f2f7f8d to c30ec4b Compare March 9, 2021 13:43
@fschade fschade requested a review from DeepDiver1975 March 9, 2021 13:44
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

micbar
micbar approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Mar 9, 2021

i like to keep the history, anyway done

we always squash before merge

@DeepDiver1975 DeepDiver1975 merged commit c942d63 into master Mar 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the relax-samesite-session-cookie branch March 9, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DeepDiver1975 DeepDiver1975 DeepDiver1975 approved these changes

@IljaN IljaN Awaiting requested review from IljaN

+1 more reviewer

@micbar micbar micbar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fschade fschade

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

5 participants

@fschade @mmattel @DeepDiver1975 @IljaN @micbar
X Tutup