Align the UI's shared-IP view with the runtime blocking decision (#759) - #776
Merged
Conversation
ServiceSinkhole.log() classified connections from getQAName(..., false), the full DNS history, while blockKnownTracker() used getQAName(..., true), alive rows only. The two sides answered "is this IP shared, and whose is it?" from different row sets, so the shared-IP marker and the ALLOWED/BLOCKED text could contradict what the blocker actually did. Align the UI to the runtime rather than the other way round: the log then describes the decision that was actually taken, and blocking strength is unchanged. Widening the runtime to full history would instead have treated more IPs as shared, letting more tracker traffic through -- the wrong direction for a tracker blocker. With both callers passing the same value, the parameter is dropped so the divergence cannot be reintroduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #759. Implements option 1 from the triage: the runtime stays alive-only and the UI aligns to it.
ServiceSinkhole.log()calledgetQAName(uid, daddr, false)— full DNS history — whileblockKnownTracker()called it withtrue, which applies the alive-row filter. Both are asking the same question ("is this IP shared, and whose is it?") and were answering it from different row sets, so the*shared-IP marker and the ALLOWED/BLOCKED text could disagree with what the blocker actually did.Why this direction
Aligning the UI to the runtime keeps the log truthful about the decision that was actually taken, and leaves blocking strength untouched. The alternative — widening the runtime to full history — would treat more IPs as shared and therefore let more tracker traffic through to avoid collateral breakage. That is the wrong direction for a tracker blocker, and it would have been a silent weakening rather than a fix.
It also removes an inconsistency inside the UI itself:
getAccessDns(), which backs the access list, is already unconditionally alive-filtered.log()was the outlier.The parameter is gone
With both call sites passing the same value,
boolean aliveis dead configurability of exactly the kind that produced this bug, so it is dropped and the filter is unconditional. That makes the divergence impossible to reintroduce.Behaviour change worth naming
When every DNS row for an IP has expired,
log()now records no dname instead of a stale one, so that entry shows the raw IP. This is the intended consequence — the blocker saw nothing either — and the window is narrow:insertDnsfloors TTLs at thettlpreference (3 days) andcleanupDns()deletes expired rows every 12h, so "expired but not yet cleaned" is at most a 12h tail on a ≥3-day record, and any IP still being contacted has been re-resolved.Deliberately out of scope
Two nearby lookups still read full history. Both are per-qname questions rather than shared-IP judgements, so they are left alone rather than folded in silently:
getDecloakedTracker(qname, dh)→getAName(qname, false), reached only from the opt-in SNI research branch.usage()→getQName(uid, daddr), reached only when the non-defaulttrack_usageis on.Verification
expiredQnameDoesNotMakeIpLookShared: an IP with one alive and one expired qname must yield one row, so it cannot be marked shared.""and the tests unchanged, exactly two fail: the new test withexpected:<1> but was:<2>(the spurious shared-IP marker itself) andaliveFilterAppliesBeforeDedup. The three pure dedup/ordering tests pass either way, as they should.The three pre-existing dedup tests moved from 1970-epoch timestamps to now-relative ones, since the filter is no longer optional.
🤖 Generated with Claude Code