Merge pull request #126 from kernel/fix/telemetry-events-vm-routing

fix: don't misroute telemetry/events to the browser VM

Author: IlyaasK

browser_routing.go

@@ -83,7 +83,10 @@ func browserRouteFromRef(ref browserrouting.Ref) (browserrouting.Route, bool) {
 func browserRoutingSubresourcesFromEnv() []string {
 	raw, ok := os.LookupEnv(browserRoutingSubresourcesEnv)
 	if !ok {
-		return []string{"curl", "telemetry"}
+		// Path prefixes eligible for direct-to-VM routing. "telemetry/stream" is
+		// the live SSE endpoint (served by the VM); "telemetry/events" is a
+		// historical read served by the control plane (S2) and must NOT be here.
+		return []string{"curl", "telemetry/stream"}
 	}
 	if strings.TrimSpace(raw) == "" {
 		return []string{}

browser_routing_test.go

@@ -72,7 +72,7 @@ func TestBrowserRoutingWarmsCacheAndRoutesAllowlistedSubresources(t *testing.T)
 }
 
 func TestBrowserRoutingRewritesTelemetryStreamToVM(t *testing.T) {
-	t.Setenv(browserRoutingSubresourcesEnv, "telemetry")
+	t.Setenv(browserRoutingSubresourcesEnv, "telemetry/stream")
 
 	var calls []struct {
 		Path string
@@ -189,8 +189,8 @@ func TestBrowserRoutingSubresourcesFromEnvDefaultsToCurl(t *testing.T) {
 		}
 		_ = os.Setenv(browserRoutingSubresourcesEnv, original)
 	})
-	if got := browserRoutingSubresourcesFromEnv(); len(got) != 2 || got[0] != "curl" || got[1] != "telemetry" {
-		t.Fatalf("expected default subresources [curl telemetry], got %#v", got)
+	if got := browserRoutingSubresourcesFromEnv(); len(got) != 2 || got[0] != "curl" || got[1] != "telemetry/stream" {
+		t.Fatalf("expected default subresources [curl telemetry/stream], got %#v", got)
 	}
 
 	t.Setenv(browserRoutingSubresourcesEnv, "")

lib/browserrouting/route_cache.go

@@ -74,10 +74,15 @@ func (c *RouteCache) Delete(sessionID string) {
 // DirectVMRoutingMiddleware rewrites allowlisted browser subresource requests to
 // the browser VM using cached base_url and jwt data.
 func DirectVMRoutingMiddleware(cache *RouteCache, subresources []string) option.Middleware {
-	allowed := map[string]struct{}{}
-	for _, subresource := range subresources {
-		if trimmed := strings.TrimSpace(subresource); trimmed != "" {
-			allowed[trimmed] = struct{}{}
+	// allowPrefixes are path prefixes (relative to browsers/{id}/) eligible for
+	// direct-to-VM routing, e.g. "curl" or "telemetry/stream". Matching is
+	// segment-boundary aware (see matchesDirectVMPrefix), so "telemetry/stream"
+	// covers "telemetry/stream[/...]" but NOT "telemetry/events" (a control-plane
+	// historical read served from S2) or "telemetry/streamfoo".
+	allowPrefixes := make([]string, 0, len(subresources))
+	for _, s := range subresources {
+		if trimmed := strings.Trim(strings.TrimSpace(s), "/"); trimmed != "" {
+			allowPrefixes = append(allowPrefixes, trimmed)
 		}
 	}
 
@@ -88,7 +93,7 @@ func DirectVMRoutingMiddleware(cache *RouteCache, subresources []string) option.
 		}
 		sessionID, subresource, suffix, ok := parseDirectVMPath(req.URL.Path)
 		if ok {
-			if _, ok := allowed[subresource]; ok {
+			if matchesDirectVMPrefix(subresource+suffix, allowPrefixes) {
 				route, ok := cache.Load(sessionID)
 				if ok {
 					base, err := url.Parse(route.BaseURL)
@@ -313,6 +318,21 @@ func parseDirectVMPath(path string) (sessionID, subresource, suffix string, ok b
 	return "", "", "", false
 }
 
+// matchesDirectVMPrefix reports whether tail (the request path after
+// browsers/{id}/) is covered by an allow prefix, matching on segment boundaries:
+// "telemetry/stream" matches "telemetry/stream" and "telemetry/stream/...", but
+// not "telemetry/events" or "telemetry/streamfoo". This keeps historical
+// control-plane reads (e.g. telemetry/events, served from S2) off the VM.
+func matchesDirectVMPrefix(tail string, prefixes []string) bool {
+	tail = strings.Trim(tail, "/")
+	for _, p := range prefixes {
+		if tail == p || strings.HasPrefix(tail, p+"/") {
+			return true
+		}
+	}
+	return false
+}
+
 func joinURLPath(basePath, subresource, suffix string) string {
 	base := "/" + strings.Trim(strings.TrimSpace(basePath), "/")
 	if base == "/" {

lib/browserrouting/route_cache_test.go

@@ -53,6 +53,57 @@ func TestDirectVMRoutingMiddlewareClearsStaleRawPath(t *testing.T) {
 	}
 }
 
+func TestDirectVMRoutingMiddlewareAllowlistMatching(t *testing.T) {
+	// Pins the segment-boundary allowlist: telemetry/stream (live SSE) routes to
+	// the VM, telemetry/events (historical, served by the control plane from S2)
+	// does NOT, and a stream-prefixed-but-different path is not matched.
+	cases := []struct {
+		name       string
+		path       string
+		routedToVM bool
+	}{
+		{"telemetry stream -> VM", "/browsers/sess-1/telemetry/stream", true},
+		{"telemetry events -> control plane", "/browsers/sess-1/telemetry/events", false},
+		{"telemetry streaming-config not a stream prefix", "/browsers/sess-1/telemetry/streaming-config", false},
+		{"bare telemetry -> control plane", "/browsers/sess-1/telemetry", false},
+		{"curl proxy -> VM", "/browsers/sess-1/curl/raw", true},
+		{"non-allowlisted subresource -> control plane", "/browsers/sess-1/fs/read", false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cache := NewRouteCache()
+			cache.Store(Route{
+				SessionID: "sess-1",
+				BaseURL:   "https://browser.example/browser/kernel",
+				JWT:       "jwt-123",
+			})
+			middleware := DirectVMRoutingMiddleware(cache, []string{"curl", "telemetry/stream"})
+
+			reqURL, err := url.Parse("https://api.example" + tc.path)
+			if err != nil {
+				t.Fatal(err)
+			}
+			req := &http.Request{URL: reqURL, Header: http.Header{}}
+
+			var got *http.Request
+			if _, err := middleware(req, func(next *http.Request) (*http.Response, error) {
+				got = next
+				return nil, nil
+			}); err != nil {
+				t.Fatal(err)
+			}
+			if got == nil {
+				t.Fatal("expected middleware to invoke next handler")
+			}
+			routedToVM := got.URL.Host == "browser.example"
+			if routedToVM != tc.routedToVM {
+				t.Fatalf("path %q routedToVM=%v, want %v (host=%q path=%q)", tc.path, routedToVM, tc.routedToVM, got.URL.Host, got.URL.Path)
+			}
+		})
+	}
+}
+
 func TestParseCacheLifecycleRejectsBrowserSubresourcePaths(t *testing.T) {
 	cases := []string{
 		"/browsers/sess-1/process/exec",