Skip to content

Commit f0f0eb1

Browse files
neylwaleckineylwalecki
authored
fix(runtime): require an access token for OAuth status (#1725)
## Description `Runtime.GetAllServers` treats a persisted DCR registration as a valid OAuth token even when `AccessToken` is empty. That makes the status payload report `authenticated=true` and `oauth.token_valid=true`, and the CLI print `Token: Valid`, while the token store correctly returns `ErrNoToken`. Require a non-empty access token before projecting authentication and expiry, matching `PersistentTokenStore.GetToken`. A DCR-only record still identifies an OAuth server, including autodiscovery, so it keeps the login action. The read preserves the registration. Actual non-expiring tokens, expired-token semantics, and static-header authentication are unchanged. Fixes #1723. ### Regression proof The test creates registrations through `UpdateOAuthClientCredentials` in temporary BBolt storage and exercises the real runtime projection. Five scenarios run in both explicit and autodiscovery modes: - DCR registration only. - Empty access token despite future expiry and refresh metadata. - Real non-expiring access token. - Real unexpired access token. - Real expired access token. On base `14cb288f`, the four empty-access-token scenarios fail (`authenticated` is true instead of false, `oauth_status` is authenticated instead of none); the six real-token controls pass. With the fix, all ten pass. The test also checks `ErrNoToken` for the incomplete records, the login action, absence of access-token expiry claims, and preservation of the registration. ### Scope This changes the runtime status projection only. It does not change token storage, refresh, or OAuth instance retirement. #1722 separately tracks pending logins being retired by replacement and callback UX; the retirement barrier remains intact. All fixtures are synthetic and use `.invalid`; no real providers or credentials are involved. ## Testing - [x] Tested locally with the regression red/green cycle described above. - [x] Added tests that demonstrate the fix and preserve the real-token controls. - [x] Existing tests pass in the scope below; full upstream CI is pending. Go 1.26.0, darwin/arm64: ```sh go test -short -race -p 2 -timeout 20m -skip 'E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint' ./internal/... go test -short -race -p 2 -tags server -timeout 10m ./internal/serveredition/... go build -o mcpproxy ./cmd/mcpproxy go build -tags server -o mcpproxy-server ./cmd/mcpproxy golangci-lint run --config .github/.golangci.yml ./... golangci-lint run --config .github/.golangci.yml --build-tags server ./... ``` Results: 79 base packages and six Server-edition packages passed with race detection; both builds passed; the CI-pinned golangci-lint v2.9.0 reported zero issues for both editions. The full runtime and OAuth packages also passed with race on Go 1.27.3 (`go test -race ./internal/runtime ./internal/oauth -count=1 -timeout=10m`). The REST smoke from `scripts/test-api-e2e.sh` passed 70/70, including the Server-edition audit companion. The harness assertions were unchanged; its scratch setup used loopback listeners, disabled local sockets/tray, and a dedicated npm cache. The first smoke attempt could not start the `everything` fixture because the global npm cache returned `EACCES`; using the dedicated cache resolved that setup failure without changing global permissions.
1 parent 4f4e1bb commit f0f0eb1

2 files changed

Lines changed: 104 additions & 2 deletions

File tree

‎internal/runtime/dcr_oauth_token_health_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package runtime
2+
3+
import (
4+
"context"
5+
"testing"
6+
"time"
7+
8+
"github.com/mark3labs/mcp-go/client/transport"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
12+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
13+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts"
14+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/health"
15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth"
16+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/stateview"
17+
)
18+
19+
// DCR persists a client registration before a login obtains an access token.
20+
// A zero expiry makes a real token long-lived, but cannot make a registration
21+
// authenticated. The server projection must agree with PersistentTokenStore.
22+
func TestGetAllServers_DCRRequiresAccessToken(t *testing.T) {
23+
for _, explicit := range []bool{false, true} {
24+
mode := "autodiscovery"
25+
if explicit {
26+
mode = "explicit"
27+
}
28+
t.Run(mode, func(t *testing.T) {
29+
for _, tc := range []struct {
30+
name string
31+
accessToken string
32+
refreshToken string
33+
expiresAt time.Time
34+
authenticated bool
35+
status string
36+
valid bool
37+
}{
38+
{name: "DCR only", status: "none"},
39+
{name: "no access token with expiry and refresh", refreshToken: "synthetic-refresh", expiresAt: time.Now().Add(time.Hour), status: "none"},
40+
{name: "non-expiring token", accessToken: "synthetic-access", authenticated: true, status: "authenticated", valid: true},
41+
{name: "unexpired token", accessToken: "synthetic-access", expiresAt: time.Now().Add(time.Hour), authenticated: true, status: "authenticated", valid: true},
42+
{name: "expired token", accessToken: "synthetic-access", expiresAt: time.Now().Add(-time.Hour), authenticated: true, status: "expired"},
43+
} {
44+
t.Run(tc.name, func(t *testing.T) {
45+
rt := newStaleTokenRuntime(t)
46+
sc := &config.ServerConfig{
47+
Name: "dcr-fixture", URL: "https://mcp.example.invalid/mcp",
48+
Protocol: "http", Enabled: true,
49+
}
50+
if explicit {
51+
sc.OAuth = &config.OAuthConfig{PKCEEnabled: true}
52+
}
53+
db := rt.storageManager.GetBoltDB()
54+
key := oauth.GenerateServerKey(sc.Name, sc.URL)
55+
require.NoError(t, db.UpdateOAuthClientCredentials(key, "synthetic-client", "", 0, ""))
56+
record, err := db.GetOAuthToken(key)
57+
require.NoError(t, err)
58+
if tc.accessToken != "" || tc.refreshToken != "" || !tc.expiresAt.IsZero() {
59+
record.AccessToken = tc.accessToken
60+
record.RefreshToken = tc.refreshToken
61+
record.ExpiresAt = tc.expiresAt
62+
require.NoError(t, db.SaveOAuthToken(record))
63+
}
64+
if tc.accessToken == "" {
65+
store := oauth.NewPersistentTokenStore(sc.Name, sc.URL, db)
66+
token, err := store.GetToken(context.Background())
67+
require.ErrorIs(t, err, transport.ErrNoToken)
68+
require.Nil(t, token)
69+
}
70+
rt.Supervisor().StateView().UpdateServer(sc.Name, func(s *stateview.ServerStatus) {
71+
s.Name, s.Config, s.Enabled, s.State = sc.Name, sc, true, "pending_auth"
72+
})
73+
74+
server := findServer(t, rt, sc.Name)
75+
assert.Equal(t, tc.authenticated, server["authenticated"])
76+
assert.Equal(t, tc.status, server["oauth_status"])
77+
oauthConfig, ok := server["oauth"].(map[string]interface{})
78+
require.True(t, ok, "a client registration still identifies an OAuth server")
79+
assert.Equal(t, tc.valid, oauthConfig["token_valid"] == true)
80+
if tc.accessToken == "" {
81+
assert.NotContains(t, server, "token_expires_at")
82+
assert.NotContains(t, oauthConfig, "token_expires_at")
83+
hs, ok := server["health"].(*contracts.HealthStatus)
84+
require.True(t, ok)
85+
assert.Equal(t, health.ActionLogin, hs.Action)
86+
}
87+
stored, err := db.GetOAuthToken(key)
88+
require.NoError(t, err)
89+
assert.Equal(t, "synthetic-client", stored.ClientID, "status reads preserve the registration")
90+
assert.Equal(t, tc.accessToken, stored.AccessToken)
91+
})
92+
}
93+
})
94+
}
95+
}

‎internal/runtime/runtime.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2795,7 +2795,9 @@ func (r *Runtime) GetAllServers() ([]map[string]interface{}, error) {
27952795
zap.Bool("token_nil", token == nil),
27962796
zap.Error(err))
27972797

2798-
if err == nil && token != nil {
2798+
// DCR persists client credentials before the code exchange. Like
2799+
// PersistentTokenStore.GetToken, require an actual access token.
2800+
if err == nil && token != nil && token.AccessToken != "" {
27992801
authenticated = true
28002802
tokenExpiresAt = token.ExpiresAt
28012803
hasRefreshToken = token.RefreshToken != ""
@@ -2829,7 +2831,12 @@ func (r *Runtime) GetAllServers() ([]map[string]interface{}, error) {
28292831
oauthStatus = string(oauth.OAuthStatusAuthenticated)
28302832
}
28312833
} else {
2832-
// No token found - check if OAuth config exists to determine status
2834+
// A DCR-only record still identifies an autodiscovered OAuth
2835+
// server, but is not evidence of a completed login.
2836+
if err == nil && token != nil && oauthConfig == nil {
2837+
oauthConfig = map[string]interface{}{"autodiscovery": true}
2838+
}
2839+
// No access token - check if OAuth config exists to determine status
28332840
if oauthConfig != nil {
28342841
oauthStatus = string(oauth.OAuthStatusNone)
28352842
}

0 commit comments

Comments
 (0)