Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set samesite=strict coockies - will work starting php 7.3 #37442

Merged
merged 1 commit into from
May 27, 2020

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 25, 2020

Description

Starting with php7.3 samesite cookies can be set. This PR sets the session cookie to be samesite=stritc.

Related Issue

https://github.com/owncloud/enterprise/issues/3890

How Has This Been Tested?

🤖

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

@DeepDiver1975 DeepDiver1975 force-pushed the feature/strict-same-site-cookies branch 2 times, most recently from 6e4ee95 to fad251e Compare May 26, 2020 07:17
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #37442 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37442      +/-   ##
============================================
- Coverage     64.68%   64.67%   -0.01%     
- Complexity    19331    19332       +1     
============================================
  Files          1277     1277              
  Lines         75514    75521       +7     
  Branches       1331     1331              
============================================
  Hits          48846    48846              
- Misses        26276    26283       +7     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <0.00%> (-0.01%) 19332.00 <0.00> (+1.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/Session/CryptoWrapper.php 33.33% <0.00%> (-13.73%) 7.00 <0.00> (+1.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ef3bad...7429797. Read the comment docs.

@DeepDiver1975 DeepDiver1975 requested a review from C0rby May 26, 2020 09:49
@DeepDiver1975 DeepDiver1975 self-assigned this May 26, 2020
@DeepDiver1975 DeepDiver1975 force-pushed the feature/strict-same-site-cookies branch from fad251e to e3b6780 Compare May 26, 2020 09:51
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review May 26, 2020 09:51
@DeepDiver1975 DeepDiver1975 force-pushed the feature/strict-same-site-cookies branch from e3b6780 to 7dbbfee Compare May 26, 2020 10:35
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

👍 looks good to me.

@owncloud owncloud deleted a comment from update-docs bot May 26, 2020
@phil-davis
Copy link
Contributor

@DeepDiver1975 override codecov results?

I noticed that codecov is not being collected for unit test runs on PHP 7.3 and 7.4 - so that is probably why codecov failed here. I raised PR #37450 to fix that.

@phil-davis phil-davis force-pushed the feature/strict-same-site-cookies branch from 7dbbfee to 2dd6d55 Compare May 27, 2020 03:18
@phil-davis
Copy link
Contributor

PR #37450 was merged. I rebased this - that will be a test of if the extra coverage data makes a different to the codecov results.

@DeepDiver1975 DeepDiver1975 force-pushed the feature/strict-same-site-cookies branch from 2dd6d55 to 7429797 Compare May 27, 2020 06:49
@DeepDiver1975
Copy link
Member Author

From my pov we can ignore the code coverage on this PR. Ready to be merged from my pov.

@phil-davis
Copy link
Contributor

Fine with me - someone with privs can override and merge.

@DeepDiver1975 DeepDiver1975 merged commit c5b037d into master May 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/strict-same-site-cookies branch May 27, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants