carlos / platform Public

edge: copy the analytics switch map; prefetch matches prefetch;prerender

SetAnalyticsOn stored the caller's map by pointer, so the serving goroutines
read a map the caller still owned. A telemetry pass that reused its map would
not have raced quietly — it would have been Go's fatal concurrent map
read-and-write, an edge-wide crash. Copy it on the way in; callers may reuse
their map, and the test now scribbles on its own map after handing it over.

Sec-Purpose is a token list. Chrome sends "prefetch;prerender" for a
speculation-rules prerender, and that request also carries
Sec-Fetch-Dest: document — so an exact-match comparison counted a page
nobody had looked at as a pageview. Match the first token instead.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev f6827709dce1cd25854aa12b30ec9210574d81e4 parent cc838ab
2 files changed, +44 −10
  • internal/edge/analytics.go +26 −7
  • internal/edge/analytics_test.go +18 −3
diff --git a/internal/edge/analytics.go b/internal/edge/analytics.go
index 09bd27195..16c1f1df9 100644
--- a/internal/edge/analytics.go
+++ b/internal/edge/analytics.go
@@ -34,14 +34,21 @@ func (e *Edge) EnableAnalytics(m AnalyticsMeter) {
// SetAnalyticsOn replaces the switch map: sqid+"/"+app → counted. The
// host's telemetry pass pushes it on every serial-triggered full pass and
// every five minutes (spec §3.3, "converged by boxes on the next pass").
-// The map is stored, never copied and never mutated here: a caller must
-// hand over a map it will not write to again.
+// The map is COPIED before it is stored, so a caller may keep and reuse
+// the map it passed: the stored copy is read by every serving goroutine
+// without a lock, and a caller that went on writing to a shared map would
+// turn that into Go's fatal concurrent map read-and-write — an edge-wide
+// crash, not a race one recovers from.
func (e *Edge) SetAnalyticsOn(on map[string]bool) {
if on == nil {
e.analyticsOn.Store(nil)
return
}
- e.analyticsOn.Store(&on)
+ cp := make(map[string]bool, len(on))
+ for k, v := range on {
+ cp[k] = v
+ }
+ e.analyticsOn.Store(&cp)
}
func (e *Edge) analyticsEnabled(rt registry.Route) bool {
@@ -62,9 +69,10 @@ func withHopVerified(ctx context.Context) context.Context {
// clientIP is the address the visitor hash uses (spec §3.3): the last
// element of the verified inbound X-Forwarded-For chain on a proven hop,
-// otherwise RemoteAddr's host. A header the client set never reaches it —
-// on an unverified request acceptHop has already stripped the forwarded
-// headers, and this function does not look at them either way.
+// otherwise RemoteAddr's host. X-Forwarded-For is read ONLY under a proven
+// hop, whose mark nothing outside acceptHop can put on a request; on every
+// other request the header is ignored whether it is present or not, so a
+// header a client set can never reach the visitor hash.
func clientIP(r *http.Request) string {
if hopVerified(r.Context()) {
chain := strings.Join(r.Header.Values("X-Forwarded-For"), ", ")
@@ -82,6 +90,17 @@ func clientIP(r *http.Request) string {
return host
}
+// isPrefetchPurpose reports whether a Sec-Purpose (or legacy Purpose)
+// header value says this request is a prefetch. The value is a token list:
+// Chrome sends a bare "prefetch" for link prefetches and
+// "prefetch;prerender" for a speculation-rules prerender, which also
+// carries Sec-Fetch-Dest: document and so would otherwise be counted as a
+// pageview nobody looked at. Only the first token decides.
+func isPrefetchPurpose(v string) bool {
+ first, _, _ := strings.Cut(v, ";")
+ return strings.EqualFold(strings.TrimSpace(first), "prefetch")
+}
+
// observeAnalytics files one Obs when the route's app is switched on. It
// runs inside the observer defer in ServeHTTP, after the response has
// completed, and reads only: the latched status and Content-Type off the
@@ -96,7 +115,7 @@ func (e *Edge) observeAnalytics(rt registry.Route, r *http.Request, ob *counting
if u, err := url.Parse(r.Header.Get("Referer")); err == nil {
refHost = u.Hostname()
}
- prefetch := strings.EqualFold(r.Header.Get("Sec-Purpose"), "prefetch") || strings.EqualFold(r.Header.Get("Purpose"), "prefetch")
+ prefetch := isPrefetchPurpose(r.Header.Get("Sec-Purpose")) || isPrefetchPurpose(r.Header.Get("Purpose"))
ref.Observe(analytics.Obs{
Acct: release.RouteAccount(rt), App: rt.App, Host: rt.Host,
Method: r.Method, Status: ob.status, ContentType: ob.ctype,
diff --git a/internal/edge/analytics_test.go b/internal/edge/analytics_test.go
index 173b24cce..33e46c3be 100644
--- a/internal/edge/analytics_test.go
+++ b/internal/edge/analytics_test.go
@@ -30,7 +30,9 @@ func TestClientIP(t *testing.T) {
if ip := clientIP(r); ip != "203.0.113.5" {
t.Fatalf("unverified: %q", ip)
}
- r2 := r.WithContext(withHopVerified(r.Context()))
+ r2 := httptest.NewRequest("GET", "http://h/", nil)
+ r2 = r2.WithContext(withHopVerified(r2.Context()))
+ r2.RemoteAddr = "203.0.113.5:4444"
r2.Header.Set("X-Forwarded-For", "10.0.0.1, 198.51.100.7")
if ip := clientIP(r2); ip != "198.51.100.7" {
t.Fatalf("verified hop: %q", ip)
@@ -72,7 +74,13 @@ func TestEdgeObservesOnlyWhenSwitchedOn(t *testing.T) {
if len(m.snap()) != 0 {
t.Fatal("nothing is observed while the app is off")
}
- e.SetAnalyticsOn(map[string]bool{release.OpsAccount + "/web": true})
+ on := map[string]bool{release.OpsAccount + "/web": true}
+ e.SetAnalyticsOn(on)
+ // The switch map is copied, so a caller may keep writing to its own:
+ // the edge reads its copy from every serving goroutine without a lock,
+ // and a shared map would be a fatal concurrent map read-and-write.
+ delete(on, release.OpsAccount+"/web")
+ on["scribble"] = true
get("/pricing?x=1", map[string]string{"Referer": "https://news.ycombinator.com/item?id=1", "User-Agent": "Mozilla/5.0 Safari", "Sec-Fetch-Dest": "document"})
obs := m.snap()
if len(obs) != 1 {
@@ -88,9 +96,16 @@ func TestEdgeObservesOnlyWhenSwitchedOn(t *testing.T) {
if !m.snap()[1].Prefetch {
t.Fatal("prefetch header must be carried")
}
+ // A speculation-rules prerender arrives as Sec-Purpose:
+ // prefetch;prerender with Sec-Fetch-Dest: document — a pageview
+ // nobody looked at unless the first token is matched.
+ get("/y", map[string]string{"Sec-Purpose": "prefetch;prerender", "Sec-Fetch-Dest": "document"})
+ if !m.snap()[2].Prefetch {
+ t.Fatal("prefetch;prerender is a prefetch")
+ }
e.SetAnalyticsOn(nil)
get("/", nil)
- if len(m.snap()) != 2 {
+ if len(m.snap()) != 3 {
t.Fatal("switching off stops observation")
}
}