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

[yurisasuke]

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[bot]]

PR Description updated to latest commit (https://github.com/TykTechnologies/tyk/commit/f1acfa845bddaa9e81333787b1e718a9ce5b2ed3)

[github-actions[bot]]

API Changes

no api changes detected

[github-actions[bot]]

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[bot]]

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 }
	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.

[sonarqubecloud[bot]]

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[bot]]

:boom: CI tests failed :see_no_evil:

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.

[github-actions[bot]]

:boom: CI tests failed :see_no_evil:

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[bot]]

:boom: CI tests failed :see_no_evil:

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[bot]]

:boom: CI tests failed :see_no_evil:

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.

[sonarqubecloud[bot]]

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