-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: add proxy support for OpenAI-compatible providers #7574
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.
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest" | ||
| import { getProxyConfig } from "../proxy-config" | ||
|
|
||
| describe("getProxyConfig", () => { | ||
| let originalEnv: NodeJS.ProcessEnv | ||
|
|
||
| beforeEach(() => { | ||
| originalEnv = { ...process.env } | ||
| // Clear all proxy-related environment variables | ||
| delete process.env.HTTP_PROXY | ||
| delete process.env.http_proxy | ||
| delete process.env.HTTPS_PROXY | ||
| delete process.env.https_proxy | ||
| delete process.env.NO_PROXY | ||
| delete process.env.no_proxy | ||
|
|
||
| // Mock require for https-proxy-agent | ||
| vi.doMock("https-proxy-agent", () => ({ | ||
| HttpsProxyAgent: vi.fn().mockImplementation((url) => ({ | ||
| proxyUrl: url, | ||
| _isHttpsProxyAgent: true, | ||
| })), | ||
| })) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| process.env = originalEnv | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| describe("when no proxy is configured", () => { | ||
| it("should return undefined for any URL", () => { | ||
| expect(getProxyConfig("https://api.openai.com/v1")).toBeUndefined() | ||
| expect(getProxyConfig("http://localhost:8080")).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("when HTTP_PROXY is set", () => { | ||
| beforeEach(() => { | ||
| process.env.HTTP_PROXY = "http://proxy.example.com:8080" | ||
| }) | ||
|
|
||
| it("should return proxy config for HTTP URLs", () => { | ||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
|
|
||
| it("should return proxy config for HTTPS URLs when HTTPS_PROXY is not set", () => { | ||
| const config = getProxyConfig("https://api.example.com") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("when HTTPS_PROXY is set", () => { | ||
| beforeEach(() => { | ||
| process.env.HTTPS_PROXY = "https://secure-proxy.example.com:8443" | ||
| }) | ||
|
|
||
| it("should return proxy config for HTTPS URLs", () => { | ||
| const config = getProxyConfig("https://api.openai.com/v1") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
|
|
||
| it("should not use HTTPS_PROXY for HTTP URLs", () => { | ||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("when both HTTP_PROXY and HTTPS_PROXY are set", () => { | ||
| beforeEach(() => { | ||
| process.env.HTTP_PROXY = "http://proxy.example.com:8080" | ||
| process.env.HTTPS_PROXY = "https://secure-proxy.example.com:8443" | ||
| }) | ||
|
|
||
| it("should use HTTPS_PROXY for HTTPS URLs", () => { | ||
| const config = getProxyConfig("https://api.openai.com/v1") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
|
|
||
| it("should use HTTP_PROXY for HTTP URLs", () => { | ||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("NO_PROXY handling", () => { | ||
| beforeEach(() => { | ||
| process.env.HTTP_PROXY = "http://proxy.example.com:8080" | ||
| process.env.HTTPS_PROXY = "https://secure-proxy.example.com:8443" | ||
| }) | ||
|
|
||
| it("should bypass proxy for exact hostname match", () => { | ||
| process.env.NO_PROXY = "api.openai.com,localhost" | ||
| expect(getProxyConfig("https://api.openai.com/v1")).toBeUndefined() | ||
| expect(getProxyConfig("http://localhost:3000")).toBeUndefined() | ||
| }) | ||
|
|
||
| it("should bypass proxy for wildcard domain match", () => { | ||
| process.env.NO_PROXY = "*.internal.com,*.local" | ||
| expect(getProxyConfig("https://api.internal.com")).toBeUndefined() | ||
| expect(getProxyConfig("https://service.internal.com")).toBeUndefined() | ||
| expect(getProxyConfig("http://myapp.local")).toBeUndefined() | ||
| }) | ||
|
|
||
| it("should bypass proxy for subdomain match", () => { | ||
| process.env.NO_PROXY = "example.com" | ||
| expect(getProxyConfig("https://api.example.com")).toBeUndefined() | ||
| expect(getProxyConfig("https://example.com")).toBeUndefined() | ||
| }) | ||
|
|
||
| it("should not bypass proxy for non-matching domains", () => { | ||
| process.env.NO_PROXY = "example.com,*.local" | ||
| expect(getProxyConfig("https://api.openai.com")).toBeDefined() | ||
| expect(getProxyConfig("https://google.com")).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle spaces in NO_PROXY list", () => { | ||
| process.env.NO_PROXY = "example.com, localhost , *.local" | ||
| expect(getProxyConfig("http://localhost:3000")).toBeUndefined() | ||
| expect(getProxyConfig("https://example.com")).toBeUndefined() | ||
| expect(getProxyConfig("https://test.local")).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("case sensitivity", () => { | ||
| it("should handle lowercase proxy environment variables", () => { | ||
| process.env.http_proxy = "http://proxy.example.com:8080" | ||
| process.env.https_proxy = "https://secure-proxy.example.com:8443" | ||
|
|
||
| expect(getProxyConfig("http://api.example.com")).toBeDefined() | ||
| expect(getProxyConfig("https://api.example.com")).toBeDefined() | ||
| }) | ||
|
|
||
| it("should prefer uppercase over lowercase", () => { | ||
| process.env.http_proxy = "http://lower-proxy.example.com:8080" | ||
| process.env.HTTP_PROXY = "http://upper-proxy.example.com:8080" | ||
|
|
||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeDefined() | ||
| // The actual proxy URL used would be from HTTP_PROXY (uppercase) | ||
| }) | ||
|
|
||
| it("should handle lowercase no_proxy", () => { | ||
| process.env.HTTP_PROXY = "http://proxy.example.com:8080" | ||
| process.env.no_proxy = "localhost,example.com" | ||
|
|
||
| expect(getProxyConfig("http://localhost:3000")).toBeUndefined() | ||
| expect(getProxyConfig("https://example.com")).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("error handling", () => { | ||
| it("should return proxy config when https-proxy-agent module is available", () => { | ||
| // Since https-proxy-agent is available in the project, | ||
| // we test that it returns a valid proxy configuration | ||
| process.env.HTTP_PROXY = "http://proxy.example.com:8080" | ||
|
|
||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeDefined() | ||
| expect(config?.httpAgent).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle invalid proxy URLs gracefully", () => { | ||
| // Mock console.warn to capture the warning | ||
| const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) | ||
|
|
||
| // Test with an invalid proxy URL | ||
| process.env.HTTP_PROXY = "not-a-valid-url" | ||
|
|
||
| // Should return undefined for invalid URLs | ||
| const config = getProxyConfig("http://api.example.com") | ||
| expect(config).toBeUndefined() | ||
|
|
||
| // Should have logged a warning | ||
| expect(warnSpy).toHaveBeenCalledWith("Invalid proxy URL: not-a-valid-url") | ||
|
|
||
| warnSpy.mockRestore() | ||
| }) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { Agent } from "http" | ||
|
|
||
| /** | ||
| * Get proxy configuration from environment variables | ||
| * Respects standard proxy environment variables: HTTP_PROXY, HTTPS_PROXY, NO_PROXY | ||
| */ | ||
| export function getProxyConfig(targetUrl: string): { httpAgent?: Agent } | undefined { | ||
| // Dynamic import to avoid bundling issues | ||
| let HttpsProxyAgent: any | ||
| try { | ||
| HttpsProxyAgent = require("https-proxy-agent").HttpsProxyAgent | ||
| } catch (error) { | ||
| // If the module is not available, return undefined | ||
| console.warn("https-proxy-agent module not available, proxy support disabled") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a proper logging mechanism instead of |
||
| return undefined | ||
| } | ||
|
|
||
| // Check if the target URL should bypass proxy based on NO_PROXY | ||
| const noProxy = process.env.NO_PROXY || process.env.no_proxy | ||
| if (noProxy) { | ||
| const noProxyList = noProxy.split(",").map((s) => s.trim()) | ||
| const url = new URL(targetUrl) | ||
| const hostname = url.hostname | ||
|
|
||
| for (const pattern of noProxyList) { | ||
| // Handle wildcard patterns like *.example.com | ||
| if (pattern.startsWith("*")) { | ||
| const domain = pattern.slice(1) | ||
| if (hostname.endsWith(domain)) { | ||
| return undefined | ||
| } | ||
| } | ||
| // Handle exact matches | ||
| else if (hostname === pattern || hostname.endsWith(`.${pattern}`)) { | ||
| return undefined | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Determine which proxy to use based on the protocol | ||
| const url = new URL(targetUrl) | ||
| const isHttps = url.protocol === "https:" | ||
|
|
||
| // Check for proxy environment variables | ||
| const proxyUrl = isHttps | ||
| ? process.env.HTTPS_PROXY || process.env.https_proxy || process.env.HTTP_PROXY || process.env.http_proxy | ||
| : process.env.HTTP_PROXY || process.env.http_proxy | ||
|
|
||
| if (proxyUrl) { | ||
| try { | ||
| // Validate the proxy URL before creating the agent | ||
| new URL(proxyUrl) | ||
| // Create and return the proxy agent | ||
| const agent = new HttpsProxyAgent(proxyUrl) | ||
| return { httpAgent: agent } | ||
| } catch (error) { | ||
| console.warn(`Invalid proxy URL: ${proxyUrl}`) | ||
| return undefined | ||
| } | ||
| } | ||
|
|
||
| return undefined | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using
require()intentional here? Dynamic requires can cause issues with bundlers. Consider using dynamic import or ensuring the bundler handles this correctly. Also, ishttps-proxy-agentlisted in package.json dependencies?