Skip to content

Commit 410d054

Browse files
authored
Fix GHES GraphQL path handling and end-to-end query preservation in DIFC proxy when upstream is /api/v3 (#3970)
The DIFC proxy was forwarding GraphQL requests to an invalid GHES endpoint when the upstream API base was configured as `.../api/v3`, causing `gh` CLI requests routed through the proxy (e.g. `/api/graphql`) to fail with 404. This change normalizes GraphQL forwarding for GHES and ensures GraphQL query strings are preserved end-to-end while keeping existing REST behavior unchanged. - **GraphQL forwarding normalization for GHES** - Updated proxy upstream URL construction so that when `githubAPIURL` ends with `/api/v3`, GraphQL requests are forwarded to `/api/graphql` instead of `/api/v3/graphql`. - GHES rewrite now applies across accepted GraphQL path forms (`/graphql`, `/api/graphql`, `/api/v3/graphql`). - Preserves query strings on GraphQL requests. - **End-to-end query-string preservation** - Updated GraphQL forwarding call sites to pass the original request path (including raw query) rather than a hard-coded `"/graphql"` path, so query parameters are not dropped in production DIFC flow. - **Targeted coverage for rewritten GraphQL paths** - Added/updated unit tests to verify GHES rewrite and query preservation for: - `/graphql` → `/api/graphql` - `/graphql?foo=bar` → `/api/graphql?foo=bar` - `/api/graphql` → `/api/graphql` - `/api/v3/graphql?foo=bar` → `/api/graphql?foo=bar` - Added handler tests to verify query-string preservation across GraphQL inbound path variants. ```go pathOnly, query, hasQuery := strings.Cut(path, "?") if strings.HasSuffix(s.githubAPIURL, "/api/v3") && IsGraphQLPath(pathOnly) { url = strings.TrimSuffix(s.githubAPIURL, "/api/v3") + "/api/graphql" if hasQuery { url += "?" + query } } ``` > [!WARNING] > >
2 parents 3906216 + 539d64d commit 410d054

File tree

5 files changed

+118
-9
lines changed

5 files changed

+118
-9
lines changed

docs/PROXY_MODE.md

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,14 +89,19 @@ The proxy reuses the same 6-phase pipeline as the MCP gateway, with Phase 3 adap
8989

9090
## REST Route Mapping
9191

92-
The proxy maps ~25 GitHub REST API URL patterns to guard tool names:
92+
The proxy maps REST API URL patterns to guard tool names (see `internal/proxy/router.go` for the exact source of truth). Inbound paths are normalized first:
93+
94+
- `GH_HOST` style REST paths with `/api/v3/...` are normalized to `/...` for routing.
95+
- Query strings are ignored for route matching and still forwarded upstream.
96+
97+
Supported path families include:
9398

9499
| URL Pattern | Guard Tool |
95100
|-------------|-----------|
96101
| `/repos/:owner/:repo/issues` | `list_issues` |
97-
| `/repos/:owner/:repo/issues/:number` | `get_issue` |
102+
| `/repos/:owner/:repo/issues/:number` | `issue_read` |
98103
| `/repos/:owner/:repo/pulls` | `list_pull_requests` |
99-
| `/repos/:owner/:repo/pulls/:number` | `get_pull_request` |
104+
| `/repos/:owner/:repo/pulls/:number` | `pull_request_read` |
100105
| `/repos/:owner/:repo/commits` | `list_commits` |
101106
| `/repos/:owner/:repo/commits/:sha` | `get_commit` |
102107
| `/repos/:owner/:repo/contents/:path` | `get_file_contents` |
@@ -106,21 +111,34 @@ The proxy maps ~25 GitHub REST API URL patterns to guard tool names:
106111
| `/search/code` | `search_code` |
107112
| `/search/repositories` | `search_repositories` |
108113
| `/user` | `get_me` |
109-
| ... | See `internal/proxy/router.go` for full list |
114+
| `/notifications` | `list_notifications` |
115+
| `/orgs/:owner/actions/(secrets|variables)[/:name]` | `actions_list` |
116+
| `/repos/:owner/:repo/discussions...` | `list_discussions` / `get_discussion_comments` |
117+
| `/repos/:owner/:repo/...` (fallback) | `get_file_contents` |
118+
| ... | See `internal/proxy/router.go` for the complete regex list and precedence |
110119

111-
Unrecognized URLs pass through without DIFC filtering.
120+
For **read operations** (GET and GraphQL POST), unmatched routes are denied (fail-closed) to avoid accidental unfiltered data exposure. For **write operations** (non-read methods), requests pass through unchanged.
112121

113122
## GraphQL Support
114123

115-
GraphQL queries to `/graphql` are parsed to extract the operation type and owner/repo context:
124+
Inbound GraphQL endpoint paths accepted by the proxy:
125+
126+
- `/graphql` (github.com style)
127+
- `/api/graphql` (GHES style used by `gh` when host is GHES/proxy)
128+
- `/api/v3/graphql` (GH_HOST prefix style; normalized)
129+
130+
GraphQL queries are parsed to extract operation type and owner/repo context:
116131

117132
- **Repository-scoped queries** (issues, PRs, commits) — mapped to corresponding tool names
118133
- **Search queries** — mapped to `search_issues` or `search_code`
119134
- **Viewer queries** — mapped to `get_me`
120-
- **Unknown queries** — passed through without filtering
135+
- **Schema introspection (`__schema`, `__type`)** — passed through (safe metadata)
136+
- **Unknown queries** — denied (fail-closed)
121137

122138
Owner and repo are extracted from GraphQL variables (`$owner`, `$name`/`$repo`) or inline string arguments.
123139

140+
When the upstream API base is GHES-style `.../api/v3`, GraphQL forwarding is rewritten to `.../api/graphql` to match GHES routing.
141+
124142
## Policy Notes
125143

126144
- **Repo names must be lowercase** in policies (e.g., `octocat/hello-world` not `octocat/Hello-World`). The guard performs case-insensitive matching against actual GitHub data.

internal/proxy/handler.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
101101
if match.ToolName == "graphql_introspection" {
102102
logHandler.Printf("GraphQL introspection query, passing through")
103103
clientAuth := r.Header.Get("Authorization")
104-
resp, respBody := h.forwardAndReadBody(w, r.Context(), http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
104+
resp, respBody := h.forwardAndReadBody(w, r.Context(), http.MethodPost, fullPath, bytes.NewReader(graphQLBody), "application/json", clientAuth)
105105
if resp == nil {
106106
return
107107
}
@@ -206,7 +206,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
206206
oteltrace.WithSpanKind(oteltrace.SpanKindClient),
207207
)
208208
if graphQLBody != nil {
209-
resp, respBody = h.forwardAndReadBody(w, fwdCtx, http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
209+
resp, respBody = h.forwardAndReadBody(w, fwdCtx, http.MethodPost, path, bytes.NewReader(graphQLBody), "application/json", clientAuth)
210210
} else {
211211
resp, respBody = h.forwardAndReadBody(w, fwdCtx, r.Method, path, nil, "", clientAuth)
212212
}

internal/proxy/handler_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,47 @@ func TestServeHTTP_GraphQLIntrospectionPassthrough(t *testing.T) {
188188
assert.Contains(t, w.Body.String(), "__schema")
189189
}
190190

191+
func TestServeHTTP_GraphQLPreservesQueryString(t *testing.T) {
192+
tests := []struct {
193+
name string
194+
path string
195+
wantPath string
196+
}{
197+
{name: "graphql path", path: "/graphql?foo=bar", wantPath: "/graphql?foo=bar"},
198+
{name: "ghes api graphql path", path: "/api/graphql?foo=bar", wantPath: "/api/graphql?foo=bar"},
199+
{name: "gh host prefixed graphql path", path: "/api/v3/graphql?foo=bar", wantPath: "/graphql?foo=bar"},
200+
}
201+
202+
for _, tt := range tests {
203+
t.Run(tt.name, func(t *testing.T) {
204+
var receivedURL string
205+
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
206+
receivedURL = r.URL.RequestURI()
207+
w.Header().Set("Content-Type", "application/json")
208+
w.WriteHeader(http.StatusOK)
209+
_, err := w.Write([]byte(`{"data":{"repository":{"issues":{"nodes":[]}}}}`))
210+
require.NoError(t, err)
211+
}))
212+
defer upstream.Close()
213+
214+
s := newTestServer(t, upstream.URL)
215+
h := &proxyHandler{server: s}
216+
217+
gqlBody, err := json.Marshal(map[string]interface{}{
218+
"query": `{ repository(owner:"org", name:"repo") { issues(first: 10) { nodes { id } } } }`,
219+
})
220+
require.NoError(t, err)
221+
req := httptest.NewRequest(http.MethodPost, tt.path, bytes.NewReader(gqlBody))
222+
req.Header.Set("Content-Type", "application/json")
223+
w := httptest.NewRecorder()
224+
h.ServeHTTP(w, req)
225+
226+
assert.Equal(t, http.StatusOK, w.Code)
227+
assert.Equal(t, tt.wantPath, receivedURL)
228+
})
229+
}
230+
}
231+
191232
// ─── ServeHTTP: query string is forwarded on REST GET ────────────────────────
192233

