amadan
Harden the module: six mutations survived the tests, and three were real bugs
GPT-6 reviewed the merged module and ran mutation testing on it. Six
deliberate regressions passed the whole suite. Its report is
CODEX-FINDINGS-MODULE.md — transcribed from its session log, because a
writing-pipeline skill on this machine needed an API it could not reach and
it correctly stopped rather than substituting itself for the missing tool.
Three were bugs in the code, not just gaps in the tests:
- The team guard read req.URL.Hostname() and ignored req.Host. Go dials the
URL and sends req.Host as the Host header, so a request could reach our
own team's instance while announcing itself as another team's, carrying
the body and the consumer token. A platform router keying on Host could
deliver it there. Both fields are checked now, and they must agree.
- Duplicate JSON keys defeated S2's duplicate-row rule entirely.
encoding/json keeps the last of a duplicate key and matches field names
case-insensitively, so {"result":"NOT_VISIBLE","result":"VISIBLE"} decodes
as VISIBLE and the row-level check never sees a duplicate to reject. A
strict pre-pass now refuses ambiguous bodies — duplicate keys at any
depth, case-folded, plus trailing content — before either decoder runs.
- CheckSiblingHost counted dots and never looked at a label, admitting
acme..oficina.works, acme.-docs.oficina.works, acme.docs_.oficina.works
and a host separated by an ideographic full stop whose label count can
change under IDNA normalisation. Labels are now held to RFC 1123 ASCII.
DecodeS1 was accepting `null`, `{}`, rows of `{}`, null gone/touched_at,
and trailing garbage after a valid page. A null field decoded to a live
zero-value row — item "" at revision 0, present rather than gone — so an
advanced cursor could commit an invalid page and a malformed departure
could lose a tombstone. Required fields are pointers now, so missing and
null are both visible.
The rest were tests that could not fail. The worst was mine: the test
written to prove the decoder is *total* ranged over its results without
checking the length, so returning nil passed it. Every failure-path case
now asserts cardinality first. The S2 tests use four distinct ids across
several revisions and exercise all four values, so dropping STALE or
ignoring Revision now fails. The cap tests use a literal 256 rather than
deriving both sides from the constant. The 403 test asserts
ErrContractBreach positively. Gate 4's redirect half runs against a real
302 through a counting dialer, using the fixture's redirect case, which was
in the file and never read.
mutations.py re-runs all nine regressions. All nine are caught.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13 files changed,
+766
−61
CODEX-FINDINGS-MODULE.md+141 −0fixtures/hostile_hosts.json+7 −1fixtures_test.go+24 −8mutations.py+86 −0s1.go+50 −2s1_test.go+55 −0s2.go+5 −0s2_test.go+143 −39s3_test.go+8 −2strictjson.go+80 −0subject_test.go+15 −7transport.go+72 −2transport_test.go+80 −0
diff --git a/CODEX-FINDINGS-MODULE.md b/CODEX-FINDINGS-MODULE.md| new file mode 100644 |
| index 0000000..aa36ef5 |
| --- /dev/null |
| +++ b/CODEX-FINDINGS-MODULE.md |
| @@ -0,0 +1,141 @@ |
| +# GPT-6 review of the contract module |
| + |
| +**Verdict: request changes.** Reviewer: GPT-6 via Codex, 2026-09-05, at |
| +`1cc739e`, against the doctrine at revision 4. |
| + |
| +The reviewer could not write this file itself — a `writing-pipeline` skill |
| +on this machine requires a drafting API that was unreachable, and it |
| +correctly stopped rather than substituting itself for the missing tool. |
| +The findings below are transcribed from its session log without |
| +editorialising. Where a claim is marked reproduced or executed, that is the |
| +reviewer's own word for work it did. |
| + |
| +**Verification it performed.** go1.26.5 on Linux; baseline `go test ./...` |
| +passing; probes under `/tmp/codex-module-review`, using `net.Pipe` to |
| +capture an HTTP request rather than a network; and six mutated copies of |
| +the module. The repository's own code was never modified by the review. |
| + |
| +**Six deliberate regressions survived the full test suite.** That is the |
| +headline, and it matters more than any single finding: the module's tests |
| +were weaker than the commit message claimed. |
| + |
| +--- |
| + |
| +## P1 |
| + |
| +**1. The team guard reads `req.URL.Hostname()` and ignores `req.Host`.** |
| +`transport.go:48-53`. A POST whose URL is `acme.docs.oficina.works` but |
| +whose `req.Host` is `other.docs.oficina.works` dials our own team and then |
| +sends `Host: other.docs.oficina.works` with the whole body and the consumer |
| +token. Reproduced in an in-memory probe. A shared platform router that |
| +routes on the HTTP Host could deliver it to the other team; that routing |
| +was not tested. This is an alternate authority field and a |
| +mis-provisioning hole, not a claim about hostile code, which §4.1 already |
| +excludes. *Correction:* reject any non-empty `Host` override whose |
| +canonical authority differs from the URL's. |
| + |
| +**2. Duplicate JSON keys defeat the duplicate-row rule.** `s2.go:51-66`. |
| +`encoding/json` collapses duplicate object keys before the pair-duplicate |
| +check ever runs, so |
| +`{"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE","result":"VISIBLE"}` |
| +decodes as `VISIBLE`. So does a second key spelled `RESULT`, and so does |
| +repeating the whole `results` array. A row carrying `revision:6,revision:7` |
| +reassigns the answer onto revision 7. All reproduced. *Correction:* |
| +validate member uniqueness at envelope and row level, aliases included. |
| + |
| +**3. The harness checks S3 status and never the body.** |
| +`conformance/conformance.go:68-80`. A handler that returns the required 404 |
| +for private and granted rows while including item-specific secret bodies |
| +passes all 19 checks, as does one that returns the wrong item's bytes with |
| +a 200. No never-existent id is ever requested. `DecodeS3` discarding a 404 |
| +body does not protect a caller reading the raw response. *Correction:* |
| +golden bodies for success and a canonical refusal body; compare raw bytes; |
| +request a never-existent id. |
| + |
| +**4. The harness accepts any feed that parses.** |
| +`conformance/conformance.go:120-126`. An always-empty feed passes, and so |
| +does a feed carrying a live row for an item that was never public. Worse, |
| +the reference server emits every fixture id — including the private, guest |
| +and link-only ones — as `gone` rows, exposing their ids, revisions and |
| +timestamps, and the harness calls that correct. *Correction:* golden |
| +initial and incremental feeds with per-consumer history; assert row |
| +membership and values; departures only for previously eligible items. |
| + |
| +**5. S2 is exercised with one asker, one revision, one request.** |
| +`conformance/conformance.go:87-114`. Every expected answer reduces to |
| +`rung == team`, and the reference server ignores the asker and the revision |
| +entirely — returning `VISIBLE` for any asker and any revision of a team |
| +item — and still passes. *Correction:* matched and mismatched subject |
| +kinds, an unknown kind, two people with different access, revisions |
| +before/equal/after current, and all four outcomes. |
| + |
| +## P2 |
| + |
| +**6. The custody fixture uses a distinct id per case**, where gate 3 asks |
| +for the same id in different states, and captures only final states with no |
| +grant, member or history setup. `doc-was-team` and `doc-link-added-late` |
| +perform no ingest, downgrade or link change; the late-link case is |
| +private-only with no team-visible control. A server caching positive |
| +answers across requests passes this single-pass setup and leaks after a |
| +downgrade. |
| + |
| +**7. The harness sends no Authorization, Origin or JSON Content-Type**, and |
| +never uses `fixture.Team`. A receiver with correct bearer auth answers 401 |
| +and fails all 19 checks; enforcing JSON Content-Type gives 415 on S2. So a |
| +server with no auth at all passes, which encourages bypassing the |
| +middleware to satisfy the gate. |
| + |
| +**8. `DecodeS1` decodes only the first JSON value and validates nothing.** |
| +It accepts `null`, `{}`, a row of `{}`, null `gone`/`touched_at`, and |
| +trailing garbage after a valid page — including trailing JSON containing a |
| +title. Duplicate `gone:true,gone:false` leaves `false`. A null or missing |
| +field yields a live zero-value row, so an advanced cursor can commit an |
| +invalid page and a malformed departure can lose a tombstone. |
| + |
| +**9. The host check counts dots and never validates a label.** |
| +`transport.go:67-90`. `CheckSiblingHost("acme", …)` admits |
| +`acme..oficina.works`, `acme.-docs.oficina.works`, `acme.docs_.oficina.works` |
| +and `acme.docs。evil.oficina.works` with an ideographic full stop, whose |
| +label count can change under IDNA normalisation. No valid same-team ASCII |
| +host is wrongly refused. *Correction:* validate non-empty bounded ASCII DNS |
| +labels; reject or canonicalise Unicode before counting. |
| + |
| +**10. Gate 4 is not a shared executable gate.** `fixtures_test.go` ignores |
| +the fixture's `redirect` field entirely; `transport_test.go:81-93` calls |
| +`CheckRedirect` directly with no 3xx and no connection counting; and |
| +`conformance.Check` takes only an `http.Handler`, so a consuming app has no |
| +way to run the client-side gate at all. An app using a redirect-following |
| +unguarded client passes the published `Run`. This is a coverage and API |
| +finding, not a claim that redirects are currently followed. |
| + |
| +**11. The non-2xx test asserts nothing when the decoder returns nothing.** |
| +`s2_test.go:55-71` ranges over the result without a length check, so |
| +mutating that branch to `return nil` makes the test perform zero |
| +assertions and the suite pass. The test cannot fail on the one path its |
| +name describes. *Executed.* |
| + |
| +**12. The S2 tests use two ids, one revision each, and never a `STALE`.** |
| +Mutating the decoder to ignore `Revision` in both map insertion and lookup |
| +passes the full suite; so does deleting `STALE` from the `known()` switch. |
| +The implementation is correct; the tests cannot tell. *Both executed.* |
| + |
| +**13. The cap test derives both sizes from `MaxS2Pairs`**, so it checks |
| +self-consistency rather than the doctrine's 256. Changing the constant to |
| +512 passes the entire suite. *Executed.* |
| + |
| +## P3 |
| + |
| +**14. The 403 test never asserts `errors.Is(err, ErrContractBreach)`.** |
| +Mutating the 403 branch to a generic error passes, losing the breach/outage |
| +distinction the file promises. |
| + |
| +--- |
| + |
| +## Gate verdicts |
| + |
| +| Gate | Verdict | |
| +|---|---| |
| +| 1. Fixture conformance | Incomplete — bodies, feed contents and auth unchecked | |
| +| 2. S2 total decoder | Incomplete — ambiguity, wire shape, and three surviving mutations | |
| +| 3. Custody | A static subset — no wire equality, no transitions | |
| +| 4. No cross-team dial | Partial unit coverage, not a shared executable gate | |
diff --git a/fixtures/hostile_hosts.json b/fixtures/hostile_hosts.json| index ded880c..d8fa8c7 100644 |
| --- a/fixtures/hostile_hosts.json |
| +++ b/fixtures/hostile_hosts.json |
| @@ -18,7 +18,13 @@ |
| { "host": "evil.com", "why": "outside the suite domain entirely" }, |
| { "host": "docs.oficina.works", "why": "no team label at all" }, |
| { "host": "acme.team.docs.oficina.works", "why": "an extra label between team and app" }, |
| - { "host": "ACME-EVIL.docs.oficina.works", "why": "case must not smuggle a different label through" } |
| + { "host": "ACME-EVIL.docs.oficina.works", "why": "case must not smuggle a different label through" }, |
| + { "host": "acme..oficina.works", "why": "an empty app label counts as two labels but names no app" }, |
| + { "host": "acme.-docs.oficina.works", "why": "a label may not begin with a hyphen" }, |
| + { "host": "acme.docs-.oficina.works", "why": "a label may not end with a hyphen" }, |
| + { "host": "acme.docs_.oficina.works", "why": "underscore is not a hostname character" }, |
| + { "host": "acme.docs。evil.oficina.works", "why": "an ideographic full stop is not a label separator here, and IDNA normalisation could turn it into one later" }, |
| + { "host": ".docs.oficina.works", "why": "an empty team label" } |
| ], |
| "reach": [ |
diff --git a/fixtures_test.go b/fixtures_test.go| index db69f59..1af7858 100644 |
| --- a/fixtures_test.go |
| +++ b/fixtures_test.go |
| @@ -11,17 +11,22 @@ type hostCase struct { |
| Why string `json:"why"` |
| } |
| +type redirectCase struct { |
| + From string `json:"from"` |
| + To string `json:"to"` |
| + Why string `json:"why"` |
| +} |
| + |
| type hostileHosts struct { |
| - Team string `json:"team"` |
| - Refuse []hostCase `json:"refuse"` |
| - Reach []hostCase `json:"reach"` |
| - Comment []string `json:"comment"` |
| + Team string `json:"team"` |
| + Refuse []hostCase `json:"refuse"` |
| + Reach []hostCase `json:"reach"` |
| + Redirect redirectCase `json:"redirect"` |
| + Comment []string `json:"comment"` |
| } |
| -// The hostile-hostname fixture is the shared list, so every app runs gate 4 |
| -// against the same hosts rather than each inventing its own. This test is the |
| -// module holding itself to the fixture it publishes. |
| -func TestHostileHostFixture(t *testing.T) { |
| +func loadHostileHosts(t *testing.T) hostileHosts { |
| + t.Helper() |
| raw, err := os.ReadFile("fixtures/hostile_hosts.json") |
| if err != nil { |
| t.Fatalf("reading fixture: %v", err) |
| @@ -30,6 +35,17 @@ func TestHostileHostFixture(t *testing.T) { |
| if err := json.Unmarshal(raw, &f); err != nil { |
| t.Fatalf("parsing fixture: %v", err) |
| } |
| + return f |
| +} |
| + |
| +// The hostile-hostname fixture is the shared list, so every app runs gate 4 |
| +// against the same hosts rather than each inventing its own. This test is the |
| +// module holding itself to the fixture it publishes. |
| +func TestHostileHostFixture(t *testing.T) { |
| + f := loadHostileHosts(t) |
| + if f.Redirect.From == "" || f.Redirect.To == "" { |
| + t.Error("fixture has no redirect case; TestSiblingClientRefusesAFixtureRedirect needs one") |
| + } |
| if len(f.Refuse) == 0 || len(f.Reach) == 0 { |
| t.Fatal("fixture has no cases; a gate that asserts nothing passes everything") |
| } |
diff --git a/mutations.py b/mutations.py| new file mode 100644 |
| index 0000000..219c06f |
| --- /dev/null |
| +++ b/mutations.py |
| @@ -0,0 +1,86 @@ |
| +"""Re-run the six regressions the GPT-6 module review found surviving. |
| + |
| +Each mutation is applied to a copy of the tree, the suite is run, and the |
| +mutation must FAIL the suite. A survivor means the tests still cannot tell. |
| +""" |
| +import io, os, shutil, subprocess, sys, tempfile |
| + |
| +ROOT = os.path.dirname(os.path.abspath(__file__)) |
| +CACHE = os.path.join(ROOT, ".gocache") |
| + |
| +MUTATIONS = [ |
| + ("11: non-2xx returns nothing instead of all-UNKNOWN", "s2.go", |
| + "\tif status < 200 || status > 299 {\n\t\treturn allUnknown(len(asked))\n\t}", |
| + "\tif status < 200 || status > 299 {\n\t\treturn nil\n\t}"), |
| + |
| + ("12a: the revision is ignored when matching answers to questions", "s2.go", |
| + "\t\tpair := ItemRevision{ItemID: r.ItemID, Revision: r.Revision}", |
| + "\t\tpair := ItemRevision{ItemID: r.ItemID}"), |
| + |
| + ("12b: STALE is dropped from the closed vocabulary", "s2.go", |
| + "\tcase VISIBLE, NOT_VISIBLE, STALE, UNKNOWN:", |
| + "\tcase VISIBLE, NOT_VISIBLE, UNKNOWN:"), |
| + |
| + ("13: the batch cap is doubled", "subject.go", |
| + "const MaxS2Pairs = 256", |
| + "const MaxS2Pairs = 512"), |
| + |
| + ("14: a 403 loses its ErrContractBreach sentinel", "s3.go", |
| + '\t\treturn nil, fmt.Errorf("%w: S3 answered 403 where the contract requires 404", ErrContractBreach)', |
| + '\t\treturn nil, fmt.Errorf("S3 answered 403")'), |
| + |
| + ("10: redirects are followed", "transport.go", |
| + '\t\t\treturn fmt.Errorf("suite: refusing to follow a redirect to %q", req.URL.Host)', |
| + "\t\t\treturn nil"), |
| + |
| + ("1: the Host header override is not checked", "transport.go", |
| + "\tif req.Host != \"\" {", |
| + "\tif false {"), |
| + |
| + ("2: duplicate JSON keys are not rejected in S2", "s2.go", |
| + "\tif err := rejectAmbiguousJSON(body); err != nil {\n\t\treturn allUnknown(len(asked))\n\t}", |
| + "\tif false {\n\t\treturn allUnknown(len(asked))\n\t}"), |
| + |
| + ("9: hostname labels are not validated", "transport.go", |
| + "\t\tif err := validLabel(label); err != nil {", |
| + "\t\tif false {\n\t\t\t_ = label\n\t\tif err := error(nil); err != nil {"), |
| +] |
| + |
| +survivors = [] |
| +for name, path, before, after in MUTATIONS: |
| + work = tempfile.mkdtemp(prefix="mut-") |
| + tree = os.path.join(work, "m") |
| + # Only the module's own files: the worktree root also holds unreadable |
| + # dotfiles that a whole-tree copy trips over. |
| + os.makedirs(tree) |
| + for entry in os.listdir(ROOT): |
| + if entry.endswith(".go") or entry == "go.mod": |
| + shutil.copy2(os.path.join(ROOT, entry), os.path.join(tree, entry)) |
| + shutil.copytree(os.path.join(ROOT, "fixtures"), os.path.join(tree, "fixtures")) |
| + shutil.copytree(os.path.join(ROOT, "conformance"), os.path.join(tree, "conformance")) |
| + |
| + target = os.path.join(tree, path) |
| + src = io.open(target, encoding="utf-8").read() |
| + if before not in src: |
| + print("SKIP %s — anchor not found in %s" % (name, path)) |
| + shutil.rmtree(work) |
| + continue |
| + io.open(target, "w", encoding="utf-8").write(src.replace(before, after, 1)) |
| + |
| + env = dict(os.environ, GOCACHE=CACHE) |
| + r = subprocess.run(["go", "test", "./..."], cwd=tree, capture_output=True, text=True, env=env) |
| + shutil.rmtree(work) |
| + |
| + if r.returncode == 0: |
| + print("SURVIVED %s" % name) |
| + survivors.append(name) |
| + else: |
| + print("caught %s" % name) |
| + |
| +print() |
| +if survivors: |
| + print("%d mutation(s) survived:" % len(survivors)) |
| + for s in survivors: |
| + print(" -", s) |
| + sys.exit(1) |
| +print("all %d mutations caught" % len(MUTATIONS)) |
diff --git a/s1.go b/s1.go| index 702bd89..cccee5f 100644 |
| --- a/s1.go |
| +++ b/s1.go |
| @@ -39,15 +39,63 @@ func DecodeS1(status int, body []byte) (Feed, error) { |
| return Feed{}, fmt.Errorf("suite: S1 answered %d", status) |
| } |
| + // A duplicated key is resolved silently by encoding/json, and a duplicated |
| + // `gone` would lose a tombstone. Trailing content is a second document |
| + // riding along unread, since Decode reads one value and stops. |
| + if err := rejectAmbiguousJSON(body); err != nil { |
| + return Feed{}, fmt.Errorf("suite: S1 body is ambiguous: %w", err) |
| + } |
| + |
| dec := json.NewDecoder(bytes.NewReader(body)) |
| // A row carrying more than the four fields is either a sibling leaking or |
| // a sibling running a different contract. Ignoring the extra field would |
| // let either go unnoticed until somebody grepped the store. |
| dec.DisallowUnknownFields() |
| - var feed Feed |
| - if err := dec.Decode(&feed); err != nil { |
| + // Pointers so a missing field and an explicit null are both visible. Left |
| + // to the plain types, either one decodes to a zero value and the row |
| + // arrives looking like a real item at revision 0 that is present rather |
| + // than gone. |
| + var wire struct { |
| + Cursor *string `json:"cursor"` |
| + Rows *[]struct { |
| + ItemID *string `json:"item_id"` |
| + Revision *int64 `json:"revision"` |
| + Gone *bool `json:"gone"` |
| + TouchedAt *time.Time `json:"touched_at"` |
| + } `json:"rows"` |
| + } |
| + if err := dec.Decode(&wire); err != nil { |
| return Feed{}, fmt.Errorf("suite: S1 body outside the contract: %w", err) |
| } |
| + |
| + if wire.Cursor == nil || *wire.Cursor == "" { |
| + // Without a cursor a consumer cannot advance, and committing the empty |
| + // one loses its place in the sibling's log. |
| + return Feed{}, fmt.Errorf("suite: S1 answered without a cursor") |
| + } |
| + if wire.Rows == nil { |
| + return Feed{}, fmt.Errorf("suite: S1 answered without a rows array") |
| + } |
| + |
| + feed := Feed{Cursor: *wire.Cursor, Rows: make([]FeedRow, 0, len(*wire.Rows))} |
| + for i, r := range *wire.Rows { |
| + switch { |
| + case r.ItemID == nil || *r.ItemID == "": |
| + return Feed{}, fmt.Errorf("suite: S1 row %d has no item_id", i) |
| + case r.Revision == nil: |
| + return Feed{}, fmt.Errorf("suite: S1 row %d has no revision", i) |
| + case r.Gone == nil: |
| + return Feed{}, fmt.Errorf("suite: S1 row %d has no gone", i) |
| + case r.TouchedAt == nil: |
| + return Feed{}, fmt.Errorf("suite: S1 row %d has no touched_at", i) |
| + } |
| + feed.Rows = append(feed.Rows, FeedRow{ |
| + ItemID: *r.ItemID, |
| + Revision: *r.Revision, |
| + Gone: *r.Gone, |
| + TouchedAt: *r.TouchedAt, |
| + }) |
| + } |
| return feed, nil |
| } |
diff --git a/s1_test.go b/s1_test.go| index 28d381b..075b573 100644 |
| --- a/s1_test.go |
| +++ b/s1_test.go |
| @@ -63,6 +63,61 @@ func TestS1GoneCarriesNoReason(t *testing.T) { |
| } |
| } |
| +// An empty feed is legitimate and must keep working: it means nothing changed. |
| +func TestS1EmptyFeedIsValid(t *testing.T) { |
| + feed, err := DecodeS1(200, []byte(`{"cursor":"c-1","rows":[]}`)) |
| + if err != nil { |
| + t.Fatalf("unexpected error: %v", err) |
| + } |
| + if feed.Cursor != "c-1" || len(feed.Rows) != 0 { |
| + t.Errorf("got %+v, want an empty feed at cursor c-1", feed) |
| + } |
| +} |
| + |
| +// Bodies that parse but say nothing. `null` and `{}` both decode without |
| +// error into a zero Feed, which a caller would read as an empty feed at an |
| +// empty cursor — and committing that cursor loses the consumer's place. |
| +// Reported by the GPT-6 module review, finding 8. |
| +func TestS1RejectsBodiesThatSayNothing(t *testing.T) { |
| + for _, body := range []string{`null`, `{}`, `{"rows":[]}`} { |
| + if _, err := DecodeS1(200, []byte(body)); err == nil { |
| + t.Errorf("accepted %s, want a refusal", body) |
| + } |
| + } |
| +} |
| + |
| +// A row missing its identity, or carrying null where a value belongs, decodes |
| +// to a live zero-value row: item "" at revision 0, present rather than gone. |
| +func TestS1RejectsIncompleteRows(t *testing.T) { |
| + bodies := []string{ |
| + `{"cursor":"c-1","rows":[{}]}`, |
| + `{"cursor":"c-1","rows":[{"item_id":"","revision":7,"gone":false,"touched_at":"2026-09-05T09:00:00Z"}]}`, |
| + `{"cursor":"c-1","rows":[{"item_id":"doc-1","revision":7,"gone":null,"touched_at":null}]}`, |
| + } |
| + for _, body := range bodies { |
| + if _, err := DecodeS1(200, []byte(body)); err == nil { |
| + t.Errorf("accepted %s, want a refusal", body) |
| + } |
| + } |
| +} |
| + |
| +// Trailing content after a complete page, and duplicate keys inside a row. |
| +// json.Decoder.Decode reads one value and stops, so a second document — one |
| +// carrying a title, say — rides along unread; and a duplicated `gone` key is |
| +// resolved silently, so `gone:true,gone:false` loses a tombstone. |
| +func TestS1RejectsAmbiguousBodies(t *testing.T) { |
| + bodies := []string{ |
| + `{"cursor":"c-1","rows":[]} {"cursor":"c-2","rows":[{"title":"Q3 plan"}]}`, |
| + `{"cursor":"c-1","rows":[{"item_id":"doc-1","revision":7,"gone":true,"gone":false,"touched_at":"2026-09-05T09:00:00Z"}]}`, |
| + `{"cursor":"c-1","cursor":"c-2","rows":[]}`, |
| + } |
| + for _, body := range bodies { |
| + if _, err := DecodeS1(200, []byte(body)); err == nil { |
| + t.Errorf("accepted %s, want a refusal", body) |
| + } |
| + } |
| +} |
| + |
| // A non-2xx feed answer is an error, not an empty feed. An empty feed means |
| // "nothing changed"; returning that for a failed call would advance a |
| // consumer's cursor past changes it never saw. |
diff --git a/s2.go b/s2.go| index e9cf5d9..4a99835 100644 |
| --- a/s2.go |
| +++ b/s2.go |
| @@ -48,6 +48,11 @@ func DecodeS2(asked []ItemRevision, status int, body []byte) []Authorization { |
| if status < 200 || status > 299 { |
| return allUnknown(len(asked)) |
| } |
| + // Before decoding, not after: a duplicate key is resolved by encoding/json |
| + // and gone by the time the duplicate-row rule below could see it. |
| + if err := rejectAmbiguousJSON(body); err != nil { |
| + return allUnknown(len(asked)) |
| + } |
| var parsed s2Body |
| if err := json.Unmarshal(body, &parsed); err != nil { |
| return allUnknown(len(asked)) |
diff --git a/s2_test.go b/s2_test.go| index 11f8adb..f7c7e04 100644 |
| --- a/s2_test.go |
| +++ b/s2_test.go |
| @@ -1,33 +1,78 @@ |
| package suite |
| -import "testing" |
| - |
| -// Gate 2, first case. A well-formed answer yields exactly one result per |
| -// requested pair, in request order. |
| -// |
| -// Doctrine §4.3: four values, none collapsible into another. §6: cap 256 |
| -// pairs, total decoder. |
| -func TestS2DecodeWellFormed(t *testing.T) { |
| +import ( |
| + "fmt" |
| + "testing" |
| +) |
| + |
| +// assertResults is the guard the first version of these tests lacked. Every |
| +// case here is about totality, and a decoder that returns nothing satisfies a |
| +// loop over its results without asserting anything at all — mutating the |
| +// non-2xx branch to `return nil` passed the whole suite. Length first, always. |
| +func assertResults(t *testing.T, got []Authorization, want []Authorization) { |
| + t.Helper() |
| + if len(got) != len(want) { |
| + t.Fatalf("got %d results, want exactly %d — the decoder is total or it is nothing", len(got), len(want)) |
| + } |
| + for i := range want { |
| + if got[i] != want[i] { |
| + t.Errorf("result %d: got %q, want %q", i, got[i], want[i]) |
| + } |
| + } |
| +} |
| + |
| +// The four values, each returned distinctly, on pairs that differ by id and by |
| +// revision. Earlier versions used two ids at one revision each and never a |
| +// STALE, so the decoder could ignore Revision entirely — or drop STALE from |
| +// its vocabulary — and the suite still passed. |
| +func TestS2DecodeAllFourValues(t *testing.T) { |
| asked := []ItemRevision{ |
| {ItemID: "doc-1", Revision: 7}, |
| {ItemID: "doc-2", Revision: 3}, |
| + {ItemID: "doc-3", Revision: 11}, |
| + {ItemID: "doc-4", Revision: 2}, |
| } |
| body := []byte(`{"results":[ |
| + {"item_id":"doc-3","revision":11,"result":"STALE"}, |
| {"item_id":"doc-1","revision":7,"result":"VISIBLE"}, |
| + {"item_id":"doc-4","revision":2,"result":"UNKNOWN"}, |
| {"item_id":"doc-2","revision":3,"result":"NOT_VISIBLE"} |
| ]}`) |
| got := DecodeS2(asked, 200, body) |
| - want := []Authorization{VISIBLE, NOT_VISIBLE} |
| - if len(got) != len(want) { |
| - t.Fatalf("got %d results, want %d", len(got), len(want)) |
| - } |
| - for i := range want { |
| - if got[i] != want[i] { |
| - t.Errorf("result %d: got %v, want %v", i, got[i], want[i]) |
| - } |
| + // Answered out of order on purpose: results come back in the order asked, |
| + // not the order the sibling felt like replying in. |
| + assertResults(t, got, []Authorization{VISIBLE, NOT_VISIBLE, STALE, UNKNOWN}) |
| +} |
| + |
| +// The revision is part of the identity of a pair. An answer about revision 6 |
| +// does not answer a question about revision 7 of the same item — that is the |
| +// whole reason STALE exists — so it must not be matched to it. |
| +func TestS2AnswerForAnotherRevisionDoesNotMatch(t *testing.T) { |
| + asked := []ItemRevision{{ItemID: "doc-1", Revision: 7}} |
| + body := []byte(`{"results":[{"item_id":"doc-1","revision":6,"result":"VISIBLE"}]}`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| +} |
| + |
| +// The same item at two revisions is two questions, and each gets its own |
| +// answer. |
| +func TestS2SameItemAtTwoRevisions(t *testing.T) { |
| + asked := []ItemRevision{ |
| + {ItemID: "doc-1", Revision: 6}, |
| + {ItemID: "doc-1", Revision: 7}, |
| } |
| + body := []byte(`{"results":[ |
| + {"item_id":"doc-1","revision":6,"result":"STALE"}, |
| + {"item_id":"doc-1","revision":7,"result":"VISIBLE"} |
| + ]}`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{STALE, VISIBLE}) |
| } |
| // An item the app of record did not answer for is UNKNOWN — never the zero |
| @@ -42,12 +87,20 @@ func TestS2MissingItemIsUnknown(t *testing.T) { |
| got := DecodeS2(asked, 200, body) |
| - if got[1] != UNKNOWN { |
| - t.Errorf("missing item: got %q, want UNKNOWN", got[1]) |
| - } |
| - if got[0] != VISIBLE { |
| - t.Errorf("answered item: got %q, want VISIBLE", got[0]) |
| - } |
| + assertResults(t, got, []Authorization{VISIBLE, UNKNOWN}) |
| +} |
| + |
| +// An answer nobody asked for is ignored rather than shifting the others along. |
| +func TestS2UnsolicitedAnswerIsIgnored(t *testing.T) { |
| + asked := []ItemRevision{{ItemID: "doc-1", Revision: 7}} |
| + body := []byte(`{"results":[ |
| + {"item_id":"doc-9","revision":1,"result":"VISIBLE"}, |
| + {"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE"} |
| + ]}`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{NOT_VISIBLE}) |
| } |
| // A non-2xx status is a whole-call failure: every pair is UNKNOWN, whatever |
| @@ -64,11 +117,7 @@ func TestS2NonSuccessStatusIsWholeCallUnknown(t *testing.T) { |
| got := DecodeS2(asked, 500, body) |
| - for i, a := range got { |
| - if a != UNKNOWN { |
| - t.Errorf("pair %d: got %q, want UNKNOWN", i, a) |
| - } |
| - } |
| + assertResults(t, got, []Authorization{UNKNOWN, UNKNOWN}) |
| } |
| // A body that does not parse is a whole-call failure too. |
| @@ -77,15 +126,13 @@ func TestS2MalformedBodyIsWholeCallUnknown(t *testing.T) { |
| got := DecodeS2(asked, 200, []byte(`{"results":[{"item_id":`)) |
| - if got[0] != UNKNOWN { |
| - t.Errorf("got %q, want UNKNOWN", got[0]) |
| - } |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| } |
| // An item answered twice is UNKNOWN for that item. Two answers are not an |
| // answer, and picking either one would be the caller inventing a decision the |
| // app of record did not make. The other pairs are unaffected. |
| -func TestS2DuplicateItemIsUnknown(t *testing.T) { |
| +func TestS2DuplicateRowIsUnknown(t *testing.T) { |
| asked := []ItemRevision{ |
| {ItemID: "doc-1", Revision: 7}, |
| {ItemID: "doc-2", Revision: 3}, |
| @@ -98,12 +145,46 @@ func TestS2DuplicateItemIsUnknown(t *testing.T) { |
| got := DecodeS2(asked, 200, body) |
| - if got[0] != UNKNOWN { |
| - t.Errorf("duplicated item: got %q, want UNKNOWN", got[0]) |
| - } |
| - if got[1] != VISIBLE { |
| - t.Errorf("other item: got %q, want VISIBLE", got[1]) |
| - } |
| + assertResults(t, got, []Authorization{UNKNOWN, VISIBLE}) |
| +} |
| + |
| +// A duplicated *key* is the same defect one level down, and encoding/json |
| +// hides it: it keeps the last value silently, so the duplicate-row check above |
| +// never runs. Reported by the GPT-6 module review, finding 2. |
| +func TestS2DuplicateKeyInARowIsRefused(t *testing.T) { |
| + asked := []ItemRevision{{ItemID: "doc-1", Revision: 7}} |
| + body := []byte(`{"results":[ |
| + {"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE","result":"VISIBLE"} |
| + ]}`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| +} |
| + |
| +// The same trick at the envelope level: two `results` arrays, the second |
| +// overwriting the first. |
| +func TestS2DuplicateEnvelopeKeyIsRefused(t *testing.T) { |
| + asked := []ItemRevision{{ItemID: "doc-1", Revision: 7}} |
| + body := []byte(`{ |
| + "results":[{"item_id":"doc-1","revision":7,"result":"NOT_VISIBLE"}], |
| + "results":[{"item_id":"doc-1","revision":7,"result":"VISIBLE"}] |
| + }`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| +} |
| + |
| +// Trailing content after a complete document is refused. A sibling appending |
| +// a second object is not speaking this contract. |
| +func TestS2TrailingContentIsRefused(t *testing.T) { |
| + asked := []ItemRevision{{ItemID: "doc-1", Revision: 7}} |
| + body := []byte(`{"results":[{"item_id":"doc-1","revision":7,"result":"VISIBLE"}]} {"results":[]}`) |
| + |
| + got := DecodeS2(asked, 200, body) |
| + |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| } |
| // A value outside the four is UNKNOWN. The vocabulary is closed (§4.3), so an |
| @@ -115,7 +196,30 @@ func TestS2UnrecognisedValueIsUnknown(t *testing.T) { |
| got := DecodeS2(asked, 200, body) |
| - if got[0] != UNKNOWN { |
| - t.Errorf("got %q, want UNKNOWN", got[0]) |
| + assertResults(t, got, []Authorization{UNKNOWN}) |
| +} |
| + |
| +// A full-size question decodes one result per pair and nothing else. 256 is |
| +// written literally rather than derived from MaxS2Pairs: a test that computes |
| +// both sides from the constant checks its own consistency, and changing the |
| +// constant to 512 passed the entire suite. |
| +func TestS2DecodesAFullBatch(t *testing.T) { |
| + const fullBatch = 256 |
| + |
| + asked := make([]ItemRevision, fullBatch) |
| + want := make([]Authorization, fullBatch) |
| + body := `{"results":[` |
| + for i := range asked { |
| + asked[i] = ItemRevision{ItemID: fmt.Sprintf("doc-%d", i), Revision: int64(i)} |
| + want[i] = VISIBLE |
| + if i > 0 { |
| + body += "," |
| + } |
| + body += fmt.Sprintf(`{"item_id":"doc-%d","revision":%d,"result":"VISIBLE"}`, i, i) |
| } |
| + body += `]}` |
| + |
| + got := DecodeS2(asked, 200, []byte(body)) |
| + |
| + assertResults(t, got, want) |
| } |
diff --git a/s3_test.go b/s3_test.go| index accf823..067db92 100644 |
| --- a/s3_test.go |
| +++ b/s3_test.go |
| @@ -36,8 +36,11 @@ func TestS3NotFoundIsIndistinguishable(t *testing.T) { |
| func TestS3ForbiddenIsAContractBreach(t *testing.T) { |
| _, err := DecodeS3(403, nil) |
| - if err == nil { |
| - t.Fatal("got nil error for a 403, want a contract breach reported") |
| + // Positively, not just "some error": mutating this branch to a plain |
| + // fmt.Errorf passed the suite, which lost exactly the breach-versus-outage |
| + // distinction the sentinel exists to carry. |
| + if !errors.Is(err, ErrContractBreach) { |
| + t.Errorf("got %v, want ErrContractBreach", err) |
| } |
| if errors.Is(err, ErrNotAvailable) { |
| t.Error("403 was folded into ErrNotAvailable; it must be distinguishable") |
| @@ -54,4 +57,7 @@ func TestS3ServerErrorIsNotARefusal(t *testing.T) { |
| if errors.Is(err, ErrNotAvailable) { |
| t.Error("503 was reported as a refusal; an outage is not an answer") |
| } |
| + if errors.Is(err, ErrContractBreach) { |
| + t.Error("503 was reported as a contract breach; a sibling being down is not a sibling misbehaving") |
| + } |
| } |
diff --git a/strictjson.go b/strictjson.go| new file mode 100644 |
| index 0000000..a14cbdd |
| --- /dev/null |
| +++ b/strictjson.go |
| @@ -0,0 +1,80 @@ |
| +package suite |
| + |
| +import ( |
| + "bytes" |
| + "encoding/json" |
| + "fmt" |
| + "strings" |
| +) |
| + |
| +// rejectAmbiguousJSON refuses a body whose meaning depends on which duplicate |
| +// wins. |
| +// |
| +// encoding/json resolves a duplicate object key by keeping the last one, and |
| +// matches field names case-insensitively, so a sibling can send |
| +// `{"result":"NOT_VISIBLE","result":"VISIBLE"}` — or spell the second one |
| +// `RESULT` — and the value that survives is not the one a reader of the bytes |
| +// would expect. That defeats every check made *after* decoding, including S2's |
| +// duplicate-row rule: the duplicate is gone before the rule can see it. |
| +// |
| +// Keys are folded to lower case before comparison because that is how |
| +// encoding/json matches them. Two keys differing only in case are therefore |
| +// treated as the same key, which is the safe reading and costs nothing: no |
| +// call in this contract has two fields whose names differ only by case. |
| +func rejectAmbiguousJSON(data []byte) error { |
| + dec := json.NewDecoder(bytes.NewReader(data)) |
| + if err := scanUnique(dec); err != nil { |
| + return err |
| + } |
| + // Anything after the first complete value means the body is two documents |
| + // wearing one Content-Length, and which one is "the" answer is a guess. |
| + if dec.More() { |
| + return fmt.Errorf("suite: trailing content after the JSON document") |
| + } |
| + return nil |
| +} |
| + |
| +func scanUnique(dec *json.Decoder) error { |
| + tok, err := dec.Token() |
| + if err != nil { |
| + return err |
| + } |
| + |
| + delim, isDelim := tok.(json.Delim) |
| + if !isDelim { |
| + return nil // a scalar; nothing nested to check |
| + } |
| + |
| + switch delim { |
| + case '{': |
| + seen := make(map[string]bool) |
| + for dec.More() { |
| + keyTok, err := dec.Token() |
| + if err != nil { |
| + return err |
| + } |
| + key, ok := keyTok.(string) |
| + if !ok { |
| + return fmt.Errorf("suite: object key is not a string") |
| + } |
| + folded := strings.ToLower(key) |
| + if seen[folded] { |
| + return fmt.Errorf("suite: duplicate object key %q", key) |
| + } |
| + seen[folded] = true |
| + |
| + if err := scanUnique(dec); err != nil { |
| + return err |
| + } |
| + } |
| + case '[': |
| + for dec.More() { |
| + if err := scanUnique(dec); err != nil { |
| + return err |
| + } |
| + } |
| + } |
| + |
| + _, err = dec.Token() // the matching close |
| + return err |
| +} |
diff --git a/subject_test.go b/subject_test.go| index 5f4092d..b0e537d 100644 |
| --- a/subject_test.go |
| +++ b/subject_test.go |
| @@ -54,18 +54,26 @@ func TestSubjectLeavesPersonRefAlone(t *testing.T) { |
| // truncated: a silently shortened question produces a confident answer about |
| // items nobody asked about, and the missing ones would decode as UNKNOWN |
| // without anyone learning why. |
| +// The numbers are literal, not derived from MaxS2Pairs. Deriving both sides |
| +// from the constant tests the code's self-consistency and nothing else: |
| +// changing the constant to 512 passed the entire suite. 256 is the doctrine's |
| +// number, so 256 is what appears here. |
| func TestS2BatchOverCapIsRefused(t *testing.T) { |
| - pairs := make([]ItemRevision, MaxS2Pairs+1) |
| - |
| - if err := CheckS2Batch(pairs); err == nil { |
| - t.Fatalf("got nil error for %d pairs, want a refusal", len(pairs)) |
| + if err := CheckS2Batch(make([]ItemRevision, 257)); err == nil { |
| + t.Fatal("got nil error for 257 pairs, want a refusal at the doctrine's cap of 256") |
| } |
| } |
| func TestS2BatchAtCapIsAccepted(t *testing.T) { |
| - pairs := make([]ItemRevision, MaxS2Pairs) |
| + if err := CheckS2Batch(make([]ItemRevision, 256)); err != nil { |
| + t.Errorf("got %v for 256 pairs, want it accepted", err) |
| + } |
| +} |
| - if err := CheckS2Batch(pairs); err != nil { |
| - t.Errorf("got %v for %d pairs, want it accepted", err, len(pairs)) |
| +// And the constant itself is the doctrine's, so a later edit to it fails here |
| +// rather than quietly widening every caller's batch. |
| +func TestS2CapIsTheDoctrinesNumber(t *testing.T) { |
| + if MaxS2Pairs != 256 { |
| + t.Errorf("MaxS2Pairs is %d, want 256 (doctrine §6, memoria §6.1)", MaxS2Pairs) |
| } |
| } |
diff --git a/transport.go b/transport.go| index b5e43f6..14e0d5d 100644 |
| --- a/transport.go |
| +++ b/transport.go |
| @@ -46,13 +46,46 @@ type teamBoundTransport struct { |
| } |
| func (t *teamBoundTransport) RoundTrip(req *http.Request) (*http.Response, error) { |
| - if err := CheckSiblingHost(t.team, req.URL.Hostname()); err != nil { |
| - // Before the transport dials, so a refused host costs no connection. |
| + // Before the transport dials, so a refused host costs no connection. |
| + urlHost := req.URL.Hostname() |
| + if err := CheckSiblingHost(t.team, urlHost); err != nil { |
| return nil, err |
| } |
| + |
| + // req.Host is a second authority field. Go dials req.URL.Host but sends |
| + // req.Host as the Host header, so checking only the URL leaves a request |
| + // that reaches our own team's instance while announcing itself as another |
| + // team's — and a platform router that routes on Host could hand the body |
| + // and the consumer token to that other team. |
| + if req.Host != "" { |
| + override := hostnameOnly(req.Host) |
| + if err := CheckSiblingHost(t.team, override); err != nil { |
| + return nil, fmt.Errorf("suite: Host header %q: %w", req.Host, err) |
| + } |
| + if !sameHost(override, urlHost) { |
| + return nil, fmt.Errorf("suite: Host header %q does not match the URL host %q", override, urlHost) |
| + } |
| + } |
| + |
| return t.next.RoundTrip(req) |
| } |
| +// hostnameOnly strips a port if one is present. net.SplitHostPort errors on a |
| +// bare hostname, which is the common case, so its failure is not interesting. |
| +func hostnameOnly(hostport string) string { |
| + if h, _, err := net.SplitHostPort(hostport); err == nil { |
| + return h |
| + } |
| + return hostport |
| +} |
| + |
| +func sameHost(a, b string) bool { |
| + norm := func(s string) string { |
| + return strings.TrimSuffix(strings.ToLower(s), ".") |
| + } |
| + return norm(a) == norm(b) |
| +} |
| + |
| // CheckSiblingHost answers whether `host` is a sibling of `team`'s instances. |
| // |
| // It is exported because it guards more than the HTTP client: §7.1's hand-off |
| @@ -84,8 +117,45 @@ func CheckSiblingHost(team, host string) error { |
| return fmt.Errorf("suite: host %q is not <team>.<app>.%s", host, SuiteDomain) |
| } |
| + // Counting labels is not enough: `acme..oficina.works` counts two and |
| + // names no app, and `acme.docs。evil.oficina.works` counts two only |
| + // because an ideographic full stop is not a separator *here* — IDNA |
| + // normalisation elsewhere in the stack may decide it is one. |
| + for _, label := range labels { |
| + if err := validLabel(label); err != nil { |
| + return fmt.Errorf("suite: host %q: %w", host, err) |
| + } |
| + } |
| + |
| if labels[0] != strings.ToLower(team) { |
| return fmt.Errorf("suite: host %q is team %q, not %q", host, labels[0], team) |
| } |
| return nil |
| } |
| + |
| +// validLabel holds a hostname label to plain ASCII letters, digits and inner |
| +// hyphens (RFC 1123). Anything else is refused rather than interpreted: a |
| +// label this code and the platform's router might read differently is exactly |
| +// the ambiguity the team boundary cannot afford. |
| +func validLabel(label string) error { |
| + if label == "" { |
| + return fmt.Errorf("empty label") |
| + } |
| + if len(label) > 63 { |
| + return fmt.Errorf("label %q is longer than 63 octets", label) |
| + } |
| + if label[0] == '-' || label[len(label)-1] == '-' { |
| + return fmt.Errorf("label %q begins or ends with a hyphen", label) |
| + } |
| + for i := 0; i < len(label); i++ { |
| + c := label[i] |
| + switch { |
| + case c >= 'a' && c <= 'z', |
| + c >= '0' && c <= '9', |
| + c == '-': |
| + default: |
| + return fmt.Errorf("label %q contains %q, which is not a hostname character", label, string(c)) |
| + } |
| + } |
| + return nil |
| +} |
diff --git a/transport_test.go b/transport_test.go| index 58d163b..78ee520 100644 |
| --- a/transport_test.go |
| +++ b/transport_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "net" |
| "net/http" |
| + "net/http/httptest" |
| "strings" |
| "testing" |
| ) |
| @@ -74,6 +75,85 @@ func TestSiblingClientReachesSameTeamHost(t *testing.T) { |
| } |
| } |
| +// The Host header is a second authority field, and the guard read only the |
| +// URL. Go sends req.Host as the Host header while dialling req.URL.Host, so a |
| +// request can dial our own team and announce itself as another team's — and a |
| +// platform router that routes on Host could hand the body and the consumer |
| +// token to that other team. Reported by the GPT-6 module review, finding 1. |
| +func TestSiblingClientRefusesForeignHostOverride(t *testing.T) { |
| + d := &recordingDialer{} |
| + c := NewSiblingClient("acme", d.dial) |
| + |
| + req, err := http.NewRequest("POST", "https://acme.docs.oficina.works/suite/s3", nil) |
| + if err != nil { |
| + t.Fatalf("building request: %v", err) |
| + } |
| + req.Host = "other.docs.oficina.works" |
| + |
| + if _, err := c.Do(req); err == nil { |
| + t.Fatal("got nil error for a foreign Host override, want a refusal") |
| + } |
| + if len(d.dialed) != 0 { |
| + t.Errorf("dialled %v, want no connection attempted at all", d.dialed) |
| + } |
| +} |
| + |
| +// A Host override naming our own host is harmless and must still work, so the |
| +// check cannot simply ban the field. |
| +func TestSiblingClientAllowsMatchingHostOverride(t *testing.T) { |
| + d := &recordingDialer{} |
| + c := NewSiblingClient("acme", d.dial) |
| + |
| + req, _ := http.NewRequest("POST", "https://acme.docs.oficina.works/suite/s3", nil) |
| + req.Host = "acme.docs.oficina.works" |
| + |
| + _, _ = c.Do(req) |
| + |
| + if len(d.dialed) != 1 { |
| + t.Errorf("dialled %v, want the request to proceed", d.dialed) |
| + } |
| +} |
| + |
| +// Gate 4's redirect half, run against a real 302 rather than by calling |
| +// CheckRedirect directly. The fixture supplies the hosts, and the dialer |
| +// counts hops: exactly one connection, to the allowed host, and no follow-up |
| +// to the label it was pointed at. Reported by the GPT-6 module review, |
| +// finding 10 — the old test never produced a 3xx and never counted anything. |
| +func TestSiblingClientRefusesAFixtureRedirect(t *testing.T) { |
| + f := loadHostileHosts(t) |
| + |
| + // A server that answers every request with a redirect to another team. |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + http.Redirect(w, r, "http://"+f.Redirect.To+"/suite/s3/doc-1", http.StatusFound) |
| + })) |
| + defer srv.Close() |
| + srvAddr := strings.TrimPrefix(srv.URL, "http://") |
| + |
| + // Every name resolves to the test server, so the only thing stopping a |
| + // second hop is the client's own refusal — not the absence of a host. |
| + var dialed []string |
| + dial := func(ctx context.Context, network, addr string) (net.Conn, error) { |
| + dialed = append(dialed, addr) |
| + return net.Dial(network, srvAddr) |
| + } |
| + |
| + c := NewSiblingClient(f.Team, dial) |
| + resp, err := c.Get("http://" + f.Redirect.From + "/suite/s3/doc-1") |
| + if resp != nil { |
| + resp.Body.Close() |
| + } |
| + |
| + if err == nil { |
| + t.Error("got nil error following a redirect to another team, want a refusal") |
| + } |
| + if len(dialed) != 1 { |
| + t.Fatalf("dialled %v, want exactly one hop — the allowed host, and no follow-up", dialed) |
| + } |
| + if !strings.HasPrefix(dialed[0], f.Redirect.From+":") { |
| + t.Errorf("dialled %q, want the allowed host %q", dialed[0], f.Redirect.From) |
| + } |
| +} |
| + |
| // Redirects are never followed. An allowed host must be reached to be told to |
| // redirect — that connection is legitimate and gate 4 counts follow-ups, not |
| // first hops — but following one would let a sibling walk the client across |