fix(ssrf): pass pinned DNS lookup to ProxyAgent via requestTls#46756
Closed
yangyitao100 wants to merge 1 commit into
Closed
fix(ssrf): pass pinned DNS lookup to ProxyAgent via requestTls#46756yangyitao100 wants to merge 1 commit into
yangyitao100 wants to merge 1 commit into
Conversation
Contributor
Greptile SummaryThis PR attempts to close a SSRF gap in explicit-proxy mode by passing Key concerns:
Confidence Score: 2/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/net/ssrf.ts
Line: 376-384
Comment:
**`requestTls.lookup` may not enforce DNS pinning in proxy mode**
The fix places `lookup` inside `requestTls`, but `requestTls` configures TLS settings for connections to the origin server — it is not where DNS resolution is controlled.
In standard HTTP CONNECT proxy flow:
1. Client connects TCP/TLS to the proxy (using `proxyTls`)
2. Client sends `CONNECT api.telegram.org:443`
3. **The proxy** resolves `api.telegram.org` via its own DNS and opens a TCP connection to the origin
4. Client receives `200 Connection Established` and performs TLS over the tunnel socket (`requestTls`)
At step 4, there is no new TCP connection from the client, so `net.connect()` (which honours the `lookup` option) is never called. The pinned `lookup` function is therefore never invoked and the DNS rebinding protection does not apply.
Contrast this with the `Agent` / `EnvHttpProxyAgent` branches, which pass `lookup` inside `connect` — these do result in a fresh TCP connection from the client with custom DNS resolution.
The tests only mock the `ProxyAgent` constructor and verify which arguments are passed; they do not exercise the actual undici request dispatch path, so they cannot catch this discrepancy.
To properly guard against DNS rebinding in explicit-proxy mode the protection needs to be applied before the request is dispatched (e.g., validating that the request hostname matches the pre-resolved pinned hostname at dispatch time), rather than relying on a `lookup` override that is unlikely to be reached.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/net/ssrf.dispatcher.test.ts
Line: 130-147
Comment:
**Tests only verify constructor arguments, not actual DNS pinning**
Both the updated test and the new test verify that `ProxyAgent` is instantiated with the expected options using a fully-mocked constructor. This does not exercise the actual undici `ProxyAgent` request dispatch path, so these tests would pass even if `requestTls.lookup` is never called at runtime.
Consider adding an integration-style test (or at minimum a unit test that stubs `net.connect` / the undici connector) that verifies the pinned `lookup` function is actually invoked when a request is dispatched through the `ProxyAgent`. Without this, the correctness of the DNS pinning behaviour in explicit-proxy mode is not validated.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 3730f48 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+376
to
384
| const requestTls = withPinnedLookup(pinned.lookup); | ||
| if (!policy.proxyTls) { | ||
| return new ProxyAgent(proxyUrl); | ||
| return new ProxyAgent({ uri: proxyUrl, requestTls }); | ||
| } | ||
| return new ProxyAgent({ | ||
| uri: proxyUrl, | ||
| requestTls, | ||
| proxyTls: { ...policy.proxyTls }, | ||
| }); |
Contributor
There was a problem hiding this comment.
requestTls.lookup may not enforce DNS pinning in proxy mode
The fix places lookup inside requestTls, but requestTls configures TLS settings for connections to the origin server — it is not where DNS resolution is controlled.
In standard HTTP CONNECT proxy flow:
- Client connects TCP/TLS to the proxy (using
proxyTls) - Client sends
CONNECT api.telegram.org:443 - The proxy resolves
api.telegram.orgvia its own DNS and opens a TCP connection to the origin - Client receives
200 Connection Establishedand performs TLS over the tunnel socket (requestTls)
At step 4, there is no new TCP connection from the client, so net.connect() (which honours the lookup option) is never called. The pinned lookup function is therefore never invoked and the DNS rebinding protection does not apply.
Contrast this with the Agent / EnvHttpProxyAgent branches, which pass lookup inside connect — these do result in a fresh TCP connection from the client with custom DNS resolution.
The tests only mock the ProxyAgent constructor and verify which arguments are passed; they do not exercise the actual undici request dispatch path, so they cannot catch this discrepancy.
To properly guard against DNS rebinding in explicit-proxy mode the protection needs to be applied before the request is dispatched (e.g., validating that the request hostname matches the pre-resolved pinned hostname at dispatch time), rather than relying on a lookup override that is unlikely to be reached.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/net/ssrf.ts
Line: 376-384
Comment:
**`requestTls.lookup` may not enforce DNS pinning in proxy mode**
The fix places `lookup` inside `requestTls`, but `requestTls` configures TLS settings for connections to the origin server — it is not where DNS resolution is controlled.
In standard HTTP CONNECT proxy flow:
1. Client connects TCP/TLS to the proxy (using `proxyTls`)
2. Client sends `CONNECT api.telegram.org:443`
3. **The proxy** resolves `api.telegram.org` via its own DNS and opens a TCP connection to the origin
4. Client receives `200 Connection Established` and performs TLS over the tunnel socket (`requestTls`)
At step 4, there is no new TCP connection from the client, so `net.connect()` (which honours the `lookup` option) is never called. The pinned `lookup` function is therefore never invoked and the DNS rebinding protection does not apply.
Contrast this with the `Agent` / `EnvHttpProxyAgent` branches, which pass `lookup` inside `connect` — these do result in a fresh TCP connection from the client with custom DNS resolution.
The tests only mock the `ProxyAgent` constructor and verify which arguments are passed; they do not exercise the actual undici request dispatch path, so they cannot catch this discrepancy.
To properly guard against DNS rebinding in explicit-proxy mode the protection needs to be applied before the request is dispatched (e.g., validating that the request hostname matches the pre-resolved pinned hostname at dispatch time), rather than relying on a `lookup` override that is unlikely to be reached.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+130
to
+147
| it("passes pinned lookup via requestTls when proxyTls is absent", () => { | ||
| const lookup = vi.fn() as unknown as PinnedHostname["lookup"]; | ||
| const pinned: PinnedHostname = { | ||
| hostname: "api.telegram.org", | ||
| addresses: ["149.154.167.220"], | ||
| lookup, | ||
| }; | ||
|
|
||
| createPinnedDispatcher(pinned, { | ||
| mode: "explicit-proxy", | ||
| proxyUrl: "http://127.0.0.1:7890", | ||
| }); | ||
|
|
||
| expect(proxyAgentCtor).toHaveBeenCalledWith({ | ||
| uri: "http://127.0.0.1:7890", | ||
| requestTls: { lookup }, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Tests only verify constructor arguments, not actual DNS pinning
Both the updated test and the new test verify that ProxyAgent is instantiated with the expected options using a fully-mocked constructor. This does not exercise the actual undici ProxyAgent request dispatch path, so these tests would pass even if requestTls.lookup is never called at runtime.
Consider adding an integration-style test (or at minimum a unit test that stubs net.connect / the undici connector) that verifies the pinned lookup function is actually invoked when a request is dispatched through the ProxyAgent. Without this, the correctness of the DNS pinning behaviour in explicit-proxy mode is not validated.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/net/ssrf.dispatcher.test.ts
Line: 130-147
Comment:
**Tests only verify constructor arguments, not actual DNS pinning**
Both the updated test and the new test verify that `ProxyAgent` is instantiated with the expected options using a fully-mocked constructor. This does not exercise the actual undici `ProxyAgent` request dispatch path, so these tests would pass even if `requestTls.lookup` is never called at runtime.
Consider adding an integration-style test (or at minimum a unit test that stubs `net.connect` / the undici connector) that verifies the pinned `lookup` function is actually invoked when a request is dispatched through the `ProxyAgent`. Without this, the correctness of the DNS pinning behaviour in explicit-proxy mode is not validated.
How can I resolve this? If you propose a fix, please make it concise.createPinnedDispatcher() passed the SSRF-safe pinned lookup to Agent and EnvHttpProxyAgent but skipped it for ProxyAgent (explicit-proxy mode). This meant DNS resolution for origin servers behind an explicit proxy bypassed the pinned hostname check, potentially allowing DNS rebinding. Pass the pinned lookup in requestTls for ProxyAgent so all dispatcher modes enforce DNS pinning consistently. Closes openclaw#46685
yangyitao100
force-pushed
the
fix/issue-46685-pinned-dispatcher-proxy
branch
from
3730f48 to
aa54fa9
Compare
Author
|
Closing: too many versions behind, will re-evaluate on latest codebase. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
requestTlsfor SSRF protectionProblem
createPinnedDispatcher()correctly passes the SSRF-safe pinned hostname lookup toAgent(direct mode) andEnvHttpProxyAgent(env-proxy mode) via theirconnectoptions. However, forProxyAgent(explicit-proxy mode), the pinned lookup was never passed. This meant DNS resolution for origin servers behind an explicit proxy bypassed the pinned hostname check, potentially allowing DNS rebinding attacks.Root Cause
In
src/infra/net/ssrf.ts, the explicit-proxy branch (lines 372-379) constructedProxyAgenteither with a plain URL string or with{ uri, proxyTls }— neither of which included therequestTlsoption needed to carry the pinned lookup function.Fix
requestTls: { lookup: pinned.lookup }when constructingProxyAgentrequestTlsin the callrequestTlsis passed even whenproxyTlsis absentTest Plan
proxyTlsnow expectsrequestTls: { lookup }proxyTlsalso getsrequestTls: { lookup }Closes #46685
🤖 Generated with Claude Code