[TT-14798] Replace JSEscapeString with json.Marshal for RFC 8259-compliant error responses

[lghiur]

Description

Related Issue

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

Ticket Details

TT-14798

Status	Blocked
Summary	Invalid JSON Return Issue with Tyk Validate Request Middleware

Generated at: 2025-11-08 19:25:42

[github-actions]

🚨 Jira Linter Failed

Commit: 40efe08
Failed at: 2025-11-08 19:25:44 UTC

The Jira linter failed to validate your PR. Please check the error details below:

🔍 Click to view error details

failed to validate Jira issue: jira ticket TT-14798 has status 'Blocked' but must be one of: In Dev, In Code Review, Ready For Dev, Dod Check

Next Steps

• Ensure your branch name contains a valid Jira ticket ID (e.g., ABC-123)
• Verify your PR title matches the branch's Jira ticket ID
• Check that the Jira ticket exists and is accessible

---

This comment will be automatically deleted once the linter passes.

[github-actions]

API Changes

no api changes detected

[probelabs]

🔍 Code Analysis Results

This pull request refactors the error handling mechanism for JSON responses to ensure RFC 8259 compliance. The core change is replacing the htmltemplate.JSEscapeString function with json.Marshal for escaping error message strings. This corrects the handling of special characters like single quotes, backslashes, and newlines, which were previously escaped incorrectly for JSON contexts. The change is accompanied by a comprehensive test suite to validate the new behavior for various edge cases and to ensure that XML error responses remain unaffected.

Files Changed Analysis

• gateway/handler_error.go: The logic for handling JSON error responses was modified. Instead of using htmltemplate.JSEscapeString, the error message is now processed with json.Marshal to produce a correctly escaped JSON string literal. The surrounding quotes are then removed before embedding the string into the final APIError struct. This change is conditional and applies only when the content type is JSON.
• gateway/handler_error_test.go: Substantial additions were made to this file. A new test, TestHandleErrorJSONEscaping, was introduced with multiple test cases to verify that various special characters (quotes, backslashes, newlines, tabs) are correctly handled in JSON error responses. Another test, TestHandleErrorXMLNoEscaping, was added to confirm that XML error responses are not inadvertently escaped.

Architecture & Impact Assessment

What this PR accomplishes

This PR fixes a bug where error messages in JSON responses were not escaped according to the JSON standard (RFC 8259). This ensures that API consumers receive well-formed JSON error payloads, preventing parsing issues when error messages contain special characters.

Key technical changes introduced

The key change is the switch from htmltemplate.JSEscapeString to json.Marshal for escaping error strings. JSEscapeString is intended for embedding strings within JavaScript in an HTML context and does not produce standard JSON string escaping. json.Marshal is the correct, standard library approach for this task.

Affected system components

The primary component affected is the Gateway's Error Handler. This handler is invoked whenever a middleware in the request processing chain returns an error. Because middleware is used for fundamental features like authentication, rate limiting, and request validation, this change impacts a wide range of error scenarios across all APIs managed by the gateway.

Here is a diagram illustrating the error handling flow:

sequenceDiagram
    participant Client
    participant Gateway
    participant MiddlewareChain
    participant ErrorHandler

    Client->>Gateway: API Request
    Gateway->>MiddlewareChain: Process Request
    MiddlewareChain-->>Gateway: Error (e.g., Auth Failed)
    Gateway->>ErrorHandler: HandleError(err)
    ErrorHandler-->>Gateway: Create JSON Error Response
    Note over ErrorHandler: Escapes error message using json.Marshal
    Gateway-->>Client: 4xx/5xx Response with valid JSON body

Loading

Scope Discovery & Context Expansion

The investigation into the usage of ErrorHandler.HandleError confirms that its impact is broad. The central invocation point is within gateway/middleware.go in the createMiddleware function. This function acts as a wrapper for all middleware executions. If any middleware's ProcessRequest method returns an error, it is caught and passed to ErrorHandler.HandleError.

This means the corrected escaping will apply to errors generated by:

• Authentication middleware (e.g., invalid tokens, expired keys).
• Rate limiting and quota checking middleware.
• Request validation middleware (e.g., OAS request validation).
• Custom middleware and plugins.

Essentially, any scenario that results in a synchronous error response during the request lifecycle will now benefit from the corrected JSON escaping. The change is confined to the gateway's core, and no other services or modules are directly affected, but all API consumers will see the corrected output.

Metadata

• Review Effort: 2 / 5
• Primary Label: bug

---

Powered by Visor from Probelabs

Last updated: 2025-11-08T19:33:00.796Z | Triggered by: opened | Commit: 40efe08

💡 TIP: You can chat with Visor using /visor ask <your question>

[probelabs]

🔍 Code Analysis Results

Security Issues (1)

