Skip to content

fix(api,cli): stop auto-deriving RFC 8707 resource indicator from URL#5204

Merged
peppescg merged 4 commits intomainfrom
issues/5203
May 6, 2026
Merged

fix(api,cli): stop auto-deriving RFC 8707 resource indicator from URL#5204
peppescg merged 4 commits intomainfrom
issues/5203

Conversation

@peppescg
Copy link
Copy Markdown
Contributor

@peppescg peppescg commented May 6, 2026

Summary

  • Remove unconditional auto-derivation of the RFC 8707 resource parameter from the server URL in the API and CLI registry paths
  • The resource parameter is now only set when explicitly provided by the user or registry metadata
  • This aligns the API and CLI-from-registry behavior with the direct CLI path, which only derives resource when --resource-url is explicitly passed

Fixes #5203

Root cause

Three code paths built remote.Config for OAuth:

Path Function Auto-derived resource?
API from registry buildRemoteAuthConfigFromMetadata Yes (unconditionally)
API direct createRequestToRemoteAuthConfig Yes (unconditionally)
CLI from registry getRemoteAuthFromRemoteServerMetadata Yes (unconditionally)
CLI direct getRemoteAuthFromRunFlags Only if --resource-url set

The first three always fell back to DefaultResourceIndicator(url), sending resource=<server-url> in the OAuth authorize URL. Servers that don't support RFC 8707 (e.g., Common Room) reject this with "Service not found".

After this fix, all paths behave consistently: resource is only set from explicit user input or registry metadata. Servers that support RFC 9728 will still work via runtime discovery (.well-known/oauth-protected-resource).

Test plan

  • Existing TestBuildRemoteAuthConfigFromMetadata tests updated and pass
  • New regression tests for createRequestToRemoteAuthConfig verify resource is not auto-derived
  • go test ./pkg/api/v1/... passes
  • go build ./cmd/thv/... passes
  • Manual test: Common Room MCP server OAuth flow succeeds from ToolHive Studio

Tested vs ToolHive Studio

Kapture.2026-05-06.at.15.14.09.mp4

🤖 Generated with Claude Code

peppescg and others added 3 commits May 6, 2026 15:05
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>
…Config

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>
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>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label May 6, 2026
@peppescg peppescg self-assigned this May 6, 2026
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 6, 2026
@peppescg peppescg requested a review from Copilot May 6, 2026 13:16
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 6, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes implicit RFC 8707 resource parameter derivation from remote server URLs in the API and CLI registry-based OAuth config builders, so resource is only set when explicitly provided (by user input or registry metadata), preventing OAuth failures on providers that reject unknown resource values.

Changes:

  • Removed unconditional resource fallback derivation (remote.DefaultResourceIndicator(url)) in API registry/direct config builders.
  • Removed unconditional resource derivation in CLI “from registry metadata” config builder.
  • Updated/added tests to ensure resource remains empty unless explicitly provided.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pkg/api/v1/workloads_types_test.go Adds regression subtests ensuring API direct path does not auto-derive resource.
pkg/api/v1/workload_service.go Removes resource auto-derivation in API registry/direct remote auth config builders.
pkg/api/v1/workload_service_test.go Updates expectations to assert resource remains empty when unset.
cmd/thv/app/run_flags.go Removes unconditional resource derivation in CLI registry-metadata path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/thv/app/run_flags.go
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.67%. Comparing base (729e56a) to head (48e9884).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5204      +/-   ##
==========================================
- Coverage   67.72%   67.67%   -0.05%     
==========================================
  Files         607      607              
  Lines       61984    61979       -5     
==========================================
- Hits        41978    41947      -31     
- Misses      16845    16872      +27     
+ Partials     3161     3160       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@peppescg
Copy link
Copy Markdown
Contributor Author

peppescg commented May 6, 2026

@claude succinct review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @peppescg's task in 1m 9s —— View job


Review

LGTM with one minor note.

The fix is correct and tightly scoped. Removing the three DefaultResourceIndicator(url) fallbacks is the right call — RFC 9728 discovery (.well-known/oauth-protected-resource) is the proper mechanism for servers that do support RFC 8707, so pre-populating the parameter unconditionally was the wrong default.

The Copilot comment on run_flags.go:952 is worth addressing for clarity: in the direct CLI path (getRemoteAuthFromRunFlags, lines 1015–1017), there's still a conditional derivation — when --resource-url is set but --remote-auth-resource is not, resource is derived from RemoteURL. This asymmetry with the registry path is pre-existing and intentional (OIDC-triggered derivation), so it's not a regression. But you might want to add a brief comment on line 951 explaining that --resource-url-based derivation is not done in the registry path to avoid future re-introduction.

Minor nit: the test comment in workloads_types_test.go (// The coder removed auto-derivation...) reads as author-local context. A reader-facing phrasing like // Guard against reintroduction of RFC 8707 auto-derivation from URL. would age better.

Everything else — the createRequestToRemoteAuthConfig simplification, the updated test names/assertions, and the buildRemoteAuthConfigFromMetadata cleanup — looks clean and correct.

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>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 6, 2026
@peppescg peppescg merged commit ec284b0 into main May 6, 2026
46 checks passed
@peppescg peppescg deleted the issues/5203 branch May 6, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: API path always derives RFC 8707 resource indicator, CLI does not — breaks Common Room OAuth

3 participants