Add WHOIS referral loop cycle detection (ENG-5457) - #137
Conversation
A referral chain that revisits a server it already queried would silently burn through the hop budget and report the symptom (referral_budget) instead of the cause. Add a visited-set to whoisQuery so cycles are detected in O(1) and reported as whoisIncompleteCycle (referral_cycle). The salvaged record from the last successful hop is still returned — de-rank, never drop. Also factor ctx.Err()-first classification into classifyIncomplete() so every exit from the referral loop applies the same precedence without duplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instruction-File Drift DetectionCode changes in this PR may have made documentation stale:
|
| Section | Issue | Evidence |
|---|---|---|
| "Incomplete lookups are observable, not silent:" (line 119) | New whoisIncompleteCycle reason not documented |
PR adds referral loop detection (whoisIncompleteCycle) as a 4th incomplete reason, but line 119 lists only 3 ways a chain can end prematurely: deadline expiry, transport failure, and hop budget exhaustion. Missing: referral cycle detection. |
| "Degraded predicate" (line 125) | Says "three incompleteness reasons" but code now has four | Documentation states "A pass counts as degraded when any of the five per-candidate counters is non-zero — the three incompleteness reasons plus `lookup_failed` and `panicked`" but code diff shows cycle detection added as a 4th incompleteness reason tracked alongside the three documented ones. |
Automated drift check — please review and update if needed.
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe WHOIS client now detects referral cycles with normalized server names and a bounded visited set. It classifies incomplete results with context cancellation and deadline precedence over referral, cycle, and hop-budget causes. Tests cover cycle detection, hostname normalization, bootstrap-seed cycles, and deadline classification. Reverse-WHOIS verification counts cycle outcomes and reports them in degraded-pass logs. ✨ 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: 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/whoisclient_test.go`:
- Around line 1119-1120: Update the cycle-detection assertion in the relevant
WHOIS referral test to require exactly 3 hops, covering the bootstrap, serverA,
and serverB requests, instead of only asserting hops is below the referral
budget.
🪄 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: ec2a3ee2-4eaa-438c-b86b-c9ec17a99cf1
📒 Files selected for processing (3)
pkg/plugins/domains/reverse_whois_verify.gopkg/plugins/domains/whoisclient.gopkg/plugins/domains/whoisclient_test.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
- pkg/plugins/domains/whoisclient.go:61:
normalizeWhoisServerdoes not canonicalize the same server identity thatwhoisDialAddruses. A referral loop can alternatewhois.exampleandwhois.example:43; both dial the same WHOIS endpoint because explicit ports are dropped, but the visited set treats them as different and reportsreferral_budgetinstead ofreferral_cycle. Consider normalizing by stripping scheme/slash/port/brackets consistently with the dial path.
Tests not run: the sandbox is read-only, and Go could not create a module/cache directory.
Reviewed by Codex (gpt-5.5)
![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: 770ef9c77e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // is used ONLY for membership testing, never logged — the raw referral string | ||
| // is unbounded attacker-chosen text and stays on the ENG-5405 denylist. | ||
| func normalizeWhoisServer(server string) string { | ||
| return strings.TrimSuffix(strings.ToLower(server), ".") |
There was a problem hiding this comment.
When a WHOIS referral carries an explicit port such as whois.iana.org:43, the visited key remains whois.iana.org:43, while whoisDialAddr strips that port and actually dials whois.iana.org on tcp/43. In a back-to-seed cycle the new visited-set check therefore misses the seed, so the lookup can re-query IANA and classify/report the chain via the hop budget (or even overwrite lastRaw with the bootstrap response) instead of taking the new referral_cycle path. Please canonicalize the visited key with the same host/port normalization used for dialing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude Review
Critical issues
None. Traced the cycle-detection loop against all three new cycle tests and the AC10 hop-budget cases: visited is seeded with the normalized defaultServer and each referral target is recorded before advancing, so A->B->A and cycle-back-to-seed are caught in O(1) before the hop budget is spent, and the no-lastRaw branch (cycle before any post-seed record) correctly errors rather than salvaging. The factored classifyIncomplete preserves the ctx.Err()-first precedence at every exit, including the reworked pendingRefer block (AC10). Recall is unchanged (salvage still returns a nil error). summarizeVerifyPass adds cycle to both the clean-pass zero-check and the degradation predicate consistently.
Security
No security concerns flagged. The untrusted, attacker-chosen referral string is used only for normalized membership testing and is never logged; only the bounded whoisIncompleteCycle enum value escapes. SSRF control and byte-cap are untouched.
Test coverage
Tests are present and comprehensive (cycle detection, trailing-dot normalization evasion, back-to-seed salvage, AC10 deadline-vs-budget discrimination, and classifyIncomplete both-exit agreement).
No critical issues -- LGTM pending human review.
…ertion - normalizeWhoisServer now strips scheme, trailing slash, and explicit port consistent with whoisDialAddr, so "whois.example:43" and "whois.example" compare equal in the visited set (Codex review) - Tighten cycle-detection test assertion to require exactly 3 hops instead of < maxWhoisReferrals (CodeRabbit review) - Add port, scheme, and combined normalization test cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instruction-File Drift DetectionCode changes in this PR may have made documentation stale:
|
| Section | Issue | Evidence |
|---|---|---|
| "Reverse-WHOIS Verify-After-Retrieve" (line 119) | New incompleteness reason not mentioned | Code adds whoisIncompleteCycle for cycle detection, but documentation only lists three causes: "deadline expiry mid-chain, a referral hop whose transport failed, or the hop budget exhausted" |
| "Reverse-WHOIS Verify-After-Retrieve" (line 125) | Documentation refers to "the three incompleteness reasons" | PR adds whoisIncompleteCycle as a fourth incompleteness type; code now has 4 incompleteness reasons but docs still say 3 |
Automated drift check — please review and update if needed.
1 participant
Summary
whoisQueryso a referral chain that revisits a server is detected in O(1) and classified aswhoisIncompleteCycle(referral_cycle) instead of silently burning through the hop budget and reportingreferral_budgetclassifyIncomplete()so every exit from the referral loop applies the same precedence without duplicationreferral_cycleinsummarizeVerifyPassdegradation predicate and slog outputWhat this deliberately does NOT change
confReverseWhoisUnverified(0.50) whether the lookup was truncated by a cycle or the domain genuinely has no registrant org. NoAddConfidencecalls are touched.nilerror — recall is unchanged.returnstatements remainreturn— nocontinue/breakintroduced in the loop body.ssrfSafeControlandmaxWhoisResponseBytesare untouched.Whois.Example.COM.from evading the visited set.Test plan
TestWhoisQuery_ReferralCycleDetected— A→B→A cycle is detected and short-circuits before hop budgetTestWhoisQuery_ReferralCycleNormalizedComparison— trailing-dot evasion blocked by normalizationTestWhoisQuery_ReferralCycleBackToSeedSalvages— cycle back to bootstrap seed still salvages the post-seed recordTestWhoisQuery_HopBudgetExhaustedWithExpiredCtxReportsDeadline— expired ctx reportsdeadline_expired, live ctx reportsreferral_budget(AC10)TestClassifyIncomplete_BothExitsAgreeUnderExpiredCtx— all non-ctx reasons map todeadline_expiredunder expired ctx, pass through under clean ctx (AC11)TestNormalizeWhoisServer— trailing dot, case, and combination normalizationTestWhoisQuery_MidHopFailureClassifiesOnCtxErrNotTheHopError(6 subtests) passes unchangedgo test ./...— all packages green🤖 Generated with Claude Code