amadan
signal: exact keys, and the validated bytes on the wire
Codex S4-1 and S4-2, both P1, both about one gap: what /signal
validated and what the fan-out actually put on every other peer's
stream were two different things.
S4-1. encoding/json matches an object's keys to struct fields
case-insensitively, and DisallowUnknownFields does not change that.
{"text":"<2049 a>","Text":"x"} is 2071 raw bytes — inside the +32 cap —
and decoded to the last match, a one-byte text, while the raw payload
forwarded to every recipient carried both keys and every client reads
the lowercase one: 2049 bytes delivered through a 2048-byte cap. So the
keys are judged byte-exactly now, by a token walk that counts members
rather than by a map, which collapses a duplicate and cannot tell
{"text":"a","text":"b"} from {"text":"b"}. Same for share's "on".
S4-2. writeEvent's json.Marshal HTML-escapes `<`, `>` and `&` inside a
json.RawMessage too, so a validated 2059-byte payload of literal `<`
went out at 12,381 bytes — measured, not estimated. Two halves to the
fix: validateFanout returns the payload re-encoded from the value it
validated (SetEscapeHTML(false)), so the wire carries the canonical
form of exactly what was checked; and writeEvent uses an encoder with
the same setting, since SSE data is not HTML and room.js puts every
string it parses on the page with textContent. What the escaping never
bought — and what a line-oriented wire actually needs — is that a
payload cannot end its own frame, which JSON's own \n escape gives:
TestChatCannotForgeASecondSSEFrame is the gate on that.
Four tests, each seen failing first: exact-key refusals (mixed case,
shouted, duplicated below the raw cap, a second key beside on), the
canonical payload arriving byte-for-byte, one chat of 2048 `<` staying
inside its cap on the wire, and the frame-injection gate.
make ci and go test -race both green.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
3 files changed,
+322
−38
internal/meet/signal.go+134 −36internal/meet/stream.go+24 −2internal/meettest/chat_test.go+164 −0
diff --git a/internal/meet/signal.go b/internal/meet/signal.go| index 5e99a23..a4bc997 100644 |
| --- a/internal/meet/signal.go |
| +++ b/internal/meet/signal.go |
| @@ -25,65 +25,158 @@ const maxChatBytes = 2048 |
| // validateFanout checks the two untargeted payload shapes without |
| // keeping anything from them: chat is exactly {"text": string} with |
| -// 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. |
| +// 1..2048 bytes of valid UTF-8; share is exactly {"on": bool}. It |
| +// returns the canonical payload to forward, plus a status and an |
| +// outcome word for the refusal log, or 0 and the payload. |
| // |
| // 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. |
| +// just the decoded text's length: a payload of {"text":"<64 KB>", |
| +// "text":"x"} decodes to a harmless one-byte text while the raw bytes — |
| +// the huge first value included — are what the sender put on the wire; |
| +// a \uXXXX escape can likewise inflate the raw 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) { |
| +// replacement character on its way into a string field, so a decoded |
| +// *p.Text is always valid UTF-8 by the time it can be inspected — |
| +// checking it there would report clean on raw bytes containing |
| +// anything at all. Checking raw itself, before decoding, is what |
| +// judges what was actually sent. |
| +// |
| +// Then the keys, byte-exactly (Codex S4-1). encoding/json matches an |
| +// object's keys to struct fields case-insensitively, and |
| +// DisallowUnknownFields does not change that: {"text":"<2049 a>", |
| +// "Text":"x"} decoded to a one-byte text — inside the cap — while the |
| +// raw bytes carried both keys and every recipient reads the lowercase |
| +// one. exactKey walks the token stream and insists on one member, |
| +// spelled exactly, appearing exactly once; a map would have collapsed |
| +// the duplicate and said nothing. |
| +// |
| +// And then, last, the payload is re-encoded from the value that was |
| +// just validated (Codex S4-2) rather than forwarded raw. The bytes on |
| +// every recipient's stream are then the canonical form of what these |
| +// checks judged — an escape delivered decoded, an angle bracket |
| +// delivered as itself, the size on the wire the size that was |
| +// measured — and no spelling the sender chose can make the two |
| +// disagree. SetEscapeHTML(false) because SSE data is not HTML, and the |
| +// escaping is what turned 2048 literal `<` into 12 KB. |
| +func validateFanout(typ string, raw json.RawMessage) (json.RawMessage, int, string) { |
| if len(raw) > maxChatBytes+32 { |
| - return http.StatusRequestEntityTooLarge, "size" |
| + return nil, http.StatusRequestEntityTooLarge, "size" |
| } |
| if !utf8.Valid(raw) { |
| - return http.StatusBadRequest, "shape" |
| + return nil, http.StatusBadRequest, "shape" |
| } |
| - dec := json.NewDecoder(bytes.NewReader(raw)) |
| - dec.DisallowUnknownFields() |
| switch typ { |
| case hub.TypeChat: |
| + if !exactKey(raw, "text") { |
| + return nil, http.StatusBadRequest, "shape" |
| + } |
| var p struct { |
| Text *string `json:"text"` |
| } |
| - if err := dec.Decode(&p); err != nil || p.Text == nil || dec.More() { |
| - return http.StatusBadRequest, "shape" |
| + if err := json.Unmarshal(raw, &p); err != nil || p.Text == nil { |
| + return nil, http.StatusBadRequest, "shape" |
| } |
| if len(*p.Text) == 0 { |
| - return http.StatusBadRequest, "shape" |
| + return nil, http.StatusBadRequest, "shape" |
| } |
| if len(*p.Text) > maxChatBytes { |
| - return http.StatusRequestEntityTooLarge, "size" |
| + return nil, http.StatusRequestEntityTooLarge, "size" |
| + } |
| + canon, ok := canonicalJSON(struct { |
| + Text string `json:"text"` |
| + }{Text: *p.Text}) |
| + if !ok { |
| + return nil, http.StatusBadRequest, "shape" |
| } |
| + return canon, 0, "" |
| case hub.TypeShare: |
| + if !exactKey(raw, "on") { |
| + return nil, http.StatusBadRequest, "shape" |
| + } |
| var p struct { |
| On *bool `json:"on"` |
| } |
| - if err := dec.Decode(&p); err != nil || p.On == nil || dec.More() { |
| - return http.StatusBadRequest, "shape" |
| + if err := json.Unmarshal(raw, &p); err != nil || p.On == nil { |
| + return nil, http.StatusBadRequest, "shape" |
| + } |
| + canon, ok := canonicalJSON(struct { |
| + On bool `json:"on"` |
| + }{On: *p.On}) |
| + if !ok { |
| + return nil, http.StatusBadRequest, "shape" |
| + } |
| + return canon, 0, "" |
| + } |
| + // Not a fan-out type at all: the dispatch below never calls this |
| + // with one, and the arm that does calls it is the only caller. |
| + return raw, 0, "" |
| +} |
| + |
| +// exactKey reports whether raw is a JSON object with exactly one |
| +// member, whose name is byte-for-byte want and appears exactly once, |
| +// and nothing after the object's closing brace. |
| +// |
| +// It walks the token stream rather than unmarshalling into a |
| +// map[string]json.RawMessage on purpose: a map collapses a duplicated |
| +// key to its last value and cannot tell {"text":"a","text":"b"} from |
| +// {"text":"b"}, and the first of those is a payload whose raw bytes say |
| +// one thing and whose decoded value says another. The walk counts every |
| +// member it passes, so it can refuse both. |
| +func exactKey(raw json.RawMessage, want string) bool { |
| + dec := json.NewDecoder(bytes.NewReader(raw)) |
| + open, err := dec.Token() |
| + if err != nil { |
| + return false |
| + } |
| + if d, ok := open.(json.Delim); !ok || d != '{' { |
| + return false |
| + } |
| + members := 0 |
| + for dec.More() { |
| + key, err := dec.Token() |
| + if err != nil { |
| + return false |
| + } |
| + name, ok := key.(string) |
| + if !ok || name != want { |
| + return false |
| } |
| + members++ |
| + if members > 1 { |
| + return false |
| + } |
| + // The value, whatever shape it takes: read past it so the next |
| + // token is the next key or the closing brace. Its type is the |
| + // business of the decode above, not of this walk. |
| + var value json.RawMessage |
| + if err := dec.Decode(&value); err != nil { |
| + return false |
| + } |
| + } |
| + // The closing brace, and then nothing: a second value after the |
| + // object is a payload with something else in it. |
| + if _, err := dec.Token(); err != nil { |
| + return false |
| + } |
| + return members == 1 && !dec.More() |
| +} |
| + |
| +// canonicalJSON encodes one validated value the way it goes out on the |
| +// wire: no HTML escaping (SSE data is not HTML) and no trailing newline |
| +// from the Encoder's own line framing. |
| +func canonicalJSON(v any) (json.RawMessage, bool) { |
| + var buf bytes.Buffer |
| + enc := json.NewEncoder(&buf) |
| + enc.SetEscapeHTML(false) |
| + if err := enc.Encode(v); err != nil { |
| + return nil, false |
| } |
| - return 0, "" |
| + return json.RawMessage(bytes.TrimRight(buf.Bytes(), "\n")), true |
| } |
| // fanoutError names the JSON error the two ways validateFanout can |
| @@ -246,11 +339,16 @@ func (a *app) signal(w http.ResponseWriter, r *http.Request) { |
| a.refuseSignal(w, sqid, body.Peer, body.Type, http.StatusBadRequest, "shape", map[string]any{"error": "bad-request"}) |
| return |
| } |
| - if status, outcome := validateFanout(body.Type, body.Payload); status != 0 { |
| + // The canonical payload, not the request's own bytes: what goes |
| + // out is a re-encoding of exactly what was validated (Codex |
| + // S4-1, S4-2), so the size and the shape on every recipient's |
| + // stream are the size and the shape these checks judged. |
| + payload, status, outcome := validateFanout(body.Type, body.Payload) |
| + if status != 0 { |
| a.refuseSignal(w, sqid, body.Peer, body.Type, status, outcome, map[string]any{"error": fanoutError(status)}) |
| return |
| } |
| - herr = a.hub.Fanout(r.Context(), sqid, session, body.Peer, hub.Message{Type: body.Type, Payload: body.Payload}) |
| + herr = a.hub.Fanout(r.Context(), sqid, session, body.Peer, hub.Message{Type: body.Type, Payload: payload}) |
| default: |
| a.refuseSignal(w, sqid, body.Peer, body.Type, http.StatusBadRequest, "type", map[string]any{"error": "bad-request"}) |
| return |
diff --git a/internal/meet/stream.go b/internal/meet/stream.go| index d9d561d..0ba6fe7 100644 |
| --- a/internal/meet/stream.go |
| +++ b/internal/meet/stream.go |
| @@ -1,6 +1,7 @@ |
| package meet |
| import ( |
| + "bytes" |
| "crypto/sha256" |
| "encoding/hex" |
| "encoding/json" |
| @@ -350,11 +351,32 @@ func (a *app) serveStream(w http.ResponseWriter, r *http.Request, sub *hub.Sub, |
| } |
| } |
| +// writeEvent puts one message on the wire as an SSE frame. |
| +// |
| +// The encoder, rather than json.Marshal, for one reason: Marshal |
| +// HTML-escapes `<`, `>` and `&` into <, > and &, and it |
| +// does so inside a json.RawMessage too. A chat of 2048 literal `<` — |
| +// validated at 2059 raw bytes, inside §4.4's cap — left this process |
| +// as 12,299 payload bytes, six times what was measured (Codex S4-2). |
| +// SSE data is not HTML and no reader of this stream treats it as |
| +// markup: room.js parses the JSON and puts every string it contains on |
| +// the page with textContent. Nothing here depends on the escaping. |
| +// |
| +// What the escaping never protected against, and what actually matters |
| +// on a line-oriented wire, is a payload ending its own frame: JSON |
| +// escapes a newline inside a string as \n rather than emitting one, so |
| +// no value can produce the blank line that dispatches an event. |
| +// meettest's TestChatCannotForgeASecondSSEFrame is the gate on that. |
| func writeEvent(w http.ResponseWriter, rc *http.ResponseController, m hub.Message) bool { |
| - b, err := json.Marshal(m) |
| - if err != nil { |
| + var buf bytes.Buffer |
| + enc := json.NewEncoder(&buf) |
| + enc.SetEscapeHTML(false) |
| + if err := enc.Encode(m); err != nil { |
| return false |
| } |
| + // Encode writes its own trailing newline; the frame below supplies |
| + // the two this wire format needs. |
| + b := bytes.TrimRight(buf.Bytes(), "\n") |
| return writeRaw(w, rc, fmt.Sprintf("event: %s\ndata: %s\n\n", m.Type, b)) |
| } |
diff --git a/internal/meettest/chat_test.go b/internal/meettest/chat_test.go| index f477448..e7f2011 100644 |
| --- a/internal/meettest/chat_test.go |
| +++ b/internal/meettest/chat_test.go |
| @@ -255,3 +255,167 @@ func TestChatRawPayloadSizeAndUTF8(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// chatCap is spec §4.4's 2 KB, retyped here because meettest is an |
| +// external test package and cannot read meet's own constant. The tests |
| +// below fail loudly if the two ever disagree: 2048 + 32 is the raw cap |
| +// they post either side of. |
| +const chatCap = 2048 |
| + |
| +// chatPair opens a meeting with two admitted members and hands back the |
| +// sender (browser, room, peer) and the other member's stream, which is |
| +// where every assertion about what actually went out on the wire is |
| +// made. The four tests below need exactly this and nothing else from |
| +// the dance that builds it. |
| +func chatPair(t *testing.T) (*client, string, string, *sseReader) { |
| + t.Helper() |
| + x, sqid, _ := memberWithMeeting(t) |
| + a := x.browser(t) |
| + a.signIn("paul@example.com") |
| + sa := a.stream("/m/" + sqid + "/stream") |
| + t.Cleanup(sa.close) |
| + pa, _ := sa.hello() |
| + a.signal(sqid, pa, `{"type":"join"}`) |
| + sa.nextOf("roster", 2*time.Second) |
| + |
| + b := newMemberBrowser(t, x, "paul@rslw.com") |
| + sb := b.stream("/m/" + sqid + "/stream") |
| + t.Cleanup(sb.close) |
| + pb, _ := sb.hello() |
| + b.signal(sqid, pb, `{"type":"join"}`) |
| + sb.nextOf("roster", 2*time.Second) |
| + sa.nextOf("peer", 2*time.Second) |
| + return a, sqid, pa, sb |
| +} |
| + |
| +// Codex S4-1: encoding/json matches an object's keys to struct fields |
| +// case-insensitively, and DisallowUnknownFields does not change that. |
| +// {"text":"<2049 a>","Text":"x"} therefore validated the *last* match — |
| +// a one-byte text — while the raw bytes handed to Fanout carried both |
| +// keys, and every recipient reads the lowercase one: 2049 bytes |
| +// delivered through a 2048-byte cap, inside a raw payload small enough |
| +// to clear the +32 guard. So the keys are judged byte-exactly and |
| +// counted now, before any value is looked at. |
| +func TestChatKeysAreExactCaseSensitiveAndUnique(t *testing.T) { |
| + a, sqid, pa, _ := chatPair(t) |
| + |
| + cases := []struct { |
| + name string |
| + typ string |
| + payload string |
| + }{ |
| + {"a mixed-case second key smuggles the value past the cap", "chat", |
| + `{"text":"` + strings.Repeat("a", chatCap+1) + `","Text":"x"}`}, |
| + {"a capitalised key alone is not text", "chat", `{"Text":"hello"}`}, |
| + {"a shouted key alone is not text", "chat", `{"TEXT":"hello"}`}, |
| + {"a duplicated key below the raw cap", "chat", |
| + `{"text":"` + strings.Repeat("a", 64) + `","text":"x"}`}, |
| + {"a capitalised on is not on", "share", `{"On":true}`}, |
| + {"a duplicated on below the raw cap", "share", `{"on":true,"on":false}`}, |
| + {"a second key beside on", "share", `{"on":true,"off":false}`}, |
| + {"an empty payload object", "chat", `{}`}, |
| + {"a payload that is not an object", "chat", `"text"`}, |
| + {"trailing content after the object", "chat", `{"text":"x"} {}`}, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + body := `{"peer":"` + pa + `","type":"` + c.typ + `","payload":` + c.payload + `}` |
| + res, respBody := a.signalRaw(sqid, body) |
| + if res.StatusCode != http.StatusBadRequest || !strings.Contains(string(respBody), `"error":"bad-request"`) { |
| + t.Fatalf("%s: %d %s, want 400 bad-request", c.name, res.StatusCode, respBody) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// Codex S4-2, and the other half of S4-1: what reaches a recipient is |
| +// the *validated* payload, re-encoded from the value that was checked, |
| +// and never whatever byte sequence the sender chose to spell it with. |
| +// So an escape is delivered decoded, an angle bracket is delivered as |
| +// itself (writeEvent no longer HTML-escapes — SSE data is not HTML), |
| +// and the payload on the wire is byte-for-byte the canonical form. |
| +func TestChatDeliversTheCanonicalValidatedPayload(t *testing.T) { |
| + a, sqid, pa, sb := chatPair(t) |
| + |
| + cases := []struct { |
| + name string |
| + sent string |
| + want string |
| + }{ |
| + {"angle brackets and ampersands go out as themselves", |
| + `{"text":"a<b>c&d"}`, `"payload":{"text":"a<b>c&d"}`}, |
| + {"an escape is delivered decoded, not re-escaped", |
| + `{"text":"A<"}`, `"payload":{"text":"A<"}`}, |
| + {"an ordinary line is untouched", |
| + `{"text":"plain"}`, `"payload":{"text":"plain"}`}, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + body := `{"peer":"` + pa + `","type":"chat","payload":` + c.sent + `}` |
| + if res, respBody := a.signalRaw(sqid, body); res.StatusCode != http.StatusOK { |
| + t.Fatalf("%s: %d %s", c.name, res.StatusCode, respBody) |
| + } |
| + ev, data, ok := sb.nextOf("chat", 2*time.Second) |
| + if !ok || ev != "chat" { |
| + t.Fatalf("%s: no chat arrived (%q)", c.name, ev) |
| + } |
| + if !strings.Contains(string(data), c.want) { |
| + t.Fatalf("%s: wire %s, want it to carry %s", c.name, data, c.want) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// Codex S4-2: json.Marshal HTML-escapes `<`, `>` and `&` even inside a |
| +// json.RawMessage, so a text of 2048 literal `<` — 2059 raw bytes, |
| +// comfortably inside the cap both checks enforce — left this process as |
| +// 12,299 payload bytes, six times what was validated. The cap is a |
| +// promise about what the fan-out puts on every other peer's stream, so |
| +// the fan-out is where it has to hold. |
| +func TestChatDoesNotExpandOnTheWire(t *testing.T) { |
| + a, sqid, pa, sb := chatPair(t) |
| + |
| + text := strings.Repeat("<", chatCap) |
| + body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"` + text + `"}}` |
| + if res, respBody := a.signalRaw(sqid, body); res.StatusCode != http.StatusOK { |
| + t.Fatalf("2048 angle brackets: %d %s", res.StatusCode, respBody) |
| + } |
| + ev, data, ok := sb.nextOf("chat", 2*time.Second) |
| + if !ok || ev != "chat" { |
| + t.Fatalf("no chat arrived (%q)", ev) |
| + } |
| + if want := `{"text":"` + text + `"}`; !strings.Contains(string(data), want) { |
| + t.Fatalf("the payload on the wire is not the 2059 bytes that were validated (%d bytes of event data)", len(data)) |
| + } |
| + // The envelope's own fields — type, from, name — are the only thing |
| + // beyond the payload, and all three are bounded: a peer id, a |
| + // display name, and the field names around them. |
| + if len(data) > chatCap+32+256 { |
| + t.Fatalf("one chat event is %d bytes on the wire for a %d-byte text", len(data), len(text)) |
| + } |
| +} |
| + |
| +// The wire is line-oriented and the payload is a person's own text, so |
| +// the one thing a chat must never be able to do is end its own frame |
| +// and start another. It cannot: JSON escapes a newline inside a string |
| +// rather than emitting one, and that stays true of the canonical |
| +// re-encoding. This is the test that says so, and it is why disabling |
| +// HTML escaping on the way out is safe. |
| +func TestChatCannotForgeASecondSSEFrame(t *testing.T) { |
| + a, sqid, pa, sb := chatPair(t) |
| + |
| + body := `{"peer":"` + pa + `","type":"chat","payload":{"text":"one\n\nevent: roster\ndata: {\"peers\":[]}\n\ntwo"}}` |
| + if res, respBody := a.signalRaw(sqid, body); res.StatusCode != http.StatusOK { |
| + t.Fatalf("newlines in text: %d %s", res.StatusCode, respBody) |
| + } |
| + ev, data, ok := sb.nextOf("chat", 2*time.Second) |
| + if !ok || ev != "chat" { |
| + t.Fatalf("no chat arrived (%q)", ev) |
| + } |
| + if !strings.Contains(string(data), `\n\nevent: roster`) { |
| + t.Fatalf("the newlines did not survive as escapes: %s", data) |
| + } |
| + if ev, data, ok := sb.next(300 * time.Millisecond); ok { |
| + t.Fatalf("a second event came out of one chat: %q %s", ev, data) |
| + } |
| +} |