oficina / meet Public

ice: fix round 1 - LiveForSession gate, header clamp, ended check

Task 3 review, fix round 1 (12 items):

- /ice now spends a store read and the rate limiter only after
  LiveForSession confirms this session holds a live participation in
  the room (signal.go's own gate, item 1) — 404 identical to /signal's,
  then a 429 with Retry-After sharing /signal's own bucket on a burst.
- The development TTL header is now clamped both ways:
  life = min(max(d, minIceLife), iceLife), so a zero, negative or
  10-hour header can no longer skip the floor or blow past the
  platform's own three-hour default (item 3).
- The ended-meeting check is proven independent of the hub's own
  admitted state: a new test ends the meeting directly in the store,
  never through the real handler (which also calls hub.End and erases
  the hub's own record of the room), so the hub still believes the
  guest is live right up to the request (item 4).
- Every refusal now logs at Warn, like /signal's own logRefusal, and
  the unknown/ended arm now logs outcome=meeting (item 7).
- hub.Authorize's doc comment now says what a nil return actually
  allows: setKindLocked can still run and broadcast a fresh roster on
  a real kind change; it is not a pure read (item 7).
- The guest half of the standing-revocation hub test now counts
  StandingFor calls for the guest's personID, so removing
  memberStandingGone's early-return exemption alone (leaving
  standingLostLocked's own admitted-guest guard standing) is caught —
  the prior version's outcome-only assertion was not discriminating
  against that specific mutation (item 8).
- newHarnessWithEnv is gone; newHarnessWithLogger takes env directly,
  and every existing caller (signal_test.go, ice_test.go) now passes
  meet.EnvDevelopment explicitly (item 7).
- The 404-equality test now also captures a real POST /signal 404 and
  a malformed (not merely unknown) sqid into the compared set (item 6).
- The logging test now asserts the log buffer's length is unchanged
  across a 200, and that neither the exact minted username nor
  credential appears anywhere in the log after a foreign session's
  authority refusal (item 2). A new test sends a long, newline-bearing
  peer and asserts none of it reaches the log (item 5).
- The endpoint's own wire shape is now pinned (required keys present,
  not just no extras) alongside relay's own golden test (item 7).

Task 1/2 re-review pins, carried in this same round:

- TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey now keeps
  build()'s *app return and asserts relayConfigured()/relayRegions(),
  not only the log line (item 9).
- New TestRegionStringRedactsTheKey pins %v/%+v/%s of a Region (item 10).
- The ICEServer golden-JSON test now asserts the required keys are
  present on each entry, not only that no extras appear (item 11).
- Parse gets two new refusal cases for unpadded base64 (outer value and
  key); the already-inert JSON-syntax-error NEEDLE case is annotated
  rather than removed (item 12, optional).

Every new or changed assertion was verified against the code it now
guards: each was run against a temporarily reverted (or bug-injected)
version of ice.go / admission.go / app.go and confirmed to fail there,
then restored and confirmed 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 1 month ago b5c29b30b8b5009a83320b6a814feb802e680702 parent b55403e

8 files changed, +446 −68

internal/meet/app_relay_test.go +8 −1
internal/meet/hub/authorize.go +12 −3
internal/meet/hub/authorize_test.go +14 −2
internal/meet/ice.go +42 −6
internal/meet/relay/relay_test.go +50 −0
internal/meettest/harness_test.go +18 −6
internal/meettest/ice_test.go +296 −46
internal/meettest/signal_test.go +6 −4

CI passed — run details

diff --git a/internal/meet/app_relay_test.go b/internal/meet/app_relay_test.go

index 12efd9d..f31b167 100644

--- a/internal/meet/app_relay_test.go

+++ b/internal/meet/app_relay_test.go

@@ -60,9 +60,16 @@ func TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey(t *testing.T) {

cfg.Logger = logger

cfg.Relay = relayVector(t)

d := openRelayTestDB(t, logger)

- if _, _, err := build(context.Background(), d, cfg); err != nil {

+ _, a, err := build(context.Background(), d, cfg)

+ if err != nil {

t.Fatal(err)

}

+ // The INFO line alone does not prove build() ever stored the parsed

+ // contract into the app's own atomic — a log call that ran and a

+ // Store call that didn't would still print this line. Assert both.

+ if !a.relayConfigured() || len(a.relayRegions()) != 1 {

+ t.Fatalf("build did not store the parsed relay: configured=%v regions=%d", a.relayConfigured(), len(a.relayRegions()))

+ }

log := buf.String()

if !strings.Contains(log, "meet: relay configured") || !strings.Contains(log, "regions=1") {

t.Fatalf("want one INFO line naming the region count, got:\n%s", log)

diff --git a/internal/meet/hub/authorize.go b/internal/meet/hub/authorize.go

index fafec08..9e5dbe8 100644

--- a/internal/meet/hub/authorize.go

+++ b/internal/meet/hub/authorize.go

@@ -5,9 +5,18 @@ import "context"

// Authorize is the §2.5 last-row check on its own: a participation

// bound to this session, in this room, admitted. A member-kind

// participation re-derives standing first (§15.12), exactly as Relay

-// does, and one whose standing is gone is ended here with lost{standing}

-// and answered ErrNotYours; a guest never consults standing (ruling 14).

-// GET /m/{sqid}/ice is its caller; it changes nothing on success.

+// does, through memberStandingGone; one whose standing is gone is ended

+// here with lost{standing} and answered ErrNotYours. A guest never

+// consults standing at all (ruling 14): memberStandingGone returns

+// immediately for a non-member kind, without a StandingFor call.

+//

+// A nil return is not "nothing happened": memberStandingGone still

+// records a kind that changed (setKindLocked), which — for an admitted

+// member — broadcasts a fresh roster to the room, the same side effect

+// Relay's own re-derivation has. What a nil return promises is only

+// that no *participation* ended and nothing was relayed; it is not a

+// pure read the way CanResume's probe is (that one never writes at

+// all). GET /m/{sqid}/ice is its caller.

func (h *Hub) Authorize(ctx context.Context, sqid, session string, peer PeerID) error {

gone, err := h.memberStandingGone(ctx, sqid, session, peer, true)

if err != nil {

diff --git a/internal/meet/hub/authorize_test.go b/internal/meet/hub/authorize_test.go

index d3bfd86..0c3062d 100644

--- a/internal/meet/hub/authorize_test.go

+++ b/internal/meet/hub/authorize_test.go

@@ -48,15 +48,23 @@ func TestAuthorizeIsTheAdmittedParticipationRule(t *testing.T) {

// only thing that could have refused the member is Authorize's own

// re-derivation. Bob is admitted as a guest (personID 100, testHub's

// own guest threshold) before his grant "expires" — ruling 14 says

-// Authorize never even asks for a guest.

+// Authorize never even *asks* for a guest, which is asserted directly

+// by counting StandingFor calls: standingLostLocked's own admitted-

+// guest guard would still make the *outcome* (nil, participation kept)

+// look right even if memberStandingGone's early return for a

+// non-member kind were deleted and it called StandingFor for Bob

+// anyway — so the outcome alone does not discriminate that mutation.

+// The call count does.

func TestAuthorizeDropsAMemberWhoseStandingIsGoneAndKeepsAnAdmittedGuest(t *testing.T) {

now := time.Date(2026, 9, 10, 9, 0, 0, 0, time.UTC)

standing := map[int64]bool{1: true, 100: true}

+ guestStandingCalls := 0

h := New(Config{

Now: func() time.Time { return now },

Logger: slog.New(slog.NewTextHandler(io.Discard, nil)),

StandingFor: func(ctx context.Context, room string, id int64) (Kind, bool, error) {

if id == 100 {

+ guestStandingCalls++

return KindGuest, standing[id], nil

}

return KindMember, standing[id], nil

@@ -89,8 +97,12 @@ func TestAuthorizeDropsAMemberWhoseStandingIsGoneAndKeepsAnAdmittedGuest(t *test

t.Fatalf("Bob's stream got %v, want gone naming Ann last", types(bev))

}

- standing[100] = false // Bob's grant has "expired"; Authorize never asks for a guest

+ standing[100] = false // Bob's grant has "expired"

+ before := guestStandingCalls

if err := h.Authorize(ctx, "r1", "sb", b.Peer); err != nil {

t.Fatalf("admitted guest with a lapsed grant: %v", err)

}

+ if guestStandingCalls != before {

+ t.Fatalf("Authorize asked StandingFor for a guest: %d calls during this Authorize, want 0", guestStandingCalls-before)

+ }

}

diff --git a/internal/meet/ice.go b/internal/meet/ice.go

index 4d62547..432abad 100644

--- a/internal/meet/ice.go

+++ b/internal/meet/ice.go

@@ -3,6 +3,7 @@ package meet

import (

"errors"

"net/http"

+ "strconv"

"time"

"github.com/go-chi/chi/v5"

@@ -43,7 +44,7 @@ func (a *app) ice(w http.ResponseWriter, r *http.Request) {

return

}

if !validSqid(sqid) {

- a.logger.Info("meet: ice refused", "room", "invalid", "peer", "", "outcome", "shape")

+ a.logger.Warn("meet: ice refused", "room", "invalid", "peer", "", "outcome", "shape")

streamJSON(w, http.StatusNotFound, a.notFoundBody())

return

}

@@ -52,13 +53,41 @@ func (a *app) ice(w http.ResponseWriter, r *http.Request) {

// peer is never logged here, exactly like signal.go's refusal:

// it has not been validated as bounded, printable text (review

// Important 2's rule, carried over).

- a.logger.Info("meet: ice refused", "room", sqid, "peer", "", "outcome", "shape")

+ a.logger.Warn("meet: ice refused", "room", sqid, "peer", "", "outcome", "shape")

streamJSON(w, http.StatusNotFound, a.notFoundBody())

return

}

+ // A session with no live participation in this exact room is

+ // refused the identical not-found answer before the store or the

+ // limiter is ever touched — signal.go's own LiveForSession gate

+ // (review Task 5 item 1), carried over here: nothing is spent and

+ // nothing is read for a room this session holds nothing in.

+ if _, live := a.hub.LiveForSession(sqid, session); !live {

+ a.logger.Warn("meet: ice refused", "room", sqid, "peer", "", "outcome", "unbound")

+ streamJSON(w, http.StatusNotFound, a.notFoundBody())

+ return

+ }

+ // Rate limiting shares /signal's own bucket: session+room, never

+ // anything the request claims, so a burst against either endpoint

+ // from one session spends the one budget it can ever spend.

+ if allowed, retry := a.signalLimiter.allow(session+"|"+sqid, a.store.now()); !allowed {

+ w.Header().Set("Retry-After", strconv.Itoa(int(retry.Seconds())+1))

+ a.logger.Warn("meet: ice refused", "room", sqid, "peer", peer, "outcome", "rate")

+ streamJSON(w, http.StatusTooManyRequests, map[string]any{"error": "rate"})

+ return

+ }

// The meeting itself must be live: a fresh process holds no hub

// state for a room nobody has opened, so ended_at is the only

- // authority it has — stream.go's own fresh-open precedent.

+ // authority it has — stream.go's own fresh-open precedent. This is

+ // checked independently of the hub's own admitted state: the hub

+ // may still think a participation here is live and admitted (ruling

+ // 14 keeps an admitted guest through a lapsed grant, and nothing

+ // tells the hub a meeting ended except the End call an ending

+ // request makes), so this store read is what a still-admitted

+ // participation is refused through once the meeting is over.

m, found, err := a.store.meetingBySqid(r.Context(), sqid)

if err != nil {

a.logger.Error("meet: ice: load meeting", "err", err)

@@ -66,13 +95,15 @@ func (a *app) ice(w http.ResponseWriter, r *http.Request) {

return

}

if !found || m.EndedAt != nil {

+ a.logger.Warn("meet: ice refused", "room", sqid, "peer", peer, "outcome", "meeting")

streamJSON(w, http.StatusNotFound, a.notFoundBody())

return

}

switch err := a.hub.Authorize(r.Context(), sqid, session, hub.PeerID(peer)); {

case err == nil:

case errors.Is(err, hub.ErrUnknownPeer), errors.Is(err, hub.ErrNotAdmitted), errors.Is(err, hub.ErrNotYours):

- a.logger.Info("meet: ice refused", "room", sqid, "peer", peer, "outcome", "authority")

+ a.logger.Warn("meet: ice refused", "room", sqid, "peer", peer, "outcome", "authority")

streamJSON(w, http.StatusNotFound, a.notFoundBody())

return

default:

@@ -84,8 +115,13 @@ func (a *app) ice(w http.ResponseWriter, r *http.Request) {

now := a.store.now()

life, renew := iceLife, iceRenewAfter

if a.cfg.development() {

- if d, err := time.ParseDuration(r.Header.Get(testIceHeader)); err == nil && d > 0 {

- life = max(d, minIceLife)

+ if d, err := time.ParseDuration(r.Header.Get(testIceHeader)); err == nil {

+ // Clamped both ways: a header cannot hand time.NewTimer a

+ // zero or a negative any more than stream.go's own

+ // testTTLHeader can (the floor), and cannot mint credentials

+ // that outlive the platform's own four-hour ceiling either

+ // (the cap, at this handler's three-hour default).

+ life = min(max(d, minIceLife), iceLife)

renew = life * 5 / 6

}

diff --git a/internal/meet/relay/relay_test.go b/internal/meet/relay/relay_test.go

index d18a306..d62e28a 100644

--- a/internal/meet/relay/relay_test.go

+++ b/internal/meet/relay/relay_test.go

@@ -1,8 +1,10 @@

package relay

import (

+ "bytes"

"encoding/base64"

"encoding/json"

+ "fmt"

"reflect"

"strings"

"testing"

@@ -110,6 +112,13 @@ func TestParseRefusesWithoutEchoingTheValue(t *testing.T) {

"not json": {base64.StdEncoding.EncodeToString([]byte("nope")), nil},

"not an array": {base64.StdEncoding.EncodeToString([]byte(`{"region":"x"}`)), nil},

"json syntax error": {

+ // Inert by construction, not merely by the fix: a JSON syntax

+ // error from encoding/json ("invalid character '}' looking

+ // for beginning of object key string") never echoes the

+ // input it failed on, with or without this package's own

+ // wrapping. Kept — and its planted needle kept alongside it

+ // — as a tripwire in case that ever stops being true of

+ // encoding/json's own error text.

base64.StdEncoding.EncodeToString([]byte(`[{"region":"NEEDLE-SHOULD-NOT-LEAK",}]`)),

[]string{"NEEDLE-SHOULD-NOT-LEAK"},

@@ -176,6 +185,19 @@ func TestParseRefusesWithoutEchoingTheValue(t *testing.T) {

encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": vectorKey}}),

[]string{vectorKey},

+ // base64.StdEncoding is strict about padding; encoding/base64's

+ // URL- or raw-encoding variants are not what CARLOS_RELAY nor a

+ // region's key are documented to use, so an unpadded value of

+ // either must be refused rather than silently accepted under a

+ // more permissive decoder.

+ "outer value is not padded": {

+ strings.TrimRight(encode(t, oneRegion()), "="),

+ nil,

+ },

+ "key is not padded": {

+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": strings.TrimRight(vectorKey, "="), "username_suffix": "abc:meet:0123456789abcdef:k1"}}),

+ nil,

+ },

}

for name, tc := range cases {

t.Run(name, func(t *testing.T) {

@@ -195,6 +217,26 @@ func TestParseRefusesWithoutEchoingTheValue(t *testing.T) {

}

+// TestRegionStringRedactsTheKey pins Region's fmt.Stringer against the

+// three verbs a stray Printf/Errorf/log call is most likely to use:

+// %v and %+v both route a struct through String() when it implements

+// fmt.Stringer, and %s does too. None may ever print the key bytes.

+func TestRegionStringRedactsTheKey(t *testing.T) {

+ regions, err := Parse(encode(t, oneRegion()))

+ if err != nil {

+ t.Fatal(err)

+ }

+ r := regions[0]

+ out := fmt.Sprintf("%v %+v %s", r, r, r)

+ if !strings.Contains(out, "redacted") {

+ t.Fatalf("Region's format verbs do not redact the key: %s", out)

+ }

+ key, _ := base64.StdEncoding.DecodeString(vectorKey)

+ if strings.Contains(out, vectorKey) || bytes.Contains([]byte(out), key) {

+ t.Fatalf("Region's format verbs leaked the key: %s", out)

+ }

func TestMintCarriesCredentialsOnTurnOnlyAndPassesURLsThrough(t *testing.T) {

regions, err := Parse(encode(t, oneRegion()))

if err != nil {

@@ -318,11 +360,19 @@ func TestICEServerJSONShapeIsExactlyTheWireContract(t *testing.T) {

if len(raw) != 2 {

t.Fatalf("want a bare entry and a turn entry, got %d", len(raw))

}

+ if _, ok := raw[0]["urls"]; !ok {

+ t.Fatal("bare entry is missing urls")

+ }

for k := range raw[0] {

if k != "urls" {

t.Fatalf("bare entry carries an extra key %q", k)

}

+ for _, want := range []string{"urls", "username", "credential"} {

+ if _, ok := raw[1][want]; !ok {

+ t.Fatalf("turn entry is missing %q", want)

+ }

for k := range raw[1] {

if k != "urls" && k != "username" && k != "credential" {

t.Fatalf("turn entry carries an extra key %q", k)

diff --git a/internal/meettest/harness_test.go b/internal/meettest/harness_test.go

index 3d7fd87..e24a099 100644

--- a/internal/meettest/harness_test.go

+++ b/internal/meettest/harness_test.go

@@ -191,14 +191,21 @@ func (m *recordingMailer) count() int {

func newHarness(t *testing.T, members ...string) *harness {

t.Helper()

- return newHarnessWithLogger(t, slog.New(slog.NewTextHandler(io.Discard, nil)), members...)

+ return newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(io.Discard, nil)), members...)

}

// newHarnessWithLogger is newHarness with the caller's own logger wired

// through both the app and its database, instead of the usual

// io.Discard one — used to capture and assert on log lines (spec §0.1,

-// §4.4: no payload, address, title or token is ever a log argument).

-func newHarnessWithLogger(t *testing.T, logger *slog.Logger, members ...string) *harness {

+// §4.4: no payload, address, title or token is ever a log argument) —

+// and the environment as its own parameter, so a test can build the

+// production harness that proves a development-only behaviour is inert

+// outside development. Production needs the full set of startup

+// secrets TestProductionRefusesToStartWithoutSecrets exercises —

+// Origin, InstanceKey, DEK, Mailer and a non-empty allowlist — so this

+// fills those in whenever env is not development, rather than merely

+// swapping Env on the development set.

+func newHarnessWithLogger(t *testing.T, env string, logger *slog.Logger, members ...string) *harness {

t.Helper()

dbPath := filepath.Join(t.TempDir(), "app.db")

d, err := db.Open(dbPath, logger)

@@ -207,9 +214,14 @@ func newHarnessWithLogger(t *testing.T, logger *slog.Logger, members ...string)

}

t.Cleanup(func() { d.Close() })

rec := &recordingMailer{}

- mux, ctl, err := meet.NewForTest(d, meet.Config{

- Env: meet.EnvDevelopment, Origin: testOrigin, AllowedAddresses: members, Mailer: rec, Logger: logger,

- })

+ cfg := meet.Config{

+ Env: env, Origin: testOrigin, AllowedAddresses: members, Mailer: rec, Logger: logger,

+ }

+ if env != meet.EnvDevelopment {

+ cfg.InstanceKey = "test-instance-key-not-a-real-secret"

+ cfg.DEK = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

+ }

+ mux, ctl, err := meet.NewForTest(d, cfg)

if err != nil {

t.Fatal(err)

}

diff --git a/internal/meettest/ice_test.go b/internal/meettest/ice_test.go

index 586f4cb..2ca2559 100644

--- a/internal/meettest/ice_test.go

+++ b/internal/meettest/ice_test.go

@@ -9,13 +9,11 @@ import (

"log/slog"

"net/http"

"net/http/httptest"

- "path/filepath"

+ "net/url"

"strings"

"testing"

"time"

- "amadan.net/rastrillo/rastrillo/db"

meet "amadan.net/oficina/meet/internal/meet"

"amadan.net/oficina/meet/internal/meet/relay"

)

@@ -56,39 +54,6 @@ func guestJoinedAndWaiting(t *testing.T, x *harness, sqid, tok string) (*client,

return g, peer

}

-// newHarnessWithEnv is newHarnessWithLogger with the environment as a

-// parameter, so a test can build the production harness that proves

-// the development-only ICE TTL header is inert outside development.

-// Production needs the full set of startup secrets

-// TestProductionRefusesToStartWithoutSecrets exercises — Origin,

-// InstanceKey, DEK, Mailer and a non-empty allowlist — so this fills

-// those in rather than merely swapping Env.

-func newHarnessWithEnv(t *testing.T, env string, members ...string) *harness {

- t.Helper()

- logger := slog.New(slog.NewTextHandler(io.Discard, nil))

- dbPath := filepath.Join(t.TempDir(), "app.db")

- d, err := db.Open(dbPath, logger)

- if err != nil {

- t.Fatal(err)

- }

- t.Cleanup(func() { d.Close() })

- rec := &recordingMailer{}

- cfg := meet.Config{

- Env: env, Origin: testOrigin, AllowedAddresses: members, Mailer: rec, Logger: logger,

- }

- if env != meet.EnvDevelopment {

- cfg.InstanceKey = "test-instance-key-not-a-real-secret"

- cfg.DEK = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="

- }

- mux, ctl, err := meet.NewForTest(d, cfg)

- if err != nil {

- t.Fatal(err)

- }

- x := &harness{h: mux, db: d, dbPath: dbPath, mail: rec, ctl: ctl, now: time.Now().UTC(), t: t}

- x.advance(0)

- return x

func TestIceHandsAnAdmittedParticipationCredentialsThatVerify(t *testing.T) {

x, sqid, _ := memberWithMeeting(t)

if err := x.ctl.SetRelay(relayContract(t)); err != nil {

@@ -131,6 +96,36 @@ func TestIceHandsAnAdmittedParticipationCredentialsThatVerify(t *testing.T) {

if want := fmt.Sprintf("%d:abc:meet:0123456789abcdef:k1", ans.ExpiresAt); turn.Username != want {

t.Fatalf("username %q, want %q", turn.Username, want)

}

+ // The wire shape itself, not just the typed struct's own tags:

+ // relay's own golden test pins Mint's output in isolation, but

+ // nothing before this pinned what actually crosses this endpoint —

+ // a bare entry carries only urls, a turn entry carries urls,

+ // username and credential and nothing else.

+ var raw struct {

+ ICEServers []map[string]json.RawMessage `json:"iceServers"`

+ }

+ if err := json.Unmarshal(rec.Body.Bytes(), &raw); err != nil {

+ t.Fatal(err)

+ }

+ if len(raw.ICEServers) != 2 {

+ t.Fatalf("want two servers over the wire, got %d", len(raw.ICEServers))

+ }

+ for k := range raw.ICEServers[0] {

+ if k != "urls" {

+ t.Fatalf("bare entry over the wire carries an extra key %q", k)

+ }

+ for _, want := range []string{"urls", "username", "credential"} {

+ if _, ok := raw.ICEServers[1][want]; !ok {

+ t.Fatalf("turn entry over the wire is missing %q", want)

+ }

+ for k := range raw.ICEServers[1] {

+ if k != "urls" && k != "username" && k != "credential" {

+ t.Fatalf("turn entry over the wire carries an extra key %q", k)

+ }

}

func TestIceIs404ForAnythingButAnAdmittedParticipationOfThisSession(t *testing.T) {

@@ -155,15 +150,32 @@ func TestIceIs404ForAnythingButAnAdmittedParticipationOfThisSession(t *testing.T

t.Fatalf("another session must get the identical 404: %d %s vs %s", foreign.Code, foreign.Body.Bytes(), unjoined.Body.Bytes())

}

+ // A real /signal 404, from the same unbound session, must be the

+ // identical body too: both endpoints share one not-found shape

+ // (spec §2.5), not merely two handlers that happen to agree today.

+ signalResp, signalBody := other.signal(sqid, peer, `{"type":"leave"}`)

+ if signalResp.StatusCode != http.StatusNotFound || !bytes.Equal(signalBody, unjoined.Body.Bytes()) {

+ t.Fatalf("a real /signal 404 must be byte-identical: %d %s vs %s", signalResp.StatusCode, signalBody, unjoined.Body.Bytes())

+ }

bad := c.getStream("/m/" + sqid + "/ice?peer=zz")

if bad.Code != http.StatusNotFound || !bytes.Equal(bad.Body.Bytes(), unjoined.Body.Bytes()) {

t.Fatal("a malformed peer is the same 404")

}

+ // "2222222222" is validSqid's own shape (ten characters, every one

+ // in its alphabet) naming a room that does not exist; "0000000000"

+ // is not that shape at all — '0' is never in sqidAlphabet — so the

+ // two exercise different refusal arms (an unknown room vs a

+ // malformed one) and both must still be the one 404.

room := c.getStream("/m/2222222222/ice?peer=" + peer)

if room.Code != http.StatusNotFound || !bytes.Equal(room.Body.Bytes(), unjoined.Body.Bytes()) {

t.Fatal("an unknown room is the same 404")

}

+ malformedRoom := c.getStream("/m/0000000000/ice?peer=" + peer)

+ if malformedRoom.Code != http.StatusNotFound || !bytes.Equal(malformedRoom.Body.Bytes(), unjoined.Body.Bytes()) {

+ t.Fatal("a malformed sqid is the same 404")

+ }

// A waiting guest: sign a guest in through the link and join; 404.

g, gpeer := guestJoinedAndWaiting(t, x, sqid, tok)

@@ -236,7 +248,7 @@ func TestIceTestTTLHeaderIsHonouredOnlyInDevelopment(t *testing.T) {

})

t.Run("production", func(t *testing.T) {

- x := newHarnessWithEnv(t, "production", "paul@example.com")

+ x := newHarnessWithLogger(t, "production", slog.New(slog.NewTextHandler(io.Discard, nil)), "paul@example.com")

if err := x.ctl.SetRelay(relayContract(t)); err != nil {

t.Fatal(err)

}

@@ -265,12 +277,15 @@ func TestIceTestTTLHeaderIsHonouredOnlyInDevelopment(t *testing.T) {

// TestIceLogsNothingOnSuccessAndNoCredentialOnRefusal is

// signal_test.go's TestSignallingLogsCarryNoContent for this endpoint:

-// nothing at all is logged for a successful call (the answer carries a

-// credential), and a refusal's log line never carries the relay key,

-// the minted username or its credential.

+// a successful call grows the log buffer by not one byte (the answer

+// itself carries the credential, so even a line mentioning the room

+// alone would be a leash on it), and once a real credential has been

+// minted, neither its authority refusal's line, nor any other line

+// this test produces, may ever contain that exact minted username or

+// credential — not merely the vector key or a hardcoded substring.

func TestIceLogsNothingOnSuccessAndNoCredentialOnRefusal(t *testing.T) {

var buf bytes.Buffer

- x := newHarnessWithLogger(t, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com", "paul@rslw.com")

if err := x.ctl.SetRelay(relayContract(t)); err != nil {

t.Fatal(err)

}

@@ -285,13 +300,31 @@ func TestIceLogsNothingOnSuccessAndNoCredentialOnRefusal(t *testing.T) {

}

before := buf.Len()

- if rec := c.getStream("/m/" + sqid + "/ice?peer=" + peer); rec.Code != http.StatusOK {

+ rec := c.getStream("/m/" + sqid + "/ice?peer=" + peer)

+ if rec.Code != http.StatusOK {

t.Fatalf("ice: %d %s", rec.Code, rec.Body.String())

}

- if success := buf.String()[before:]; strings.Contains(success, "ice") {

- t.Fatalf("success logged something: %q", success)

+ if buf.Len() != before {

+ t.Fatalf("success grew the log by %d bytes: %q", buf.Len()-before, buf.String()[before:])

+ }

+ var ans iceAnswer

+ if err := json.Unmarshal(rec.Body.Bytes(), &ans); err != nil {

+ t.Fatal(err)

+ }

+ if len(ans.ICEServers) != 2 || ans.ICEServers[1].Username == "" || ans.ICEServers[1].Credential == "" {

+ t.Fatalf("no minted credential to check the log against: %+v", ans.ICEServers)

}

+ turn := ans.ICEServers[1]

+ // An authority refusal: a foreign session naming this session's own

+ // admitted peer.

+ other := x.browser(t)

+ other.signIn("paul@rslw.com")

+ if rec := other.getStream("/m/" + sqid + "/ice?peer=" + peer); rec.Code != http.StatusNotFound {

+ t.Fatalf("foreign session: %d", rec.Code)

+ }

+ // A shape refusal too, for the same reasons signal_test.go's own

+ // content test covers more than one refusal path.

if rec := c.getStream("/m/" + sqid + "/ice?peer=zz"); rec.Code != http.StatusNotFound {

t.Fatalf("malformed peer: %d", rec.Code)

}

@@ -300,9 +333,226 @@ func TestIceLogsNothingOnSuccessAndNoCredentialOnRefusal(t *testing.T) {

if !strings.Contains(logs, "meet: ice refused") {

t.Fatalf("the refusal was not logged:\n%s", logs)

}

- for _, forbidden := range []string{vectorKey, ":abc:meet:"} {

+ for _, forbidden := range []string{vectorKey, ":abc:meet:", turn.Username, turn.Credential} {

if strings.Contains(logs, forbidden) {

- t.Fatalf("log leaked a credential:\n%s", logs)

+ t.Fatalf("log leaked a credential (%q):\n%s", forbidden, logs)

+ }

+// TestIceRefusesAnUnboundSessionBeforeTheStoreOrTheLimiter is

+// signal.go's own LiveForSession gate (review Task 5 item 1), carried

+// over to this endpoint: a session with no live participation in this

+// room at all is refused before either the store or the limiter is

+// ever touched. The meetings table is dropped first, so a store read

+// that happened anyway would 500, not 404 — and the limiter's own map

+// staying empty is the second, independent witness that nothing was

+// spent for a room this session holds nothing in.

+func TestIceRefusesAnUnboundSessionBeforeTheStoreOrTheLimiter(t *testing.T) {

+ x, sqid, _ := memberWithMeeting(t)

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ // No stream opened at all: this session has no live participation

+ // anywhere, in this room least of all.

+ if err := x.db.G.Exec("DROP TABLE meetings").Error; err != nil {

+ t.Fatal(err)

+ }

+ rec := c.getStream("/m/" + sqid + "/ice?peer=" + strings.Repeat("a", 32))

+ if rec.Code != http.StatusNotFound {

+ t.Fatalf("unbound session: %d, want 404 (a store read would have hit the dropped table and 500ed)", rec.Code)

+ }

+ if n := x.ctl.SignalLimiterSize(); n != 0 {

+ t.Fatalf("the limiter minted a bucket for an unbound session: %d keys", n)

+ }

+// TestIceRateLimitsOneSessionsBurst proves /ice spends the same

+// session+room bucket signal_test.go's own rate-limit test exercises

+// for POST /signal — sharing one limiter, per item 1, not a limiter of

+// its own.

+func TestIceRateLimitsOneSessionsBurst(t *testing.T) {

+ x, sqid, _ := memberWithMeeting(t)

+ if err := x.ctl.SetRelay(relayContract(t)); err != nil {

+ t.Fatal(err)

+ }

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ s := c.stream("/m/" + sqid + "/stream")

+ defer s.close()

+ peer, _ := s.hello()

+ if resp, b := c.signal(sqid, peer, `{"type":"join"}`); resp.StatusCode != http.StatusOK {

+ t.Fatalf("member join: %d %s", resp.StatusCode, b)

+ }

+ var got429 bool

+ for i := 0; i < 100; i++ {

+ rec := c.getStream("/m/" + sqid + "/ice?peer=" + peer)

+ if rec.Code == http.StatusTooManyRequests {

+ if rec.Header().Get("Retry-After") == "" {

+ t.Fatal("429 without Retry-After")

+ }

+ got429 = true

+ break

+ }

+ if rec.Code != http.StatusOK {

+ t.Fatalf("unexpected status mid-burst: %d %s", rec.Code, rec.Body.String())

}

+ if !got429 {

+ t.Fatal("100 requests in a burst never hit the rate limit")

+ }

+// TestIceHeaderClampsExtremeValues is item 3's clamp:

+// life = min(max(d, minIceLife), iceLife). A zero or negative header

+// floors to the two-second minimum; an unparseable one is ignored

+// outright (the untouched three-hour default); an oversized one is

+// capped at the three-hour default rather than minting a credential

+// that outlives it. renewAt is always strictly before expiresAt.

+//

+// The harness clock is truncated to a whole second first: expiresAt

+// and renewAt are both truncated to whole seconds by Unix(), and the

+// floor case's own two-thirds-of-a-second gap (2s vs on the order of

+// 1.667s) is not reliably on the same side of a rounding boundary

+// starting from a fractional second.

+func TestIceHeaderClampsExtremeValues(t *testing.T) {

+ x, sqid, _ := memberWithMeeting(t)

+ if err := x.ctl.SetRelay(relayContract(t)); err != nil {

+ t.Fatal(err)

+ }

+ x.now = x.now.Truncate(time.Second)

+ x.advance(0)

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ s := c.stream("/m/" + sqid + "/stream")

+ defer s.close()

+ peer, _ := s.hello()

+ if resp, b := c.signal(sqid, peer, `{"type":"join"}`); resp.StatusCode != http.StatusOK {

+ t.Fatalf("member join: %d %s", resp.StatusCode, b)

+ }

+ ask := func(ttl string) iceAnswer {

+ t.Helper()

+ req := httptest.NewRequest(http.MethodGet, "/m/"+sqid+"/ice?peer="+peer, nil)

+ req.Header.Set("X-Meet-Test-Ice-TTL", ttl)

+ rec := c.do(req)

+ if rec.Code != http.StatusOK {

+ t.Fatalf("ttl=%q: %d %s", ttl, rec.Code, rec.Body.String())

+ }

+ var ans iceAnswer

+ if err := json.Unmarshal(rec.Body.Bytes(), &ans); err != nil {

+ t.Fatal(err)

+ }

+ return ans

+ }

+ now := x.now

+ cases := []struct {

+ name string

+ ttl string

+ wantExpiresAt, wantRenewAt int64

+ }{

+ {"zero clamps to the floor", "0s", now.Add(2 * time.Second).Unix(), now.Add(1 * time.Second).Unix()},

+ {"negative clamps to the floor", "-5s", now.Add(2 * time.Second).Unix(), now.Add(1 * time.Second).Unix()},

+ {"unparseable falls back to the default", "not a duration", now.Add(3 * time.Hour).Unix(), now.Add(150 * time.Minute).Unix()},

+ {"an outsized value clamps to the ceiling", "10h", now.Add(3 * time.Hour).Unix(), now.Add(150 * time.Minute).Unix()},

+ }

+ for _, tc := range cases {

+ t.Run(tc.name, func(t *testing.T) {

+ ans := ask(tc.ttl)

+ if ans.ExpiresAt != tc.wantExpiresAt {

+ t.Errorf("ttl=%q: expiresAt %d, want %d", tc.ttl, ans.ExpiresAt, tc.wantExpiresAt)

+ }

+ if ans.RenewAt != tc.wantRenewAt {

+ t.Errorf("ttl=%q: renewAt %d, want %d", tc.ttl, ans.RenewAt, tc.wantRenewAt)

+ }

+ if ans.RenewAt >= ans.ExpiresAt {

+ t.Errorf("ttl=%q: renewAt %d is not before expiresAt %d", tc.ttl, ans.RenewAt, ans.ExpiresAt)

+ }

+ })

+ }

+// TestIceRefusesAnEndedMeetingEvenForAStillAdmittedGuest proves ice.go's

+// own ended_at read is load-bearing on its own: the meeting is ended

+// directly in the store here, never through the real end-meeting

+// handler (which also calls hub.End and erases the hub's own record of

+// the room) — so the hub still believes this guest is live and

+// admitted right up to the request this test makes. Only ice.go's own

+// store read stands between that stale hub state and a 200.

+func TestIceRefusesAnEndedMeetingEvenForAStillAdmittedGuest(t *testing.T) {

+ x, sqid, tok := memberWithMeeting(t)

+ if err := x.ctl.SetRelay(relayContract(t)); err != nil {

+ t.Fatal(err)

+ }

+ m := x.browser(t)

+ m.signIn("paul@example.com")

+ sm := m.stream("/m/" + sqid + "/stream")

+ defer sm.close()

+ pm, _ := sm.hello()

+ if resp, b := m.signal(sqid, pm, `{"type":"join"}`); resp.StatusCode != http.StatusOK {

+ t.Fatalf("member join: %d %s", resp.StatusCode, b)

+ }

+ g := x.browser(t)

+ g.get("/j/" + tok)

+ g.signIn("guest@example.com")

+ sg := g.stream("/m/" + sqid + "/stream")

+ defer sg.close()

+ pg, _ := sg.hello()

+ if resp, b := g.signal(sqid, pg, `{"type":"join"}`); resp.StatusCode != http.StatusOK {

+ t.Fatalf("guest join: %d %s", resp.StatusCode, b)

+ }

+ if resp, b := m.signal(sqid, pm, `{"type":"admit","payload":{"peer":"`+pg+`"}}`); resp.StatusCode != http.StatusOK {

+ t.Fatalf("admit: %d %s", resp.StatusCode, b)

+ }

+ if ev, _, ok := sg.nextOf("roster", 2*time.Second); !ok {

+ t.Fatalf("the guest was never admitted (got %q)", ev)

+ }

+ // A baseline 404 from an unrelated, unbound session, established

+ // before the meeting ends: every ice.go refusal shares this one

+ // body, so it stands in for a.notFoundBody() without reaching into

+ // the app package.

+ baseline := x.browser(t)

+ baseline.signIn("paul@rslw.com")

+ base := baseline.getStream("/m/" + sqid + "/ice?peer=" + pg)

+ if base.Code != http.StatusNotFound {

+ t.Fatalf("baseline 404: %d", base.Code)

+ }

+ if err := x.db.G.Exec("UPDATE meetings SET ended_at = ? WHERE sqid = ?", x.now, sqid).Error; err != nil {

+ t.Fatal(err)

+ }

+ rec := g.getStream("/m/" + sqid + "/ice?peer=" + pg)

+ if rec.Code != http.StatusNotFound || !bytes.Equal(rec.Body.Bytes(), base.Body.Bytes()) {

+ t.Fatalf("ended meeting: %d %s, want the same 404 as %s", rec.Code, rec.Body.Bytes(), base.Body.Bytes())

+ }

+// TestIceNeverLogsAMalformedPeerVerbatim is validPeerID's load-bearing

+// proof for this endpoint, signal.go's own precedent (review Important

+// 2): a peer that fails validation is never logged, whatever it

+// contains — a newline that could forge a second log line, or a length

+// that could bloat one.

+func TestIceNeverLogsAMalformedPeerVerbatim(t *testing.T) {

+ var buf bytes.Buffer

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ sqid, _ := createMeeting(t, c, "Board meeting", "")

+ s := c.stream("/m/" + sqid + "/stream")

+ defer s.close()

+ s.hello()

+ needle := "forged-log-line\n" + strings.Repeat("z", 4096)

+ rec := c.getStream("/m/" + sqid + "/ice?peer=" + url.QueryEscape(needle))

+ if rec.Code != http.StatusNotFound {

+ t.Fatalf("malformed peer: %d", rec.Code)

+ }

+ if strings.Contains(buf.String(), needle) || strings.Contains(buf.String(), "forged-log-line") {

+ t.Fatalf("the malformed peer reached the log:\n%s", buf.String())

+ }

}

diff --git a/internal/meettest/signal_test.go b/internal/meettest/signal_test.go

index c6ce4f1..ce7cd86 100644

--- a/internal/meettest/signal_test.go

+++ b/internal/meettest/signal_test.go

@@ -10,6 +10,8 @@ import (

"strings"

"testing"

"time"

+ meet "amadan.net/oficina/meet/internal/meet"

)

// Spec §4.2: every POST /signal must carry a peer the hub finds in this

@@ -153,7 +155,7 @@ func TestSignalRateLimitSharesOneBucketAcrossClaimedPeerIDs(t *testing.T) {

// already known to be exactly 32 lowercase hex.

func TestSignalRefusesAnOversizedPeerAndNeverLogsIt(t *testing.T) {

var buf bytes.Buffer

- x := newHarnessWithLogger(t, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

c := x.browser(t)

c.signIn("paul@example.com")

sqid, _ := createMeeting(t, c, "x", "")

@@ -219,7 +221,7 @@ func TestSignalSqidRotationNeverMintsARateLimiterBucket(t *testing.T) {

// before it can reach the limiter's key, mint a bucket, or a log line.

func TestSignalRefusesAnOversizedRoomIDAndNeverLogsIt(t *testing.T) {

var buf bytes.Buffer

- x := newHarnessWithLogger(t, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

c := x.browser(t)

c.signIn("paul@example.com")

@@ -292,7 +294,7 @@ func TestSignalFromADetachedSessionInsideItsWindowIsAccepted(t *testing.T) {

// behind — otherwise the rate-limit log is written by the attacker.

func TestTheRateLimitLogNamesTheSessionsRealPeer(t *testing.T) {

var buf bytes.Buffer

- x := newHarnessWithLogger(t, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

c := x.browser(t)

c.signIn("paul@example.com")

sqid, _ := createMeeting(t, c, "x", "")

@@ -574,7 +576,7 @@ func TestSignalAdmittedResumeReplaysAfterRoster(t *testing.T) {

// "zaphod-xyzzy") is its own needle.

func TestSignallingLogsCarryNoContent(t *testing.T) {

var buf bytes.Buffer

- x := newHarnessWithLogger(t, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

+ x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com")

c := x.browser(t)

c.signIn("paul@example.com")

sqid, tok := createMeeting(t, c, "Secret title", "")