From 22bd6b0b7f6dffa1c4d82fca2440affd46b8bd41 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:10:46 +0200 Subject: [PATCH] Align the UI's shared-IP view with the runtime blocking decision (#759) 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 --- .../eu/faircode/netguard/DatabaseHelper.java | 17 ++++-- .../eu/faircode/netguard/ServiceSinkhole.java | 4 +- .../DatabaseHelperDnsAttributionTest.java | 60 +++++++++++++++---- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java index 64546ae1..cfb1f140 100644 --- a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java +++ b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java @@ -1189,7 +1189,18 @@ public String getQName(int uid, String ip) { } } - public Cursor getQAName(int uid, String ip, boolean alive) { + /** + * DNS evidence for an IP, freshest qname first, one row per qname. + * + *

Expired rows are always excluded. Both callers — the runtime + * blocking decision in {@code blockKnownTracker()} and the UI + * classification in {@code ServiceSinkhole.log()} — must answer "is this + * IP shared, and whose is it?" from the same row set. When the UI alone + * saw the full history (see issue #759), it drew the shared-IP marker and + * the ALLOWED/BLOCKED text from evidence the blocker had already + * discarded, so the log contradicted what actually happened. + */ + public Cursor getQAName(int uid, String ip) { long now = new Date().getTime(); lock.readLock().lock(); try { @@ -1197,9 +1208,7 @@ public Cursor getQAName(int uid, String ip, boolean alive) { readableDb = this.getReadableDatabase(); SQLiteDatabase db = readableDb; String escapedIp = ip.replace("'", "''"); - String aliveFilter = alive - ? " AND (d.time IS NULL OR d.time + d.ttl >= " + now + ")" - : ""; + String aliveFilter = " AND (d.time IS NULL OR d.time + d.ttl >= " + now + ")"; // There is a segmented index on resource. A shared IP can carry // DNS evidence for several qnames; keep only the most recently // observed row per qname (dedup) and order qnames by recency, so diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index b6b7c723..2309cb5e 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -999,7 +999,7 @@ private void log(Packet packet, int connection, boolean interactive) { int uncertain = DatabaseHelper.ACCESS_UNCERTAIN_NONE; boolean isTracker = false; - try (Cursor lookup = dh.getQAName(packet.uid, packet.daddr, false)) { + try (Cursor lookup = dh.getQAName(packet.uid, packet.daddr)) { uncertain = (lookup != null && lookup.getCount() > 1) ? DatabaseHelper.ACCESS_UNCERTAIN_SHARED_IP : DatabaseHelper.ACCESS_UNCERTAIN_NONE; @@ -2697,7 +2697,7 @@ private boolean blockKnownTracker(String daddr, int uid) { boolean sawFirstRow = false; boolean sawTrackerEvidence = false; boolean sawNonTrackerEvidence = false; - try (Cursor lookup = dh.getQAName(uid, daddr, true)) { + try (Cursor lookup = dh.getQAName(uid, daddr)) { // Loop through all fresh DNS candidates for this IP and only fail closed // when ambiguous tracker blocking is enabled or the evidence is tracker-only. if (lookup != null) { diff --git a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java index a1561e89..9a01302f 100644 --- a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java +++ b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java @@ -20,6 +20,8 @@ @RunWith(RobolectricTestRunner.class) public class DatabaseHelperDnsAttributionTest { + private static final long NOW = System.currentTimeMillis(); + @Test public void mostRecentlyObservedQnameIsReturnedFirst() { DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); @@ -27,10 +29,10 @@ public void mostRecentlyObservedQnameIsReturnedFirst() { String ip = "203.0.113.10"; // Alphabetically "aaa..." would sort first, but "zzz..." was resolved later. - dh.insertDns(rr(1_000L, "aaa-old.example.com", "aaa-old.example.com", ip, 3600)); - dh.insertDns(rr(2_000L, "zzz-new.example.com", "zzz-new.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 2_000L, "aaa-old.example.com", "aaa-old.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "zzz-new.example.com", "zzz-new.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals(2, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("zzz-new.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); @@ -46,16 +48,16 @@ public void repeatedObservationsOfSameQnameCollapseToFreshestRow() { String ip = "203.0.113.20"; // Same qname, two distinct CNAME targets observed at different times. - dh.insertDns(rr(1_000L, "tracker.example.com", "old-cname.example.com", ip, 3600)); - dh.insertDns(rr(5_000L, "tracker.example.com", "new-cname.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 5_000L, "tracker.example.com", "old-cname.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "tracker.example.com", "new-cname.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals("duplicate rows for the same qname must collapse to one", 1, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("tracker.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); assertEquals("the freshest row's aname should win", "new-cname.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); - assertEquals(5_000L, c.getLong(c.getColumnIndexOrThrow("time"))); + assertEquals(NOW - 1_000L, c.getLong(c.getColumnIndexOrThrow("time"))); } } @@ -65,15 +67,15 @@ public void caseVariantQnamesShareOneRowAndDoNotLookUncertain() { dh.clearDns(); String ip = "203.0.113.25"; - dh.insertDns(rr(1_000L, "Graph.Facebook.Com", "Alias.Example.Com", ip, 3600)); - dh.insertDns(rr(5_000L, "graph.facebook.com", "alias.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 5_000L, "Graph.Facebook.Com", "Alias.Example.Com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "graph.facebook.com", "alias.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals("case variants must share one stored qname", 1, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("graph.facebook.com", c.getString(c.getColumnIndexOrThrow("qname"))); assertEquals("alias.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); - assertEquals(5_000L, c.getLong(c.getColumnIndexOrThrow("time"))); + assertEquals(NOW - 1_000L, c.getLong(c.getColumnIndexOrThrow("time"))); } } @@ -84,7 +86,7 @@ public void aliveFilterAppliesBeforeDedup() { String ip = "203.0.113.30"; // The freshest observation has already expired; an older one is still - // alive. With alive=true the expired row must not shadow the alive one. + // alive. The expired row must not shadow the alive one. // Rows are inserted directly because insertDns() clamps the TTL to the // "ttl" preference floor, which would keep the fresh row alive. long now = System.currentTimeMillis(); @@ -97,13 +99,45 @@ public void aliveFilterAppliesBeforeDedup() { + (now - 5_000) + ", 't2.example.com', 'expired-cname.example.com', '" + ip + "', 1000)"); - try (Cursor c = dh.getQAName(-1, ip, true)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals(1, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("alive-cname.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); } } + /** + * Issue #759: an expired qname must not make an IP look shared. The UI + * derived its shared-IP marker from a row set that still contained + * expired evidence while the blocker had already dropped it, so the log + * flagged an ambiguity the blocking decision never saw. + */ + @Test + public void expiredQnameDoesNotMakeIpLookShared() { + DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); + dh.clearDns(); + + String ip = "203.0.113.40"; + // Two different qnames on one IP, but only one is still alive. Rows are + // inserted directly because insertDns() clamps the TTL to the "ttl" + // preference floor, which would keep the expired one alive. + long now = System.currentTimeMillis(); + dh.getWritableDatabase().execSQL( + "INSERT INTO dns (time, qname, aname, resource, ttl) VALUES (" + + (now - 10_000) + ", 'alive.example.com', 'alive.example.com', '" + + ip + "', 3600000)"); + dh.getWritableDatabase().execSQL( + "INSERT INTO dns (time, qname, aname, resource, ttl) VALUES (" + + (now - 5_000) + ", 'expired.example.com', 'expired.example.com', '" + + ip + "', 1000)"); + + try (Cursor c = dh.getQAName(-1, ip)) { + assertEquals("an expired qname must not count towards shared-IP", 1, c.getCount()); + assertTrue(c.moveToFirst()); + assertEquals("alive.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); + } + } + private static ResourceRecord rr(long time, String qname, String aname, String resource, int ttl) { ResourceRecord rr = new ResourceRecord(); rr.Time = time;