Skip to content

feat(data-fetching): scope docStore's cache to the signed-in user automatically - #1006

Open
netchampfaris wants to merge 1 commit into
mainfrom
forge/user-scoped-doc-cache
Open

feat(data-fetching): scope docStore's cache to the signed-in user automatically#1006
netchampfaris wants to merge 1 commit into
mainfrom
forge/user-scoped-doc-cache

Conversation

@netchampfaris

@netchampfaris netchampfaris commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Problem

useDoc caches every fetched document unconditionally in docStore, keyed only by `${doctype}/${name}`, both in memory and in IndexedDB. On a shared browser, user B signing in can be served user A's cached document — offline, or even briefly before B's own fetch resolves. This is a cross-account data leak.

useList and useCall already avoid this via a per-call cacheKey (frappe/gameplan#516 uses it to scope Gameplan's list/call caches to the session user), but that pattern doesn't transfer to useDoc: those two only persist to IDB when a caller opts in with cacheKey, so namespacing the key is something the opted-in call site can do. useDoc persists on every fetch with no opt-in point at all.

Design: automatic, zero adoption code

This PR went through two earlier shapes before landing here — a per-call useDoc({ cacheKey }) option, then a global setCacheNamespace(namespace) function apps would call once at session start. Both were opt-in, and opt-in is the wrong shape for this specific problem: any call an app has to remember to make is a call it can forget, and a forgotten one leaks silently by default — which is the exact failure mode this PR exists to close. There's nothing to forget if there's nothing to call.

So docStore now derives its namespace itself, automatically, from the standard Frappe session cookie user_id — a cookie every logged-in Frappe app already sets, with nothing for the app to configure. It's read once when the store is constructed and prefixes every doc cache key for the lifetime of that page load. There is no public API change in this PR at alldocStore was never exported from the package, still isn't, and src/data-fetching/index.ts has a zero-line diff against main.

Cookie handling:

  • The value is decodeURIComponent-ed (Frappe URL-encodes it, e.g. for an email address).
  • A missing cookie, an empty value, or an explicit Guest all resolve to null — the unnamespaced default, identical to today's behavior for a signed-out visitor.
  • typeof document === 'undefined' (SSR, or a non-browser test environment) also resolves to null.

Purge-on-switch, adapted to being automatic

There's no explicit "switch" event anymore, so detection moves to construction time: the store records the last-seen namespace in IDB (a reserved meta key, not a doc:-prefixed one, so it can never collide with a real doctype/name). Each time the store is constructed it compares the freshly-read cookie namespace against that record:

  • Both non-null and different → a real account handoff. The outgoing namespace's docs are purged from IDB (scanned by key prefix, the same way clearAll() already scans) rather than just left fenced off behind their own prefix indefinitely — a shared browser shouldn't keep accumulating every past account's cached docs forever.
  • No stored record yet (first time this browser has ever recorded a namespace) → nothing is purged. There's no prior namespace to distrust, and wiping IDB here would throw away a returning user's own offline cache — the entire point of this caching layer (see the offline-support work in frappe/gameplan).
  • A Guest/no-cookie load never touches the record at all, so an intervening logged-out page load can't erase the trail — the next real login is still compared against the last account that was actually signed in.

Limitation, called out explicitly: namespace changes are only ever detected at construction (page load) — there's no live cookie watcher. In practice this is fine, because a Frappe login/logout flow reloads the document, which re-runs this module and constructs a fresh store. An app that somehow swapped the session user without a full reload would not see the new namespace take effect (or the purge fire) until the next load.

Tests

Reworked src/data-fetching/docStore.test.ts's cache namespacing group to drive document.cookie directly (this file runs under @vitest-environment node, so document doesn't exist unless a test stubs it) and construct fresh DocStore instances to stand in for separate page loads:

  • no cookie, and an explicit Guest cookie, behave byte-identically to before this feature (bare doctype/name keys)
  • a signed-in user gets keys prefixed with their (decoded) cookie value
  • the same user across two inits keeps their cache
  • a different user across inits purges the outgoing account's docs and can't read them
  • the first namespaced init doesn't purge data cached before any user was known

yarn test: 54 files / 473 tests, all green. No pre-existing failures encountered.

Adoption

