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

Fix cookie parsing removing extra = #44218

Merged
merged 2 commits into from
Jan 3, 2023
Merged

Fix cookie parsing removing extra = #44218

merged 2 commits into from
Jan 3, 2023

Conversation

fli
Copy link
Contributor

@fli fli commented Dec 21, 2022

When parsing a cookie, extra = characters are removed, when only the first should be removed.

e.g. with the cookie csrf_token_ae6261a96213c493a37ea69489ee39c8bc33a53cda7d95f84efa53146145d09c=lnQptRUO/gpU26e8ZKpGIFHKqtP54vVfR7RBiph8Uc0=

You would expect:
key: csrf_token_ae6261a96213c493a37ea69489ee39c8bc33a53cda7d95f84efa53146145d09c
value: lnQptRUO/gpU26e8ZKpGIFHKqtP54vVfR7RBiph8Uc0=

If you use split, it will remove the last = in value, so you get:
key: csrf_token_ae6261a96213c493a37ea69489ee39c8bc33a53cda7d95f84efa53146145d09c
value: lnQptRUO/gpU26e8ZKpGIFHKqtP54vVfR7RBiph8Uc0

This is because split still removes all = characters, even if you use the limit parameter to limit it to the first 2 elements (as in the existing code).

Solution is to not use split (I've used slice instead)

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have a helpful link attached, see contributing.md

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR can we add a test case for this to ensure we don't regress?

@fli
Copy link
Contributor Author

fli commented Dec 31, 2022

Hi, thanks for the PR can we add a test case for this to ensure we don't regress?

Sure thing, have added a few tests (hopefully in the right place)

@fli fli requested a review from ijjk December 31, 2022 03:16
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Change and added tests look good, thanks for the PR!

@ijjk ijjk merged commit 9ab6d3b into vercel:canary Jan 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants