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): Regex header values in traditional router are now validated #12365

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

StarlightIbuki
Copy link
Contributor

Summary

ATT

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

Issue reference

Fix FTI-5403

@StarlightIbuki StarlightIbuki added cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee and removed size/M labels Jan 18, 2024
@kikito kikito requested review from gszr and nowNick February 20, 2024 18:14
Copy link
Contributor

@nowNick nowNick left a comment

Choose a reason for hiding this comment

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

Could we reuse the code for path validation?


Also I'm wondering - what if someone want's a header to start with ~ but not to be treated as regex? Can it be escaped somehow? I assume for route's paths this is not supported but is it supported for headers?

kong/db/schema/typedefs.lua Outdated Show resolved Hide resolved
kong/db/schema/typedefs.lua Show resolved Hide resolved
@StarlightIbuki
Copy link
Contributor Author

what if someone want's a header to start with ~ but not to be treated as regex?

@nowNick It's not a problem for URL paths given every valid path begins with "/". Regex header inherits that design and we did not consider the issue. There is not a way to escape the "~", so something like "~1" can only be expressed with a trivial regex: "~~1".

@StarlightIbuki StarlightIbuki force-pushed the fix/trad-router-header-regex-validate branch from f004449 to 04a01c9 Compare February 22, 2024 07:31
Copy link
Contributor

@nowNick nowNick left a comment

Choose a reason for hiding this comment

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

Good to go! Great job! 🚀

path, err)

local function validate_regex_or_plain_pattern(pattern)
if pattern:sub(1, 1) ~= "~" then
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally extract this to yet another function and we could've ended up with code like this:

local function is_plain_patter(pattern)
  return pattern:sub(1, 1) ~= "~"
end

local function validate_regex_or_plain_pattern(pattern)
  return is_plain_patter(pattern) or is_valid_regex(pattern:sub(2))
end

We could also reuse then is_plain_patter in the function above so that there'll one less check against "~" character which imho is a bit magic_stringy

... but that's probably just nitpicking - I'm happy with the code we have right now 😃

Copy link
Member

Choose a reason for hiding this comment

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

+1

@gszr
Copy link
Member

gszr commented Feb 23, 2024

Does the path typedef have logic in common with this new typedef? Can we refactor to simplify and avoid duplication?

@gszr
Copy link
Member

gszr commented Feb 28, 2024

@StarlightIbuki please review the comments above.

@StarlightIbuki StarlightIbuki force-pushed the fix/trad-router-header-regex-validate branch from b128c5e to 4bd3ada Compare March 14, 2024 02:53
@StarlightIbuki
Copy link
Contributor Author

Does the path typedef have logic in common with this new typedef? Can we refactor to simplify and avoid duplication?

Pathes have different requirements compared to headers. They need to start with slashes.

I've updated the PR to reuse the code as much as possible.

@AndyZhang0707 AndyZhang0707 requested a review from chronolaw April 10, 2024 09:52
@StarlightIbuki StarlightIbuki force-pushed the fix/trad-router-header-regex-validate branch from f991bdd to 6114933 Compare April 26, 2024 08:11
@ADD-SP ADD-SP force-pushed the fix/trad-router-header-regex-validate branch from 6114933 to 219af03 Compare June 19, 2024 07:19
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

Merge on CI green

@StarlightIbuki StarlightIbuki force-pushed the fix/trad-router-header-regex-validate branch from 219af03 to 27f525f Compare June 20, 2024 05:14
@ADD-SP ADD-SP merged commit b5f2fd2 into master Jun 20, 2024
27 checks passed
@ADD-SP ADD-SP deleted the fix/trad-router-header-regex-validate branch June 20, 2024 07:16
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

@randmonkey
Copy link
Contributor

In my impression, the headers are using ~* as the prefix of regex patterns. Would this be a breaking change? I see KIC's tests against the nightly images are failing.

StarlightIbuki added a commit that referenced this pull request Jun 24, 2024
StarlightIbuki added a commit that referenced this pull request Jun 24, 2024
StarlightIbuki added a commit that referenced this pull request Jun 24, 2024
StarlightIbuki added a commit that referenced this pull request Jun 25, 2024
StarlightIbuki added a commit that referenced this pull request Jun 25, 2024
StarlightIbuki added a commit that referenced this pull request Jul 1, 2024
StarlightIbuki added a commit that referenced this pull request Jul 1, 2024
2 condtions:
1. only 1 header value presents
2. it starts with "~*"

Fixup #12365
Fix KAG-4788

Co-authored-by: Qi <[email protected]>
StarlightIbuki added a commit that referenced this pull request Jul 2, 2024
2 condtions:
1. only 1 header value presents
2. it starts with "~*"

Fixup #12365
Fix KAG-4788

Co-authored-by: Qi <[email protected]>
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.

6 participants