[TT-11960]fix panic in Oas when using mode public #6241

Keithwachira · 2024-04-23T07:59:29Z

User description

TT-11960
On gateway 5.3.0 I get a panic when I try to fetch a single OAS api when the mode query parameter is set to public.(I am trying to fetch with the endpoint tyk/apis/oas/{apiID}?mode=public) . From the code I can see that a user is allowed to send mode as a query parameter.

This happens Since we return a pointer. When deleting x-tyk-gateway in OAS we get a panic due to a race condition.

Type

bug_fix, enhancement

Description

Fixed a race condition in handleGetAPIOAS by ensuring that modifications are made on a copy of the object, preventing potential data races.
Refactored error messages into grouped constant blocks for cleaner code and easier management.
Enhanced code readability by improving formatting and updating comments.
Modified file permission settings to use octal notation, aligning with Go best practices.

Changes walkthrough

Relevant files

Bug_fix

api.go `Fix Race Condition and Code Refactoring in API Handling` gateway/api.go Fixed race condition in `handleGetAPIOAS` by creating a copy of `apiOAS` before modification. Improved code formatting and comments for better readability. Updated error constants to grouped `const` declarations. Changed file permission format to octal in `ioutil.WriteFile`.	+18/-29

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

github-actions · 2024-04-23T08:00:10Z

PR Description updated to latest commit (f1acfa8)

github-actions · 2024-04-23T08:00:25Z

API Changes

no api changes detected

github-actions · 2024-04-23T08:00:35Z

PR Review

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across a large file, including race condition fixes, error handling, and code formatting. The changes are significant but not overly complex, requiring careful review to ensure functionality and thread safety are maintained.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The direct assignment `oasCopy := *apiOAS` in `handleGetAPIOAS` might not deep copy all nested structs or slices if `apiOAS` contains any, potentially leading to unintended shared references.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/api.go
suggestion	Consider implementing a deep copy function for `oas.OAS` if it contains slices or other reference types, to ensure that all data is truly duplicated and no references are shared. This is important to prevent race conditions or data corruption. [important]
relevant line	oasCopy := *apiOAS

relevant file	gateway/api.go
suggestion	Replace the direct error string with a constant for consistency and maintainability, especially since similar errors are grouped as constants. [medium]
relevant line	return apiError("There is no such key found"), http.StatusNotFound

relevant file	gateway/api.go
suggestion	Ensure that the error handling for `ioutil.WriteFile` includes more specific actions or logging, such as retry mechanisms or detailed logging, which could help in diagnosing write failures more effectively. [medium]
relevant line	if err := ioutil.WriteFile(polFilePath, asByte, 0o644); err != nil {

relevant file	gateway/api.go
suggestion	Refactor the repeated pattern of error handling in HTTP handlers into a separate function to reduce code duplication and improve maintainability. [medium]
relevant line	doJSONWrite(w, http.StatusBadRequest, apiError("cannot parse form. Form malformed"))

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

github-actions · 2024-04-23T08:01:37Z

PR Code Suggestions

Category	Suggestions
Enhancement	Add handling for cases where `newSession.Expires` is less than or equal to 1. Consider handling the case where `newSession.Expires` is less than or equal to 1 to prevent potential issues with session expiration logic. gateway/api.go [507-508] -if time.Now().After(time.Unix(newSession.Expires, 0)) && newSession.Expires > 1 { +if newSession.Expires <= 1 { + // Handle or log error for invalid expiration +} else if time.Now().After(time.Unix(newSession.Expires, 0)) { newSession.Expires = originalKey.Expires }
Enhancement	Add validation for empty or invalid `fileName` in `doJSONExport`. To improve the robustness of the `doJSONExport` function, handle the case where `fileName` is empty or invalid, which could lead to file handling errors. gateway/api.go [143] func doJSONExport(w http.ResponseWriter, code int, obj interface{}, fileName string) { + if fileName == "" { + http.Error(w, "Invalid file name", http.StatusBadRequest) + return + } if code != http.StatusOK { doJSONWrite(w, code, obj) return }
Security	Add error handling for empty `KeyID` in session details. Add error handling for the case where `session.KeyID` might be an empty string after retrieving session details, which could lead to unexpected behavior or security issues. gateway/api.go [837-838] session, ok := gw.GlobalSessionManager.SessionDetail(orgID, keyName, true) +if !ok \|\| session.KeyID == "" { + return apiError("Invalid session or missing KeyID"), http.StatusBadRequest +} keyName = session.KeyID
Best practice	Replace deprecated `ioutil.WriteFile` with `os.WriteFile`. Instead of directly using `ioutil.WriteFile` which is deprecated, use `os.WriteFile` for writing files. gateway/api.go [935-937] -if err := ioutil.WriteFile(polFilePath, asByte, 0o644); err != nil { +if err := os.WriteFile(polFilePath, asByte, 0o644); err != nil { log.Error("Failed to create file! - ", err) return apiError("Failed to create file!"), http.StatusInternalServerError }

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

gateway/api.go

sonarcloud · 2024-04-23T08:13:06Z

Quality Gate failed

Failed conditions
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

github-actions · 2024-04-23T08:31:31Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api.go b/gateway/api.go
index ae50f95..ae1d25c 100644
--- a/gateway/api.go
+++ b/gateway/api.go
@@ -3478,4 +3478,4 @@ func updateOASServers(spec *APISpec, conf config.Config, apiDef *apidef.APIDefin
 
 	newAPIURL := getAPIURL(*apiDef, conf)
 	oasObj.UpdateServers(newAPIURL, oldAPIURL)
-}
\ No newline at end of file
+}

Please look at the run or in the Checks tab.

titpetric · 2024-04-23T10:56:46Z

gateway/api.go

@@ -1039,7 +1039,9 @@ func (gw *Gateway) handleGetAPIOAS(apiID string, modePublic bool) (interface{},

 	obj, code := gw.handleGetAPI(apiID, true)
 	if apiOAS, ok := obj.(*oas.OAS); ok && modePublic {
-		apiOAS.RemoveTykExtension()
+		oasCopy := *apiOAS


Use OAS.Clone for a deep copy.

Use OAS.Clone for a deep copy.

@titpetric I have tried to use OAS.Clone but when I use OAS.Clone it still panics. I am not sure why

github-actions · 2024-04-23T11:18:58Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api.go b/gateway/api.go
index 1ea518b..d7e88f5 100644
--- a/gateway/api.go
+++ b/gateway/api.go
@@ -1039,8 +1039,8 @@ func (gw *Gateway) handleGetAPIOAS(apiID string, modePublic bool) (interface{},
 
 	obj, code := gw.handleGetAPI(apiID, true)
 	if apiOAS, ok := obj.(*oas.OAS); ok && modePublic {
-		oasCopy,err := apiOAS.Clone()
-		if(err!=nil){
+		oasCopy, err := apiOAS.Clone()
+		if err != nil {
 			return apiError("marshalling failed"), http.StatusInternalServerError
 		}
 		oasCopy.RemoveTykExtension()
@@ -3481,4 +3481,4 @@ func updateOASServers(spec *APISpec, conf config.Config, apiDef *apidef.APIDefin
 
 	newAPIURL := getAPIURL(*apiDef, conf)
 	oasObj.UpdateServers(newAPIURL, oldAPIURL)
-}
\ No newline at end of file
+}

Please look at the run or in the Checks tab.

github-actions · 2024-04-23T11:18:59Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api.go b/gateway/api.go
index 1ea518b..d7e88f5 100644
--- a/gateway/api.go
+++ b/gateway/api.go
@@ -1039,8 +1039,8 @@ func (gw *Gateway) handleGetAPIOAS(apiID string, modePublic bool) (interface{},
 
 	obj, code := gw.handleGetAPI(apiID, true)
 	if apiOAS, ok := obj.(*oas.OAS); ok && modePublic {
-		oasCopy,err := apiOAS.Clone()
-		if(err!=nil){
+		oasCopy, err := apiOAS.Clone()
+		if err != nil {
 			return apiError("marshalling failed"), http.StatusInternalServerError
 		}
 		oasCopy.RemoveTykExtension()
@@ -3481,4 +3481,4 @@ func updateOASServers(spec *APISpec, conf config.Config, apiDef *apidef.APIDefin
 
 	newAPIURL := getAPIURL(*apiDef, conf)
 	oasObj.UpdateServers(newAPIURL, oldAPIURL)
-}
\ No newline at end of file
+}

Please look at the run or in the Checks tab.

github-actions · 2024-04-23T12:04:07Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api.go b/gateway/api.go
index ae50f95..ae1d25c 100644
--- a/gateway/api.go
+++ b/gateway/api.go
@@ -3478,4 +3478,4 @@ func updateOASServers(spec *APISpec, conf config.Config, apiDef *apidef.APIDefin
 
 	newAPIURL := getAPIURL(*apiDef, conf)
 	oasObj.UpdateServers(newAPIURL, oldAPIURL)
-}
\ No newline at end of file
+}

Please look at the run or in the Checks tab.

sonarcloud · 2024-09-17T11:01:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fix race condition in oas

f1acfa8

github-actions bot added enhancement bug_fix labels Apr 23, 2024

github-actions bot added the Review effort [1-5]: 3 label Apr 23, 2024

github-advanced-security bot found potential problems Apr 23, 2024

View reviewed changes

gateway/api.go Fixed Show fixed Hide fixed

gateway/api.go Fixed Show fixed Hide fixed

gateway/api.go Fixed Show fixed Hide fixed

gateway/api.go Fixed Show fixed Hide fixed

remove formating in pull request

3fcba9d

Keithwachira requested review from jeffy-mathew and lghiur April 23, 2024 09:00

Keithwachira changed the title ~~[TT-11960]fix race condition in oas~~ [TT-11960]fix panic in Oas when using mode public Apr 23, 2024

titpetric reviewed Apr 23, 2024

View reviewed changes

change from dereference to using clone to make a deep copy of oas

6858229

change from deepcopy to dereference

d3b2073

Merge branch 'master' into TT-11960-fix-race-condition-in-oas-delete

082001a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TT-11960]fix panic in Oas when using mode public #6241

[TT-11960]fix panic in Oas when using mode public #6241

Keithwachira commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

sonarcloud bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

titpetric Apr 23, 2024

Keithwachira Apr 23, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

sonarcloud bot commented Sep 17, 2024

[TT-11960]fix panic in Oas when using mode public #6241

Are you sure you want to change the base?

[TT-11960]fix panic in Oas when using mode public #6241

Conversation

Keithwachira commented Apr 23, 2024 • edited Loading

User description

Type

Description

Changes walkthrough

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024 • edited Loading

github-actions bot commented Apr 23, 2024

PR Review

github-actions bot commented Apr 23, 2024

PR Code Suggestions

sonarcloud bot commented Apr 23, 2024

Quality Gate failed

github-actions bot commented Apr 23, 2024

💥 CI tests failed 🙈

git-state

titpetric Apr 23, 2024

Choose a reason for hiding this comment

Keithwachira Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024

💥 CI tests failed 🙈

git-state

github-actions bot commented Apr 23, 2024

💥 CI tests failed 🙈

git-state

github-actions bot commented Apr 23, 2024

💥 CI tests failed 🙈

git-state

sonarcloud bot commented Sep 17, 2024

Quality Gate passed

Keithwachira commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading