MemberJunction / MemberJunction/MJ
Design review: cacheInvalidation subscription broadcasts row content (RecordData) to all clients with no permission filter (B50)
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
**Needs a design decision — this touches the cross-server cache-invalidation architecture, so it's not a drop-in fix. Tagging @rkihm-bc to lead.**
## The issue (B50, found by the `subscription-isolation` integration work)
`CacheInvalidationResolver.cacheInvalidation` (`packages/MJServer/src/generic/CacheInvalidationResolver.ts`) is a `@Subscription` with **no filter at all** — the code comment even states *"No session filter — every browser connected via WebSocket receives every event."* The publisher (`ResolverBase.PublishCacheInvalidation`) attaches the full row on saves:
```ts
recordData: action === 'save' ? JSON.stringify(entityObject.GetAll()) : undefined,
```
So every authenticated browser receives the **full content of every row anyone saves**, with **zero permission filtering**.
### Blast radius (three dimensions)
- **Cross-tenant** (multi-tenant `MJTenantFilterMiddleware` deployments): tenant X's row content reaches tenant Y's browser. Highest severity.
- **Row-level**: within a tenant, a client receives rows it could not `RunView` (RLS is bypassed on this path).
- **Column/field-level**: `GetAll()` serializes *every* column, so even a client that legitimately can see the row may receive fields it has no field-level permission to read.
Precondition is only "be a connected client" — every authenticated browser already subscribes — so reach is broad. It's "content of rows mutated while connected," not a bulk export, but the cross-tenant leak in particular is serious.
## Why it's not a trivial fix — RecordData is load-bearing
`RecordData` is **used**, not dead weight: the client (`GraphQLDataProvider.SubscribeToCacheInvalidation`) forwards it into a `remote-invalidate` event, and `BaseEngine.applyRemoteRecordData` applies the row **in-place** to update engine caches **without a refetch**. This is the cross-server cache-invalidation performance design. Removing it changes that architecture, which is why this is a design review, not a patch.
Mitigating fact for the options below: the client **already has a graceful fallback** — when `recordData` is absent, `BaseEngine` falls through to a permissioned server refetch (`LoadSingleConfig`), which runs today for every delete and every save-without-recordData.
## Options for the team
1. **PK-only** — drop `RecordData`; broadcast only `{EntityName, PrimaryKeyValues, Action}`. Every client takes the existing refetch fallback through `RunView`, which applies RLS **and** field-level permissions in SQL, correctly, once. Correct-by-construction, zero new client code. **Cost:** loses the in-place-update optimization — interested clients refetch instead. This is the security-simplest option but the biggest hit to the caching design.
2. **Per-subscriber filtering** — keep `RecordData`, filter delivery per subscriber (the subscription filter has `context.userPayload`):
- **Entity-permission + tenant** (cheap, metadata lookups only): kills the cross-tenant and cross-entity leaks on the hot path; leaves row- and column-level residual.
- **Full row + column RLS** (expensive/complex): MJ's RLS is a SQL WHERE clause (`GetUserRowLevelSecurityWhereClause`), not a JS predicate — evaluating per-row-per-subscriber needs either a JS predicate evaluator (fragile) or a per-subscriber re-read (defeats the point). Cache invalidation is a hot path → O(mutations × subscribers). Field-level would additionally need per-subscriber column stripping.
3. **Hybrid** — entity-permission + tenant filter as a cheap floor (removes the worst leak), keep `RecordData` for same-tenant same-entity, accept row/column residual, or strip to a permitted-column subset.
## Recommendation to seed the discussion
Lean PK-only for correctness (it reuses the one already-correct permission path and honors field-level perms for free), OR the entity-permission+tenant floor if preserving the in-place optimization matters for the deployment profile. The right call depends on how much the caching design depends on in-place application vs. how multi-tenant the target deployments are.
## Testing
Whatever lands should be pinned by a **two-identity (multi-connection) WebSocket harness** — the integration suite doesn't have one yet (`subscription-isolation.SI2` currently documents this gap rather than testing it). That harness is a planned next-phase item and would de-risk both this and B49 (the sibling `statusUpdates` session-hijack).
Refs: `CacheInvalidationResolver.ts`, `ResolverBase.PublishCacheInvalidation`, `GraphQLDataProvider.SubscribeToCacheInvalidation`, `BaseEngine.applyRemoteRecordData`. Bug register: B50 / B5.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor guide
Assessment
This issue has not been assessed yet.