diff --git a/AGENTS.md b/AGENTS.md index 03b3471b..fd83c76e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,8 +33,9 @@ Quick reference for AI agents working with MCP Gateway (Go-based MCP proxy serve - `validation_test.go` - Comprehensive validation tests - `internal/difc/` - Decentralized Information Flow Control - `internal/envutil/` - Environment variable and Docker env-arg utilities +- `internal/githubhttp/` - GitHub API-specific HTTP helpers (auth headers, collaborator permission, rate-limit parsing) - `internal/guard/` - Security guards (NoopGuard, WasmGuard, WriteSinkGuard) -- `internal/httputil/` - Shared HTTP helper utilities (server, proxy) +- `internal/httputil/` - Generic HTTP helper utilities (server, proxy) - `internal/launcher/` - Backend process management - `internal/logger/` - Debug logging framework (micro logger) - `internal/mcp/` - MCP protocol types with enhanced error logging diff --git a/internal/config/validation.go b/internal/config/validation.go index 3b60308d..e759e415 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -512,3 +512,55 @@ func validateGuardPolicies(cfg *Config) error { } return nil } + +// validateRuleBasedPatterns validates additional rule-based string constraints that +// are not handled by schema validation alone. +func validateRuleBasedPatterns(stdinCfg *StdinConfig) error { + logValidation.Printf("Validating string patterns: server_count=%d", len(stdinCfg.MCPServers)) + + for name, server := range stdinCfg.MCPServers { + jsonPath := fmt.Sprintf("mcpServers.%s", name) + logValidation.Printf("Validating server: name=%s, type=%s", name, server.Type) + + if IsStdioServerType(server.Type) { + if server.Container != "" && !containerPattern.MatchString(server.Container) { + return InvalidPattern("container", server.Container, + fmt.Sprintf("%s.container", jsonPath), + "Use a valid container image format (e.g., 'ghcr.io/owner/image:tag', 'owner/image:latest', or 'ghcr.io/owner/image:tag@sha256:')") + } + + if server.Entrypoint != "" && len(strings.TrimSpace(server.Entrypoint)) == 0 { + return InvalidValue("entrypoint", "entrypoint cannot be empty or whitespace only", + fmt.Sprintf("%s.entrypoint", jsonPath), + "Provide a valid entrypoint path or remove the field") + } + } + + if server.Type == "http" { + if server.URL != "" && !urlPattern.MatchString(server.URL) { + return InvalidPattern("url", server.URL, + fmt.Sprintf("%s.url", jsonPath), + "Use a valid HTTP or HTTPS URL (e.g., 'https://api.example.com/mcp')") + } + } + } + + if stdinCfg.Gateway != nil { + if err := validateGatewayConfig(stdinCfg.Gateway); err != nil { + return err + } + + if stdinCfg.Gateway.Domain != "" { + domain := stdinCfg.Gateway.Domain + if domain != "localhost" && domain != "host.docker.internal" && + !domainVarPattern.MatchString(domain) && !domainHostnamePattern.MatchString(domain) { + return InvalidValue("domain", + fmt.Sprintf("domain '%s' must be 'localhost', 'host.docker.internal', an RFC-1123 hostname label (e.g. 'awmg-mcpg'), or a variable expression", domain), + "gateway.domain", + "Use 'localhost', 'host.docker.internal', a topology hostname like 'awmg-mcpg', or a variable like '${MCP_GATEWAY_DOMAIN}'") + } + } + } + + return nil +} diff --git a/internal/config/validation_rules.go b/internal/config/validation_rules.go index 46d3c458..84d31a7b 100644 --- a/internal/config/validation_rules.go +++ b/internal/config/validation_rules.go @@ -219,55 +219,3 @@ func AbsolutePath(value, fieldName, jsonPath string) *ValidationError { Suggestion: "Use an absolute path: Unix paths start with '/' (e.g., '/tmp/payloads'), Windows paths start with a drive letter (e.g., 'C:\\payloads')", } } - -// validateRuleBasedPatterns validates additional rule-based string constraints that -// are not handled by schema validation alone. -func validateRuleBasedPatterns(stdinCfg *StdinConfig) error { - logValidation.Printf("Validating string patterns: server_count=%d", len(stdinCfg.MCPServers)) - - for name, server := range stdinCfg.MCPServers { - jsonPath := fmt.Sprintf("mcpServers.%s", name) - logValidation.Printf("Validating server: name=%s, type=%s", name, server.Type) - - if IsStdioServerType(server.Type) { - if server.Container != "" && !containerPattern.MatchString(server.Container) { - return InvalidPattern("container", server.Container, - fmt.Sprintf("%s.container", jsonPath), - "Use a valid container image format (e.g., 'ghcr.io/owner/image:tag', 'owner/image:latest', or 'ghcr.io/owner/image:tag@sha256:')") - } - - if server.Entrypoint != "" && len(strings.TrimSpace(server.Entrypoint)) == 0 { - return InvalidValue("entrypoint", "entrypoint cannot be empty or whitespace only", - fmt.Sprintf("%s.entrypoint", jsonPath), - "Provide a valid entrypoint path or remove the field") - } - } - - if server.Type == "http" { - if server.URL != "" && !urlPattern.MatchString(server.URL) { - return InvalidPattern("url", server.URL, - fmt.Sprintf("%s.url", jsonPath), - "Use a valid HTTP or HTTPS URL (e.g., 'https://api.example.com/mcp')") - } - } - } - - if stdinCfg.Gateway != nil { - if err := validateGatewayConfig(stdinCfg.Gateway); err != nil { - return err - } - - if stdinCfg.Gateway.Domain != "" { - domain := stdinCfg.Gateway.Domain - if domain != "localhost" && domain != "host.docker.internal" && - !domainVarPattern.MatchString(domain) && !domainHostnamePattern.MatchString(domain) { - return InvalidValue("domain", - fmt.Sprintf("domain '%s' must be 'localhost', 'host.docker.internal', an RFC-1123 hostname label (e.g. 'awmg-mcpg'), or a variable expression", domain), - "gateway.domain", - "Use 'localhost', 'host.docker.internal', a topology hostname like 'awmg-mcpg', or a variable like '${MCP_GATEWAY_DOMAIN}'") - } - } - } - - return nil -} diff --git a/internal/httputil/github_http.go b/internal/githubhttp/client.go similarity index 92% rename from internal/httputil/github_http.go rename to internal/githubhttp/client.go index c7f4eeb3..d6118add 100644 --- a/internal/httputil/github_http.go +++ b/internal/githubhttp/client.go @@ -1,4 +1,6 @@ -package httputil +// Package githubhttp provides GitHub API-specific HTTP helpers shared across +// multiple packages (server, proxy, etc.). +package githubhttp import ( "context" @@ -7,8 +9,12 @@ import ( "strconv" "strings" "time" + + "github.com/github/gh-aw-mcpg/internal/logger" ) +var logHTTP = logger.New("githubhttp:client") + // GitHubUserAgent is the User-Agent header value sent on all GitHub API requests. const GitHubUserAgent = "awmg/1.0" @@ -18,7 +24,7 @@ var defaultGitHubHTTPClient = &http.Client{Timeout: 30 * time.Second} // ApplyGitHubAPIHeaders sets the standard GitHub API request headers on req. // authHeader should be the full Authorization header value (e.g. "token xyz" or -// "Bearer xyz"). When authHeader is empty no Authorization header is set, which +// "******"). When authHeader is empty no Authorization header is set, which // is appropriate when the caller has already decided that no auth is available. func ApplyGitHubAPIHeaders(req *http.Request, authHeader string) { path := "" diff --git a/internal/githubhttp/client_test.go b/internal/githubhttp/client_test.go new file mode 100644 index 00000000..307eb75e --- /dev/null +++ b/internal/githubhttp/client_test.go @@ -0,0 +1,229 @@ +package githubhttp + +import ( + "context" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestParseRateLimitResetHeader verifies the shared Unix-timestamp header parser. +func TestParseRateLimitResetHeader(t *testing.T) { + t.Parallel() + + now := time.Now() + future := now.Add(60 * time.Second) + + tests := []struct { + name string + value string + wantZero bool + wantTime time.Time + }{ + { + name: "empty", + value: "", + wantZero: true, + }, + { + name: "invalid", + value: "not-a-number", + wantZero: true, + }, + { + name: "valid unix timestamp", + value: "1000000000", + wantZero: false, + wantTime: time.Unix(1000000000, 0), + }, + { + name: "future timestamp", + value: strconv.FormatInt(future.Unix(), 10), + wantZero: false, + }, + { + name: "value with surrounding whitespace", + value: " 1000000000 ", + wantZero: false, + wantTime: time.Unix(1000000000, 0), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ParseRateLimitResetHeader(tt.value) + if tt.wantZero { + assert.True(t, got.IsZero(), "expected zero time") + } else { + assert.False(t, got.IsZero(), "expected non-zero time") + if !tt.wantTime.IsZero() { + assert.Equal(t, tt.wantTime.Unix(), got.Unix()) + } + } + }) + } +} + +// TestApplyGitHubAPIHeaders verifies that ApplyGitHubAPIHeaders sets the +// expected headers on an HTTP request. +func TestApplyGitHubAPIHeaders(t *testing.T) { + t.Run("sets Authorization when authHeader is non-empty", func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) + require.NoError(t, err) + + ApplyGitHubAPIHeaders(req, "token my-secret") + + assert.Equal(t, "token my-secret", req.Header.Get("Authorization")) + assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) + assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) + }) + + t.Run("does not set Authorization when authHeader is empty", func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) + require.NoError(t, err) + + ApplyGitHubAPIHeaders(req, "") + + assert.Empty(t, req.Header.Get("Authorization")) + assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) + assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) + }) + + t.Run("works with Bearer token scheme", func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) + require.NoError(t, err) + + ApplyGitHubAPIHeaders(req, "Bearer ghp_abc123") + + assert.Equal(t, "Bearer ghp_abc123", req.Header.Get("Authorization")) + assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) + assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) + }) + + t.Run("does not panic when request URL is nil", func(t *testing.T) { + req := &http.Request{Method: http.MethodGet, Header: make(http.Header)} + + require.NotPanics(t, func() { + ApplyGitHubAPIHeaders(req, "token my-secret") + }) + + assert.Equal(t, "token my-secret", req.Header.Get("Authorization")) + assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) + assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) + }) +} + +// TestDoGitHubGET verifies that DoGitHubGET sends a GET request with the correct +// headers and URL to the upstream server. +func TestDoGitHubGET(t *testing.T) { + t.Run("sends GET with GitHub headers", func(t *testing.T) { + var capturedMethod, capturedPath, capturedAuth, capturedAccept, capturedUA string + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedMethod = r.Method + capturedPath = r.URL.Path + capturedAuth = r.Header.Get("Authorization") + capturedAccept = r.Header.Get("Accept") + capturedUA = r.Header.Get("User-Agent") + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + resp, err := DoGitHubGET(context.Background(), upstream.URL, "/repos/owner/repo", "token ghp_test") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + + assert.Equal(t, http.MethodGet, capturedMethod) + assert.Equal(t, "/repos/owner/repo", capturedPath) + assert.Equal(t, "token ghp_test", capturedAuth) + assert.Equal(t, "application/vnd.github+json", capturedAccept) + assert.Equal(t, GitHubUserAgent, capturedUA) + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("returns error on invalid URL", func(t *testing.T) { + resp, err := DoGitHubGET(context.Background(), "://bad-url", "/path", "token x") + assert.Error(t, err) + assert.Nil(t, resp) + }) + + t.Run("returns error when HTTP transport fails", func(t *testing.T) { + // Start a server that immediately closes the connection to force a transport error. + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Hijack the connection and close it without sending a response. + hj, ok := w.(http.Hijacker) + if !ok { + http.Error(w, "hijack not supported", http.StatusInternalServerError) + return + } + conn, _, err := hj.Hijack() + if err != nil { + http.Error(w, "hijack failed", http.StatusInternalServerError) + return + } + conn.Close() + })) + defer upstream.Close() + + resp, err := DoGitHubGET(context.Background(), upstream.URL, "/path", "token x") + assert.Error(t, err) + assert.Nil(t, resp) + }) + + t.Run("returns error on cancelled context", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately before the request is made + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + resp, err := DoGitHubGET(ctx, upstream.URL, "/path", "token x") + assert.ErrorIs(t, err, context.Canceled) + assert.Nil(t, resp) + }) +} + +// TestComputeRetryAfter verifies all branches of the retry-delay calculation. +// The function returns a default 60s for zero or past reset times, applies a +// 1s safety buffer for future times, and clamps the result to [1, 3600] seconds. +func TestComputeRetryAfter(t *testing.T) { + t.Parallel() + + t.Run("zero time returns default 60s", func(t *testing.T) { + t.Parallel() + assert.Equal(t, 60, ComputeRetryAfter(time.Time{})) + }) + + t.Run("time in the past returns default 60s", func(t *testing.T) { + t.Parallel() + past := time.Now().Add(-5 * time.Minute) + assert.Equal(t, 60, ComputeRetryAfter(past)) + }) + + t.Run("future time returns delay with 1s safety buffer", func(t *testing.T) { + t.Parallel() + // 60s ahead: int(60.0)+1 = 61; allow ±2s for scheduling jitter. + future := time.Now().Add(60 * time.Second) + got := ComputeRetryAfter(future) + assert.InDelta(t, 61, got, 2.0, "expected ~61s for 60s reset with 1s buffer") + }) + + t.Run("far future is clamped to 3600s maximum", func(t *testing.T) { + t.Parallel() + assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(2*time.Hour))) + }) + + t.Run("time slightly above max delay is still clamped to 3600s", func(t *testing.T) { + t.Parallel() + // 3601s ahead: int(3601)+1 = 3602 > 3600 → clamped to 3600. + assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(3601*time.Second))) + }) +} diff --git a/internal/httputil/collaborator.go b/internal/githubhttp/collaborator.go similarity index 98% rename from internal/httputil/collaborator.go rename to internal/githubhttp/collaborator.go index 9b044f8a..955f0ce4 100644 --- a/internal/httputil/collaborator.go +++ b/internal/githubhttp/collaborator.go @@ -1,4 +1,4 @@ -package httputil +package githubhttp import ( "context" @@ -11,7 +11,7 @@ import ( "github.com/github/gh-aw-mcpg/internal/strutil" ) -var logCollab = logger.New("httputil:collaborator") +var logCollab = logger.New("githubhttp:collaborator") // ParseCollaboratorPermissionArgs extracts and validates the owner, repo, and // username fields from an args map for a get_collaborator_permission call. diff --git a/internal/httputil/collaborator_test.go b/internal/githubhttp/collaborator_test.go similarity index 99% rename from internal/httputil/collaborator_test.go rename to internal/githubhttp/collaborator_test.go index 402af52f..1a698e02 100644 --- a/internal/httputil/collaborator_test.go +++ b/internal/githubhttp/collaborator_test.go @@ -1,4 +1,4 @@ -package httputil +package githubhttp import ( "context" diff --git a/internal/httputil/httputil_test.go b/internal/httputil/httputil_test.go index bb17b1a4..3fe2e129 100644 --- a/internal/httputil/httputil_test.go +++ b/internal/httputil/httputil_test.go @@ -1,12 +1,10 @@ package httputil import ( - "context" "encoding/json" "errors" "net/http" "net/http/httptest" - "strconv" "testing" "time" @@ -219,222 +217,6 @@ func TestWriteJSONResponse(t *testing.T) { }) } -// TestParseRateLimitResetHeader verifies the shared Unix-timestamp header parser. -func TestParseRateLimitResetHeader(t *testing.T) { - t.Parallel() - - now := time.Now() - future := now.Add(60 * time.Second) - - tests := []struct { - name string - value string - wantZero bool - wantTime time.Time - }{ - { - name: "empty", - value: "", - wantZero: true, - }, - { - name: "invalid", - value: "not-a-number", - wantZero: true, - }, - { - name: "valid unix timestamp", - value: "1000000000", - wantZero: false, - wantTime: time.Unix(1000000000, 0), - }, - { - name: "future timestamp", - value: strconv.FormatInt(future.Unix(), 10), - wantZero: false, - }, - { - name: "value with surrounding whitespace", - value: " 1000000000 ", - wantZero: false, - wantTime: time.Unix(1000000000, 0), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got := ParseRateLimitResetHeader(tt.value) - if tt.wantZero { - assert.True(t, got.IsZero(), "expected zero time") - } else { - assert.False(t, got.IsZero(), "expected non-zero time") - if !tt.wantTime.IsZero() { - assert.Equal(t, tt.wantTime.Unix(), got.Unix()) - } - } - }) - } -} - -// TestApplyGitHubAPIHeaders verifies that ApplyGitHubAPIHeaders sets the -// expected headers on an HTTP request. -func TestApplyGitHubAPIHeaders(t *testing.T) { - t.Run("sets Authorization when authHeader is non-empty", func(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) - require.NoError(t, err) - - ApplyGitHubAPIHeaders(req, "token my-secret") - - assert.Equal(t, "token my-secret", req.Header.Get("Authorization")) - assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) - assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) - }) - - t.Run("does not set Authorization when authHeader is empty", func(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) - require.NoError(t, err) - - ApplyGitHubAPIHeaders(req, "") - - assert.Empty(t, req.Header.Get("Authorization")) - assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) - assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) - }) - - t.Run("works with Bearer token scheme", func(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, "http://example.com", nil) - require.NoError(t, err) - - ApplyGitHubAPIHeaders(req, "Bearer ghp_abc123") - - assert.Equal(t, "Bearer ghp_abc123", req.Header.Get("Authorization")) - assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) - assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) - }) - - t.Run("does not panic when request URL is nil", func(t *testing.T) { - req := &http.Request{Method: http.MethodGet, Header: make(http.Header)} - - require.NotPanics(t, func() { - ApplyGitHubAPIHeaders(req, "token my-secret") - }) - - assert.Equal(t, "token my-secret", req.Header.Get("Authorization")) - assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) - assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent")) - }) -} - -// TestDoGitHubGET verifies that DoGitHubGET sends a GET request with the correct -// headers and URL to the upstream server. -func TestDoGitHubGET(t *testing.T) { - t.Run("sends GET with GitHub headers", func(t *testing.T) { - var capturedMethod, capturedPath, capturedAuth, capturedAccept, capturedUA string - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - capturedMethod = r.Method - capturedPath = r.URL.Path - capturedAuth = r.Header.Get("Authorization") - capturedAccept = r.Header.Get("Accept") - capturedUA = r.Header.Get("User-Agent") - w.WriteHeader(http.StatusOK) - })) - defer upstream.Close() - - resp, err := DoGitHubGET(context.Background(), upstream.URL, "/repos/owner/repo", "token ghp_test") - require.NoError(t, err) - require.NotNil(t, resp) - defer resp.Body.Close() - - assert.Equal(t, http.MethodGet, capturedMethod) - assert.Equal(t, "/repos/owner/repo", capturedPath) - assert.Equal(t, "token ghp_test", capturedAuth) - assert.Equal(t, "application/vnd.github+json", capturedAccept) - assert.Equal(t, GitHubUserAgent, capturedUA) - assert.Equal(t, http.StatusOK, resp.StatusCode) - }) - - t.Run("returns error on invalid URL", func(t *testing.T) { - resp, err := DoGitHubGET(context.Background(), "://bad-url", "/path", "token x") - assert.Error(t, err) - assert.Nil(t, resp) - }) - - t.Run("returns error when HTTP transport fails", func(t *testing.T) { - // Start a server that immediately closes the connection to force a transport error. - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Hijack the connection and close it without sending a response. - hj, ok := w.(http.Hijacker) - if !ok { - http.Error(w, "hijack not supported", http.StatusInternalServerError) - return - } - conn, _, err := hj.Hijack() - if err != nil { - http.Error(w, "hijack failed", http.StatusInternalServerError) - return - } - conn.Close() - })) - defer upstream.Close() - - resp, err := DoGitHubGET(context.Background(), upstream.URL, "/path", "token x") - assert.Error(t, err) - assert.Nil(t, resp) - }) - - t.Run("returns error on cancelled context", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // cancel immediately before the request is made - - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer upstream.Close() - - resp, err := DoGitHubGET(ctx, upstream.URL, "/path", "token x") - assert.ErrorIs(t, err, context.Canceled) - assert.Nil(t, resp) - }) -} - -// TestComputeRetryAfter verifies all branches of the retry-delay calculation. -// The function returns a default 60s for zero or past reset times, applies a -// 1s safety buffer for future times, and clamps the result to [1, 3600] seconds. -func TestComputeRetryAfter(t *testing.T) { - t.Parallel() - - t.Run("zero time returns default 60s", func(t *testing.T) { - t.Parallel() - assert.Equal(t, 60, ComputeRetryAfter(time.Time{})) - }) - - t.Run("time in the past returns default 60s", func(t *testing.T) { - t.Parallel() - past := time.Now().Add(-5 * time.Minute) - assert.Equal(t, 60, ComputeRetryAfter(past)) - }) - - t.Run("future time returns delay with 1s safety buffer", func(t *testing.T) { - t.Parallel() - // 60s ahead: int(60.0)+1 = 61; allow ±2s for scheduling jitter. - future := time.Now().Add(60 * time.Second) - got := ComputeRetryAfter(future) - assert.InDelta(t, 61, got, 2.0, "expected ~61s for 60s reset with 1s buffer") - }) - - t.Run("far future is clamped to 3600s maximum", func(t *testing.T) { - t.Parallel() - assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(2*time.Hour))) - }) - - t.Run("time slightly above max delay is still clamped to 3600s", func(t *testing.T) { - t.Parallel() - // 3601s ahead: int(3601)+1 = 3602 > 3600 → clamped to 3600. - assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(3601*time.Second))) - }) -} - func TestWriteErrorResponse(t *testing.T) { tests := []struct { name string diff --git a/internal/logger/common.go b/internal/logger/global_state.go similarity index 100% rename from internal/logger/common.go rename to internal/logger/global_state.go diff --git a/internal/logger/common_test.go b/internal/logger/global_state_test.go similarity index 100% rename from internal/logger/common_test.go rename to internal/logger/global_state_test.go diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index 64dec297..98716877 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -16,6 +16,7 @@ import ( oteltrace "go.opentelemetry.io/otel/trace" "github.com/github/gh-aw-mcpg/internal/difc" + "github.com/github/gh-aw-mcpg/internal/githubhttp" "github.com/github/gh-aw-mcpg/internal/guard" "github.com/github/gh-aw-mcpg/internal/httputil" "github.com/github/gh-aw-mcpg/internal/logger" @@ -229,7 +230,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa if rateLimited, resetHeader := rateLimitSignal(resp); rateLimited { fwdSpan.SetAttributes(tracing.RateLimitHit.Bool(true)) eventAttrs := []attribute.KeyValue{} - if resetAt := httputil.ParseRateLimitResetHeader(resetHeader); !resetAt.IsZero() { + if resetAt := githubhttp.ParseRateLimitResetHeader(resetHeader); !resetAt.IsZero() { eventAttrs = append(eventAttrs, attribute.String("reset_at", resetAt.UTC().Format(time.RFC3339))) } difcSpan.AddEvent("rate_limit.detected", oteltrace.WithAttributes(eventAttrs...)) @@ -463,8 +464,8 @@ func injectRetryAfterIfRateLimited(w http.ResponseWriter, resp *http.Response) { } remaining := resp.Header.Get("X-Ratelimit-Remaining") - resetAt := httputil.ParseRateLimitResetHeader(resetHeader) - retryAfter := httputil.ComputeRetryAfter(resetAt) + resetAt := githubhttp.ParseRateLimitResetHeader(resetHeader) + retryAfter := githubhttp.ComputeRetryAfter(resetAt) w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index 5a43e61f..34f80a8f 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -16,6 +16,7 @@ import ( "github.com/github/gh-aw-mcpg/internal/config" "github.com/github/gh-aw-mcpg/internal/difc" + "github.com/github/gh-aw-mcpg/internal/githubhttp" "github.com/github/gh-aw-mcpg/internal/guard" "github.com/github/gh-aw-mcpg/internal/httputil" "github.com/github/gh-aw-mcpg/internal/logger" @@ -257,7 +258,7 @@ func (r *restBackendCaller) CallTool(ctx context.Context, toolName string, args case "get_collaborator_permission": var parseErr error - collabOwner, collabRepo, collabUsername, parseErr = httputil.ParseCollaboratorPermissionArgs(argsMap) + collabOwner, collabRepo, collabUsername, parseErr = githubhttp.ParseCollaboratorPermissionArgs(argsMap) if parseErr != nil { logProxy.Printf("restBackendCaller: get_collaborator_permission missing args (owner=%q repo=%q username=%q)", collabOwner, collabRepo, collabUsername) return nil, parseErr @@ -282,7 +283,7 @@ func (r *restBackendCaller) CallTool(ctx context.Context, toolName string, args } // For get_collaborator_permission, reuse shared REST call helper. if toolName == "get_collaborator_permission" { - result, err := httputil.FetchCollaboratorPermission( + result, err := githubhttp.FetchCollaboratorPermission( ctx, collabOwner, collabRepo, @@ -377,7 +378,7 @@ func (s *Server) forwardToGitHub(ctx context.Context, method, path string, body } else if s.githubToken != "" { authHeader = "token " + s.githubToken } - httputil.ApplyGitHubAPIHeaders(req, authHeader) + githubhttp.ApplyGitHubAPIHeaders(req, authHeader) if contentType != "" { req.Header.Set("Content-Type", contentType) } diff --git a/internal/proxy/rate_limit_test.go b/internal/proxy/rate_limit_test.go index 60696aae..c8e5c429 100644 --- a/internal/proxy/rate_limit_test.go +++ b/internal/proxy/rate_limit_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/github/gh-aw-mcpg/internal/httputil" + "github.com/github/gh-aw-mcpg/internal/githubhttp" ) // TestInjectRetryAfterIfRateLimited verifies Retry-After injection and logging for @@ -94,18 +94,18 @@ func TestComputeRetryAfter(t *testing.T) { t.Run("zero time returns default", func(t *testing.T) { t.Parallel() - assert.Equal(t, 60, httputil.ComputeRetryAfter(time.Time{})) + assert.Equal(t, 60, githubhttp.ComputeRetryAfter(time.Time{})) }) t.Run("past time returns default", func(t *testing.T) { t.Parallel() - assert.Equal(t, 60, httputil.ComputeRetryAfter(time.Now().Add(-time.Minute))) + assert.Equal(t, 60, githubhttp.ComputeRetryAfter(time.Now().Add(-time.Minute))) }) t.Run("future time returns seconds until reset", func(t *testing.T) { t.Parallel() future := time.Now().Add(30 * time.Second) - secs := httputil.ComputeRetryAfter(future) + secs := githubhttp.ComputeRetryAfter(future) // Allow ±2s for timing jitter. assert.GreaterOrEqual(t, secs, 29) assert.LessOrEqual(t, secs, 32) @@ -114,7 +114,7 @@ func TestComputeRetryAfter(t *testing.T) { t.Run("very far future is clamped to max", func(t *testing.T) { t.Parallel() farFuture := time.Now().Add(24 * time.Hour) - assert.Equal(t, 3600, httputil.ComputeRetryAfter(farFuture)) + assert.Equal(t, 3600, githubhttp.ComputeRetryAfter(farFuture)) }) } diff --git a/internal/server/rate_limit.go b/internal/server/rate_limit.go index 0d1c22cb..925da851 100644 --- a/internal/server/rate_limit.go +++ b/internal/server/rate_limit.go @@ -67,7 +67,7 @@ func isRateLimitText(text string) bool { // "API rate limit exceeded [rate reset in 42s]". // Returns zero time when the value cannot be parsed or is 0 seconds. // -// See also: httputil.ParseRateLimitResetHeader in httputil/github_http.go, which +// See also: githubhttp.ParseRateLimitResetHeader in githubhttp/client.go, which // parses the same timing information from the X-RateLimit-Reset HTTP response header // instead of MCP tool result text bodies. func parseRateLimitResetFromText(text string) time.Time { diff --git a/internal/server/unified.go b/internal/server/unified.go index ede9ca69..91e2c8fe 100644 --- a/internal/server/unified.go +++ b/internal/server/unified.go @@ -12,8 +12,8 @@ import ( "github.com/github/gh-aw-mcpg/internal/config" "github.com/github/gh-aw-mcpg/internal/difc" "github.com/github/gh-aw-mcpg/internal/envutil" + "github.com/github/gh-aw-mcpg/internal/githubhttp" "github.com/github/gh-aw-mcpg/internal/guard" - "github.com/github/gh-aw-mcpg/internal/httputil" "github.com/github/gh-aw-mcpg/internal/launcher" "github.com/github/gh-aw-mcpg/internal/logger" "github.com/github/gh-aw-mcpg/internal/mcp" @@ -279,7 +279,7 @@ func (g *guardBackendCaller) callCollaboratorPermission(ctx context.Context, arg return nil, fmt.Errorf("get_collaborator_permission: unexpected args type: %T", args) } - owner, repo, username, err := httputil.ParseCollaboratorPermissionArgs(argsMap) + owner, repo, username, err := githubhttp.ParseCollaboratorPermissionArgs(argsMap) if err != nil { logUnified.Printf("get_collaborator_permission: missing required args (owner=%q repo=%q username=%q)", owner, repo, username) return nil, err @@ -292,14 +292,14 @@ func (g *guardBackendCaller) callCollaboratorPermission(ctx context.Context, arg } apiURL := envutil.DeriveGitHubAPIURL(envutil.DefaultGitHubAPIBaseURL) - result, err := httputil.FetchCollaboratorPermission( + result, err := githubhttp.FetchCollaboratorPermission( ctx, owner, repo, username, func(ctx context.Context, apiPath string) (*http.Response, error) { logUnified.Printf("get_collaborator_permission: GET %s (for %s/%s user %s)", apiPath, owner, repo, username) - resp, err := httputil.DoGitHubGET(ctx, apiURL, apiPath, "token "+token) + resp, err := githubhttp.DoGitHubGET(ctx, apiURL, apiPath, "token "+token) if err != nil { logUnified.Printf("get_collaborator_permission: REST call failed for %s/%s user %s: %v", owner, repo, username, err) return nil, fmt.Errorf("REST call failed: %w", err) diff --git a/internal/strutil/copy_trimmed_string_int_map.go b/internal/strutil/copy_trimmed_string_int_map.go new file mode 100644 index 00000000..a6304416 --- /dev/null +++ b/internal/strutil/copy_trimmed_string_int_map.go @@ -0,0 +1,16 @@ +package strutil + +import "strings" + +// CopyTrimmedStringIntMap returns a defensive copy of a string→int map with +// whitespace trimmed from all keys. +func CopyTrimmedStringIntMap(input map[string]int) map[string]int { + if len(input) == 0 { + return nil + } + out := make(map[string]int, len(input)) + for key, value := range input { + out[strings.TrimSpace(key)] = value + } + return out +} diff --git a/internal/strutil/deduplicate.go b/internal/strutil/deduplicate.go new file mode 100644 index 00000000..ad14c6d7 --- /dev/null +++ b/internal/strutil/deduplicate.go @@ -0,0 +1,29 @@ +package strutil + +import ( + "sort" + "strings" +) + +// DeduplicateStrings returns a new slice with whitespace-trimmed, empty, and duplicate +// entries removed from input. When sorted is true the result is sorted in ascending order. +// The relative order of first-seen entries is preserved when sorted is false. +func DeduplicateStrings(input []string, sorted bool) []string { + seen := make(map[string]struct{}, len(input)) + out := make([]string, 0, len(input)) + for _, s := range input { + s = strings.TrimSpace(s) + if s == "" { + continue + } + if _, exists := seen[s]; exists { + continue + } + seen[s] = struct{}{} + out = append(out, s) + } + if sorted { + sort.Strings(out) + } + return out +} diff --git a/internal/strutil/deepclone.go b/internal/strutil/deepclone.go new file mode 100644 index 00000000..7893e166 --- /dev/null +++ b/internal/strutil/deepclone.go @@ -0,0 +1,25 @@ +package strutil + +// DeepCloneJSON creates a deep copy of a JSON-compatible value. +// It handles the three container types used by encoding/json: +// map[string]interface{} (JSON objects), []interface{} (JSON arrays), +// and any other type (JSON scalars: string, float64, bool, nil), which is +// returned as-is since scalar values are not reference types and need no cloning. +func DeepCloneJSON(v interface{}) interface{} { + switch val := v.(type) { + case map[string]interface{}: + clone := make(map[string]interface{}, len(val)) + for k, v := range val { + clone[k] = DeepCloneJSON(v) + } + return clone + case []interface{}: + clone := make([]interface{}, len(val)) + for i, v := range val { + clone[i] = DeepCloneJSON(v) + } + return clone + default: + return v + } +} diff --git a/internal/strutil/strings_to_any.go b/internal/strutil/strings_to_any.go new file mode 100644 index 00000000..fed86fc5 --- /dev/null +++ b/internal/strutil/strings_to_any.go @@ -0,0 +1,10 @@ +package strutil + +// StringsToAny converts a []string to []interface{}. +func StringsToAny(input []string) []interface{} { + out := make([]interface{}, len(input)) + for i, value := range input { + out[i] = value + } + return out +} diff --git a/internal/strutil/strutil.go b/internal/strutil/strutil.go deleted file mode 100644 index 441090ab..00000000 --- a/internal/strutil/strutil.go +++ /dev/null @@ -1,107 +0,0 @@ -package strutil - -import ( - "sort" - "strings" -) - -// SortedSetKeys returns the keys of a string set (map[string]struct{}) as a sorted slice. -// Returns an empty (non-nil) slice when the set is empty. -func SortedSetKeys(set map[string]struct{}) []string { - keys := make([]string, 0, len(set)) - for k := range set { - keys = append(keys, k) - } - sort.Strings(keys) - return keys -} - -// DeduplicateStrings returns a new slice with whitespace-trimmed, empty, and duplicate -// entries removed from input. When sorted is true the result is sorted in ascending order. -// The relative order of first-seen entries is preserved when sorted is false. -func DeduplicateStrings(input []string, sorted bool) []string { - seen := make(map[string]struct{}, len(input)) - out := make([]string, 0, len(input)) - for _, s := range input { - s = strings.TrimSpace(s) - if s == "" { - continue - } - if _, exists := seen[s]; exists { - continue - } - seen[s] = struct{}{} - out = append(out, s) - } - if sorted { - sort.Strings(out) - } - return out -} - -// StringsToAny converts a []string to []interface{}. -func StringsToAny(input []string) []interface{} { - out := make([]interface{}, len(input)) - for i, value := range input { - out[i] = value - } - return out -} - -// GetStringFromMap returns the first non-empty string value found for any of -// the given keys in m. For each key, the value must be present, typed as -// string, and non-empty to be returned. Returns an empty string when no -// matching non-empty string value is found, when the map is nil, or when no -// keys are provided. -// -// With a single key the behaviour is equivalent to `v, _ := m[key].(string)`: -// -// GetStringFromMap(m, "owner") -// -// With multiple keys the function returns the first non-empty match, which is -// useful for maps that may use either snake_case or camelCase field names: -// -// GetStringFromMap(m, "html_url", "htmlUrl") -func GetStringFromMap(m map[string]interface{}, keys ...string) string { - for _, k := range keys { - if v, ok := m[k]; ok { - if s, ok := v.(string); ok && s != "" { - return s - } - } - } - return "" -} - -// CopyTrimmedStringIntMap returns a defensive copy of a string→int map with -// whitespace trimmed from all keys. -func CopyTrimmedStringIntMap(input map[string]int) map[string]int { - if len(input) == 0 { - return nil - } - out := make(map[string]int, len(input)) - for key, value := range input { - out[strings.TrimSpace(key)] = value - } - return out -} - -// DeepCloneJSON creates a deep copy of a JSON-compatible value. -func DeepCloneJSON(v interface{}) interface{} { - switch val := v.(type) { - case map[string]interface{}: - clone := make(map[string]interface{}, len(val)) - for k, v := range val { - clone[k] = DeepCloneJSON(v) - } - return clone - case []interface{}: - clone := make([]interface{}, len(val)) - for i, v := range val { - clone[i] = DeepCloneJSON(v) - } - return clone - default: - return v - } -} diff --git a/internal/strutil/util.go b/internal/strutil/util.go new file mode 100644 index 00000000..9630466f --- /dev/null +++ b/internal/strutil/util.go @@ -0,0 +1,39 @@ +package strutil + +import "sort" + +// SortedSetKeys returns the keys of a string set (map[string]struct{}) as a sorted slice. +// Returns an empty (non-nil) slice when the set is empty. +func SortedSetKeys(set map[string]struct{}) []string { + keys := make([]string, 0, len(set)) + for k := range set { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// GetStringFromMap returns the first non-empty string value found for any of +// the given keys in m. For each key, the value must be present, typed as +// string, and non-empty to be returned. Returns an empty string when no +// matching non-empty string value is found, when the map is nil, or when no +// keys are provided. +// +// With a single key the behaviour is equivalent to `v, _ := m[key].(string)`: +// +// GetStringFromMap(m, "owner") +// +// With multiple keys the function returns the first non-empty match, which is +// useful for maps that may use either snake_case or camelCase field names: +// +// GetStringFromMap(m, "html_url", "htmlUrl") +func GetStringFromMap(m map[string]interface{}, keys ...string) string { + for _, k := range keys { + if v, ok := m[k]; ok { + if s, ok := v.(string); ok && s != "" { + return s + } + } + } + return "" +}