Repository navigation
AccountId must be a non-nullable Guid: the write guard and the #562 token walk are both fail-open for any other shape #673
Description
Found by the tenant-isolation adversary seat reviewing #562 / PR #671 (2026-09-02). Latent — no such property exists today — but both layers are fail-OPEN for it, and the guard that should catch it mirrors the same predicate.
The shape
Both write-side layers key on AccountId being a non-nullable Guid:
TenantStampInterceptor.Verify/StampOrVerifyAdded:if (value is not Guid accountId) return;— anull(nullableGuid?) or value-convertedAccountIdis silently not checked.- The Write guard trusts OriginalValue as DB provenance; detached Update/Remove can bypass the tenant theft check #562 model walk in
AppDbContext.OnModelCreating:if (accountId is null || accountId.ClrType != typeof(Guid)) continue;— the same property gets no concurrency token.
So a future tenant-owned entity mapped with public Guid? AccountId (or a strongly-typed id with a converter) reopens exactly the #562 shape: a detached Update(stub)/Remove(stub) with AccountId = null emits UPDATE … WHERE "Id" = @id alone and the database relabels or deletes another farm's row — with every test green.
PR #671's fix round tightened AccountIdConcurrencyTokenModelTests so that the guard now selects carriers by name and non-key only and asserts ClrType == typeof(Guid) — a nullable AccountId reddens the model test instead of vanishing from it. That is a guard, not a mechanism.
Close it by construction
- Make the walk fail closed: an
AccountIdproperty whose CLR type is not exactlyGuidthrows at model build (InvalidOperationExceptionnaming the entity), so a mis-typedAccountIdfails every boot and every test rather than passing unguarded. - Make the interceptor fail closed on the same condition: a non-
GuidAccountIdvalue on anAdded/Modified/Deletedentry under a resolved tenant throwsTenantWriteMismatchException(or a dedicated exception) rather than returning.
Verify
A model-only test that maps a throwaway entity with Guid? AccountId into a ModelBuilder and asserts the walk throws; a mutation that deletes the throw must redden it. For the interceptor, a stub whose AccountId property is boxed as null on a Modified entry must be refused.
Activity
mforce commentedon Sep 3, 2026
Sharper detail from the round-2 adversary seat on PR #671 (2026-09-02), recorded so the by-construction fix covers it:
Guid?boxes to a plainGuid, soTenantStampInterceptor'svalue is not Guidchecks still fire for it — the interceptor is fail-open for that shape only when the value isnull. The model walk, by contrast, skips the whole property (ClrTypeisGuid?, notGuid), so the token layer is fail-open for the type unconditionally.nullcase on Added is the interesting one:StampOrVerifyAddedreturns early on a non-Guidvalue without stamping, so a row would be inserted withAccountId = NULL— invisible to every query filter (e.AccountId == tenant.AccountIdisNULLfor every tenant) and unreachable through the app afterwards. A fail-closed interceptor should refuse that insert, not skip it.PR #671's model guard names any non-
GuidAccountId(AccountIdConcurrencyTokenModelTests), and its message was corrected in the same PR to say exactly the above rather than "fail-open in both".mforce commentedon Sep 3, 2026
One more wording note from the round-3 adversary seat on PR #671, parked here because #673 is where converted-type
AccountIds get handled: the model guard's assertion message says the write guard "skips a null value" — it skips any value that does not box to aGuid, which includes a converter-backed / strongly-typed id that is non-null. The comment above the selector already states the general form; only the message's parenthetical narrows it. One-word fix ("a value that is not aGuid") whenever the file is next touched.chore(main): release 0.1.0 (#542)