Consolidate WHOIS - #125
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a98d60678a
ℹ️ 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".
|
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:
WalkthroughThe change adds shared WHOIS models, RDAP and TCP-43 lookup, privacy detection, normalization, embedded data, and SSRF-safe dialing. The WHOIS plugin now emits structured results and normalized preseeds. The ViewDNS reverse-WHOIS plugin replaces the removed implementation. Whoxy supports organization, person, and email queries with aggregated findings. Shared string and domain-finding utilities are added, and discovery registration is updated. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (3)
pkg/whois/lookup.go (1)
63-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap both failures in the returned error.
The message drops
rdapErrandtcp43Err. Callers then cannot tell a referral failure from a parse failure from a transport failure.♻️ Proposed fix
if rdapErr != nil && tcp43Err != nil { - return Result{}, fmt.Errorf("whois: all methods failed for %s", domain) + return Result{}, fmt.Errorf("whois: all methods failed for %s: rdap: %w; tcp43: %w", + domain, rdapErr, tcp43Err) }🤖 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 `@pkg/whois/lookup.go` around lines 63 - 65, Update the error return in the lookup method’s `rdapErr` and `tcp43Err` failure branch to wrap and preserve both underlying errors, while retaining the domain context in the message.pkg/lib/netutil/ssrf.go (1)
30-48: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd table tests for the denylist.
This guard is the security boundary for untrusted WHOIS referrals. No test file accompanies it in this change. A small table test over representative addresses (
127.0.0.1,10.0.0.1,169.254.169.254,::1,fd00::1,::ffff:127.0.0.1,8.8.8.8,2606:4700::1111) locks the behavior in place.🤖 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 `@pkg/lib/netutil/ssrf.go` around lines 30 - 48, Add a focused table-driven test for the denylist represented by disallowedPrefixes, covering the listed IPv4, IPv6, IPv4-mapped IPv6, denied, and allowed addresses. Assert each address is rejected or accepted according to its expected classification, including the mapped loopback case.pkg/plugins/domains/whois.go (1)
130-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCross-role duplicates reach the output.
The dedupe key includes
role, so one organization listed as registrant, admin, tech, and billing produces four identical preseed findings with the sameValueand the same score. Downstream consumers must then deduplicate.If the role only matters for the justification text, key
seenon{field, value}and record the first role that observed it.♻️ Proposed fix
- seen := map[candidate]bool{} + type key struct{ field, value string } + seen := map[key]bool{} @@ for _, cd := range candidates { - if cd.value == "" || seen[cd] || whois.IsPrivacy(cd.value) { + k := key{field: cd.field, value: cd.value} + if cd.value == "" || seen[k] || whois.IsPrivacy(cd.value) { continue } if cd.field == "email" && !whois.IsEmail(cd.value) { continue } - seen[cd] = true + seen[k] = true🤖 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 `@pkg/plugins/domains/whois.go` around lines 130 - 154, The deduplication in the contact-processing flow uses role as part of the key, allowing identical values across roles into the output. Update the `seen` key and related tracking in the function containing `AllContacts()` to deduplicate by `{field, value}` while retaining the first observed role for justification text; preserve the existing filtering and finding-generation behavior.
🤖 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 `@pkg/lib/strutil/strutil.go`:
- Around line 27-41: Update TokenSimilarity to convert both token lists into
unique token sets before choosing the shorter denominator and counting matches,
so repeated terms do not affect the score. Preserve the existing
set-intersection ratio using the unique-token count, and add an exact-value test
covering repeated tokens.
In `@pkg/plugins/domains/viewdns_reverse_whois.go`:
- Around line 79-84: Update the reverse-WHOIS processing in
pkg/plugins/domains/viewdns_reverse_whois.go lines 79-84 and
pkg/plugins/domains/whoxy_reverse_whois.go lines 84-94 so each candidate is
verified with RDAP first and WHOIS fallback before invoking domainFindings.
Preserve mismatches and unresolved candidates with individual needs-review
confidence scores in [0.35, 0.65), limiting verification to 500 candidates, six
workers, 10-second per-lookup timeouts, and a 90-second pass-wide budget.
In `@pkg/plugins/domains/whois.go`:
- Around line 96-114: The buildWhoisResultFinding flow must not expose
unredacted WHOIS data through the Raw field in FindingWhoisResult. Update the
whoisResultData construction to omit r.Raw by default, or only include a safely
redacted version when an explicit opt-in is enabled, while preserving the
normalized contact fields and existing result behavior.
- Around line 19-21: Refactor WhoisPlugin and the pkg/whois lookup flow to use a
configured pkg/client.Client for both RDAP requests, rather than leaving
HTTPClient nil or sharing only its transport. Ensure both RDAP calls pass the
provided ctx, so the shared client policy—including retries, response limits,
and User-Agent—is applied consistently.
In `@pkg/plugins/domains/whoxy_reverse_whois.go`:
- Around line 85-90: Update the query-error handling in Run around paginateQuery
so it checks ctx.Err() when an error occurs and returns that cancellation error
if non-nil; retain the existing warning-and-continue behavior for
non-cancellation query failures.
- Around line 85-90: Update the query loop in
pkg/plugins/domains/whoxy_reverse_whois.go:85-90 to return ctx.Err() when
paginateQuery fails due to cancellation, while logging and continuing only for
non-cancellation errors. In the pagination loop at
pkg/plugins/domains/whoxy_reverse_whois.go:117-124, check ctx.Err() before
returning partial results and propagate it when non-nil so Run respects caller
cancellation.
In `@pkg/plugins/plugin.go`:
- Line 30: Update the timestamp documentation near the purchased, updated, and
expiration fields to state that values are RFC3339 when available and otherwise
may use the registry-native format, matching the producer behavior in the Whois
result flow.
In `@pkg/whois/data/prefixes.txt`:
- Line 9: Clean up the WHOIS prefix data by removing duplicate entries for “NOT
DISCLOSED!”, “NetNames Hostmaster”, and “Domain Administrator”, keeping one
canonical occurrence of each. Repair the truncated BRAZIL entry at line 9 to the
correct registrant value if it can be determined from the source; otherwise
remove the invalid entry.
- Around line 2-5: Remove corporate, company-name, and generic business entries
from prefixes.txt, retaining only identifiers that clearly indicate proxy or
privacy services. Ensure matchesPrefix no longer classifies values such as
AbbVie Inc., Microsoft Corporation, Protected Wealth Management LLC, or Hidden
Valley Holdings LLC as privacy values, while preserving matching for legitimate
proxy-service identifiers.
In `@pkg/whois/data/privacy_orgs.txt`:
- Around line 24-25: Remove the broad registrar identities from
pkg/whois/data/privacy_orgs.txt at lines 24-25, or narrow them to unambiguous
proxy-service wording so IsPrivacy does not redact legitimate registrant data.
Also update the broad corporate email domains in pkg/whois/data/suffixes.txt at
lines 14-19 by removing them or replacing them with proxy-only mailbox patterns.
In `@pkg/whois/normalize.go`:
- Around line 137-146: Update IsPlausibleDomain to split the domain into DNS
labels and validate each one before returning true: reject empty labels, labels
exceeding 63 bytes, characters outside letters, digits, and hyphens, and labels
beginning or ending with a hyphen. Preserve the existing overall domain checks
and reject invalid reverse-WHOIS values such as consecutive dots or hyphen-edged
labels.
- Around line 57-60: Update IsEmail to capture the parsed mail.Address, require
Address.Name to be empty, and compare Address.Address against
strings.TrimSpace(s) so display-name inputs are rejected. Ensure classifyEmail
uses the normalized Address.Address value if display names are intentionally
supported there.
In `@pkg/whois/rdap.go`:
- Around line 84-103: The enrichFromRegistrar referral flow must validate
parsed.Href as http or https before dialing, then use an HTTP client/transport
whose dialer applies netutil.SSRFSafeControl. Reuse the existing WHOIS
response-size limit maxWhoisResponseBytes (1 MiB) when fetching the referral,
while preserving the current early returns for invalid URLs and request
failures.
- Around line 149-168: Update extractAddressFromVCard to read locality from adr
value index 3 alongside region at index 4, and return both country and
city/province data. Update extractContact to assign the extracted locality to
Contact.City, preserving existing country and region handling so mergeContact
receives the RDAP city.
- Around line 15-30: The RDAP flow drops the caller context and lacks an
explicit deadline. Update Lookup in pkg/whois/lookup.go:47 to pass ctx into
rdapLookup, then update rdapLookup in pkg/whois/rdap.go:15-30 to derive a
timeout context with context.WithTimeout and apply it via WithContext to both
the primary RDAP request and registrar follow-up request.
In `@pkg/whois/tcp43.go`:
- Line 137: Update the deferred connection cleanup in the relevant TCP43 lookup
function to explicitly handle or intentionally discard the error returned by
conn.Close, eliminating the golangci-lint errcheck warning while preserving
deferred cleanup.
- Around line 32-34: Update tcp43Lookup and tcp43Raw to distinguish caller
cancellation from budget expiry: preserve the salvaged lastRaw result when the
budget expires, but propagate an error when the caller’s context is canceled.
Remove or narrow the post-tcp43Raw ctx.Err check so it does not discard
successful partial results, while ensuring tcp43Raw’s salvage path cannot
suppress caller cancellation.
---
Nitpick comments:
In `@pkg/lib/netutil/ssrf.go`:
- Around line 30-48: Add a focused table-driven test for the denylist
represented by disallowedPrefixes, covering the listed IPv4, IPv6, IPv4-mapped
IPv6, denied, and allowed addresses. Assert each address is rejected or accepted
according to its expected classification, including the mapped loopback case.
In `@pkg/plugins/domains/whois.go`:
- Around line 130-154: The deduplication in the contact-processing flow uses
role as part of the key, allowing identical values across roles into the output.
Update the `seen` key and related tracking in the function containing
`AllContacts()` to deduplicate by `{field, value}` while retaining the first
observed role for justification text; preserve the existing filtering and
finding-generation behavior.
In `@pkg/whois/lookup.go`:
- Around line 63-65: Update the error return in the lookup method’s `rdapErr`
and `tcp43Err` failure branch to wrap and preserve both underlying errors, while
retaining the domain context in the message.
🪄 Autofix
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 Plus
Run ID: 821aadef-0217-49d4-9d13-a0217be7d3a1
📒 Files selected for processing (34)
pkg/lib/discovery.gopkg/lib/netutil/ssrf.gopkg/lib/strutil/strutil.gopkg/plugins/domains/domain_helpers.gopkg/plugins/domains/github_org.gopkg/plugins/domains/github_org_test.gopkg/plugins/domains/reverse_whois.gopkg/plugins/domains/reverse_whois_test.gopkg/plugins/domains/reverse_whois_verify.gopkg/plugins/domains/reverse_whois_verify_test.gopkg/plugins/domains/viewdns_reverse_whois.gopkg/plugins/domains/viewdns_reverse_whois_test.gopkg/plugins/domains/whois.gopkg/plugins/domains/whois_internal_test.gopkg/plugins/domains/whoisclient.gopkg/plugins/domains/whoisclient_test.gopkg/plugins/domains/whoxy_reverse_whois.gopkg/plugins/domains/whoxy_reverse_whois_test.gopkg/plugins/plugin.gopkg/whois/data/embed.gopkg/whois/data/legal_suffixes.txtpkg/whois/data/prefixes.txtpkg/whois/data/privacy_names.txtpkg/whois/data/privacy_orgs.txtpkg/whois/data/redaction_markers.txtpkg/whois/data/registry_artifacts.txtpkg/whois/data/suffixes.txtpkg/whois/lookup.gopkg/whois/normalize.gopkg/whois/privacy.gopkg/whois/rdap.gopkg/whois/result.gopkg/whois/tcp43.gowhois-test-harness
💤 Files with no reviewable changes (6)
- pkg/plugins/domains/whoisclient.go
- pkg/plugins/domains/reverse_whois_test.go
- pkg/plugins/domains/reverse_whois_verify.go
- pkg/plugins/domains/whois_internal_test.go
- pkg/plugins/domains/reverse_whois.go
- pkg/plugins/domains/whoisclient_test.go
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 `@pkg/whois/normalize.go`:
- Around line 115-118: Update the hostname normalization flow after
strings.TrimSpace in the relevant normalization function to remove exactly one
trailing "." before the IP check and downstream EffectiveTLDPlusOne/RootDomain
processing, while preserving other hostname content. Add a regression test
covering a valid fully qualified hostname such as app.praetorian.com. and verify
it returns the expected root domain.
🪄 Autofix
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 Plus
Run ID: 903f26b1-97d6-4f0a-b7b1-32ed86adb279
📒 Files selected for processing (1)
pkg/whois/normalize.go
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 `@pkg/plugins/domains/whois.go`:
- Around line 149-150: Update the email handling in the preseed creation flow
around whois.IsEmail to parse valid mailbox values with mail.ParseAddress,
replace cd.value with the parsed addr.Address, and perform deduplication using
the normalized address. Preserve skipping invalid email values and leave
non-email fields unchanged.
🪄 Autofix
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 Plus
Run ID: ca20f4bd-a4e0-47e7-8f90-f1a87dcaab57
📒 Files selected for processing (2)
pkg/lib/strutil/strutil.gopkg/plugins/domains/whois.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/plugins/domains/whoxy_reverse_whois.go (1)
97-97: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVerify each reverse-WHOIS candidate before emitting findings.
Runsends all provider results directly todomainFindings. The changed flow has no RDAP or WHOIS verification. This can emit unrelated domains for organization, person, or email queries.Verify each candidate with RDAP first and WHOIS fallback. Keep mismatches and unresolved candidates with per-candidate needs-review confidence. Apply the required candidate cap, worker limit, lookup timeout, and pass-wide budget.
As per coding guidelines, “Reverse-WHOIS candidates must be verified against the queried organization using RDAP first and WHOIS fallback, with per-candidate confidence decisions rather than a flat score.”
🤖 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 `@pkg/plugins/domains/whoxy_reverse_whois.go` at line 97, Update Run and the reverse-WHOIS candidate processing around domainFindings to verify every candidate against the queried organization, using RDAP first and WHOIS as fallback. Assign per-candidate confidence, marking mismatches and unresolved lookups as needs-review instead of emitting a flat score; enforce the required candidate cap, worker limit, per-lookup timeout, and pass-wide budget before returning findings.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@pkg/plugins/domains/whoxy_reverse_whois.go`:
- Line 97: Update Run and the reverse-WHOIS candidate processing around
domainFindings to verify every candidate against the queried organization, using
RDAP first and WHOIS as fallback. Assign per-candidate confidence, marking
mismatches and unresolved lookups as needs-review instead of emitting a flat
score; enforce the required candidate cap, worker limit, per-lookup timeout, and
pass-wide budget before returning findings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 579ce156-4f15-49cd-a537-a5f78c643a6c
📒 Files selected for processing (6)
pkg/plugins/domains/whoxy_reverse_whois.gopkg/whois/lookup.gopkg/whois/normalize_test.gopkg/whois/privacy.gopkg/whois/privacy_test.gopkg/whois/rdap.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/whois/privacy.go
- pkg/whois/rdap.go
- pkg/whois/lookup.go
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 `@pkg/whois/lookup.go`:
- Around line 67-68: Preserve privacy-detection state in the whois.Result flow
before Lookup calls ScrubContacts, so pkg/plugins/domains/whois.go can still
emit PrivacyRedaction for privacy registrants and proxy emails. Update the
relevant Result fields or derive and retain the finding before scrubbing, while
ensuring scrubbed output does not restore provider placeholder text.
🪄 Autofix
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 Plus
Run ID: 2669e624-a118-479e-b487-867615ca49f8
📒 Files selected for processing (6)
pkg/plugins/domains/whois.gopkg/whois/lookup.gopkg/whois/normalize.gopkg/whois/rdap.gopkg/whois/result.gopkg/whois/tcp43.go
💤 Files with no reviewable changes (1)
- pkg/whois/rdap.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/whois/tcp43.go
- pkg/whois/normalize.go
| applyISOCILFallback(&result, tcp43Raw) | ||
| result.ScrubContacts() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Retain privacy state when scrubbing contacts.
Lookup calls ScrubContacts before pkg/plugins/domains/whois.go receives the result. Scrub converts known privacy values to empty strings. The plugin then cannot detect a privacy registrant or proxy email, so it emits blank fields instead of PrivacyRedaction.
Carry scrub-safe privacy state in whois.Result, or derive the finding before scrubbing raw values. Do not restore provider placeholder text.
🤖 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 `@pkg/whois/lookup.go` around lines 67 - 68, Preserve privacy-detection state
in the whois.Result flow before Lookup calls ScrubContacts, so
pkg/plugins/domains/whois.go can still emit PrivacyRedaction for privacy
registrants and proxy emails. Update the relevant Result fields or derive and
retain the finding before scrubbing, while ensuring scrubbed output does not
restore provider placeholder text.
No description provided.