Severity	Location	Issue
🟠 Error	gateway/handler_error.go:162-163	The change from `htmltemplate.JSEscapeString` to `json.Marshal` introduces a potential Cross-Site Scripting (XSS) vulnerability. The new implementation wraps the JSON-escaped string in `htmltemplate.HTML`, which disables the template engine's contextual auto-escaping. While `json.Marshal` escapes `<`, `>`, and `&`, it does not escape single quotes (`'`). This creates an XSS vector if a client application renders the error message within a single-quoted HTML attribute, allowing an attacker to break out of the attribute and execute arbitrary JavaScript. 💡 Suggestion To mitigate the XSS risk while ensuring RFC 8259 compliance, the raw error message should be HTML-escaped *before* being marshaled into a JSON string. This provides defense-in-depth by ensuring the final string is safe for rendering in any HTML context. 🔧 Suggested Fix // First, HTML-escape the raw error message to prevent XSS. safeErrMsg := htmltemplate.HTMLEscapeString(errMsg) // Then, marshal the HTML-safe string to ensure it is a valid JSON string. jsonBytes, _ := json.Marshal(safeErrMsg) apiError.Message = htmltemplate.HTML(string(jsonBytes[1 : len(jsonBytes)-1]))

Architecture Issues (1)

Severity	Location	Issue
🟡 Warning	gateway/handler_error.go:164-167	The implementation for JSON error handling could be simplified and made more robust. The current approach manually manipulates a JSON-marshaled string within a generic template-based error handler. This mixes concerns and has minor robustness issues such as ignoring the error from `json.Marshal` and using fragile string slicing which can panic. 💡 Suggestion Separate the JSON response generation from the template-based logic used for XML. Handle the JSON case by marshaling a dedicated error structure or map directly to the HTTP response writer. This avoids using templates for JSON, improves robustness by allowing for proper error handling, and increases code clarity. Example of a cleaner approach: // In HandleError function if contentType == header.ApplicationXML || contentType == header.TextXML { // ... existing template logic for XML } else { // Handle JSON case directly w.Header().Set(header.ContentType, header.ApplicationJSON) w.WriteHeader(errCode) err := json.NewEncoder(w).Encode(map[string]string{"error": errMsg}) if err != nil { log.WithError(err).Error("Failed to write JSON error response") } return // Return to avoid further writes by the template logic } // The rest of the template execution logic would only apply to XML.

Performance Issues (1)

Severity	Location	Issue
🟡 Warning	gateway/handler_error.go:165	The use of `json.Marshal` followed by slicing and conversion to a string introduces extra memory allocations in the error handling path. `json.Marshal(errMsg)` allocates a byte slice, and `string(jsonBytes[1:len(jsonBytes)-1])` allocates another string and copies the data. This results in at least two allocations per error response for JSON content types. In a high-performance gateway, minimizing allocations in the request path—including error handling—is important for maintaining low latency and high throughput, especially under high error-rate conditions. 💡 Suggestion To optimize this, consider implementing a more direct JSON string escaping function that doesn't add surrounding quotes, avoiding the `json.Marshal` overhead. This can be done efficiently using `strings.Builder` by iterating over the input string and appending escaped characters as needed. This approach would reduce the operation to a single memory allocation for the final escaped string, avoiding the overhead of `json.Marshal` and the subsequent slicing and type conversion.

Quality Issues (2)

Severity	Location	Issue
🟡 Warning	gateway/handler_error_test.go:66	In `TestHandleErrorJSONEscaping`, the test server, gateway, and error handler are set up inside the `for` loop, meaning they are re-initialized for every test case. This is inefficient as the setup does not depend on the test case data. This setup code can be moved outside the loop to run only once for the entire test function, which will make the tests run faster and improve the test structure. 💡 Suggestion Refactor the test to perform the setup once before iterating through the test cases. // Suggested refactored structure func TestHandleErrorJSONEscaping(t *testing.T) { // ... test cases definition ... ts := StartTest(nil) defer ts.Close() // Build a properly initialized API spec ts.Gw.BuildAndLoadAPI(func(spec *APISpec) { spec.Proxy.ListenPath = "/" spec.DoNotTrack = true }) // Get the spec from the gateway specs := ts.Gw.apiSpecs if len(specs) == 0 { t.Fatal("no API specs loaded") } spec := specs[0] // Create error handler errorHandler := &ErrorHandler{ BaseMiddleware: &BaseMiddleware{ Spec: spec, Gw: ts.Gw, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Create request and response recorder for each test case req := httptest.NewRequest("GET", "/test", nil) req.Header.Set(header.ContentType, header.ApplicationJSON) w := httptest.NewRecorder() // Call HandleError errorHandler.HandleError(w, req, tc.errorMsg, http.StatusBadRequest, true) // Assertions... }) } }
🟡 Warning	gateway/handler_error.go:162	The pattern of using `json.Marshal` on a string and then slicing the result to get the raw escaped content is effective but might not be immediately obvious to all developers. A brief comment explaining why this is done would improve code clarity and maintainability. 💡 Suggestion Add a comment to explain the logic. // Marshal the string to get it properly JSON-escaped, then strip the surrounding // quotes as the template will add its own. jsonBytes, _ := json.Marshal(errMsg) apiError.Message = htmltemplate.HTML(string(jsonBytes[1 : len(jsonBytes)-1]))

✅ Dependency Check Passed

No dependency issues found – changes LGTM.

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.

---

Powered by Visor from Probelabs

Last updated: 2025-11-08T19:33:01.497Z | Triggered by: opened | Commit: 40efe08

💡 TIP: You can chat with Visor using /visor ask <your question>

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE