[WIP] Add HTTP mode. #1849
[WIP] Add HTTP mode. #1849
Conversation
Auto-generated by license-check workflow
* add readonly and toolset support * address feedback * forgotten files * remove redundant checks in WithRequestConfig * move middleware in RegisterRoutes * improve comment and add TestParseCommaSeparated * fix broken TestInventoryFiltersForRequest * parse X-MCP-Tools into ctx * clean up TestInventoryFiltersForRequest * Pass context to handler, but use request context for per-request data * Pass through the context in MCP server creation functions --------- Co-authored-by: Adam Holt <me@adamholt.co.uk>
* add readonly and toolset support * address feedback * forgotten files * remove redundant checks in WithRequestConfig * move middleware in RegisterRoutes * improve comment and add TestParseCommaSeparated * fix broken TestInventoryFiltersForRequest * parse X-MCP-Tools into ctx * clean up TestInventoryFiltersForRequest * review http args and add lockdown ctx helpers * parse lockdown header and update GetFlags to retrieve ff from ctx * clean up and fix tests * fix GetFlags check, move lockdown parsing in WithRequestConfig and fix broken tests
…r for the HTTP server.
omgitsads
force-pushed
the
http-stack-2
branch
from
42b7f64 to
97e8f35
Compare
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Looks like this is going really, a couple of questions/comments/clarifications for now. Let me know if any specific things I should look deeper at sooner rather than later.
|
|
||
| // Construct GraphQL client | ||
| // We use NewEnterpriseClient unconditionally since we already parsed the API host | ||
| gqlHTTPClient := &http.Client{ |
There was a problem hiding this comment.
We should be wrapping the transport to use the graphql features: https://github.com/github/github-mcp-server-remote/blob/a1ac0d596860727ada2477fb1e51dd765b03835b/pkg/http/mcp.go#L270
Ah I see we do so here: https://github.com/github/github-mcp-server/pull/1849/changes#diff-e722b319ba2e477b26a8285df61a7b8960f99cbd36c3c7711aa7d1d02846d1e5R20
Just need to make sure whatever we do will translate to our remote repo I guess. Ideally automatically 😍
| // MCPLockdownHeader indicates whether lockdown mode is enabled. | ||
| MCPLockdownHeader = "X-MCP-Lockdown" | ||
| // MCPFeaturesHeader is a comma-separated list of feature flags to enable. | ||
| MCPFeaturesHeader = "X-MCP-Features" |
There was a problem hiding this comment.
Probably should shove graphql feautures header here also: https://github.com/github/github-mcp-server-remote/blob/a1ac0d596860727ada2477fb1e51dd765b03835b/pkg/http/headers/http.go#L28
| errUnsupportedAuthorizationHeader = fmt.Errorf("%w: unsupported Authorization header", mark.ErrBadRequest) | ||
| ) | ||
|
|
||
| var supportedThirdPartyTokenPrefixes = []string{ |
There was a problem hiding this comment.
ThirdParty seems like a strange name
| _, token, err := parseAuthorizationHeader(r) | ||
| if err != nil { | ||
| // For missing Authorization header, return 401 with WWW-Authenticate header per MCP spec | ||
| if errors.Is(err, errMissingAuthorizationHeader) { |
There was a problem hiding this comment.
Should we add scopes here, or write a ticket to do so? I guess the following make sense here:
- https://github.com/github/copilot-mcp-core/issues/1193
- https://github.com/github/copilot-mcp-core/issues/904
And we can deal with the ability to do it within GH infra later, but perhaps add a ticket to do this for the OSS first, as we more easily can.
| return 0, "", errUnsupportedAuthorizationHeader | ||
| default: | ||
| // support both "Bearer" and "bearer" to conform to api.github.com | ||
| if len(authHeader) > 7 && strings.EqualFold(authHeader[:7], "Bearer ") { |
There was a problem hiding this comment.
We really should use strings.Equalfold I gave that feedback previously, but now it's going in this repo I feel even more strongly 😂 https://www.geeksforgeeks.org/go-language/strings-equalfold-function-in-golang-with-examples/
Maybe I'll make a PR.
| func withReadonly(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ctx := ghcontext.WithReadonly(r.Context(), true) | ||
| next.ServeHTTP(w, r.WithContext(ctx)) | ||
| }) | ||
| } | ||
|
|
||
| // withToolset is middleware that extracts the toolset from the URL and sets it in the request context | ||
| func withToolset(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| toolset := chi.URLParam(r, "toolset") | ||
| ctx := ghcontext.WithToolsets(r.Context(), []string{toolset}) | ||
| next.ServeHTTP(w, r.WithContext(ctx)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Should we be handling the header here too? I guess not, I just want to make sure we are very clear about precedence order for the URL and headers, I think URL probably wins, as it shows very clear intent, but I want to make sure we either don't change behavior (probably what is happening), or we do something very clear at least.
Summary
Adds support for Streamable HTTP mode.
Why
Addresses https://github.com/github/copilot-mcp-core/issues/1125.
What changed
httpsubcommand to the core binary.MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs