amadan

oficina / meet Public

signal: judge chat/share by raw bytes, not decoded fields

Fix round 1 from review.

1. HIGH: validateFanout measured the decoded text/bool while the
dispatch hands hub.Fanout the raw payload bytes unmodified. A
duplicated "text" key (encoding/json keeps only the last value) let
up to maxChatBytes+32-ish... actually up to 64 KB of raw bytes past
the decoded-length check, forwarded verbatim to every peer; share had
no size check at all; \uXXXX escapes inflate the wire ~6x past a
decoded-length check. Fixed by capping len(raw) at maxChatBytes+32
before decoding anything, for both types.

   The brief's own fix also asked for a decoded-string
   utf8.ValidString(*p.Text) check. Verified by scratch test that this
   is dead code: encoding/json's decoder replaces an invalid byte with
   U+FFFD on the way into a string field, so the decoded string is
   always valid UTF-8 regardless of what the raw bytes contained.
   Checked utf8.Valid(raw) instead, before decoding, which is what
   actually judges the bytes Fanout forwards.

2. MEDIUM: gave the oversize chat in TestSignallingLogsCarryNoContent
its own "xyzzy-oversize-" marker and added it to the forbidden list.

3. LOW: TestChatNeverReachesAWaitingGuestAndAWaitingGuestCannotSend now
admits the guest and sends a second chat as a positive control.

4. LOW: TestChatLimitsAndShapes now asserts the refusal body
(too-large on 413, bad-request on 400) alongside the status.

5. LOW: signalJSON's doc comment now says it normalises through a map
and cannot express a duplicate key or a raw invalid byte, and that
those go through client.signalRaw instead.

Verified each new/changed assertion against the pre-fix code first
(reverting internal/meet/signal.go to HEAD and confirming
TestChatRawPayloadSizeAndUTF8 failed 4 of 5 subtests with 200 instead
of 413/400), then restored the fix and reran to green.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 012fb787d5256f7b4ab47cd8873ed28db7a800bb parent c997b9b
3 files changed, +158 −8
  • internal/meet/signal.go +34 −2
  • internal/meettest/chat_test.go +118 −4
  • internal/meettest/signal_test.go +6 −2

CI passed — run details

diff --git a/internal/meet/signal.go b/internal/meet/signal.go
index cae25ef..5e99a23 100644
--- a/internal/meet/signal.go
+++ b/internal/meet/signal.go
@@ -8,6 +8,7 @@ import (
"net/http"
"regexp"
"strconv"
+ "unicode/utf8"
"github.com/go-chi/chi/v5"
@@ -24,9 +25,40 @@ const maxChatBytes = 2048
// validateFanout checks the two untargeted payload shapes without
// keeping anything from them: chat is exactly {"text": string} with
-// 1..2048 bytes; share is exactly {"on": bool}. Returns a status and
-// an outcome word for the refusal log, or 0.
+// 1..2048 bytes of valid UTF-8; share is exactly {"on": bool}. Returns
+// a status and an outcome word for the refusal log, or 0.
+//
+// The dispatch below hands body.Payload — these exact raw bytes,
+// unmodified — to hub.Fanout, which ships it to every other admitted
+// peer verbatim (review fix round 1, item 1): nothing here is
+// re-encoded from the decoded fields, so every check below judges the
+// raw bytes, never anything encoding/json has already sanitized on
+// their way into a Go value.
+//
+// The raw byte length is capped first, before anything is decoded, not
+// just the decoded text's length: encoding/json keeps only the last
+// value of a duplicated key, so a payload of {"text":"<64 KB>","text":
+// "x"} would decode to a harmless one-byte text while these same raw
+// bytes — the huge first value included — are exactly what gets
+// forwarded to every peer's stream; a \uXXXX escape can likewise
+// inflate the raw wire size to roughly six times what it decodes to.
+// +32 is JSON's own structural overhead for either shape
+// ({"text":"…"} or {"on":…}), never budget for the payload's content.
+//
+// UTF-8 validity is checked the same way and for the same reason:
+// encoding/json's decoder replaces an invalid byte with the Unicode
+// replacement character on its way into a string field, so *p.Text is
+// always valid UTF-8 by the time it can be inspected — checking it
+// there would pass raw bytes containing anything at all straight
+// through to Fanout. Checking raw itself, before decoding, is what
+// actually judges the bytes that get forwarded.
func validateFanout(typ string, raw json.RawMessage) (int, string) {
+ if len(raw) > maxChatBytes+32 {
+ return http.StatusRequestEntityTooLarge, "size"
+ }
+ if !utf8.Valid(raw) {
+ return http.StatusBadRequest, "shape"
+ }
dec := json.NewDecoder(bytes.NewReader(raw))
dec.DisallowUnknownFields()
switch typ {
diff --git a/internal/meettest/chat_test.go b/internal/meettest/chat_test.go
index fe9ba87..f477448 100644
--- a/internal/meettest/chat_test.go
+++ b/internal/meettest/chat_test.go
@@ -15,6 +15,13 @@ import (
// handful of chat/share cases below need dynamic values (a repeated
// string, a peer id inlined as "to") that a literal template would make
// unreadable.
+//
+// It normalises through a map (json.Marshal, then json.Unmarshal inside
+// client.signal itself), so it can only ever produce one well-formed
+// JSON object with each key once — it cannot express a duplicated key
+// or a raw, unescaped byte sequence. A case that needs either of those
+// byte-level shapes goes through client.signalRaw instead, which posts
+// its string argument verbatim.
func signalJSON(t *testing.T, body map[string]any) string {
t.Helper()
b, err := json.Marshal(body)
@@ -65,13 +72,16 @@ func TestChatReachesTheOtherAdmittedPeerWithTheSendersNameAndNotTheSender(t *tes
// Spec §2.5 and §4.3: a waiting guest is not admitted, so fan-out never
// reaches it, and its own signal is the usual "a meeting that is not
// yours does not exist" 404 — never a 403, whatever it tries to send.
+// A positive control closes the test: the same guest, once admitted,
+// does receive a chat — proof the silence above was the guest's
+// standing, not a stream that never delivers anything.
//
// ice_test.go's guestJoinedAndWaiting builds the identical waiting
// guest but returns only (*client, peer): its own stream is registered
// for t.Cleanup and never handed back. This test needs to read from
-// that stream directly (to prove the guest receives nothing), so the
-// same three steps — join link, sign in, open stream, join — are
-// inlined here instead of reusing that helper.
+// that stream directly (to prove the guest receives nothing, and later
+// that it does), so the same three steps — join link, sign in, open
+// stream, join — are inlined here instead of reusing that helper.
func TestChatNeverReachesAWaitingGuestAndAWaitingGuestCannotSend(t *testing.T) {
x, sqid, tok := memberWithMeeting(t)
a := x.browser(t)
@@ -101,6 +111,21 @@ func TestChatNeverReachesAWaitingGuestAndAWaitingGuestCannotSend(t *testing.T) {
if res.StatusCode != http.StatusNotFound || !strings.Contains(string(body), `"error":"not-found"`) {
t.Fatalf("a waiting guest's chat must be the usual 404: %d %s", res.StatusCode, body)
}
+
+ if resp, b := a.signal(sqid, pa, `{"type":"admit","payload":{"peer":"`+gpeer+`"}}`); resp.StatusCode != http.StatusOK {
+ t.Fatalf("admit: %d %s", resp.StatusCode, b)
+ }
+ if ev, _, ok := gs.nextOf("roster", 2*time.Second); !ok {
+ t.Fatalf("the newly admitted guest never saw its own roster (got %q)", ev)
+ }
+ res, _ = a.signal(sqid, pa, signalJSON(t, map[string]any{"type": "chat", "payload": map[string]any{"text": "welcome"}}))
+ if res.StatusCode != http.StatusOK {
+ t.Fatalf("chat after admit: %d", res.StatusCode)
+ }
+ ev, data, ok := gs.nextOf("chat", 2*time.Second)
+ if !ok || ev != "chat" || !strings.Contains(string(data), `"text":"welcome"`) {
+ t.Fatalf("the now-admitted guest did not receive the chat: %q ok=%v %s", ev, ok, data)
+ }
}
// Spec §4.4: chat text is capped at 2048 bytes, counted in bytes, not
@@ -134,10 +159,99 @@ func TestChatLimitsAndShapes(t *testing.T) {
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
- res, _ := a.signal(sqid, pa, signalJSON(t, c.body))
+ res, body := a.signal(sqid, pa, signalJSON(t, c.body))
if res.StatusCode != c.status {
t.Fatalf("%s: %d, want %d", c.name, res.StatusCode, c.status)
}
+ switch c.status {
+ case http.StatusRequestEntityTooLarge:
+ if !strings.Contains(string(body), `"error":"too-large"`) {
+ t.Fatalf("%s: body %s, want error=too-large", c.name, body)
+ }
+ case http.StatusBadRequest:
+ if !strings.Contains(string(body), `"error":"bad-request"`) {
+ t.Fatalf("%s: body %s, want error=bad-request", c.name, body)
+ }
+ }
})
}
}
+
+// Review fix round 1, item 1 (HIGH): the dispatch in signal.go hands
+// hub.Fanout the payload's raw bytes verbatim — never a re-encoding of
+// what validateFanout decoded — so validateFanout must judge those same
+// raw bytes, not just the decoded text's length. Each case here needs a
+// shape signalJSON cannot express (a duplicated JSON key, or a literal
+// invalid byte), so all five go through client.signalRaw, which posts
+// its string argument verbatim and, unlike client.signal, does not
+// inject "peer" itself.
+func TestChatRawPayloadSizeAndUTF8(t *testing.T) {
+ x, sqid, _ := memberWithMeeting(t)
+ a := x.browser(t)
+ a.signIn("paul@example.com")
+ sa := a.stream("/m/" + sqid + "/stream")
+ defer sa.close()
+ pa, _ := sa.hello()
+ a.signal(sqid, pa, `{"type":"join"}`)
+ sa.nextOf("roster", 2*time.Second)
+
+ t.Run("4096-byte duplicated text key", func(t *testing.T) {
+ // encoding/json keeps only the last value of a duplicated key —
+ // this decodes to a one-byte "x" — but the raw payload handed to
+ // Fanout still carries all 4096+ bytes of the first value.
+ body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"` + strings.Repeat("a", 4096) + `","text":"x"}}`
+ res, respBody := a.signalRaw(sqid, body)
+ if res.StatusCode != http.StatusRequestEntityTooLarge || !strings.Contains(string(respBody), `"error":"too-large"`) {
+ t.Fatalf("duplicated text key: %d %s", res.StatusCode, respBody)
+ }
+ })
+
+ t.Run("5000 duplicated on keys", func(t *testing.T) {
+ var b strings.Builder
+ b.WriteByte('{')
+ for i := 0; i < 5000; i++ {
+ if i > 0 {
+ b.WriteByte(',')
+ }
+ b.WriteString(`"on":true`)
+ }
+ b.WriteByte('}')
+ body := `{"peer":"` + pa + `","type":"share","payload":` + b.String() + `}`
+ res, respBody := a.signalRaw(sqid, body)
+ if res.StatusCode != http.StatusRequestEntityTooLarge || !strings.Contains(string(respBody), `"error":"too-large"`) {
+ t.Fatalf("duplicated on keys: %d %s", res.StatusCode, respBody)
+ }
+ })
+
+ t.Run("2048 backslash-u escapes", func(t *testing.T) {
+ // Each A escape decodes to one ASCII byte ("A"), so the
+ // decoded text is exactly 2048 bytes — at the cap, not over it —
+ // while the raw escapes are six times that on the wire.
+ text := strings.Repeat(`\u0041`, 2048)
+ body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"` + text + `"}}`
+ res, respBody := a.signalRaw(sqid, body)
+ if res.StatusCode != http.StatusRequestEntityTooLarge || !strings.Contains(string(respBody), `"error":"too-large"`) {
+ t.Fatalf("escaped text: %d %s", res.StatusCode, respBody)
+ }
+ })
+
+ t.Run("2048-byte multibyte text is still accepted", func(t *testing.T) {
+ // "é" is two raw UTF-8 bytes, unescaped: 1024 of them is exactly
+ // 2048 bytes both raw and decoded — legitimate multibyte content
+ // at the cap must not be caught by the new raw-size guard.
+ text := strings.Repeat("é", 1024)
+ body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"` + text + `"}}`
+ res, respBody := a.signalRaw(sqid, body)
+ if res.StatusCode != http.StatusOK {
+ t.Fatalf("multibyte text at the cap: %d %s", res.StatusCode, respBody)
+ }
+ })
+
+ t.Run("invalid UTF-8 is refused", func(t *testing.T) {
+ body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"bad` + "\xff" + `byte"}}`
+ res, respBody := a.signalRaw(sqid, body)
+ if res.StatusCode != http.StatusBadRequest || !strings.Contains(string(respBody), `"error":"bad-request"`) {
+ t.Fatalf("invalid UTF-8: %d %s", res.StatusCode, respBody)
+ }
+ })
+}
diff --git a/internal/meettest/signal_test.go b/internal/meettest/signal_test.go
index 85a9987..96071e6 100644
--- a/internal/meettest/signal_test.go
+++ b/internal/meettest/signal_test.go
@@ -619,7 +619,11 @@ func TestSignallingLogsCarryNoContent(t *testing.T) {
if buf.Len() != beforeChat {
t.Fatalf("an accepted chat appended %d bytes to the log: %q", buf.Len()-beforeChat, buf.String()[beforeChat:])
}
- oversizeChat := `{"type":"chat","payload":{"text":"` + strings.Repeat("a", 2049) + `"}}`
+ // "xyzzy-oversize-" is its own needle: the earlier assertion only
+ // proved the accepted chat's text never grew the log, not that a
+ // *refused* chat's text is equally absent (review fix round 1,
+ // item 2).
+ oversizeChat := `{"type":"chat","payload":{"text":"xyzzy-oversize-` + strings.Repeat("a", 2049) + `"}}`
if resp, _ := c.signal(sqid, peer, oversizeChat); resp.StatusCode != http.StatusRequestEntityTooLarge {
t.Fatalf("oversize chat: %d", resp.StatusCode)
} // outcome=size
@@ -662,7 +666,7 @@ func TestSignallingLogsCarryNoContent(t *testing.T) {
forbidden := []string{
"Secret title", "SDPSECRET", "203.0.113.9", "STALESECRET", tok,
"paul@example.com", "zaphod-xyzzy@example.com", "zaphod-xyzzy",
- "xyzzy-plugh-chat",
+ "xyzzy-plugh-chat", "xyzzy-oversize-",
}
for _, ck := range c.cookies {
forbidden = append(forbidden, ck.Value)