mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
fix: four codex round-16 findings, all four real and pinned (TASK-2878)
Round 16 (internal/server) returned five findings. Four are real, confirmed by a test that FAILS against the build without its own fix and passes with it; the fifth is a true observation whose proposed remedy closes nothing, and is recorded rather than fixed. Every test ships with an owner control leg, so none of them passes against a build that simply refuses or drops everything. 1. BULK MOVE NEVER RAN THE SUPPLIED-HALF VISIBILITY CHECK (real, fixed). Round 11 established that `req.Status` on a bulk collection move is CALLER INPUT and wired `suppliedByCaller` so the store classifier would REFUSE an unresolvable value rather than drop it. It did not carry across the other half of the supplied contract: the store resolver cannot answer a request-scoped question, which is why the single move door, the copy and the preflight all call refuseInvisibleRelationOverrides first. Bulk move was the only one of the three MigrateRelationReferents call sites that never did. So a caller who cannot see the relation's target collection could name a live item in it and have the value stored, receiving its canonical id back. Mutant: remove the refuseInvisibleRelationOverrides block -> FAIL, "a caller who cannot see People pointed a relation at one of its items: 21c93e0b-8340-423c-a528-4eb78340bbf5". 2. THE PREFLIGHT'S CARRIED DROPS NEVER PASSED THE VISIBILITY COLLAPSE (real, fixed). Round 15 hoisted the collapse so ResolveLateRelationDefaults' issues stop naming live items to callers who cannot see them. MigrateRelationReferents returns its issues down a SECOND path — `relationDropReason`, rendered straight into `fields.dropped[].reason` — and that path was never collapsed. `wrong_collection` is the reason that names a LIVE item, so a hidden-but-live target reported differently from a value naming nothing. Class: this is the ONLY site where a MigrateRelationReferents drop reason reaches a caller. Move and bulk move report dropped KEYS and no reasons, so the class is one site, not five, and the grep says so rather than the assumption. Mutant: remove the collapse -> FAIL, "a LIVE hidden item reports 'wrong_collection' and a nonexistent value reports 'not_found'". 3. THE STALE CARRIED SET IS REAL AFTER ALL, AND ROUND 15's CLEARANCE WAS MY INSTRUMENT'S FAULT (real, fixed, and it reversesace2d3f5). Round 15 reported this; every mutant survived; it shipped as a documented robustness change; the lead ruled it out as unobservable and I reverted it inace2d3f5. The ruling required a concrete request under which the two sets differ before it could be re-reported. Round 16 supplied one, and the test fails against the unfixed build: "carried":[{"key":"owner_ref","type":"relation", "value":"a8fdfa7a-3487-4acb-9ad4-53b67cd96a19","from":"default"}] with valid:true and no dropped row — a live item in a collection the caller cannot see. WHY EVERY MUTANT SURVIVED, which is the part worth keeping: my probe used a source value that RESOLVED. The relation pass never drops a resolvable value, so the pre-drop and post-drop carried sets were identical BY CONSTRUCTION and the probe could not have failed. That is the same defect as the fixture I recorded yesterday — two outcomes with identical bytes — with the instrument being a probe rather than a test, which is why the CONVE-30 habit did not catch it. The discriminating shape is a non-nil DANGLING source value plus a required destination default naming a hidden item. The fix is NOT the one that was dropped. That was 71 lines across three files with a new exported store helper. This is one expression at the call site — store.CarriedSourceValues(currentFields, migrated.Dropped) — because the loop above already extended migrated.Dropped with the relation pass's drops. No new export, no helper, and it makes the preflight agree with move and bulk move BY CONSTRUCTION rather than by argument, which is what the lead's ruling was right to object to in the first version. 4. A NULL OVERRIDE DID NOT CLEAR A STALE NON-NIL CARRIED KEY (real, fixed). `notDefaultKeys` learned in round 14 that a nil value is not a value, and skipped nils PER MAP. A caller who nulls a key whose STORED value is non-nil still had it counted — out of `carried`, on the strength of a value the request had just discarded — so the destination default injected in its place was exempted from the visibility check. The relation pass does not rescue this: an override of nil leaves nothing to resolve, so the key is never dropped and never leaves the carried set. The existing null-source test cannot reach it; that fixture has no stored value to go stale. Fixed at the shared helper, so move, bulk move and preflight get it at once. Mutant: drop the explicit-nil-in-supplied clause -> FAIL, the move stores and returns 550d947c-5970-4d61-9db2-d8e22c8a25a1 to a caller who cannot see People. 5. CANONICALISING A CARRIED REF DISCLOSES A HIDDEN TARGET'S UUID (true observation, NOT fixed, and the reason is a measurement rather than a judgement). A carried ref naming a live item in a hidden collection canonicalises to its UUID and survives; a dangling one drops. The caller can therefore tell the two apart, and the reviewer asked for the response to redact the UUID. Redacting the response closes nothing. The canonical UUID is written into the stored blob of an item the caller CAN read, so the same bytes come back from the ordinary read door: probed as the same restricted editor, GET of the moved item returned 200 with the UUID present. A response-only fix would move the disclosure one request to the left. The remedies that would actually close it are not this unit's to make: stop canonicalising carried values, or drop carried values by visibility — and the second is precisely the data destruction this unit's carry rule exists to avoid, since it would silently delete a valid relation because the MOVER cannot see its target. Recorded as an open design question alongside IDEA-2886. Gates on this tip: build, go vet, gofmt clean; go test ./internal/server -run 'TestRelationDoors|TestCopyEndpoint|TestCopyPreflight|TestImportArtifact' ok 44.309s. The full SQLite suite and Postgres are owed on this tip and are NOT claimed here. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
This commit is contained in:
@@ -788,6 +788,22 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m
|
||||
// overrides on this path — only `status`, merged above — so every relation
|
||||
// value here is CARRIED, and nothing on this door can refuse. Passing nil
|
||||
// for `supplied` says that rather than leaving it implied.
|
||||
// The supplied half owes the visibility check, exactly as at the single
|
||||
// move door. Round 11 made `status` supplied so the store classifier
|
||||
// would REFUSE an unresolvable value rather than drop it; it did not
|
||||
// carry across the other half of the supplied contract, which the store
|
||||
// resolver structurally cannot provide — see
|
||||
// refuseInvisibleRelationOverrides. This was the only one of the three
|
||||
// MigrateRelationReferents call sites without it, so a caller who cannot
|
||||
// see the target collection could name a live item in it and have the
|
||||
// value stored (codex round 16).
|
||||
if invisible, err := s.refuseInvisibleRelationOverrides(
|
||||
r, workspaceID, workspaceRole(r), items.SchemaForMigratedFields(targetSchema),
|
||||
suppliedByCaller); err != nil {
|
||||
return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"}
|
||||
} else if len(invisible) > 0 {
|
||||
return nil, &bulkOpError{message: relationIssuesMessage(invisible), code: "validation_error"}
|
||||
}
|
||||
relRefusals, relDropped, relErr := s.store.MigrateRelationReferents(
|
||||
workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields,
|
||||
suppliedByCaller, store.CarriedSourceValues(currentFields, result.Dropped),
|
||||
|
||||
@@ -782,6 +782,21 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request)
|
||||
// `origin` loses its entry for the same reason: if a default does
|
||||
// re-populate the key, its origin is the destination's default, not the
|
||||
// source value that was just discarded.
|
||||
// The CARRIED drops reach the caller too — `relationDropReason` below is
|
||||
// rendered straight into `fields.dropped[].reason` — so they owe the same
|
||||
// visibility collapse the late-default drops get a few lines down.
|
||||
// `wrong_collection` is the reason that names a LIVE item, so without
|
||||
// this a carried value pointing at one the caller cannot see reported
|
||||
// differently from a value naming nothing: the existence oracle round 3
|
||||
// closed on the main pass, reached by the carried path (codex round 16).
|
||||
//
|
||||
// This is the only site where a MigrateRelationReferents drop reason
|
||||
// reaches a caller — move and bulk move report dropped KEYS and no
|
||||
// reasons — so the class is one site, not five.
|
||||
if cerr := s.collapseInvisibleRelationIssues(r, dst.WorkspaceID(), dst.Role, relDropped); cerr != nil {
|
||||
writeInternalError(w, fmt.Errorf("copy preflight: carried relation visibility: %w", cerr))
|
||||
return
|
||||
}
|
||||
relationDropReason := make(map[string]string, len(relDropped))
|
||||
for _, ri := range relDropped {
|
||||
migrated.Dropped = append(migrated.Dropped, ri.Key)
|
||||
@@ -823,7 +838,23 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request)
|
||||
}
|
||||
invisibleDefaults, invErr := s.dropInvisibleRelationDefaults(r, dst.WorkspaceID(), dst.Role,
|
||||
items.SchemaForMigratedFields(targetSchema), final,
|
||||
notDefaultKeys(input.FieldOverrides, carriedSource))
|
||||
// `carriedSource` is the PRE-relation-pass snapshot and is the wrong
|
||||
// input here. It is right for the origin label and for the relation
|
||||
// classifier, which ask "did this value come across from the source?"
|
||||
// — a different question from "is the value in hand now a destination
|
||||
// default?". Recomputing against `migrated.Dropped`, which the loop
|
||||
// above extended with the relation pass's drops, answers the second
|
||||
// one: a key that pass dropped is no longer carried, so the default
|
||||
// ValidateFields put in its place is checked rather than exempted.
|
||||
//
|
||||
// Round 15 reported this and every mutant survived, because the probe
|
||||
// used a source value that RESOLVED — which the relation pass never
|
||||
// drops, so the two sets were identical by construction. Round 16
|
||||
// supplied the shape that differs: a non-nil DANGLING source value,
|
||||
// under which the unfixed form hands a caller who cannot see the
|
||||
// target collection the id of the item the default names.
|
||||
notDefaultKeys(input.FieldOverrides,
|
||||
store.CarriedSourceValues(currentFields, migrated.Dropped)))
|
||||
if invErr != nil {
|
||||
writeInternalError(w, fmt.Errorf("copy preflight: relation default visibility: %w", invErr))
|
||||
return
|
||||
|
||||
@@ -801,3 +801,50 @@ func newCopyRelationFixtureIncompatibleSource(t *testing.T) *relationFixture {
|
||||
f.source = src
|
||||
return f
|
||||
}
|
||||
|
||||
// A carried key the RELATION pass dropped must stop counting as carried when
|
||||
// the destination default that replaces it is visibility-checked (codex round
|
||||
// 16, the concrete case round 15 could not produce).
|
||||
//
|
||||
// The preflight computes `carriedSource` from MigrateFields' drops, then runs
|
||||
// MigrateRelationReferents, which drops MORE keys — and hands the
|
||||
// default-visibility call the set captured BEFORE that second pass. A key the
|
||||
// relation pass dropped therefore still reads as "carried", and
|
||||
// `notDefaultKeys` exempts it from the visibility check. When ValidateFields
|
||||
// then refills that very key with the destination schema's default, the
|
||||
// default is exempted on the strength of a value that is no longer there.
|
||||
//
|
||||
// Round 15 reported this shape and every mutant survived, so it shipped as a
|
||||
// robustness change and was then REVERTED by lead ruling as unobservable. The
|
||||
// probe that cleared it used a source value that RESOLVED — which the relation
|
||||
// pass never drops, so the two sets were identical by construction and the
|
||||
// probe could not have failed. THIS fixture uses a non-nil DANGLING source
|
||||
// value, which is the only shape that makes the two sets differ.
|
||||
func TestCopyPreflight_RelationDroppedCarriedKeyDoesNotExemptTheDefault(t *testing.T) {
|
||||
dangling := badRef
|
||||
f := newCopyRelationFixtureWith(t, resolvableDestDefault, &dangling, true)
|
||||
|
||||
// An editor who can see the source and the destination collections, but
|
||||
// NOT People B — the collection the destination's default points into.
|
||||
blind := f.restrictedEditor("blind-carried-default@example.com", "blindcarrieddef",
|
||||
[]string{f.collA.ID}, []string{f.collB.ID})
|
||||
|
||||
rr := f.call(blind, reqOpts{wsRoleCtx: "editor"}, f.baseBody())
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("preflight: got %d, want 200: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
if strings.Contains(rr.Body.String(), f.targetB.ID) {
|
||||
t.Fatalf("the preflight handed a caller who cannot see People B the id of the item "+
|
||||
"its default names: %s", rr.Body.String())
|
||||
}
|
||||
|
||||
// Control: the OWNER can see People B, so the same preflight resolves the
|
||||
// default and carries its canonical id. Without this leg the test passes
|
||||
// against a build that dropped every destination default.
|
||||
pre := f.ok(f.baseBody())
|
||||
if v, carried := carriedValue(pre, "owner_ref"); !carried || v != f.targetB.ID {
|
||||
t.Fatalf("the owner's identical preflight did not resolve the destination default "+
|
||||
"(%#v, carried=%v, want %q); the omission above is not visibility-dependent",
|
||||
v, carried, f.targetB.ID)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -331,6 +331,15 @@ func (s *Server) refuseInvisibleRelationOverrides(
|
||||
// and counting it here excludes it from the very check it needs (codex round
|
||||
// 14). Third time nil has done this in one unit: rounds 4 and 5 were the same
|
||||
// distinction in the origin label.
|
||||
//
|
||||
// An EXPLICIT nil in `supplied` beats a non-nil `carried`, and that is a
|
||||
// separate rule from the one above rather than a restatement of it (codex
|
||||
// round 16). Round 14 skipped nils PER MAP, so a caller who nulled a key
|
||||
// whose stored value was non-nil still had it counted — out of `carried`,
|
||||
// on the strength of a value the request had just discarded. The request is
|
||||
// the later statement about that key: it says there is no value here, so
|
||||
// whatever ValidateFields puts in its place is a default and owes the
|
||||
// visibility check.
|
||||
func notDefaultKeys(supplied, carried map[string]any) map[string]bool {
|
||||
out := make(map[string]bool, len(supplied)+len(carried))
|
||||
for _, m := range []map[string]any{supplied, carried} {
|
||||
@@ -338,6 +347,9 @@ func notDefaultKeys(supplied, carried map[string]any) map[string]bool {
|
||||
if v == nil {
|
||||
continue
|
||||
}
|
||||
if sv, overridden := supplied[k]; overridden && sv == nil {
|
||||
continue
|
||||
}
|
||||
out[k] = true
|
||||
}
|
||||
}
|
||||
|
||||
@@ -152,6 +152,24 @@ func (f *doorFixture) storedRelation(itemID string) (any, bool) {
|
||||
return v, ok
|
||||
}
|
||||
|
||||
// storedRelationKey is storedRelation for a key other than owner_ref. Same
|
||||
// reason it reads the database rather than the response: a handler that
|
||||
// echoed the right thing while writing the wrong thing would pass a
|
||||
// response-only check.
|
||||
func (f *doorFixture) storedRelationKey(itemID, key string) (any, bool) {
|
||||
f.t.Helper()
|
||||
it, err := f.srv.store.GetItem(itemID)
|
||||
if err != nil || it == nil {
|
||||
f.t.Fatalf("GetItem(%s): %v", itemID, err)
|
||||
}
|
||||
var m map[string]any
|
||||
if err := json.Unmarshal([]byte(it.Fields), &m); err != nil {
|
||||
f.t.Fatalf("parse fields %q: %v", it.Fields, err)
|
||||
}
|
||||
v, ok := m[key]
|
||||
return v, ok
|
||||
}
|
||||
|
||||
// badRef is shaped like a real issue ref and names nothing. Not a slug and not
|
||||
// free text: those are refused by a DIFFERENT rule (the resolver has no slug
|
||||
// fallback), and a fixture rejectable two ways discriminates nothing.
|
||||
@@ -1154,3 +1172,225 @@ func TestRelationDoors_LateDefaultWrongCollectionDoesNotDiscloseExistence(t *tes
|
||||
t.Fatalf("the late-default refusal handed back the hidden item's canonical id: %s", hidden)
|
||||
}
|
||||
}
|
||||
|
||||
// A BULK move owes its supplied half the same visibility check the single
|
||||
// move door runs (codex round 16).
|
||||
//
|
||||
// Round 11 established that `req.Status` on a bulk collection move is CALLER
|
||||
// INPUT and wired `suppliedByCaller` so the store classifier would refuse an
|
||||
// unresolvable value instead of dropping it. What that round did not carry
|
||||
// across is the OTHER half of the supplied contract: the single move door,
|
||||
// the copy, and the preflight all call refuseInvisibleRelationOverrides
|
||||
// before handing the values to the store, because the store resolver cannot
|
||||
// answer a request-scoped question. Bulk move is the fourth migrate door and
|
||||
// the only one that never called it — the N+1th site of a rule three doors
|
||||
// already applied.
|
||||
//
|
||||
// So a caller who cannot see the relation's target collection could name a
|
||||
// live item in it and have the value stored, receiving its canonical id back.
|
||||
func TestRelationDoors_BulkMoveRefusesInvisibleSuppliedStatus(t *testing.T) {
|
||||
f := newDoorFixture(t)
|
||||
dst := mustSchemaCollection(t, f.srv, f.ws.ID, "Bulk Status Relation", fmt.Sprintf(`{"fields":[
|
||||
{"key":"status","label":"Status","type":"relation","collection":%q}
|
||||
]}`, f.people.Slug))
|
||||
|
||||
blind := mustUser(t, f.srv, "blind-bulk-status@example.com", "blindbulkstatus", "")
|
||||
if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
// Sees the source and the destination; NOT People.
|
||||
if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific",
|
||||
[]string{f.tasks.ID, dst.ID}); err != nil {
|
||||
t.Fatalf("SetMemberCollectionAccess: %v", err)
|
||||
}
|
||||
|
||||
item := f.seed(`{"status":"open"}`)
|
||||
rr := f.callAs(blind, "editor", f.srv.handleBulkItems, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil,
|
||||
map[string]any{"op": "move", "ids": []string{item.ID}, "collection": dst.Slug,
|
||||
"status": f.target.Ref})
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("bulk move: expected 200 with a per-item failure, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
if strings.Contains(rr.Body.String(), f.target.ID) {
|
||||
t.Fatalf("the bulk move handed a caller who cannot see People the target's id: %s",
|
||||
rr.Body.String())
|
||||
}
|
||||
if v, ok := f.storedRelationKey(item.ID, "status"); ok && v == f.target.ID {
|
||||
t.Fatalf("a caller who cannot see People pointed a relation at one of its items: %#v", v)
|
||||
}
|
||||
var out bulkItemsResponse
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil {
|
||||
t.Fatalf("parse bulk response: %v: %s", err, rr.Body.String())
|
||||
}
|
||||
if len(out.Failed) == 0 {
|
||||
t.Fatalf("a supplied value naming an item the caller cannot see was accepted: %+v", out)
|
||||
}
|
||||
|
||||
// Control: the OWNER can see People, so the identical bulk move lands and
|
||||
// stores the canonical id. Without this leg the test passes against a
|
||||
// build that refused every supplied status.
|
||||
item2 := f.seed(`{"status":"open"}`)
|
||||
rr2 := f.call(f.srv.handleBulkItems, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil,
|
||||
map[string]any{"op": "move", "ids": []string{item2.ID}, "collection": dst.Slug,
|
||||
"status": f.target.Ref})
|
||||
if rr2.Code != http.StatusOK {
|
||||
t.Fatalf("bulk move as owner: expected 200, got %d: %s", rr2.Code, rr2.Body.String())
|
||||
}
|
||||
if v, ok := f.storedRelationKey(item2.ID, "status"); !ok || v != f.target.ID {
|
||||
t.Fatalf("the owner's identical bulk move did not store the relation (%#v, present=%v); "+
|
||||
"the refusal above is not visibility-dependent", v, ok)
|
||||
}
|
||||
}
|
||||
|
||||
// The preflight's CARRIED relation drops owe the same visibility collapse the
|
||||
// late-default drops get (codex round 16).
|
||||
//
|
||||
// Round 15 hoisted the collapse so `ResolveLateRelationDefaults`' issues stop
|
||||
// naming live items to callers who cannot see them. `MigrateRelationReferents`
|
||||
// returns its issues down a second path — `relationDropReason`, rendered
|
||||
// straight into `fields.dropped[].reason` — and that path never passed through
|
||||
// the collapse. `wrong_collection` is the reason that names a LIVE item, so a
|
||||
// carried value pointing at one the caller cannot see was reported
|
||||
// differently from a value naming nothing: the existence oracle, on the
|
||||
// carried path rather than the default path.
|
||||
//
|
||||
// The two legs are the whole test: a hidden-but-live target and a dangling
|
||||
// value must produce the SAME reason for a caller who can see neither.
|
||||
func TestRelationDoors_PreflightCarriedDropDoesNotDiscloseExistence(t *testing.T) {
|
||||
f := newDoorFixture(t)
|
||||
// A collection the restricted caller cannot see, holding a live item.
|
||||
secret := mustSchemaCollection(t, f.srv, f.ws.ID, "Secret People", `{"fields":[]}`)
|
||||
hidden, err := f.srv.store.CreateItem(f.ws.ID, secret.ID, models.ItemCreate{
|
||||
Title: "Hidden Person", CreatedBy: f.owner.ID,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateItem(hidden): %v", err)
|
||||
}
|
||||
// The destination declares owner_ref against People, which the caller CAN
|
||||
// see — so the only thing hidden is the collection the source value
|
||||
// actually points into.
|
||||
dst := mustSchemaCollection(t, f.srv, f.ws.ID, "Preflight Dest", fmt.Sprintf(`{"fields":[
|
||||
{"key":"status","label":"Status","type":"select","options":["open","done"]},
|
||||
{"key":"owner_ref","label":"Owner","type":"relation","collection":%q}
|
||||
]}`, f.people.Slug))
|
||||
|
||||
blind := mustUser(t, f.srv, "blind-carried-drop@example.com", "blindcarrieddrop", "")
|
||||
if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific",
|
||||
[]string{f.tasks.ID, dst.ID, f.people.ID}); err != nil {
|
||||
t.Fatalf("SetMemberCollectionAccess: %v", err)
|
||||
}
|
||||
|
||||
reasonFor := func(user *models.User, role, storedRef string) string {
|
||||
t.Helper()
|
||||
item := f.seed(fmt.Sprintf(`{"status":"open","owner_ref":%q}`, storedRef))
|
||||
rr := f.callAs(user, role, f.srv.handleCopyItemPreflight, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/"+item.Slug+"/copy/preflight",
|
||||
map[string]string{"itemSlug": item.Slug},
|
||||
map[string]any{"target_workspace": f.ws.Slug, "target_collection": dst.Slug})
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("preflight (%s): got %d, want 200: %s", storedRef, rr.Code, rr.Body.String())
|
||||
}
|
||||
if strings.Contains(rr.Body.String(), hidden.ID) && user == blind {
|
||||
t.Fatalf("the preflight handed a caller who cannot see Secret People the hidden "+
|
||||
"item's id: %s", rr.Body.String())
|
||||
}
|
||||
var pre ItemCopyPreflight
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &pre); err != nil {
|
||||
t.Fatalf("parse preflight: %v: %s", err, rr.Body.String())
|
||||
}
|
||||
for _, d := range pre.Fields.Dropped {
|
||||
if d.Key == "owner_ref" {
|
||||
return d.Reason
|
||||
}
|
||||
}
|
||||
t.Fatalf("preflight (%s) reported no drop for owner_ref: %+v", storedRef, pre.Fields)
|
||||
return ""
|
||||
}
|
||||
|
||||
hiddenReason := reasonFor(blind, "editor", hidden.Ref)
|
||||
danglingReason := reasonFor(blind, "editor", badRef)
|
||||
if hiddenReason != danglingReason {
|
||||
t.Fatalf("a caller who can see neither target distinguishes them: a LIVE hidden item "+
|
||||
"reports %q and a nonexistent value reports %q — that difference is the existence "+
|
||||
"oracle", hiddenReason, danglingReason)
|
||||
}
|
||||
|
||||
// Control: the OWNER can see Secret People, so they still get the
|
||||
// specific reason. Without this leg the test passes against a build that
|
||||
// collapsed every reason to not_found and told nobody anything.
|
||||
if got := reasonFor(f.owner, "owner", hidden.Ref); got == danglingReason {
|
||||
t.Fatalf("the owner's identical preflight also reports %q for a live item they CAN "+
|
||||
"see; the collapse is not visibility-dependent", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A NULL override does not make a stale non-nil source value stop counting as
|
||||
// carried (codex round 16).
|
||||
//
|
||||
// `notDefaultKeys` already knows that a nil value is not a value — round 14
|
||||
// taught it that, because ValidateFields treats a present-but-nil key as
|
||||
// absent and injects the destination default in its place. What it checks is
|
||||
// the SUPPLIED and CARRIED maps; the carried map here is built from the
|
||||
// item's STORED blob, which holds a non-nil legacy value. So a caller who
|
||||
// nulls the key gets the default injected AND the key exempted from the
|
||||
// visibility check, on the strength of a stored value the request just
|
||||
// discarded.
|
||||
//
|
||||
// The relation pass does not rescue this: an override of nil leaves nothing
|
||||
// for it to resolve, so the key is never dropped and never leaves the carried
|
||||
// set. The existing null-source test does not reach it — that fixture has no
|
||||
// stored value to go stale.
|
||||
func TestRelationDoors_NullOverrideDoesNotExemptDefaultFromVisibility(t *testing.T) {
|
||||
f := newDoorFixture(t)
|
||||
dst := mustSchemaCollection(t, f.srv, f.ws.ID, "Null Override Dest", fmt.Sprintf(`{"fields":[
|
||||
{"key":"status","label":"Status","type":"select","options":["open","done"]},
|
||||
{"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"default":%q,"required":true}
|
||||
]}`, f.people.Slug, f.target.Ref))
|
||||
|
||||
blind := mustUser(t, f.srv, "blind-null-override@example.com", "blindnulloverride", "")
|
||||
if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific",
|
||||
[]string{f.tasks.ID, dst.ID}); err != nil {
|
||||
t.Fatalf("SetMemberCollectionAccess: %v", err)
|
||||
}
|
||||
|
||||
// A non-nil STORED value, which is what goes stale. badRef so the value
|
||||
// itself is unresolvable and cannot be confused for the thing under test.
|
||||
item := f.seed(fmt.Sprintf(`{"status":"open","owner_ref":%q}`, badRef))
|
||||
rr := f.callAs(blind, "editor", f.srv.handleMoveItem, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/"+item.Slug+"/move",
|
||||
map[string]string{"itemSlug": item.Slug},
|
||||
map[string]any{"target_collection": dst.Slug,
|
||||
"field_overrides": map[string]any{"owner_ref": nil}})
|
||||
if strings.Contains(rr.Body.String(), f.target.ID) {
|
||||
t.Fatalf("the move handed a caller who cannot see People the id of the item the "+
|
||||
"destination default names: %s", rr.Body.String())
|
||||
}
|
||||
if v, ok := f.storedRelation(item.ID); ok && v == f.target.ID {
|
||||
t.Fatalf("the hidden default was stored for a caller who cannot see People: %#v", v)
|
||||
}
|
||||
|
||||
// Control: the OWNER can see People, so the same move keeps the default.
|
||||
// Without this leg the test passes against a build that dropped every
|
||||
// default, or refused every move.
|
||||
item2 := f.seed(fmt.Sprintf(`{"status":"open","owner_ref":%q}`, badRef))
|
||||
rr2 := f.call(f.srv.handleMoveItem, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/"+item2.Slug+"/move",
|
||||
map[string]string{"itemSlug": item2.Slug},
|
||||
map[string]any{"target_collection": dst.Slug,
|
||||
"field_overrides": map[string]any{"owner_ref": nil}})
|
||||
if rr2.Code != http.StatusOK {
|
||||
t.Fatalf("move as owner: expected 200, got %d: %s", rr2.Code, rr2.Body.String())
|
||||
}
|
||||
if v, ok := f.storedRelation(item2.ID); !ok || v != f.target.ID {
|
||||
t.Fatalf("the owner's identical move did not store the default (%#v, present=%v); "+
|
||||
"the drop above is not visibility-dependent", v, ok)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user