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.

Sign up for GitHub

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

Jump to bottom

Feature/cookie same site #420

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature/cookie same site #420

wants to merge 5 commits into from

Conversation

piotrbaczek
Copy link
Member

@piotrbaczek piotrbaczek commented Sep 2, 2020 •
edited

PR Details

This PR implements Cookie property "SameSite".

Description

  1. Added 2 classes: Kohana_Cookie_Properties with dictionary related to general cookie properties and Kohana_Cookie_Samesite with settings related to SameSite property.

  2. Class Kohana_Cookie method setcookie got extended with new property $samesite. This method now uses alternative call to PHP native setcookie function, with data passed as an associative array.

Related Issue

None

Please link to the issue here

#418

How Has This Been Tested

This feature has been tested by automatic unit test - adding required cookie parameter to already existing unittest.
Secondary method of testing was done manually by adding following line in Controller_Welcome
Cookie::set('aaa','bbb');
and manually verifying existence of Cookie in Chrome Developer's Console's Application\Cookies tab.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@piotrbaczek piotrbaczek linked an issue Sep 15, 2020 that may be closed by this pull request
Missing Cookie Attribute "SameSite" = None | Strict | Lax #418
Open
@piotrbaczek piotrbaczek self-assigned this Oct 1, 2020
@toitzi
Copy link
Member

toitzi commented Oct 14, 2020

@piotrbaczek Thank you i will look into this pr, i will probably open a new one and merge this devel since it is a new feature.

@piotrbaczek
Copy link
Member Author

I'd like to know if this can be merged and if not then why.

@toitzi
Copy link
Member

toitzi commented Oct 15, 2020

@piotrbaczek Unfortunately not. As far as i can tell it is a new feature and adds fuctionality, this is the reason why it needs to be merged into devel (next feature release) not into master (current feature release). Usually only bug and hotfixes in master (or documentation fixes and that) small features (probably like this one) may also be merged in master but i need time to check it.

@piotrbaczek piotrbaczek changed the base branch from master to devel October 29, 2020 12:09
@piotrbaczek piotrbaczek changed the base branch from devel to master October 29, 2020 12:10
@Daijobou
Copy link

Daijobou commented Feb 3, 2022 •
edited

I'm not that big fan for two extra files because few constants and I don't understand why its need constants for "secure", "expires", .... and I don't think you have to explain every constant in such detail. The following URL as source would be sufficient: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

P.S.
In comment above "protected static function _setcookie(" there is parameter "$samesite" missing.
And you should not use $agent in salt(). See #363 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@piotrbaczek piotrbaczek

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing Cookie Attribute "SameSite" = None | Strict | Lax
3 participants
@piotrbaczek @toitzi @Daijobou