feat(mcpclient): fail-fast on empty bearer token
mcpclient.New previously accepted an empty token and silently omitted the Authorization header at request time. When the env var sourcing the token was missing from a Kubernetes Secret (envFrom doesn't warn on missing keys), this surfaced as an opaque 401 from the upstream MCP server with no log trail — see hyperguild #13 and brain entry "mcpclient-empty-token-silent-401-envfrom-missing-key". mcpclient.New now returns ErrTokenRequired when token is empty. The routing pod's project_create init checks the error and exits with a clear message pointing at routing-secrets, turning a runtime 401 storm into a startup crashloop the operator can fix immediately. Tests pass a dummy "test" token (httptest servers don't enforce bearer auth, so any non-empty value works). Added a regression test asserting empty-token construction returns ErrTokenRequired. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -103,12 +103,17 @@ func main() {
|
|||||||
}))
|
}))
|
||||||
|
|
||||||
if cfg.GiteaMCPURL != "" {
|
if cfg.GiteaMCPURL != "" {
|
||||||
|
mcpC, err := mcpclient.New(cfg.GiteaMCPURL, cfg.GiteaMCPToken)
|
||||||
|
if err != nil {
|
||||||
|
logger.Error("mcpclient init for project_create — GITEA_MCP_URL is set but GITEA_MCP_TOKEN is empty (check routing-secrets)", "err", err)
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
var ghClient *githubclient.Client
|
var ghClient *githubclient.Client
|
||||||
if cfg.GitHubPAT != "" {
|
if cfg.GitHubPAT != "" {
|
||||||
ghClient = githubclient.New(cfg.GitHubPAT)
|
ghClient = githubclient.New(cfg.GitHubPAT)
|
||||||
}
|
}
|
||||||
reg.Register(project.New(project.Config{
|
reg.Register(project.New(project.Config{
|
||||||
Client: mcpclient.New(cfg.GiteaMCPURL, cfg.GiteaMCPToken),
|
Client: mcpC,
|
||||||
GitHub: ghClient,
|
GitHub: ghClient,
|
||||||
GiteaOwner: cfg.GiteaOwner,
|
GiteaOwner: cfg.GiteaOwner,
|
||||||
GitHubOwner: cfg.GitHubOwner,
|
GitHubOwner: cfg.GitHubOwner,
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import (
|
|||||||
"bytes"
|
"bytes"
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"net/http"
|
"net/http"
|
||||||
@@ -20,13 +21,27 @@ type Client struct {
|
|||||||
http *http.Client
|
http *http.Client
|
||||||
}
|
}
|
||||||
|
|
||||||
// New returns a Client. token may be empty for unauthenticated servers.
|
// ErrTokenRequired is returned by New when token is empty. Empty token
|
||||||
func New(url, token string) *Client {
|
// causes mcpclient to omit the Authorization header at request time,
|
||||||
|
// which is silently misread as 401 by bearer-auth servers — see
|
||||||
|
// hyperguild #13 and the brain entry on the failure mode.
|
||||||
|
var ErrTokenRequired = errors.New("mcpclient: token required")
|
||||||
|
|
||||||
|
// New returns a Client. Returns ErrTokenRequired when token is empty:
|
||||||
|
// every MCP server we talk to today is bearer-protected, and an empty
|
||||||
|
// token is always a configuration bug (typically a Kubernetes Secret
|
||||||
|
// missing the expected key, see hyperguild #13). Callers that genuinely
|
||||||
|
// need an unauthenticated client should construct &Client{} directly in
|
||||||
|
// tests, not call New.
|
||||||
|
func New(url, token string) (*Client, error) {
|
||||||
|
if token == "" {
|
||||||
|
return nil, ErrTokenRequired
|
||||||
|
}
|
||||||
return &Client{
|
return &Client{
|
||||||
url: url,
|
url: url,
|
||||||
token: token,
|
token: token,
|
||||||
http: &http.Client{Timeout: 60 * time.Second},
|
http: &http.Client{Timeout: 60 * time.Second},
|
||||||
}
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// WithHTTPClient overrides the underlying HTTP client (test injection).
|
// WithHTTPClient overrides the underlying HTTP client (test injection).
|
||||||
|
|||||||
@@ -14,6 +14,13 @@ import (
|
|||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
func TestNew_EmptyTokenFailsFast(t *testing.T) {
|
||||||
|
c, err := mcpclient.New("http://example.invalid", "")
|
||||||
|
require.Error(t, err)
|
||||||
|
require.Nil(t, c)
|
||||||
|
require.ErrorIs(t, err, mcpclient.ErrTokenRequired)
|
||||||
|
}
|
||||||
|
|
||||||
func TestCallTool_Success(t *testing.T) {
|
func TestCallTool_Success(t *testing.T) {
|
||||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
assert.Equal(t, http.MethodPost, r.Method)
|
assert.Equal(t, http.MethodPost, r.Method)
|
||||||
@@ -30,13 +37,13 @@ func TestCallTool_Success(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := mcpclient.New(srv.URL, "tok")
|
c, err := mcpclient.New(srv.URL, "tok")
|
||||||
|
require.NoError(t, err)
|
||||||
var out struct {
|
var out struct {
|
||||||
OK bool `json:"ok"`
|
OK bool `json:"ok"`
|
||||||
N int `json:"n"`
|
N int `json:"n"`
|
||||||
}
|
}
|
||||||
err := c.CallTool(context.Background(), "x_y", map[string]any{"a": 1}, &out)
|
require.NoError(t, c.CallTool(context.Background(), "x_y", map[string]any{"a": 1}, &out))
|
||||||
require.NoError(t, err)
|
|
||||||
assert.True(t, out.OK)
|
assert.True(t, out.OK)
|
||||||
assert.Equal(t, 7, out.N)
|
assert.Equal(t, 7, out.N)
|
||||||
}
|
}
|
||||||
@@ -48,8 +55,9 @@ func TestCallTool_RPCError(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := mcpclient.New(srv.URL, "")
|
c, err := mcpclient.New(srv.URL, "test")
|
||||||
err := c.CallTool(context.Background(), "x", nil, nil)
|
require.NoError(t, err)
|
||||||
|
err = c.CallTool(context.Background(), "x", nil, nil)
|
||||||
require.Error(t, err)
|
require.Error(t, err)
|
||||||
var me *mcpclient.Error
|
var me *mcpclient.Error
|
||||||
require.True(t, errors.As(err, &me))
|
require.True(t, errors.As(err, &me))
|
||||||
@@ -64,8 +72,9 @@ func TestCallTool_HTTPError(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := mcpclient.New(srv.URL, "")
|
c, err := mcpclient.New(srv.URL, "test")
|
||||||
err := c.CallTool(context.Background(), "x", nil, nil)
|
require.NoError(t, err)
|
||||||
|
err = c.CallTool(context.Background(), "x", nil, nil)
|
||||||
require.Error(t, err)
|
require.Error(t, err)
|
||||||
assert.Contains(t, err.Error(), "401")
|
assert.Contains(t, err.Error(), "401")
|
||||||
}
|
}
|
||||||
@@ -77,6 +86,7 @@ func TestCallTool_NilResult(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := mcpclient.New(srv.URL, "")
|
c, err := mcpclient.New(srv.URL, "test")
|
||||||
|
require.NoError(t, err)
|
||||||
require.NoError(t, c.CallTool(context.Background(), "x", nil, nil))
|
require.NoError(t, c.CallTool(context.Background(), "x", nil, nil))
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -125,7 +125,7 @@ func newSkill(t *testing.T, f *fakeGiteaMCP) (*project.Skill, *fakeGitHub) {
|
|||||||
t.Cleanup(ghSrv.Close)
|
t.Cleanup(ghSrv.Close)
|
||||||
|
|
||||||
return project.New(project.Config{
|
return project.New(project.Config{
|
||||||
Client: mcpclient.New(srv.URL, ""),
|
Client: mustClient(t, srv.URL),
|
||||||
GitHub: githubclient.New("ghp_test").WithBaseURL(ghSrv.URL),
|
GitHub: githubclient.New("ghp_test").WithBaseURL(ghSrv.URL),
|
||||||
GiteaOwner: "mathias",
|
GiteaOwner: "mathias",
|
||||||
GitHubOwner: "mathiasb",
|
GitHubOwner: "mathiasb",
|
||||||
@@ -141,13 +141,23 @@ func newSkillNoGitHub(t *testing.T, f *fakeGiteaMCP) *project.Skill {
|
|||||||
srv := httptest.NewServer(f.handler())
|
srv := httptest.NewServer(f.handler())
|
||||||
t.Cleanup(srv.Close)
|
t.Cleanup(srv.Close)
|
||||||
return project.New(project.Config{
|
return project.New(project.Config{
|
||||||
Client: mcpclient.New(srv.URL, ""),
|
Client: mustClient(t, srv.URL),
|
||||||
GiteaOwner: "mathias",
|
GiteaOwner: "mathias",
|
||||||
GitHubOwner: "mathiasb",
|
GitHubOwner: "mathiasb",
|
||||||
InfraRepo: "infra",
|
InfraRepo: "infra",
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// mustClient builds an mcpclient against an httptest server. Uses a
|
||||||
|
// non-empty dummy token because httptest servers don't enforce bearer
|
||||||
|
// auth, but mcpclient.New now requires non-empty token (see #13).
|
||||||
|
func mustClient(t *testing.T, url string) *mcpclient.Client {
|
||||||
|
t.Helper()
|
||||||
|
c, err := mcpclient.New(url, "test-token")
|
||||||
|
require.NoError(t, err)
|
||||||
|
return c
|
||||||
|
}
|
||||||
|
|
||||||
func happyArgs() json.RawMessage {
|
func happyArgs() json.RawMessage {
|
||||||
return json.RawMessage(`{
|
return json.RawMessage(`{
|
||||||
"name":"my-experiment",
|
"name":"my-experiment",
|
||||||
|
|||||||
Reference in New Issue
Block a user