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

INS-2627: sanitize path-variable name regex capture for kong 3.x #5958

Conversation

filfreire
Copy link
Member

@filfreire filfreire commented May 12, 2023

Closes INS-2627
Closes FTI-5060

changelog(Fixes): Added a fix to handle an edge of how path variable names are captured from regex when parsing OAS specs for Kong 3.x.

Minor change, to mimic what was done in kced - see https://github.com/Kong/go-apiops/blob/8842f7b0ecf3654f62f8e0bd867b5b67766794c9/convertoas3/oas3.go#L50

@filfreire filfreire requested review from a team, nephel and Tieske May 12, 2023 13:22
if (result.startsWith('_')) {
result = 'a' + result;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we externalize this in to its own function? that function can then be called from here, as well as from the validation-generator, to ensure both will have the same name, and the user doesn't have to manually update his/her OAS spec

here's the Go-function; https://github.com/Kong/go-apiops/blob/8842f7b0ecf3654f62f8e0bd867b5b67766794c9/convertoas3/oas3.go#L47-L57

called when generating the regex: https://github.com/Kong/go-apiops/blob/8842f7b0ecf3654f62f8e0bd867b5b67766794c9/convertoas3/oas3.go#L819

called when generating the validation: https://github.com/Kong/go-apiops/blob/8842f7b0ecf3654f62f8e0bd867b5b67766794c9/convertoas3/validator.go#L60-L64

Copy link
Member Author

@filfreire filfreire May 15, 2023

Choose a reason for hiding this comment

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

Done, moved it into it's own function, and has the same name 👍

// Remove illegal chars from path-variable name.
// see https://github.com/Kong/go-apiops/blob/8842f7b0ecf3654f62f8e0bd867b5b67766794c9/convertoas3/oas3.go#L50
if (!legacy) {
result = result.replace(/-/g, '_');
Copy link
Member

Choose a reason for hiding this comment

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

To do this properly, use some slugification (to fix non-ascii chars), and then reduce to [A-Za-z0-9_] whilst starting with [A-Za-z].

Copy link
Member

Choose a reason for hiding this comment

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

from: https://www.pcre.org/current/doc/html/pcre2pattern.html#SEC16

When PCRE2_UTF is not set, they may contain only ASCII alphanumeric characters and underscores, but must start with a non-digit.

I don't know if PCRE2_UTF is set in our implementation, but better safe than sorry I guess.

Copy link
Member Author

@filfreire filfreire May 15, 2023

Choose a reason for hiding this comment

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

Added slugification is done ✅

The PCRE2_UTF part not sure how to do it though, but should be fine as is.

Copy link
Member

Choose a reason for hiding this comment

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

what I meant mostly was removing accents etc from characters; eg. reduce èëé into eee

@filfreire filfreire force-pushed the feature/ins-2627-fti-5060-inso-cli-regex-problem branch from 6a9a206 to 2708d6c Compare May 15, 2023 10:35
@filfreire filfreire enabled auto-merge May 15, 2023 13:43
@filfreire filfreire added this pull request to the merge queue May 15, 2023
Merged via the queue into Kong:develop with commit 96b36d9 May 15, 2023
@filfreire filfreire deleted the feature/ins-2627-fti-5060-inso-cli-regex-problem branch May 15, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants