Fix bedrock DNS resolution when behind a corporate proxy#906
Conversation
When behind a corporate proxy, Node.js resolves DNS locally before contacting the proxy. If the proxy is the only path to the endpoint, the request fails with ENOTFOUND. HttpsProxyAgent uses CONNECT tunneling so the proxy handles DNS resolution. Configure NodeHttpHandler with HttpsProxyAgent when a system proxy is detected.
LouisClt
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthroughBedrock now resolves proxy settings from environment variables or VS Code configuration and routes AWS requests through an HTTPS proxy agent when configured. Tests cover proxied, direct, and API-key authentication scenarios, with dependencies and a patch changeset added. ChangesBedrock proxy routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AwsBedrockHandler
participant getSystemProxyUrl
participant HttpsProxyAgent
participant NodeHttpHandler
participant BedrockRuntimeClient
AwsBedrockHandler->>getSystemProxyUrl: Resolve system proxy URL
getSystemProxyUrl-->>AwsBedrockHandler: Return URL or undefined
AwsBedrockHandler->>HttpsProxyAgent: Create proxy agent
AwsBedrockHandler->>NodeHttpHandler: Create request handler
AwsBedrockHandler->>BedrockRuntimeClient: Set requestHandler
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LouisClt
force-pushed
the
fix_bedrock_proxy
branch
from
ad39470 to
204a5da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/networkProxy.ts`:
- Around line 354-360: Update the environment proxy selection in the surrounding
proxy-resolution function to trim the chosen
HTTPS_PROXY/https_proxy/HTTP_PROXY/http_proxy value before returning it, and
treat the trimmed result as unset when blank so resolution can continue to the
fallback path. Preserve the existing HTTPS-over-HTTP precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6711a1da-2a4c-48ec-a911-3f1c9a030d08
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.changeset/itchy-moles-thank.mdsrc/api/providers/__tests__/bedrock.spec.tssrc/api/providers/bedrock.tssrc/package.jsonsrc/utils/networkProxy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/itchy-moles-thank.md
- src/package.json
- src/api/providers/bedrock.ts
- src/api/providers/tests/bedrock.spec.ts
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | ||
| const fromEnv = | ||
| process.env.HTTPS_PROXY || | ||
| process.env.https_proxy || | ||
| process.env.HTTP_PROXY || | ||
| process.env.http_proxy | ||
| if (fromEnv) return fromEnv |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim environment proxy values before returning them.
Unlike the VS Code fallback, environment values are not trimmed. A whitespace-only HTTPS_PROXY/HTTP_PROXY value is truthy and will be passed to HttpsProxyAgent as an invalid proxy URL, preventing Bedrock client initialization. Normalize the selected environment value and treat blank values as unset.
Proposed fix
- const fromEnv =
+ const fromEnv = (
process.env.HTTPS_PROXY ||
process.env.https_proxy ||
process.env.HTTP_PROXY ||
process.env.http_proxy
- if (fromEnv) return fromEnv
+ )?.trim()
+ if (fromEnv) return fromEnv📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | |
| const fromEnv = | |
| process.env.HTTPS_PROXY || | |
| process.env.https_proxy || | |
| process.env.HTTP_PROXY || | |
| process.env.http_proxy | |
| if (fromEnv) return fromEnv | |
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | |
| const fromEnv = ( | |
| process.env.HTTPS_PROXY || | |
| process.env.https_proxy || | |
| process.env.HTTP_PROXY || | |
| process.env.http_proxy | |
| )?.trim() | |
| if (fromEnv) return fromEnv |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/networkProxy.ts` around lines 354 - 360, Update the environment
proxy selection in the surrounding proxy-resolution function to trim the chosen
HTTPS_PROXY/https_proxy/HTTP_PROXY/http_proxy value before returning it, and
treat the trimmed result as unset when blank so resolution can continue to the
fallback path. Preserve the existing HTTPS-over-HTTP precedence.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
1 participant
When behind a corporate proxy, Node.js resolves DNS locally before contacting the proxy. If the proxy is the only path to the endpoint, the request fails with ENOTFOUND.
HttpsProxyAgent uses CONNECT tunneling so the proxy handles DNS resolution. Configure NodeHttpHandler with HttpsProxyAgent when a system proxy is detected.
I had the need to use bedrock behind a corporate proxy, as there was no fix available, I made one so that I could use it, using Claude code. Here is a proposed PR if you want to merge it to the main branch ...
I reviewed the changes myself, and it seems to be good, but I am no typescript expert so ...
Related GitHub Issue
Closes: #905
Description
Node.js resolves DNS locally before contacting the proxy. When behind a corporate proxy that blocks external DNS, Bedrock calls fail with
ENOTFOUND.This configures
NodeHttpHandlerwithHttpsProxyAgentinBedrockRuntimeClient, which uses CONNECT tunneling so the proxy handles DNS resolution.Proxy is detected from
HTTPS_PROXY/HTTP_PROXYenv vars or VS Codehttp.proxysetting (which seems to be filled from the windows configured proxy).Test Procedure
Test Procedure
pnpm test -- bedrock.spec→ 102 passingPre-Submission Checklist
Documentation Updates
[x] No documentation updates are required.
Additional Notes
VS Code automatically detects Windows system proxy, so this works transparently for users with proxy configured in Internet Options.
Get in Touch
I have no discord username for the moment.
Thanks
Summary by CodeRabbit