From ed03d488da810f70307c40cc36ecbd17a6974013 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 4 Sep 2026 16:16:03 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20four=20codex=20round-1=20findings=20?= =?UTF-8?q?=E2=80=94=20origin,=20visibility,=20bulk=20reporting=20(TASK-28?= =?UTF-8?q?78)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 1 named four; three were real P1s and I verified each in the code before accepting it. Codex could not run anything ("Go could not create its build cache because the workspace is read-only"), so every finding here is a static read that I confirmed and pinned. ## A THIRD ORIGIN, not two `items.MigrateFields` injects the DESTINATION schema's defaults for keys the source item has nothing for. My classifier split on `supplied` versus everything-else, so a destination default was filed as CARRIED — and on a cross-workspace copy every carried relation drops without a lookup. The destination's own default was discarded and reported `referent_not_portable`, which is flatly false about a value the destination chose. There are three origins: SUPPLIED (refuse on failure), CARRIED from the source item (cross-workspace: drop as not-portable), and DESTINATION DEFAULT (resolve against the destination in BOTH modes; drop with the resolver's own reason on failure, because nobody in this request typed it). Telling the last two apart needs the source field map, which all four migrate doors have as `currentFields`, so it is now a parameter. Empty values are skipped at every origin. An empty relation is a cleared field, not a referent, and reporting it as dropped tells a user they lost something they never had. WHAT THE FIRST VERSION OF THIS TEST PROVED: nothing. The mutant that reverts the classifier SURVIVED it. `ValidateFields` re-injects the default after my resolver deleted the key, so the value comes back either way and `StillDropped` filters the false report out — the end state is identical unless the default is a REF. A UUID default is already its own canonical form, so "resolved" and "dropped then re-injected raw" produce the same bytes. With `PEOP-1` as the default the mutant is DETECTED, because only a resolved default lands as the id. ## SUPPLIED OVERRIDES AT THE MIGRATE DOORS SKIPPED THE VISIBILITY CHECK The four write doors go through `s.resolveRelationReferents`, which adds `checkItemVisible` on top of the store resolver. The migrate doors called `store.MigrateRelationReferents` directly — it is a store function and cannot answer a request-scoped question — so their SUPPLIED half, which this unit's own rule calls an ordinary write, resolved against the database alone. A caller able to edit both collections could point a relation at an item they cannot see. The ROLE is the part worth getting right. For `move` it is `workspaceRole(r)`. For copy and preflight it is the caller's role in the DESTINATION, and `CrossWorkspaceAccess.Role` is exactly that — its own doc says never to substitute `workspaceRole(r)`. So `resolveRelationReferents` now takes the role explicitly (`resolveRelationReferentsAs`), and the new `refuseInvisibleRelationOverrides` runs at all three doors with the right one. At the copy it runs in the HANDLER, before the store call: a pre-write refusal must not open a transaction to roll it back, and the preflight runs the identical check — DR-6's "the preview IS the copy" only holds if both doors refuse the same request. ## BULK COLLECTION MOVE DISCARDED FIELDS SILENTLY `bulkMoveCollection` has populated `result.Dropped` since MigrateFields existed and NOTHING read it — the only reference in the file was my own append. BUG-2674 fixed the single-item door and left this one, so a bulk move discarded values with no record anywhere. Pre-existing, and routing relation drops into the same dead list is what made it mine to fix. Reported on the activity row, same key, same joined-string shape and the same BUG-2628 reason as `handleMoveItem`, filtered against the final map so the report is true when written. Threaded as an out-parameter, deliberately: only this branch produces drops, the caller needs them for ONE activity row per item, and a third return value would put `nil` in fourteen unrelated returns. ## THE P2, AND WHAT IT IS NOT PINNED BY `resolveRelationReferents` did `if item == nil { continue }` after the visibility read — "treat a race as someone else's 404". It now refuses with the same `not_found` the resolver would have given moments later. This whole unit exists to keep a dangling referent out of the blob, and a target that vanished mid-request is the one case where waving it through would have been deliberate. NO TEST. Reproducing it means deleting a row between two reads inside one request, and a test that faked that would pin the fake. Stated here rather than left to look covered. ## Counterfactuals Every fix has a mutant that its own test detects, each build-checked first: classifier reverted -> DETECTED (destination default); empty-skip removed -> DETECTED; visibility helper neutered -> DETECTED at both the move door and the copy/preflight pair; bulk-move report removed -> DETECTED. Gates: internal/server ok 224.6s · internal/store ok 261.5s · internal/items ok · internal/mcp ok 15.1s · go vet clean · gofmt clean · make lint 0 issues. --- internal/server/handlers_items.go | 14 +- internal/server/handlers_items_bulk.go | 35 +++- internal/server/handlers_items_copy.go | 31 +++ .../server/handlers_items_copy_preflight.go | 13 +- .../handlers_items_copy_relation_test.go | 182 ++++++++++++++++++ internal/server/relation_referents.go | 76 +++++++- .../server/relation_referents_doors_test.go | 131 +++++++++++++ internal/store/items_cross_workspace_copy.go | 2 +- internal/store/relation_referents.go | 106 ++++++++-- internal/store/relation_referents_test.go | 23 ++- 10 files changed, 580 insertions(+), 33 deletions(-) diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 9d3d0895..23e01991 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -2354,9 +2354,21 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { // move. Refusing carried values here would make every legacy item — and // `internal/items` has accepted any string for a relation all along — // permanently unmovable. + // The supplied half is a write, so it owes the same visibility check the + // four write doors run. The store resolver cannot do it — see + // refuseInvisibleRelationOverrides. Same workspace here, so the role + // stashed for this request is the right one. + if invisible, err := s.refuseInvisibleRelationOverrides( + r, workspaceID, workspaceRole(r), items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides); err != nil { + writeInternalError(w, err) + return + } else if refuseRelationIssues(w, invisible) { + return + } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, - input.FieldOverrides, store.RelationCarryWithinWorkspace) + input.FieldOverrides, currentFields, store.RelationCarryWithinWorkspace) if relErr != nil { writeInternalError(w, relErr) return diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index f6598ead..67040eba 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -269,7 +269,8 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) { continue } - updated, opErr := s.applyBulkOp(r, workspaceID, item, &req, actor, source, visibleIDs, resolvedTarget, batchID) + var droppedFields []string + updated, opErr := s.applyBulkOp(r, workspaceID, item, &req, actor, source, visibleIDs, resolvedTarget, batchID, &droppedFields) if opErr != nil { resp.Failed = append(resp.Failed, bulkItemFailure{ Ref: itemRefOrSlug(*item), @@ -305,6 +306,12 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) { action = "moved" meta["from_collection"] = item.CollectionSlug meta["to_collection"] = req.Collection + // Same key, same joined-string shape and the same reason as + // handleMoveItem's: this map is map[string]string, and a raw array + // renders as a Go map literal in the timeline (BUG-2628). + if len(droppedFields) > 0 { + meta["dropped_fields"] = strings.Join(droppedFields, ", ") + } } s.logActivityWithMeta(workspaceID, item.ID, action, r, auditMeta(meta)) @@ -406,7 +413,14 @@ func bulkStoreError(err error) *bulkOpError { return &bulkOpError{message: err.Error()} } -func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, actor, source string, visibleIDs []string, resolvedTarget *models.Collection, batchID string) (*models.Item, *bulkOpError) { +// droppedFields is an OUT-PARAMETER, and it is one deliberately. Only the +// collection-move branch produces dropped field keys, the caller needs them to +// build ONE activity row per item (a second row would misreport a single +// move as two events), and threading a third return value through fourteen +// unrelated returns would put `nil` in every branch that has nothing to say. +// Reset by the caller each iteration; nil is accepted and means "not +// interested". +func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, actor, source string, visibleIDs []string, resolvedTarget *models.Collection, batchID string, droppedFields *[]string) (*models.Item, *bulkOpError) { switch req.Op { case "archive": if err := s.store.DeleteItem(item.ID, store.WithEventBatch(batchID)); err != nil { @@ -440,7 +454,7 @@ func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.I case "move": if req.Collection != "" { - return s.bulkMoveCollection(r, workspaceID, item, req, visibleIDs, resolvedTarget, batchID) + return s.bulkMoveCollection(r, workspaceID, item, req, visibleIDs, resolvedTarget, batchID, droppedFields) } // Status-only move = a field update on the same collection. return s.bulkFieldUpdate(r, workspaceID, item, map[string]any{"status": req.Status}, req.Force, visibleIDs, actor, source, batchID) @@ -661,7 +675,7 @@ func (s *Server) bulkTagUpdate(item *models.Item, tags []string, add bool, actor // resolves to nothing — deliberately, so that case and an existing-but-hidden // target fail identically here rather than at different HTTP statuses (the // existence oracle from codex round 4). Hence the nil check below. -func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, visibleIDs []string, targetColl *models.Collection, batchID string) (*models.Item, *bulkOpError) { +func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, visibleIDs []string, targetColl *models.Collection, batchID string, droppedFields *[]string) (*models.Item, *bulkOpError) { if targetColl == nil { return nil, &bulkOpError{message: "target collection not found", code: "invalid_collection"} } @@ -721,7 +735,7 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m // for `supplied` says that rather than leaving it implied. relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, - nil, store.RelationCarryWithinWorkspace) + nil, currentFields, store.RelationCarryWithinWorkspace) if relErr != nil { return nil, &bulkOpError{message: relErr.Error(), code: "internal_error"} } @@ -736,6 +750,17 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m if err := items.ValidateFields(result.Fields, items.SchemaForMigratedFields(targetSchema)); err != nil { return nil, &bulkOpError{message: err.Error(), code: "validation_error"} } + // Hand the discarded keys back so the caller's activity row can name them + // (BUG-2674, which fixed only the SINGLE-item move). `result.Dropped` has + // been populated on this path since MigrateFields existed and NOTHING read + // it — a bulk move discarded field values with no record anywhere, which is + // the same silence that convention closed one door over. Filtered against + // the FINAL map for the reason handleMoveItem filters: a key MigrateFields + // listed may have been re-supplied or re-defaulted since, and reporting + // that would be a confident falsehood about data sitting on the item. + if droppedFields != nil { + *droppedFields = items.StillDropped(result.Dropped, result.Fields) + } fieldsJSON, err := json.Marshal(result.Fields) if err != nil { diff --git a/internal/server/handlers_items_copy.go b/internal/server/handlers_items_copy.go index 4384ad2f..df69961e 100644 --- a/internal/server/handlers_items_copy.go +++ b/internal/server/handlers_items_copy.go @@ -2,12 +2,14 @@ package server import ( "database/sql" + "encoding/json" "errors" "fmt" "log/slog" "net/http" "runtime/debug" + "github.com/PerpetualSoftware/pad/internal/items" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" ) @@ -320,6 +322,35 @@ func (s *Server) handleCopyItem(w http.ResponseWriter, r *http.Request) { // has no counterpart, because it writes nothing to attribute. actor, actorSource := actorFromRequest(r) + // Visibility on the SUPPLIED relation overrides, before the store call + // (TASK-2878). The store's migrate function resolves them against the + // database and cannot answer "may this requester see that item" — see + // refuseInvisibleRelationOverrides. Against the DESTINATION, with the + // caller's role THERE: dst.Role is derived fresh from membership and + // grants, and workspaceRole(r) here is the SOURCE's. + // + // Before the store call rather than inside it, and that is deliberate: + // this is a pre-write refusal, so it must not open a transaction and roll + // it back, and the preflight runs the identical check — DR-6's "the + // preview IS the copy" only holds if both doors refuse the same request. + if len(input.FieldOverrides) > 0 { + var targetSchema models.CollectionSchema + if err := json.Unmarshal([]byte(targetColl.Schema), &targetSchema); err != nil { + writeInternalError(w, fmt.Errorf("copy item: parse destination schema: %w", err)) + return + } + invisible, err := s.refuseInvisibleRelationOverrides( + r, dst.WorkspaceID(), dst.Role, items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides) + if err != nil { + writeInternalError(w, err) + return + } + if refuseRelationIssues(w, invisible) { + return + } + } + // ---- The copy. ONE call, no retry (DR-13). --------------------------- res, err := s.copyItemAcrossWorkspaces(store.CrossWorkspaceCopyRequest{ SourceItemID: item.ID, diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index 13a84d3c..07fe7ab5 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -718,9 +718,20 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) if items.ScopeFor(item.WorkspaceID, dst.WorkspaceID()) == items.CrossWorkspace { relMode = store.RelationCarryCrossWorkspace } + // Visibility on the supplied half, against the DESTINATION and the + // caller's role THERE — dst.Role, never workspaceRole(r), which is the + // source's. See refuseInvisibleRelationOverrides. + if invisible, err := s.refuseInvisibleRelationOverrides( + r, dst.WorkspaceID(), dst.Role, items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides); err != nil { + writeInternalError(w, err) + return + } else if refuseRelationIssues(w, invisible) { + return + } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( dst.WorkspaceID(), items.SchemaForMigratedFields(targetSchema), final, - input.FieldOverrides, relMode) + input.FieldOverrides, currentFields, relMode) if relErr != nil { writeInternalError(w, fmt.Errorf("copy preflight: resolve relation referents: %w", relErr)) return diff --git a/internal/server/handlers_items_copy_relation_test.go b/internal/server/handlers_items_copy_relation_test.go index 7918aaf7..b10d22f7 100644 --- a/internal/server/handlers_items_copy_relation_test.go +++ b/internal/server/handlers_items_copy_relation_test.go @@ -3,6 +3,7 @@ package server import ( "fmt" "net/http" + "net/http/httptest" "strings" "testing" @@ -270,3 +271,184 @@ func containsAll(s string, subs ...string) bool { } return true } + +// newCopyRelationFixtureWith builds the relation fixture with two knobs the +// default constructor does not need: a `default` on the DESTINATION's relation +// field, and the source item's own stored value for that field. +// +// Both exist to reach origins the plain fixture cannot. `MigrateFields` +// injects a destination default for any target field the source has nothing +// for, so a value can be present in the migrated map having been chosen by the +// DESTINATION rather than carried from the source — a third origin, and the +// one that is invisible until a schema declares a default. +func newCopyRelationFixtureWith(t *testing.T, destDefault bool, sourceOwnerRef *string) *relationFixture { + t.Helper() + srv := testServer(t) + bus := events.New() + srv.SetEventBus(bus) + t.Cleanup(bus.Close) + + owner := mustUser(t, srv, "rel2-owner@example.com", "rel2owner", "") + wsA := mustWorkspace(t, srv, "Rel2 Source WS", owner.ID) + wsB := mustWorkspace(t, srv, "Rel2 Dest WS", owner.ID) + + targetsA := mustSchemaCollection(t, srv, wsA.ID, "People A", `{"fields":[]}`) + targetsB := mustSchemaCollection(t, srv, wsB.ID, "People B", `{"fields":[]}`) + + targetA, err := srv.store.CreateItem(wsA.ID, targetsA.ID, models.ItemCreate{Title: "Ada in A", CreatedBy: owner.ID}) + if err != nil { + t.Fatalf("CreateItem(targetA): %v", err) + } + // Created BEFORE collB, because its id is what the destination default + // names — a default pointing at nothing would fail for the wrong reason. + targetB, err := srv.store.CreateItem(wsB.ID, targetsB.ID, models.ItemCreate{Title: "Grace in B", CreatedBy: owner.ID}) + if err != nil { + t.Fatalf("CreateItem(targetB): %v", err) + } + + relSchema := func(targetSlug, def string) string { + defaultClause := "" + if def != "" { + defaultClause = fmt.Sprintf(`,"default":%q`, def) + } + return fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"],"required":true}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q%s} + ]}`, targetSlug, defaultClause) + } + + collA := mustSchemaCollection(t, srv, wsA.ID, "Rel2 Tasks A", relSchema(targetsA.Slug, "")) + // A REF, not the UUID. This is the discriminating choice: a UUID is + // already its canonical form, so a default that was resolved and one that + // was dropped-then-re-injected-raw by ValidateFields produce IDENTICAL + // bytes, and the test proves nothing. Supplied as `PEOP-1`, only a + // resolved default lands as targetB.ID. + destDef := "" + if destDefault { + destDef = targetB.Ref + } + collB := mustSchemaCollection(t, srv, wsB.ID, "Rel2 Tasks B", relSchema(targetsB.Slug, destDef)) + + sourceFields := `{"status":"open"}` + if sourceOwnerRef != nil { + sourceFields = fmt.Sprintf(`{"status":"open","owner_ref":%q}`, *sourceOwnerRef) + } + source, err := srv.store.CreateItem(wsA.ID, collA.ID, models.ItemCreate{ + Title: "Rel2 Source", Content: "body", Fields: sourceFields, CreatedBy: owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(source): %v", err) + } + + return &relationFixture{ + copyPreflightFixture: ©PreflightFixture{ + t: t, srv: srv, bus: bus, owner: owner, + wsA: wsA, wsB: wsB, collA: collA, collB: collB, hiddenB: targetsB, + source: source, + }, + targetsA: targetsA, targetsB: targetsB, targetA: targetA, targetB: targetB, + } +} + +// A DESTINATION DEFAULT is not a carried referent, and must not be dropped as +// one (codex round 1, P1). +// +// `MigrateFields` fills in the destination schema's defaults for keys the +// source item has nothing for. A classifier that splits only on "did the +// caller supply it" then files that value as CARRIED, and a cross-workspace +// copy drops every carried relation without a lookup — so the destination's +// own default was discarded and reported `referent_not_portable`, which is +// flatly false about a value the destination chose. +// +// The source item deliberately has NO owner_ref: with one, the key is carried +// and the default never enters the migrated map, which is the arrangement that +// hid this. +func TestCopyEndpoint_DestinationDefaultRelationIsNotDroppedAsNotPortable(t *testing.T) { + f := newCopyRelationFixtureWith(t, true, nil) + body := f.baseBody() + + pre := f.ok(body) + if reason, dropped := droppedReason(pre, "owner_ref"); dropped { + t.Fatalf("the preflight drops the DESTINATION's own default for reason %q; it never "+ + "pointed at the source workspace", reason) + } + // RESOLVED, not merely present. Misclassifying the default as carried + // deletes it and ValidateFields then re-injects the raw default, so the + // key comes back either way — the value is the only thing that tells the + // two apart. + if v, carried := carriedValue(pre, "owner_ref"); !carried || v != f.targetB.ID { + t.Fatalf("preflight carries owner_ref = %#v (carried=%v), want the destination default "+ + "RESOLVED to %q; the raw ref %q means it was dropped and re-injected unresolved", + v, carried, f.targetB.ID, f.targetB.Ref) + } + + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "destination default relation", body) + if got := f.persistedFields(res.Item.ID)["owner_ref"]; got != f.targetB.ID { + t.Fatalf("the copy persisted owner_ref = %#v, want the destination default %q", got, f.targetB.ID) + } + if hasDroppedField(res, "owner_ref") { + t.Fatalf("the copy reported the destination's own default as dropped: %+v", res.Warnings) + } +} + +// An EMPTY relation value is a cleared field, not a referent, and reporting it +// as dropped tells a user they lost something they never had (codex round 1, +// P1, second half). +func TestCopyEndpoint_EmptyCarriedRelationIsNotReportedDropped(t *testing.T) { + empty := "" + f := newCopyRelationFixtureWith(t, false, &empty) + + pre := f.ok(f.baseBody()) + if reason, dropped := droppedReason(pre, "owner_ref"); dropped { + t.Fatalf("the preflight reports an EMPTY relation as dropped (%q) — there was no "+ + "referent to lose: %+v", reason, pre.Fields.Dropped) + } + + res := f.copyOK(f.baseBody()) + if hasDroppedField(res, "owner_ref") { + t.Fatalf("the copy reports an empty relation as dropped: %+v", res.Warnings) + } +} + +// A supplied relation override must name an item the REQUESTER CAN SEE, on the +// copy and its preflight alike (codex round 1, P1). +// +// The four write doors have always checked this; the migrate doors called the +// store resolver directly, and the store cannot answer a request-scoped +// question. The role that matters here is the caller's in the DESTINATION — +// `workspaceRole(r)` is the source's. +// +// The unrestricted owner leg is the control: without it this passes against a +// build that refuses every override. +func TestCopyEndpoint_InvisibleRelationOverrideIsRefused(t *testing.T) { + f := newCopyRelationFixture(t) + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": f.targetB.Ref} + + // Editor in both, but with no access to People B — so the override names a + // live item in the right collection that this caller cannot see. + blind := f.restrictedEditor("blind-rel@example.com", "blindrel", + []string{f.collA.ID}, []string{f.collB.ID}) + + for _, d := range []struct { + name string + rr *httptest.ResponseRecorder + }{ + {"preflight", f.call(blind, reqOpts{wsRoleCtx: "editor"}, body)}, + {"copy", f.callCopy(blind, reqOpts{wsRoleCtx: "editor"}, body)}, + } { + if d.rr.Code != http.StatusBadRequest { + t.Fatalf("%s: expected 400 for an override naming an item the caller cannot see, "+ + "got %d: %s", d.name, d.rr.Code, d.rr.Body.String()) + } + if code := errCode(t, d.rr); code != "validation_error" { + t.Fatalf("%s: error code = %q, want validation_error", d.name, code) + } + } + + // Control: the same override from the owner, who can see People B. + if rr := f.call(f.owner, reqOpts{}, body); rr.Code != http.StatusOK { + t.Fatalf("the owner's identical override was refused %d — the check is refusing "+ + "visibility-independent of who asks: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index 9697a3fb..0355a026 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -42,6 +42,25 @@ func (s *Server) resolveRelationReferents( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, +) ([]store.RelationIssue, error) { + return s.resolveRelationReferentsAs(r, workspaceID, workspaceRole(r), schema, fieldMap) +} + +// resolveRelationReferentsAs is resolveRelationReferents with the requester's +// effective role passed EXPLICITLY rather than read from the request. +// +// The cross-workspace copy and its preflight need this. `workspaceRole(r)` is +// the role the middleware stashed for the workspace in the URL — the SOURCE — +// and a relation override on a copy names an item in the DESTINATION, where +// the caller's role can be different or absent. `CrossWorkspaceAccess.Role` +// is that role, derived fresh from membership and grants, and its own doc says +// in as many words never to substitute `workspaceRole(r)` for it. +func (s *Server) resolveRelationReferentsAs( + r *http.Request, + workspaceID string, + role string, + schema models.CollectionSchema, + fieldMap map[string]any, ) ([]store.RelationIssue, error) { issues, err := s.store.ResolveRelationReferents(workspaceID, schema, fieldMap) if err != nil { @@ -70,9 +89,19 @@ func (s *Server) resolveRelationReferents( return nil, err } if item == nil { - continue // resolved a moment ago; treat a race as someone else's 404 + // It resolved moments ago and is gone now — soft-deleted between + // the two reads. Treated as unresolvable rather than waved + // through: this whole unit exists to stop a dangling referent + // reaching the blob, and "the target vanished mid-request" is the + // one case where letting it through would be a deliberate one. + // Same `not_found` the resolver would have given a moment later, + // so a retry reports it identically. + issues = append(issues, store.RelationIssue{ + Key: def.Key, Value: id, Target: def.Collection, Reason: store.RelationTargetNotFound, + }) + continue } - visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), workspaceRole(r), isBearerAuth(r)) + visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), role, isBearerAuth(r)) if err != nil { return nil, err } @@ -132,3 +161,46 @@ func refuseRelationIssues(w http.ResponseWriter, issues []store.RelationIssue) b func relationIssuesMessage(issues []store.RelationIssue) string { return store.RelationIssuesMessage(issues) } + +// refuseInvisibleRelationOverrides is the visibility check the MIGRATE doors +// owe their SUPPLIED half. +// +// The four write doors go through resolveRelationReferents, which adds +// checkItemVisible on top of the store resolver. The migrate doors call +// store.MigrateRelationReferents directly — it is a store function and cannot +// answer a request-scoped question — so without this their supplied overrides +// resolved against the database alone. This unit's own rule says a supplied +// value is an ordinary write; an ordinary write cannot name an item the +// requester may not see, and a caller able to edit both collections could +// otherwise point a relation at a hidden one. +// +// Only relation keys are probed, on a COPY of the values: the store call that +// follows does the canonicalising write, and a helper that also mutated would +// leave two functions writing one map. +// +// `role` is explicit for the reason resolveRelationReferentsAs documents — at a +// cross-workspace copy the relevant role is the caller's in the DESTINATION. +func (s *Server) refuseInvisibleRelationOverrides( + r *http.Request, + workspaceID string, + role string, + schema models.CollectionSchema, + supplied map[string]any, +) ([]store.RelationIssue, error) { + if len(supplied) == 0 { + return nil, nil + } + probe := make(map[string]any, len(supplied)) + for _, def := range schema.Fields { + if def.Type != "relation" { + continue + } + if v, ok := supplied[def.Key]; ok && v != nil { + probe[def.Key] = v + } + } + if len(probe) == 0 { + return nil, nil + } + return s.resolveRelationReferentsAs(r, workspaceID, role, schema, probe) +} diff --git a/internal/server/relation_referents_doors_test.go b/internal/server/relation_referents_doors_test.go index 3962b49e..c090e598 100644 --- a/internal/server/relation_referents_doors_test.go +++ b/internal/server/relation_referents_doors_test.go @@ -496,3 +496,134 @@ func TestRelationDoors_CrossWorkspaceCopyAndPreflight(t *testing.T) { assertRefused(t, "preflight", f.call(f.owner, reqOpts{}, body)) }) } + +// A supplied relation override at the MOVE door must name an item the +// requester can see (codex round 1, P1). +// +// The write doors have always checked this; the migrate doors call the store +// resolver directly, and the store cannot answer a request-scoped question. +// Same workspace here, so the role stashed for the request is the right one — +// the cross-workspace half of this rule, where it is NOT, is pinned in +// TestCopyEndpoint_InvisibleRelationOverrideIsRefused. +func TestRelationDoors_MoveRefusesInvisibleOverride(t *testing.T) { + f := newDoorFixture(t) + dst := f.targetCollection() + item := f.seed(`{"status":"open"}`) + + // An editor who can see the two task collections but NOT People. + blind := mustUser(t, f.srv, "blind-move@example.com", "blindmove", "") + 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) + } + + body := map[string]any{ + "target_collection": dst.Slug, + "field_overrides": map[string]any{"owner_ref": f.target.Ref}, + } + + 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}, body) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for an override naming an item the caller cannot see, got %d: %s", + rr.Code, rr.Body.String()) + } + if code := errCode(t, rr); code != "validation_error" { + t.Fatalf("error code = %q, want validation_error", code) + } + + // Control: the owner, who can see People, gets the same override through. + // Without this the test passes against a build that refuses every override. + item2 := f.seed(`{"status":"open"}`) + body2 := map[string]any{ + "target_collection": dst.Slug, + "field_overrides": map[string]any{"owner_ref": f.target.Ref}, + } + if rr := f.call(f.srv.handleMoveItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+item2.Slug+"/move", + map[string]string{"itemSlug": item2.Slug}, body2); rr.Code != http.StatusOK { + t.Fatalf("the owner's identical override was refused %d — the check is not "+ + "visibility-dependent: %s", rr.Code, rr.Body.String()) + } +} + +// A BULK collection move that discards field values must say so (codex round +// 1, P1). +// +// `bulkMoveCollection` has populated `result.Dropped` since MigrateFields +// existed and NOTHING read it — BUG-2674 fixed the single-item door only, so +// the bulk door discarded values with no record anywhere. Wiring relation +// drops into that same dead list is what made it worth fixing rather than +// noting. +// +// Asserted on the ACTIVITY ROW, not the response: the bulk response is a +// per-item outcome list with no room for this, and the timeline is where +// someone asking "what happened to my item" actually looks. +func TestRelationDoors_BulkMoveReportsDroppedFields(t *testing.T) { + f := newDoorFixture(t) + dst := f.targetCollection() + item := f.seed(fmt.Sprintf(`{"status":"open","owner_ref":%q}`, badRef)) + + rr := f.call(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}) + if rr.Code != http.StatusOK { + t.Fatalf("bulk move: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if v, ok := f.storedRelation(item.ID); ok { + t.Fatalf("an unresolvable relation survived the bulk move as %#v", v) + } + + acts, err := f.srv.store.ListDocumentActivity(item.ID, models.ActivityListParams{Limit: 20}) + if err != nil { + t.Fatalf("ListDocumentActivity: %v", err) + } + var moved *models.Activity + for i := range acts { + if acts[i].Action == "moved" { + moved = &acts[i] + break + } + } + if moved == nil { + t.Fatalf("no `moved` activity row for a bulk collection move: %+v", acts) + } + if !strings.Contains(moved.Metadata, "dropped_fields") || !strings.Contains(moved.Metadata, "owner_ref") { + t.Fatalf("the bulk move discarded owner_ref without naming it in the activity row: %s", + moved.Metadata) + } +} + +// callAs is `call` with an explicit user and workspace role, for the legs that +// need someone other than the fixture owner. +func (f *doorFixture) callAs(user *models.User, role string, h http.HandlerFunc, method, path string, params map[string]string, body any) *httptest.ResponseRecorder { + f.t.Helper() + var reader io.Reader + if body != nil { + raw, err := json.Marshal(body) + if err != nil { + f.t.Fatalf("marshal body: %v", err) + } + reader = bytes.NewReader(raw) + } + r := httptest.NewRequest(method, path, reader) + if body != nil { + r.Header.Set("Content-Type", "application/json") + } + ctx := WithCurrentUser(r.Context(), user) + ctx = contextWithWorkspaceRoleForTest(ctx, role) + ctx = contextWithResolvedWorkspaceIDForTest(ctx, f.ws.ID) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("slug", f.ws.Slug) + for k, v := range params { + rctx.URLParams.Add(k, v) + } + ctx = context.WithValue(ctx, chi.RouteCtxKey, rctx) + rr := httptest.NewRecorder() + h(rr, r.WithContext(ctx)) + return rr +} diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 9322fd94..948f8bcd 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -1121,7 +1121,7 @@ func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON, mode = RelationCarryCrossWorkspace } relRefusals, relDropped, relErr := s.MigrateRelationReferentsQ(q, destWorkspaceID, - items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, mode) + items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, currentFields, mode) if relErr != nil { return nil, nil, fmt.Errorf("copy item across workspaces: resolve relation referents: %w", relErr) } diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index 7e894758..56b045a9 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -348,6 +348,26 @@ const ( RelationCarryCrossWorkspace ) +// RelationOrigin says where a relation value in a migrate door's field map +// came from. THREE origins, not two, and the third is the one that is easy to +// miss: `items.MigrateFields` injects the DESTINATION schema's defaults into +// the map it returns, so a value can be present having been chosen by the +// destination rather than carried from the source. +type RelationOrigin int + +const ( + // RelationOriginSupplied — an explicit override on the move or copy. The + // caller asserted it, so it is a write. + RelationOriginSupplied RelationOrigin = iota + // RelationOriginCarried — present on the SOURCE item. Asserted by nobody. + RelationOriginCarried + // RelationOriginDestinationDefault — absent from the source item and not + // supplied, so `MigrateFields` filled it from the destination schema's + // `default`. It never pointed at the source workspace, which is why it + // must not be dropped as `referent_not_portable`. + RelationOriginDestinationDefault +) + // MigrateRelationReferents is the ONE decision the four migrate doors share // (PLAN-2857 U1 / TASK-2878). Same-workspace move, bulk move, cross-workspace // copy and the copy preflight all call this; none of them reimplements it. @@ -358,28 +378,39 @@ const ( // preflight that says "carried" while the copy drops — or the reverse — is one // request answered two ways, which is the exact defect that comment describes. // -// PROVENANCE, not door, decides the outcome: +// ORIGIN, not door, decides the outcome: // // - SUPPLIED (present in `supplied`, i.e. an explicit `--field` override on // the move or copy): the caller asserted this value, so it is a write and // an unresolvable one is REFUSED. Returned in `refusals`. -// - CARRIED (everything else): not asserted by anyone, so refusing it would -// make a legacy item — and `internal/items` has accepted any string for a -// relation all along, so most of them are legacy — unmovable and -// uncopyable. Dropped and REPORTED instead, through the same -// `dropped_fields` channel BUG-2674 established for a move and the same +// - CARRIED (present in `sourceFields`): not asserted by anyone, so refusing +// it would make a legacy item — and `internal/items` has accepted any +// string for a relation all along, so most of them are legacy — unmovable +// and uncopyable. Dropped and REPORTED instead, through the same +// `dropped_fields` channel BUG-2674 established and the same // `referent_not_portable` bucket the copy already uses for `github_pr`. +// - DESTINATION DEFAULT (neither): `MigrateFields` filled it in from the +// destination schema. Resolved against the destination REGARDLESS of mode, +// because a value the destination chose is not a source-workspace referent +// and dropping it as "not portable" would be a false statement about where +// it came from. A default that does not resolve is a schema problem, so it +// is dropped and reported with the resolver's own reason rather than +// refused — nobody in this request typed it. // -// Mutates fieldMap: dropped keys are deleted, and carried values that survive -// are canonicalised to their target's ID. +// EMPTY VALUES ARE NOT REFERENTS and are skipped at every origin. Reporting a +// blank field as dropped tells a user they lost something they never had. +// +// Mutates fieldMap: dropped keys are deleted, and values that survive are +// canonicalised to their target's ID. func (s *Store) MigrateRelationReferents( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, supplied map[string]any, + sourceFields map[string]any, mode RelationCarryMode, ) (refusals []RelationIssue, dropped []RelationIssue, err error) { - return s.MigrateRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, supplied, mode) + return s.MigrateRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, supplied, sourceFields, mode) } // MigrateRelationReferentsQ is MigrateRelationReferents on a caller-supplied @@ -390,13 +421,16 @@ func (s *Store) MigrateRelationReferentsQ( schema models.CollectionSchema, fieldMap map[string]any, supplied map[string]any, + sourceFields map[string]any, mode RelationCarryMode, ) (refusals []RelationIssue, dropped []RelationIssue, err error) { - // Split the map by provenance FIRST, so the two halves cannot be confused - // by anything below. Schema order, for the determinism the preflight - // promises its callers. - carried := map[string]any{} - suppliedRelations := map[string]any{} + // Split the map by ORIGIN first, so nothing below can confuse the three. + // Schema order, for the determinism the preflight promises its callers. + byOrigin := map[RelationOrigin]map[string]any{ + RelationOriginSupplied: {}, + RelationOriginCarried: {}, + RelationOriginDestinationDefault: {}, + } for _, def := range schema.Fields { if def.Type != "relation" { continue @@ -405,15 +439,24 @@ func (s *Store) MigrateRelationReferentsQ( if !exists || raw == nil { continue } - if _, isSupplied := supplied[def.Key]; isSupplied { - suppliedRelations[def.Key] = raw + // An empty value is a cleared relation, not a referent. Skipping it + // here keeps it out of every bucket, so it is neither refused nor + // reported as a drop the user cannot act on. + if str, isStr := raw.(string); isStr && strings.TrimSpace(str) == "" { continue } - carried[def.Key] = raw + switch { + case hasKey(supplied, def.Key): + byOrigin[RelationOriginSupplied][def.Key] = raw + case hasKey(sourceFields, def.Key): + byOrigin[RelationOriginCarried][def.Key] = raw + default: + byOrigin[RelationOriginDestinationDefault][def.Key] = raw + } } // Supplied values are ordinary writes. - if len(suppliedRelations) > 0 { + if suppliedRelations := byOrigin[RelationOriginSupplied]; len(suppliedRelations) > 0 { issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, suppliedRelations) if resolveErr != nil { return nil, nil, resolveErr @@ -425,6 +468,26 @@ func (s *Store) MigrateRelationReferentsQ( } } + // Destination defaults resolve against the DESTINATION in both modes — the + // destination picked the value, so the mode says nothing about it. + if defaults := byOrigin[RelationOriginDestinationDefault]; len(defaults) > 0 { + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, defaults) + if resolveErr != nil { + return nil, nil, resolveErr + } + for _, ri := range issues { + dropped = append(dropped, ri) + delete(fieldMap, ri.Key) + } + for k, v := range defaults { + if _, survived := fieldMap[k]; !survived { + continue + } + fieldMap[k] = v + } + } + + carried := byOrigin[RelationOriginCarried] switch mode { case RelationCarryCrossWorkspace: // No lookup: a source-workspace id cannot mean anything here. @@ -465,3 +528,10 @@ func (s *Store) MigrateRelationReferentsQ( } return refusals, dropped, nil } + +// hasKey reports whether m declares key. A nil map has no keys, which is how a +// door with no overrides (bulk move) or no source item says so. +func hasKey(m map[string]any, key string) bool { + _, ok := m[key] + return ok +} diff --git a/internal/store/relation_referents_test.go b/internal/store/relation_referents_test.go index 4b37e7db..c4585dd3 100644 --- a/internal/store/relation_referents_test.go +++ b/internal/store/relation_referents_test.go @@ -263,7 +263,7 @@ func TestMigrateRelationReferents_SameWorkspaceKeepsWhatResolves(t *testing.T) { // relation must survive. Dropping it would lose data on every move of a // correctly-related item. fields := map[string]any{"color": red.Ref, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -283,7 +283,7 @@ func TestMigrateRelationReferents_SameWorkspaceDropsWhatDoesNot(t *testing.T) { // accepted any string for a relation all along, so an item carrying "red" // must stay MOVABLE. Dropped and reported, never refused. fields := map[string]any{"color": "red", "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -310,7 +310,7 @@ func TestMigrateRelationReferents_CrossWorkspaceDropsEveryCarriedRelation(t *tes // so there is nothing in the destination it could mean. Same reason // github_pr uses, because it is the same fact about the same kind of value. fields := map[string]any{"color": red.ID, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -335,7 +335,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing for _, mode := range []RelationCarryMode{RelationCarryWithinWorkspace, RelationCarryCrossWorkspace} { fields := map[string]any{"color": "nope"} supplied := map[string]any{"color": "nope"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, mode) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), mode) if err != nil { t.Fatalf("mode %v: %v", mode, err) } @@ -356,7 +356,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing // on a copy at all. fields := map[string]any{"color": red.ID} supplied := map[string]any{"color": red.ID} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -367,3 +367,16 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing t.Fatalf("supplied override lost: %v", fields["color"]) } } + +// carriedFrom snapshots a field map so it can be passed as the migrate +// function's `sourceFields` — i.e. "every one of these came off the source +// item". A snapshot rather than the map itself because the function DELETES +// dropped keys from fieldMap, and aliasing the two would make the origin +// classification depend on mutation order. +func carriedFrom(fields map[string]any) map[string]any { + out := make(map[string]any, len(fields)) + for k, v := range fields { + out[k] = v + } + return out +}