amadan

oficina / suite Public

The harness now catches four more classes of broken server half

Findings 3 and 5 of the module review, both P1, both about the harness
being too easy to pass.

S3 was checked by status alone, so a server could return the required 404
and put the item's name and revision in the body, and every caller reading
the raw response would get them. It could also return a 200 carrying a
different document entirely. Bodies are now compared exactly: the fixture
declares what each item's bytes are, and a refusal must be byte-identical
to the refusal for an id that is not in the fixture at all. That baseline
is the only way to test §4.2's real requirement — "not team-visible" and
"does not exist" must be the same answer — because every case in the
fixture is an item that exists.

S2 was asked once, by one person, at each item's current revision, so
every expected answer reduced to the rung and a server could read neither
the asker nor the revision it was sent. It is now asked three times: the
fixture's asker at the current revision, the same asker one revision
behind (a team item is STALE — readable, but the app of record has moved
on), and a stranger with no team standing, who reaches nothing, the
team-visible items included.

Each is driven by a deliberately broken reference server that must fail,
so the harness is tested rather than trusted. It now catches eight classes
of bug: 403-for-404, a grant beating custody, a visibility flag in the
feed, a secret in a refusal body, the wrong item's bytes, a distinguishable
"never existed", a server ignoring the asker, and one ignoring the revision.

Findings 4, 6 and 7 stay open and are recorded as open in
CODEX-FINDINGS-MODULE.md: the feed's contents are still unasserted, the
fixture is single-pass with no ingest-then-downgrade transition, and the
harness sends no Authorization, Origin or Content-Type — so a server with
real bearer auth fails every check while one with none passes. Finding 7
wants a fixture that can carry consumer credentials, which is a change to
its shape; §12 step 3 says the second implementation corrects the fixtures,
and that is the right moment for it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 20a87ceb44eebce181f77792acbe7dd55684b12e parent 03e0be0
5 files changed, +241 −27
  • CODEX-FINDINGS-MODULE.md +33 −0
  • conformance/conformance.go +51 −8
  • conformance/conformance_test.go +91 −1
  • fixtures.go +29 −9
  • fixtures/custody.json +37 −9

Intent

Attributed to this commit: 213 g CO₂e (26802131 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-module-hardening, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.

full ledger: worktree-module-hardening

diff --git a/CODEX-FINDINGS-MODULE.md b/CODEX-FINDINGS-MODULE.md
index aa36ef5..8bb0924 100644
--- a/CODEX-FINDINGS-MODULE.md
+++ b/CODEX-FINDINGS-MODULE.md
@@ -19,6 +19,39 @@ the module. The repository's own code was never modified by the review.
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
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"
}
]
}