Integrate token exchange middleware

[jhrozek]

Adds OAuth 2.0 Token Exchange (RFC 8693) support to both 'thv proxy' and 'thv run' commands for secure downstream token swapping.

Changes include:
- New token exchange configuration flags in RemoteAuthFlags
- Refactored secret resolution into reusable resolveSecretFromSources
- Token exchange middleware registration in runner config builder
- Conditional middleware application based on configuration

When token exchange is configured via --token-exchange-url, the
middleware exchanges the user's bearer token for a downstream token
suitable for backend authentication.

Fixes: #2066

[codecov]

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.84%. Comparing base (866b10d) to head (a4d0673).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/runner/config_builder.go	0.00%	8 Missing ⚠️
pkg/runner/middleware.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2143      +/-   ##
==========================================
+ Coverage   48.82%   48.84%   +0.02%     
==========================================
  Files         242      242              
  Lines       30714    30723       +9     
==========================================
+ Hits        14996    15007      +11     
+ Misses      14596    14588       -8     
- Partials     1122     1128       +6     

☔ 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.

[Copilot]

Pull Request Overview

Adds OAuth 2.0 Token Exchange (RFC 8693) support to both 'thv proxy' and 'thv run' commands for secure downstream token swapping.

• New token exchange configuration flags added to RemoteAuthFlags for specifying exchange endpoints, credentials, and token parameters
• Refactored secret resolution into a reusable function that supports multiple sources (flags, files, environment variables)
• Token exchange middleware integrated into both proxy and runner configurations with conditional activation

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/runner/middleware.go	Registers token exchange middleware factory in supported middleware map
pkg/runner/config_builder.go	Adds token exchange config parameter and conditional middleware creation
pkg/api/v1/workload_service.go	Updates API service to pass nil token exchange config (not supported via API)
docs/cli/thv_run.md	Documents new token exchange CLI flags for run command
docs/cli/thv_proxy.md	Documents new token exchange CLI flags for proxy command
cmd/thv/app/run_flags.go	Integrates token exchange config creation and passes to middleware builder
cmd/thv/app/proxy.go	Refactors secret resolution and adds token exchange middleware selection logic
cmd/thv/app/auth_flags.go	Defines token exchange flags structure and config building logic

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The function comment states it returns empty string when no source provides a value, but this behavior may not be clear to callers who expect secrets. Consider clarifying that callers should validate if an empty result is acceptable for their use case.

Suggested change

	// Returns empty string if no source provides a value (not an error)
	// Returns empty string if no source provides a value (not an error).
	// Callers MUST validate whether an empty result is acceptable for their use case,
	// especially when expecting secrets. An empty string means no secret was found.

[Copilot]

The error is logged as a warning but then silently ignored by setting clientSecret to empty string. This could lead to configuration issues being overlooked. Consider either propagating the error or making the warning more explicit about the fallback behavior.

Suggested change

	logger.Warnf("Failed to resolve token exchange client secret: %v", err)
	clientSecret = ""
	logger.Errorf("Failed to resolve token exchange client secret: %v. Token exchange will be disabled.", err)
	return nil

[jhrozek]

@claude review this PR. When reviewing make sure to take existing patterns in the CLI and the rest of the project into account.

[claude]

Claude finished @jhrozek's task —— View job

---

Code Review: PR #2143 - Integrate token exchange middleware

I've reviewed this PR against existing patterns in the codebase. Overall, the implementation is well-structured and follows established conventions. Here are my findings:

---

✅ Strengths

1. Consistent Pattern Usage

   • The resolveSecretFromSources function follows the same pattern as resolveClientSecret in pkg/api/v1/workload_service.go (lines 280-297)
   • Secret resolution priority order (flag → file → env var) matches existing OAuth patterns
   • Environment variable naming convention follows the established TOOLHIVE_* pattern

2. Good Code Organization

   • Public methods (BuildTokenExchangeConfig) are at the top of auth_flags.go, private methods would go below (per CLAUDE.md line 152-153)
   • Token exchange configuration properly integrated into the middleware pipeline in config_builder.go
   • Follows the factory pattern for middleware creation (CLAUDE.md line 93)

3. Proper Integration

   • Token exchange middleware correctly registered in pkg/runner/middleware.go:19
   • Middleware applied conditionally based on configuration presence
   • Consistent with how other optional middlewares (telemetry, authz, audit) are handled

---

⚠️ Issues to Address

1. Error Handling Inconsistency (cmd/thv/app/auth_flags.go:52-55)

