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(core): definition of cookie name validate #11881

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Oct 30, 2023

Summary

We have a too strong limitation on the character used in cookie names. For example "." should be allowed.

We choose not to do the same to the header because we rely on Nginx's API to handle headers, which has more strict restrictions on the characters.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md

Full changelog

  • [Implement ...]

Issue reference

Fix #11860

@bungle
Copy link
Member

bungle commented Oct 30, 2023

This regex looks simpler (and whitelist feels safer than blacklist):
https://github.com/guzzle/guzzle/blob/7.9/src/Cookie/SetCookie.php#L464

@StarlightIbuki what do you think?

@StarlightIbuki
Copy link
Contributor Author

StarlightIbuki commented Oct 31, 2023

This regex looks simpler (and whitelist feels safer than blacklist): guzzle/guzzle@7.9/src/Cookie/SetCookie.php#L464

@StarlightIbuki what do you think?

Applied the change. And notice the code you refer to also uses a blacklist.

@StarlightIbuki StarlightIbuki marked this pull request as ready for review October 31, 2023 06:39
@StarlightIbuki StarlightIbuki requested a review from bungle October 31, 2023 07:22
kong/tools/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
@StarlightIbuki StarlightIbuki force-pushed the fix/http_token branch 2 times, most recently from f8bbdeb to 5a2cb3e Compare October 31, 2023 08:33
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Typo, otherwise looks good to me.

kong/tools/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
@bungle
Copy link
Member

bungle commented Oct 31, 2023

Applied the change. And notice the code you refer to also uses a blacklist.

Ah, damn, you are right. I read it wrong.

kong/tools/utils.lua Outdated Show resolved Hide resolved

typedefs.cookie_name = Schema.define {
type = "string",
custom_validator = utils.validate_cookie_name,
Copy link
Member

Choose a reason for hiding this comment

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

If the function is only used here, please remove it from utils and into typedefs.lua

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unit-tested so it needs to be exported. Also before the patch, this function was already exported and in the util module.

@StarlightIbuki
Copy link
Contributor Author

@hanshuebner Could you review this again?

@hanshuebner hanshuebner merged commit 04f0b3e into master Nov 6, 2023
23 checks passed
@hanshuebner hanshuebner deleted the fix/http_token branch November 6, 2023 15:48
@StarlightIbuki StarlightIbuki added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 8, 2024
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-11881-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-11881-to-master-to-upstream
git checkout -b cherry-pick-11881-to-master-to-upstream
ancref=$(git merge-base 5f34a49edc356b798f25a340522d8efe2c4f5d95 8b88a976268f1c211962b309c9017e9d8b291883)
git cherry-pick -x $ancref..8b88a976268f1c211962b309c9017e9d8b291883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/db schema-change-noteworthy size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie Name validation too strict
6 participants