amadan
stream: 406 for a mis-headered client, and three tests that can fail
Fix round 2. The Accept refusal is 406 with {"error":"accept"}: the check runs before any authorization or lookup, so it is identical for every meeting and every person and discloses nothing, and it tells a mis-headered client what is wrong instead of sending it round the fresh-stream loop.
The three new tests from round 1 were vacuous, each verified against a mutated build before and after. The oversize last is now valid JSON above the cap, so the parse cannot refuse it for us. The clamp test uses a zero heartbeat with a real TTL and waits for a beat: hello is written before the ticker exists and net/http recovers per connection, so a panicking stream satisfied every earlier assertion. The Accept test uses a new bounded getNav rather than the unbounded get, and asserts the body exactly. The harness also cancels the app context at cleanup, ahead of closing the test server, so no live stream can hold Close.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
3 files changed,
+66
−20
internal/meet/stream.go+10 −4internal/meettest/harness_test.go+20 −3internal/meettest/stream_test.go+36 −13
CI passed — run details
diff --git a/internal/meet/stream.go b/internal/meet/stream.go| index d00ca9d..10172e4 100644 |
| --- a/internal/meet/stream.go |
| +++ b/internal/meet/stream.go |
| @@ -62,7 +62,8 @@ func (a *app) sessionKey(r *http.Request) (string, bool) { |
| // opened by EventSource, which always asks for text/event-stream; a |
| // cross-site top-level navigation asks for HTML, and without this check |
| // it would take the session's one peer slot in the room and hold it for |
| -// the resume window. The probe forms are exempt: they attach nothing. |
| +// the resume window. Anything else is 406 with {"error":"accept"}. The |
| +// probe forms are exempt: they attach nothing. |
| func acceptsEventStream(r *http.Request) bool { |
| return strings.Contains(strings.ToLower(r.Header.Get("Accept")), "text/event-stream") |
| } |
| @@ -110,9 +111,14 @@ func (a *app) stream(w http.ResponseWriter, r *http.Request) { |
| } |
| probe := r.URL.Query().Get("probe") == "1" |
| if !probe && !acceptsEventStream(r) { |
| - // Not a stream client: the same 404 an unknown meeting gets, and |
| - // no participation is created (ruling 15). |
| - streamJSON(w, http.StatusNotFound, map[string]any{"boot": a.hub.Boot()}) |
| + // Not a stream client, so nothing is looked up and nothing is |
| + // allocated (ruling 15). This runs before any authorization, so |
| + // the answer is identical for every sqid and every person and |
| + // discloses nothing — which is why it can afford to be the |
| + // truthful 406 rather than the room's 404, and why a |
| + // mis-headered client is told to fix its header instead of |
| + // being sent round the fresh-stream loop. |
| + streamJSON(w, http.StatusNotAcceptable, map[string]any{"error": "accept"}) |
| return |
| } |
diff --git a/internal/meettest/harness_test.go b/internal/meettest/harness_test.go| index e51ffd4..68db65f 100644 |
| --- a/internal/meettest/harness_test.go |
| +++ b/internal/meettest/harness_test.go |
| @@ -58,7 +58,12 @@ type harness struct { |
| func (x *harness) server() *httptest.Server { |
| if x.srv == nil { |
| x.srv = httptest.NewServer(x.h) |
| + // Registered before the shutdown, so it runs after it: Close |
| + // waits for outstanding requests, and a live stream ends when |
| + // the app's context is cancelled. A stream's own cleanup is |
| + // registered later still, so it runs before both. |
| x.t.Cleanup(x.srv.Close) |
| + x.t.Cleanup(x.shutdown) |
| } |
| return x.srv |
| } |
| @@ -245,11 +250,23 @@ func (c *client) get(target string) *httptest.ResponseRecorder { |
| // stream fails in two seconds instead of holding the recorder open for |
| // the whole TTL. A recorder cannot read a stream anyway. |
| func (c *client) getStream(target string) *httptest.ResponseRecorder { |
| + return c.do(c.boundedGet(target, "text/event-stream")) |
| +} |
| + |
| +// getNav is the other half of ruling 15: a top-level navigation to a |
| +// stream URL, which asks for HTML and must be refused. Bounded like |
| +// getStream, so a regression that lets it attach fails in two seconds |
| +// instead of holding the recorder open for the whole TTL. |
| +func (c *client) getNav(target string) *httptest.ResponseRecorder { |
| + return c.do(c.boundedGet(target, "text/html,application/xhtml+xml")) |
| +} |
| + |
| +func (c *client) boundedGet(target, accept string) *http.Request { |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) |
| - defer cancel() |
| + c.t.Cleanup(cancel) |
| req := httptest.NewRequest(http.MethodGet, target, nil).WithContext(ctx) |
| - req.Header.Set("Accept", "text/event-stream") |
| - return c.do(req) |
| + req.Header.Set("Accept", accept) |
| + return req |
| } |
| func (c *client) post(target string, form url.Values) *httptest.ResponseRecorder { |
diff --git a/internal/meettest/stream_test.go b/internal/meettest/stream_test.go| index 0320afa..edf5528 100644 |
| --- a/internal/meettest/stream_test.go |
| +++ b/internal/meettest/stream_test.go |
| @@ -2,6 +2,7 @@ package meettest |
| import ( |
| "encoding/json" |
| + "fmt" |
| "net/http" |
| "net/http/httptest" |
| "net/url" |
| @@ -294,17 +295,17 @@ func TestAStreamWithoutTheEventStreamAcceptTakesNoPeer(t *testing.T) { |
| c := x.browser(t) |
| c.signIn("paul@example.com") |
| - rec := c.get("/m/" + sqid + "/stream") // a navigation, not EventSource |
| - if rec.Code != http.StatusNotFound { |
| - t.Fatalf("stream without the Accept header: %d, want 404", rec.Code) |
| + rec := c.getNav("/m/" + sqid + "/stream") // a navigation, not EventSource |
| + if rec.Code != http.StatusNotAcceptable { |
| + t.Fatalf("stream without the Accept header: %d, want 406", rec.Code) |
| } |
| - // The 404 an unknown meeting gets, and nothing was allocated: the |
| - // real stream that follows is a fresh peer, not a 409. |
| - if _, boot, _ := answer(t, rec); boot == "" { |
| - t.Fatalf("404 body %q, want the boot id", rec.Body.String()) |
| + // The check runs before any lookup, so the answer is the same for |
| + // every meeting and every person, and says what is actually wrong. |
| + if got := strings.TrimSpace(rec.Body.String()); got != `{"error":"accept"}` { |
| + t.Fatalf("406 body %s", got) |
| } |
| - if rec := c.get("/m/" + sqid + "/stream?peer=whatever"); rec.Code != http.StatusNotFound { |
| - t.Fatalf("resume without the Accept header: %d, want 404", rec.Code) |
| + if rec := c.getNav("/m/" + sqid + "/stream?peer=whatever"); rec.Code != http.StatusNotAcceptable { |
| + t.Fatalf("resume without the Accept header: %d, want 406", rec.Code) |
| } |
| s := c.stream("/m/" + sqid + "/stream") |
| defer s.close() |
| @@ -324,8 +325,22 @@ func TestAnOversizeLastIsRefusedBeforeItIsParsed(t *testing.T) { |
| peer, _ := s.hello() |
| s.close() |
| - big := strings.Repeat("a", 9<<10) |
| - rec := c.getStream("/m/" + sqid + "/stream?peer=" + peer + "&last=" + big) |
| + // Valid JSON, and too big: a last that only the cap can refuse. Bad |
| + // JSON would be refused by the parse whether the cap is there or |
| + // not, and would prove nothing. |
| + var big strings.Builder |
| + big.WriteString("{") |
| + for i := 0; i < 700; i++ { |
| + if i > 0 { |
| + big.WriteString(",") |
| + } |
| + fmt.Fprintf(&big, "%q:1", fmt.Sprintf("%032x", i)) |
| + } |
| + big.WriteString("}") |
| + if !json.Valid([]byte(big.String())) || big.Len() <= 8<<10 { |
| + t.Fatalf("the oversize last is %d bytes and valid=%v", big.Len(), json.Valid([]byte(big.String()))) |
| + } |
| + rec := c.getStream("/m/" + sqid + "/stream?peer=" + peer + "&last=" + url.QueryEscape(big.String())) |
| if rec.Code != http.StatusBadRequest { |
| t.Fatalf("oversize last: %d, want 400", rec.Code) |
| } |
| @@ -360,7 +375,12 @@ func TestShutdownEndsLiveStreams(t *testing.T) { |
| // ticker inside a request goroutine: both timings have a floor. |
| func TestZeroStreamTimingsAreClampedNotFatal(t *testing.T) { |
| x, sqid, _ := memberWithMeeting(t) |
| - x.ctl.SetStreamTimings(0, 0) |
| + // A zero heartbeat with a TTL that is not zero: the heartbeat is |
| + // what proves the clamp. hello is written before the ticker exists, |
| + // and net/http recovers a panic per connection, so a stream that |
| + // panicked on time.NewTicker(0) would still say hello and still |
| + // close — but it would never beat. |
| + x.ctl.SetStreamTimings(0, 500*time.Millisecond) |
| c := x.browser(t) |
| c.signIn("paul@example.com") |
| s := c.stream("/m/" + sqid + "/stream") |
| @@ -368,8 +388,11 @@ func TestZeroStreamTimingsAreClampedNotFatal(t *testing.T) { |
| if peer, _ := s.hello(); peer == "" { |
| t.Fatal("no peer") |
| } |
| + if !s.waitHeartbeat(2 * time.Second) { |
| + t.Fatal("a floored heartbeat never beat") |
| + } |
| if !s.waitClosed(2 * time.Second) { |
| - t.Fatal("a floored TTL did not end the stream") |
| + t.Fatal("the stream did not end at its TTL") |
| } |
| // The app is still serving: nothing panicked. |
| if rec := c.get("/m/" + sqid + "/join"); rec.Code != http.StatusOK { |