test: enumerating tenant-bypass guard + two-farm isolation matrix (#536) - #584
Conversation
…rd and two-farm matrix Design and plan for #536 (T8, epic #530): an enumerating tenant-bypass guard (Roslyn walk + EF model-driven discovery + committed allow-list) and a two-farm end-to-end isolation matrix. Design reviewed by a fresh-context claude pass (8 merge-blocking findings, all folded in); plan reviewed by a second claude pass scoped to invariants (5 merge-blocking, 2 follow-up, all folded in — including the EF 10 GetQueryFilter obsolescence, verified against the resolved 10.0.11 dll). Owner signoffs: design + amendments (Phase 5/8), D2 (file-and-continue), D3 (shared seeder justification), hook-budget proceed.
Task 1 (part) of #536. The guard's banned surface is derived from the EF model (GetDeclaredQueryFilters — the pre-10 GetQueryFilter is [Obsolete] in EF 10.0.11 and a build error under warnings-as-errors), not hand-recalled. Pinned filter-free surface: ApplicationUser, ApplicationRole, RefreshToken, IdempotencyRecord, and the six Identity tables. Deliberate exclusions, each stated in the test: DurableJob (job scheduling, no tenant by design — #271) and Money (owned value types, no DbSet, reachable only through their filtered owner). Includes the declared-filter equivalence probe (Account filtered, ApplicationUser not) that justifies using GetDeclaredQueryFilters for filter-ness. Adds Microsoft.CodeAnalysis.CSharp 5.0.0 (the SDK 10.0.400-bundled Roslyn, C# 14) and the Infrastructure project reference to the test project; lock files regenerated in the same commit.
Task 1 (completion) of #536. GuardScanner walks every .cs under src/ with Roslyn (420 files, zero parse errors) and reports banned occurrences: IgnoreQueryFilters (36 — matching the code-only baseline; the 16 comment mentions are structurally absent from the syntax tree, which is the false-positive control), raw-SQL APIs (13), Identity string-lookups, SignInManager, UserManager.Users, and filter-free DbSet accesses (separate leg fed by the model-discovered set). False-green guards are inside the scanner (review M2): any parse error fails the scan, a 150-file floor proves the walk saw the tree, bin/obj excluded by path, and the repo root resolves by walking up to Cluckwork.sln — a missing root fails, never defaults. Enclosing methods are keyed in reconstructed symbol display form (Namespace.Type.Method(paramText)); calls inside local functions key as ContainingMethod.Local(name) and are NOT covered by the parent's entry (M7). The allow-list JSON (empty for now — Task 3 populates it) is copied to the test output.
Task 2 of #536. Four named behaviours, each its own assertion against a temp source tree (never the repo), so the mutation matrix can aim at exactly these and a real-tree mutant reds the real-tree test instead: * UnlistedBypass_Fails — a bypass in a non-allow-listed method is unexcused * AllowListedBypass_IsExcused — an entry excuses it; deleting the entry un-excuses it again (same assertion, same lever) * StaleEntry_Fails — an entry matching zero sites fails (a deleted bypass must not leave a live exemption) * MissingAccountIdCompare_Fails — a filter-free-set query without an AccountId comparison is flagged; with one, it is not (shape, not provenance — review M4/F4) * WrapperForwarding_Fails — an extension-method wrapper that forwards IgnoreQueryFilters() is itself an unexcused occurrence; allow-listing it does not excuse callers (review M6) The scanner's enclosing-method symbol is the reconstructed display form Namespace.Type.Method(paramText). Fixed a symbol-ordering bug found while making the temp-tree tests pass: the namespace was added before the type walk and then the whole list reversed, yielding R.A instead of A.R — types are now collected, reversed to outermost-first, and the namespace prepended.
…walk + stability leg Task 3 of #536. The guard now runs against the ACTUAL src/ tree and fails the build on any unexcused bypass, a stale allow-list entry, or a raw-SQL row lock missing an AccountId predicate. Three real-tree tests (TenantBypassRealTreeTests): * RealSourceTree_AllBypassesAreAllowListed — the core gate. All 49 bypass sites (36 IgnoreQueryFilters + 13 raw-SQL FOR UPDATE/FOR SHARE) are on a committed, justified allow-list (Data/tenant-bypass-allowlist.json). * RealSourceTree_FilterFreeSetSitesAreStableAndClassified — the db.<tenant- table> leg, implemented as a STABILITY test (option 1, per the design's implementation-resolution note). The shape check cannot distinguish a by-id/by-hash/caller-scoped query from an unscoped leak, so the 16 candidate sites are classified in Data/filter-free-set-sites.tsv with the REASON each is scoped; the test fails only on drift (a new unclassified candidate, a disappeared site, or a needs-review sentinel). * GraphCompletenessCrossCheck_DocumentedNotYetActive — a placeholder that activates when #583 (graphify code<->table linkage) lands. Scanner additions: * A raw-SQL row-lock predicate walk (M3/M4 lever): every ExecuteSqlRaw / FromSqlRaw / *Interpolated / Sql(...) statement carrying FOR UPDATE or FOR SHARE must name AccountId in its SQL text, checked independent of the allow-list. All 13 current lock queries carry it. * A meaningful file-count floor (RealTreeFileFloor=400, static, below the actual 420) replacing the tautological files.Count floor the reviewer flagged as a false-green. * A DbContext-receiver check so in-memory domain collections (user.Roles, actor.Roles) are not mistaken for db.<Table> query accesses. Mutation matrix (all run, all red on the named assertion, all reverted): * unlisted IgnoreQueryFilters -> RealSourceTree_AllBypasses red * removed IgnoreQueryFilters (stale entry) -> RealSourceTree_AllBypasses red * dropped AccountId from a FOR UPDATE raw SQL -> real-tree gate red (M4) * removed Flock query filter (filter-free set grows) -> discovery floor red * syntax-error file -> ParseError_IsSurfacedNotSwallowed red * floor tautology -> RealTreeFileFloor_IsMeaningfulNotTautological guards The db.<tenant-table> predicate gate was demoted to the stability leg after testing showed the shape check flags 16 sites, most defensible by by-id/by-hash scoping the shape cannot prove. graphify was evaluated and ruled out as the guard tool (no db.<DbSet> call-site or SQL<->table links); the missing linkage is scoped in #583, fed by the existing tools/schema-docs generator. Design doc updated with an implementation-resolution section recording option 1, the raw-SQL walk, the graphify finding, and the mutation results. 165 tests green in Cluckwork.Application.Tests (149 pre-guard + 16 guard).
Task 4 of #536 (Part 2). Two farms are provisioned THROUGH AccountProvisioner.ProvisionAsync (not the seeders — #533's parity guard and this fixture describe the same farm), both owners log in with the returned temporary passwords, change-password clears MustChangePassword (the first-run flow), and farm B drives the egg loop. Then: * VISIBILITY: A sees none of B's egg lots / sales orders / flocks / audit rows by B's real ids, and B sees none of A's pre-existing lot (both directions). * NEGATIVE ISOLATION (Q1: 404 + no mutation): A's owner, fully authenticated, targets B's REAL ids — confirm B's order, read B's order, post a daily entry against B's flock. Every one is a 404 (masked NotFound, no existence leak) AND B's rows stay unmutated (lot still at 90 after the failed confirm; no daily entry created in B's farm). This is the runtime proof the Part 1 guard protects: the guard fails the build on a bypass, and the matrix proves the pipeline actually scopes at runtime with real ids crossing the tenant boundary. Two-layer defense (the mutation must defeat BOTH): the 404 is upheld by (1) the global query filter scoping db.SalesOrders to A, and (2) the confirm handler's post-load AccountId check (TenantMismatch, also surfaced as 404). The mutation that reds the matrix is both — IgnoreQueryFilters on SalesOrderRepository. GetByIdAsync AND deleting the order.AccountId != accountId check — then A's confirm of B's order proceeds (422 NotDraft) and the 404 assertion reds. Run and verified red, then reverted. The test documents this so a single-layer bypass is not mistaken for the leak the matrix guards against. change-password revokes the prior token (credential-epoch bump, #364) and returns a fresh access token in the body; the fixture rebuilds the authed client from that response — reusing the pre-change token 401s on the next call. Design doc updated with a Part 2 implementation-resolution section recording the two-layer finding and the token-revocation rebuild. 1421 integration tests green (no regressions).
mforce
added
enhancement
|
You have reached your Codex usage limits for security reviews. Please try again later. |
…CI wiring The design doc's Definition of Done now carries the actual results: full solution 1,936 passed / 0 failed (baseline 1,919 + 17), the mutation matrix results (Part 1 six mutants + Part 2 two-layer-leak), the pre-commit hook and CI wiring (no changes needed — the guard is in Cluckwork.Application.Tests and the matrix in Cluckwork.Api.IntegrationTests, both in dotnet test Cluckwork.sln), and the guard wall time (~1s tests, within the hook's 2s tripwire target).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f74aaa9d89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["FromSqlRaw"] = BypassKind.RawSql, | ||
| ["FromSqlInterpolated"] = BypassKind.RawSql, | ||
| ["ExecuteSqlRaw"] = BypassKind.RawSql, | ||
| ["ExecuteSqlInterpolated"] = BypassKind.RawSql, | ||
| ["SqlQuery"] = BypassKind.RawSql, |
There was a problem hiding this comment.
The exact-name list omits ExecuteSqlRawAsync and ExecuteSqlInterpolatedAsync, even though the current source already uses ExecuteSqlInterpolatedAsync in FirstRunAdminService and IdempotencyMiddleware. These calls are therefore neither reported as bypass occurrences nor inspected by the row-lock predicate pass, so a new async raw-SQL query—including a tenant-unsafe lock—can leave this guard green. Include the async variants in both raw-SQL method checks.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
| var propertyNames = typeof(AppDbContext).GetProperties() | ||
| .Where(p => p.PropertyType.IsGenericType | ||
| && p.PropertyType.GetGenericTypeDefinition() == typeof(Microsoft.EntityFrameworkCore.DbSet<>) | ||
| && filterFreeEntityTypes.Contains(p.PropertyType.GetGenericArguments()[0]) | ||
| && p.PropertyType.GetGenericArguments()[0].GetProperty("AccountId") != null) |
There was a problem hiding this comment.
Restricting propertyNames to entity types with an AccountId property removes UserRoles, UserClaims, UserLogins, and the other filter-free Identity sets from the real-tree scan. This contradicts the discovery test's explicit rule that any query against those sets requires review; current db.UserRoles queries in AccountUserDirectory, IdentityProvider, and FirstRunStatusService are already invisible to the guard, and a future unscoped query would also pass. Walk every discovered filter-free DbSet, with separate predicate/classification handling for sets without AccountId.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
| if (methodName is not null && BannedMethods.TryGetValue(methodName, out var kind)) | ||
| { | ||
| occurrences.Add(MakeOccurrence(kind, repoRoot, file, invocation, $"{methodName}({receiverText})")); | ||
| continue; |
There was a problem hiding this comment.
The scanner reports only invocations whose method name is directly in BannedMethods; it never records callers of a method that forwards IgnoreQueryFilters(). Consequently, after the wrapper itself is allow-listed, unlimited calls such as query.Unfiltered() remain green—the exact laundering scenario the wrapper test claims to prevent. The test currently asserts only that the wrapper definition is reported, so it does not expose this false green; identify forwarding methods and treat their call sites as occurrences too.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
| // UserManager.Users — a member access, not an invocation. | ||
| foreach (var access in root.DescendantNodes().OfType<MemberAccessExpressionSyntax>()) | ||
| { | ||
| if (access.Name.Identifier.ValueText == "Users" | ||
| && access.Expression.ToString().Contains("UserManager", StringComparison.Ordinal)) | ||
| { |
There was a problem hiding this comment.
This check examines receiver source text rather than its type, so the conventional userManager.Users receiver does not contain the case-sensitive string UserManager and is silently ignored; the analogous signInManager.PasswordSignInAsync(...) check has the same defect. Any future use of these explicitly banned Identity APIs through normal camel-case variables therefore bypasses the gate. Use Roslyn symbol information or derive manager-typed parameter/field names before scanning member accesses.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
…apper callers, camelCase Identity Codex (chatgpt-codex-connector) reviewed PR #536's guard and raised 4 findings (3x P1, 1x P2). All verified real against the code and fixed; each fix is proven by a mutation that reds on the named assertion (run, red, reverted, rebuilt green — full solution 1,936 passed / 0 failed after the fixes). P1-1 (async raw-SQL): ExecuteSqlRawAsync/ExecuteSqlInterpolatedAsync were omitted from the banned list + raw-SQL predicate walk, though 5 current src/ sites use the async form. Added both to BannedMethods and the predicate walk's method set; allow-listed the 3 newly-reported idempotency_records async sites (all carry WHERE AccountId, not row locks). P1-2 (non-tenant track): the filter-free leg walked only tenant sets (entity has an AccountId property), leaving UserRoles/Roles/UserClaims/ UserLogins/UserTokens/RoleClaims/DurableJobs invisible — a future unscoped db.UserRoles query passed silently. The leg now walks ALL filter-free DbSets in two tracks: tenant (predicate rule) + non-tenant (every db.<Table> access is a candidate). Added the 19 non-tenant sites to the TSV with scoping reasons. Note: AspNetUserRoles has columns [UserId, RoleId] only (no AccountId column) — the earlier 'HAS AccountId' was a grep of the mermaid Relations block — so it is correctly non-tenant, scoped by the join to db.Users. P1-3 (wrapper forwarding): the test asserted only the wrapper DEFINITION was reported, not the CALLER; allow-listing the wrapper left callers green (the laundering the test claimed to prevent). The scanner now does a two-pass walk: find methods whose body contains a banned call (forwarding wrappers), then flag their call sites. The test now asserts the caller is reported too. This surfaced 8 real forwarding call sites in src/ (previously invisible), all allow-listed. Stated limitation: same-file only (a syntax walk cannot resolve cross-file wrappers). P2-4 (camelCase Identity receivers): the receiver check was case-sensitive text-matching (Contains('UserManager')); a camelCase userManager.Users / signInManager.X was silently ignored. Replaced with IsIdentityManagerReceiver, which matches the generic type text OR a conventional receiver name. No current src/ sites use the camelCase form, so this is future-proofing (hence P2). Design doc updated with a Codex-review-findings section recording all 4, the fixes, and the mutation proofs.
|
All four findings addressed in P1 — Include async raw-SQL methods in the banned surface ✅ P1 — Walk filter-free tables that lack AccountId ✅ P1 — Propagate bypass status through forwarding wrappers ✅ P2 — Resolve Identity manager receivers instead of matching spelling ✅ |
…e predicate, comparison-shape filter
A second independent reviewer (a pi agent on deepseek-v4-flash, run in a
herdr tab) re-verified the 4 Codex fixes and found 3 new P1s one layer
deeper — all the text-presence-vs-predicate-shape family. Each is proven by a
mutation that reds on the named assertion (run, red, reverted, rebuilt green —
full solution 1,936 passed / 0 failed after the fixes). Full write-up:
scratchpad/t8-pi-review.md.
P1-1 (EF10 raw-SQL surface): the banned list + predicate walk only covered the
sync/async pair + FromSql*. Added SqlQueryRaw, SqlQueryInterpolated, ExecuteSql,
ExecuteSqlAsync, FromSql, FromSqlAsync to BannedMethods, and the full raw-SQL
set (incl. SqlQuery, SqlQueryRaw, SqlQueryInterpolated, ExecuteSql,
ExecuteSqlAsync, FromSql, FromSqlAsync) to the predicate walk's method set.
Verified against the resolved Microsoft.EntityFrameworkCore.Relational.dll
(10.0.x) API surface. None is used in src/ today — this closes the escape-hatch
surface so a future raw query cannot pick an unbanned entry point. Mutation:
SqlQueryRaw with FOR UPDATE + no AccountId reds on both legs.
P1-2 (WHERE-clause predicate): the raw-SQL predicate walk was
sqlText.Contains("AccountId") — a string-presence check. A lock query with
AccountId in the SELECT list (not the WHERE) passed, locking every row for every
tenant. Replaced with HasAccountIdPredicateInWhereClause: take the SQL up to the
lock keyword, find the last FROM (the outer table source), and require AccountId
after that FROM (in the WHERE/JOIN predicates, not the SELECT list). Still a
text heuristic, strictly stricter than before, still allows the legit src/ forms
(quoted column or {accountId} hole in the WHERE). Mutation: moving AccountId from
the WHERE to the SELECT list in GetByIdLockedAsync now reds (before it passed).
P1-3 (comparison-shape filter): PredicateHasAccountId was
statement.Contains("AccountId"), so a query projecting AccountId
(Select(u => u.AccountId)) read as "has a predicate" and skipped the stability
leg — a cross-tenant by-email enumeration passed. Replaced with
HasAccountIdComparison: AccountId must be in a comparison shape (==, !=, <, >,
<=, >=, =), not merely present. Changed the tenant-track candidate filter from
== false to != true so an unclassifiable (null) site is a candidate too.
Provenance is still the allow-list justification's job. The tightening correctly
surfaced one legit src/ site (IdentityProvider.cs:1261 db.RefreshTokens, a
by-token-hash logout attribution) as a candidate — now classified by-hash in the
TSV. Mutation: db.Users.Where(Email).Select(AccountId).Count() is now an
unclassified candidate (red); before it passed.
Not addressed (recorded in scratchpad/t8-pi-review.md, future-proofing /
coverage-scope, not the false-green P1s): receiver-name whitelist blind spot,
cross-file wrapper laundering (real + honestly documented), the two-farm matrix
asserting the 404 without proving which layer catches it (P2-3, the most
substantive next hardening), matrix "full egg loop" overstating API coverage,
raw SQL in a stored string not flagged.
|
Second review round — the 3 P1s from the independent pi (deepseek-v4-flash) pass are addressed in P1-1 — EF Core 10 raw-SQL surface ✅ P1-2 — WHERE-clause predicate, not string presence ✅ P1-3 — comparison-shape filter, not presence ✅ Not addressed this round (recorded in |
…QL-comment strip, token-based comparison A third independent pass (a claude agent in a fresh herdr tab, reviewing the fix commits specifically, not the original code) re-verified the 4 Codex + 3 pi fixes and REFUTED 1, PARTIALLY REFUTED 2. The refutations were re-proven by running the exact mutations the reviewer described (run, red/green as claimed, reverted, full solution 1,936 passed / 0 failed after the fixes). Full write-up: scratchpad/t8-fix-rereview.md. Root cause of all three: text matching cannot tell code from comment/string trivia. Codex P1-3 (REFUTED) — wrapper forwarding: the two-pass walk collected forwarding names only from MethodDeclarationSyntax. An expression-bodied PROPERTY wrapper (private IQueryable<T> X => db.T.FromSql(...).IgnoreQueryFilters()) and a LOCAL-FUNCTION wrapper were invisible, so a same-file caller laundered past an allow-listed definition — the fix's own stated scope. Widen forwardingNames to methods + LocalFunctionStatementSyntax + PropertyDeclarationSyntax (ExpressionBody and accessor ArrowExpressionClause forms); flag the property-wrapper use site as a MEMBER ACCESS (not an invocation — X.Select(...) is a member access, so the invocation loop never saw it). Mutation: property wrapper + caller, property def allow-listed -> caller now reds as 'forwards-bypass (property) <name>' (was green). pi P1-2 (PARTLY REFUTED) — WHERE-clause predicate: (a) a -- SQL comment in the predicate region naming AccountId laundered a whole-table FOR UPDATE (the repo's own house style puts -- comments inside FOR UPDATE WHERE clauses — EggLotRepository.cs:44-48 — so this is the real shape, not contrived); (b) a CTE lock was falsely rejected because LastIndexOf(FROM) landed on the innermost FROM, excluding the CTE's scoping WHERE. StripSqlComments (--...\n and /*...*/) before the region check; test the region after EVERY FROM (not just the last) and accept if any contains AccountId. The SELECT-list false-green stays closed (the projection is before the first FROM). Mutation: (a) AccountId only in a -- comment -> now reds (was green); (b) a scoped CTE -> now passes (was a false-red). pi P1-3 (PARTLY REFUTED) — comparison-shape filter: HasAccountIdComparison matched on statement.ToString(), which includes comment trivia and string-literal text. A '// scoped by AccountId == tenant.AccountId' comment beside a real cross-tenant db.Users.Where(Email == x) laundered the statement. False-red: accountIds.Contains (u.AccountId) and u.AccountId.Equals(id) no longer read as predicates. Compare on TOKENS (StatementCodeOnly): drop StringLiteralToken / InterpolatedStringTextToken text and comment trivia, keep code + whitespace. Extend the regex for Contains(...AccountId...) and AccountId.Equals(...). Mutation: (a) real cross-tenant enum with AccountId == only in a comment -> now an unclassified candidate (was green); (b) accountIds.Contains(u.AccountId) scoping -> now passes (was a false-red). The 4 confirmed fixes (Codex P1-1 async raw-SQL, Codex P1-2 non-tenant track, Codex P2-4 camelCase for named forms, pi P1-1 full EF10 raw-SQL surface) held under the re-review. Claude reflected the real Microsoft.EntityFrameworkCore. Relational 10.0.11 API via MetadataLoadContext and confirmed all 11 raw-SQL entry points are banned + predicate-walked (two names — SqlQueryInterpolated, FromSqlAsync — do not exist in EF 10; harmless over-inclusion).
|
Third review round — the Claude re-review (fresh-context, reviewing the fix commits specifically) refuted 1 and partially refuted 2 of the prior fixes. All 3 defects are addressed in Root cause of all three: text matching cannot tell code from comment/string trivia. Codex P1-3 — wrapper forwarding (REFUTED) ✅ pi P1-2 — WHERE-clause predicate (PARTLY REFUTED) ✅ pi P1-3 — comparison-shape filter (PARTLY REFUTED) ✅ Confirmed (held under re-review): Codex P1-1 (async raw-SQL), Codex P1-2 (non-tenant track), Codex P2-4 (camelCase, named forms), pi P1-1 (full EF10 raw-SQL surface — Claude reflected the real |
…document F3/F4/F7
A fourth independent pass (a pi agent on kimi-k3, reviewing the three fix
commits) found 7 new findings. The three clear ones are fixed; the other three
are the same 'narrower escape hole' family and are documented as known
limitations, because each prior fix in that family has opened another (F1 itself
was a regression introduced by the round-4 every-FROM fix). Full write-up:
scratchpad/t8-round4-review.md. The reviewer walked all 9 real src/ lock sites +
the advisory pair + the CTE + every TSV line and found no false-red from the
round-4 tightening on the actual codebase.
F1 (REGRESSION introduced by round 4) — the every-FROM fix opened a
multi-statement false-green: a batched raw SQL whose first statement is scoped
(DELETE ... WHERE AccountId = ...) and whose second takes an unscoped lock
passed, because the every-FROM rule found AccountId in the first statement's
FROM region. Round 3's last-FROM logic read the lock's own statement and would
have caught it. The predicate walk is now statement-aware: split the stripped
SQL on ';' and apply the every-FROM rule only to the statement containing the
lock keyword. A CTE stays inside one statement, so the CTE fix survives; a
previous statement's AccountId can no longer launder the lock. Mutation: 1st
stmt scoped + 2nd unscoped lock -> now reds (was green); scoped CTE -> still
passes.
F5 (FALSE-GREEN) — FOR NO KEY UPDATE / FOR KEY SHARE never entered the
predicate walk; the hasLock gate matched only FOR UPDATE / FOR SHARE even
though the prefix loop already listed all four. Extracted HasRowLockKeyword (all
four Postgres row-lock keywords) and used it for both the gate and the prefix
loop. Mutation: FOR NO KEY UPDATE lock with AccountId dropped -> now reds (was
green).
F6 (code-comment falsehood) — StatementCodeOnly's comment claimed
'interpolated-string holes are dropped too - they are string content.' Wrong: a
hole's contents are ordinary code tokens and survive, so a comparison embedded
in a string hole launders (a contrived false-green). Corrected the comment to
state the hole case is a known limitation, not closed. Closing it requires
tracking which tokens are inside an interpolated string - a larger change than
the guard's budget warrants. No code change.
Documented as limitations (not fixed - the narrower-hole family, where each fix
risks opening another, as F1 proved):
F3 - this.-rooted property-wrapper use site (and 'var q = X; q.Select(...)')
escapes the member-access root walk.
F4 - wrappers forwarding a banned MEMBER ACCESS (UserManager.Users) are never
registered; RecordForwarding only matches banned invocations.
F7 - '--' inside a SQL string literal eats the predicate (StripSqlComments is
not quote-aware); false-red-only, no current src/ site has this shape.
Guard suite 16/16, full app project 165/165, two-farm matrix passes. (One
unrelated flaky OTLP sidecar-boot integration test failed in the full run and
passed in isolation - it spawns a subprocess and is timing-sensitive; the guard
only touches test-scanner files, not the API runtime.)
|
Round 4 (pi on kimi-k3, fresh-context review of the three fix commits) found 7 new findings. The three clear ones are fixed in F1 — REGRESSION introduced by round 4 (FIXED). The every-FROM fix opened a multi-statement false-green: a batched raw SQL whose first statement is scoped ( F5 — FALSE-GREEN (FIXED). F6 — code-comment falsehood (FIXED, comment only). Documented as limitations (not fixed):
The reviewer's overall finding, stated plainly: round 4 holds for every case it names, but the syntax-walk heuristic will keep yielding this class of finding (text-presence vs. structural position) one layer deeper per pass. The residual items (F3/F4/F7 plus the earlier-documented receiver-name whitelist, cross-file wrapper laundering, |
2 participants
What
T8 of Epic #530 — cross-tenant isolation hardening. Two parts:
Part 1 — an enumerating guard that fails the build if any tenant-filter
bypass in
src/is not on an explicit, justified allow-list. Implemented asApproach C (Roslyn walk + EF Core model discovery), per the reviewed design.
Part 2 — a two-farm E2E matrix that provisions two farms through
AccountProvisionerand proves the pipeline scopes at runtime (visibility +negative isolation with real ids crossing the tenant boundary).
Part 1 — the guard
tests/Cluckwork.Application.Tests/TenantBypass/(new). Four test classes:model (
GetDeclaredQueryFilters()), pinned to the 9 filter-free types with astated exclusion policy. Removing a
HasQueryFilterreds the floor.src/files, zero parse errors, a meaningfulfile-count floor (400, static), and false-green guards (parse-error
surfacing, floor-not-tautological). Banned:
IgnoreQueryFilters, raw-SQL(
ExecuteSqlRaw/FromSql*/Sql), Identity lookups (FindByEmailAsyncetc.),
SignInManager,UserManager.Users.is excused, stale entry fails, missing-AccountId fails, wrapper-forwarding
fails.
RealSourceTree_AllBypassesAreAllowListed(all 49 siteson a justified allow-list), a raw-SQL row-lock predicate walk (every
FOR UPDATE/FOR SHAREmust nameAccountIdin its SQL text, independentof the allow-list), and a stability leg for
db.<tenant-table>queries.The
db.<tenant-table>leg is a stability test, not a shape gateThe "does the statement name an AccountId compare" shape check flagged 16 sites,
most defensible by by-id/by-hash scoping the shape cannot prove (reviewer
M4/F4: "shape, not provenance"). So instead of auto-failing on shape, the 16
candidate sites are classified in
Data/filter-free-set-sites.tsvwith theREASON each is scoped, and the leg fails only on drift (a new unclassified
candidate, a disappeared site, or a
needs-reviewsentinel). Nothing issilently un-banned — a new unscoped
db.<tenant-table>query cannot landwithout a classification decision.
Part 2 — the two-farm matrix
tests/Cluckwork.Api.IntegrationTests/TwoFarmIsolationMatrixTests.cs(new).Two farms provisioned through
AccountProvisioner.ProvisionAsync(not theseeders — #533 parity), owners log in with the returned temp passwords,
change-passwordclearsMustChangePassword, farm B drives the egg loop.Then: A sees none of B's rows by B's real ids (and vice versa), and A's
authenticated attempts to confirm/read B's order or post a daily entry against
B's flock are all 404 + no mutation.
Two-layer defense
The 404 is upheld by (1) the global query filter and (2) the confirm
handler's post-load AccountId check. The mutation that reds the matrix defeats
both (bypass the filter AND delete the check) → A's confirm of B's order
proceeds (422 NotDraft). Run, red, reverted.
Verification
dotnet build Cluckwork.sln -c Release— clean.dotnet test Cluckwork.sln -c Release --no-build— 1,936 passed, 0failed (baseline 1,919 + 17 new: 16 guard + 1 matrix). No regressions.
assertion, all reverted, rebuilt green. See the design doc's
implementation-resolution sections.
Cluckwork.Application.Tests, which thehook already runs on
.csstaging — a staged unlisted bypass makes it exit1 (verified). No hook change needed.
build-and-testrunsdotnet test Cluckwork.sln, which includes boththe guard and the matrix. No CI change needed.
Follow-up
db.<DbSet>query call-sites and raw-SQL → tablelinks (fed by the existing
tools/schema-docsgenerator). The guard shipswith a documented (not yet enforced) graph-completeness cross-check
placeholder that activates when graphify: model db.<DbSet> query call-sites and raw-SQL → table links #583 lands. graphify was evaluated and ruled
out as the guard tool (no
db.<DbSet>call-site or SQL↔table links today).Design
docs/superpowers/specs/2026-08-22-t8-cross-tenant-isolation-guard-design.md— reviewed by Claude (8 merge-blocking findings → Approach C), with
implementation-resolution sections recording option 1, the raw-SQL walk, the
graphify finding, and all mutation results.