Handle non-IP DNS lookup results#957
Conversation
Ignore non-IP aliases emitted by Node-compatible DNS implementations when a lookup also returns actual IP addresses. Fail closed for alias-only results and continue checking every returned IP. Assisted-by: Codex:gpt-5
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the validateLookupAddresses function to handle DNS lookup results, specifically ignoring CNAME records from Node.js-compatible runtimes while ensuring actual IP addresses are validated. However, two security concerns were identified: first, a critical SSRF vulnerability where DNS lookup failures are silently caught and ignored, which could allow requests to bypass validation; second, a defense-in-depth improvement to fail closed and throw an error if no valid IP addresses are returned at all.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } catch { | ||
| addresses = []; | ||
| } | ||
| for (const { address, family } of addresses) { | ||
| validateLookupAddresses(addresses); |
There was a problem hiding this comment.
Security Vulnerability: SSRF Bypass via DNS Lookup Failure
Currently, if the DNS lookup fails (e.g., due to a temporary network issue, DNS timeout, or rate limiting), the catch block silently ignores the error and sets addresses = [].
Since addresses is empty, validateLookupAddresses(addresses) does not throw any error because addresses.length > 0 is false. This allows validatePublicUrl to complete successfully.
When the application subsequently performs the actual HTTP request (e.g., via fetch), the DNS resolution might succeed (either because the temporary issue resolved, or as part of a DNS rebinding attack). If it resolves to a private IP address (e.g., 127.0.0.1), the request will be sent to the private network, completely bypassing the SSRF protection.
Remediation:
Fail closed by throwing a UrlError when the DNS lookup fails.
| } catch { | |
| addresses = []; | |
| } | |
| for (const { address, family } of addresses) { | |
| validateLookupAddresses(addresses); | |
| } catch (error) { | |
| throw new UrlError("DNS lookup failed", { cause: error }); | |
| } | |
| validateLookupAddresses(addresses); |
| if (addresses.length > 0 && ipAddressCount === 0) { | ||
| throw new UrlError("DNS lookup did not return an IP address"); | ||
| } |
There was a problem hiding this comment.
Security Improvement: Fail Closed on Empty DNS Results
If the DNS lookup succeeds but returns an empty list of addresses, or if an empty list is passed, the function currently returns successfully without validating any IP addresses.
To ensure robust SSRF protection (defense in depth), we should fail closed and throw an error if no valid IP addresses are returned, regardless of whether the input array was empty or not.
| if (addresses.length > 0 && ipAddressCount === 0) { | |
| throw new UrlError("DNS lookup did not return an IP address"); | |
| } | |
| if (ipAddressCount === 0) { | |
| throw new UrlError("DNS lookup did not return any IP address"); | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR improves @fedify/vocab-runtime’s SSRF-oriented URL validation to tolerate Node-compatible DNS implementations that include non-IP alias (e.g., CNAME target hostnames) entries in dns.lookup({ all: true }) results, while still validating every actual IP address and failing closed when only aliases are returned.
Changes:
- Refactors DNS result checking into
validateLookupAddresses()and uses it fromvalidatePublicUrl(). - Ignores non-IP
addressentries when at least one real IP is present; rejects alias-only results. - Adds unit tests covering mixed alias+IP results and alias-only / unsafe-IP scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/vocab-runtime/src/url.ts | Adds validateLookupAddresses() and switches validatePublicUrl() to validate only actual IP literals while failing closed on alias-only DNS results. |
| packages/vocab-runtime/src/url.test.ts | Adds tests asserting that alias entries are ignored when IPs are present and that alias-only / private-IP results are rejected. |
| if (addresses.length > 0 && ipAddressCount === 0) { | ||
| throw new UrlError("DNS lookup did not return an IP address"); | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1102bf5ba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * | ||
| * @internal | ||
| */ | ||
| export function validateLookupAddresses( |
There was a problem hiding this comment.
Because this commit fixes #956 in @fedify/vocab-runtime, it falls under the Bugfix process in /workspace/fedify/AGENTS.md, which requires updating CHANGES.md with the issue/PR reference and contributor name. I checked the current Version 2.0.23 unreleased section and it remains empty, so this user-facing fix will be omitted from release notes unless the changelog entry is added before merge.
Useful? React with 👍 / 👎.
dahlia
added
component/vocab
| /** | ||
| * Validates the IP addresses returned by `node:dns.lookup()`. | ||
| * | ||
| * Some Node.js-compatible runtimes include CNAME records in lookup results, |
There was a problem hiding this comment.
Could you make it explicit here that this is a workaround for a bug in Cloudflare Workers' node:dns implementation, rather than a general variation among Node-compatible runtimes? Workerd currently maps every record in the DNS-over-HTTPS Answer array—including CNAME records—to LookupAddress, even though Node.js specifies that address is an IPv4 or IPv6 literal.
Please link the upstream Workerd issue once it is filed, and explain why this remains fail-closed: non-IP entries may be ignored only when at least one actual IP address is present, and every returned IP must still be validated. That context will help future readers understand why the spec-violating input is tolerated and when the workaround can be revisited. It would also be helpful to rename the test to mention Cloudflare Workers so the regression's origin is visible there as well.
Clarify that non-IP lookup entries are tolerated only for Workerd issue #6886 while retaining fail-closed validation, and identify the regression source in test names. Assisted-by: Codex:gpt-5
Fixes #956
Ignore non-IP aliases emitted by Node-compatible DNS implementations when a lookup also returns actual IP addresses. Fail closed for alias-only results and continue checking every returned IP.
Assisted-by: Codex:gpt-5