Closed titpetric closed 1 month ago
API Changes
--- prev.txt 2024-09-12 19:01:01.859261628 +0000
+++ current.txt 2024-09-12 19:00:58.683242013 +0000
@@ -5512,6 +5512,49 @@
// Regular expressions and parameterized routes will be left alone regardless of this setting.
EnableStrictRoutes bool `json:"enable_strict_routes"`
+ // EnablePathPrefixMatching changes the URL matching from wildcard mode to prefix mode.
+ // For example, `/json` matches `*/json*` by current default behaviour.
+ // If prefix matching is enabled, the match will be performed as a prefix match (`/json*`).
+ //
+ // The `/json` url would be matched as `^/json` against the following paths:
+ //
+ // - Full listen path and versioning URL (`/listen-path/v4/json`)
+ // - Stripped listen path URL (`/v4/json`)
+ // - Stripped version information (`/json`) - match.
+ //
+ // If versioning is disabled then the following URLs are considered:
+ //
+ // - Full listen path and endpoint (`/listen-path/json`)
+ // - Stripped listen path (`/json`) - match.
+ //
+ // For inputs that start with `/`, a prefix match is ensured by
+ // prepending the start of string `^` caret.
+ //
+ // For all other cases, the pattern remains unmodified.
+ //
+ // Combine this option with `enable_path_suffix_matching` to achieve
+ // exact url matching with `/json` being evaluated as `^/json$`.
+ EnablePathPrefixMatching bool `json:"enable_path_prefix_matching"`
+
+ // EnablePathSuffixMatching changes the URL matching to match as a suffix.
+ // For example: `/json` is matched as `/json$` against the following paths:
+ //
+ // - Full listen path and versioning URL (`/listen-path/v4/json`)
+ // - Stripped listen path URL (`/v4/json`)
+ // - Stripped version information (`/json`) - match.
+ //
+ // If versioning is disabled then the following URLs are considered:
+ //
+ // - Full listen path and endpoint (`/listen-path/json`)
+ // - Stripped listen path (`/json`) - match.
+ //
+ // If the input pattern already ends with a `$` (`/json$`)
+ // then the pattern remains unmodified.
+ //
+ // Combine this option with `enable_path_prefix_matching` to achieve
+ // exact url matching with `/json` being evaluated as `^/json$`.
+ EnablePathSuffixMatching bool `json:"enable_path_suffix_matching"`
+
// Disable TLS verification. Required if you are using self-signed certificates.
SSLInsecureSkipVerify bool `json:"ssl_insecure_skip_verify"`
@@ -7422,10 +7465,16 @@
func CloneAPI(a *APISpec) *APISpec
func (a *APISpec) CheckSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (bool, interface{})
- CheckSpecMatchesStatus checks if a url spec has a specific status
+ CheckSpecMatchesStatus checks if a URL spec has a specific status.
+ Deprecated: The function doesn't follow go return conventions (T, ok);
+ use FindSpecMatchesStatus;
func (a *APISpec) Expired() bool
+func (a *APISpec) FindSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (*URLSpec, bool)
+ FindSpecMatchesStatus checks if a URL spec has a specific status and returns
+ the URLSpec for it.
+
func (s *APISpec) FireEvent(name apidef.TykEvent, meta interface{})
func (a *APISpec) GetSessionLifetimeRespectsKeyExpiration() bool
@@ -7447,7 +7496,13 @@
func (a *APISpec) StopSessionManagerPool()
-func (a *APISpec) StripListenPath(r *http.Request, path string) string
+func (a *APISpec) StripListenPath(reqPath string) string
+ StripListenPath will strip the listen path from the URL, keeping version in
+ tact.
+
+func (a *APISpec) StripVersionPath(reqPath string) string
+ StripVersionPath will strip the version from the URL. The input URL should
+ already have listen path stripped.
func (a *APISpec) URLAllowedAndIgnored(r *http.Request, rxPaths []URLSpec, whiteListStatus bool) (RequestStatus, interface{})
URLAllowedAndIgnored checks if a url is allowed and ignored.
@@ -10066,7 +10121,6 @@
system, return an error to have the chain fail
type URLSpec struct {
- Spec *regexp.Regexp
Status URLStatus
MethodActions map[string]apidef.EndpointMethodMeta
Whitelist apidef.EndPointMeta
@@ -10094,6 +10148,7 @@
PersistGraphQL apidef.PersistGraphQLMeta
IgnoreCase bool
+ // Has unexported fields.
}
URLSpec represents a flattened specification for URLs, used to check if
a proxy URL path is on any of the white, black or ignored lists. This is
@@ -11424,6 +11479,9 @@
func Cert(domain string) tls.Certificate
Generate cert
+func Exclusive(t *testing.T)
+ Exclusive uses a lock to gate only a single test running.
+
func Flaky(t *testing.T, fake ...func() (bool, func(...interface{})))
Flaky skips a flaky test in a CI environment
โฑ๏ธ Estimated effort to review: 3 ๐ต๐ต๐ตโชโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Key issues to review Code Refactor The `GranularAccessMiddleware` has been significantly refactored to support new matching configurations (`EnablePathPrefixMatching` and `EnablePathSuffixMatching`). Ensure that the new logic correctly handles URL path matching according to these settings, especially the combination of prefix and suffix matching which aims to achieve exact URL matching. New Utility Functions New utility functions `PreparePathRegexp`, `StripListenPath`, and `MatchPaths` have been added to handle URL path manipulations and matching. Review these for correct implementation of regex patterns and ensure they handle edge cases, especially in URL transformations and matching logic. Refactoring The API definition loading and URL spec compilation logic have been refactored. Review the changes to ensure that the new methods correctly compile and handle URL specs, particularly the changes involving the handling of virtual paths, Go plugins, and tracked endpoints. |
Category | Suggestion | Score |
Possible bug |
Ensure the
___
**The method | 10 |
Correct the method call to include all required parameters___ **Ensure that theStripListenPath method is called with the correct number of parameters. The original method seems to require two parameters, but only one is provided in the new code.** [gateway/mw_request_signing.go [86]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-4930d7e23ba3866e7a220c1eacff5701bc59b3029b47226c55432db0615fb55aR86-R86) ```diff if s.Spec.Proxy.StripListenPath { - path = s.Spec.StripListenPath(path) + path = s.Spec.StripListenPath(r, path) // Assuming the original method requires the request object and path } ``` Suggestion importance[1-10]: 10Why: The suggestion correctly identifies a potential bug where the `StripListenPath` method is called with an incorrect number of parameters, which could lead to runtime errors. | 10 | |
Add error handling after regex compilation to ensure stability___ **Ensure error handling after compiling the regex to prevent runtime panics orunexpected behavior if the regex compilation fails.** [gateway/api_definition.go [835-836]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR835-R836) ```diff asRegex, err := regexp.Compile(pattern) -log.WithError(err).Debugf("URLSpec: %s => %s type=%d", stringSpec, pattern, specType) +if err != nil { + log.WithError(err).Error("Failed to compile regex") + return +} +log.Debugf("URLSpec: %s => %s type=%d", stringSpec, pattern, specType) ``` Suggestion importance[1-10]: 9Why: The suggestion correctly adds error handling after regex compilation, which is crucial to prevent runtime panics or unexpected behavior if the regex compilation fails. This addresses a potential bug and improves the robustness of the code. | 9 | |
Best practice |
Improve error handling for regex compilation to ensure continued pattern checking___ **The error handling for regex compilation inGranularAccessMiddleware should be improved to ensure that the loop continues to the next pattern if the current one fails to compile, rather than blocking access immediately.** [gateway/mw_granular_access.go [100-109]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R100-R109) ```diff +asRegex, err := regexp.Compile(pattern) +if err != nil { + logger.WithError(err).Error("error compiling regex") + continue +} - ``` Suggestion importance[1-10]: 9Why: The suggestion addresses a best practice by ensuring that regex compilation errors do not block access immediately, allowing the loop to continue checking other patterns. This is important for robust error handling. | 9 |
Performance |
Cache compiled regex patterns to enhance performance___ **To improve the performance and reduce the overhead of regex operations, considercaching compiled regex patterns instead of compiling them every time in the MatchPath function.**
[internal/httputil/mux.go [113-118]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-3d9ee5f5e946d72e6f2ae662ff03ee5253bbdc15203d2e4f6e9f46c13011ebf8R113-R118)
```diff
-asRegex, err := regexp.Compile(pattern)
-if err != nil {
- return false, err
+// Use a global cache to store compiled regex patterns
+var regexCache = make(map[string]*regexp.Regexp)
+asRegex, found := regexCache[pattern]
+if !found {
+ var err error
+ asRegex, err = regexp.Compile(pattern)
+ if err != nil {
+ return false, err
+ }
+ regexCache[pattern] = asRegex
}
return asRegex.MatchString(endpoint), nil
```
Suggestion importance[1-10]: 9Why: Caching compiled regex patterns can significantly enhance performance by reducing the overhead of repeated compilations, especially in high-frequency operations like URL matching. | 9 |
Enhancement |
Use error logging for regex compilation failures to improve error visibility___ **Use a more specific log level thanDebug for regex compilation errors to ensure visibility in production environments.** [gateway/api_definition.go [836]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR836-R836) ```diff -log.WithError(err).Debugf("URLSpec: %s => %s type=%d", stringSpec, pattern, specType) +if err != nil { + log.WithError(err).Errorf("Failed to compile regex for URLSpec: %s", stringSpec) +} else { + log.Debugf("URLSpec: %s => %s type=%d", stringSpec, pattern, specType) +} ``` Suggestion importance[1-10]: 8Why: Changing the log level to `Error` for regex compilation failures is a good enhancement for production environments, as it ensures that such critical issues are not overlooked. | 8 |
Handle cases where
___
**The | 8 | |
Possible issue |
Validate
___
**Consider validating | 8 |
Security |
Improve security by using more restrictive regex patterns in URL matching___ **Consider using a more specific regex pattern to avoid potential security risksassociated with overly permissive patterns. For example, change the wildcard replacement from .* to a more restrictive pattern.**
[internal/httputil/mux.go [42-45]](https://github.com/TykTechnologies/tyk/pull/6510/files#diff-3d9ee5f5e946d72e6f2ae662ff03ee5253bbdc15203d2e4f6e9f46c13011ebf8R42-R45)
```diff
if strings.Contains(pattern, "/*") {
pattern = strings.ReplaceAll(pattern, "/*/", "/[^/]+/")
- pattern = strings.ReplaceAll(pattern, "/*", "/.*")
+ pattern = strings.ReplaceAll(pattern, "/*", "/[^/]*") // Change to match any character except '/'
}
```
Suggestion importance[1-10]: 8Why: The suggestion improves security by recommending a more restrictive regex pattern, reducing the risk of unintended matches, which is crucial for URL matching. | 8 |
Maintainability |
Refactor
___
**Refactor the | 7 |
Improve logging clarity and reduce redundancy in
___
**Refactor the logging within the | 6 | |
Refactor repeated code blocks into a loop for setting
___
**Consider using a loop to avoid code duplication when setting up | 5 |
Failed conditions
78.9% Coverage on New Code (required โฅ 80%)
C Reliability Rating on New Code (required โฅ A)
See analysis details on SonarCloud
Catch issues before they fail your Quality Gate with our IDE extension SonarLint
User description
TT-12865 Rename config parameter, update usage, support mux params on legacy (#6506)
enhancement
EnablePrefixMatching
andEnableSuffixMatching
toEnablePathPrefixMatching
andEnablePathSuffixMatching
respectively, across multiple files.config.go
Rename configuration fields for path matching options
config/config.go
EnablePrefixMatching
toEnablePathPrefixMatching
.EnableSuffixMatching
toEnablePathSuffixMatching
.api_definition.go
Update API definition to use new path matching config
gateway/api_definition.go
mw_granular_access.go
Refactor middleware to use updated path matching config
gateway/mw_granular_access.go
PreparePathRegexp
.session_manager.go
Update session manager for new path matching config
gateway/session_manager.go
schema.json
Update JSON schema for path matching configuration
cli/linter/schema.json - Renamed JSON schema fields for path matching options.
api_definition_test.go
Update test configuration for path prefix matching
gateway/api_definition_test.go - Modified test configuration to use `EnablePathPrefixMatching`.
mw_granular_access_test.go
Update middleware test for path prefix matching
gateway/mw_granular_access_test.go - Modified test configuration to use `EnablePathPrefixMatching`.
TT-12865: https://tyktech.atlassian.net/browse/TT-12865?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Enhancement, Tests
EnablePathPrefixMatching
andEnablePathSuffixMatching
to enhance URL path matching capabilities.6 files
config.go
Add configuration for path prefix and suffix matching
config/config.go
EnablePathPrefixMatching
andEnablePathSuffixMatching
.api_definition.go
Refactor URLSpec and update path matching logic
gateway/api_definition.go
model_apispec.go
Refactor and deprecate CheckSpecMatchesStatus
gateway/model_apispec.go
CheckSpecMatchesStatus
to a separate file and deprecated it.FindSpecMatchesStatus
for improved status checking.model_urlspec.go
Add path and method matching methods to URLSpec
gateway/model_urlspec.go
modeSpecificSpec
method.mw_granular_access.go
Update GranularAccessMiddleware for new path matching
gateway/mw_granular_access.go
mux.go
Refactor path regex preparation and matching
internal/httputil/mux.go
4 files
api_definition_test.go
Update and add test cases for path matching logic
gateway/api_definition_test.go
mw_granular_access_test.go
Add tests for GranularAccessMiddleware path matching
gateway/mw_granular_access_test.go
mux_test.go
Add tests for path regex preparation and matching
internal/httputil/mux_test.go
issue_12865_test.go
Add regression tests for issue 12865
tests/regression/issue_12865_test.go
2 files
test.yml
Update test task configuration
.taskfiles/test.yml
schema.json
Update schema for new path matching configuration
cli/linter/schema.json - Added new schema fields for path prefix and suffix matching.
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
EnablePathPrefixMatching
andEnablePathSuffixMatching
to enhance URL path matching capabilities.Changes walkthrough ๐
5 files
config.go
Add new configuration fields for path matching
config/config.go
EnablePathPrefixMatching
andEnablePathSuffixMatching
.configurations.
api_definition.go
Refactor URLSpec and enhance path matching logic
gateway/api_definition.go
mw_granular_access.go
Enhance GranularAccessMiddleware with path matching options
gateway/mw_granular_access.go
options.
mux.go
Add utility functions for path regex handling
internal/httputil/mux.go
strings.go
Implement concurrency-safe string map
internal/maps/strings.go
5 files
api_definition_test.go
Update and add tests for path matching configurations
gateway/api_definition_test.go
mw_granular_access_test.go
Add tests for GranularAccessMiddleware path matching
gateway/mw_granular_access_test.go
configurations.
mux_test.go
Add tests for path regex utilities
internal/httputil/mux_test.go
issue_12865_test.go
Add regression tests for issue 12865
tests/regression/issue_12865_test.go
regression_test.go
Refactor and test API spec loading utilities
tests/regression/regression_test.go
replacement.