MemberJunction / MemberJunction/MJ

Design review: cacheInvalidation subscription broadcasts row content (RecordData) to all clients with no permission filter (B50)

Open
#3,242 0 comments 0 reactions 1 assignee Claimed by @rkihm-BC View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.