MemberJunction / MemberJunction/MJ

ProviderBase record-name cache is not user-scoped — safe only because the FLS gate sits in front of it

Open
#4,298 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary

`ProviderBase._entityRecordNameCache` is keyed by **entity + primary key only**, with no user dimension:

```ts
// packages/MJCore/src/generic/providerBase.ts
private _entityRecordNameCache = new MJLruCache({ maxSize: 10000, ttlMs: 60 * 60 * 1000 });

private getCacheKey(entityName: string, compositeKey: CompositeKey): string {
return `${entityName}|${compositeKey.ToString()}`;
}
```

A record's display **name** is ordinary entity data, and field-level security can deny read on the name field like any other column. So the cached value is user-dependent while the key is not.

**This is not a live leak today.** Two gates keep denied names out of the cache in the first place:

- `EntityRecordNameResolver` checks field-level read on the entity's name field and returns `Success: false` before the provider lookup runs;
- `BaseEntity.GetRecordName()` returns `null` for a denied name field instead of calling `Get()` — explicitly, in part so the name never reaches this cache.

The second of those was written *because* the leak was found and fixed once during FLS development. The cache is the reason the fix had to be a null return rather than a caught exception.

## The risk

The safety property lives in the callers, not in the cache. Any future caller that reaches `GetEntityRecordName` / `GetCachedRecordName` / `SetCachedRecordName` without replicating the field-level check re-opens it: one permitted user warms the entry, and every later caller — including one denied read on that name field — is served it straight from memory, with no lookup and therefore no gate.

`SetCachedRecordName` is public and takes a name from the caller, so it is reachable without going through either guarded path at all.

## Options

1. **Add the acting user to the cache key** on FLS-enabled entities (empty segment otherwise, so unrestricted entities keep byte-identical keys and full sharing — the same shape `GenerateRunViewFingerprint` uses for its `rls:` / `fls:` segments).
2. **Move the field-level check into the provider**, so the cache is populated and read behind the gate rather than in front of it, and callers cannot forget it.
3. **Do not cache a name on an FLS-enabled entity at all** — simplest, and the entity count involved is expected to be small.

(2) or (3) look better than (1): the current arrangement's problem is that the invariant is enforced at the call sites, and (1) leaves it there while adding a key dimension.

## References

- `packages/MJCore/src/generic/providerBase.ts` — `getCacheKey`, `GetEntityRecordName`, `GetCachedRecordName`, `SetCachedRecordName`
- `packages/MJCore/src/generic/baseEntity.ts` — `GetRecordName()`, and the comment recording why it returns null
- `packages/MJServer/src/resolvers/EntityRecordNameResolver.ts` — `IsNameFieldDeniedToUser`
- `guides/FIELD_LEVEL_SECURITY_GUIDE.md` §2, "Record names" row

Contributor guide

Open the contributing guide

Research direction

Start with packages/MJCore/src/generic/providerBase.ts and trace getCacheKey, GetEntityRecordName, GetCachedRecordName, and SetCachedRecordName. Then read BaseEntity.GetRecordName(), EntityRecordNameResolver.ts, and the record-names section of guides/FIELD_LEVEL_SECURITY_GUIDE.md. Done means the selected design preserves field-level security for every cache read and write, including direct public-method use, with tests covering permitted and denied users.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.