add URL scheme validation in DAHR and handleWeb2ProxyRequest#465
add URL scheme validation in DAHR and handleWeb2ProxyRequest#465massouji82 merged 5 commits intotestnetfrom
Conversation
WalkthroughAdds an HTTP(S) URL validator with SSRF protections, integrates it into START_PROXY handler and DAHR.startProxy, and adds runtime DNS-based SSRF preflight checks in the proxy. Also bumps Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Handler as START_PROXY handler
participant Validator as validateAndNormalizeHttpUrl
participant DAHR as DAHR.startProxy
participant DNS as DNS lookup
participant Proxy as HTTP Proxy
Client->>Handler: START_PROXY (web2Request.raw.url)
Handler->>Validator: validateAndNormalizeHttpUrl(url)
alt validation fails
Validator-->>Handler: {ok:false, status:400, message}
Handler-->>Client: RPC 400 (message)
else validation passes
Validator-->>Handler: {ok:true, normalizedUrl}
Handler->>DAHR: startProxy(with normalizedUrl)
DAHR->>Validator: validateAndNormalizeHttpUrl(normalizedUrl) %% defensive check
alt defensive validation fails
Validator-->>DAHR: {ok:false, status:400, message}
DAHR-->>Handler: throw Error(status/message)
Handler-->>Client: Error propagated
else defensive validation passes
DAHR->>DNS: resolve(targetHostname) note right of DNS: runtime DNS preflight
DNS-->>DAHR: addresses
alt disallowed address
DAHR-->>Client: 400 "Invalid target host"
else allowed
DAHR->>Proxy: forward request to normalizedUrl
Proxy-->>DAHR: response
DAHR-->>Handler: result
Handler-->>Client: Success
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to ce64b28
Previous suggestions✅ Suggestions up to commit a242c53
|
|||||||||||||||||||||||||||
…AHR and handleWeb2ProxyRequest
…t, and loopback addresses; update error handling in DAHR and handleWeb2ProxyRequest
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libs/network/routines/transactions/handleWeb2ProxyRequest.ts (2)
77-85: Sanitize forwarded headers; set Host to the normalized target.Passing user-supplied Host/X-Forwarded-* can cause cache poisoning or origin confusion. Strip sensitive hop-by-hop and proxy headers and set Host from the normalized URL.
Apply this diff:
- const { method, headers } = web2Request.raw + const { method, headers } = web2Request.raw + const target = new URL(validation.normalizedUrl) + const sanitizedHeaders = { ...headers } + delete (sanitizedHeaders as any).host + delete (sanitizedHeaders as any)["x-forwarded-host"] + delete (sanitizedHeaders as any)["x-forwarded-proto"] + delete (sanitizedHeaders as any)["x-forwarded-for"] + delete (sanitizedHeaders as any)["proxy-authorization"] + sanitizedHeaders.host = target.host @@ - const response = await dahr.startProxy({ + const response = await dahr.startProxy({ method, - headers, + headers: sanitizedHeaders, payload, authorization, url: validation.normalizedUrl, })
98-102: Map thrown status codes and avoid returning raw Error objects.Currently all errors are returned as 500 with the Error object in response. Respect
error.status(e.g., 400 from DAHR validation) and only return the message.Apply this diff:
- } catch (error: any) { - console.error("Error in handleWeb2ProxyRequest:", error) - - return createRPCResponse(500, error, error.message) - } + } catch (error: any) { + console.error("Error in handleWeb2ProxyRequest:", error) + const status = + typeof error?.status === "number" && error.status >= 400 && error.status < 600 + ? error.status + : 500 + const msg = typeof error?.message === "string" ? error.message : "Internal error" + return createRPCResponse(status, null, msg) + }
🧹 Nitpick comments (3)
src/features/web2/dahr/DAHR.ts (1)
74-81: Good: validate in DAHR as well (defense-in-depth), but use a typed error.Monkey-patching
err.statusis brittle. Prefer a small HttpError class.Apply:
+class HttpError extends Error { + status: number + constructor(status: number, message: string) { + super(message) + this.status = status + } +} @@ - const validation = validateAndNormalizeHttpUrl(url) - if (!validation.ok) { - const err = new Error(validation.message) - ;(err as any).status = validation.status - throw err - } + const validation = validateAndNormalizeHttpUrl(url) + if (!validation.ok) { + throw new HttpError(validation.status, validation.message) + }src/features/web2/validator.ts (2)
74-91: Nit: reusing the same URL object avoids extra parse.You can mutate the existing
parsedinstead of round-tripping throughnew URL(parsed.toString()).Apply:
- const canonicalUrlObject = new URL(parsed.toString()) - canonicalUrlObject.hostname = canonicalUrlObject.hostname.toLowerCase() + const canonicalUrlObject = parsed + canonicalUrlObject.hostname = canonicalUrlObject.hostname.toLowerCase()
1-3: Type is clear and ergonomic. Add unit tests for edge cases.Please add tests for:
- IPv4-mapped IPv6 (::ffff:127.0.0.1)
- RFC1918 IPv4 ranges
- Link-local (169.254.0.0/16, fe80::/10)
- IDN/punycode hostnames
- Default port stripping and fragment removal
I can generate a jest test suite covering these cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
package.json(1 hunks)src/features/web2/dahr/DAHR.ts(2 hunks)src/features/web2/validator.ts(1 hunks)src/libs/network/routines/transactions/handleWeb2ProxyRequest.ts(3 hunks)
🔇 Additional comments (5)
package.json (1)
49-49: Confirm necessity of new dependency @aptos-labs/ts-sdk.Looks unrelated to the URL-validation change. If unused, please drop it to keep the diff focused.
Run to verify usage:
#!/bin/bash # Find imports/usages of @aptos-labs/ts-sdk rg -nP --type=ts -C2 $'from\\s+[\'"]@aptos-labs/ts-sdk[\'"]|require\\([\'"]@aptos-labs/ts-sdk[\'"]\\)'src/libs/network/routines/transactions/handleWeb2ProxyRequest.ts (1)
61-71: Good defensive validation with clear 400s.Early scheme validation and normalized URL usage are solid.
src/features/web2/dahr/DAHR.ts (1)
82-89: LGTM: normalized URL is used for the proxied request.This keeps downstream components consistent.
src/features/web2/validator.ts (2)
15-33: LGTM: clear parse and scheme checks.Trimming, URL() parse, and http/https enforcement are correct.
34-42: LGTM: credential-in-URL rejection.Prevents accidental secrets leakage.
| const validation = validateAndNormalizeHttpUrl( | ||
| web2Request.raw.url, | ||
| ) | ||
| if (!validation.ok) { | ||
| return createRPCResponse( | ||
| validation.status, | ||
| null, | ||
| validation.message, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
SSRF gaps remain: DNS and redirects can still target private/metadata IPs.
Blocking only localhost/loopback at parse-time is insufficient. Attackers can use a public hostname that resolves to 169.254.169.254, 10.0.0.0/8, fc00::/7, etc., or abuse redirects. Enforce IP allow/block after DNS resolution and on every redirect in the Proxy layer.
Action:
- In Proxy HTTP client, provide a custom lookup/agent to resolve the target and reject connections to private/link-local/reserved ranges (IPv4 and IPv6, including IPv4-mapped IPv6).
- Re-check the resolved address after each redirect hop before following it.
- Consider an allowlist instead (e.g., public CIDRs) if feasible.
Want a patch against Proxy to add a guarded lookup + redirect guard?
🤖 Prompt for AI Agents
In src/libs/network/routines/transactions/handleWeb2ProxyRequest.ts around lines
61-71: the current URL validation only blocks parse-time localhost/loopback
which leaves SSRF via DNS and redirects possible; update the Proxy HTTP client
to use a custom DNS lookup/agent that resolves hostnames before connecting and
rejects/resolves-to addresses in private/link-local/reserved ranges (e.g.,
10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16,
100.64.0.0/10, 192.0.0.0/24, 198.18.0.0/15, fc00::/7, fe80::/10, IPv4-mapped
IPv6 ranges), add the same check for every redirect hop before following it, and
fail the request if a resolved IP is disallowed; implement this by injecting a
guarded lookup function into the HTTP/HTTPS agent used by the proxy, optionally
replace the blacklist with an allowlist of public CIDRs if policy permits, and
ensure errors include clear status/messages so createRPCResponse can return
appropriate error codes.
…ivate, link-local, and loopback addresses; update dependencies
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/web2/proxy/Proxy.ts (1)
250-266: Mitigate DNS rebinding by enforcing a safe lookup in the upstream connection.You validate via preflight, but the actual
http(s).requestin http-proxy will resolve again, enabling TOCTOU DNS rebinding. Provide an Agent with a guardedlookupthat rejects disallowed IPs.const proxyServer = httpProxy.createProxyServer({ target: targetUrl, changeOrigin: true, secure: targetProtocol === "https:" ? this._sslConfig.verifyCertificates : false, ssl: targetProtocol === "https:" ? { rejectUnauthorized: this._sslConfig.verifyCertificates, } : undefined, + // Use a guarded lookup to ensure the connected IP remains allowed + agent: targetProtocol === "https:" + ? new https.Agent({ + keepAlive: true, + rejectUnauthorized: this._sslConfig.verifyCertificates, + lookup: (hostname, options, cb) => { + dns.lookup(hostname, options as any, (err: any, address: any, family: any) => { + if (err) return cb(err, address, family) + if (Array.isArray(address)) { + const allowed = address.find(a => !isDisallowedAddress(a.address)) + return allowed + ? cb(null, allowed.address, allowed.family) + : cb(new Error("Resolved to disallowed address"), undefined as any, undefined as any) + } + return isDisallowedAddress(String(address)) + ? cb(new Error("Resolved to disallowed address"), undefined as any, undefined as any) + : cb(null, address, family) + }) + }, + }) + : new http.Agent({ + keepAlive: true, + lookup: (hostname, options, cb) => { + dns.lookup(hostname, options as any, (err: any, address: any, family: any) => { + if (err) return cb(err, address, family) + if (Array.isArray(address)) { + const allowed = address.find(a => !isDisallowedAddress(a.address)) + return allowed + ? cb(null, allowed.address, allowed.family) + : cb(new Error("Resolved to disallowed address"), undefined as any, undefined as any) + } + return isDisallowedAddress(String(address)) + ? cb(new Error("Resolved to disallowed address"), undefined as any, undefined as any) + : cb(null, address, family) + }) + }, + }), })This binds the connection to an IP vetted by your policy and prevents rebinding between preflight and connect.
Also applies to: 342-349
🧹 Nitpick comments (4)
src/features/web2/proxy/Proxy.ts (2)
223-248: Don’t reject the already-resolved server Promise from inside preflight; add a DNS timeout.Calling
reject(e)here targets the outercreateNewServerpromise, which is already settled by request time—this is a no-op and confusing. Also,dns.lookupcan hang; add a short timeout.- const preflight = async () => { + const DNS_TIMEOUT_MS = 2000 + const preflight = async () => { try { // If hostname is already an IP, just check it; otherwise resolve all const ipVersion = net.isIP(targetHostname) if (ipVersion) { if (isDisallowedAddress(targetHostname)) { throw new Error( "Target resolves to a private/link-local/loopback address", ) } } else { - const answers = await dns.lookup(targetHostname, { - all: true, - }) + const answers = await Promise.race([ + dns.lookup(targetHostname, { all: true }), + new Promise<never>((_, rej) => + setTimeout(() => rej(new Error("DNS lookup timeout")), DNS_TIMEOUT_MS), + ), + ]) if (answers.some(a => isDisallowedAddress(a.address))) { throw new Error( "Target resolves to a private/link-local/loopback address", ) } } } catch (e) { - reject(e) return false } return true }
320-334: Per-request preflight is good; consider returning a JSON error body for clients.Small DX improvement: include a machine-readable error.
- if (!ok) { - res.writeHead(400) - res.end("Invalid target host") + if (!ok) { + res.writeHead(400, { "Content-Type": "application/json" }) + res.end(JSON.stringify({ error: "Invalid target host" })) return }src/features/web2/validator.ts (2)
54-64: Tighten localhost checks to catch trailing-dot variants.
localhost.and*.localhost.should also be rejected.- if (hostLower === "localhost" || hostLower.endsWith(".localhost")) { + if ( + hostLower === "localhost" || + hostLower === "localhost." || + hostLower.endsWith(".localhost") || + hostLower.endsWith(".localhost.") + ) { return { ok: false, status: 400, message: "Localhost targets are not allowed", } }
17-24: Avoid drift: share the address-blocking helper with Proxy.
validator.tsandProxy.tsnow implement overlapping blocklists. Extract a small shared helper (e.g., src/features/web2/net/isDisallowedAddress.ts) and use it in both places to keep behavior consistent.Also applies to: 103-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
package.json(1 hunks)src/features/web2/proxy/Proxy.ts(3 hunks)src/features/web2/validator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (1)
src/features/web2/proxy/Proxy.ts (1)
6-6: LGTM: importing node:dns/promises for preflight is appropriate.
…l IPv6 multicast and unspecified addresses; enhance disallowed address checks for IPv4
1 participant
User description
add URL scheme validation in DAHR and handleWeb2ProxyRequest
PR Type
Enhancement
Description
Add URL scheme validation to prevent transport crashes
Restrict proxy requests to HTTP/HTTPS protocols only
Update demosdk dependency to version 2.3.22
Diagram Walkthrough
File Walkthrough
DAHR.ts
Add URL scheme validation in DAHRsrc/features/web2/dahr/DAHR.ts
handleWeb2ProxyRequest.ts
Add URL validation in proxy handlersrc/libs/network/routines/transactions/handleWeb2ProxyRequest.ts
package.json
Update demosdk dependency versionpackage.json
@kynesyslabs/demosdkfrom 2.3.17 to 2.3.22Summary by CodeRabbit
Bug Fixes
New Features
Chores