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 79.31034% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.34%. Comparing base (1778767) to head (67fa475).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/tokenexchange/middleware.go	81.94%	11 Missing and 2 partials ⚠️
pkg/runner/config_builder.go	0.00%	10 Missing ⚠️
pkg/runner/middleware.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2143      +/-   ##
==========================================
+ Coverage   49.29%   49.34%   +0.05%     
==========================================
  Files         244      244              
  Lines       30920    31009      +89     
==========================================
+ Hits        15241    15301      +60     
- Misses      14539    14573      +34     
+ Partials     1140     1135       -5     

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

[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

[yrobla]

added some comments

[jhrozek]

@claude please review this PR. One of the things to take into account is the integration aspect of the middleware

[claude]

Claude finished @jhrozek's task —— View job

---

PR Review: Token Exchange Middleware Integration

Comprehensive review of the OAuth 2.0 Token Exchange (RFC 8693) integration focusing on middleware patterns and architecture.

---

Executive Summary

This PR successfully integrates token exchange middleware into ToolHive's architecture using well-established patterns. The implementation demonstrates strong understanding of the existing middleware system and follows most project conventions. A few integration aspects merit attention.

---

Middleware Integration Analysis

✅ Strengths

1. Consistent Factory Pattern (pkg/runner/middleware.go:19)

tokenexchange.MiddlewareType: tokenexchange.CreateMiddleware,

The middleware is correctly registered in the factory map alongside existing middlewares (auth, telemetry, authz, audit). This follows the exact pattern used by all other middlewares in the system.

2. Proper Conditional Application (pkg/runner/config_builder.go:537-545)

// Token Exchange middleware (conditionally present)
if tokenExchangeConfig != nil {
    tokenExchangeParams := tokenexchange.MiddlewareParams{
        TokenExchangeConfig: tokenExchangeConfig,
    }
    if tokenExchangeMwConfig, err := types.NewMiddlewareConfig(...); err == nil {
        middlewareConfigs = append(middlewareConfigs, *tokenExchangeMwConfig)
    }
}

Excellent: The middleware is only added when configured, matching the pattern for telemetry (line 562), authz (line 579), and audit (line 604). This is the correct approach.

3. Clean Separation of Concerns

• CLI Layer (cmd/thv/app/auth_flags.go:59-115): Handles configuration building and user input
• Middleware Layer (pkg/auth/tokenexchange/middleware.go): Handles HTTP request processing
• Exchange Logic (pkg/auth/tokenexchange/exchange.go): Implements RFC 8693 protocol

This three-layer separation is clean and maintainable.

4. Smart Token Selection (pkg/auth/tokenexchange/middleware.go:208-238)

The middleware correctly handles token type selection based on configuration:

switch config.SubjectTokenType {
case tokenTypeIDToken, tokenTypeJWT:
    // Extract ID token from Extra field
    idToken, ok := token.Extra("id_token").(string)
    ...
case "", tokenTypeAccessToken:
    return token.AccessToken, nil
}

This is elegant - the middleware encapsulates the complexity of token source selection.

5. Dual-Mode Operation (pkg/auth/tokenexchange/middleware.go:299-323)

if subjectTokenProvider != nil {
    // Mode 2: Use provided token source (OAuth flow during startup)
    tokenProvider = subjectTokenProvider
} else {
    // Mode 1: Extract from Authorization header (OIDC validation)
    subjectToken := strings.TrimPrefix(authHeader, "Bearer ")
    ...
}

Excellent architectural decision: Supporting both modes (external OAuth + header extraction) makes the middleware flexible for different use cases.

---

⚠️ Integration Issues to Address

1. Middleware Ordering Comment is Confusing (pkg/runner/config_builder.go:447-455)

Severity: Low

// NOTE: order matters here. Specifically, these routines use append
// to add new middleware configs, but once these routines are called,
// inside the proxy, they are applied in reverse order, so the first
// being added here is effectively the last being called at HTTP
// request time.

Problem: This comment suggests middleware are applied in reverse order, but looking at the actual ordering:

1. Tool filter middlewares
2. Auth middleware (line 533)
3. Token exchange middleware (line 542)
4. MCP Parser (line 549)
5. Telemetry (line 571)
6. Authz (line 596)
7. Audit (line 627)

This ordering makes sense if executed in order (not reverse):

• Auth validates incoming tokens → Token exchange swaps them → Parser handles MCP → Telemetry/Authz/Audit observe

Question for author: Is the comment outdated? The code at config_builder.go:534-544 adds auth before token exchange, which is correct if middlewares execute in order. If they truly execute in reverse, token exchange would run before auth validation, which would be problematic.

Recommendation:

• Clarify or update this comment
• Add integration tests that verify the actual execution order
• Consider documenting the expected order in CLAUDE.md since it's architectural

2. No Integration Test for Middleware Order

Severity: Medium

Looking at the test files:

• middleware_test.go has excellent unit tests for the middleware itself
• exchange_test.go tests the RFC 8693 implementation

Missing: An integration test showing:

// Pseudo-code for recommended test
func TestMiddlewareOrder(t *testing.T) {
    // 1. Auth middleware should populate claims in context
    // 2. Token exchange middleware should read those claims
    // 3. Token exchange should inject new token
    // 4. Verify the final request has the exchanged token
}

This would catch ordering bugs and document expected behavior.

3. Secret Resolution Pattern Differs Between Commands

Severity: Low

In proxy.go:395-416 (addExternalTokenMiddleware):

func addExternalTokenMiddleware(middlewares *[]types.MiddlewareFunction, tokenSource *oauth2.TokenSource) error {
    if remoteAuthFlags.TokenExchangeURL != "" {
        tokenExchangeConfig, err := remoteAuthFlags.BuildTokenExchangeConfig()
        if err != nil {
            return fmt.Errorf("invalid token exchange configuration: %w", err)
        }
        ...
        tokenExchangeMiddleware, err := tokenexchange.CreateMiddlewareFromTokenSource(...)
    }
}

In run_flags.go:459-462:

tokenExchangeConfig, err := runFlags.RemoteAuthFlags.BuildTokenExchangeConfig()
if err != nil {
    return nil, fmt.Errorf("invalid token exchange configuration: %w", err)
}

Observation: The thv proxy command creates middleware directly via CreateMiddlewareFromTokenSource, while thv run builds config and lets the factory system create it. This is intentional (different use cases), but worth noting:

• thv proxy: Needs immediate token source from OAuth flow → uses direct construction
• thv run: Uses factory pattern for consistency with other middlewares

This is actually correct, but could benefit from a comment explaining why proxy bypasses the factory.

4. Environment Variable Handling in Middleware (pkg/auth/tokenexchange/middleware.go:266-274)

Severity: Low

clientSecret := config.ClientSecret
if clientSecret == "" {
    if envSecret := getEnv(EnvClientSecret); envSecret != "" {
        clientSecret = envSecret
        logger.Debug("Using client secret from environment variable")
    }
}

Good: Uses dependency injection (envGetter) for testability.

Question: The BuildTokenExchangeConfig in auth_flags.go:68-83 only resolves from flags and files, NOT environment variables. The comment says:

// Environment variable is handled by the middleware for Kubernetes deployments

Concern: This split responsibility (CLI resolves flag/file, middleware resolves env var) is subtle and could confuse future maintainers. Consider adding a comment in auth_flags.go explaining this design decision more explicitly.

Recommendation:

// BuildTokenExchangeConfig creates a TokenExchangeConfig from the RemoteAuthFlags.
// Returns nil if TokenExchangeURL is empty (token exchange is not configured).
// Returns error if there is a configuration error (e.g., file read failure).
//
// NOTE: Environment variable resolution (TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET) 
// is intentionally handled by the middleware layer, not here. This design allows
// Kubernetes deployments to inject secrets via environment without CLI involvement.

---

⚙️ Open Review Questions

These are the questions raised by other reviewers that relate to integration:

1. yrobla: "we actually have a secretsmanager functionality, shall we reuse it?" (auth_flags.go:17)

Impact: Medium

Currently, secret resolution in readSecretFromFile is standalone. The project has pkg/secret/ infrastructure that handles 1Password, encrypted storage, etc.

Analysis:

• Current approach: Simple file reading for CLI secrets
• Project's secret manager: More sophisticated, supports multiple backends

Recommendation: This should be addressed in a follow-up PR, not blocking. The current implementation works for the immediate use case. Integration with the secret manager would be valuable for:

• Consistency with how other secrets are handled (see --secret flag handling)
• Supporting 1Password, Vault, etc. for token exchange credentials

For this PR: The current implementation is acceptable. Add a TODO or file an issue to track secret manager integration.

2. yrobla: "here i am not sure if we can just call about id_token and access_token and forget about jwt?" (middleware.go:216)

Looking at the code (tokenexchange/exchange.go:26-36):

const (
    tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token"
    tokenTypeIDToken = "urn:ietf:params:oauth:token-type:id_token"  
    tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt"
)

And the switch statement (middleware.go:215-237):

switch config.SubjectTokenType {
case tokenTypeIDToken, tokenTypeJWT:
    // Extract ID token from Extra field
    idToken, ok := token.Extra("id_token").(string)
    ...

Analysis: Both id_token and jwt are treated identically (both extract from Extra("id_token")). This is correct because:

• Google STS specifically requires tokenTypeIDToken
• The generic tokenTypeJWT is semantically similar (both are JWTs)
• RFC 8693 allows both token types

Verdict: Current implementation is correct. The handling is the same because both are JWT-based tokens from the OIDC flow.

3. yrobla: "this was added by claude, maybe we can add better comments here?" (middleware.go:301)

This refers to the dual-mode logic. Let me check if the comments are sufficient...

Current comments:

// Determine token source based on whether external provider was given
if subjectTokenProvider != nil {
    // Mode 2: Use provided token source (e.g., from OAuth flow during startup)
    logger.Debug("Using provided token source for token exchange")
    tokenProvider = subjectTokenProvider
} else {
    // Mode 1: Extract token from Authorization header (OIDC validation scenario)
    ...
}

Recommendation: These comments are actually quite good! But could be enhanced:

// Determine token source based on deployment scenario:
//   - Proxy command (Mode 2): Uses token from OAuth flow performed at startup
//   - Run command with OIDC (Mode 1): Extracts token from incoming request headers
//
// This dual-mode design allows the same middleware to work in both scenarios:
//   thv proxy --token-exchange-url (with OAuth)
//   thv run --oidc-issuer --token-exchange-url (with incoming auth)

---

Architecture Assessment

Overall Integration Quality: 8.5/10

What's excellent:

• ✅ Follows established middleware factory pattern
• ✅ Proper conditional registration
• ✅ Clean separation of concerns
• ✅ Comprehensive test coverage (unit tests)
• ✅ RFC 8693 compliance
• ✅ Flexible dual-mode design

What needs attention:

• ⚠️ Middleware ordering clarification (comment may be outdated)
• ⚠️ Missing integration tests for ordering
• ⚠️ Secret resolution split between CLI and middleware (needs clearer docs)
• ⚠️ Consider secret manager integration (follow-up PR)

---

Specific Code References

Files with Good Integration Patterns

1. pkg/runner/config_builder.go:523-554 ✅

   • Correctly adds token exchange between auth and MCP parser
   • Follows same pattern as telemetry/authz/audit

2. pkg/auth/tokenexchange/middleware.go:100-130 ✅

   • Factory function matches auth.CreateMiddleware pattern exactly
   • Proper error handling and validation

3. cmd/thv/app/proxy.go:394-416 ✅

   • Direct construction for proxy use case is appropriate
   • Clean separation from factory-based construction

Files Needing Clarification

1. pkg/runner/config_builder.go:447-455 ⚠️

   • Middleware ordering comment needs verification

2. cmd/thv/app/auth_flags.go:68-70 ⚠️

   • Add comment explaining why env var is NOT resolved here

---

Recommendations

Must Address Before Merge:

1. Clarify middleware execution order - Update or correct the comment at config_builder.go:447
2. Enhance documentation - Add comments explaining the split between proxy's direct construction vs run's factory pattern

Should Address (Can be Follow-up):

1. Add integration test - Test showing auth → token exchange → downstream flow
2. Secret manager integration - File issue to track using pkg/secret/ infrastructure
3. Improve inline comments - Enhance the dual-mode explanation per yrobla's feedback

Nice to Have:

1. Architecture docs - Document middleware ordering in CLAUDE.md
2. Metrics - Add telemetry to track token exchange success/failure rates

---

Comparison with Existing Middlewares

Aspect	Auth	Telemetry	Authz	Token Exchange
Factory registration	✅	✅	✅	✅
Conditional application	N/A (always)	✅	✅	✅
Error handling	✅	✅	✅	✅
Test coverage	✅	✅	✅	✅
Integration tests	⚠️	⚠️	⚠️	⚠️

Note: Integration testing is a general gap across all middlewares, not specific to token exchange.

---

Final Verdict

The middleware integration is well-executed and follows ToolHive's established patterns. The implementation demonstrates understanding of:

• Factory pattern for middleware registration
• Conditional middleware application
• Clean separation between CLI and middleware layers
• Dual-mode operation for different use cases

The main concerns are around documentation clarity (middleware ordering, secret resolution) and missing integration tests, but these don't block the core functionality. The architecture is sound.

Recommendation: Approve with minor documentation improvements. Consider filing follow-up issues for integration tests and secret manager unification.

---

Branch: thv-te-cli

[Copilot]

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

---

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