feat: add AliDNS and Cloudflare DNS resolvers with corresponding tests#30
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughExtracted Google, Cloudflare, and Ali DNS-over-HTTPS resolvers into dedicated modules, used them as the default resolver list, tightened resolver selection (nullish coalescing), fixed a conditional assignment in record parsing, added resolver tests, and re-exported resolver utilities from the main entrypoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as getDocumentStoreRecords / queryDns
participant Resolver as CustomDnsResolver (google/cloudflare/ali)
participant DoH as External DoH Service
participant Parser as parseOpenAttestationRecord
Caller->>Resolver: request TXT for domain
Resolver->>DoH: HTTPS GET /resolve?name=<domain>&type=TXT (provider-specific)
DoH-->>Resolver: JSON DNS response
Resolver-->>Caller: IDNSQueryResponse
Caller->>Parser: parse DNS Answer records
Parser-->>Caller: structured record objects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/util/dns-resolvers/ali-dns-resolver.ts (1)
3-7: Apply the same URL encoding and error handling improvements.Same concerns as the Google resolver: the domain should be URL-encoded, and
res.okshould be checked before parsing JSON.♻️ Proposed fix
export const aliDnsResolver: CustomDnsResolver = async (domain) => { - const res = await fetch(`https://dns.alidns.com/resolve?name=${domain}&type=16`, { + const res = await fetch(`https://dns.alidns.com/resolve?name=${encodeURIComponent(domain)}&type=16`, { method: "GET", }); + if (!res.ok) { + throw new Error(`AliDNS request failed with status ${res.status}`); + } return res.json() as Promise<IDNSQueryResponse>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/dns-resolvers/ali-dns-resolver.ts` around lines 3 - 7, The aliDnsResolver function should URL-encode the domain and handle non-OK HTTP responses before parsing JSON; update aliDnsResolver to call encodeURIComponent(domain) when building the request URL, check res.ok after fetch and throw or return a rejected Error including response.status and statusText when not ok, then only call res.json() and cast to Promise<IDNSQueryResponse> on successful responses to avoid parsing error pages as JSON.src/util/dns-resolvers/google-dns-resolver.ts (1)
3-7: URL-encode the domain parameter to prevent malformed URLs.If
domaincontains special characters (e.g.,&,=,?), the URL could break or produce unexpected query parameters. UseencodeURIComponentfor the domain value.Additionally,
fetchdoes not throw on HTTP error responses (4xx/5xx). Without checkingres.ok, the code will attempt to parse JSON from an error response, potentially returning malformed data or throwing an unrelated JSON parse error rather than surfacing the actual HTTP failure.♻️ Proposed fix
export const googleDnsResolver: CustomDnsResolver = async (domain) => { - const res = await fetch(`https://dns.google/resolve?name=${domain}&type=TXT`, { + const res = await fetch(`https://dns.google/resolve?name=${encodeURIComponent(domain)}&type=TXT`, { method: "GET", }); + if (!res.ok) { + throw new Error(`Google DNS request failed with status ${res.status}`); + } return res.json() as Promise<IDNSQueryResponse>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/dns-resolvers/google-dns-resolver.ts` around lines 3 - 7, In googleDnsResolver, URL-encode the domain with encodeURIComponent(domain) when building the query string and check the fetch response before parsing: if (!res.ok) throw a descriptive error including res.status and await res.text(); only then return await res.json() as Promise<IDNSQueryResponse>; this prevents malformed URLs and surfaces HTTP errors instead of JSON parse failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/dns-resolvers/cloudflare-dns-resolver.ts`:
- Line 6: The fetch call in cloudflare-dns-resolver.ts is sending unnecessary
headers and not validating inputs/response: remove the extra headers so only
Accept: application/dns-json is sent, ensure the domain/query parameter is
URL-encoded before building the request URL (where the resolver constructs the
DoH URL), and check response.ok (or response.status) and handle non-2xx
responses (throw or return an error) before calling response.json() in the
resolver function that performs the Cloudflare DoH request.
In `@src/util/dns-resolvers/dns-resolvers.test.ts`:
- Around line 53-55: The test is asserting the wrong header key ("contenttype")
which mirrors the buggy implementation; update the assertion in
dns-resolvers.test.ts to check the correct header name returned by the fetch
request (use request.headers.get("content-type")). Locate the failing
expectation around the Cloudflare resolver mock (the lines referencing
request.headers.get) and replace the "contenttype" getter with "content-type" so
the test matches the actual header sent by the Cloudflare resolver (request).
---
Nitpick comments:
In `@src/util/dns-resolvers/ali-dns-resolver.ts`:
- Around line 3-7: The aliDnsResolver function should URL-encode the domain and
handle non-OK HTTP responses before parsing JSON; update aliDnsResolver to call
encodeURIComponent(domain) when building the request URL, check res.ok after
fetch and throw or return a rejected Error including response.status and
statusText when not ok, then only call res.json() and cast to
Promise<IDNSQueryResponse> on successful responses to avoid parsing error pages
as JSON.
In `@src/util/dns-resolvers/google-dns-resolver.ts`:
- Around line 3-7: In googleDnsResolver, URL-encode the domain with
encodeURIComponent(domain) when building the query string and check the fetch
response before parsing: if (!res.ok) throw a descriptive error including
res.status and await res.text(); only then return await res.json() as
Promise<IDNSQueryResponse>; this prevents malformed URLs and surfaces HTTP
errors instead of JSON parse failures.
🪄 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
Run ID: c58e0886-e115-4ca3-8179-70e2cf0686bc
📒 Files selected for processing (7)
src/index.test.tssrc/index.tssrc/util/dns-resolvers/ali-dns-resolver.tssrc/util/dns-resolvers/cloudflare-dns-resolver.tssrc/util/dns-resolvers/dns-resolvers.test.tssrc/util/dns-resolvers/google-dns-resolver.tssrc/util/dns-resolvers/index.ts
…hod specification
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/dns-resolvers/cloudflare-dns-resolver.ts`:
- Around line 6-11: Trim and validate the incoming domain before using it: in
the Cloudflare DNS resolver where `domain` is checked and `url.searchParams` are
set (the code around the `if (!domain)` check and the subsequent
`url.searchParams.set("name", domain)` / `url.searchParams.set("type", "TXT")`
lines), replace the raw `domain` usage with a trimmed/normalized value (e.g.,
`const normalized = domain?.trim()`), throw an error if `normalized` is
falsy/empty, and then use that `normalized` value when calling
`url.searchParams.set("name", normalized)` so whitespace-only inputs are
rejected and not sent to the DNS query.
🪄 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
Run ID: daa50e8f-ab42-4f42-a9f1-78089467e008
📒 Files selected for processing (4)
src/util/dns-resolvers/ali-dns-resolver.tssrc/util/dns-resolvers/cloudflare-dns-resolver.tssrc/util/dns-resolvers/dns-resolvers.test.tssrc/util/dns-resolvers/google-dns-resolver.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/util/dns-resolvers/google-dns-resolver.ts
- src/util/dns-resolvers/ali-dns-resolver.ts
- src/util/dns-resolvers/dns-resolvers.test.ts
| if (!domain) { | ||
| throw new Error("Domain is required"); | ||
| } | ||
|
|
||
| url.searchParams.set("name", domain); | ||
| url.searchParams.set("type", "TXT"); |
There was a problem hiding this comment.
Harden domain validation for whitespace-only input.
At Line 6, if (!domain) allows values like " ", which then get sent as a DNS query name at Line 10. Trim before validation and use the normalized value in searchParams.
🔧 Suggested patch
export const cloudflareDnsResolver: CustomDnsResolver = async (domain) => {
const url = new URL("https://cloudflare-dns.com/dns-query");
+ const normalizedDomain = domain?.trim();
- if (!domain) {
+ if (!normalizedDomain) {
throw new Error("Domain is required");
}
- url.searchParams.set("name", domain);
+ url.searchParams.set("name", normalizedDomain);
url.searchParams.set("type", "TXT");📝 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.
| if (!domain) { | |
| throw new Error("Domain is required"); | |
| } | |
| url.searchParams.set("name", domain); | |
| url.searchParams.set("type", "TXT"); | |
| export const cloudflareDnsResolver: CustomDnsResolver = async (domain) => { | |
| const url = new URL("https://cloudflare-dns.com/dns-query"); | |
| const normalizedDomain = domain?.trim(); | |
| if (!normalizedDomain) { | |
| throw new Error("Domain is required"); | |
| } | |
| url.searchParams.set("name", normalizedDomain); | |
| url.searchParams.set("type", "TXT"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/dns-resolvers/cloudflare-dns-resolver.ts` around lines 6 - 11, Trim
and validate the incoming domain before using it: in the Cloudflare DNS resolver
where `domain` is checked and `url.searchParams` are set (the code around the
`if (!domain)` check and the subsequent `url.searchParams.set("name", domain)` /
`url.searchParams.set("type", "TXT")` lines), replace the raw `domain` usage
with a trimmed/normalized value (e.g., `const normalized = domain?.trim()`),
throw an error if `normalized` is falsy/empty, and then use that `normalized`
value when calling `url.searchParams.set("name", normalized)` so whitespace-only
inputs are rejected and not sent to the DNS query.
| @@ -208,22 +216,7 @@ describe("queryDns", () => { | |||
| Comment: "Response from 205.251.199.177.", | |||
| }; | |||
There was a problem hiding this comment.
can help update to use donotuse.trustvc.io
There was a problem hiding this comment.
this will fail too, since OA removed the url.
can change to donotuse.trustvc.io
| await expect(queryDns("https://donotuse.openattestation.com", testDnsResolvers)).rejects.toMatchObject({ | ||
| code: DnsproveStatusCode.IDNS_QUERY_ERROR_GENERAL, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.test.ts (1)
15-21: RenamesampleDnsTextRecordWithDnssec.After Line 19 this fixture now represents
dnssec: false, so the name is backwards and makes the expectation harder to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` around lines 15 - 21, The fixture constant sampleDnsTextRecordWithDnssec is misnamed because it sets dnssec: false; rename this symbol to something accurate (e.g., sampleDnsTextRecordWithoutDnssec or sampleDnsTextRecordDnssecFalse) and update all references to it in the test file (look for occurrences of sampleDnsTextRecordWithDnssec) so the variable name matches the dnssec: false value and test expectations remain readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.test.ts`:
- Around line 23-24: The test in src/index.test.ts asserts full live DNS TXT
records returned by getDocumentStoreRecords("donotuse.trustvc.io"), which
couples tests to mutable public DNS; change the spec to use a controlled fixture
or MSW to mock DNS responses for deterministic unit tests, or at minimum replace
strict equality assertions
(expect(...).toStrictEqual([...sampleDnsTextRecordWithDnssec])) with partial
assertions using arrayContaining/expect.objectContaining to only assert
invariant fields for the TXT record(s); keep any full-live-DNS assertions in a
separate integration test file or guarded by an environment flag.
---
Nitpick comments:
In `@src/index.test.ts`:
- Around line 15-21: The fixture constant sampleDnsTextRecordWithDnssec is
misnamed because it sets dnssec: false; rename this symbol to something accurate
(e.g., sampleDnsTextRecordWithoutDnssec or sampleDnsTextRecordDnssecFalse) and
update all references to it in the test file (look for occurrences of
sampleDnsTextRecordWithDnssec) so the variable name matches the dnssec: false
value and test expectations remain readable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const records = await getDocumentStoreRecords("donotuse.trustvc.io"); | ||
| expect(records).toStrictEqual([sampleDnsTextRecordWithDnssec]); |
There was a problem hiding this comment.
These tests are still coupled to mutable public DNS data.
Switching to donotuse.trustvc.io and updating the Astron fixture fixes the current failures, but these cases still hit live zones and assert the full TXT record set. This PR already shows the failure mode: upstream DNS changed, so the test had to be edited. Please move these expectations behind fixtures/MSW, or at least assert only the invariant record(s) with arrayContaining and keep live-DNS checks as separate integration coverage.
Also applies to: 38-47, 340-365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.test.ts` around lines 23 - 24, The test in src/index.test.ts
asserts full live DNS TXT records returned by
getDocumentStoreRecords("donotuse.trustvc.io"), which couples tests to mutable
public DNS; change the spec to use a controlled fixture or MSW to mock DNS
responses for deterministic unit tests, or at minimum replace strict equality
assertions (expect(...).toStrictEqual([...sampleDnsTextRecordWithDnssec])) with
partial assertions using arrayContaining/expect.objectContaining to only assert
invariant fields for the TXT record(s); keep any full-live-DNS assertions in a
separate integration test file or guarded by an environment flag.
Opps! Linted! d2eb43c |
|
🎉 This PR is included in version 2.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
This pull request refactors DNS resolver logic by modularizing and expanding support for multiple public DNS-over-HTTPS providers. It introduces dedicated resolver functions for Google, Cloudflare, and AliDNS, updates the default resolver chain to include all three, and improves test coverage for these resolvers. Additionally, minor improvements were made to parsing and documentation.
DNS Resolver Refactoring and Expansion:
src/util/dns-resolversdirectory, each encapsulating the logic for querying their respective public DNS-over-HTTPS endpoints. [1] [2] [3]index.tsto use the new resolver functions, now supporting Google, Cloudflare, and AliDNS by default.src/util/dns-resolvers/index.tsbarrel file for easier imports. [1] [2]Testing Improvements:
Minor Improvements:
parseOpenAttestationRecordto better handle missing type tokens.||to??for better handling of falsy values. [1] [2]These changes make the DNS resolution logic more modular, robust, and easier to extend or test.
Reference
The implementation is based off the PR made in the Open-Attestation DNS Prove repo: https://github.com/Open-Attestation/dnsprove/pull/61/changes#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80
Summary by CodeRabbit
New Features
Bug Fixes
Tests