From eb8c3fc3d0344bbee9c91166ec5faadd9c36f396 Mon Sep 17 00:00:00 2001 From: Josh Free Date: Wed, 5 Aug 2026 16:39:24 -0700 Subject: [PATCH 1/4] Add ETag conditional requests to the REST transport Every REST request was issued unconditionally: the ETag returned by the GitHub API was never stored or replayed, so repeated tool calls that read the same resource (for example pull request reads, file and commit listings, and reviews) re-downloaded the full response each time. This adds an ETagTransport round tripper that caches the ETag and body of cacheable GET responses and sends If-None-Match on the next identical request. When the API answers 304 Not Modified, the cached body is served instead of re-downloading it. The transport is inserted below the user-agent and auth layers in createGitHubClients(), so cached entries are scoped by the request's Authorization header and never shared across tokens. The cache is bounded (LRU) and safe for concurrent use. Every request is still sent to the server, so responses are always revalidated and never served stale. Per the GitHub REST API docs, a 304 Not Modified response does not count against the token's primary rate limit, so repeated reads conserve rate-limit budget and bandwidth while returning identical data. Rate-limit headers are surfaced from the live 304 response so downstream rate-limit accounting stays correct. Closes #3025 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- internal/ghmcp/server.go | 8 +- pkg/http/headers/headers.go | 4 + pkg/http/transport/etag.go | 198 +++++++++++++++++++++++++++++++ pkg/http/transport/etag_test.go | 201 ++++++++++++++++++++++++++++++++ 4 files changed, 410 insertions(+), 1 deletion(-) create mode 100644 pkg/http/transport/etag.go create mode 100644 pkg/http/transport/etag_test.go diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index f13bdc476e..854f628e0c 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -66,8 +66,14 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv // authenticate via BearerAuthTransport and skip go-github's WithAuthToken: // the latter installs its own round tripper that would pin the static token // and shadow the dynamic one. + // + // ETagTransport sits below the user-agent (and auth) layers so that, by the + // time it runs, the Authorization header is set and can scope the + // conditional-request cache per token. It adds ETag/If-None-Match handling + // so unchanged resources are revalidated with a 304 instead of being + // re-downloaded in full. restUATransport := &transport.UserAgentTransport{ - Transport: http.DefaultTransport, + Transport: &transport.ETagTransport{Transport: http.DefaultTransport}, Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version), } var restClient *gogithub.Client diff --git a/pkg/http/headers/headers.go b/pkg/http/headers/headers.go index e032a0ce93..ce975cdcba 100644 --- a/pkg/http/headers/headers.go +++ b/pkg/http/headers/headers.go @@ -9,6 +9,10 @@ const ( AcceptHeader = "Accept" // UserAgentHeader is a standard HTTP Header. UserAgentHeader = "User-Agent" + // ETagHeader is a standard HTTP Header carrying a response entity tag. + ETagHeader = "ETag" + // IfNoneMatchHeader is a standard HTTP Header used to make a request conditional on an entity tag. + IfNoneMatchHeader = "If-None-Match" // ContentTypeJSON is the standard MIME type for JSON. ContentTypeJSON = "application/json" diff --git a/pkg/http/transport/etag.go b/pkg/http/transport/etag.go new file mode 100644 index 0000000000..0559e9c9bf --- /dev/null +++ b/pkg/http/transport/etag.go @@ -0,0 +1,198 @@ +package transport + +import ( + "bytes" + "container/list" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "net/http" + "sync" + + "github.com/github/github-mcp-server/pkg/http/headers" +) + +// defaultETagCacheSize bounds the number of cached conditional responses held +// in memory by an ETagTransport. +const defaultETagCacheSize = 512 + +// rateLimitHeaders are copied from the live 304 response onto a cache-served +// response so downstream rate-limit accounting observes the current state. +var rateLimitHeaders = []string{ + "X-RateLimit-Limit", + "X-RateLimit-Remaining", + "X-RateLimit-Used", + "X-RateLimit-Reset", + "X-RateLimit-Resource", + "Retry-After", + "Date", +} + +// etagEntry is a cached response body and headers keyed by an ETag. +type etagEntry struct { + etag string + status int + header http.Header + body []byte +} + +// response reconstructs an *http.Response from a cached entry, layering the +// live 304 response's rate-limit and timing headers on top so the caller sees +// the current rate-limit state while receiving the cached body. +func (e etagEntry) response(live *http.Response) *http.Response { + h := e.header.Clone() + for _, name := range rateLimitHeaders { + if values := live.Header.Values(name); len(values) > 0 { + h.Del(name) + for _, v := range values { + h.Add(name, v) + } + } + } + return &http.Response{ + Status: fmt.Sprintf("%d %s", e.status, http.StatusText(e.status)), + StatusCode: e.status, + Proto: live.Proto, + ProtoMajor: live.ProtoMajor, + ProtoMinor: live.ProtoMinor, + Header: h, + Body: io.NopCloser(bytes.NewReader(e.body)), + ContentLength: int64(len(e.body)), + Request: live.Request, + } +} + +type lruItem struct { + key string + entry etagEntry +} + +// ETagTransport is an http.RoundTripper that adds HTTP conditional-request +// support (ETag / If-None-Match) to GET requests. For each cacheable GET it +// stores the response ETag and body; on a subsequent identical request it sends +// If-None-Match and, when the server answers 304 Not Modified, serves the +// cached body instead of re-downloading it. +// +// Every request is still sent to the server, so responses are always +// revalidated and never served stale. A 304 Not Modified does not count against +// the GitHub REST API primary rate limit, so revalidated requests conserve +// rate-limit budget and bandwidth. +// +// Cached entries are scoped by the request's Authorization header so responses +// are never shared across tokens. The cache is bounded (LRU) and safe for +// concurrent use. +type ETagTransport struct { + Transport http.RoundTripper + + // MaxEntries bounds the number of cached responses. When zero, + // defaultETagCacheSize is used. + MaxEntries int + + mu sync.Mutex + ll *list.List + items map[string]*list.Element +} + +func (t *ETagTransport) RoundTrip(req *http.Request) (*http.Response, error) { + rt := t.Transport + if rt == nil { + rt = http.DefaultTransport + } + + // Only cache GET requests, and never override a caller-supplied conditional + // header. + if req.Method != http.MethodGet || req.Header.Get(headers.IfNoneMatchHeader) != "" { + return rt.RoundTrip(req) + } + + key := cacheKey(req) + cached, ok := t.get(key) + + req = req.Clone(req.Context()) + if ok { + req.Header.Set(headers.IfNoneMatchHeader, cached.etag) + } + + resp, err := rt.RoundTrip(req) + if err != nil { + return resp, err + } + + if resp.StatusCode == http.StatusNotModified && ok { + // Discard the empty 304 body and serve the cached response instead. + if resp.Body != nil { + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + return cached.response(resp), nil + } + + if resp.StatusCode == http.StatusOK { + if etag := resp.Header.Get(headers.ETagHeader); etag != "" { + body, readErr := io.ReadAll(resp.Body) + resp.Body.Close() + if readErr != nil { + return nil, readErr + } + t.add(key, etagEntry{ + etag: etag, + status: resp.StatusCode, + header: resp.Header.Clone(), + body: body, + }) + resp.Body = io.NopCloser(bytes.NewReader(body)) + resp.ContentLength = int64(len(body)) + } + } + + return resp, nil +} + +func cacheKey(req *http.Request) string { + sum := sha256.Sum256([]byte(req.Header.Get(headers.AuthorizationHeader))) + return req.Method + " " + req.URL.String() + " " + hex.EncodeToString(sum[:8]) +} + +func (t *ETagTransport) get(key string) (etagEntry, bool) { + t.mu.Lock() + defer t.mu.Unlock() + if t.items == nil { + return etagEntry{}, false + } + el, ok := t.items[key] + if !ok { + return etagEntry{}, false + } + t.ll.MoveToFront(el) + return el.Value.(*lruItem).entry, true +} + +func (t *ETagTransport) add(key string, entry etagEntry) { + t.mu.Lock() + defer t.mu.Unlock() + if t.items == nil { + t.items = make(map[string]*list.Element) + t.ll = list.New() + } + if el, ok := t.items[key]; ok { + el.Value.(*lruItem).entry = entry + t.ll.MoveToFront(el) + return + } + el := t.ll.PushFront(&lruItem{key: key, entry: entry}) + t.items[key] = el + + max := t.MaxEntries + if max <= 0 { + max = defaultETagCacheSize + } + for t.ll.Len() > max { + oldest := t.ll.Back() + if oldest == nil { + break + } + t.ll.Remove(oldest) + delete(t.items, oldest.Value.(*lruItem).key) + } +} diff --git a/pkg/http/transport/etag_test.go b/pkg/http/transport/etag_test.go new file mode 100644 index 0000000000..93bd088e1e --- /dev/null +++ b/pkg/http/transport/etag_test.go @@ -0,0 +1,201 @@ +package transport + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/github/github-mcp-server/pkg/http/headers" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestETagTransport_ServesCachedBodyOn304 verifies the core conditional-request +// flow: the first GET carries no If-None-Match and is cached with its ETag; the +// second GET sends the cached ETag and, on a 304 Not Modified, is served the +// cached body instead of the empty 304 body. +func TestETagTransport_ServesCachedBodyOn304(t *testing.T) { + t.Parallel() + + const etag = `"abc123"` + const body = `{"number":1}` + + var requests int32 + var lastIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&requests, 1) + lastIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, etag) + if n == 1 { + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, body) + return + } + // Second request revalidates and is unchanged. + w.WriteHeader(http.StatusNotModified) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + do := func() (*http.Response, string) { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp, string(data) + } + + resp1, body1 := do() + assert.Equal(t, http.StatusOK, resp1.StatusCode) + assert.Equal(t, body, body1) + assert.Empty(t, lastIfNoneMatch, "first request must not send If-None-Match") + + resp2, body2 := do() + assert.Equal(t, http.StatusOK, resp2.StatusCode, "304 is translated to the cached 200") + assert.Equal(t, body, body2, "cached body is served on 304") + assert.Equal(t, etag, lastIfNoneMatch, "second request sends the cached ETag") + assert.Equal(t, int32(2), atomic.LoadInt32(&requests), "every request still reaches the server") +} + +// TestETagTransport_UpdatesRateLimitHeadersFrom304 verifies that a cache-served +// response surfaces the live 304 response's rate-limit headers rather than the +// stale headers captured with the cached body. +func TestETagTransport_UpdatesRateLimitHeadersFrom304(t *testing.T) { + t.Parallel() + + const etag = `"v1"` + + var requests int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + n := atomic.AddInt32(&requests, 1) + w.Header().Set(headers.ETagHeader, etag) + if n == 1 { + w.Header().Set("X-RateLimit-Remaining", "100") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "cached") + return + } + w.Header().Set("X-RateLimit-Remaining", "99") + w.WriteHeader(http.StatusNotModified) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + do := func() *http.Response { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + return resp + } + + resp1 := do() + assert.Equal(t, "100", resp1.Header.Get("X-RateLimit-Remaining")) + + resp2 := do() + assert.Equal(t, "99", resp2.Header.Get("X-RateLimit-Remaining"), "rate-limit headers come from the live 304") +} + +// TestETagTransport_ScopesCacheByAuthorization verifies cached bodies are not +// shared across tokens. +func TestETagTransport_ScopesCacheByAuthorization(t *testing.T) { + t.Parallel() + + const etag = `"shared-url"` + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set(headers.ETagHeader, etag) + if r.Header.Get(headers.IfNoneMatchHeader) != "" { + w.WriteHeader(http.StatusNotModified) + return + } + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, r.Header.Get(headers.AuthorizationHeader)) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + get := func(auth string) string { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + req.Header.Set(headers.AuthorizationHeader, auth) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return string(data) + } + + assert.Equal(t, "Bearer a", get("Bearer a")) + assert.Equal(t, "Bearer b", get("Bearer b"), "a different token must not receive the other token's cached body") + assert.Equal(t, "Bearer a", get("Bearer a"), "the first token's cached body is served on revalidation") +} + +// TestETagTransport_OnlyCachesGET verifies non-GET requests bypass the cache and +// are never sent a conditional header. +func TestETagTransport_OnlyCachesGET(t *testing.T) { + t.Parallel() + + var sawIfNoneMatch bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get(headers.IfNoneMatchHeader) != "" { + sawIfNoneMatch = true + } + w.Header().Set(headers.ETagHeader, `"x"`) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + do := func() { + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + do() + do() + assert.False(t, sawIfNoneMatch, "POST requests must not be revalidated") +} + +// TestETagTransport_DoesNotOverrideCallerConditional verifies a caller-supplied +// If-None-Match header is preserved and not replaced by the cache. +func TestETagTransport_DoesNotOverrideCallerConditional(t *testing.T) { + t.Parallel() + + var gotIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + req.Header.Set(headers.IfNoneMatchHeader, `"caller"`) + + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, `"caller"`, gotIfNoneMatch) +} From e33d6569aba1a211a60f3ee179d2b5f9b8aac890 Mon Sep 17 00:00:00 2001 From: Josh Free Date: Wed, 5 Aug 2026 17:22:40 -0700 Subject: [PATCH 2/4] test(transport): satisfy bodyclose in etag_test helpers The do/helper closures returned *http.Response, which the bodyclose linter flags at each call site even though the body is closed inside the closure. Return only the asserted values (status code, body, and headers) so no response escapes the helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- pkg/http/transport/etag_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/http/transport/etag_test.go b/pkg/http/transport/etag_test.go index 93bd088e1e..f790378622 100644 --- a/pkg/http/transport/etag_test.go +++ b/pkg/http/transport/etag_test.go @@ -42,7 +42,7 @@ func TestETagTransport_ServesCachedBodyOn304(t *testing.T) { rt := &ETagTransport{Transport: http.DefaultTransport} - do := func() (*http.Response, string) { + do := func() (int, string) { req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) require.NoError(t, err) resp, err := rt.RoundTrip(req) @@ -50,16 +50,16 @@ func TestETagTransport_ServesCachedBodyOn304(t *testing.T) { defer resp.Body.Close() data, err := io.ReadAll(resp.Body) require.NoError(t, err) - return resp, string(data) + return resp.StatusCode, string(data) } - resp1, body1 := do() - assert.Equal(t, http.StatusOK, resp1.StatusCode) + status1, body1 := do() + assert.Equal(t, http.StatusOK, status1) assert.Equal(t, body, body1) assert.Empty(t, lastIfNoneMatch, "first request must not send If-None-Match") - resp2, body2 := do() - assert.Equal(t, http.StatusOK, resp2.StatusCode, "304 is translated to the cached 200") + status2, body2 := do() + assert.Equal(t, http.StatusOK, status2, "304 is translated to the cached 200") assert.Equal(t, body, body2, "cached body is served on 304") assert.Equal(t, etag, lastIfNoneMatch, "second request sends the cached ETag") assert.Equal(t, int32(2), atomic.LoadInt32(&requests), "every request still reaches the server") @@ -90,21 +90,21 @@ func TestETagTransport_UpdatesRateLimitHeadersFrom304(t *testing.T) { rt := &ETagTransport{Transport: http.DefaultTransport} - do := func() *http.Response { + do := func() http.Header { req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) require.NoError(t, err) resp, err := rt.RoundTrip(req) require.NoError(t, err) _, _ = io.Copy(io.Discard, resp.Body) resp.Body.Close() - return resp + return resp.Header } - resp1 := do() - assert.Equal(t, "100", resp1.Header.Get("X-RateLimit-Remaining")) + h1 := do() + assert.Equal(t, "100", h1.Get("X-RateLimit-Remaining")) - resp2 := do() - assert.Equal(t, "99", resp2.Header.Get("X-RateLimit-Remaining"), "rate-limit headers come from the live 304") + h2 := do() + assert.Equal(t, "99", h2.Get("X-RateLimit-Remaining"), "rate-limit headers come from the live 304") } // TestETagTransport_ScopesCacheByAuthorization verifies cached bodies are not From 5c8069e3a19a93e7697c6fe8d4c0610392a77c6f Mon Sep 17 00:00:00 2001 From: Josh Free Date: Wed, 5 Aug 2026 17:56:01 -0700 Subject: [PATCH 3/4] fix(transport): rename local max to avoid shadowing builtin revive's redefines-builtin-id flags the local variable named max in the LRU eviction loop, which shadows the Go 1.21 builtin. Rename it to limit; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- pkg/http/transport/etag.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/http/transport/etag.go b/pkg/http/transport/etag.go index 0559e9c9bf..3ed82dd132 100644 --- a/pkg/http/transport/etag.go +++ b/pkg/http/transport/etag.go @@ -183,11 +183,11 @@ func (t *ETagTransport) add(key string, entry etagEntry) { el := t.ll.PushFront(&lruItem{key: key, entry: entry}) t.items[key] = el - max := t.MaxEntries - if max <= 0 { - max = defaultETagCacheSize + limit := t.MaxEntries + if limit <= 0 { + limit = defaultETagCacheSize } - for t.ll.Len() > max { + for t.ll.Len() > limit { oldest := t.ll.Back() if oldest == nil { break From ed5fdb83fa0129bc1d1380b80b74159ec102b105 Mon Sep 17 00:00:00 2001 From: Josh Free Date: Thu, 6 Aug 2026 11:15:02 -0700 Subject: [PATCH 4/4] Scope ETag cache to REST client and bound it by bytes Restrict the conditional-request cache to the long-lived local (stdio) REST client and give the raw-content client a separate transport without it, so large file bodies are streamed rather than buffered into memory. Bound the cache by a per-entry and total-byte budget in addition to the entry count, and never retain responses marked non-storable by HTTP cache directives (request or response Cache-Control: no-store, or Vary: *). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- internal/ghmcp/server.go | 58 +++++++---- pkg/http/headers/headers.go | 4 + pkg/http/transport/etag.go | 178 ++++++++++++++++++++++++++------ pkg/http/transport/etag_test.go | 174 +++++++++++++++++++++++++++++++ 4 files changed, 365 insertions(+), 49 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 854f628e0c..bd686a7b42 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -72,26 +72,17 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv // conditional-request cache per token. It adds ETag/If-None-Match handling // so unchanged resources are revalidated with a 304 instead of being // re-downloaded in full. + // + // The conditional-request cache is enabled only for the REST API client on + // this long-lived local (stdio) server. The raw-content client below uses a + // separate transport without it, so large file bodies are never buffered + // into the cache. The hosted, horizontally-scaled server builds a fresh REST + // client per request (see pkg/github RequestDeps) and does not use this path. restUATransport := &transport.UserAgentTransport{ Transport: &transport.ETagTransport{Transport: http.DefaultTransport}, Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version), } - var restClient *gogithub.Client - if cfg.TokenProvider != nil { - restClient, err = gogithub.NewClient( - gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ - Transport: restUATransport, - TokenProvider: cfg.TokenProvider, - }}), - gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), - ) - } else { - restClient, err = gogithub.NewClient( - gogithub.WithHTTPClient(&http.Client{Transport: restUATransport}), - gogithub.WithAuthToken(cfg.Token), - gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), - ) - } + restClient, err := newRESTClient(cfg, restUATransport, restURL.String(), uploadURL.String()) if err != nil { return nil, fmt.Errorf("failed to create REST client: %w", err) } @@ -110,8 +101,18 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv gqlClient := githubv4.NewEnterpriseClient(graphQLURL.String(), gqlHTTPClient) - // Create raw content client (shares REST client's HTTP transport) - rawClient, err := raw.NewClient(restClient, rawURL) + // Create raw content client. It shares the REST client's authentication but + // uses a transport without the conditional-request cache: raw file bodies can + // be large and are streamed rather than retained in memory. + rawUATransport := &transport.UserAgentTransport{ + Transport: http.DefaultTransport, + Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version), + } + rawRESTClient, err := newRESTClient(cfg, rawUATransport, restURL.String(), uploadURL.String()) + if err != nil { + return nil, fmt.Errorf("failed to create raw REST client: %w", err) + } + rawClient, err := raw.NewClient(rawRESTClient, rawURL) if err != nil { return nil, fmt.Errorf("failed to create raw client: %w", err) } @@ -138,6 +139,27 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv }, nil } +// newRESTClient builds a go-github REST client that sends requests through the +// supplied user-agent transport, wiring authentication to match the server +// configuration: a dynamic TokenProvider via BearerAuthTransport, or a static +// token via go-github's WithAuthToken. +func newRESTClient(cfg github.MCPServerConfig, uaTransport *transport.UserAgentTransport, restURL, uploadURL string) (*gogithub.Client, error) { + if cfg.TokenProvider != nil { + return gogithub.NewClient( + gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ + Transport: uaTransport, + TokenProvider: cfg.TokenProvider, + }}), + gogithub.WithEnterpriseURLs(restURL, uploadURL), + ) + } + return gogithub.NewClient( + gogithub.WithHTTPClient(&http.Client{Transport: uaTransport}), + gogithub.WithAuthToken(cfg.Token), + gogithub.WithEnterpriseURLs(restURL, uploadURL), + ) +} + func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Server, error) { apiHost, err := utils.NewAPIHost(cfg.Host) if err != nil { diff --git a/pkg/http/headers/headers.go b/pkg/http/headers/headers.go index ce975cdcba..0ac24e5471 100644 --- a/pkg/http/headers/headers.go +++ b/pkg/http/headers/headers.go @@ -13,6 +13,10 @@ const ( ETagHeader = "ETag" // IfNoneMatchHeader is a standard HTTP Header used to make a request conditional on an entity tag. IfNoneMatchHeader = "If-None-Match" + // CacheControlHeader is a standard HTTP Header carrying caching directives. + CacheControlHeader = "Cache-Control" + // VaryHeader is a standard HTTP Header describing which request headers a response varies on. + VaryHeader = "Vary" // ContentTypeJSON is the standard MIME type for JSON. ContentTypeJSON = "application/json" diff --git a/pkg/http/transport/etag.go b/pkg/http/transport/etag.go index 3ed82dd132..827b787efd 100644 --- a/pkg/http/transport/etag.go +++ b/pkg/http/transport/etag.go @@ -8,14 +8,26 @@ import ( "fmt" "io" "net/http" + "strings" "sync" "github.com/github/github-mcp-server/pkg/http/headers" ) -// defaultETagCacheSize bounds the number of cached conditional responses held -// in memory by an ETagTransport. -const defaultETagCacheSize = 512 +const ( + // defaultETagCacheSize bounds the number of cached conditional responses held + // in memory by an ETagTransport. + defaultETagCacheSize = 512 + + // defaultMaxEntryBytes bounds the size of a single cached body. Responses + // larger than this are still revalidated normally but are never retained, so + // a few large reads cannot pin large buffers in memory. + defaultMaxEntryBytes = 1 << 20 // 1 MiB + + // defaultMaxTotalBytes bounds the combined size of all cached bodies. The + // least-recently-used entries are evicted until the cache is within budget. + defaultMaxTotalBytes = 32 << 20 // 32 MiB +) // rateLimitHeaders are copied from the live 304 response onto a cache-served // response so downstream rate-limit accounting observes the current state. @@ -80,8 +92,14 @@ type lruItem struct { // rate-limit budget and bandwidth. // // Cached entries are scoped by the request's Authorization header so responses -// are never shared across tokens. The cache is bounded (LRU) and safe for -// concurrent use. +// are never shared across tokens. Responses marked non-storable by HTTP cache +// directives (request or response Cache-Control: no-store, or Vary: *) are never +// retained. The cache is bounded both by entry count and by a total-byte budget +// (LRU) and is safe for concurrent use. +// +// This transport is intended for the long-lived local (stdio) server only. The +// hosted, horizontally-scaled server constructs a fresh REST client per request +// and does not use it, so an in-process cache adds nothing there. type ETagTransport struct { Transport http.RoundTripper @@ -89,9 +107,19 @@ type ETagTransport struct { // defaultETagCacheSize is used. MaxEntries int - mu sync.Mutex - ll *list.List - items map[string]*list.Element + // MaxEntryBytes bounds the size of a single cached body. Responses larger + // than this are revalidated but not retained. When zero, defaultMaxEntryBytes + // is used. + MaxEntryBytes int + + // MaxTotalBytes bounds the combined size of all cached bodies. When zero, + // defaultMaxTotalBytes is used. + MaxTotalBytes int + + mu sync.Mutex + ll *list.List + items map[string]*list.Element + curBytes int } func (t *ETagTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -100,9 +128,9 @@ func (t *ETagTransport) RoundTrip(req *http.Request) (*http.Response, error) { rt = http.DefaultTransport } - // Only cache GET requests, and never override a caller-supplied conditional - // header. - if req.Method != http.MethodGet || req.Header.Get(headers.IfNoneMatchHeader) != "" { + // Only cache GET requests, never override a caller-supplied conditional + // header, and honor a client request to bypass the cache entirely. + if req.Method != http.MethodGet || req.Header.Get(headers.IfNoneMatchHeader) != "" || hasNoStore(req.Header) { return rt.RoundTrip(req) } @@ -129,26 +157,76 @@ func (t *ETagTransport) RoundTrip(req *http.Request) (*http.Response, error) { } if resp.StatusCode == http.StatusOK { - if etag := resp.Header.Get(headers.ETagHeader); etag != "" { - body, readErr := io.ReadAll(resp.Body) - resp.Body.Close() - if readErr != nil { - return nil, readErr - } - t.add(key, etagEntry{ - etag: etag, - status: resp.StatusCode, - header: resp.Header.Clone(), - body: body, - }) - resp.Body = io.NopCloser(bytes.NewReader(body)) - resp.ContentLength = int64(len(body)) + etag := resp.Header.Get(headers.ETagHeader) + + // Drop any prior entry and skip caching when the response is missing an + // ETag or is marked non-storable by HTTP cache directives. + if etag == "" || !storable(resp) { + t.remove(key) + return resp, nil } + + // Skip caching bodies that exceed the per-entry byte budget. When the + // length is known up front, avoid buffering the body at all. + maxEntry := t.maxEntryBytes() + if resp.ContentLength > int64(maxEntry) { + t.remove(key) + return resp, nil + } + + body, readErr := io.ReadAll(resp.Body) + resp.Body.Close() + if readErr != nil { + return nil, readErr + } + resp.Body = io.NopCloser(bytes.NewReader(body)) + resp.ContentLength = int64(len(body)) + + if len(body) > maxEntry { + t.remove(key) + return resp, nil + } + + t.add(key, etagEntry{ + etag: etag, + status: resp.StatusCode, + header: resp.Header.Clone(), + body: body, + }) } return resp, nil } +// hasNoStore reports whether a Cache-Control header carries the no-store +// directive. +func hasNoStore(h http.Header) bool { + for _, cc := range h.Values(headers.CacheControlHeader) { + for _, directive := range strings.Split(cc, ",") { + if strings.EqualFold(strings.TrimSpace(directive), "no-store") { + return true + } + } + } + return false +} + +// storable reports whether a response may be retained. Responses that request +// no-store or that vary on every request (Vary: *) must not be cached. +func storable(resp *http.Response) bool { + if hasNoStore(resp.Header) { + return false + } + for _, vary := range resp.Header.Values(headers.VaryHeader) { + for _, field := range strings.Split(vary, ",") { + if strings.TrimSpace(field) == "*" { + return false + } + } + } + return true +} + func cacheKey(req *http.Request) string { sum := sha256.Sum256([]byte(req.Header.Get(headers.AuthorizationHeader))) return req.Method + " " + req.URL.String() + " " + hex.EncodeToString(sum[:8]) @@ -168,6 +246,36 @@ func (t *ETagTransport) get(key string) (etagEntry, bool) { return el.Value.(*lruItem).entry, true } +func (t *ETagTransport) maxEntryBytes() int { + if t.MaxEntryBytes > 0 { + return t.MaxEntryBytes + } + return defaultMaxEntryBytes +} + +func (t *ETagTransport) maxTotalBytes() int { + if t.MaxTotalBytes > 0 { + return t.MaxTotalBytes + } + return defaultMaxTotalBytes +} + +// remove drops a cached entry if present, keeping the byte accounting in sync. +func (t *ETagTransport) remove(key string) { + t.mu.Lock() + defer t.mu.Unlock() + if t.items == nil { + return + } + el, ok := t.items[key] + if !ok { + return + } + t.curBytes -= len(el.Value.(*lruItem).entry.body) + t.ll.Remove(el) + delete(t.items, key) +} + func (t *ETagTransport) add(key string, entry etagEntry) { t.mu.Lock() defer t.mu.Unlock() @@ -176,23 +284,31 @@ func (t *ETagTransport) add(key string, entry etagEntry) { t.ll = list.New() } if el, ok := t.items[key]; ok { - el.Value.(*lruItem).entry = entry + item := el.Value.(*lruItem) + t.curBytes += len(entry.body) - len(item.entry.body) + item.entry = entry t.ll.MoveToFront(el) - return + } else { + el := t.ll.PushFront(&lruItem{key: key, entry: entry}) + t.items[key] = el + t.curBytes += len(entry.body) } - el := t.ll.PushFront(&lruItem{key: key, entry: entry}) - t.items[key] = el limit := t.MaxEntries if limit <= 0 { limit = defaultETagCacheSize } - for t.ll.Len() > limit { + maxBytes := t.maxTotalBytes() + // Evict least-recently-used entries until within both the entry-count and + // total-byte budgets. Always keep at least the entry just inserted. + for t.ll.Len() > 1 && (t.ll.Len() > limit || t.curBytes > maxBytes) { oldest := t.ll.Back() if oldest == nil { break } + item := oldest.Value.(*lruItem) + t.curBytes -= len(item.entry.body) t.ll.Remove(oldest) - delete(t.items, oldest.Value.(*lruItem).key) + delete(t.items, item.key) } } diff --git a/pkg/http/transport/etag_test.go b/pkg/http/transport/etag_test.go index f790378622..ebbc222dee 100644 --- a/pkg/http/transport/etag_test.go +++ b/pkg/http/transport/etag_test.go @@ -199,3 +199,177 @@ func TestETagTransport_DoesNotOverrideCallerConditional(t *testing.T) { assert.Equal(t, `"caller"`, gotIfNoneMatch) } + +// TestETagTransport_SkipsBodiesOverEntryByteBudget verifies a response whose +// body exceeds the per-entry byte budget is served but never cached, so a +// subsequent request is not revalidated against a stored ETag. +func TestETagTransport_SkipsBodiesOverEntryByteBudget(t *testing.T) { + t.Parallel() + + const etag = `"big"` + body := make([]byte, 64) + + var lastIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, etag) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body) + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport, MaxEntryBytes: 16} + + do := func() []byte { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return data + } + + assert.Equal(t, body, do()) + assert.Equal(t, body, do(), "oversized body is served in full each time") + assert.Empty(t, lastIfNoneMatch, "an oversized response must not be cached or revalidated") +} + +// TestETagTransport_EvictsByTotalByteBudget verifies that inserting a second +// entry that pushes the cache over its total-byte budget evicts the +// least-recently-used entry, which is then re-fetched in full. +func TestETagTransport_EvictsByTotalByteBudget(t *testing.T) { + t.Parallel() + + body := make([]byte, 10) + ifNoneMatch := map[string]string{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ifNoneMatch[r.URL.Path] = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, `"`+r.URL.Path+`"`) + if r.Header.Get(headers.IfNoneMatchHeader) != "" { + w.WriteHeader(http.StatusNotModified) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body) + })) + defer server.Close() + + // Budget holds a single 10-byte entry; a second entry evicts the first. + rt := &ETagTransport{Transport: http.DefaultTransport, MaxTotalBytes: 15} + + get := func(path string) { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL+path, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + get("/a") // cache A + get("/b") // cache B, evicts A + get("/a") // A was evicted -> full GET, no conditional header + + assert.Empty(t, ifNoneMatch["/a"], "A must be re-fetched in full after eviction") +} + +// TestETagTransport_DoesNotCacheNoStoreResponse verifies a response marked +// Cache-Control: no-store is never retained. +func TestETagTransport_DoesNotCacheNoStoreResponse(t *testing.T) { + t.Parallel() + + var lastIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, `"ns"`) + w.Header().Set(headers.CacheControlHeader, "private, no-store") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "secret") + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + do := func() { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + do() + do() + assert.Empty(t, lastIfNoneMatch, "a no-store response must not be cached or revalidated") +} + +// TestETagTransport_DoesNotCacheVaryStar verifies a response with Vary: * is +// never retained. +func TestETagTransport_DoesNotCacheVaryStar(t *testing.T) { + t.Parallel() + + var lastIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, `"vary"`) + w.Header().Set(headers.VaryHeader, "*") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "body") + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + do := func() { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + do() + do() + assert.Empty(t, lastIfNoneMatch, "a Vary:* response must not be cached or revalidated") +} + +// TestETagTransport_RequestNoStoreBypassesCache verifies a client request that +// sends Cache-Control: no-store neither reads from nor revalidates against the +// cache, even after a prior response was cached. +func TestETagTransport_RequestNoStoreBypassesCache(t *testing.T) { + t.Parallel() + + var lastIfNoneMatch string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastIfNoneMatch = r.Header.Get(headers.IfNoneMatchHeader) + w.Header().Set(headers.ETagHeader, `"x"`) + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "body") + })) + defer server.Close() + + rt := &ETagTransport{Transport: http.DefaultTransport} + + // Prime the cache with a normal GET. + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + + // A no-store request must not send If-None-Match. + req2, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + req2.Header.Set(headers.CacheControlHeader, "no-store") + resp2, err := rt.RoundTrip(req2) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp2.Body) + resp2.Body.Close() + + assert.Empty(t, lastIfNoneMatch, "a no-store request must bypass the cache") +}