Skip to content

Commit 6b798e5

Browse files
authored
refactor(proxy): extract writeJSONResponse and forwardAndReadBody helpers in handler.go (#2646)
`internal/proxy/handler.go` repeated the same 3-line JSON response pattern at 4 sites and the `forwardToGitHub` + `io.ReadAll` sequence at 3 sites, with no shared helpers unlike the `server` package. ## Changes ### `writeJSONResponse` helper (issue #2629) Mirrors the existing helper in `internal/server/http_helpers.go`. Replaces inline `Content-Type → WriteHeader → json.Encode` blocks at: - Health check (200) - Unknown GraphQL query block (403) - DIFC write violation (403) - Strict-mode filter violation (403) ### `forwardAndReadBody` method on `proxyHandler` (issue #2630) Encapsulates the forward + read + dual error-check pattern. Returns `(*http.Response, []byte)`; callers check `resp == nil` on failure. Replaces inline sequences in `ServeHTTP` (GraphQL introspection), `handleWithDIFC` (Phase 3), and `passthrough`. Also fixes a latent bug: the GraphQL introspection site was silently discarding `io.ReadAll` errors (`respBody, _ := io.ReadAll(...)`), inconsistent with the other two sites. ```go // Before — repeated verbatim in 3 places: resp, err := h.server.forwardToGitHub(ctx, method, path, body, ct, auth) if err != nil { http.Error(w, "upstream request failed", http.StatusBadGateway) return } defer resp.Body.Close() respBody, err := io.ReadAll(resp.Body) if err != nil { http.Error(w, "failed to read upstream response", http.StatusBadGateway) return } // After: resp, respBody := h.forwardAndReadBody(w, ctx, method, path, body, ct, auth) if resp == nil { return } ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1901554330/b334/launcher.test /tmp/go-build1901554330/b334/launcher.test -test.testlogfile=/tmp/go-build1901554330/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8469�� ache/go/1.25.8/x64/src/runtime/cgo ache/go/1.25.8/x64/src/encoding/encoding.go x_amd64/vet -nxv` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1901554330/b319/config.test /tmp/go-build1901554330/b319/config.test -test.testlogfile=/tmp/go-build1901554330/b319/testlog.txt -test.paniconexit0 -test.timeout=10m0s abis�� ternal/engine/interpreter/compiler.go ternal/engine/interpreter/format.go x_amd64/compile` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1901554330/b334/launcher.test /tmp/go-build1901554330/b334/launcher.test -test.testlogfile=/tmp/go-build1901554330/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8469�� ache/go/1.25.8/x64/src/runtime/cgo ache/go/1.25.8/x64/src/encoding/encoding.go x_amd64/vet -nxv` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1901554330/b334/launcher.test /tmp/go-build1901554330/b334/launcher.test -test.testlogfile=/tmp/go-build1901554330/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8469�� ache/go/1.25.8/x64/src/runtime/cgo ache/go/1.25.8/x64/src/encoding/encoding.go x_amd64/vet -nxv` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1901554330/b343/mcp.test /tmp/go-build1901554330/b343/mcp.test -test.testlogfile=/tmp/go-build1901554330/b343/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o /mcp/connection.go /mcp/dockerenv.go x_amd64/vet -p ions =0 x_amd64/vet ortc�� g_.a 64/src/compress/gzip/gunzip.go x_amd64/vet --gdwarf-5 ernal/launcher lcache/go/1.25.8/x64=/_/GOROOT x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT TIPS --> --- ⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with [Raycast](https://gh.io/cca-raycast-docs).
2 parents 6cde047 + 05ac399 commit 6b798e5

1 file changed

Lines changed: 42 additions & 40 deletions

File tree

internal/proxy/handler.go

Lines changed: 42 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package proxy
22

33
import (
44
"bytes"
5+
"context"
56
"encoding/json"
67
"fmt"
78
"io"
@@ -34,9 +35,7 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
3435

3536
// Health check endpoint
3637
if rawPath == "/health" || rawPath == "/healthz" {
37-
w.Header().Set("Content-Type", "application/json")
38-
w.WriteHeader(http.StatusOK)
39-
json.NewEncoder(w).Encode(map[string]string{"status": "ok"})
38+
writeJSONResponse(w, http.StatusOK, map[string]string{"status": "ok"})
4039
return
4140
}
4241

@@ -68,9 +67,7 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
6867
if match == nil {
6968
// Unknown GraphQL query — fail closed: deny rather than risk leaking unfiltered data
7069
logHandler.Printf("unknown GraphQL query, blocking request: %s", strutil.Truncate(string(graphQLBody), 500))
71-
w.Header().Set("Content-Type", "application/json")
72-
w.WriteHeader(http.StatusForbidden)
73-
json.NewEncoder(w).Encode(map[string]interface{}{
70+
writeJSONResponse(w, http.StatusForbidden, map[string]interface{}{
7471
"errors": []map[string]string{{"message": "access denied: unrecognized GraphQL operation"}},
7572
"data": nil,
7673
})
@@ -80,13 +77,10 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
8077
if match.ToolName == "graphql_introspection" {
8178
logHandler.Printf("GraphQL introspection query, passing through")
8279
clientAuth := r.Header.Get("Authorization")
83-
resp, err := h.server.forwardToGitHub(r.Context(), http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
84-
if err != nil {
85-
http.Error(w, "upstream request failed", http.StatusBadGateway)
80+
resp, respBody := h.forwardAndReadBody(w, r.Context(), http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
81+
if resp == nil {
8682
return
8783
}
88-
defer resp.Body.Close()
89-
respBody, _ := io.ReadAll(resp.Body)
9084
h.writeResponse(w, resp, respBody)
9185
return
9286
}
@@ -157,9 +151,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
157151
} else {
158152
// Write blocked
159153
logHandler.Printf("[DIFC] Phase 2: BLOCKED %s %s — %s", r.Method, path, evalResult.Reason)
160-
w.Header().Set("Content-Type", "application/json")
161-
w.WriteHeader(http.StatusForbidden)
162-
json.NewEncoder(w).Encode(map[string]string{
154+
writeJSONResponse(w, http.StatusForbidden, map[string]string{
163155
"message": fmt.Sprintf("DIFC policy violation: %s", evalResult.Reason),
164156
})
165157
return
@@ -169,22 +161,13 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
169161
// **Phase 3: Forward to upstream GitHub API**
170162
clientAuth := r.Header.Get("Authorization")
171163
var resp *http.Response
164+
var respBody []byte
172165
if graphQLBody != nil {
173-
resp, err = s.forwardToGitHub(ctx, http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
166+
resp, respBody = h.forwardAndReadBody(w, ctx, http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
174167
} else {
175-
resp, err = s.forwardToGitHub(ctx, r.Method, path, nil, "", clientAuth)
176-
}
177-
if err != nil {
178-
logHandler.Printf("[DIFC] Phase 3 failed: %v", err)
179-
http.Error(w, "upstream request failed", http.StatusBadGateway)
180-
return
168+
resp, respBody = h.forwardAndReadBody(w, ctx, r.Method, path, nil, "", clientAuth)
181169
}
182-
defer resp.Body.Close()
183-
184-
// Read the response body
185-
respBody, err := io.ReadAll(resp.Body)
186-
if err != nil {
187-
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
170+
if resp == nil {
188171
return
189172
}
190173

@@ -241,9 +224,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
241224
// Strict mode: block entire response if any item filtered
242225
if s.enforcementMode == difc.EnforcementStrict && filtered.GetFilteredCount() > 0 {
243226
logHandler.Printf("[DIFC] STRICT: blocking response — %d filtered items", filtered.GetFilteredCount())
244-
w.Header().Set("Content-Type", "application/json")
245-
w.WriteHeader(http.StatusForbidden)
246-
json.NewEncoder(w).Encode(map[string]string{
227+
writeJSONResponse(w, http.StatusForbidden, map[string]string{
247228
"message": fmt.Sprintf("DIFC policy violation: %d of %d items not accessible",
248229
filtered.GetFilteredCount(), filtered.TotalCount),
249230
})
@@ -329,16 +310,8 @@ func (h *proxyHandler) passthrough(w http.ResponseWriter, r *http.Request, path
329310
defer r.Body.Close()
330311
}
331312

332-
resp, err := h.server.forwardToGitHub(r.Context(), r.Method, path, body, r.Header.Get("Content-Type"), r.Header.Get("Authorization"))
333-
if err != nil {
334-
http.Error(w, "upstream request failed", http.StatusBadGateway)
335-
return
336-
}
337-
defer resp.Body.Close()
338-
339-
respBody, err := io.ReadAll(resp.Body)
340-
if err != nil {
341-
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
313+
resp, respBody := h.forwardAndReadBody(w, r.Context(), r.Method, path, body, r.Header.Get("Content-Type"), r.Header.Get("Authorization"))
314+
if resp == nil {
342315
return
343316
}
344317

@@ -378,6 +351,35 @@ func (h *proxyHandler) writeEmptyResponse(w http.ResponseWriter, resp *http.Resp
378351
w.Write([]byte(empty))
379352
}
380353

354+
// writeJSONResponse sets the Content-Type header, writes the status code, and encodes
355+
// body as JSON. It centralises the three-line pattern used across HTTP handlers.
356+
func writeJSONResponse(w http.ResponseWriter, statusCode int, body interface{}) {
357+
w.Header().Set("Content-Type", "application/json")
358+
w.WriteHeader(statusCode)
359+
json.NewEncoder(w).Encode(body)
360+
}
361+
362+
// forwardAndReadBody forwards a request to the upstream GitHub API and reads the
363+
// entire response body. On success it returns the response and body bytes. It writes
364+
// a 502 error to w and returns nil, nil on failure.
365+
func (h *proxyHandler) forwardAndReadBody(
366+
w http.ResponseWriter, ctx context.Context,
367+
method, path string, body io.Reader, contentType, clientAuth string,
368+
) (*http.Response, []byte) {
369+
resp, err := h.server.forwardToGitHub(ctx, method, path, body, contentType, clientAuth)
370+
if err != nil {
371+
http.Error(w, "upstream request failed", http.StatusBadGateway)
372+
return nil, nil
373+
}
374+
defer resp.Body.Close()
375+
respBody, err := io.ReadAll(resp.Body)
376+
if err != nil {
377+
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
378+
return nil, nil
379+
}
380+
return resp, respBody
381+
}
382+
381383
// copyResponseHeaders copies relevant headers from upstream to the client response.
382384
func copyResponseHeaders(w http.ResponseWriter, resp *http.Response) {
383385
for _, h := range []string{

0 commit comments

Comments
 (0)