Skip to content

feat: honor KEY_PROVIDER_URL scheme and validate at config load - #272

Merged
danielpeng1 merged 1 commit into
masterfrom
WCN-1927/key-provider-url-scheme
Sep 25, 2026
Merged

danielpeng1 merged 1 commit into
masterfrom
WCN-1927/key-provider-url-scheme

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Stop rewriting the key provider URL in the client and validate provider URLs at config load.

  • KeyProviderClient: use the configured URL scheme; keep existing mTLS agent behavior when TLS_MODE=mtls
  • initConfig: validate KEY_PROVIDER_URL and optional BACKUP_KMS_URL; add ALLOW_PLAINTEXT_KEY_PROVIDER for local/dev setups
  • README and docker-compose.yml updates for the new env var

Tests

  • config.test.ts: URL scheme validation
  • keyProviderClient.test.ts: no scheme downgrade when TLS is disabled on the app

Ticket: WCN-1927

@linear-code

linear-code Bot commented Sep 25, 2026

Copy link
Copy Markdown

WCN-1927

@danielpeng1
danielpeng1 marked this pull request as ready for review September 25, 2026 05:46
@danielpeng1
danielpeng1 requested review from a team as code owners September 25, 2026 05:46
@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Claude finished @danielpeng1's task —— View job


PR Review: Key Provider URL Scheme Validation

Todo List:

  • Read and analyze changed files
  • Review configuration changes in initConfig.ts
  • Review KeyProviderClient changes
  • Analyze test coverage and quality
  • Check documentation updates
  • Provide comprehensive review feedback

Review Summary

This PR successfully implements URL scheme validation for key provider configurations and stops automatic URL rewriting in the client. The implementation is well-designed and addresses important security concerns around plaintext HTTP usage.

✅ Strengths

Security-Focused Design

  • Explicit scheme validation: validateKeyProviderUrl() in initConfig.ts:139-164 properly validates URL schemes and rejects invalid configurations
  • Safe defaults: Requires ALLOW_PLAINTEXT_KEY_PROVIDER=true for HTTP URLs, preventing accidental plaintext key transmission
  • Clear security warnings: Logs prominent warnings when plaintext HTTP is used (lines 161-163)

Clean Client Implementation

  • Honors configured scheme: KeyProviderClient in keyProviderClient.ts:40-55 now respects the configured URL scheme instead of rewriting it
  • Preserved mTLS behavior: Agent configuration still works correctly when TLS_MODE=mtls (lines 43-53)
  • Simplified logic: Removed complex URL rewriting, making the code more predictable

Comprehensive Test Coverage

  • URL scheme validation tests: config.test.ts:319-377 covers all validation scenarios including edge cases
  • Client behavior verification: keyProviderClient.test.ts:228-251 ensures no unwanted scheme downgrading
  • Both positive and negative test cases: Proper error conditions and success scenarios

Good Documentation

  • Updated README: Clear explanation of the new environment variable and security implications
  • Docker compose example: Shows proper usage with the new flag for development

🔍 Code Quality Observations

Configuration Validation (initConfig.ts)

// Lines 139-164: Well-structured validation function
function validateKeyProviderUrl(url: string, envVar: string, tlsMode: TlsMode): void {
  // Good: Proper URL parsing with error handling
  // Good: Clear error messages with context
  // Good: Security-conscious defaults
}

Client Changes (keyProviderClient.ts)

// Lines 40-55: Simplified and more predictable
const urlObj = new URL(cfg.keyProviderUrl);
// Good: Honors the configured scheme
// Good: Only sets up agent when actually needed for mTLS

💡 Minor Suggestions

  1. Error Message Consistency: The validation function could benefit from more consistent error message formatting across different validation failures.

  2. Type Safety: Consider adding a branded type for validated URLs to prevent passing unvalidated URLs to the client.

✅ Test Quality

The test coverage is excellent:

  • Comprehensive validation scenarios: All URL scheme combinations tested
  • Error message verification: Tests check specific error content
  • Client behavior verification: Ensures no regression in URL handling
  • Edge cases covered: Invalid URLs, missing schemes, backup KMS validation

📋 Final Assessment

Approve: This is a well-implemented security enhancement that:

  • ✅ Properly validates URL schemes at config load time
  • ✅ Maintains backward compatibility with existing configurations
  • ✅ Provides clear security guidance through warnings and documentation
  • ✅ Has comprehensive test coverage
  • ✅ Simplifies client code by removing URL rewriting logic

The PR successfully addresses the ticket requirements (WCN-1927) and improves the security posture of the application by making URL scheme handling explicit and validated.


Job Run: Link | Branch: WCN-1927/key-provider-url-scheme

@danielpeng1
danielpeng1 merged commit 6b52d82 into master Sep 25, 2026
23 checks passed
@danielpeng1
danielpeng1 deleted the WCN-1927/key-provider-url-scheme branch September 25, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants