Skip to content

Commit ec284b0

Browse files
peppescgclaude
andauthored
fix(api,cli): stop auto-deriving RFC 8707 resource indicator from URL (#5204)
* fix(api): stop auto-deriving RFC 8707 resource indicator from URL The API path unconditionally derived the RFC 8707 resource parameter from the server URL, while the CLI only did so when --resource-url was explicitly passed. This broke OAuth for servers like Common Room that don't support RFC 8707. Remove the automatic URL-to-resource fallback in both buildRemoteAuthConfigFromMetadata and createRequestToRemoteAuthConfig, keeping only user-provided and metadata-provided resource values. Fixes: #5203 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(api): add resource indicator tests for createRequestToRemoteAuthConfig Verify that resource is NOT auto-derived from URL when not explicitly set, and that an explicitly provided resource is preserved verbatim. These tests guard against re-introducing the removed fallback derivation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(api,cli): remove auto-derivation of RFC 8707 resource from all paths Also fix the CLI registry path (getRemoteAuthFromRemoteServerMetadata) which had the same unconditional fallback to DefaultResourceIndicator. Inline orphaned variable in createRequestToRemoteAuthConfig. The resource parameter is now only set when explicitly provided by the user or registry metadata, matching the behavior of the direct CLI path which gates on --resource-url. Refs: #5203 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: address review nits — clarify comments Add explanatory comment on registry path resource assignment explaining why --resource-url derivation is intentionally skipped. Reword test comment to be reader-facing rather than author-local. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bf3ebe7 commit ec284b0

4 files changed

Lines changed: 47 additions & 19 deletions

File tree

cmd/thv/app/run_flags.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -948,12 +948,11 @@ func getRemoteAuthFromRemoteServerMetadata(
948948
authCfg.AuthorizeURL = firstNonEmpty(f.RemoteAuthAuthorizeURL, oc.AuthorizeURL)
949949
authCfg.TokenURL = firstNonEmpty(f.RemoteAuthTokenURL, oc.TokenURL)
950950

951-
resourceIndicator := firstNonEmpty(f.RemoteAuthResource, oc.Resource)
952-
if resourceIndicator != "" {
953-
authCfg.Resource = resourceIndicator
954-
} else {
955-
authCfg.Resource = remote.DefaultResourceIndicator(remoteServerMetadata.URL)
956-
}
951+
// Resource is only set from explicit user flag or registry metadata.
952+
// Unlike the direct-URL path (getRemoteAuthFromRunFlags), --resource-url
953+
// derivation is intentionally not applied here: registry metadata is the
954+
// authoritative source for the resource indicator in this path.
955+
authCfg.Resource = firstNonEmpty(f.RemoteAuthResource, oc.Resource)
957956

958957
// OAuthParams: REPLACE metadata when CLI provides any key/value.
959958
if len(runFlags.OAuthParams) > 0 {

pkg/api/v1/workload_service.go

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -429,14 +429,11 @@ func buildRemoteAuthConfigFromMetadata(req *createRequest, md *regtypes.RemoteSe
429429
return nil
430430
}
431431

432-
// Default resource: user-provided > registry metadata > derived from remote URL
432+
// Default resource: user-provided > registry metadata
433433
resource := req.OAuthConfig.Resource
434434
if resource == "" {
435435
resource = md.OAuthConfig.Resource
436436
}
437-
if resource == "" && md.URL != "" {
438-
resource = remote.DefaultResourceIndicator(md.URL)
439-
}
440437

441438
cfg := &remote.Config{
442439
ClientID: req.OAuthConfig.ClientID,
@@ -466,12 +463,6 @@ func createRequestToRemoteAuthConfig(
466463
req *createRequest,
467464
) *remote.Config {
468465

469-
// Default resource: user-provided > derived from remote URL
470-
resource := req.OAuthConfig.Resource
471-
if resource == "" && req.URL != "" {
472-
resource = remote.DefaultResourceIndicator(req.URL)
473-
}
474-
475466
// Create RemoteAuthConfig
476467
remoteAuthConfig := &remote.Config{
477468
ClientID: req.OAuthConfig.ClientID,
@@ -480,7 +471,7 @@ func createRequestToRemoteAuthConfig(
480471
AuthorizeURL: req.OAuthConfig.AuthorizeURL,
481472
TokenURL: req.OAuthConfig.TokenURL,
482473
UsePKCE: req.OAuthConfig.UsePKCE,
483-
Resource: resource,
474+
Resource: req.OAuthConfig.Resource,
484475
OAuthParams: req.OAuthConfig.OAuthParams,
485476
CallbackPort: req.OAuthConfig.CallbackPort,
486477
SkipBrowser: req.OAuthConfig.SkipBrowser,

pkg/api/v1/workload_service_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -753,7 +753,7 @@ func TestBuildRemoteAuthConfigFromMetadata(t *testing.T) {
753753
assert.Equal(t, "https://resource.example.com", cfg.Resource)
754754
})
755755

756-
t.Run("resource derived from URL when both user and metadata unset", func(t *testing.T) {
756+
t.Run("resource empty when both user and metadata unset", func(t *testing.T) {
757757
t.Parallel()
758758

759759
md := baseMetadata()
@@ -762,7 +762,7 @@ func TestBuildRemoteAuthConfigFromMetadata(t *testing.T) {
762762
cfg := buildRemoteAuthConfigFromMetadata(&createRequest{}, md)
763763

764764
require.NotNil(t, cfg)
765-
assert.NotEmpty(t, cfg.Resource, "resource should be derived from md.URL")
765+
assert.Empty(t, cfg.Resource, "resource should not be auto-derived from URL")
766766
})
767767

768768
t.Run("user ClientSecret is applied in CLI string format", func(t *testing.T) {

pkg/api/v1/workloads_types_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,44 @@ func TestCreateRequestToRemoteAuthConfig(t *testing.T) {
515515
assert.Equal(t, tt.expectedBearerToken, result.BearerToken)
516516
})
517517
}
518+
519+
// Guard against reintroduction of RFC 8707 auto-derivation from URL.
520+
// Resource must only be set when explicitly provided (see #5203).
521+
t.Run("resource empty when URL set but resource not explicitly provided", func(t *testing.T) {
522+
t.Parallel()
523+
524+
req := &createRequest{
525+
updateRequest: updateRequest{
526+
URL: "https://mcp.example.com/mcp",
527+
OAuthConfig: remoteOAuthConfig{ClientID: "some-client"},
528+
},
529+
}
530+
531+
cfg := createRequestToRemoteAuthConfig(context.Background(), req)
532+
533+
require.NotNil(t, cfg)
534+
assert.Empty(t, cfg.Resource, "resource must not be auto-derived from URL when not explicitly set")
535+
})
536+
537+
t.Run("resource preserved when user provides it explicitly", func(t *testing.T) {
538+
t.Parallel()
539+
540+
req := &createRequest{
541+
updateRequest: updateRequest{
542+
URL: "https://mcp.example.com/mcp",
543+
OAuthConfig: remoteOAuthConfig{
544+
ClientID: "some-client",
545+
Resource: "https://explicit-resource.example.com",
546+
},
547+
},
548+
}
549+
550+
cfg := createRequestToRemoteAuthConfig(context.Background(), req)
551+
552+
require.NotNil(t, cfg)
553+
assert.Equal(t, "https://explicit-resource.example.com", cfg.Resource,
554+
"user-provided resource must be preserved verbatim")
555+
})
518556
}
519557

520558
func TestValidateHeaderForwardConfig(t *testing.T) {

0 commit comments

Comments
 (0)