Skip to content

Commit f3c0e7c

Browse files
rgarciaclaude
andcommitted
feat: default telemetry to VM routing allowlist + fallback hardening
Address adversarial review notes on the control-plane fallback: - Add "telemetry" to the default direct-VM routing subresource allowlist (was ["curl"] only) so the telemetry SSE stream is routed to the VM and can fall back to the control plane when the VM is gone. - isBrowserGoneResponse: on a mid-stream body read error, leave the body in place (via MultiReader) instead of truncating it, and treat it as a non-marker so the live VM response is surfaced unchanged. - Comment the body-less GET/HEAD assumption at the fallback re-issue site. - Add regression tests: 503/504 fallback, HEAD fallback, gone header with wrong code (no fallback, body preserved), browser_gone body without the X-Kernel-Upstream header (no fallback, body preserved), and final control-plane body readability after a 502 fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 740115a commit f3c0e7c

4 files changed

Lines changed: 200 additions & 9 deletions

File tree

browser_routing.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ func withBrowserRouteCache(cache *browserrouting.RouteCache) option.RequestOptio
4747

4848
func browserRouteCacheFromOptions(opts []option.RequestOption) *browserrouting.RouteCache {
4949
for _, opt := range opts {
50-
if carrier, ok := opt.(interface{ browserRouteCache() *browserrouting.RouteCache }); ok {
50+
if carrier, ok := opt.(interface {
51+
browserRouteCache() *browserrouting.RouteCache
52+
}); ok {
5153
if cache := carrier.browserRouteCache(); cache != nil {
5254
return cache
5355
}
@@ -81,7 +83,7 @@ func browserRouteFromRef(ref browserrouting.Ref) (browserrouting.Route, bool) {
8183
func browserRoutingSubresourcesFromEnv() []string {
8284
raw, ok := os.LookupEnv(browserRoutingSubresourcesEnv)
8385
if !ok {
84-
return []string{"curl"}
86+
return []string{"curl", "telemetry"}
8587
}
8688
if strings.TrimSpace(raw) == "" {
8789
return []string{}

browser_routing_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,8 @@ func TestBrowserRoutingSubresourcesFromEnvDefaultsToCurl(t *testing.T) {
127127
}
128128
_ = os.Setenv(browserRoutingSubresourcesEnv, original)
129129
})
130-
if got := browserRoutingSubresourcesFromEnv(); len(got) != 1 || got[0] != "curl" {
131-
t.Fatalf("expected default subresources [curl], got %#v", got)
130+
if got := browserRoutingSubresourcesFromEnv(); len(got) != 2 || got[0] != "curl" || got[1] != "telemetry" {
131+
t.Fatalf("expected default subresources [curl telemetry], got %#v", got)
132132
}
133133

134134
t.Setenv(browserRoutingSubresourcesEnv, "")

lib/browserrouting/route_cache.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,9 @@ func DirectVMRoutingMiddleware(cache *RouteCache, subresources []string) option.
142142
}
143143

144144
// Restore the original control-plane target: URL/Host/Authorization
145-
// and drop the VM jwt query param. A body-less GET/HEAD is safe to
146-
// resend.
145+
// and drop the VM jwt query param. The GET/HEAD gate in
146+
// shouldFallBackToControlPlane guarantees this request is body-less, so
147+
// re-issuing it without re-buffering a request body is safe.
147148
snapshot.restore(req)
148149

149150
res, err = next(req)
@@ -260,13 +261,17 @@ func isBrowserGoneResponse(res *http.Response) bool {
260261
}
261262

262263
body, err := io.ReadAll(res.Body)
264+
if err != nil {
265+
// On a mid-stream read error, leave the original body in place (it has
266+
// already been partly consumed but we avoid truncating ContentLength) and
267+
// treat this as "not the gone marker" so we surface the live VM response.
268+
res.Body = io.NopCloser(io.MultiReader(bytes.NewReader(body), res.Body))
269+
return false
270+
}
263271
_ = res.Body.Close()
264272
// Restore the body so the response remains readable regardless of outcome.
265273
res.Body = io.NopCloser(bytes.NewReader(body))
266274
res.ContentLength = int64(len(body))
267-
if err != nil {
268-
return false
269-
}
270275

271276
var payload struct {
272277
Code string `json:"code"`

lib/browserrouting/route_cache_test.go

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,190 @@ func TestFallbackNonRoutedRequestUntouched(t *testing.T) {
299299
}
300300
}
301301

302+
// TestFallback503And504ToControlPlane: 503 and 504 from a dead VM also trigger a
303+
// single control-plane fallback (same as 502).
304+
func TestFallback503And504ToControlPlane(t *testing.T) {
305+
for _, status := range []int{http.StatusServiceUnavailable, http.StatusGatewayTimeout} {
306+
cache := NewRouteCache()
307+
cache.Store(Route{SessionID: "sess-1", BaseURL: "https://browser.example/browser/kernel", JWT: "jwt-123"})
308+
middleware := DirectVMRoutingMiddleware(cache, []string{"curl"})
309+
310+
var calls int
311+
var lastReq *http.Request
312+
res, err := middleware(routedGetRequest(t), func(req *http.Request) (*http.Response, error) {
313+
calls++
314+
lastReq = req
315+
if calls == 1 {
316+
return &http.Response{
317+
StatusCode: status,
318+
Header: http.Header{},
319+
Body: io.NopCloser(strings.NewReader("upstream down")),
320+
}, nil
321+
}
322+
return &http.Response{
323+
StatusCode: http.StatusOK,
324+
Header: http.Header{},
325+
Body: io.NopCloser(strings.NewReader("cp-ok")),
326+
}, nil
327+
})
328+
if err != nil {
329+
t.Fatal(err)
330+
}
331+
if calls != 2 {
332+
t.Fatalf("status %d: expected VM attempt + CP fallback, got %d attempts", status, calls)
333+
}
334+
if lastReq.URL.Host != "api.example" {
335+
t.Fatalf("status %d: expected fallback to control plane, got host %q", status, lastReq.URL.Host)
336+
}
337+
if res.StatusCode != http.StatusOK {
338+
t.Fatalf("status %d: expected control-plane 200, got %d", status, res.StatusCode)
339+
}
340+
}
341+
}
342+
343+
// TestFallbackHeadToControlPlane: HEAD is idempotent and must fall back like GET.
344+
func TestFallbackHeadToControlPlane(t *testing.T) {
345+
cache := NewRouteCache()
346+
cache.Store(Route{SessionID: "sess-1", BaseURL: "https://browser.example/browser/kernel", JWT: "jwt-123"})
347+
middleware := DirectVMRoutingMiddleware(cache, []string{"curl"})
348+
349+
req := routedGetRequest(t)
350+
req.Method = http.MethodHead
351+
352+
var calls int
353+
var lastReq *http.Request
354+
res, err := middleware(req, func(req *http.Request) (*http.Response, error) {
355+
calls++
356+
lastReq = req
357+
if calls == 1 {
358+
return &http.Response{
359+
StatusCode: http.StatusBadGateway,
360+
Header: http.Header{},
361+
Body: io.NopCloser(strings.NewReader("Upstream not available")),
362+
}, nil
363+
}
364+
return &http.Response{
365+
StatusCode: http.StatusOK,
366+
Header: http.Header{},
367+
Body: io.NopCloser(strings.NewReader("")),
368+
}, nil
369+
})
370+
if err != nil {
371+
t.Fatal(err)
372+
}
373+
if calls != 2 {
374+
t.Fatalf("expected HEAD to fall back (VM + CP), got %d attempts", calls)
375+
}
376+
if lastReq.URL.Host != "api.example" {
377+
t.Fatalf("expected HEAD fallback to control plane, got host %q", lastReq.URL.Host)
378+
}
379+
if lastReq.Header.Get("Authorization") != "Bearer sk_test" {
380+
t.Fatalf("expected Authorization restored on HEAD fallback, got %q", lastReq.Header.Get("Authorization"))
381+
}
382+
if res.StatusCode != http.StatusOK {
383+
t.Fatalf("expected control-plane 200 for HEAD, got %d", res.StatusCode)
384+
}
385+
}
386+
387+
// TestFallbackGoneHeaderWrongCodeNoFallback: a 404 with the X-Kernel-Upstream:
388+
// gone header but a JSON code that is NOT "browser_gone" must NOT fall back, and
389+
// its body must remain readable on the returned response.
390+
func TestFallbackGoneHeaderWrongCodeNoFallback(t *testing.T) {
391+
cache := NewRouteCache()
392+
cache.Store(Route{SessionID: "sess-1", BaseURL: "https://browser.example/browser/kernel", JWT: "jwt-123"})
393+
middleware := DirectVMRoutingMiddleware(cache, []string{"curl"})
394+
395+
const bodyText = `{"code":"not_found","message":"nope"}`
396+
var calls int
397+
res, err := middleware(routedGetRequest(t), func(req *http.Request) (*http.Response, error) {
398+
calls++
399+
return &http.Response{
400+
StatusCode: http.StatusNotFound,
401+
Header: http.Header{
402+
"Content-Type": []string{"application/json"},
403+
"X-Kernel-Upstream": []string{"gone"},
404+
},
405+
Body: io.NopCloser(strings.NewReader(bodyText)),
406+
}, nil
407+
})
408+
if err != nil {
409+
t.Fatal(err)
410+
}
411+
if calls != 1 {
412+
t.Fatalf("expected no fallback when code != browser_gone, got %d attempts", calls)
413+
}
414+
body, _ := io.ReadAll(res.Body)
415+
if string(body) != bodyText {
416+
t.Fatalf("expected body preserved after non-triggering gone sniff, got %q", string(body))
417+
}
418+
}
419+
420+
// TestFallbackGoneBodyWithoutHeaderNoFallback: a browser_gone JSON body WITHOUT
421+
// the X-Kernel-Upstream: gone header must NOT fall back (header is checked before
422+
// the body, so the body is never even sniffed).
423+
func TestFallbackGoneBodyWithoutHeaderNoFallback(t *testing.T) {
424+
cache := NewRouteCache()
425+
cache.Store(Route{SessionID: "sess-1", BaseURL: "https://browser.example/browser/kernel", JWT: "jwt-123"})
426+
middleware := DirectVMRoutingMiddleware(cache, []string{"curl"})
427+
428+
const bodyText = `{"code":"browser_gone","message":"browser not found"}`
429+
var calls int
430+
res, err := middleware(routedGetRequest(t), func(req *http.Request) (*http.Response, error) {
431+
calls++
432+
return &http.Response{
433+
StatusCode: http.StatusNotFound,
434+
Header: http.Header{"Content-Type": []string{"application/json"}},
435+
Body: io.NopCloser(strings.NewReader(bodyText)),
436+
}, nil
437+
})
438+
if err != nil {
439+
t.Fatal(err)
440+
}
441+
if calls != 1 {
442+
t.Fatalf("expected no fallback without X-Kernel-Upstream header, got %d attempts", calls)
443+
}
444+
body, _ := io.ReadAll(res.Body)
445+
if string(body) != bodyText {
446+
t.Fatalf("expected body untouched (header gate hit first), got %q", string(body))
447+
}
448+
}
449+
450+
// TestFallbackFinalCPBodyReadable: after a 502 fallback, the control-plane
451+
// response body must be fully readable by downstream consumers.
452+
func TestFallbackFinalCPBodyReadable(t *testing.T) {
453+
cache := NewRouteCache()
454+
cache.Store(Route{SessionID: "sess-1", BaseURL: "https://browser.example/browser/kernel", JWT: "jwt-123"})
455+
middleware := DirectVMRoutingMiddleware(cache, []string{"curl"})
456+
457+
const cpBody = `{"code":"browser_not_found"}`
458+
var calls int
459+
res, err := middleware(routedGetRequest(t), func(req *http.Request) (*http.Response, error) {
460+
calls++
461+
if calls == 1 {
462+
return &http.Response{
463+
StatusCode: http.StatusBadGateway,
464+
Header: http.Header{},
465+
Body: io.NopCloser(strings.NewReader("Upstream not available")),
466+
}, nil
467+
}
468+
return &http.Response{
469+
StatusCode: http.StatusNotFound,
470+
Header: http.Header{"Content-Type": []string{"application/json"}},
471+
Body: io.NopCloser(strings.NewReader(cpBody)),
472+
}, nil
473+
})
474+
if err != nil {
475+
t.Fatal(err)
476+
}
477+
body, readErr := io.ReadAll(res.Body)
478+
if readErr != nil {
479+
t.Fatalf("expected readable CP body, got error %v", readErr)
480+
}
481+
if string(body) != cpBody {
482+
t.Fatalf("expected CP body %q, got %q", cpBody, string(body))
483+
}
484+
}
485+
302486
func TestDirectVMRoutingMiddlewareClearsStaleRawPath(t *testing.T) {
303487
cache := NewRouteCache()
304488
cache.Store(Route{

0 commit comments

Comments
 (0)