Attribute shared-IP DNS evidence by recency, not alphabetically - #690
Merged
Conversation
getQAName() previously deduped and ordered candidate qnames for a shared IP alphabetically, so blockKnownTracker() and the traffic-log attribution in ServiceSinkhole.log() always locked onto whichever qname sorted first lexically, regardless of when it was actually resolved. Order by most recent observation instead, so the freshest DNS evidence — the one most likely to correspond to the connection actually being made — wins.
kasnder
marked this pull request as ready for review
July 28, 2026 07:48
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
2 participants
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.
Summary
Addresses part of #655 (DNS-based tracker blocking ambiguity).
DatabaseHelper.getQAName()— used byServiceSinkhole.blockKnownTracker()andServiceSinkhole.log()to attribute a shared IP to a hostname/tracker — deduped and ordered candidate qnames alphabetically (GROUP BY d.qname/ORDER BY d.qname). This meant the attributed hostname for an ambiguous shared IP was always whichever qname happened to sort first lexically, with no relation to which DNS resolution actually caused the current connection.This PR changes the query to dedupe and order by recency instead: for each qname sharing a resource IP, only the most recently observed row is kept, and qnames are returned most-recent-first. The freshest DNS evidence is the one most likely tied to the connection actually being made right now, so this should make attribution (and therefore the derived tracker category / block decision) meaningfully more accurate without any schema change, migration, or new knob.
As covered in the issue discussion, this is intentionally the "low-risk partial improvement" half of #655 — it does not attempt genuine per-app DNS evidence (option 2 in the issue), since DNS queries are resolved through netd (uid 0) rather than the requesting app's uid, and netd's resolver cache means a same-network app can get a cache hit with no query at all — so uid-scoped DNS evidence would under-block rather than fix attribution. The uid-global limitation documented in AGENTS.md §4.4 stands.
Changes
DatabaseHelper.getQAName(): replaced the alphabeticalGROUP BY/ORDER BY d.qnamewith a recency-based dedup (freshest row per qname via a correlated subquery) andORDER BY d.time DESC. ThealiveTTL filter is preserved and applied consistently to both the outer query and the inner "pick freshest" subquery.DatabaseHelperDnsAttributionTestcovering: (1) a more-recently-resolved qname is returned ahead of an alphabetically-earlier one, and (2) repeated observations of the same qname (e.g. distinct CNAME targets) collapse to a single, freshest row.Test plan
sqlite3(recency ordering, dedup-to-freshest, and thealivefilter all behave as intended) since this sandbox has no Android SDK to run Robolectric../gradlew :app:testGithubDebugUnitTest --tests 'eu.faircode.netguard.DatabaseHelperDnsAttributionTest'(needs to run in CI/an environment with the Android SDK)Generated by Claude Code