feat: add proxy support for mcp server CF-2438#81
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request introduces proxy and SSL configuration support using the undici library, allowing outbound requests to respect environment variables like HTTP_PROXY, HTTPS_PROXY, and NO_PROXY. The review feedback highlights two potential robustness issues where missing protocols in proxy URIs or request origins could cause TypeError exceptions and crash the application. Code suggestions are provided to safely sanitize and parse these URLs.
There was a problem hiding this comment.
Pull Request Overview
The current implementation is not up to standards and contains critical flaws that prevent it from fulfilling its requirements. The most significant issue is an architectural mismatch: proxy configuration is applied to the undici global dispatcher, but the project uses node-fetch, which does not respect this dispatcher. Consequently, the proxy settings will be ignored by the main API client.
Furthermore, the implementation introduces high-security risks by allowing global bypass of TLS certificate validation, exposing the server to Man-in-the-Middle (MITM) attacks. There is also a gap in requirements as the implementation fails to support proxy configurations injected explicitly from the extension, relying solely on environment variables. Finally, src/proxy.ts is identified as a high-risk file due to its cyclomatic complexity and total lack of test coverage.
About this PR
- The project relies on
node-fetch, but the proxy implementation configuration targets theundiciglobal dispatcher. These two are incompatible in the current environment; existing API requests will bypass the proxy entirely. - Please provide a PR description detailing the changes, the rationale behind the chosen implementation, and steps taken for verification.
Test suggestions
- Outbound requests are routed through the configured HTTP_PROXY
- Outbound requests are routed through the configured HTTPS_PROXY
- Requests to hosts in NO_PROXY bypass the proxy and connect directly
- TLS certificate validation is disabled when NODE_TLS_REJECT_UNAUTHORIZED is set to '0'
- Unit tests for ProxyRoutingDispatcher logic (src/proxy.ts has 0% coverage)
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Outbound requests are routed through the configured HTTP_PROXY
2. Outbound requests are routed through the configured HTTPS_PROXY
3. Requests to hosts in NO_PROXY bypass the proxy and connect directly
4. TLS certificate validation is disabled when NODE_TLS_REJECT_UNAUTHORIZED is set to '0'
5. Unit tests for ProxyRoutingDispatcher logic (src/proxy.ts has 0% coverage)
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
No description provided.