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

jeffy-mathew commented 2 months ago

User description

Description

Backport critical fixes from PRs in order https://github.com/TykTechnologies/tyk/pull/6480 https://github.com/TykTechnologies/tyk/pull/6437 https://github.com/TykTechnologies/tyk/pull/6475 https://github.com/TykTechnologies/tyk/pull/6506

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

Enhancement, Bug fix, Tests

Description

Introduced new configuration options for path prefix and suffix matching to enhance URL matching capabilities.
Refactored URLSpec to improve regex generation and logging for URL matching.
Enhanced GranularAccessMiddleware with improved URL matching logic and error handling.
Added utility functions for handling path regex preparation and matching, with caching for performance.
Updated and added test cases to verify new URL matching logic and configurations.
Added regression tests for issue 12865 to ensure stability and correctness of new features.

Changes walkthrough 📝

Relevant files

Enhancement

4 files

config.go `Introduce path prefix and suffix matching configuration` config/config.go Added new configuration options for path prefix and suffix matching. Enhanced documentation for new URL matching behavior.	+43/-0
api_definition.go `Refactor and enhance URLSpec for better URL matching` gateway/api_definition.go Refactored URLSpec to use private fields. Enhanced regex generation for URL matching. Improved logging for URL matching process.	+52/-36
mw_granular_access.go `Enhance URL matching and error handling in GranularAccessMiddleware` gateway/mw_granular_access.go Enhanced URL matching in GranularAccessMiddleware. Improved error handling and logging.	+79/-12
mux.go `Add utility functions for path regex handling` internal/httputil/mux.go Introduced utility functions for path regex preparation and matching. Implemented caching for path regex patterns.	+139/-0

Tests

4 files

api_definition_test.go `Update and add test cases for enhanced URL matching` gateway/api_definition_test.go Updated test cases to reflect changes in URL matching logic. Added new test scenarios for prefix and suffix matching.	+15/-33
mw_granular_access_test.go `Add tests for enhanced URL matching in GranularAccessMiddleware` gateway/mw_granular_access_test.go Added tests for new URL matching logic in GranularAccessMiddleware. Verified behavior with different path configurations.	+50/-24
mux_test.go `Add tests for path regex utility functions` internal/httputil/mux_test.go Added tests for new path regex utility functions. Verified regex preparation and matching logic.	+162/-0
issue_12865_test.go `Add regression tests for issue 12865` tests/regression/issue_12865_test.go Added regression tests for issue 12865. Tested various path matching configurations.	+171/-0

Configuration changes

1 files

schema.json

Update schema for new path matching options

cli/linter/schema.json - Updated schema to include new path matching configuration options.

+6/-0

💡 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 2 months ago

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Possible Bug
The method `generateRegex` has been refactored to use `httputil.PreparePathRegexp` for regex preparation. However, there's no error handling for the regex compilation which might lead to runtime panics if the regex is invalid. Consider adding error handling after the regex compilation. Code Improvement
The method `ProcessRequest` in `GranularAccessMiddleware` has a complex conditional structure for URL matching which could be simplified or broken down into smaller methods for better readability and maintainability. Error Handling
The function `MatchPaths` in `mux.go` does not handle errors effectively. Errors from `MatchPath` are collected but not returned if a match is found, potentially swallowing significant errors that should be addressed.

github-actions[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Modify the loop to correctly remove items from a slice during iteration ___ The loop that removes a specific transfer encoding from `response.TransferEncoding` may cause incorrect results if there are multiple encodings that match `victim`. Use a slice to collect indices to remove and then remove them in a separate step to avoid altering the slice during iteration. [internal/httputil/response.go [29-31]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-5ff81ef286affaddab506e96d00756dede0d59b287101ebe5a3de5a5b8a489beR29-R31) ```diff +var toRemove []int for i, value := range response.TransferEncoding { if value == victim { - response.TransferEncoding = append(response.TransferEncoding[:i], response.TransferEncoding[i+1:]...) + toRemove = append(toRemove, i) } } +for _, i := range toRemove { + response.TransferEncoding = append(response.TransferEncoding[:i], response.TransferEncoding[i+1:]...) +} ``` Suggestion importance[1-10]: 10 Why: The suggestion addresses a potential bug where modifying a slice during iteration can lead to incorrect behavior. Collecting indices first and removing them later is a robust solution.	10
	Add error handling for regex compilation to prevent runtime errors ___ Add error handling for the `regexp.Compile` function to prevent runtime panics if the regex pattern is invalid. [gateway/api_definition.go [838-839]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR838-R839) ```diff asRegex, err := regexp.Compile(pattern) +if err != nil { + log.WithError(err).Error("Failed to compile regex pattern") + return +} log.WithError(err).Debugf("URLSpec: %s => %s type=%d", stringSpec, pattern, specType) ``` Suggestion importance[1-10]: 9 Why: Adding error handling for the `regexp.Compile` function is crucial to prevent runtime panics, ensuring the application remains stable even if an invalid regex pattern is encountered.	9
	Correct the regular expression to properly escape special characters ___ *The regular expression pattern `"/users.": "^/users."` in the test map does not correctly escape the asterisk (``), which is a special character in regex. It should be escaped to ensure it is treated as a literal character, not as a quantifier.** [internal/httputil/mux_test.go [22]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-8f7ce1891e221d7adb9e68f2e951f33edfbde2128187abb6e837ac01952d7888R22-R22) ```diff -"/users.": "^/users.", +"/users\\.": "^/users\\.", ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential bug where the asterisk is not escaped, which could lead to incorrect regex behavior. Escaping the asterisk ensures the regex matches the intended pattern.	9
Possible issue	Ensure mutual exclusivity between `EnablePathPrefixMatching` and `EnablePathSuffixMatching` to avoid conflicts ___ Ensure that the `EnablePathPrefixMatching` and `EnablePathSuffixMatching` options are mutually exclusive to prevent conflicts in URL matching logic. [config/config.go [433-452]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-fe44f09c4d5977b5f5eaea29170b6a0748819c9d02271746a20d81a5f3efca17R433-R452) ```diff EnablePathPrefixMatching bool `json:"enable_path_prefix_matching"` EnablePathSuffixMatching bool `json:"enable_path_suffix_matching"` +// Ensure that both prefix and suffix matching cannot be enabled simultaneously +if EnablePathPrefixMatching && EnablePathSuffixMatching { + log.Fatal("Cannot enable both prefix and suffix matching") +} ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential logical conflict in URL matching by ensuring that both prefix and suffix matching cannot be enabled simultaneously, which could lead to ambiguous behavior.	8
Enhancement	Add an end anchor to the regex to limit matches to the exact pattern ___ The test case for `"/users/{id}": "^/users/([^/]+)"` should include the end anchor `$` to ensure that the regex does not match additional unwanted characters beyond the intended pattern. [internal/httputil/mux_test.go [28]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-8f7ce1891e221d7adb9e68f2e951f33edfbde2128187abb6e837ac01952d7888R28-R28) ```diff -"/users/{id}": "^/users/([^/]+)", +"/users/{id}": "^/users/([^/]+)$", ``` Suggestion importance[1-10]: 8 Why: Adding an end anchor to the regex is a good enhancement that ensures the pattern matches only the intended string, preventing unintended matches.	8
	Use `strings.ReplaceAll` for replacing all instances in a string ___ Replace the `strings.Replace` method with `strings.ReplaceAll` for clarity and to ensure all instances are replaced. [gateway/api_definition.go [1809]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR1809-R1809) ```diff -return strings.Replace(reqPath, "/"+part+"/", "/", 1) +return strings.ReplaceAll(reqPath, "/"+part+"/", "/", 1) ``` Suggestion importance[1-10]: 7 Why: Using `strings.ReplaceAll` improves code clarity and ensures that all instances of the substring are replaced, although in this context, the original `strings.Replace` with a count of 1 was already correct.	7
	Improve the clarity of the test assertion ___ The test `TestIsUpgrade` uses `assert.Empty(t, upgradeType)` which might not clearly convey the intention of the test. It's better to use `assert.Equal(t, "",` `upgradeType)` for clarity. [internal/httputil/streaming_test.go [63]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-fc336fa4b4fb0ed97489c9f588772e0fb0225883339ff49000f26a4324ef99a0R63-R63) ```diff -assert.Empty(t, upgradeType) +assert.Equal(t, "", upgradeType) ``` Suggestion importance[1-10]: 6 Why: The suggestion improves code readability by making the test assertion more explicit, which is beneficial for understanding the test's intention.	6
Maintainability	Consolidate error handling in `GranularAccessMiddleware` for clarity and maintainability ___ Refactor the error handling in the `GranularAccessMiddleware` to consolidate the logging and error response into a single function. [gateway/mw_granular_access.go [80-83]](https://github.com/TykTechnologies/tyk/pull/6513/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R80-R83) ```diff -if err != nil \|\| !match { - continue +if err != nil { + logger.WithError(err).Error("Regex match failed") + return m.block(logger) +} +if !match { + return m.block(logger) } return m.pass() ``` Suggestion importance[1-10]: 8 Why: This refactoring improves code maintainability by consolidating error handling and logging, making the code easier to read and reducing redundancy.	8