amadan

oficina / suite Public

Merge branch 'worktree-module-hardening' into main

claude pushed by paul@keymail.dev 210312fb06e3b583903d8495e79355105029162a parent af87733parent 20a87ce
17 files changed, +1007 −88
  • CODEX-FINDINGS-MODULE.md +174 −0
  • conformance/conformance.go +51 −8
  • conformance/conformance_test.go +91 −1
  • fixtures.go +29 −9
  • fixtures/custody.json +37 −9
  • fixtures/hostile_hosts.json +7 −1
  • fixtures_test.go +24 −8
  • mutations.py +86 −0
  • s1.go +50 −2
  • s1_test.go +55 −0
  • s2.go +5 −0
  • s2_test.go +143 −39
  • s3_test.go +8 −2
  • strictjson.go +80 −0
  • subject_test.go +15 −7
  • transport.go +72 −2
  • transport_test.go +80 −0
diff --git a/CODEX-FINDINGS-MODULE.md b/CODEX-FINDINGS-MODULE.md
new file mode 100644
index 0000000..8bb0924
--- /dev/null
+++ b/CODEX-FINDINGS-MODULE.md
@@ -0,0 +1,174 @@
+# 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.
+
+## What has been done about it
+
+Fixed, each with a test that fails without the fix: **1** (the `Host`
+override), **2** (duplicate JSON keys), **3** (S3 bodies compared, and a
+never-existent id as the refusal baseline), **5** (three S2 questions — the
+asker, the asker one revision behind, and a person with no team standing),
+**8** (`DecodeS1` rejects nulls, missing fields, trailing content and
+duplicate keys), **9** (RFC 1123 label validation), **10** (gate 4's
+redirect half runs against a real 302 through a counting dialer, using the
+fixture case that was never read), **11**, **12**, **13** and **14** (the
+tests that could not fail).
+
+`mutations.py` re-runs all nine regressions, including the three the review
+did not name. All nine are caught.
+
+**Still open, and honestly so:**
+
+- **Finding 4.** The harness asserts the feed parses, not what is in it. An
+ always-empty feed still passes, and the reference server still announces
+ every fixture id as `gone`, which exposes the ids of items that were
+ never team-visible.
+- **Finding 6.** The fixture is single-pass and captures final states only.
+ There is no ingest-then-downgrade transition, so a server caching a
+ positive answer across requests passes.
+- **Finding 7.** The harness sends no Authorization, Origin or JSON
+ Content-Type, and never uses `fixture.Team`. A server with correct bearer
+ auth fails every check; a server with no auth at all passes.
+
+Finding 7 in particular wants a fixture that can carry consumer credentials,
+which is a change to the fixture's shape rather than a patch. Left for the
+first app that adopts the module, per §12 step 3 — the second implementation
+is what corrects the fixtures.
+
+---
+
+## 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/conformance/conformance.go b/conformance/conformance.go
index d2ce631..3fcc01e 100644
--- a/conformance/conformance.go
+++ b/conformance/conformance.go
@@ -66,6 +66,15 @@ func Check(h http.Handler) Report {
// the same 404 as a deleted item — a consumer that could tell "not
// team-visible" from "does not exist" would learn that a private item exists.
func checkS3(h http.Handler, f suite.CustodyFixture, r *Report) {
+ // The refusal every non-team item must be indistinguishable from. Taken
+ // from an id that is not in the fixture at all, so it is the server's
+ // answer for "no such item" rather than anything the fixture arranged.
+ baseline := do(h, "GET", suite.RouteS3+"/"+suite.NonexistentItemID+"?revision=1", nil)
+ r.Checked++
+ if baseline.Code != 404 {
+ r.fail("S3 for an item that does not exist: got %d, want 404", baseline.Code)
+ }
+
for _, c := range f.Cases {
url := suite.RouteS3 + "/" + c.ItemID + "?revision=" + strconv.FormatInt(c.Revision, 10)
rec := do(h, "GET", url, nil)
@@ -73,9 +82,29 @@ func checkS3(h http.Handler, f suite.CustodyFixture, r *Report) {
if rec.Code != c.S3Status {
r.fail("S3 %s (%s, rung %s): got %d, want %d", c.ItemID, c.Name, c.Rung, rec.Code, c.S3Status)
+ continue
}
if rec.Code == 403 {
r.fail("S3 %s: answered 403, and the contract is 404 never 403 — a 403 says the item exists, which is the fact custody hides", c.ItemID)
+ continue
+ }
+
+ body := rec.Body.String()
+ if c.S3Status == 200 {
+ // The item's own bytes, not merely some bytes. A 200 carrying a
+ // different item is a leak that a status check cannot see.
+ if body != c.S3Body {
+ r.fail("S3 %s: body %q, want %q", c.ItemID, body, c.S3Body)
+ }
+ continue
+ }
+
+ // A refusal must be byte-identical to the refusal for an item that
+ // never existed. Anything a server adds here — a reason, a name, a
+ // revision — tells the caller the item is real.
+ if body != baseline.Body.String() {
+ r.fail("S3 %s: refusal body %q differs from the refusal for an item that does not exist (%q), so the answer reveals that this item exists",
+ c.ItemID, body, baseline.Body.String())
}
}
}
@@ -85,19 +114,33 @@ func checkS3(h http.Handler, f suite.CustodyFixture, r *Report) {
// deleted one: the custody intersection (§4.3) has to beat the grant, or a
// consumer holding an ingested id learns the item is still there.
func checkS2(h http.Handler, f suite.CustodyFixture, r *Report) {
- pairs := make([]suite.ItemRevision, 0, len(f.Cases))
- want := make(map[string]suite.Authorization, len(f.Cases))
+ // Three questions, not one. A single question at the current revision for
+ // a single person lets a server answer from the rung alone and read
+ // neither the asker nor the revision it was sent.
+ current := make([]suite.ItemRevision, 0, len(f.Cases))
+ behind := make([]suite.ItemRevision, 0, len(f.Cases))
for _, c := range f.Cases {
- pairs = append(pairs, suite.ItemRevision{ItemID: c.ItemID, Revision: c.Revision})
- want[c.ItemID] = c.S2Result
+ current = append(current, suite.ItemRevision{ItemID: c.ItemID, Revision: c.Revision})
+ behind = append(behind, suite.ItemRevision{ItemID: c.ItemID, Revision: c.Revision - 1})
}
+ askS2(h, f, r, "the fixture's asker, at the current revision", f.Asker, current,
+ func(c suite.CustodyCase) suite.Authorization { return c.S2Result })
+
+ askS2(h, f, r, "the fixture's asker, one revision behind", f.Asker, behind,
+ func(c suite.CustodyCase) suite.Authorization { return c.S2ResultSuperseded })
+
+ askS2(h, f, r, "a person with no team standing", f.Stranger, current,
+ func(c suite.CustodyCase) suite.Authorization { return c.S2ResultStranger })
+}
+
+func askS2(h http.Handler, f suite.CustodyFixture, r *Report, what string, asker suite.Subject, pairs []suite.ItemRevision, want func(suite.CustodyCase) suite.Authorization) {
if err := suite.CheckS2Batch(pairs); err != nil {
r.fail("custody fixture is over the S2 cap: %v", err)
return
}
- body, err := json.Marshal(suite.S2Request{Asker: f.Asker, Pairs: pairs})
+ body, err := json.Marshal(suite.S2Request{Asker: asker, Pairs: pairs})
if err != nil {
r.fail("encoding the S2 question: %v", err)
return
@@ -106,10 +149,10 @@ func checkS2(h http.Handler, f suite.CustodyFixture, r *Report) {
rec := do(h, "POST", suite.RouteS2, body)
got := suite.DecodeS2(pairs, rec.Code, rec.Body.Bytes())
- for i, p := range pairs {
+ for i, c := range f.Cases {
r.Checked++
- if got[i] != want[p.ItemID] {
- r.fail("S2 %s: got %s, want %s", p.ItemID, got[i], want[p.ItemID])
+ if got[i] != want(c) {
+ r.fail("S2 %s asked for %s: got %s, want %s", c.ItemID, what, got[i], want(c))
}
}
}
diff --git a/conformance/conformance_test.go b/conformance/conformance_test.go
index ddd0ccb..acc53b7 100644
--- a/conformance/conformance_test.go
+++ b/conformance/conformance_test.go
@@ -21,6 +21,10 @@ type referenceServer struct {
forbidInsteadOfNotFound bool // answers 403 where the contract says 404
honourGrantOverCustody bool // answers VISIBLE for a granted item
leakVisibilityInFeed bool // adds a field the feed row may not carry
+ leakBodyInRefusal bool // right status, and the item's secrets in the body
+ wrongBodyForTeamItem bool // 200, carrying a different item's bytes
+ tellExistenceApart bool // a different refusal for "never existed"
+ ignoreAskerAndRevision bool // VISIBLE for any asker, at any revision
}
func (s *referenceServer) find(itemID string) (suite.CustodyCase, bool) {
@@ -50,11 +54,21 @@ func (s *referenceServer) serveS3(w http.ResponseWriter, r *http.Request) {
c, ok := s.find(id)
if !ok {
w.WriteHeader(404)
+ if s.tellExistenceApart {
+ // A different refusal for something that never existed lets a
+ // consumer separate "no such item" from "not yours", which is
+ // exactly what §4.2 forbids.
+ w.Write([]byte("no such item"))
+ }
return
}
if c.Rung == suite.RungTeam {
w.WriteHeader(200)
- w.Write([]byte("body of " + id))
+ if s.wrongBodyForTeamItem {
+ w.Write([]byte("the bytes of some other document entirely"))
+ return
+ }
+ w.Write([]byte(c.S3Body))
return
}
if s.forbidInsteadOfNotFound {
@@ -62,6 +76,10 @@ func (s *referenceServer) serveS3(w http.ResponseWriter, r *http.Request) {
return
}
w.WriteHeader(404)
+ if s.leakBodyInRefusal {
+ // The status the contract demands, and the secret in the body anyway.
+ w.Write([]byte("refused: " + c.Name + " at revision " + strconv.FormatInt(c.Revision, 10)))
+ }
}
func (s *referenceServer) serveS2(w http.ResponseWriter, r *http.Request) {
@@ -80,14 +98,32 @@ func (s *referenceServer) serveS2(w http.ResponseWriter, r *http.Request) {
Results []result `json:"results"`
}{}
+ stranger := req.Asker.Value != s.fixture.Asker.Value
+
for _, p := range req.Pairs {
c, ok := s.find(p.ItemID)
answer := suite.NOT_VISIBLE
switch {
case !ok:
answer = suite.NOT_VISIBLE
+
+ case s.ignoreAskerAndRevision:
+ // Answers from the rung alone: any asker, any revision. This is
+ // the shape a server takes when nobody ever varied either.
+ if c.Rung == suite.RungTeam {
+ answer = suite.VISIBLE
+ }
+
+ case stranger:
+ // No team standing reaches nothing, the team rung included.
+ answer = suite.NOT_VISIBLE
+
+ case c.Rung == suite.RungTeam && p.Revision != c.Revision:
+ answer = suite.STALE
+
case c.Rung == suite.RungTeam:
answer = suite.VISIBLE
+
case s.honourGrantOverCustody && c.Rung == suite.RungGranted:
// The bug the intersection exists to prevent: answering from the
// grants join alone, so a grantee learns a non-team item exists.
@@ -164,6 +200,60 @@ func TestHarnessCatchesGrantBeatingCustody(t *testing.T) {
}
}
+// Status alone is not enough. A server can return the required 404 and put
+// the item's secrets in the body, and every caller that reads the raw response
+// gets them. GPT-6 module review, finding 3.
+func TestHarnessCatchesASecretInARefusalBody(t *testing.T) {
+ s := newReference(t)
+ s.leakBodyInRefusal = true
+
+ report := conformance.Check(s)
+
+ if len(report.Failures) == 0 {
+ t.Error("harness passed a server leaking the item in its 404 body")
+ }
+}
+
+// A 200 carrying the wrong item's bytes is a leak in the other direction, and
+// checking only the status misses it.
+func TestHarnessCatchesTheWrongBodyForATeamItem(t *testing.T) {
+ s := newReference(t)
+ s.wrongBodyForTeamItem = true
+
+ report := conformance.Check(s)
+
+ if len(report.Failures) == 0 {
+ t.Error("harness passed a server returning another item's bytes")
+ }
+}
+
+// "Not team-visible" and "does not exist" must be the same answer, and no
+// fixture case can prove it — every case is an item that exists.
+func TestHarnessCatchesDistinguishableNonexistence(t *testing.T) {
+ s := newReference(t)
+ s.tellExistenceApart = true
+
+ report := conformance.Check(s)
+
+ if len(report.Failures) == 0 {
+ t.Error("harness passed a server whose refusal reveals whether the item exists")
+ }
+}
+
+// S2 carries a named asker and a revision, and a server can pass a
+// status-and-rung harness while reading neither. GPT-6 module review,
+// finding 5.
+func TestHarnessCatchesAServerIgnoringAskerAndRevision(t *testing.T) {
+ s := newReference(t)
+ s.ignoreAskerAndRevision = true
+
+ report := conformance.Check(s)
+
+ if len(report.Failures) == 0 {
+ t.Error("harness passed a server that answers from the rung alone, reading neither asker nor revision")
+ }
+}
+
// The leak memoria's draft 1 shipped: a visibility flag on a feed row.
func TestHarnessCatchesAFeedRowCarryingVisibility(t *testing.T) {
s := newReference(t)
diff --git a/fixtures.go b/fixtures.go
index 4541ab9..1e63ad5 100644
--- a/fixtures.go
+++ b/fixtures.go
@@ -41,21 +41,41 @@ const (
// CustodyCase is one row of the custody fixture: an item, where it sits, and
// what both calls must answer for the fixture's asker.
type CustodyCase struct {
- Name string `json:"name"`
- ItemID string `json:"item_id"`
- Revision int64 `json:"revision"`
- Rung Rung `json:"rung"`
- Link string `json:"link"`
- S3Status int `json:"s3_status"`
+ Name string `json:"name"`
+ ItemID string `json:"item_id"`
+ Revision int64 `json:"revision"`
+ Rung Rung `json:"rung"`
+ Link string `json:"link"`
+ S3Status int `json:"s3_status"`
+ // S3Body is the exact body a content fetch must return: the item's bytes
+ // for a 200, and empty for every refusal. Checking the status alone let a
+ // server return the required 404 while including the item's secrets in the
+ // body, or return a 200 carrying a different item entirely.
+ S3Body string `json:"s3_body"`
S2Result Authorization `json:"s2_result"`
+ // S2ResultSuperseded is the answer when the same item is asked about one
+ // revision behind. A team item is STALE — readable, but the app of record
+ // has moved on — and anything custody already refuses stays refused.
+ S2ResultSuperseded Authorization `json:"s2_result_superseded"`
+ // S2ResultStranger is the answer for a person with no team standing.
+ S2ResultStranger Authorization `json:"s2_result_stranger"`
}
+// NonexistentItemID is never in the fixture. A refusal for an item that does
+// not exist has to be byte-for-byte the refusal for one that exists and is not
+// team-visible — that indistinguishability is the whole of §4.2, and no case
+// in the fixture can test it, because every case is an item that exists.
+const NonexistentItemID = "doc-no-such-item-ever"
+
// CustodyFixture is gate 3's world: one case per rung, plus team-plus-link and
// the after-ingest transitions the reviews asked for.
type CustodyFixture struct {
- Team string `json:"team"`
- Asker Subject `json:"asker"`
- Cases []CustodyCase `json:"cases"`
+ Team string `json:"team"`
+ Asker Subject `json:"asker"`
+ // Stranger has no team standing. Every case refuses them, the team-visible
+ // ones included: team standing is what makes a team item readable.
+ Stranger Subject `json:"stranger"`
+ Cases []CustodyCase `json:"cases"`
}
// LoadCustodyFixture returns the golden custody cases every app runs.
diff --git a/fixtures/custody.json b/fixtures/custody.json
index 6d5865e..fb10c04 100644
--- a/fixtures/custody.json
+++ b/fixtures/custody.json
@@ -18,88 +18,116 @@
"team": "acme",
"asker": { "kind": "person_ref", "value": "pers_ana" },
+ "stranger": { "kind": "person_ref", "value": "pers_bo" },
"cases": [
{
"name": "team rung is the only thing custody admits",
"item_id": "doc-team",
+ "s3_body": "the Q3 plan, shared with the whole team",
"revision": 7,
"rung": "team",
"link": null,
"s3_status": 200,
- "s2_result": "VISIBLE"
+ "s2_result": "VISIBLE",
+ "s2_result_superseded": "STALE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "private, and the asker owns it",
"item_id": "doc-private",
+ "s3_body": "",
"revision": 4,
"rung": "private",
"link": null,
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "granted to the asker individually",
"item_id": "doc-granted",
+ "s3_body": "",
"revision": 9,
"rung": "granted",
"link": null,
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "granted to a guest only",
"item_id": "doc-guest",
+ "s3_body": "",
"revision": 2,
"rung": "granted",
"link": null,
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "link-shared but not team-shared: a link is not a rung",
"item_id": "doc-link",
+ "s3_body": "",
"revision": 5,
"rung": "private",
"link": "read",
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "team-shared and link-shared: team-visible because of the rung",
"item_id": "doc-team-link",
+ "s3_body": "the launch checklist, team-shared and link-shared",
"revision": 3,
"rung": "team",
"link": "read",
"s3_status": 200,
- "s2_result": "VISIBLE"
+ "s2_result": "VISIBLE",
+ "s2_result_superseded": "STALE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "deleted, with a former grant held by the asker",
"item_id": "doc-deleted",
+ "s3_body": "",
"revision": 6,
"rung": "deleted",
"link": null,
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "ingested on the team rung, since moved to private",
"item_id": "doc-was-team",
+ "s3_body": "",
"revision": 8,
"rung": "private",
"link": null,
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
},
{
"name": "a link added after ingest changes nothing either way",
"item_id": "doc-link-added-late",
+ "s3_body": "",
"revision": 11,
"rung": "private",
"link": "read",
"s3_status": 404,
- "s2_result": "NOT_VISIBLE"
+ "s2_result": "NOT_VISIBLE",
+ "s2_result_superseded": "NOT_VISIBLE",
+ "s2_result_stranger": "NOT_VISIBLE"
}
]
}
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