Write guard trusts OriginalValue as DB provenance; detached Update/Remove can bypass the tenant theft check

[mforce]

Found by the codex review of #561 (slice T2 of epic #530), verified against the tree at 8b28be0f.

The gap

TenantStampInterceptor refuses a Modified or Deleted write whose AccountId is not the resolved tenant's, and for Modified it compares both the original and the current value. The original-value half is what stops a row loaded through IgnoreQueryFilters being relabelled into the current tenant — theft rather than a leak.

That check treats OriginalValue as database provenance. It is only that for an entity that was loaded while tracked.

DbSet.Update and DbSet.Remove attach a detached instance and seed its original values from the caller's own current values. So a hand-built stub whose primary key names another tenant's row, but whose AccountId is set to the current tenant, satisfies both halves of the check — and the generated UPDATE/DELETE keys on the primary key alone.

Why it is not exploitable today

Every repository mutation read is a tracked read behind the tenant query filter, so the snapshot really is the database's. Verified across all current Update/Remove call sites:

Handler	Load
DepleteFlock / ArchiveFlock / ReactivateFlock / UpdateFlock	GetByIdAsync (tracked, filtered)
RemoveFarmLogo / RemoveFarmBanner	GetTrackedAsync
UpdateInventoryItem	GetByIdLockedAsync
UpdateProduct / UpdateEggGrade / UpdateEggUnitConversion / UpdateWaterUsage	GetByIdAsync

No handler constructs an entity, or reads one AsNoTracking, and then hands it to Update/Remove.

So this is a latent soundness gap, not a live bypass. The risk is the next handler: an AsNoTracking read followed by Update would void the theft protection silently, with nothing going red.

What #561 already did about it

• Corrected the interceptor comment, which had overstated OriginalValue as database provenance without qualification.
• Added TrackedMutationReadTests, which pins the precondition the guard depends on — that the repository mutation read is tracked. Flipping FlockRepository.GetByIdAsync to AsNoTracking turns it red (Expected: Unchanged / Actual: Detached), which is exactly the change that would open the hole.

Deliberately not done there: a test asserting that a detached cross-tenant write succeeds. That would pin a known loss as specified behaviour.

What this issue is for

Close it by construction, so the guarantee does not rest on every future repository author preserving a convention. Options, in rough order of cost:

1. Enforce the tenant in the generated SQL for Modified/Deleted rather than trusting the change-tracker snapshot — the strongest, and the only one that does not depend on call-site discipline.
2. Forbid DbSet.Update / DbSet.Remove in repositories, requiring tracked mutation only, with a guard test walking src/Cluckwork.Infrastructure/Repositories to enforce it. Cheaper, but it is still a convention — just a mechanically-checked one.
3. Extend TrackedMutationReadTests to cover every repository mutation read rather than the one representative it pins today. Cheapest, narrowest, and leaves the underlying mechanism intact.

Worth doing alongside T8 (#536), the cross-tenant isolation hardening slice, whose negative isolation matrix is the natural place to prove the closed behaviour.

Verify

Whatever the fix, it must come with a test that fails when the fix is reverted — a detached stub carrying another tenant's primary key and this tenant's AccountId must be refused, and the refusal must be the assertion that goes red.

[mforce]

Shipped in #671 (merge 4d1dfa37, 2026-09-02). What closed it: AccountId is an EF concurrency token on every entity that carries one — set by a model walk, so a new entity is covered automatically — and the database refuses a detached stub's UPDATE/DELETE whatever the change tracker believed. Reproduced and pinned in three shapes (DetachedTenantWriteTests): Update(stub), Remove(stub), and an Attach as Unchanged with only the owned Money edited — that third one was live, not latent. Refusals under a resolved tenant log Tenant.WriteRefusedByDatabase; the log path cannot replace the exception the caller sees (LoggerFailure_DoesNotChangeTheException).

Deliberately not closed here, tracked separately: #670 (Identity's AccountId-less tables, AspNetUserRoles is live RBAC state) and #673 (a nullable or converted AccountId is fail-open in both layers; the model guard names one the day it is mapped). Option 3 of this issue (extend TrackedMutationReadTests) had already shipped in #561's round 2.