Mock response seems to be not executed when calling non-base version of OAS API #7254
Conversation
|
Let's make that PR title a 💯 shall we? 💪 Your PR title and story title look slightly different. Just checking in to know if it was intentional!
Check out this guide to learn more about PR best-practices. |
PR Reviewer Guide 🔍(Review updated until commit 848b2db)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 848b2db
Previous suggestionsSuggestions up to commit 7c76968
|
|
API Changes no api changes detected |
PR Review: OAS API Versions Path Handling FixOverviewThis PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic for versioned APIs. Technical AnalysisThe change modifies how request paths are handled for non-base OAS API versions: -v.Spec.SanitizeProxyPaths(r)
+new_listen_path := v.Spec.Proxy.ListenPath[:len(v.Spec.Proxy.ListenPath)-1] + "-" + targetVersion
+r.URL.Path = new_listen_path + v.Spec.StripListenPath(r.URL.Path)What the fix does:
How this resolves the issue:When a request is made to a non-base version of an OAS API, the handler now receives the correct path with proper version information, allowing it to correctly execute mock responses and other version-specific functionality. Potential Concerns
RecommendationThe fix correctly addresses the issue with mock responses not executing for non-base OAS API versions. However, I recommend adding validation to prevent potential panics with empty paths and ensuring proper slash handling in path concatenation. Tip: Mention me again using |
Security Review: OAS API Versions Path Handling FixOverviewThis PR fixes an issue where mock responses were not being executed when calling non-base versions of OAS API. The change modifies how request paths are handled for versioned APIs to ensure proper routing. Key ChangesThe PR makes a targeted change to
- v.Spec.SanitizeProxyPaths(r)
+ new_listen_path := v.Spec.Proxy.ListenPath[:len(v.Spec.Proxy.ListenPath)-1] + "-" + targetVersion
+ r.URL.Path = new_listen_path + v.Spec.StripListenPath(r.URL.Path)Security AnalysisPotential Issues
Mitigating Factors
Recommendations
ConclusionThis PR appears to be a functional fix with minimal security implications. The changes are focused on ensuring correct internal routing for versioned APIs, particularly for mock responses. The potential issues identified are edge cases rather than security vulnerabilities. While there are some minor improvements that could be made for robustness, the PR as implemented should resolve the reported issue without introducing significant security concerns. Tip: Mention me again using |
Dependency Impact Analysis: OAS API Versions Mock Response FixIssue OverviewThe PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. This was happening because the path handling for versioned APIs was incorrect, preventing the proper invocation of mock response handlers. Code Changes AnalysisThe fix makes two critical changes to the
Dependency ImpactDirect Dependencies
Potential Risks
Affected Functionality
ConclusionThe change is focused and addresses the specific issue of mock responses not executing for non-base OAS API versions. The implementation correctly modifies the request path to include the version suffix and uses the appropriate internal handler. While there are some potential edge cases around empty paths and slash handling, the fix is generally sound and should resolve the reported issue without introducing significant new risks. The PR successfully ensures that mock responses work consistently across all API versions. Tip: Mention me again using |
PR Review: Fix for Mock Response Execution in Non-Base OAS API VersionsIssue SummaryThe PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. This occurred because the path handling for versioned APIs was not correctly managing the listen path, which prevented the proper handler from being invoked for mock responses. Root Cause AnalysisWhen a request is made to a non-base version of an OAS API, the
Solution ImplementedThe PR replaces the previous path handling with a more explicit approach: // Old code
v.Spec.SanitizeProxyPaths(r)
// New code
new_listen_path := v.Spec.Proxy.ListenPath[:len(v.Spec.Proxy.ListenPath)-1] + "-" + targetVersion
r.URL.Path = new_listen_path + v.Spec.StripListenPath(r.URL.Path)This change:
This ensures that the request is forwarded to the correct handler with the proper path structure, allowing mock responses to be correctly executed for non-base API versions. Technical AssessmentStrengths
Potential Concerns
Testing ConsiderationsThe PR should be tested with:
ConclusionThis PR effectively fixes the issue with mock responses not executing for non-base OAS API versions by correctly handling the path transformation for versioned APIs. The implementation is straightforward and aligns with the existing codebase architecture, though some additional validation could be added to handle edge cases more robustly. Tip: Mention me again using |
📦 Impact Review Snapshot
## Impact AssessmentThis PR fixes a bug where mock responses weren't executing for non-base versions of OAS APIs by modifying the internal path handling logic. The change is isolated to the gateway's internal request processing and doesn't alter any API schemas, protocols, or interfaces. The fix properly constructs versioned paths for OAS APIs by appending the version suffix to the listen path, rather than using the generic path sanitization method that was causing the issue. ## Required UpdatesNo updates are required in downstream repositories:
## Compatibility ConcernsThe change is backward compatible and doesn't introduce any breaking changes:
## Summary & Recommendations
Tip: Mention me again using |
Performance Impact Analysis of OAS API Versioning Fix🚀 Performance Snapshot
## Performance Impact AnalysisThe PR addresses a bug where mock responses weren't executing for non-base versions of OAS APIs by modifying path handling in the From a performance perspective, this change is minimal and focused on a specific code path that only executes when handling versioned OAS APIs. The new code performs simple string operations (substring, concatenation) which are efficient and have predictable performance characteristics. The change doesn't introduce any new loops, database calls, or expensive operations that would significantly impact request processing time. ## Critical AreasThe modified code is in the request processing path, which is performance-sensitive, but the changes are lightweight:
## Optimization RecommendationsWhile the current implementation is efficient for most cases, there are a few potential improvements:
## Summary
Tip: Mention me again using |
🛡️ Security Snapshot
## Security Impact AnalysisThe PR addresses a bug where mock responses weren't executing for non-base versions of OAS APIs. The fix modifies the path handling logic in The new code creates a version-specific listen path by removing the trailing slash from the base path and appending the version identifier. This approach is more direct than the previous method using ## Identified VulnerabilitiesMedium Risk:
Low Risk:
## Security Recommendations
## OWASP ComplianceThis change doesn't directly impact OWASP Top 10 categories, but relates to:
## Summary
Tip: Mention me again using |
🚦 Connectivity Review Snapshot
## Connectivity Assessment
## Test Coverage Validation
## Security & Performance Impact
## Summary & Recommendations
Tip: Mention me again using |
Analysis of PR #7254: OAS API Versions Path Handling FixIssue OverviewThis PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic for versioned APIs, which prevented the correct operation from being found in the OAS definition. Technical AnalysisRoot CauseWhen a request is made to a non-base version of an OAS API, the SolutionThe PR modifies the path handling logic for versioned OAS APIs: // Before
v.Spec.SanitizeProxyPaths(r)
// After
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_pathThis change ensures that:
By fixing the path handling, the Impact on Downstream RepositoriesThis PR has no impact on downstream repositories:
Potential Concerns
ConclusionThis PR is a focused bug fix that addresses a specific issue with path handling for versioned OAS APIs. It ensures that mock responses work correctly for non-base versions of OAS APIs without affecting external interfaces or downstream repositories. The implementation is straightforward and uses standard Go libraries for path handling, making it a safe and effective solution. Tip: Mention me again using |
Analysis of PR #7254: OAS API Versions Path Handling FixThis PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic for versioned APIs. Core ChangesThe main fix is in // Before
v.Spec.SanitizeProxyPaths(r)
// After
if v.Spec.IsOAS {
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_path
} else {
v.Spec.SanitizeProxyPaths(r)
}How This Fixes the IssueThe problem was that for non-base versions of OAS APIs, the request path wasn't being properly constructed to include the version information. This caused the mock response handlers to not be triggered correctly. The fix:
This ensures that the request is properly routed to the correct handler for the versioned API, allowing mock responses to be executed correctly. Potential Concerns
Overall, this is a focused fix that addresses the specific issue without introducing significant new complexity. Tip: Mention me again using |
Analysis of OAS API Versions Mock Response FixIssue SummaryThe PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. This occurred because the path handling for versioned APIs wasn't correctly managing the listen path, which prevented the proper handler from being invoked for mock responses. Root CauseWhen a request is made to a non-base version of an OAS API, the
SolutionThe PR implements a conditional approach to path handling based on API type: if v.Spec.IsOAS {
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_path
} else {
v.Spec.SanitizeProxyPaths(r)
}This change:
Technical ImpactThe fix ensures that when a request is made to a non-base version of an OAS API:
This is a targeted fix that addresses a specific issue without introducing new functionality or changing existing behavior. It simply ensures that the existing functionality works correctly for all API versions. Tip: Mention me again using |
Understanding the OAS API Versioning FixThe IssueThe PR addresses a bug where mock responses weren't being executed when calling non-base versions of OAS APIs. This occurred because of how path handling was implemented for versioned APIs in the Root Cause AnalysisWhen a request is made to a non-base version of an OAS API, the following sequence occurs:
The issue was in step 3. Previously, all APIs (both classic and OAS) used Looking at the func (m *mockResponseMiddleware) mockResponse(r *http.Request) (*http.Response, error) {
// if response is nil we go further
operation := m.Spec.findOperation(r)
if operation == nil {
return nil, nil
}
mockResponse := operation.MockResponse
if mockResponse == nil || !mockResponse.Enabled {
return nil, nil
}
// ... serve mock response
}The key issue is that The FixThe PR modifies the if v.Spec.IsOAS {
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_path
} else {
v.Spec.SanitizeProxyPaths(r)
}This change:
Why This Fixes the IssueThe fix works because:
The key insight is that OAS APIs need special path handling for versioned endpoints to ensure the correct operation is found, which is essential for features like mock responses to work properly. Technical DetailsThe func (a *APISpec) findOperation(r *http.Request) *Operation {
middleware := a.OAS.GetTykMiddleware()
if middleware == nil {
return nil
}
if a.oasRouter == nil {
log.Warningf("OAS router not initialized properly. Unable to find route for %s %v", r.Method, r.URL)
return nil
}
rClone := *r
rClone.URL = ctxGetInternalRedirectTarget(r)
route, pathParams, err := a.oasRouter.FindRoute(&rClone)
if err != nil {
// Log and return nil
return nil
}
operation, ok := middleware.Operations[route.Operation.OperationID]
if !ok {
log.Warningf("No operation found for ID: %s", route.Operation.OperationID)
return nil
}
return &Operation{
Operation: operation,
route: route,
pathParams: pathParams,
}
}With the corrected path handling, this function can now successfully find the operation for versioned APIs, allowing mock responses to be executed correctly. Tip: Mention me again using |
radkrawczyk
force-pushed
the
TT-15301-Mock-response-seems-to-be-not-executed-when-calling-non-base-version-of-OAS-API
branch
from
9b36cfa to
848b2db
Compare
🚀 Performance Snapshot
## Performance Impact AnalysisThe PR modifies how request paths are handled for non-base versions of OAS APIs. The change replaces a simple string manipulation approach with a more robust path handling implementation using ## Critical AreasThe changes touch the request processing middleware which is in the hot path, but the modifications are lightweight:
## Optimization RecommendationsThe implementation is already well-optimized for the specific use case:
## Summary
Tip: Mention me again using |
Analysis of PR #7254: OAS API Versions Path Handling FixIssue OverviewThis PR addresses a bug (TT-15301) where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic for versioned APIs, which prevented the correct operation from being found in the OAS definition. Root Cause AnalysisWhen a request is made to a non-base version of an OAS API, the Previously, the code was using a simplistic approach to construct the versioned path: new_listen_path := v.Spec.Proxy.ListenPath[:len(v.Spec.Proxy.ListenPath)-1] + "-" + targetVersion
r.URL.Path = new_listen_path + v.Spec.StripListenPath(r.URL.Path)This approach didn't correctly handle the path for OAS APIs with version suffixes, causing the Solution ImplementationThe PR implements a more robust approach to path handling for OAS APIs: if v.Spec.IsOAS {
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_path
} else {
v.Spec.SanitizeProxyPaths(r)
}This change:
Benefits of the Fix
The fix is minimal and focused on the specific issue without introducing new functionality or changing existing behavior. It simply ensures that the existing functionality works correctly for all API versions. Tip: Mention me again using |
🛡️ Security Snapshot
## Security Impact AnalysisThis PR fixes a bug where mock responses weren't being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic in the ## Identified VulnerabilitiesNo significant vulnerabilities were introduced by this change. The PR actually improves security by:
## Security RecommendationsThe implementation is secure as it uses standard library functions for path manipulation and includes proper error handling. No additional security measures are needed for this specific change. ## OWASP ComplianceThis change doesn't directly impact OWASP Top 10 categories. It's a functional fix for internal path handling that doesn't affect authentication, authorization, or input validation mechanisms. The use of ## Summary
Tip: Mention me again using |
Understanding the OAS API Versioning Mock Response FixThe IssueThe PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. This occurred because of how path handling was implemented in the Root Cause AnalysisWhen a request is made to a non-base version of an OAS API, the following sequence occurs:
The issue was in step 3. Before the fix, the code was using a generic approach for all API types: new_listen_path := v.Spec.Proxy.ListenPath[:len(v.Spec.Proxy.ListenPath)-1] + "-" + targetVersion
r.URL.Path = new_listen_path + v.Spec.StripListenPath(r.URL.Path)This approach didn't work correctly for OAS APIs because:
The FixThe PR introduces a conditional path handling approach based on whether the API is an OAS API or not: if v.Spec.IsOAS {
target_api := v.Gw.apisByID[subVersionID]
stripped_path := v.Spec.StripListenPath(r.URL.Path)
new_path, err := url.JoinPath(target_api.Proxy.ListenPath, stripped_path)
if err != nil {
log.Errorf("Url Join Error, err: %s", err)
return errors.New(http.StatusText(http.StatusInternalServerError)), http.StatusInternalServerError
}
r.URL.Path = new_path
} else {
v.Spec.SanitizeProxyPaths(r)
}This fix:
The key improvement is that for OAS APIs, the path is now constructed in a way that the OAS router can correctly match it to an operation in the OAS definition. This allows the Why This WorksThe fix works because:
The use of Technical ImpactThis change only affects the internal path handling for versioned OAS APIs and doesn't modify any external interfaces or behaviors. It's a targeted fix that ensures existing functionality (mock responses) works correctly for all API versions. Tip: Mention me again using |
|
Persistent review updated to latest commit 848b2db |
🚀 Performance Snapshot
## Performance Impact AnalysisThe PR fixes a bug where mock responses weren't executing for non-base versions of OAS APIs by modifying path handling in the ## Critical AreasThe changes touch the request processing middleware which is in the hot path, but the modifications are lightweight:
## Optimization RecommendationsThe implementation is already well-optimized for the specific use case:
## Summary
Tip: Mention me again using |
📦 Impact Review Snapshot
Impact AssessmentThis PR fixes a bug where mock responses weren't executing for non-base versions of OAS APIs by modifying the internal path handling logic. The change is isolated to the gateway's internal request processing and doesn't alter any API schemas, protocols, or interfaces. The fix properly constructs versioned paths for OAS APIs by appending the version suffix to the listen path, rather than using the generic path sanitization method that was causing the issue. Required UpdatesNo updates are required in downstream repositories:
Compatibility ConcernsThe change is backward compatible and doesn't introduce any breaking changes:
Summary & Recommendations
Tip: Mention me again using |
🚀 Performance Snapshot
## Performance Impact AnalysisThis PR fixes a bug where mock responses weren't executing for non-base versions of OAS APIs by modifying path handling in the ## Critical AreasThe changes touch the request processing middleware which is in the hot path, but the modifications are lightweight:
## Optimization RecommendationsThe implementation is already well-optimized for the specific use case:
## Summary
Tip: Mention me again using |
Analysis of PR #7254: OAS API Versions Path Handling FixIssue OverviewThis PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic in the Root Cause AnalysisWhen a request is made to a non-base version of an API, the The problem was that the middleware was using the same path handling logic for both OAS and non-OAS APIs: v.Spec.SanitizeProxyPaths(r)This approach worked fine for classic APIs but didn't correctly handle the path for OAS APIs with version suffixes, causing the The FixThe PR adds a conditional check to handle OAS APIs differently: if v.Spec.IsOAS {
targetApi, ok := v.Gw.apisByID[subVersionID]
if !ok {
return errors.New(string(VersionDoesNotExist)), http.StatusNotFound
}
strippedPath := v.Spec.StripListenPath(r.URL.Path)
newPath, err := url.JoinPath(targetApi.Proxy.ListenPath, strippedPath)
if err != nil {
return err, http.StatusInternalServerError
}
r.URL.Path = newPath
} else {
v.Spec.SanitizeProxyPaths(r)
}This change:
Why This Fixes the IssueThe fix works because:
The use of Technical ImpactThis change only affects the internal path handling for versioned OAS APIs and doesn't modify any external interfaces or behaviors. It's a targeted fix that ensures existing functionality (mock responses) works correctly for all API versions. The implementation is robust, handling error cases and maintaining backward compatibility with non-OAS APIs. Tip: Mention me again using |
🛡️ Security Snapshot
## Security Impact AnalysisThis PR addresses a bug where mock responses weren't executing for non-base versions of OAS APIs. The issue was in the path handling logic in the The fix modifies the
This change is focused on functionality rather than security, but it does involve URL path manipulation which warrants security consideration. ## Identified VulnerabilitiesNo significant vulnerabilities were introduced by this change. The PR actually improves security by:
The code includes appropriate error checking for cases where:
## Security RecommendationsThe implementation is secure as it uses standard library functions for path manipulation and includes proper error handling. The use of No additional security measures are needed for this specific change. ## OWASP ComplianceThis change doesn't directly impact OWASP Top 10 categories. It's a functional fix for internal path handling that doesn't affect authentication, authorization, or input validation mechanisms. The use of ## Summary
Tip: Mention me again using |
Security Analysis of OAS API Versions Path Handling Fix🛡️ Security Snapshot
## Security Impact AnalysisThis PR fixes a bug where mock responses weren't being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic in the ## Identified VulnerabilitiesNo significant vulnerabilities were introduced by this change. The PR actually improves security by:
## Security RecommendationsThe implementation is secure as it uses standard library functions for path manipulation and includes proper error handling. No additional security measures are needed for this specific change. ## OWASP ComplianceThis change doesn't directly impact OWASP Top 10 categories. It's a functional fix for internal path handling that doesn't affect authentication, authorization, or input validation mechanisms. The use of ## Summary
Tip: Mention me again using |
Analysis of OAS API Versioning Path Handling FixIssue SummaryThis PR addresses a bug where mock responses weren't being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic in the Root Cause AnalysisThe problem occurred in the
Solution ImplementationThe PR introduces a conditional approach to path handling based on API type: if v.Spec.IsOAS {
targetApi, ok := v.Gw.apisByID[subVersionID]
if !ok {
return errors.New(string(VersionDoesNotExist)), http.StatusNotFound
}
strippedPath := v.Spec.StripListenPath(r.URL.Path)
newPath, err := url.JoinPath(targetApi.Proxy.ListenPath, strippedPath)
if err != nil {
return err, http.StatusInternalServerError
}
r.URL.Path = newPath
} else {
v.Spec.SanitizeProxyPaths(r)
}This solution:
Technical ImpactThe fix ensures that when a request is made to a non-base version of an OAS API:
The use of ConclusionThis is a focused fix that addresses a specific issue with path handling for versioned OAS APIs. The implementation is straightforward and uses standard Go libraries for path handling, making it a safe and effective solution. The change only affects internal path handling and doesn't modify any external interfaces or behaviors, ensuring backward compatibility. Tip: Mention me again using |
Analysis of PR #7254: OAS API Versions Path Handling FixIssue OverviewThis PR addresses a bug where mock responses were not being executed when calling non-base versions of OAS APIs. The issue was in the path handling logic in the Root Cause AnalysisWhen a request is made to a non-base version of an OAS API:
The issue occurs because the OAS router needs a properly formatted path to find the correct operation. The Solution ImplementationThe PR modifies the if v.Spec.IsOAS {
targetApi, ok := v.Gw.apisByID[subVersionID]
if !ok {
return errors.New(string(VersionDoesNotExist)), http.StatusNotFound
}
strippedPath := v.Spec.StripListenPath(r.URL.Path)
newPath, err := url.JoinPath(targetApi.Proxy.ListenPath, strippedPath)
if err != nil {
return err, http.StatusInternalServerError
}
r.URL.Path = newPath
} else {
v.Spec.SanitizeProxyPaths(r)
}This change:
By correctly formatting the path for OAS APIs, the router can now find the appropriate operation, allowing the mock response middleware to work correctly for non-base versions of OAS APIs. Technical ImpactThis change is focused solely on internal path handling and doesn't affect any external interfaces or behaviors. It doesn't require changes to any downstream repositories (tyk-operator, tyk-charts, portal, tyk-sink) as it doesn't modify any API schemas, protocols, or configurations. The fix ensures that all OAS API features, particularly mock responses, work correctly for all API versions, improving the consistency and reliability of the API gateway. Tip: Mention me again using |
|
radkrawczyk
changed the title
User description
TT-15301
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
PR Type
Bug fix
Description
Fixes mock response execution for non-base OAS API versions
Adds path handling logic for OAS APIs in version check middleware
Ensures correct URL path joining for OAS API sub-versions
Retains classic API path sanitization logic
Diagram Walkthrough
File Walkthrough
mw_version_check.go
Fix OAS API version path handling in middlewaregateway/mw_version_check.go