193234
func TestServeHTTP_QueryStringForwardedToUpstream(t *testing.T) {

internal/proxy/proxy.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,13 @@ func (r *restBackendCaller) CallTool(ctx context.Context, toolName string, args
343343
// if non-empty it is forwarded as-is, otherwise the configured fallback token is used.
344344
func (s *Server) forwardToGitHub(ctx context.Context, method, path string, body io.Reader, contentType string, clientAuth string) (*http.Response, error) {
345345
url := s.githubAPIURL + path
346+
pathOnly, query, hasQuery := strings.Cut(path, "?")
347+
if strings.HasSuffix(s.githubAPIURL, "/api/v3") && IsGraphQLPath(pathOnly) {
348+
url = strings.TrimSuffix(s.githubAPIURL, "/api/v3") + "/api/graphql"
349+
if hasQuery {
350+
url += "?" + query
351+
}
352+
}
346353
logProxy.Printf("forwarding %s %s → %s", method, path, url)
347354

348355
req, err := http.NewRequestWithContext(ctx, method, url, body)

internal/proxy/proxy_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package proxy
22

33
import (
4+
"context"
45
"io"
56
"net/http"
67
"net/http/httptest"
@@ -844,3 +845,45 @@ func TestUnwrapSingleObject(t *testing.T) {
844845
})
845846
}
846847
}
848+
func TestForwardToGitHub_RewritesGraphQLPathForGHESAPIBase(t *testing.T) {
849+
var receivedPath string
850+
var receivedQuery string
851+
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
852+
receivedPath = r.URL.Path
853+
receivedQuery = r.URL.RawQuery
854+
w.WriteHeader(http.StatusOK)
855+
}))
856+
defer upstream.Close()
857+
858+
s := &Server{
859+
githubAPIURL: upstream.URL + "/api/v3",
860+
httpClient: upstream.Client(),
861+
}
862+
863+
tests := []struct {
864+
name string
865+
path string
866+
wantPath string
867+
wantQuery string
868+
}{
869+
{name: "plain graphql endpoint", path: "/graphql", wantPath: "/api/graphql"},
870+
{name: "graphql endpoint with query string", path: "/graphql?foo=bar", wantPath: "/api/graphql", wantQuery: "foo=bar"},
871+
{name: "ghes api graphql endpoint", path: "/api/graphql", wantPath: "/api/graphql"},
872+
{name: "gh host prefixed graphql endpoint with query string", path: "/api/v3/graphql?foo=bar", wantPath: "/api/graphql", wantQuery: "foo=bar"},
873+
}
874+
875+
for _, tt := range tests {
876+
t.Run(tt.name, func(t *testing.T) {
877+
receivedPath = ""
878+
receivedQuery = ""
879+
880+
resp, err := s.forwardToGitHub(context.Background(), http.MethodPost, tt.path, nil, "application/json", "")
881+
require.NoError(t, err)
882+
require.NotNil(t, resp)
883+
defer resp.Body.Close()
884+
885+
assert.Equal(t, tt.wantPath, receivedPath)
886+
assert.Equal(t, tt.wantQuery, receivedQuery)
887+
})
888+
}
889+
}

0 commit comments

Comments
 (0)