feat: support webhook delivery through proxy

[alexluong]

resolves #504

Implemented webhook proxy support via DESTINATIONS_WEBHOOK_PROXY_URL environment variable, allowing users to route webhook traffic through a proxy server for easier migration from existing systems with static IPs. The implementation uses Go's built-in http.ProxyURL() for proxy configuration and is isolated to webhook destinations only.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
outpost-docs	Ready	Preview	Comment	Oct 7, 2025 0:25am
outpost-website	Ready	Preview	Comment	Oct 7, 2025 0:25am

[claude]

Code Review - PR #513: Webhook Proxy Support

Thanks for implementing webhook proxy support! This is a well-structured implementation that adds useful functionality for users migrating from existing systems with static IPs. Here's my detailed review:

✅ Strengths

1. Clean Implementation: The proxy configuration is properly isolated to webhook destinations only, using Go's standard http.ProxyURL() function
2. Good Test Coverage: Added comprehensive unit tests for both proxy configuration and user agent handling
3. Documentation: Updated configuration docs with clear descriptions of the new environment variable
4. Error Handling: Proper validation and error messages for invalid proxy URLs

🐛 Potential Issues

Critical: Missing Test Setup Function

Location: internal/destregistry/baseprovider_test.go:17

The test file references newMockProvider() but doesn't define it. This will cause compilation failure.

Fix needed: Add the mock provider setup function to the test file. You can reference the pattern from internal/destregistry/registry_test.go:83-92:

func newMockProvider() (*destregistry.BaseProvider, error) {
    return destregistry.NewBaseProvider(&mockMetadataLoader{}, "mock")
}

type mockMetadataLoader struct{}

func (m *mockMetadataLoader) Load(providerType string) (*metadata.ProviderMetadata, error) {
    return &metadata.ProviderMetadata{
        Type: providerType,
    }, nil
}

Minor: Inconsistent Whitespace Changes

Location: internal/app/app.go:201-204

The PR includes unrelated formatting changes (list indentation from spaces to tabs). While these improve consistency, they should ideally be in a separate commit or the PR description should mention reformatting.

🔒 Security Considerations

1. Proxy URL Validation: ✅ Good - properly validates and returns error for malformed URLs
2. No Credential Exposure: ✅ The proxy URL is configured via environment variable, which is appropriate for credentials
3. Scope Limitation: ✅ Excellent - proxy only affects webhook destinations, not internal communications

Recommendation: Consider documenting security implications in the configuration docs:

• Proxy URLs may contain authentication credentials (e.g., http://user:pass@proxy:8080)
• These should be treated as secrets and not logged

🚀 Performance Considerations

1. Transport Cloning: The implementation clones http.DefaultTransport which is good practice - avoids modifying shared state
2. Client Reuse: HTTP clients are created per publisher and reused, which is optimal
3. No Connection Pooling Issues: The transport configuration properly preserves connection pooling from the default transport

🧪 Test Coverage Assessment

Good Coverage:

• ✅ Proxy routing validation
• ✅ Invalid proxy URL handling
• ✅ User agent injection
• ✅ Empty user agent handling

Missing Coverage:

• ⚠️ No E2E test verifying proxy actually works end-to-end with webhook delivery
• ⚠️ No test for proxy URL with authentication (http://user:pass@proxy:8080)
• ⚠️ No test combining both proxy and user agent together (though this should work based on the implementation)

Recommendation: Consider adding an E2E test in cmd/e2e/destwebhook_test.go that:

1. Sets up a test proxy server
2. Configures a webhook destination with proxy URL
3. Publishes an event
4. Verifies the request went through the proxy

📝 Code Quality

Best Practices: ✅

• Clean separation of concerns with userAgentTransport wrapper
• Proper nil checking for optional parameters
• Good use of pointer types for optional config

Naming: ✅ Clear and consistent variable names

Documentation: ⚠️ Consider adding godoc comments for:

• userAgentTransport type (line 243)
• HTTPClientConfig.ProxyURL field

🔧 Recommendations

1. Fix the compilation issue by adding the missing test setup code
2. Consider adding proxy authentication test to ensure URLs like http://user:pass@proxy:8080 work correctly
3. Add godoc comments for new exported types/fields
4. Optional: Add E2E test for complete proxy flow verification

Summary

This is a solid implementation with good architecture and test coverage. The only blocking issue is the missing test setup function that will prevent compilation. Once that's fixed, this should be ready to merge after addressing any optional recommendations you'd like to include.

Approval Status: Approve with changes ✅ (fix compilation issue first)

---

🤖 Generated with Claude Code

[alexbouchardd]

@alexluong How do we handle authentication with the proxy? In some cases, the proxy might live in a different network boundary.

[claude]

Pull Request Review: Webhook Proxy Support

Overview

This PR adds webhook proxy support through the DESTINATIONS_WEBHOOK_PROXY_URL environment variable. The implementation is well-designed and focused, properly isolating the proxy functionality to webhook destinations only.

Code Quality ✅

Strengths

1. Clean Architecture: The proxy configuration is properly layered through the config → provider → client chain
2. Good Separation of Concerns: Proxy logic is isolated to webhook destinations, not affecting other destination types (Hookdeck, AWS Kinesis, etc.)
3. Proper Error Handling: Invalid proxy URLs are caught and return meaningful errors
4. Test Coverage: New unit tests cover both proxy routing and user agent functionality
5. Documentation: Configuration is well-documented in the MDX file

Code Structure

• The refactoring of MakeHTTPClient to return (*http.Client, error) is correct and necessary for proxy URL validation
• The userAgentTransport wrapper is a clean approach to combining proxy and user agent functionality
• Transport cloning from http.DefaultTransport preserves good defaults

Issues Found 🔴

Critical: Missing Test Helper Function

Location: internal/destregistry/baseprovider_test.go:17,66

The test file references newMockProvider() but this function is not defined in baseprovider_test.go. It's defined in registry_test.go but is not exported or accessible from the new test file.

Impact: Tests will fail to compile.

Fix Required: Add the helper function to baseprovider_test.go:

func newMockProvider() (*destregistry.BaseProvider, error) {
    loader := metadata.NewEmbeddedMetadataLoader()
    return destregistry.NewBaseProvider(loader, "webhook")
}

Or import it from a shared test utilities package if one exists.

Minor: Formatting Inconsistency

Location: internal/app/app.go:198-207

The indentation was changed from spaces to proper alignment, but this appears to be an unrelated formatting change. While it's an improvement, it's not directly related to the proxy feature.

Recommendation: This is fine as-is, but consider whether formatting-only changes should be in separate commits.

Security Considerations ⚠️

Proxy Credentials in URLs

Location: internal/config/destinations.go:38-40

The code correctly identifies that proxy URLs may contain credentials (http://user:pass@proxy:8080). The TODO comment references issue #480 for sensitive value handling.

Recommendations:

1. ✅ The TODO is appropriate and links to a tracking issue
2. Consider if proxy URLs should be obfuscated in logs/responses (though the current implementation doesn't expose them inappropriately)
3. Ensure proxy URLs aren't logged in plaintext anywhere in the codebase

No Proxy Bypass

The current implementation doesn't support NO_PROXY or similar environment variables for bypassing the proxy for certain destinations. This is acceptable for an initial implementation but may be needed later for:

• Internal endpoints that shouldn't go through proxy
• Localhost/private IP ranges

Performance Considerations ✅

1. Transport Reuse: The code properly clones http.DefaultTransport, which includes connection pooling
2. No Performance Regression: For users not using proxy, the performance impact is minimal (just a nil check)
3. Proxy Connection Pooling: Go's http.Transport handles proxy connection pooling automatically

Test Coverage 📊

What's Covered ✅

• User agent header injection
• Proxy routing for HTTP requests
• Invalid proxy URL error handling
• Empty user agent string handling

Gaps in Coverage ⚠️

1. Proxy with Authentication: No test for proxy URLs with embedded credentials (e.g., http://user:pass@proxy:8080)
2. Combination Tests: No test verifying proxy + user agent work together correctly
3. Integration Testing: The tests are unit tests with mock servers; consider adding integration tests with actual webhook destinations

Recommendation: Add a test case for authenticated proxies:

t.Run("handles proxy with authentication", func(t *testing.T) {
    proxyWithAuth := "http://user:pass@proxy-server:8080"
    client, err := provider.MakeHTTPClient(destregistry.HTTPClientConfig{
        ProxyURL: &proxyWithAuth,
    })
    require.NoError(t, err)
    assert.NotNil(t, client)
})

Additional Observations

Positive

1. Error propagation is handled correctly in both desthookdeck.go:149 and destwebhook.go:228
2. The proxy configuration is optional and backward compatible
3. Documentation includes clear descriptions of when the feature applies

Questions

1. Proxy for Hookdeck Destinations: The desthookdeck provider also calls MakeHTTPClient but doesn't pass ProxyURL. Is this intentional? Should Hookdeck destinations also support proxies, or is the isolation intentional?

Summary

This is a solid implementation with good architecture and testing. The main blocking issue is the missing test helper function that will cause compilation failures. Once that's fixed, this PR will be ready to merge.

Required Before Merge

• ❌ Fix missing newMockProvider() function in baseprovider_test.go

Recommended Improvements

• ⚠️ Add test for authenticated proxy URLs
• ⚠️ Add test for proxy + user agent combination
• 💡 Consider future support for NO_PROXY/proxy bypass rules

Overall Assessment

Quality: High
Security: Good (with TODO for credential handling)
Test Coverage: Good (with room for improvement)
Ready to Merge: No (blocking test issue)

Great work on this feature! The proxy support will be valuable for users migrating from systems with static IPs. 🚀

[alexluong]

hmm I think there's something @claude is missing here, the newMockProvider() is already in scope in the test package

---

@alexbouchardd the current mechanism only supports a proxy url, so auth must be part of the URL, i.e. https://username:password@proxy.com

[claude]

Claude finished @alexluong's task —— View job

---

Code Review - PR #513: Webhook Proxy Support (Corrected)

You're absolutely right - I apologize for the error in my previous reviews. The newMockProvider() function is indeed in scope since both baseprovider_test.go and registry_test.go are in the same destregistry_test package. In Go, all functions in the same package are accessible across files, so there's no compilation issue.

---

✅ Overall Assessment

This is a well-implemented feature that adds webhook proxy support cleanly and safely. The implementation properly isolates proxy functionality to webhook destinations only and includes good test coverage.

Code Quality ✅

Strengths:

1. Clean Architecture: Proxy configuration flows properly through config → provider → client
2. Good Separation: Proxy logic isolated to webhook destinations (not affecting Hookdeck, AWS Kinesis, etc.)
3. Proper Error Handling: Invalid proxy URLs caught with meaningful error messages
4. Test Coverage: Comprehensive unit tests for both proxy routing and user agent functionality
5. Documentation: Configuration well-documented in MDX file
6. Best Practices:
   • Clones http.DefaultTransport to avoid modifying shared state
   • Uses userAgentTransport wrapper for clean composition
   • HTTP clients are created per publisher and reused

Security ✅

1. Proxy URL with Credentials: Code correctly handles authentication URLs like http://user:pass@proxy:8080 (as confirmed by @alexluong's response)
2. Sensitive Value Handling: TODO comment at internal/config/destinations.go:38-40 appropriately tracks issue Improve Config & secrets handling #480 for sensitive value obfuscation
3. Scope Limitation: Proxy only affects webhook destinations, not internal communications

Performance ✅

• Transport cloning preserves connection pooling from default transport
• No performance impact for users not using proxy (just a nil check)
• Go's http.Transport handles proxy connection pooling automatically

Test Coverage 📊

Well Covered:

• ✅ User agent header injection (baseprovider_test.go:14-61)
• ✅ Proxy routing validation (baseprovider_test.go:69-106)
• ✅ Invalid proxy URL error handling (baseprovider_test.go:108-119)
• ✅ Empty user agent handling (baseprovider_test.go:48-60)

Optional Enhancements:

• Consider adding test for authenticated proxy URLs (http://user:pass@proxy:8080)
• Consider E2E test in cmd/e2e/destwebhook_test.go verifying proxy works end-to-end

Minor Observations

1. Formatting Change (internal/app/app.go:198-207): Indentation cleanup is fine but unrelated to the feature - could have been separate commit
2. Documentation: Consider adding godoc comments for userAgentTransport type (baseprovider.go:243) and HTTPClientConfig.ProxyURL field

Summary

Ready to merge: ✅ Yes

This implementation is solid with no blocking issues. The proxy support will be valuable for users migrating from systems with static IPs. Great work on this feature!

---

Proxy Authentication Note (per @alexluong's comment): Authentication is supported via the proxy URL format https://username:password@proxy.com

---

Job Run • Branch: webhook-proxy