Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+172 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion gateway/handler_error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"bytes"
"encoding/base64"
"encoding/json"
"errors"
htmltemplate "html/template"
"io"
Expand Down Expand Up @@ -152,14 +153,17 @@
var tmplExecutor TemplateExecutor
tmplExecutor = tmpl

apiError := APIError{htmltemplate.HTML(htmltemplate.JSEscapeString(errMsg))}
var apiError APIError

if contentType == header.ApplicationXML || contentType == header.TextXML {
apiError.Message = htmltemplate.HTML(errMsg)

//we look up in the last defined templateName to obtain the template.
rawTmpl := e.Gw.templatesRaw.Lookup(templateName)

Check warning on line 162 in gateway/handler_error.go

View check run for this annotation

probelabs / Visor: quality

documentation Issue 

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.
Raw output 
Add a comment to explain the logic.

```go
// 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]))
```
tmplExecutor = rawTmpl

Check failure on line 163 in gateway/handler_error.go

View check run for this annotation

probelabs / Visor: security

security Issue 

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.
Raw output 
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.
} else {
jsonBytes, _ := json.Marshal(errMsg)

Check failure on line 165 in gateway/handler_error.go

View workflow job for this annotation

GitHub Actions / lint 

Error return value of `json.Marshal` is not checked (errcheck)

Check warning on line 165 in gateway/handler_error.go

View check run for this annotation

probelabs / Visor: performance

performance Issue 

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.
Raw output 
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.
apiError.Message = htmltemplate.HTML(string(jsonBytes[1 : len(jsonBytes)-1]))
}

Check warning on line 167 in gateway/handler_error.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

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.
Raw output 
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:
```go
// 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.
```

var log bytes.Buffer
Expand Down
167 changes: 167 additions & 0 deletions gateway/handler_error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

import (
"bytes"
"encoding/json"
"io/ioutil"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -61,7 +63,7 @@
})
}

func TestHandleDefaultErrorXml(t *testing.T) {

Check warning on line 66 in gateway/handler_error_test.go

View check run for this annotation

probelabs / Visor: quality

performance Issue 

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.
Raw output 
Refactor the test to perform the setup once before iterating through the test cases.

```go
// 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...
        })
    }
}
```

expect := `<?xml version = "1.0" encoding = "UTF-8"?>
<error>There was a problem proxying the request</error>`
Expand Down Expand Up @@ -122,3 +124,168 @@
})

}

// TestHandleErrorJSONEscaping tests that error messages with special characters
// are properly escaped in JSON responses according to RFC 8259
func TestHandleErrorJSONEscaping(t *testing.T) {
testCases := []struct {
name string
errorMsg string
expectedMsg string
}{
{
name: "single quotes should be unescaped in JSON",
errorMsg: "parameter doesn't match the pattern",
expectedMsg: "parameter doesn't match the pattern",
},
{
name: "double quotes should be properly handled",
errorMsg: `parameter "name" is invalid`,
expectedMsg: `parameter "name" is invalid`,
},
{
name: "backslashes should be properly handled",
errorMsg: `pattern: \d+\w+`,
expectedMsg: `pattern: \d+\w+`,
},
{
name: "newlines should be properly handled",
errorMsg: "line1\nline2",
expectedMsg: "line1\nline2",
},
{
name: "tabs should be properly handled",
errorMsg: "col1\tcol2",
expectedMsg: "col1\tcol2",
},
{
name: "complex validation error with regex pattern",
errorMsg: `parameter "categoryPurpose" in header has an error: string doesn't match the regular expression "^[a-zA-Z0-9 /\-\?:\(\)\.,'\+]{1,3}$"`,
expectedMsg: `parameter "categoryPurpose" in header has an error: string doesn't match the regular expression "^[a-zA-Z0-9 /\-\?:\(\)\.,'\+]{1,3}$"`,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
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,
},
}

// Create a test request with JSON content type
req := httptest.NewRequest("GET", "/test", nil)
req.Header.Set(header.ContentType, header.ApplicationJSON)

// Create response writer
w := httptest.NewRecorder()

// Call HandleError with our test error message
errorHandler.HandleError(w, req, tc.errorMsg, http.StatusBadRequest, true)

// Get response body
resp := w.Result()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
t.Fatalf("failed to read response body: %v", err)
}

// Verify the response is valid JSON
var jsonResponse map[string]interface{}
if err := json.Unmarshal(body, &jsonResponse); err != nil {
t.Fatalf("response is not valid JSON: %v\nBody: %s", err, string(body))
}

// Verify the error message is properly escaped
errorMsg, ok := jsonResponse["error"].(string)
if !ok {
t.Fatalf("error field is not a string")
}

if errorMsg != tc.expectedMsg {
t.Errorf("expected error message:\n%s\n\ngot:\n%s", tc.expectedMsg, errorMsg)
}

// Verify Content-Type header
if contentType := resp.Header.Get(header.ContentType); contentType != header.ApplicationJSON {
t.Errorf("expected Content-Type %s, got %s", header.ApplicationJSON, contentType)
}
})
}
}

// TestHandleErrorXMLNoEscaping verifies that XML responses don't escape special characters
func TestHandleErrorXMLNoEscaping(t *testing.T) {
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,
},
}

// Test error message with special characters
errorMsg := `parameter "name" doesn't match pattern`

// Create a test request with XML content type
req := httptest.NewRequest("GET", "/test", nil)
req.Header.Set(header.ContentType, header.TextXML)

// Create response writer
w := httptest.NewRecorder()

// Call HandleError
errorHandler.HandleError(w, req, errorMsg, http.StatusBadRequest, true)

// Get response body
resp := w.Result()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
t.Fatalf("failed to read response body: %v", err)
}

// Verify the error message is NOT escaped (raw content for XML)
bodyStr := string(body)
if !strings.Contains(bodyStr, errorMsg) {
t.Errorf("expected XML body to contain raw error message:\n%s\n\ngot:\n%s", errorMsg, bodyStr)
}

// Verify Content-Type header
if contentType := resp.Header.Get(header.ContentType); contentType != header.TextXML {
t.Errorf("expected Content-Type %s, got %s", header.TextXML, contentType)
}
}
Loading