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

Support SameSite cookie #50

Merged
merged 7 commits into from
Dec 13, 2022
Merged

Support SameSite cookie #50

merged 7 commits into from
Dec 13, 2022

Conversation

ckifer
Copy link
Contributor

@ckifer ckifer commented Dec 5, 2022

Issue # (if available): #47

Description of changes:

  • Support SameSite directive similar to feat: httpOnly param #41
    • Add type for SameSite
    • Set SameSite using existing pattern
    • Unit test for addition and for incorrect value validation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

borisfba and others added 4 commits December 5, 2022 15:13
Encode illegal characters inside cookies and change cookies parsing l…
15d2534 
…ogic (awslabs#46)

* Make produced cookies RFC 6265 compliant by URI encoding illegal characters.
* Revise cookies parsing logic to fix issues with subdomains (awslabs#43 )
@borisfba
Release 1.3.1 (awslabs#49)
f1e4c4c
feat: support SameSite cookie directive
428e6e6
Merge branch 'main' into feat/sameSiteCookie
abb9a4d
@ckifer
Copy link
Contributor Author

ckifer commented Dec 9, 2022

@borisfba @jeandek any chance this gets merged soon? Thanks :)

borisfba
borisfba reviewed Dec 13, 2022
View reviewed changes
Copy link
Member

@borisfba borisfba left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! There are a few small things that could be improved and it should be ready to merge

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/util/cookie.ts Show resolved Hide resolved
src/util/cookie.ts Outdated Show resolved Hide resolved
@borisfba
Copy link
Member

Please also update the readme with a new configuration parameter

@ckifer
Copy link
Contributor Author

ckifer commented Dec 13, 2022 •
edited
Loading

Hey @borisfba. Thanks for the review! Made documentation fixes/additions, and made sameSite optional!

Not sure if I agree on the use of an enum here, if my solution doesn't satisfy your concern I will refactor to an enum (the difference is nit, happy to use an enum).
I want to avoid having to export the SameSite enum for use by consumers if possible. This way a user can just specify one of the union type values and be on their way without a new import.

Defined the allowed values directly below the union type and added a unit test as a mitigation.

borisfba
borisfba approved these changes Dec 13, 2022
View reviewed changes
Copy link
Member

@borisfba borisfba left a comment

Choose a reason for hiding this comment

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

LGTM!

@borisfba borisfba merged commit 49865ae into awslabs:main Dec 13, 2022
@ckifer ckifer deleted the feat/sameSiteCookie branch December 13, 2022 19:18
@ckifer ckifer mentioned this pull request Dec 13, 2022
Closed
@jeandek jeandek added this to the 1.3.2 milestone Feb 16, 2023
@jeandek jeandek added the added-feature For PRs which containg a new feature (may be in response to a `feature-request`) label Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borisfba borisfba borisfba approved these changes

Assignees
No one assigned
Labels
added-feature For PRs which containg a new feature (may be in response to a `feature-request`)
Projects
None yet
Milestone
1.3.2
Development

Successfully merging this pull request may close these issues.
3 participants
@ckifer @borisfba @jeandek