[TT-1944/TT-12550/TT-12865] Backport critical fixes - 2

jeffy-mathew commented 1 month ago

User description

JIRA: https://tyktech.atlassian.net/browse/TT-1944

Match used * allowing it to match 0 characters. Match now uses +, matching a minimum of 1 character.

Behaviour change:

Previously requests for /users/{id} would allow /users/ to pass without parameter. When replacing named parameter patterns, {} would be matched, which is invalid.

Follow up issue:

Mux can define a regex as {id:[0-9]+} or similar custom regex rules. The match does not respect this.

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

[ ] I ensured that the documentation is up to date
[ ] I explained why this PR updates go.mod in detail with reasoning why it's required
[ ] I would like a code coverage CI quality gate exception and have explained why

PR Type

Bug fix, Tests

Description

Fixed regex pattern in api_definition.go to ensure parameters match at least one character, preventing invalid matches.
Updated test cases in api_definition_test.go to align with the new regex matching logic, ensuring correct HTTP status responses.
Modified test configuration in .taskfiles/test.yml to run tests in single-threaded mode by adding -parallel 1.

Changes walkthrough 📝

Relevant files

Bug fix

api_definition.go

Fix regex pattern for parameter matching in API definitions

gateway/api_definition.go

Changed regex pattern to enforce at least one character match for
parameters.

Updated regex to replace mux named parameters with regex path match.

+7/-2

Tests

api_definition_test.go

Update test cases for revised regex matching logic

gateway/api_definition_test.go

Updated test cases to reflect new regex matching behavior.

Modified expected HTTP status codes for certain paths.

+4/-4

Configuration changes

test.yml

Configure test execution to run in single-threaded mode

.taskfiles/test.yml

Added -parallel 1 to test commands to ensure single-threaded
execution.

+2/-2

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Regex Pattern Change
The change from `([^/]*)` to `([^/]+)` in the regex pattern might affect existing URL matching behavior by now requiring at least one character. Ensure this change doesn't break existing functionalities. Test Case Adjustment
The test case changes reflect the new regex behavior, which forbids empty segments (e.g., changing expected status from `http.StatusOK` to `http.StatusForbidden` for paths ending with a slash). Verify that these changes are consistent with the intended API behavior.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Handle potential errors from regex compilation ___ Consider handling the error returned by `regexp.Compile` to avoid runtime panics if the regex compilation fails. [gateway/api_definition.go [832]](https://github.com/TykTechnologies/tyk/pull/6514/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR832-R832) ```diff -asRegex, _ := regexp.Compile(asRegexStr) +asRegex, err := regexp.Compile(asRegexStr) +if err != nil { + return nil, err +} ``` Suggestion importance[1-10]: 9 Why: Handling the error from `regexp.Compile` is crucial to prevent runtime panics, making this a significant improvement for robustness.	9
Possible bug	Prevent potential nil pointer dereference by checking if `whiteListPaths` is nil ___ Add a check to ensure `whiteListPaths` is not nil before appending to `combinedPath` to prevent potential nil pointer dereference. [gateway/api_definition.go [815]](https://github.com/TykTechnologies/tyk/pull/6514/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR815-R815) ```diff -combinedPath = append(combinedPath, whiteListPaths...) +if whiteListPaths != nil { + combinedPath = append(combinedPath, whiteListPaths...) +} ``` Suggestion importance[1-10]: 7 Why: Adding a nil check for `whiteListPaths` enhances the code's robustness by preventing potential runtime errors, though the likelihood of `whiteListPaths` being nil may be low.	7
Enhancement	Ensure regex matches empty strings for consistency ___ *Replace the regex pattern `([^/]+)` with `([^/])` to include matching of empty strings, ensuring consistency with previous behavior.** [gateway/api_definition.go [825]](https://github.com/TykTechnologies/tyk/pull/6514/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR825-R825) ```diff -asRegexStr := apiLangIDsRegex.ReplaceAllString(stringSpec, `([^/]+)`) +asRegexStr := apiLangIDsRegex.ReplaceAllString(stringSpec, `([^/]*)`) ``` Suggestion importance[1-10]: 8 Why: Changing the regex to match empty strings maintains consistency with previous behavior, which is important for ensuring existing functionality is preserved.	8
Maintainability	Improve code readability by using descriptive variable names ___ Consider using a more descriptive variable name than `apiLangIDsRegex` for clarity, such as `muxVariableRegex`. [gateway/api_definition.go [821]](https://github.com/TykTechnologies/tyk/pull/6514/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR821-R821) ```diff -var apiLangIDsRegex = regexp.MustCompile(`{([^}]+)}`) +var muxVariableRegex = regexp.MustCompile(`{([^}]+)}`) ``` Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name improves code readability and maintainability, though it is a minor improvement.	6