Skip to content

Add test coverage for strategy name mismatch validation #2454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Add test coverage for strategy name mismatch validation #2454

wants to merge 11 commits into from
+2,052 −648

Conversation

Copy link

Copilot AI commented Nov 4, 2025 •
edited
Loading

The RegisterStrategy method validates that the registration name matches strategy.Name(), but this validation had no test coverage.

Changes

  • Added test case "register strategy name mismatch fails" that verifies:
    • Error returned when registration name differs from strategy.Name()
    • Error message includes both the registered name and actual strategy name
    • Error message contains "strategy name mismatch"
// Test validates this validation logic is enforced
if name != strategy.Name() {
    return fmt.Errorf("strategy name mismatch: registered as %q but strategy.Name() returns %q",
        name, strategy.Name())
}

The test uses a mock strategy returning "actual_name" and attempts registration with "different_name", ensuring the validation prevents mismatched registrations.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

jhrozek and others added 10 commits November 4, 2025 14:03
@jhrozek
Reorganize incoming auth factory into subfolder to prevent an import …
799e80e 
…cycle

Move incoming authentication factory from pkg/vmcp/auth/ to
pkg/vmcp/auth/factory/ subfolder to improve code organization.
This separates factory code from core authentication types and
middleware.
@jhrozek
Refactor outgoing auth to separate registry from strategy
8bb674d 
Rename OutgoingAuthenticator to OutgoingAuthRegistry to better reflect
its responsibility as a strategy registry rather than an authenticator.

The interface now focuses solely on strategy management (registration
and retrieval), while authentication is performed directly by Strategy
implementations.

This separation improves performance by eliminating indirection in the
hot path (per-request authentication) and clarifies the single
responsibility of each component: the registry manages strategies,
strategies perform authentication.
@jhrozek
Add factory package to resolve auth import cycle
571bdfc 
Introduces pkg/vmcp/auth/factory to break the circular dependency
between pkg/vmcp/auth and pkg/vmcp/auth/strategies.

The import cycle occurred because:
- auth package needed to import strategies to instantiate them
- strategies package imported auth for Identity and context helpers

The factory package sits at the composition layer and can import both
auth (for interfaces) and strategies (for implementations) without
creating cycles.
@jhrozek
Integrate authentication registry into HTTP backend client
f6b287a 
Refactors HTTPBackendClient to accept an OutgoingAuthRegistry and
apply authentication strategies to all backend requests via a new
authRoundTripper middleware.

Authentication is now resolved and validated once at client creation
time rather than per-request, improving performance and enabling
early error detection for misconfigurations.

The authRoundTripper clones requests to preserve immutability before
applying authentication, ensuring thread-safety and preventing
unintended side effects.
@jhrozek
Apply auth configuration in backend discoverer
dec109a 
The CLI backend discoverer now accepts authentication configuration and
applies it to discovered backends during the discovery process.

This change enables per-backend authentication by:
- Adding authConfig parameter to NewCLIBackendDiscoverer constructor
- Implementing resolveAuthConfig() to select backend-specific or default
authentication settings with proper precedence
- Populating Backend.AuthStrategy and Backend.AuthMetadata fields during
backend creation

Authentication configuration follows this precedence:
1. Backend-specific configuration (cfg.Backends[backendID])
2. Default configuration (cfg.Default)
3. No authentication (if neither is configured)

The populated authentication fields are later consumed when converting
Backend instances to BackendTarget for use by the HTTP client's
authRoundTripper.
@jhrozek
Complete outgoing authentication integration in serve command
24dc69d 
Finalizes the end-to-end authentication flow by connecting the
authentication factory, backend discoverer, and HTTP client in the
serve command. This enables vMCP proxy to authenticate requests to
downstream MCP servers using configured authentication strategies.

The serve command now:
- Creates outgoing authenticator from configuration using the factory
- Provides authentication config to backend discoverer for setup
- Supplies authenticator to HTTP client for request signing
- Uses factory for incoming authentication middleware (consistency)

This completes the authentication architecture where configuration
flows through the factory to create strategies that are applied by
the client's round tripper to outgoing requests.

Also simplifies redundant type annotation in client variable
declaration for consistency with Go style conventions.
@jhrozek
Add explicit unauthenticated strategy for vMCP
1538dd0 
Replace the pattern of passing nil authenticators with an explicit
UnauthenticatedStrategy that implements the Strategy interface as a
no-op. This makes the intent clear in configuration and improves
type safety by eliminating nil checks.

The strategy is appropriate for backends on trusted networks or where
authentication is handled at the network layer. Configuration now
explicitly declares "strategy: unauthenticated" instead of relying
on implicit nil behavior.
@jhrozek
Implement HeaderInjection authentication strategy
3131412 
Add HeaderInjectionStrategy for injecting static header values into
backend requests. This general-purpose strategy supports any HTTP header
with any static value, enabling flexible authentication schemes like API
keys, bearer tokens, and custom auth headers.

The strategy extracts header_name and api_key from metadata configuration
and validates them to prevent CRLF injection attacks using pkg/validation
functions. Validation occurs at configuration time for fail-fast behavior.

Changes:
- Add HeaderInjectionStrategy implementation with Authenticate/Validate
- Include comprehensive test coverage (408 test lines)
- Use ValidateHTTPHeaderName/Value for security checks
- Prepared for future secret reference resolution
@jhrozek
Fix package name in factory directory
c117b5d 
Change package declaration from 'auth' to 'factory' in incoming.go
and incoming_test.go to match outgoing.go and prevent typecheck error.

All files in pkg/vmcp/auth/factory/ must use package factory.
Initial plan
d9623f2
Copilot AI mentioned this pull request Nov 4, 2025
Merged
Copilot AI changed the title [WIP] Update outgoing authentication strategies for vMCP Add test coverage for strategy name mismatch validation Nov 4, 2025
Copilot AI requested a review from jhrozek November 4, 2025 14:48
Copilot finished work on behalf of jhrozek November 4, 2025 14:48
@jhrozek jhrozek force-pushed the vmcp_out_strategies branch from c117b5d to f1dd6a2 Compare November 4, 2025 23:35
Base automatically changed from vmcp_out_strategies to main November 5, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhrozek jhrozek Awaiting requested review from jhrozek

At least 1 approving review is required to merge this pull request.

Assignees

@jhrozek jhrozek

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jhrozek