Skip to content

Commit b7a2747

Browse files
fix(registries): address Codex review — full-listing add lookup + wire opt-in registry API key
- FindServerByID now matches against the FULL registry listing instead of the 50-entry SearchServers UI cap, so a server beyond the first page is addable, not merely searchable (Codex RV #1). Forwards the ID as a server-side search hint with a full-fetch fallback; drops the dead findServerByIDLimit const. - Send the configured MCPPROXY_REGISTRY_<ID>_API_KEY as an "Authorization: Bearer" header in both request builders (generic + official protocol) so the opt-in Pulse/Smithery registries authenticate as documented instead of reading the key but querying unauthenticated (Codex RV #2). - docs/registries.md: document the Bearer scheme. - Tests: beyond-limit add lookup, key-on-wire (generic + official), no-key-no-header. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 parent d9e0016 commit b7a2747

5 files changed

Lines changed: 173 additions & 8 deletions

File tree

docs/registries.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ available via the `search_servers` / `list_registries` MCP tools, the
1616

1717
Key-requiring registries are **skipped** (not failed) when no key is configured, so
1818
a default search always succeeds. The API-key env var is
19-
`MCPPROXY_REGISTRY_<ID>_API_KEY` (ID upper-cased, non-alphanumerics → `_`).
19+
`MCPPROXY_REGISTRY_<ID>_API_KEY` (ID upper-cased, non-alphanumerics → `_`). When a
20+
key is configured it is sent on every request to that registry as an
21+
`Authorization: Bearer <key>` header.
2022

2123
User-configured registries in `mcp_config.json` (`registries: [...]`) are **merged**
2224
with these defaults (keyed by ID); a custom entry never drops the shipped set.
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
package registries
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"fmt"
7+
"net/http"
8+
"net/http/httptest"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
// TestFindServerByID_BeyondSearchLimit reproduces Codex RV #1: a server that
16+
// sits past the first page / UI limit of a registry listing must still be
17+
// addable via FindServerByID, not merely searchable. The official listing here
18+
// returns 60 entries on a single page with the target at index 55 — past the
19+
// old 50-entry add cap that SearchServers applied before matching.
20+
func TestFindServerByID_BeyondSearchLimit(t *testing.T) {
21+
const target = "io.example/target-server"
22+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
23+
items := make([]map[string]interface{}, 0, 60)
24+
for i := 0; i < 60; i++ {
25+
name := fmt.Sprintf("io.example/filler-%02d", i)
26+
if i == 55 {
27+
name = target
28+
}
29+
items = append(items, map[string]interface{}{
30+
"server": map[string]interface{}{
31+
"name": name,
32+
"description": "test server",
33+
"remotes": []interface{}{
34+
map[string]interface{}{"type": "streamable-http", "url": "https://example.com/" + name},
35+
},
36+
},
37+
})
38+
}
39+
w.Header().Set("Content-Type", "application/json")
40+
_ = json.NewEncoder(w).Encode(map[string]interface{}{
41+
"servers": items,
42+
"metadata": map[string]interface{}{}, // no nextCursor => single page
43+
})
44+
}))
45+
defer srv.Close()
46+
47+
registryList = []RegistryEntry{
48+
{ID: "official", Name: "Official", ServersURL: srv.URL, Protocol: protocolOfficial},
49+
}
50+
51+
got, err := FindServerByID(context.Background(), "official", target, nil)
52+
require.NoError(t, err)
53+
require.NotNil(t, got)
54+
assert.Equal(t, target, got.ID, "server beyond the UI/search limit must still be addable")
55+
}
56+
57+
// TestFetchServers_SendsConfiguredAPIKey covers Codex RV #2 for the generic
58+
// (non-official) request builder used by Pulse: a configured key must reach the
59+
// wire as a Bearer token, not be read-but-ignored.
60+
func TestFetchServers_SendsConfiguredAPIKey(t *testing.T) {
61+
const key = "pulse-secret-123"
62+
t.Setenv(RegistryKeyEnvVar("pulse"), key)
63+
64+
var gotAuth string
65+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66+
gotAuth = r.Header.Get("Authorization")
67+
w.Header().Set("Content-Type", "application/json")
68+
_, _ = w.Write([]byte(`{"servers":[]}`))
69+
}))
70+
defer srv.Close()
71+
72+
reg := &RegistryEntry{ID: "pulse", Protocol: "custom/pulse", ServersURL: srv.URL, RequiresKey: true}
73+
_, err := fetchServers(context.Background(), reg, nil, "")
74+
require.NoError(t, err)
75+
assert.Equal(t, "Bearer "+key, gotAuth, "configured registry key must be sent as a Bearer token")
76+
}
77+
78+
// TestFetchOfficialServers_SendsConfiguredAPIKey covers Codex RV #2 for the
79+
// official-protocol request builder used by Smithery.
80+
func TestFetchOfficialServers_SendsConfiguredAPIKey(t *testing.T) {
81+
const key = "smithery-secret-456"
82+
t.Setenv(RegistryKeyEnvVar("smithery"), key)
83+
84+
var gotAuth string
85+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
86+
gotAuth = r.Header.Get("Authorization")
87+
w.Header().Set("Content-Type", "application/json")
88+
_, _ = w.Write([]byte(`{"servers":[],"metadata":{}}`))
89+
}))
90+
defer srv.Close()
91+
92+
reg := &RegistryEntry{ID: "smithery", Protocol: protocolOfficial, ServersURL: srv.URL, RequiresKey: true}
93+
_, err := fetchOfficialServers(context.Background(), reg, nil, "")
94+
require.NoError(t, err)
95+
assert.Equal(t, "Bearer "+key, gotAuth, "configured registry key must be sent as a Bearer token")
96+
}
97+
98+
// TestFetchServers_NoKeyNoAuthHeader ensures no Authorization header is attached
99+
// when no key is configured (never send a bare "Bearer ").
100+
func TestFetchServers_NoKeyNoAuthHeader(t *testing.T) {
101+
t.Setenv(RegistryKeyEnvVar("pulse"), "")
102+
103+
var gotAuth string
104+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
105+
gotAuth = r.Header.Get("Authorization")
106+
w.Header().Set("Content-Type", "application/json")
107+
_, _ = w.Write([]byte(`{"servers":[]}`))
108+
}))
109+
defer srv.Close()
110+
111+
reg := &RegistryEntry{ID: "pulse", Protocol: "custom/pulse", ServersURL: srv.URL}
112+
_, err := fetchServers(context.Background(), reg, nil, "")
113+
require.NoError(t, err)
114+
assert.Empty(t, gotAuth, "no Authorization header should be sent without a configured key")
115+
}

internal/registries/official.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ func fetchOfficialServers(ctx context.Context, reg *RegistryEntry, guesser *expe
5454
req.Header.Set("Accept", "application/json")
5555
// Some registries reject empty/bare User-Agents (issue #566).
5656
req.Header.Set("User-Agent", registryUserAgent())
57+
// Opt-in official-protocol registries (e.g. Smithery) authenticate via
58+
// their configured key.
59+
applyRegistryAuth(req, reg)
5760

5861
resp, err := client.Do(req)
5962
if err != nil {

internal/registries/registry_key.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package registries
33
import (
44
"errors"
55
"fmt"
6+
"net/http"
67
"os"
78
"strings"
89
)
@@ -38,6 +39,17 @@ func registryAPIKey(reg *RegistryEntry) string {
3839
return os.Getenv(RegistryKeyEnvVar(reg.ID))
3940
}
4041

42+
// applyRegistryAuth attaches the configured API key for a registry to the
43+
// outgoing request as a Bearer token. Registries with no configured key are
44+
// left unauthenticated. Bearer is the scheme used by the opt-in registries we
45+
// ship (Smithery, Pulse) and makes the documented MCPPROXY_REGISTRY_<ID>_API_KEY
46+
// contract actually take effect on the wire rather than being read-but-ignored.
47+
func applyRegistryAuth(req *http.Request, reg *RegistryEntry) {
48+
if key := registryAPIKey(reg); key != "" {
49+
req.Header.Set("Authorization", "Bearer "+key)
50+
}
51+
}
52+
4153
// checkRegistryKey enforces FR-008: when a registry requires a key and none is
4254
// configured, it returns a wrapped ErrRegistryKeyMissing naming the env var to
4355
// set. Returns nil when the registry needs no key or one is present.

internal/registries/search.go

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,6 @@ var (
2121
ErrServerNotFound = errors.New("server not found in registry")
2222
)
2323

24-
// findServerByIDLimit caps how many servers FindServerByID fetches before
25-
// matching. Most registries return far fewer; the resilience phase (US4) can
26-
// refine pagination if a target sits beyond this window.
27-
const findServerByIDLimit = 50
28-
2924
// Constants for repeated strings
3025
const (
3126
protocolDocker = "custom/docker"
@@ -114,15 +109,51 @@ func SearchServers(ctx context.Context, registryID, tag, query string, limit int
114109
// every add-from-registry surface (CN-001/CN-004). Returns ErrRegistryNotFound
115110
// when registryID does not resolve and ErrServerNotFound when no server matches.
116111
func FindServerByID(ctx context.Context, registryID, serverID string, guesser *experiments.Guesser) (*ServerEntry, error) {
117-
if FindRegistry(registryID) == nil {
112+
reg := FindRegistry(registryID)
113+
if reg == nil {
118114
return nil, ErrRegistryNotFound
119115
}
120116

121-
servers, err := SearchServers(ctx, registryID, "", "", findServerByIDLimit, guesser)
117+
// Honor key-requiring registries (FR-008) on the add path too.
118+
if err := checkRegistryKey(reg); err != nil {
119+
return nil, err
120+
}
121+
122+
// Match against the FULL registry listing, never the UI/search limit: a
123+
// server beyond the first page of results must still be addable. Routing the
124+
// add path through SearchServers truncated the listing to 50 before
125+
// matching, so any entry past the first page was searchable but not addable
126+
// (Codex RV #1).
127+
match, err := findServerByIDFetch(ctx, reg, serverID)
122128
if err != nil {
123129
return nil, err
124130
}
125131

132+
// Enrich only the single matched entry (cheap) rather than the whole listing.
133+
if guesser != nil {
134+
if enriched := applyBatchRepositoryGuessing(ctx, []ServerEntry{*match}, guesser); len(enriched) > 0 {
135+
match = &enriched[0]
136+
}
137+
}
138+
match.Registry = reg.Name
139+
return match, nil
140+
}
141+
142+
// findServerByIDFetch resolves serverID against a registry's full listing. It
143+
// first forwards serverID as a server-side `search` hint (cheap for the
144+
// paginating official protocol) and falls back to a full unfiltered fetch when
145+
// the hinted search does not surface the exact entry — so the match is found
146+
// regardless of its position in the listing.
147+
func findServerByIDFetch(ctx context.Context, reg *RegistryEntry, serverID string) (*ServerEntry, error) {
148+
if servers, err := fetchServers(ctx, reg, nil, serverID); err == nil {
149+
if match, err := findServerByIDIn(servers, serverID); err == nil {
150+
return match, nil
151+
}
152+
}
153+
servers, err := fetchServers(ctx, reg, nil, "")
154+
if err != nil {
155+
return nil, err
156+
}
126157
return findServerByIDIn(servers, serverID)
127158
}
128159

@@ -164,6 +195,8 @@ func fetchServers(ctx context.Context, reg *RegistryEntry, guesser *experiments.
164195
// Some registries (e.g. Pulse, issue #566) reject requests with an empty
165196
// or bare User-Agent and require a versioned one (mirror guesser.go).
166197
req.Header.Set("User-Agent", registryUserAgent())
198+
// Opt-in registries (RequiresKey) authenticate via their configured key.
199+
applyRegistryAuth(req, reg)
167200

168201
resp, err := client.Do(req)
169202
if err != nil {

0 commit comments

Comments
 (0)