opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-10-16 14:48:56 +08:00
Code Issues Packages Projects Releases Wiki Activity
Files
main
coder/coderd/mcp_internal_test.go
T
Michael Suchacz fa7cd62c8b feat: send RFC 8707 resource parameter on MCP server OAuth2 flows (#30651) 
Coder's MCP OAuth2 client never sent the RFC 8707 `resource` parameter,
which the MCP authorization spec requires on authorization and token
requests. Authorization servers that enforce it cannot be connected:
with Dagster+, discovery and dynamic client registration succeed, then
the authorize request fails with "MCP OAuth requests must include
resource".

MCP server configs get an `oauth2_resource` field (migration 000618,
admin-only in API responses, audited). Coder sends it on the authorize
redirect, the authorization-code exchange, and token refresh. Refresh
now goes through `clientcredentials` with `grant_type=refresh_token`,
because `oauth2.Config` cannot add parameters to refresh requests; it
keeps the same auth-style detection and error types, and adds no
dependency.

Auto-discovery fills the field with the most specific Protected Resource
Metadata `resource` that covers the server URL, stored as advertised. As
in the MCP SDKs, a resource covers URLs on its origin at or under its
path, so `/mcp` covers `/mcp/` but `/mcp/` does not cover `/mcp`. URLs
compare in RFC 3986 normal form (default ports, percent-encodings, dot
segments, and an empty path as `/`), ports ignore leading zeros, IPv6
literals compare by address, and `%2F` stays inside its segment. Unlike
the SDKs, a resource with a query, even an empty one, only covers a
server URL with the same query. When none covers it, Coder uses the
server URL with the scheme and host lowercased and user information and
fragment removed, or sends no resource when that is not a valid resource
indicator. An explicit value in the create request wins.

Manual configurations send no `resource` unless an admin sets one,
because some providers reject it (Microsoft Entra ID v2 fails with
AADSTS901002). Values must be absolute RFC 3986 URIs without a fragment,
user information, or an IPv6 zone identifier.

The resource binds grants like the other OAuth settings: changing it
deletes stored user tokens, and a callback whose config resource changed
after the user started connecting returns 409 before the code is
redeemed. The connect step records the resource in a callback-scoped
cookie for that check.

Discovery only runs when a configuration is created, so existing
auto-discovered configurations, including the Dagster+ one that prompted
this, keep an empty resource until an admin sets the new "Resource URL"
field or re-creates the server.

![MCP server edit form showing the Resource URL field filled by
auto-discovery](https://github.com/user-attachments/assets/319bc8a0-f6e1-4b7d-8878-8e341c94e590)

## Testing

- Go: discovery (most specific match, path and trailing-slash
boundaries, query matching including empty queries, equivalent ports,
IPv6 literals, escapes, and dot segments including escaped ones, the
normalized fallback and an invalid one, explicit override), validation
(URI characters, brackets, IP literals, user information, trimming),
grant deletion on change, clearing on an auth type change, redaction for
members, refresh with and without a resource, the 409 when the resource
changes mid-flow (the authorization server never receives the code), and
a connect and callback flow against an authorization server that rejects
requests without the resource. Each new test was checked to fail when
the behavior it covers is broken, except two malformed IP literal hosts
that Go's URL parser already rejects.
- Vitest: the form round-trips the stored value, and clearing the field
sends `""` on update.
- Remote UAT through Coder Agents
([chat](https://dogfood.cdr.dev/agents/58695d5b-2112-400b-88f7-ab64c678f756))
on a test deployment with loopback MCP and authorization server mocks:
- Round 1 on 2b875068e0: literal verdict FAIL. When the resource
changed between connect and callback, the callback redeemed the code for
the new resource and stored a grant. The 409 binding fixes it.
- Round 2 on 4b54ffb5f9: PASS. The mid-flow change returned 409 with no
token request in every variant (strict and lenient servers, set,
cleared, abandoned flows, parallel configs); auto-discovery, connect,
refresh, validation, and the docs matched. Minor observations: the
binding cookie is an unkeyed digest, so a member who edits their own
HttpOnly cookie can redeem a code with the current resource, which a
fresh connect also gives them; other settings changed mid-flow still
reach the token endpoint, as on main; the 409 popup shows raw JSON; the
UI does not warn that a resource change disconnects users; validation
toasts omit the server's detail.
- Round 3 on the final head 2d6abfac49: PASS. The head moved during the
round, so it ran in four parts, the last on 2d6abfac49. That part
covered the matching rules (queries including empty ones, default and
zero-padded ports, `%2F`, escaped dot segments, trailing slashes, IPv6
literals), the invalid fallback (stored empty, no `resource` sent),
every validation value through the API and the UI, and the discovery,
connect, refresh, and mid-flow 409 flows. The equivalent-encoding cases
last ran live on 5c23fe00dca; unit tests cover them on the final head.
Observation: the path-aware metadata URL is built from the decoded path,
so `/mcp%2Ftenant` fetches metadata for `/mcp/tenant`, as on main.
- `frontend-review` and the eight-role `coder-frontend-review` ran on
the `site/` diff; every role passes after the label, help text, row
layout, API doc wording, and form test fixes. Declined: linking field
descriptions with `aria-describedby`, because the form's shared `Field`
primitive renders every description without one, so it belongs in a
change to all fields.

## Review record

| Review | Commit | Findings | Disposition |
|---|---|---|---|
| [Codex round
1](https://github.com/coder/coder/pull/30651#pullrequestreview-5492085359)
| 4b54ffb5f9 | P2: a resource with another query was treated as
covering the server URL | Reproduced with a failing test, fixed in
bf0879560c, resolved |
| [Codex round
2](https://github.com/coder/coder/pull/30651#pullrequestreview-5492305363)
| bf0879560c | P2: explicit default ports; P2: `%2F` treated as a
segment boundary; P2: `url.Parse` accepts spaces and invalid escapes |
Each reproduced with a failing test, fixed in 16a1d783fa, resolved |
| [Codex round
3](https://github.com/coder/coder/pull/30651#pullrequestreview-5492677517)
| 16a1d783fa | P1: docs wrapped mid-sentence (DOC1); P2: equivalent
percent-encodings compared unequal; P2: brackets accepted outside an IP
literal host | Fixed in 8d617f0528 (the P2s reproduced with failing
tests; the same pass also covers the query, dot segment, and user
information cases), resolved |
| [Codex round
4](https://github.com/coder/coder/pull/30651#pullrequestreview-5493033885)
| 8d617f0528 | P2: the server URL fallback was not validated; P2:
escaped dot segments were compared before decoding | Each reproduced
with a failing test, fixed in 5c23fe00dc, resolved |
| [Codex round
5](https://github.com/coder/coder/pull/30651#pullrequestreview-5493342889)
| 5c23fe00dc | P2: an empty query (`?`) was treated as no query |
Reproduced with a failing test, fixed in 72c019ff5e, resolved |
| [Codex round
6](https://github.com/coder/coder/pull/30651#pullrequestreview-5493524846)
| 72c019ff5e | P2: `/mcp/` covered `/mcp`; P2: equivalent IPv6 literals
compared unequal; P2: malformed IP literals accepted | The first two
reproduced with failing tests. Go 1.26.5 `url.Parse` already rejects the
hosts named in the third, but it accepted an IPv6 zone identifier. All
fixed in ef2df2c4e7, resolved |
| [Codex round
7](https://github.com/coder/coder/pull/30651#pullrequestreview-5493668131)
| ef2df2c4e7 | P2: leading zeros in a port made equal origins differ |
Reproduced with a failing test, fixed in 2d6abfac49, resolved |
| [Codex round
8](https://github.com/coder/coder/pull/30651#issuecomment-6128743972) |
2d6abfac49 | None: "Didn't find any major issues" | Clean verdict on
the current head |

> Xum opened this PR on behalf of @ibetitsmike.
2026-10-13 23:03:08 +02:00

469 lines
14 KiB
Go
Raw Permalink Blame History

package coderd
import (
"context"
"encoding/json"
"net/url"
"sync/atomic"
"testing"
"time"
"github.com/google/uuid"
"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/require"
"golang.org/x/oauth2"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/testutil"
)
// dbauthzTestStore wraps the test database with the same dbauthz layer
// used in production (coderd.go:370). Without it the test would not
// catch RBAC failures from the chatd subject; with it the test fails
// loudly if the elevation in OIDCAccessToken is removed or weakened.
func dbauthzTestStore(t *testing.T, db database.Store) database.Store {
t.Helper()
authz := rbac.NewStrictCachingAuthorizer(prometheus.NewRegistry())
acs := &atomic.Pointer[dbauthz.AccessControlStore]{}
var tacs dbauthz.AccessControlStore = fakeAccessControlStore{}
acs.Store(&tacs)
return dbauthz.New(db, authz, testutil.Logger(t), acs)
}
// fakeAccessControlStore mirrors coderdtest.FakeAccessControlStore but is
// inlined here to avoid an import cycle (coderdtest imports coderd).
type fakeAccessControlStore struct{}
func (fakeAccessControlStore) GetTemplateAccessControl(t database.Template) dbauthz.TemplateAccessControl {
return dbauthz.TemplateAccessControl{
RequireActiveVersion: t.RequireActiveVersion,
}
}
func (fakeAccessControlStore) SetTemplateAccessControl(context.Context, database.Store, uuid.UUID, dbauthz.TemplateAccessControl) error {
panic("not implemented")
}
func TestShouldRefreshOIDCToken(t *testing.T) {
t.Parallel()
now := dbtime.Now()
cases := []struct {
name string
link database.UserLink
want bool
}{
{
name: "NoRefreshToken",
link: database.UserLink{OAuthExpiry: now.Add(-time.Hour)},
},
{
name: "ZeroExpiry",
link: database.UserLink{OAuthRefreshToken: "refresh"},
},
{
name: "Expired",
link: database.UserLink{
OAuthRefreshToken: "refresh",
OAuthExpiry: now.Add(-time.Hour),
},
want: true,
},
{
name: "Fresh",
link: database.UserLink{
OAuthRefreshToken: "refresh",
OAuthExpiry: now.Add(time.Hour),
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
got, _ := shouldRefreshOIDCToken(tc.link)
require.Equal(t, tc.want, got)
})
}
}
func TestOIDCMCPTokenSource(t *testing.T) {
t.Parallel()
logger := testutil.Logger(t)
t.Run("NilConfig", func(t *testing.T) {
t.Parallel()
db, _ := dbtestutil.NewDB(t)
require.Nil(t, newOIDCMCPTokenSource(db, nil, logger))
})
t.Run("NoLink", func(t *testing.T) {
// When the user has no OIDC link the source returns ("", nil)
// rather than an error so the caller can fall through to
// "no Authorization header".
t.Parallel()
db, _ := dbtestutil.NewDB(t)
store := dbauthzTestStore(t, db)
user := dbgen.User(t, db, database.User{LoginType: database.LoginTypeOIDC})
src := newOIDCMCPTokenSource(store, &testutil.OAuth2Config{}, logger)
ctx := dbauthz.AsChatd(context.Background())
tok, err := src.OIDCAccessToken(ctx, user.ID)
require.NoError(t, err)
require.Empty(t, tok)
})
t.Run("FreshToken", func(t *testing.T) {
// A non-expired token is returned as-is; no refresh is performed.
t.Parallel()
db, _ := dbtestutil.NewDB(t)
store := dbauthzTestStore(t, db)
user := dbgen.User(t, db, database.User{})
dbgen.UserLink(t, db, database.UserLink{
UserID: user.ID,
LoginType: database.LoginTypeOIDC,
OAuthAccessToken: "fresh",
OAuthRefreshToken: "refresh",
OAuthExpiry: dbtime.Now().Add(time.Hour),
})
src := newOIDCMCPTokenSource(store, &testutil.OAuth2Config{
Token: &oauth2.Token{AccessToken: "should-not-be-used"},
}, logger)
ctx := dbauthz.AsChatd(context.Background())
tok, err := src.OIDCAccessToken(ctx, user.ID)
require.NoError(t, err)
require.Equal(t, "fresh", tok)
})
t.Run("RefreshExpired", func(t *testing.T) {
// An expired token triggers a refresh; the new token is
// persisted via UpdateUserLink. This exercises the dbauthz
// elevation: chatd lacks ResourceSystem.Read and
// ResourceUser.UpdatePersonal so a non-elevated context
// would fail both reads and writes.
t.Parallel()
db, _ := dbtestutil.NewDB(t)
store := dbauthzTestStore(t, db)
user := dbgen.User(t, db, database.User{})
dbgen.UserLink(t, db, database.UserLink{
UserID: user.ID,
LoginType: database.LoginTypeOIDC,
OAuthAccessToken: "stale",
OAuthRefreshToken: "refresh",
OAuthExpiry: dbtime.Now().Add(-time.Hour),
})
src := newOIDCMCPTokenSource(store, &testutil.OAuth2Config{
Token: &oauth2.Token{
AccessToken: "fresh",
RefreshToken: "new-refresh",
Expiry: dbtime.Now().Add(time.Hour),
},
}, logger)
ctx := dbauthz.AsChatd(context.Background())
tok, err := src.OIDCAccessToken(ctx, user.ID)
require.NoError(t, err)
require.Equal(t, "fresh", tok)
// Verify the refresh was persisted via UpdateUserLink.
got, err := db.GetUserLinkByUserIDLoginType(
dbauthz.AsSystemRestricted(context.Background()),
database.GetUserLinkByUserIDLoginTypeParams{
UserID: user.ID,
LoginType: database.LoginTypeOIDC,
},
)
require.NoError(t, err)
require.Equal(t, "fresh", got.OAuthAccessToken)
require.Equal(t, "new-refresh", got.OAuthRefreshToken)
})
t.Run("RefreshFailureReturnsEmpty", func(t *testing.T) {
// A refresh attempt that fails (e.g. invalid client config)
// must not surface an error to the caller; per the
// UserOIDCTokenSource contract this is treated as "no
// Authorization header".
t.Parallel()
db, _ := dbtestutil.NewDB(t)
store := dbauthzTestStore(t, db)
user := dbgen.User(t, db, database.User{})
dbgen.UserLink(t, db, database.UserLink{
UserID: user.ID,
LoginType: database.LoginTypeOIDC,
OAuthAccessToken: "stale",
OAuthRefreshToken: "refresh",
OAuthExpiry: dbtime.Now().Add(-time.Hour),
})
// An empty oauth2.Config triggers a refresh failure
// because it has no token endpoint to call.
src := newOIDCMCPTokenSource(store, &oauth2.Config{}, logger)
ctx := dbauthz.AsChatd(context.Background())
tok, err := src.OIDCAccessToken(ctx, user.ID)
require.NoError(t, err)
require.Empty(t, tok)
})
}
func TestProtectedResourceMetadataResourceUnmarshal(t *testing.T) {
t.Parallel()
tests := []struct {
name string
input string
want resourceIdentifiers
wantErr bool
}{
{
name: "String",
input: `{"resource": "https://gitlab.example.com/api/v4/mcp", "authorization_servers": ["https://gitlab.example.com"]}`,
want: resourceIdentifiers{"https://gitlab.example.com/api/v4/mcp"},
},
{
// GitLab's official MCP server returns an array of
// resources despite RFC 9728 defining a string.
name: "Array",
input: `{"resource": ["https://gitlab.example.com/api/v4/mcp", "https://gitlab.example.com/api/v4/orbit/mcp"], "authorization_servers": ["https://gitlab.example.com"]}`,
want: resourceIdentifiers{
"https://gitlab.example.com/api/v4/mcp",
"https://gitlab.example.com/api/v4/orbit/mcp",
},
},
{
name: "Absent",
input: `{"authorization_servers": ["https://gitlab.example.com"]}`,
want: nil,
},
{
name: "InvalidType",
input: `{"resource": 42, "authorization_servers": ["https://gitlab.example.com"]}`,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
var meta protectedResourceMetadata
err := json.Unmarshal([]byte(tt.input), &meta)
if tt.wantErr {
require.Error(t, err)
return
}
require.NoError(t, err)
require.Equal(t, tt.want, meta.Resource)
})
}
}
func TestMCPOAuth2ResourceIndicator(t *testing.T) {
t.Parallel()
cases := []struct {
name string
serverURL string
advertised []string
want string
}{
{
name: "ExactMatch",
serverURL: "https://mcp.agent.dagster.cloud/mcp",
advertised: []string{"https://mcp.agent.dagster.cloud/mcp"},
want: "https://mcp.agent.dagster.cloud/mcp",
},
{
name: "OriginCoversPath",
serverURL: "https://mcp.example.com/v1/mcp",
advertised: []string{"https://mcp.example.com"},
want: "https://mcp.example.com",
},
{
name: "CaseInsensitiveOriginKeptVerbatim",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"HTTPS://MCP.Example.com/mcp"},
want: "HTTPS://MCP.Example.com/mcp",
},
{
name: "PathCoversSubpath",
serverURL: "https://mcp.example.com/mcp/x",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "TrailingSlashResourceDoesNotCoverBarePath",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://mcp.example.com/mcp/"},
want: "https://mcp.example.com/mcp",
},
{
name: "EmptyPathEqualsSlash",
serverURL: "https://mcp.example.com",
advertised: []string{"https://mcp.example.com/"},
want: "https://mcp.example.com/",
},
{
name: "PathPrefixIsNotSegment",
serverURL: "https://mcp.example.com/mcp2",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp2",
},
{
name: "OtherOriginFallsBack",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://other.example.com/mcp", "http://mcp.example.com/mcp", "https://mcp.example.com:8443/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "MostSpecificWins",
serverURL: "https://mcp.example.com/a/b/c",
advertised: []string{
"https://mcp.example.com/",
"https://mcp.example.com/a/b",
"https://mcp.example.com/a",
},
want: "https://mcp.example.com/a/b",
},
{
name: "QueryMustMatch",
serverURL: "https://mcp.example.com/mcp?tenant=b",
advertised: []string{
"https://mcp.example.com/mcp?tenant=a",
"https://mcp.example.com/mcp?tenant=b",
},
want: "https://mcp.example.com/mcp?tenant=b",
},
{
name: "MatchingQueryBeatsNoQuery",
serverURL: "https://mcp.example.com/mcp?tenant=b",
advertised: []string{"https://mcp.example.com/mcp", "https://mcp.example.com/mcp?tenant=b"},
want: "https://mcp.example.com/mcp?tenant=b",
},
{
name: "NoQueryCoversQuery",
serverURL: "https://mcp.example.com/mcp?tenant=a",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "EmptyQueryIsNotOtherQuery",
serverURL: "https://mcp.example.com/mcp?tenant=b",
advertised: []string{"https://mcp.example.com/mcp?"},
want: "https://mcp.example.com/mcp?tenant=b",
},
{
name: "EmptyQueryIsNotNoQuery",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://mcp.example.com/mcp?"},
want: "https://mcp.example.com/mcp",
},
{
name: "ExplicitDefaultPortOnServer",
serverURL: "https://mcp.example.com:443/mcp",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "ExplicitDefaultPortOnResource",
serverURL: "http://mcp.example.com/mcp",
advertised: []string{"http://mcp.example.com:80/mcp"},
want: "http://mcp.example.com:80/mcp",
},
{
name: "PortLeadingZerosAreIgnored",
serverURL: "https://mcp.example.com:0443/mcp",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "EscapedSlashIsNotSegmentBoundary",
serverURL: "https://mcp.example.com/mcp%2Ftenant",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp%2Ftenant",
},
{
name: "EscapeHexCaseIsEquivalent",
serverURL: "https://mcp.example.com/mcp%2ftenant",
advertised: []string{"https://mcp.example.com/mcp%2Ftenant"},
want: "https://mcp.example.com/mcp%2Ftenant",
},
{
name: "EscapedUnreservedIsEquivalent",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://mcp.example.com/%6Dcp"},
want: "https://mcp.example.com/%6Dcp",
},
{
name: "EscapedQueryIsEquivalent",
serverURL: "https://mcp.example.com/mcp?tenant=%61",
advertised: []string{"https://mcp.example.com/mcp?tenant=a"},
want: "https://mcp.example.com/mcp?tenant=a",
},
{
name: "DotSegmentsAreRemoved",
serverURL: "https://mcp.example.com/a/../mcp",
advertised: []string{"https://mcp.example.com/mcp"},
want: "https://mcp.example.com/mcp",
},
{
name: "EscapedDotSegmentsAreRemoved",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://mcp.example.com/a/%2E%2E/mcp"},
want: "https://mcp.example.com/a/%2E%2E/mcp",
},
{
name: "InvalidFallbackIsEmpty",
serverURL: "https://mcp.example.com/a[b]",
want: "",
},
{
name: "IPLiteralHost",
serverURL: "https://[::1]:8443/mcp",
advertised: []string{"https://[::1]:8443/"},
want: "https://[::1]:8443/",
},
{
name: "EquivalentIPv6Literal",
serverURL: "https://[0:0:0:0:0:0:0:1]/mcp",
advertised: []string{"https://[::1]/mcp"},
want: "https://[::1]/mcp",
},
{
name: "NoEntriesFallsBack",
serverURL: "https://mcp.example.com/mcp",
want: "https://mcp.example.com/mcp",
},
{
name: "FallbackIsCanonical",
serverURL: "HTTPS://user:pass@MCP.Example.COM:8443/Path/MCP?tenant=Acme#frag",
want: "https://mcp.example.com:8443/Path/MCP?tenant=Acme",
},
{
name: "InvalidEntriesIgnored",
serverURL: "https://mcp.example.com/mcp",
advertised: []string{"https://mcp.example.com/#frag", "/mcp", "", " "},
want: "https://mcp.example.com/mcp",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
serverURL, err := url.Parse(tc.serverURL)
require.NoError(t, err)
require.Equal(t, tc.want, mcpOAuth2ResourceIndicator(serverURL, tc.advertised))
})
}
}
Reference in New Issue View Git Blame Copy Permalink