Severity: Medium

if err != nil {
    logger.Warnf("Failed to resolve token exchange client secret: %v", err)
    clientSecret = ""
}

Problem: This mirrors the copilot review comment. The error is logged as a warning but silently ignored by setting clientSecret to empty string. This creates an unclear contract - callers don't know if token exchange will be disabled or if an empty secret is acceptable.

Recommendation: Follow the pattern from resolveClientSecret in proxy.go:391-408, which returns the error and lets the caller decide:

clientSecret, err := resolveSecretFromSources(...)
if err != nil {
    return nil, fmt.Errorf("failed to resolve token exchange client secret: %w", err)
}
// Continue with clientSecret, which may be empty (acceptable for some flows)

Or if empty is truly not acceptable for token exchange:

if err != nil {
    logger.Errorf("Failed to resolve token exchange client secret: %v. Token exchange requires a client secret.", err)
    return nil
}

Pattern Reference: Look at pkg/api/v1/workload_service.go:280-297 where secret resolution errors are properly propagated.

---

2. Documentation of Empty Return Semantics (cmd/thv/app/proxy.go:351-389)

Severity: Low

The copilot review is correct that the function comment should be clearer about what an empty string means:

Current comment (line 351-353):

// resolveSecretFromSources resolves a secret from multiple sources with priority ordering
// Priority: 1. Direct value (flag), 2. File path, 3. Environment variable
// Returns empty string if no source provides a value (not an error)

Suggested improvement:

// resolveSecretFromSources resolves a secret from multiple sources with priority ordering.
// Priority: 1. Direct value (flag), 2. File path, 3. Environment variable
// Returns empty string if no source provides a value - this is NOT an error condition.
// Callers must validate whether an empty result is acceptable for their use case.
// For example, OAuth PKCE flows may work without a client secret, while token exchange may require it.

---

3. Code Coverage (General observation)

Severity: Low

Codecov reports 10% patch coverage with 9 lines missing coverage:

• pkg/runner/config_builder.go: 8 lines missing
• pkg/runner/middleware.go: 1 line missing

Recommendation: The project uses Ginkgo/Gomega for tests (CLAUDE.md line 63). Consider adding tests for:

1. Token exchange config building with various flag combinations
2. Middleware conditional application based on config presence
3. Secret resolution with different sources

Pattern Reference: Look at existing test files in pkg/runner/ for patterns (named *_test.go).

---

📋 Minor Observations

1. Consistency in run_flags.go:

   • Line 462 correctly calls getTokenExchangeConfigFromRunFlags(runFlags) which delegates to BuildTokenExchangeConfig()
   • This is good - it maintains separation of concerns

2. API Service Correctly Excludes Token Exchange (pkg/api/v1/workload_service.go:232):

   • Line 232 correctly passes nil for token exchange config with a comment explaining it's not supported via API yet
   • This is appropriate since the API service uses a different authentication mechanism

3. Documentation Updated:

   • Both docs/cli/thv_proxy.md and docs/cli/thv_run.md have been updated with the new flags
   • Follows the pattern from other authentication flags

---

🎯 Alignment with Project Patterns

✅ Follows Factory Pattern (CLAUDE.md line 93)
✅ Middleware Pattern correctly implemented (CLAUDE.md line 95)
✅ Interface Segregation - clean separation between CLI and middleware concerns (CLAUDE.md line 94)
✅ Security Model - proper secret management with multiple backends (CLAUDE.md line 110)
✅ Code Organization - public methods at top (CLAUDE.md lines 152-153)
⚠️ Testing Strategy - needs more test coverage (CLAUDE.md lines 114-125)

---

📝 Recommendations Summary

1. Must Fix:

   • Error handling in BuildTokenExchangeConfig (auth_flags.go:52-55) - decide whether to propagate error or explicitly return nil

2. Should Fix:

   • Improve documentation of resolveSecretFromSources to clarify empty string semantics
   • Add unit tests to improve coverage from 10% to at least 70%

3. Nice to Have:

   • Consider extracting the header strategy logic setup from BuildTokenExchangeConfig into a separate helper function for clarity

---

✅ Overall Assessment

The implementation is solid and follows ToolHive's established patterns well. The main concern is the error handling inconsistency in BuildTokenExchangeConfig. Once that's addressed and test coverage is improved, this will be a clean integration of the token exchange feature.

The refactoring of resolveSecretFromSources is particularly good - it makes secret resolution consistent across both OAuth and token exchange scenarios.

---

Job Run | Branch: thv-te-cli