Skip to content

Commit 3d55d31

Browse files
authored
proxy: replace http.Error with httputil.WriteErrorResponse for consistent JSON error shape (#5819)
`internal/proxy/handler.go` mixed `http.Error` (plain-text `text/plain` responses) with `httputil.WriteErrorResponse`/`WriteJSONResponse` (JSON), breaking client-side error handling. The `httputil` package explicitly documents that both server and proxy packages must use these helpers so API consumers always receive the same `{"error":"...","message":"..."}` shape. ## Changes - **`internal/proxy/handler.go`**: Replace all 7 `http.Error` calls with `httputil.WriteErrorResponse`, assigning appropriate error codes: | HTTP status | error code | |---|---| | 400 Bad Request | `bad_request` | | 403 Forbidden | `forbidden` | | 502 Bad Gateway | `bad_gateway` | | 503 Service Unavailable | `service_unavailable` | | 500 Internal Server Error | `internal_error` | ```go // Before — plain text, inconsistent Content-Type http.Error(w, "upstream request failed", http.StatusBadGateway) // After — uniform JSON shape across all error paths httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "upstream request failed") ```
2 parents bdc7597 + 7fbe817 commit 3d55d31

2 files changed

Lines changed: 25 additions & 10 deletions

File tree

internal/proxy/handler.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
8080
graphQLBody, err = io.ReadAll(r.Body)
8181
r.Body.Close()
8282
if err != nil {
83-
http.Error(w, "failed to read request body", http.StatusBadRequest)
83+
httputil.WriteErrorResponse(w, http.StatusBadRequest, "bad_request", "failed to read request body")
8484
return
8585
}
8686

@@ -116,7 +116,7 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
116116
if match == nil {
117117
// Unknown REST endpoint — fail closed: deny rather than risk leaking unfiltered data
118118
logHandler.Printf("unknown REST endpoint %s, blocking request", rawPath)
119-
http.Error(w, "access denied: unrecognized endpoint", http.StatusForbidden)
119+
httputil.WriteErrorResponse(w, http.StatusForbidden, "forbidden", "access denied: unrecognized endpoint")
120120
return
121121
}
122122
toolName = match.ToolName
@@ -152,7 +152,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
152152
errMsg := "returning 503: proxy enforcement not configured (no --policy flag provided)"
153153
logHandler.Print(errMsg)
154154
logger.LogError("proxy", "%s", errMsg)
155-
http.Error(w, "proxy enforcement not configured", http.StatusServiceUnavailable)
155+
httputil.WriteErrorResponse(w, http.StatusServiceUnavailable, "service_unavailable", "proxy enforcement not configured")
156156
return
157157
}
158158

@@ -166,7 +166,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
166166
if err != nil {
167167
logHandler.Printf("[DIFC] Phase 1 failed: %v", err)
168168
// On labeling failure, fail closed to prevent enforcement bypass
169-
http.Error(w, "resource labeling failed", http.StatusBadGateway)
169+
httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "resource labeling failed")
170170
return
171171
}
172172

@@ -332,7 +332,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
332332
} else {
333333
filteredJSON, err := json.Marshal(finalData)
334334
if err != nil {
335-
http.Error(w, "failed to serialize filtered response", http.StatusInternalServerError)
335+
httputil.WriteErrorResponse(w, http.StatusInternalServerError, "internal_error", "failed to serialize filtered response")
336336
return
337337
}
338338
copyResponseHeaders(w, resp)
@@ -401,13 +401,13 @@ func (h *proxyHandler) forwardAndReadBody(
401401
) (*http.Response, []byte) {
402402
resp, err := h.server.forwardToGitHub(ctx, method, path, body, contentType, clientAuth)
403403
if err != nil {
404-
http.Error(w, "upstream request failed", http.StatusBadGateway)
404+
httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "upstream request failed")
405405
return nil, nil
406406
}
407407
defer resp.Body.Close()
408408
respBody, err := io.ReadAll(resp.Body)
409409
if err != nil {
410-
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
410+
httputil.WriteErrorResponse(w, http.StatusBadGateway, "bad_gateway", "failed to read upstream response")
411411
return nil, nil
412412
}
413413
return resp, respBody

internal/proxy/handler_test.go

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,22 @@ func mockUpstream(t *testing.T, status int, body interface{}) *httptest.Server {
4848
}))
4949
}
5050

51+
func assertJSONErrorResponse(t *testing.T, resp *http.Response, wantStatus int, wantCode, wantMessage string) {
52+
t.Helper()
53+
54+
require.NotNil(t, resp)
55+
assert.Equal(t, wantStatus, resp.StatusCode)
56+
assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
57+
58+
var got struct {
59+
Error string `json:"error"`
60+
Message string `json:"message"`
61+
}
62+
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
63+
assert.Equal(t, wantCode, got.Error)
64+
assert.Equal(t, wantMessage, got.Message)
65+
}
66+
5167
// ─── ServeHTTP: health check ─────────────────────────────────────────────────
5268

5369
func TestServeHTTP_HealthCheck(t *testing.T) {
@@ -112,8 +128,7 @@ func TestServeHTTP_UnknownRESTEndpointBlocked(t *testing.T) {
112128
w := httptest.NewRecorder()
113129
h.ServeHTTP(w, req)
114130

115-
assert.Equal(t, http.StatusForbidden, w.Code)
116-
assert.Contains(t, w.Body.String(), "access denied")
131+
assertJSONErrorResponse(t, w.Result(), http.StatusForbidden, "forbidden", "access denied: unrecognized endpoint")
117132
}
118133

119134
// ─── ServeHTTP: /api/v3 GH-host prefix is stripped ───────────────────────────
@@ -456,7 +471,7 @@ func TestForwardAndReadBody_NetworkError(t *testing.T) {
456471

457472
assert.Nil(t, resp)
458473
assert.Nil(t, body)
459-
assert.Equal(t, http.StatusBadGateway, w.Code)
474+
assertJSONErrorResponse(t, w.Result(), http.StatusBadGateway, "bad_gateway", "upstream request failed")
460475
}
461476

462477
// ─── ServeHTTP: search query param is passed to args ─────────────────────────

0 commit comments

Comments
 (0)