[WIP] Verify website ownership for domain identities#897
Open
HaykK-Solicy wants to merge 4 commits into
Open
Conversation
Contributor
Review Summary by QodoAdd domain identity verification with website ownership proof
WalkthroughsDescription• Adds domain identity verification via website ownership proof • Implements HTTPS-based proof fetching with TLS certificate validation • Integrates domain proof parser into web2 identity verification flow • Adds new opcode and handler for domain proof retrieval Diagramflowchart LR
A["Domain Proof Request"] --> B["Validate URL Format"]
B --> C["Fetch via HTTPS"]
C --> D["Verify TLS Certificate"]
D --> E["Parse Proof Payload"]
E --> F["DomainProofParser"]
F --> G["Web2 Identity Flow"]
File Changes1. src/libs/abstraction/index.ts
|
Contributor
|
Warning Review limit reached
More reviews will be available in 24 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds domain-based Web2 proof support: SSRF-checked HTTPS fetching of proof URLs, a DomainProofParser, verifier integration validating URL scheme/path/hostname, a network handler endpoint, and an OmniProtocol opcode/registry entry. ChangesDomain Proof Verification
Sequence DiagramsequenceDiagram
participant Client
participant getDomainProof_Handler
participant fetchDomainProof
participant HTTPS_Server
participant DomainProofParser
Client->>getDomainProof_Handler: GET_DOMAIN_PROOF (url)
getDomainProof_Handler->>getDomainProof_Handler: validate URL structure
getDomainProof_Handler->>fetchDomainProof: fetch proof from URL
fetchDomainProof->>HTTPS_Server: HTTPS request (with TLS agent)
HTTPS_Server->>fetchDomainProof: proof body
fetchDomainProof->>getDomainProof_Handler: { hostname, body }
getDomainProof_Handler->>Client: HTTP 200 { success, hostname, body }
Note over Client,DomainProofParser: Later verification flow
Client->>verifyWeb2Proof: verify domain proof
verifyWeb2Proof->>DomainProofParser: readData(proofUrl)
DomainProofParser->>fetchDomainProof: fetch proof
fetchDomainProof->>DomainProofParser: { hostname, body }
DomainProofParser->>Client: { message, type, signature }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
Code Review by Qodo
1.
|
HaykK-Solicy
changed the title
Greptile SummaryThis PR adds node-side domain ownership verification: the node fetches a well-known proof file over HTTPS, validates its signature against the sender's wallet, and enforces that the proof URL's hostname matches the claimed domain. The implementation includes an SSRF guard with DNS pinning, a size cap, and redirect suppression.
Confidence Score: 3/5The domain ownership check can be satisfied from a non-standard HTTPS port, so a party who controls port 9999 (or any non-443 port) on a host could forge a proof for that domain. The core hostname match in verifyWeb2Proof uses URL.hostname, which is port-stripped. A proof at https://example.com:9999/.well-known/demos-cci.txt passes the hostname matches claimed domain check against example.com and then connects to port 9999. The same gap exists in fetchDomainProof and the getDomainProof handler. Without an explicit port guard, the ownership model is weaker than intended. src/libs/abstraction/index.ts (verifyWeb2Proof domain case), src/libs/abstraction/web2/domain.ts (fetchDomainProof missing port check), and src/libs/network/handlers/identityHandlers.ts (getDomainProof handler missing port check). Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Node
participant DNS
participant TargetDomain
Note over Client,Node: getDomainProof (unauthenticated helper)
Client->>Node: "GET_DOMAIN_PROOF {url}"
Node->>Node: validate protocol + path
Node->>DNS: resolveAndValidateHost(hostname)
DNS-->>Node: resolved IPs (all validated as public)
Node->>Node: pin socket to resolved[0]
Node->>TargetDomain: HTTPS GET /.well-known/demos-cci.txt
TargetDomain-->>Node: proof file body
Node-->>Client: "{hostname, body}"
Note over Client,Node: verifyWeb2Proof (domain context)
Client->>Node: "REGISTER_WEB2_IDENTITY {context:domain, username, proof:url}"
Node->>Node: check protocol, path, hostname vs username
Node->>Node: DomainProofParser.readData(proofUrl)
Node->>DNS: resolveAndValidateHost(hostname)
DNS-->>Node: resolved IPs
Node->>TargetDomain: HTTPS GET (pinned socket)
TargetDomain-->>Node: proof body
Node->>Node: parsePayload
Node->>Node: verify signature
Node-->>Client: "{success: true/false}"
Reviews (4): Last reviewed commit: "fixed error from fetchDomainProof" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/libs/abstraction/web2/domain.ts (1)
2-2: 💤 Low valuePrefer
node:protocol prefix for Node.js built-in modules.Using
node:httpsexplicitly identifies this as a Node.js built-in module, improving clarity and avoiding potential conflicts with npm packages of the same name.Suggested fix
-import https from "https" +import https from "node:https"🤖 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/libs/abstraction/web2/domain.ts` at line 2, Replace the built-in module import to use the Node.js protocol prefix: change the import of https (the line importing https in src/libs/abstraction/web2/domain.ts) to use "node:https" so the runtime and readers clearly see this is a core Node module and avoid conflicts with similarly named npm packages.
🤖 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/libs/abstraction/index.ts`:
- Around line 225-230: The hostname check using proofUrl.hostname vs
payload.username should be done case-insensitively; update the comparison (where
proofUrl.hostname and payload.username are used) to normalize both sides (e.g.,
toLowerCase()) before comparing so "Example.COM" and "example.com" match, and
keep the same return shape when the comparison fails.
In `@src/libs/abstraction/web2/domain.ts`:
- Around line 70-76: The log currently concatenates the caught error which can
become "[object Object]"; update the catch in the domain proof fetch block that
calls fetchDomainProof(proofUrl) to log the real error message/stack (e.g.,
log.error(`[DOMAIN] Failed to fetch proof for ${proofUrl}: ${error?.message ??
String(error)}`, { stack: error?.stack })) and when rethrowing either include
the original error as the cause or append the error message to the new Error so
the underlying error details are preserved; reference the fetchDomainProof call,
the catch block, log.error, and the proofUrl variable when making this change.
In `@src/libs/network/handlers/identityHandlers.ts`:
- Around line 192-199: The catch block in getDomainProof should stop
concatenating the error into the message; instead pass the error as a structured
argument to the logger so stack and metadata are preserved (e.g., call log.error
with a message and the error object or an object containing the error) and keep
setting response.result and response.response as before; update the catch in
identityHandlers.ts where log.error is called to use structured logging
(reference: getDomainProof and log.error).
- Around line 188-189: fetchDomainProof currently enforces HTTPS but allows SSRF
by resolving arbitrary hostnames; update fetchDomainProof (and callers like
getDomainProof) to perform DNS resolution (dns.promises.lookup with { all: true
}) and reject requests if any resolved address is
private/loopback/link-local/unique-local/metadata (check RFC1918 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, 169.254.169.254,
::1, fe80::/10, fc00::/7, plus carrier-grade 100.64.0.0/10) or if the hostname
is an IP literal in those ranges; optionally add an allowlist of explicit public
hostnames and short-circuit to allow only those. Implement a helper like
isPrivateOrLocalAddress(ip) to centralize range checks and throw/return an error
before creating the https.Agent or making the request. Ensure getDomainProof
remains authRequired: false but relies on the hardened fetchDomainProof to block
internal targets.
---
Nitpick comments:
In `@src/libs/abstraction/web2/domain.ts`:
- Line 2: Replace the built-in module import to use the Node.js protocol prefix:
change the import of https (the line importing https in
src/libs/abstraction/web2/domain.ts) to use "node:https" so the runtime and
readers clearly see this is a core Node module and avoid conflicts with
similarly named npm packages.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2a6a8d7-9d1e-4010-9e50-3cd8840eeae5
📒 Files selected for processing (6)
src/libs/abstraction/index.tssrc/libs/abstraction/web2/domain.tssrc/libs/abstraction/web2/parsers.tssrc/libs/network/handlers/identityHandlers.tssrc/libs/omniprotocol/protocol/opcodes.tssrc/libs/omniprotocol/protocol/registry.ts
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- add SSRF guard to fetchDomainProof: resolve all A/AAAA records and reject private/loopback/link-local/metadata/CGNAT targets before the request (link-local/metadata blocked in all envs; loopback/private dev-only) - getDomainProof: explicit https protocol check with a clear error - verifyWeb2Proof: case-insensitive proof-host vs claimed-domain comparison - domain proof fetch: preserve real error message/detail in logs + rethrow - getDomainProof: structured error logging instead of string concat - clarify the "domain" parser format only checks scheme (binding enforced upstream) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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/libs/abstraction/web2/domain.ts`:
- Around line 34-64: assertAllowedHostname currently resolves the hostname and
classifies addresses but fetchDomainProof calls axios.get with the original
hostname allowing DNS rebinding between check and connect; fix by resolving once
with dns.promises.lookup (as in assertAllowedHostname), validate each IP against
DEV_ALLOWED_RANGES, then force the HTTP(S) connection to that validated IP by
passing the IP as the request host (for IPv6 use bracketed form), set the
original hostname in the Host header and set the TLS SNI/servername on the HTTPS
agent (e.g., via new https.Agent({ servername: hostname, ... })) so TLS and Host
header preserve the hostname while the socket connects to the pinned IP, and
ensure axios is configured with maxRedirects:0 to avoid re-checks; update
fetchDomainProof to call the resolution+validation logic (reusing
assertAllowedHostname or extracting its logic) and to construct the custom
agent/headers before calling axios.get.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 871438b4-809b-4356-a075-9fbbbca8f5c9
📒 Files selected for processing (4)
src/libs/abstraction/index.tssrc/libs/abstraction/web2/domain.tssrc/libs/abstraction/web2/parsers.tssrc/libs/network/handlers/identityHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/libs/abstraction/web2/parsers.ts
- src/libs/network/handlers/identityHandlers.ts
- src/libs/abstraction/index.ts
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.
Adds the node side of website-ownership identity.
The node fetches the wallet-signed proof file from the claimed domain over HTTPS (TLS cert binds content to the hostname), verifies the signature against the sender, and enforces that the proof URL's host matches the claimed domain. Verified domains flow through the existing generic web2 GCR write path as web2.domain.
Summary by CodeRabbit