feat: http mode for centralized deployments #1907
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add InsiderMode field to HTTPServerConfig and pass it from CLI flags. This ensures that the --insider-mode flag works for HTTP mode just like it does for stdio mode, enabling experimental features when set. Co-authored-by: atharva1051 <53966412+atharva1051@users.noreply.github.com>
Co-authored-by: atharva1051 <53966412+atharva1051@users.noreply.github.com>
Add InsiderMode support to HTTP server mode
need health check at lb level Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
feat: http mode for centralized deployments
There was a problem hiding this comment.
Pull request overview
Adds an HTTP server transport mode to support centralized, multi-tenant deployments where each client authenticates per request using an Authorization: Bearer <token> header.
Changes:
- Introduces
httpsubcommand andRunHTTPServer()using MCP SDK’s streamable HTTP handler with per-request token extraction. - Adds HTTP-specific configuration (
HTTPServerConfig) and container exposure for the default HTTP port. - Documents HTTP mode usage, client configuration (VS Code), and deployment/security considerations in README.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ghmcp/server.go | Adds HTTP server config, token extraction helper, and RunHTTPServer() implementation. |
| cmd/github-mcp-server/main.go | Adds http subcommand and --port flag wiring into HTTPServerConfig. |
| README.md | Documents HTTP server mode, auth model, deployment options, and troubleshooting. |
| Dockerfile | Exposes port 8080 for HTTP mode. |
| go.mod | Removes unused color-related dependencies. |
| go.sum | Updates dependency checksums to match go.mod changes. |
| authHeader := req.Header.Get("Authorization") | ||
| if authHeader == "" { | ||
| return "" | ||
| } | ||
|
|
||
| // Check for "Bearer " prefix | ||
| const bearerPrefix = "Bearer " | ||
| if strings.HasPrefix(authHeader, bearerPrefix) { | ||
| return strings.TrimPrefix(authHeader, bearerPrefix) | ||
| } | ||
|
|
||
| return "" |
There was a problem hiding this comment.
extractTokenFromAuthHeader returns the remainder of the header verbatim after "Bearer ", which means values like "Bearer <token>" (extra spaces) or a trailing space will produce an invalid token. It also treats the auth scheme as case-sensitive even though HTTP auth schemes are case-insensitive. Consider trimming whitespace around the extracted token and parsing the scheme case-insensitively (e.g., split on the first space and strings.EqualFold on the scheme).
| authHeader := req.Header.Get("Authorization") | |
| if authHeader == "" { | |
| return "" | |
| } | |
| // Check for "Bearer " prefix | |
| const bearerPrefix = "Bearer " | |
| if strings.HasPrefix(authHeader, bearerPrefix) { | |
| return strings.TrimPrefix(authHeader, bearerPrefix) | |
| } | |
| return "" | |
| authHeader := strings.TrimSpace(req.Header.Get("Authorization")) | |
| if authHeader == "" { | |
| return "" | |
| } | |
| // Split into scheme and credentials, and compare scheme case-insensitively. | |
| parts := strings.SplitN(authHeader, " ", 2) | |
| if len(parts) != 2 { | |
| return "" | |
| } | |
| if !strings.EqualFold(parts[0], "Bearer") { | |
| return "" | |
| } | |
| token := strings.TrimSpace(parts[1]) | |
| if token == "" { | |
| return "" | |
| } | |
| return token |
| // extractTokenFromAuthHeader extracts a GitHub token from the Authorization header. | ||
| // It supports "Bearer <token>" format. | ||
| func extractTokenFromAuthHeader(req *http.Request) string { | ||
| authHeader := req.Header.Get("Authorization") | ||
| if authHeader == "" { | ||
| return "" | ||
| } | ||
|
|
||
| // Check for "Bearer " prefix | ||
| const bearerPrefix = "Bearer " | ||
| if strings.HasPrefix(authHeader, bearerPrefix) { | ||
| return strings.TrimPrefix(authHeader, bearerPrefix) | ||
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
The new HTTP mode introduces new authentication parsing/behavior (extractTokenFromAuthHeader) but there are no unit tests covering common/edge cases (missing header, wrong scheme, extra whitespace, etc.). Adding a small table-driven test in internal/ghmcp/server_test.go would help prevent regressions.
| // Parse toolsets | ||
| var enabledToolsets []string | ||
| if viper.IsSet("toolsets") { | ||
| if err := viper.UnmarshalKey("toolsets", &enabledToolsets); err != nil { | ||
| return fmt.Errorf("failed to unmarshal toolsets: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The toolsets/tools/features parsing logic is now duplicated across stdio, http, and other commands (e.g., list_scopes). This increases the risk of future behavior drifting between commands. Consider extracting a shared helper (e.g., parseEnabledToolsets/Tools/Features() or a single function returning a struct) and using it from each command.
| - Bind to localhost (`127.0.0.1`) for local-only access | ||
| - Use firewalls to restrict access to trusted networks |
There was a problem hiding this comment.
The README suggests users can "Bind to localhost (127.0.0.1)" for local-only access, but the implementation currently always binds to all interfaces via Addr=":<port>". Either update the docs to reflect that binding must be handled externally (reverse proxy / container/network config), or add a flag/config option to set the bind address (e.g., --listen-addr).
| - Bind to localhost (`127.0.0.1`) for local-only access | |
| - Use firewalls to restrict access to trusted networks | |
| - The HTTP server listens on all interfaces; restrict exposure using your deployment configuration (Docker port mappings, reverse proxy bind address, and host firewalls) | |
| - For local-only access, publish the container/host port only on localhost (for Docker: `-p 127.0.0.1:8080:8080`) or bind your reverse proxy to `127.0.0.1` |
| if cfg.LogFilePath != "" { | ||
| file, err := os.OpenFile(cfg.LogFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open log file: %w", err) | ||
| } | ||
| defer file.Close() | ||
| logOutput = file | ||
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelDebug}) | ||
| } else { | ||
| logOutput = os.Stderr | ||
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) | ||
| } | ||
| logger := slog.New(slogHandler) |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
| if cfg.LogFilePath != "" { | |
| file, err := os.OpenFile(cfg.LogFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) | |
| if err != nil { | |
| return fmt.Errorf("failed to open log file: %w", err) | |
| } | |
| defer file.Close() | |
| logOutput = file | |
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelDebug}) | |
| } else { | |
| logOutput = os.Stderr | |
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) | |
| } | |
| logger := slog.New(slogHandler) | |
| var logger *slog.Logger | |
| if cfg.LogFilePath != "" { | |
| file, err := os.OpenFile(cfg.LogFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) | |
| if err != nil { | |
| return fmt.Errorf("failed to open log file: %w", err) | |
| } | |
| defer func() { | |
| if cerr := file.Close(); cerr != nil { | |
| if logger != nil { | |
| logger.Error("failed to close log file", "error", cerr) | |
| } else { | |
| fmt.Fprintf(os.Stderr, "failed to close log file: %v\n", cerr) | |
| } | |
| } | |
| }() | |
| logOutput = file | |
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelDebug}) | |
| } else { | |
| logOutput = os.Stderr | |
| slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) | |
| } | |
| logger = slog.New(slogHandler) |
1 participant
Summary
Adds HTTP server mode enabling multi-tenant deployments where each client provides their own GitHub token via
Authorization: Bearer <token>header.Why
Enterprise scenarios require centralized MCP infrastructure serving multiple authenticated clients concurrently, which stdio mode's single-token-per-process model cannot support.
What changed
httpsubcommand with--portflag (default 8080)RunHTTPServer()using MCP SDK'sStreamableHTTPHandlerAuthorizationheaderHTTPServerConfigstruct for HTTP-specific configurationMCP impact
HTTP mode uses existing tools—only the transport and authentication model changed.
Prompts tested (tool changes only)
N/A
Security / limits
Tokens validated per-request, never stored. Each client's token creates isolated server instance with their permissions. Token scope filtering applied for PAT tokens.
Tool renaming
Lint & tests
./script/lint./script/testDocs
Added HTTP mode section to README with client configuration examples (VS Code), Docker deployment patterns (basic, production with docker-compose), and troubleshooting guide.
The program was tested solely for our own use cases, which might differ from yours.
Atharva Patil <atharva.a.patil@mercedes-benz.com> on behalf of Mercedes-Benz Research And Development India, Provider Information