None needed. Gameplan (frappe/gameplan#516) already scopes its own useList/useCall caches to the session user with an explicit cacheKey; useDoc now gets the same protection automatically, with no code change on the Gameplan side.

🤖 Generated with Claude Code

Docs preview: https://ui.frappe.io/pr-preview/pr-1006/

Coverage: 64.27% (+0.05% vs main)

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 3/5

This PR is not safe to merge until open tabs update or invalidate their cache namespace when the authenticated account changes.

A live tab retains account A’s namespace after another tab changes the shared session to account B, allowing A’s cached documents to be displayed under B’s session and B’s responses to be stored under A.

Files Needing Attention: src/data-fetching/docStore.ts

Security Review

A tab retained across an account change continues serving and writing through the previous account’s namespace.

Fix All in Claude Code Fix All in Codex

Reviews (3): Last reviewed commit: "feat(data-fetching): scope docStore's ca..." | Re-trigger Greptile

Comment thread src/data-fetching/docStore.ts
Comment thread src/data-fetching/docStore.ts Outdated
@barista-for-frappe

barista-for-frappe Bot commented Aug 9, 2026

Copy link
Copy Markdown

Concerns (2/5) — the namespace shape is right, but the switch path doesn't fully deliver the isolation the PR claims.

  • src/data-fetching/docStore.ts:100 — a live useDoc keeps rendering the previous account's doc after a switch. namespace is a plain field, so useDoc's doc computed (useDoc.ts:166) doesn't re-run on setCacheNamespace, and cleanup() (:248) deletes the map entry without ever setting docRef.value = null. The mounted component still holds that ref with alice's data on it. Nulling the ref before docs.delete(key) in the purge path would push subscribers to re-evaluate, hit the new key, and refetch. This is exactly the logout-without-reload case the doc comment calls out.
  • src/data-fetching/docStore.ts:102purgeNamespace only scans this.docs (in-memory), so IDB rows for that namespace written in an earlier page load are never deleted. Combined with "first call never purges", a shared browser still accumulates every past account's docs indefinitely — the thing the PR says it prevents. clearAll() at :261 already has the pattern: filter idbStore.keys() by storePrefix + prefix.
  • docStore.test.ts:259 — nit: the "does not collide" test ends by comparing two strings built by the test's own helper (aliceKey !== bobKey). That asserts nothing about the store; bob never writes. The subscribeLikeUseDoc helper at :53 is the one that would have caught the first finding.
  • src/data-fetching/docStore.ts:74 — nit: the doc says "pass null (or omit)", but the param isn't optional, so setCacheNamespace() is a type error. Make it namespace?: string | null.
  • Nit: docStore is newly public but docs/content/docs/data-fetching/use-doc.md doesn't mention setCacheNamespace. An app that never learns this method exists keeps the leak.

The global setter over a per-call cacheKey is the right call for a cache that isn't opt-in, and docStore.setCacheNamespace(...) is properly namespaced rather than a loose helper (P9). Default-unchanged behavior is well covered.

barista · claude-opus-5 · 63 in / 10.0k out · 1215k cached · 151s · $0.783

@netchampfaris

Copy link
Copy Markdown
Contributor Author

I would prefer to not expand the public API surface further, can this be achieved without exposing docStore?

@netchampfaris
netchampfaris force-pushed the forge/user-scoped-doc-cache branch from 5d5aad6 to 29a1cda Compare August 9, 2026 15:17
…omatically

useDoc caches every fetched doc unconditionally, keyed only doctype/name, in
memory and in IndexedDB. On a shared browser, user B signing in can be served
user A's cached doc offline (or before B's own fetch resolves) -- a
cross-account data leak. useList/useCall avoid this with a per-call cacheKey,
but that only works because their persistence is opt-in; useDoc has no such
call site to namespace from.

An earlier version of this change added a setCacheNamespace(namespace) entry
point for apps to call once at session start. Dropped that in favor of doing
it automatically instead: any opt-in API, even a single global call, is a
call site an app can forget, and a missed one leaks silently by default --
the exact failure mode this is supposed to close. There is nothing to forget
if there is nothing to call.

docStore now reads the standard Frappe session cookie (`user_id`, set by
every logged-in Frappe app already) once at construction and prefixes every
doc cache key with it. No public API changes at all -- docStore stays fully
internal, reached only by useDoc/useNewDoc/useDoctype/useList, and this repo's
public barrel (src/data-fetching/index.ts) is untouched.

Because the cookie is only read at construction, a namespace change is only
ever detected on the next page load (a Frappe login/logout flow reloads the
document, so this holds in practice; an app that swapped the session user
without a reload would not see the new namespace take effect until the next
load). The store records the last-seen namespace in IDB across loads; when a
load's namespace differs from the previous one and both are real accounts (not
Guest/no-cookie), the outgoing account's docs are purged from IDB rather than
left to sit there indefinitely. A load with no prior recorded namespace never
purges, so a returning user's own offline cache survives the first time this
runs for them.

Ref frappe/gameplan#516, which worked around this for lists and calls.
@netchampfaris
netchampfaris force-pushed the forge/user-scoped-doc-cache branch from 29a1cda to 0f01590 Compare August 9, 2026 17:01
@netchampfaris netchampfaris changed the title feat(data-fetching): scope docStore's cache to an account namespace feat(data-fetching): scope docStore's cache to the signed-in user automatically Aug 9, 2026
Comment on lines +77 to +81
* no live cookie watcher. In practice that's fine: a Frappe login/logout
* flow reloads the document, which re-runs this module and constructs a
* fresh store. An app that somehow swaps the session user without a reload
* would not see the new namespace take effect until the next load.
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Live namespace remains stale

When another tab changes the shared session from account A to account B, an already-open tab retains A's construction-time namespace while subsequent requests authenticate as B, causing A's cached documents to be displayed in B's session and B's responses to be stored in A's cache.

Knowledge Base Used: Data Fetching Resources

Fix in Claude Code Fix in Codex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant