fix(webapp): three readers on one network are three share opens (BEA-151) (#185)

The share actor key was token+"/"+IP, so every browser behind one NAT was the same reader and the 10-minute visit debounce folded a whole office into a single open — three personas each measured "1 open" for three readers, and the panel's own copy promised the opposite. The key gains a truncated hash of the User-Agent. ShareOpens already sums across actor buckets and takes the max Last, so opens: 3 and an advancing last_opened fall out with no aggregator change, no new field, and no change to readDebounce. The UA is hashed because Record persists the actor through ReadRepo into storage; token+"/"+IP stays the prefix so the existing leak assertions keep covering the wider key. The copy now states the rule the code implements, including its residual: two people on one network in the same browser still count as one. Deviation from the plan, deliberate: TestSec_Share_VisitorCannotInflateOrRedirectTheLedger pinned "a visitor cannot split its own visits by varying the User-Agent". That is now intended behavior, so the two UA rows move out of the must-collapse set into an explicit assertion that they count separately. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-25 08:08:08 +02:00 · 2026-08-19 12:32:53 -07:00
parent eef37fa89b
commit ae14d11ac3
8 changed files with 138 additions and 23 deletions
@@ -51,13 +51,16 @@ export function shareDetail(s: ShareInfo, showProject: boolean): string {
  return bits.join(" · ");
 }

-// The two honesties about the number, worded once per section rather than
-// once per row: opens are debounced VISITS (readDebounce, reads.go), and the
-// count is per FILE not per link (heat is keyed by path), so two tokens on
-// one file report the same number.
+// The honesties about the number, worded once per section rather than once
+// per row: opens are debounced VISITS (readDebounce, reads.go) keyed by
+// browser+network rather than by person (shareActor, shares.go) — so the note
+// states the residual instead of promising precision the signal lacks — and
+// the count is per FILE not per link (heat is keyed by path), so two tokens
+// on one file report the same number.
 export const OPENS_NOTE =
  "Opens count how many times a file has been read through a public link. " +
-  "Repeat opens by the same reader within 10 minutes count once.";
+  "Repeat opens from the same browser and network within 10 minutes count once — " +
+  "two people on one network using the same browser still count as one.";

 export function SharesTable({
  shares,
@@ -460,9 +460,9 @@ type ShareOpen struct {
 // HeatEntry.LastRead is cross-kind, so a member viewing the file in the hub
 // would otherwise move the "opened through the link" date.
 //
-// Counts, never identities — the share actor is token+"/"+IP, a public
-// credential joined to an IP, and it must not leave the ledger. There is
-// deliberately no distinct-openers field.
+// Counts, never identities — the share actor is token+"/"+IP+"/"+UA hash, a
+// public credential joined to a network and a browser, and it must not leave
+// the ledger. There is deliberately no distinct-openers field.
 //
 // One byKey scan per project, never one per share: callers build this map
 // once and index it, because byKey is the full map and a project with 40
@@ -357,8 +357,12 @@ func TestSec_Share_PublicHitRecordsShareKindEndToEnd(t *testing.T) {
 	if b.Project != p.ID || b.Path != "hr/eng/payroll.md" {
 		t.Errorf("share visit recorded (%s, %s), want (%s, hr/eng/payroll.md)", b.Project, b.Path, p.ID)
 	}
-	if want := token + "/203.0.113.7"; b.Actor != want {
-		t.Errorf("share actor = %q, want %q", b.Actor, want)
+	// token/ip/uahash (BEA-151): the browser component is what stops a whole
+	// office behind one NAT from reading as one person. Asserted as a PREFIX
+	// so the identity half — link plus network, never a name — stays pinned
+	// without pinning the hash of an empty User-Agent.
+	if want := token + "/203.0.113.7/"; !strings.HasPrefix(b.Actor, want) {
+		t.Errorf("share actor = %q, want prefix %q", b.Actor, want)
 	}

 	// The member view: a share hit is share traffic, never a human reader.
@@ -450,6 +454,14 @@ func TestSec_Ledger_ReplicationAndHistoryViewsAreNeverReads(t *testing.T) {
 //     visitor controls — the query string, the method, the headers, the
 //     casing of the token, X-Forwarded-For?
 //   - can a visitor get an identity of its choosing recorded as an actor?
+//
+// User-Agent is the ONE exception, and it is deliberate (BEA-151): the actor
+// key includes a hash of it, because without it three people in one office
+// were one reader and the share panel reported "1 open" for all of them. So a
+// visitor CAN split its own visits by rotating UAs — bounded by the per-IP
+// limiter above the handler (ratelimit.go) and by retention folding, and it
+// inflates only the count on a link the visitor already holds. Asserted below
+// as intended behavior rather than left to look like a hole.
 func TestSec_Share_VisitorCannotInflateOrRedirectTheLedger(t *testing.T) {
 	h, srv, c, p := permHub(t)
 	secledReads(t, srv)
@@ -472,8 +484,6 @@ func TestSec_Share_VisitorCannotInflateOrRedirectTheLedger(t *testing.T) {
 		{"/s/" + token + "?cachebust=2", nil},
 		{"/s/" + token + "?", nil},
 		{"/s/" + token + "?download=1&x=" + strings.Repeat("y", 200), nil},
-		{"/s/" + token, map[string]string{"User-Agent": "one"}},
-		{"/s/" + token, map[string]string{"User-Agent": "two"}},
 		{"/s/" + token, map[string]string{"X-Forwarded-For": "10.1.1.1"}},
 		{"/s/" + token, map[string]string{"X-Forwarded-For": "10.1.1.2, 10.1.1.3"}},
 		{"/s/" + token, map[string]string{"X-Real-IP": "10.2.2.2"}},
@@ -504,6 +514,18 @@ func TestSec_Share_VisitorCannotInflateOrRedirectTheLedger(t *testing.T) {
 		t.Errorf("one visitor at one address produced %d ledger buckets — the 10-minute "+
 			"visit debounce is defeated by something the visitor chooses: %+v", len(got), got)
 	}
+
+	// The documented exception: distinct browsers are distinct readers, so two
+	// UAs are two buckets. That is the fix, not a defeat of the debounce — and
+	// the actor still says only "this link, this network, some browser".
+	for _, ua := range []string{"one", "two"} {
+		secledGet(h, "/s/"+token, addr, map[string]string{"User-Agent": ua})
+	}
+	got = secledBuckets(srv.Reads)
+	if len(got) != 3 {
+		t.Errorf("two more browsers on one network produced %d buckets in total, want 3 — "+
+			"distinct browsers must count separately (BEA-151): %+v", len(got), got)
+	}
 	for _, b := range got {
 		if b.Project != p.ID {
 			t.Errorf("a share visitor wrote a bucket for project %s, but the share is on %s: %+v",
@@ -512,8 +534,8 @@ func TestSec_Share_VisitorCannotInflateOrRedirectTheLedger(t *testing.T) {
 		if b.Kind != ReadKindShare {
 			t.Errorf("an anonymous share visitor recorded a %s-kind bucket: %+v", b.Kind, b)
 		}
-		if !strings.HasPrefix(b.Actor, token+"/") {
-			t.Errorf("share actor %q is not token/ip — the visitor chose part of it: %+v", b.Actor, b)
+		if !strings.HasPrefix(b.Actor, token+"/203.0.113.7/") {
+			t.Errorf("share actor %q is not token/ip/browser — the visitor chose the identifying part of it: %+v", b.Actor, b)
 		}
 		for _, planted := range []string{"alice@x.io", "dev-alice", "pwned", "10.1.1.1", "10.1.1.2", "10.2.2.2"} {
 			if strings.Contains(b.Actor, planted) {
@@ -1,6 +1,8 @@
 package webapp

 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"html"
@@ -500,6 +502,21 @@ func (s *Server) shareCreatorStillBelongs(sh Share) bool {

 // handleShared serves a share link: public, sandboxed, always the latest
 // synced content.
+// shareActor identifies one reader of a link well enough to debounce their
+// own reloads without folding a whole office into a single visit: token+IP
+// alone made every browser behind one NAT the same reader (BEA-151).
+//
+// The User-Agent is HASHED, never stored raw — Record persists the actor into
+// the read buckets and out through ReadRepo, and a raw User-Agent there is a
+// fingerprint sitting in storage indefinitely. Truncated because this only
+// ever needs to GROUP, never to identify, and it must not leave the ledger
+// either way. token+"/"+IP stays the prefix so the existing leak assertions
+// keep covering the wider key.
+func (s *Server) shareActor(r *http.Request, token string) string {
+	sum := sha256.Sum256([]byte(r.UserAgent()))
+	return token + "/" + s.clientIP(r) + "/" + hex.EncodeToString(sum[:8])
+}
+
 func (s *Server) handleShared(w http.ResponseWriter, r *http.Request) {
 	// Sandbox everything under /s/ before anything can answer the request:
 	// shared content executes in an opaque origin (scripts allowed — charts in
@@ -543,9 +560,9 @@ func (s *Server) handleShared(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	fi := snap.files[sp]
-	// A share hit is external consumption. Actor is token+IP: one audience
-	// member reloading is debounced to a visit, distinct visitors still count.
-	s.Reads.Record(sh.Project, sp, ReadKindShare, sh.Token+"/"+s.clientIP(r))
+	// A share hit is external consumption: one audience member reloading is
+	// debounced to a visit, distinct visitors still count.
+	s.Reads.Record(sh.Project, sp, ReadKindShare, s.shareActor(r, sh.Token))

 	// Share links are the only unauthenticated door to stored bytes, so they
 	// are the only egress a plan actually caps. The per-IP limiter above
@@ -583,12 +583,85 @@ func TestShareOpensOnTheWire(t *testing.T) {
 		t.Fatal("an opened link must carry last_opened")
 	}

-	// The actor is token+"/"+IP — a public credential joined to an IP. It
-	// must not appear anywhere in the response, in any shape.
+	// opens reads one link's count and last_opened back off the WIRE rather
+	// than out of the ledger, because the wire is what the panel shows.
+	opens := func(tok string) (float64, time.Time) {
+		t.Helper()
+		for _, sh := range listShares(t, srv, h, p.ID) {
+			if sh["token"] != tok {
+				continue
+			}
+			last, _ := sh["last_opened"].(string)
+			if last == "" {
+				return sh["opens"].(float64), time.Time{}
+			}
+			ts, err := time.Parse(time.RFC3339, last)
+			if err != nil {
+				t.Fatalf("last_opened %q: %v", last, err)
+			}
+			return sh["opens"].(float64), ts
+		}
+		t.Fatalf("link %s missing from the list", tok)
+		return 0, time.Time{}
+	}
+	// The debounce collapses reloads; it does not cap a link. Only the clock
+	// is under test, so it is moved rather than waited on.
+	ageDebounce := func() {
+		srv.Reads.mu.Lock()
+		defer srv.Reads.mu.Unlock()
+		for k, v := range srv.Reads.seen {
+			srv.Reads.seen[k] = v.Add(-readDebounce - time.Minute)
+		}
+	}
+	ageDebounce()
+	if rec := do(t, h, "GET", "/s/"+token, nil); rec.Code != 200 {
+		t.Fatalf("public fetch past the window: %d %s", rec.Code, rec.Body)
+	}
+	if got, _ := opens(token); got != 2 {
+		t.Fatalf("opens = %v after a reload past the debounce window, want 2", got)
+	}
+
+	// Three readers, ONE network, seconds apart: distinct browsers are
+	// distinct actors, so the count moves and last_opened follows the newest
+	// hit. token+"/"+IP alone made a whole office one reader (BEA-151) —
+	// httptest hands every request the same 192.0.2.1, which is exactly the
+	// NAT the personas were sitting behind.
+	uas := []string{
+		"Mozilla/5.0 (Macintosh; Intel Mac OS X 14_5) Safari/605.1",
+		"Mozilla/5.0 (X11; Linux x86_64) Firefox/128.0",
+		"Mozilla/5.0 (Windows NT 10.0; Win64) Chrome/126.0.0.0",
+	}
+	var afterFirst time.Time
+	for i, ua := range uas {
+		req := jsonReq(t, "GET", "/s/"+unopened, nil)
+		req.Header.Set("User-Agent", ua)
+		if rec := doHTTP(h, req); rec.Code != 200 {
+			t.Fatalf("reader %d: %d %s", i, rec.Code, rec.Body)
+		}
+		if i == 0 {
+			_, afterFirst = opens(unopened)
+		}
+	}
+	got, afterThird := opens(unopened)
+	if got != 3 {
+		t.Fatalf("three readers on one network reported %v opens, want 3 — distinct browsers are distinct readers", got)
+	}
+	if !afterThird.After(afterFirst) {
+		t.Fatalf("last_opened stuck at %s after two later readers; it must follow the newest open", afterFirst)
+	}
+
+	// The actor is token+"/"+IP+"/"+UA hash — a public credential joined to a
+	// network and a browser. No part of it may appear anywhere in the
+	// response, in any shape; a hash is not an exemption.
 	req := jsonReq(t, "GET", "/api/p/"+p.ID+"/shares", nil)
 	authAs(t, srv, req)
 	body := doHTTP(h, req).Body.String()
-	for _, leak := range []string{token + "/", "192.0.2.1", "actor", "openers"} {
+	leaks := []string{token + "/", unopened + "/", "192.0.2.1", "actor", "openers"}
+	for _, ua := range append(uas, "") {
+		sum := sha256.Sum256([]byte(ua))
+		leaks = append(leaks, hex.EncodeToString(sum[:8]))
+	}
+	for _, leak := range leaks {
 		if strings.Contains(body, leak) {
 			t.Fatalf("shares response leaks %q: %s", leak, body)
 		}
@@ -5,7 +5,7 @@
 <meta name="viewport" content="width=device-width, initial-scale=1">
 <title>BearDrive</title>
 <link rel="icon" href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32' fill='%23f5a623'><rect x='4' y='4' width='5.6' height='24'/><rect x='11.2' y='4' width='14.4' height='11.2'/><rect x='11.2' y='16.8' width='16.8' height='11.2'/></svg>">
-  <script type="module" crossorigin src="/assets/index-C17b7d2I.js"></script>
+  <script type="module" crossorigin src="/assets/index-DvCVMCS-.js"></script>
  <link rel="modulepreload" crossorigin href="/assets/_commonjsHelpers-CqkleIqs.js">
  <link rel="modulepreload" crossorigin href="/assets/mermaid-DQuCJ8Gi.js">
  <link rel="stylesheet" crossorigin href="/assets/index-DISTZ6FW.css">