feat(instances): scope every SpiceDB read to one Frontegg instance (FR-26219) - #75
feat(instances): scope every SpiceDB read to one Frontegg instance (FR-26219)#75dianaKhortiuk-frontegg wants to merge 6 commits into
Conversation
Adds a per-instance schema prefix so one SpiceDB can serve several Frontegg vendors with no cross-instance reads. - InstanceRegistry validates instances at construction, never at request time; no `instances` synthesises a legacy instance with an empty prefix - SchemaScope prefixes object types on the way in, strips on the way out, and rejects any caller type containing '/' - resolveInstance implements the four resolution rules and throws InstanceResolutionException rather than returning a fallback boolean - scope threaded through all four query strategies, both lookup builders, the response mappers and the batch feature path - route relationship cache is keyed per prefix - fallback configuration resolves per instance, then client-wide - guard spec fails on any request object type not built via scope.type() Legacy single-instance behaviour is unchanged: all 171 pre-existing tests pass untouched apart from the added scope argument. FR-26219
…ate raw spiceClient readSchemaFor(instanceId) returns only that prefix's definitions and caveats, closing the schema leak where any caller could read every vendor's schema. Log lines now carry instanceId, and the raw spiceClient escape hatch is marked deprecated for multi-instance use. FR-26219
Code review of #75 found the prefix-escape guard was defeated by the same catch-all this PR was written to fix. - executeEntitlementQuery and resolveFeatureEntitlements now rethrow ConfigurationInputIsInvalidException alongside InstanceResolutionException, so an entityType containing '/' propagates instead of becoming the configured fallback boolean. With defaultFallback: true it was returning an allow. Covered by tests that go through isEntitledTo / isEntitledToMany rather than the strategy directly, which is how the original tests missed it. - InstanceRegistry now defaults its defaultInstanceId parameter from the configuration object, so the config field is honoured and validated at construction instead of being silently dropped. - The scope guard also treats direct client.<method>({...}) calls as request spans, so an object type in a plain literal is no longer invisible to it. - resolveInstance catches a rejected logRequest rather than leaving an unhandled rejection. - Dropped the no-op scope.strip() in the lookup response mappers: it was stripping the caller's own unprefixed input, not anything returned, which read as protection that was not there. FR-26219
mariavlasov
left a comment
mariavlasov
left a comment
There was a problem hiding this comment.
Reviewed this against the description. Two framing notes before the line items:
- This scopes SpiceDB reads/writes for multiple Frontegg instances/vendors behind one shared
engineEndpoint/engineTokenand one SpiceDB, partitioned by schema prefix — it doesn't add support for separate credentials per account (ClientConfigurationstill has a singleengineEndpoint/engineToken). Flagging in case the ask was literally per-account credentials rather than logical partitioning. - There are regexes in both production and test code:
schema-prefix.ts(SPICEDB_PREFIX_PATTERN,.replace(/-/g, '_')),spicedb-entitlements.client.tsreadSchemaFor(split(/\n(?=definition |caveat )/)), andschema-scope.guard.spec.ts(OBJECT_TYPE_FIELD,REQUEST_SPAN_START). Calling this out since regex-free was a stated goal.
Findings
1. [High / data-leak risk] No duplicate-schema-prefix validation — src/instances/instance-registry.ts, assertInstance
assertInstance rejects a duplicate raw instanceId or vendorId, but never checks whether two instances resolve to the same schemaPrefix — either two explicit overrides colliding, or two different vendorIds normalizing to the same derived prefix (deriveSchemaPrefix lowercases and swaps -→_, so e.g. "ACME-CORP" and "acme_corp" derive to the same prefix). If that happens, two vendors silently share the same SpiceDB namespace — the exact cross-tenant leak this PR exists to prevent. Suggest tracking seenSchemaPrefixes alongside seenVendorIds and throwing on collision.
2. [Medium] schema-scope.guard.spec.ts guard is regex-based and admittedly incomplete
The PR description already flags this ("a creative enough refactor could slip past this guard") — agreed, and worth taking seriously since this is the only thing guarding against an unscoped objectType leaking cross-instance data. REQUEST_SPAN_START only recognizes three call shapes; a request built any other way won't be scanned, with no failure signal. Given this backs a real security property, I'd treat "replace with an actual ESLint rule with type info" as a near-term follow-up rather than a someday item.
3. [Low] readSchemaFor's text-split heuristic — spicedb-entitlements.client.ts
schemaText.split(/\n(?=definition |caveat )/) is a plain-text heuristic over the SpiceDB schema DSL rather than a real parse. Low risk of an actual cross-instance leak given the trailing / in the marker check, but it'll silently misplace blocks with leading comments or non-standard formatting. Worth a comment noting the assumption, or a real parser if one's available.
4. [Low] Exception messages enumerate configured instance/vendor IDs
UnknownInstanceException, InstanceIdRequiredException, and the ConfigurationInputIsInvalidException variants all include the full configured instanceIds/vendorId list in the thrown message. Fine if these stay internal, but if a caller (e.g. entitlements-agent) ever bubbles an SDK exception message straight into an API response, that's an info leak of the vendor list to an unauthenticated or cross-tenant caller. Worth a doc note that callers should catch and sanitize before surfacing.
5. [Nit] Dead-looking branch — spicedb-entitlements.client.ts, executeEntitlementQuery catch block
err instanceof InstanceResolutionException can't actually be thrown here anymore since resolution now happens in the caller before this method runs. Harmless as defense-in-depth, but worth a comment on why it's there (or drop it if genuinely unreachable) so it doesn't look like dead code.
6. [Nit] Redundant default-instance-id wiring — entitlements-client-factory.ts
new InstanceRegistry(configuration, configuration.defaultInstanceId) — the second constructor param already defaults to configuration.defaultInstanceId, so this is two ways of saying the same thing.
7. [Nit] API asymmetry — instanceId placement
isEntitledTo/isEntitledToMany take instanceId via a separate options param, while the lookup* methods embed it in the request object (req: X & InstanceOptions). Not wrong, just inconsistent for callers to remember.
Performance-wise nothing stood out: prefix derivation/validation happens once at registry construction (not per request), and the route cache is correctly keyed per schema prefix so it scales with configured instance count, not request volume.
Not requesting changes — flagging #1 as the one I'd want addressed before merge, the rest are discretionary.
Review feedback on #75. - InstanceRegistry now rejects two instances that resolve to the same schemaPrefix, whether from colliding explicit overrides or from two vendorIds that normalise to the same value ("ACME-CORP" and "acme_corp" both derive v_acme_corp). Without this two vendors silently share one SpiceDB namespace, which is the leak this work exists to prevent. Two explicitly-legacy instances are rejected on the same grounds. - readSchemaFor splits the schema by tracking brace depth rather than by a regex lookahead, so blocks that are indented or preceded by comments are attributed correctly instead of being silently dropped or merged. - UnknownInstanceException and InstanceIdRequiredException no longer put the configured instance list in the message; it stays available as configuredInstanceIds for debugging, so a caller that surfaces err.message cannot leak the vendor list. - Dropped the InstanceResolutionException branch from the query catch blocks; resolution happens in the caller, so it was unreachable. - Dropped the redundant defaultInstanceId argument at both InstanceRegistry call sites now that the parameter defaults from the configuration object. FR-26219
…rule The guard was regex over source text: it only recognised three call shapes, so a request built any other way was invisible to it with no failure signal. That was the one thing holding the scoping invariant, so it is now a real ESLint rule with type information. eslint-rules/require-scoped-object-type.js flags any objectType / resourceObjectType / subjectObjectType / resourceType property whose contextual type originates in @authzed/authzed-node and whose value is not a scope.type(...) call. Keying on the declaring package is what separates a genuine request field from a log payload or a value read back off a response, which the regex version could only approximate. Contextual types arrive as PartialMessage<T> | undefined, so the rule walks union members and generic arguments rather than reading the property off the top-level type, which resolves to nothing. Verified against three planted violations: a raw type inside v1.X.create(), a plain object literal passed straight to client.checkPermission() (the shape the regex guard missed entirely), and a RelationshipFilter.resourceType. All three are reported; the existing log payloads and response reads are not. schema-scope.guard.spec.ts is deleted. Its replacement asserts the rule stays wired as an error, with a files glob covering src and with parserOptions .project set — without type information the rule silently matches nothing, which is the failure mode worth guarding. Addresses review finding 2 on #75.
|
Thanks — all seven addressed.
One thing back to you: regex-free — I can't find this as a convention here (no CONTRIBUTING or lint rule, and On single credentials — correct, and intentional: that's D5 in the plan (one SpiceDB, one preshared key, so the agent process is the trust boundary). Per-prefix credentials don't exist in self-hosted SpiceDB. |
…method lookupTargetEntities, lookupEntities and lookupEntitlements took instanceId inside the request object while isEntitledTo and isEntitledToMany took it as a trailing options argument. All five now use the options argument, so callers have one thing to remember. Doing it now because the multi-instance API is unreleased; once shipped this becomes a breaking change. Addresses review finding 7 on #75.
|
Split as requested — part 1 is #78 (the scoping mechanism). Part 2 is the review follow-ups: prefix-collision rejection, the typed ESLint rule replacing the regex guard, and Closing this one. |
Review feedback on #75. - InstanceRegistry now rejects two instances that resolve to the same schemaPrefix, whether from colliding explicit overrides or from two vendorIds that normalise to the same value ("ACME-CORP" and "acme_corp" both derive v_acme_corp). Without this two vendors silently share one SpiceDB namespace, which is the leak this work exists to prevent. Two explicitly-legacy instances are rejected on the same grounds. - readSchemaFor splits the schema by tracking brace depth rather than by a regex lookahead, so blocks that are indented or preceded by comments are attributed correctly instead of being silently dropped or merged. - UnknownInstanceException and InstanceIdRequiredException no longer put the configured instance list in the message; it stays available as configuredInstanceIds for debugging, so a caller that surfaces err.message cannot leak the vendor list. - Dropped the InstanceResolutionException branch from the query catch blocks; resolution happens in the caller, so it was unreachable. - Dropped the redundant defaultInstanceId argument at both InstanceRegistry call sites now that the parameter defaults from the configuration object. FR-26219
…rule The guard was regex over source text: it only recognised three call shapes, so a request built any other way was invisible to it with no failure signal. That was the one thing holding the scoping invariant, so it is now a real ESLint rule with type information. eslint-rules/require-scoped-object-type.js flags any objectType / resourceObjectType / subjectObjectType / resourceType property whose contextual type originates in @authzed/authzed-node and whose value is not a scope.type(...) call. Keying on the declaring package is what separates a genuine request field from a log payload or a value read back off a response, which the regex version could only approximate. Contextual types arrive as PartialMessage<T> | undefined, so the rule walks union members and generic arguments rather than reading the property off the top-level type, which resolves to nothing. Verified against three planted violations: a raw type inside v1.X.create(), a plain object literal passed straight to client.checkPermission() (the shape the regex guard missed entirely), and a RelationshipFilter.resourceType. All three are reported; the existing log payloads and response reads are not. schema-scope.guard.spec.ts is deleted. Its replacement asserts the rule stays wired as an error, with a files glob covering src and with parserOptions .project set — without type information the rule silently matches nothing, which is the failure mode worth guarding. Addresses review finding 2 on #75.
…method lookupTargetEntities, lookupEntities and lookupEntitlements took instanceId inside the request object while isEntitledTo and isEntitledToMany took it as a trailing options argument. All five now use the options argument, so callers have one thing to remember. Doing it now because the multi-instance API is unreleased; once shipped this becomes a breaking change. Addresses review finding 7 on #75.
Review feedback on #75. - InstanceRegistry now rejects two instances that resolve to the same schemaPrefix, whether from colliding explicit overrides or from two vendorIds that normalise to the same value ("ACME-CORP" and "acme_corp" both derive v_acme_corp). Without this two vendors silently share one SpiceDB namespace, which is the leak this work exists to prevent. Two explicitly-legacy instances are rejected on the same grounds. - readSchemaFor splits the schema by tracking brace depth rather than by a regex lookahead, so blocks that are indented or preceded by comments are attributed correctly instead of being silently dropped or merged. - UnknownInstanceException and InstanceIdRequiredException no longer put the configured instance list in the message; it stays available as configuredInstanceIds for debugging, so a caller that surfaces err.message cannot leak the vendor list. - Dropped the InstanceResolutionException branch from the query catch blocks; resolution happens in the caller, so it was unreachable. - Dropped the redundant defaultInstanceId argument at both InstanceRegistry call sites now that the parameter defaults from the configuration object. FR-26219
…rule The guard was regex over source text: it only recognised three call shapes, so a request built any other way was invisible to it with no failure signal. That was the one thing holding the scoping invariant, so it is now a real ESLint rule with type information. eslint-rules/require-scoped-object-type.js flags any objectType / resourceObjectType / subjectObjectType / resourceType property whose contextual type originates in @authzed/authzed-node and whose value is not a scope.type(...) call. Keying on the declaring package is what separates a genuine request field from a log payload or a value read back off a response, which the regex version could only approximate. Contextual types arrive as PartialMessage<T> | undefined, so the rule walks union members and generic arguments rather than reading the property off the top-level type, which resolves to nothing. Verified against three planted violations: a raw type inside v1.X.create(), a plain object literal passed straight to client.checkPermission() (the shape the regex guard missed entirely), and a RelationshipFilter.resourceType. All three are reported; the existing log payloads and response reads are not. schema-scope.guard.spec.ts is deleted. Its replacement asserts the rule stays wired as an error, with a files glob covering src and with parserOptions .project set — without type information the rule silently matches nothing, which is the failure mode worth guarding. Addresses review finding 2 on #75.
…method lookupTargetEntities, lookupEntities and lookupEntitlements took instanceId inside the request object while isEntitledTo and isEntitledToMany took it as a trailing options argument. All five now use the options argument, so callers have one thing to remember. Doing it now because the multi-instance API is unreleased; once shipped this becomes a breaking change. Addresses review finding 7 on #75.
Review feedback on #75. - InstanceRegistry now rejects two instances that resolve to the same schemaPrefix, whether from colliding explicit overrides or from two vendorIds that normalise to the same value ("ACME-CORP" and "acme_corp" both derive v_acme_corp). Without this two vendors silently share one SpiceDB namespace, which is the leak this work exists to prevent. Two explicitly-legacy instances are rejected on the same grounds. - readSchemaFor splits the schema by tracking brace depth rather than by a regex lookahead, so blocks that are indented or preceded by comments are attributed correctly instead of being silently dropped or merged. - UnknownInstanceException and InstanceIdRequiredException no longer put the configured instance list in the message; it stays available as configuredInstanceIds for debugging, so a caller that surfaces err.message cannot leak the vendor list. - Dropped the InstanceResolutionException branch from the query catch blocks; resolution happens in the caller, so it was unreachable. - Dropped the redundant defaultInstanceId argument at both InstanceRegistry call sites now that the parameter defaults from the configuration object. FR-26219
…method lookupTargetEntities, lookupEntities and lookupEntitlements took instanceId inside the request object while isEntitledTo and isEntitledToMany took it as a trailing options argument. All five now use the options argument, so callers have one thing to remember. Doing it now because the multi-instance API is unreleased; once shipped this becomes a breaking change. Addresses review finding 7 on #75.
2 participants
Scopes every SpiceDB read in the SDK to exactly one Frontegg instance, so a single SpiceDB can serve several vendors with no cross-instance reads.
Workstream C (steps 8–10) of FR-26219. Depends on nothing else being merged first;
entitlements-agent(workstream D) depends on this shipping and being published.What changed
instances/defaultInstanceIdonClientConfiguration;InstanceRegistryvalidates at construction and throws at boot, never at request timeSchemaScope/readSchemaFor(instanceId), per-instance fallback,instanceIdon log lines,spiceClientmarked@deprecatedscope.type()masterNo
instancesconfigured ⇒ a legacy instance with an empty prefix ⇒ behaviour identical to today.The change that matters most
isEntitledTocatches every error and returns the configured fallback boolean. That would have turned a misconfigured instance into a silentfalse— a wrong answer to a question scoped to no instance at all.Instance resolution now happens before the try block, so it throws and makes no gRPC call. A genuine SpiceDB error still returns the fallback, as before.
This was confirmed against real SpiceDB in the FR-26219 Step 0 spike: a check against an undefined prefix returns
FailedPrecondition, notNO_PERMISSION. SpiceDB fails closed; the SDK was the part defeating it.Verification
scopeargument.yarn buildclean; lint warnings unchanged frommaster(33, zero errors).masterand this branch and diffing the serialized gRPC objects: identical.Three corrections to the written plan
The plan was written against an earlier state of this repo. Reviewers checking the code against it will find mismatches — these are the plan's errors, not the code's:
spiceDBEndpoint/spiceDBToken. This repo usesengineEndpoint/engineToken.lookupResources/lookupSubjects. The real public methods arelookupTargetEntities,lookupEntitiesandlookupEntitlements— the last with cursor-paginated multi-stream logic the plan does not mention. All three now acceptinstanceId.isEntitledToMany/spiceDBBatchFeatureQuery(an entire batch path), plusisLookupEntitlementsTenantMemberandgetLookupEntitlementsStreamsinside the client, all set object types directly. Left unscoped they would have leaked.And one bug following the plan literally would have introduced: in
route-spicedb.query.ts,firstRule.resourceTypeis read back from SpiceDB and is therefore already prefixed. Scoping it as C4 instructs yieldsv_a/v_a/frontegg_route. It needsscope.strip()first — done, with a test pinning it.Reviewing
Commits split cleanly if you want them separate:
aefcd95— registry, scope, resolution, threadinge1a4ec8—readSchemaFor, logging, deprecationWorth a close look at
route-spicedb.query.ts(the strip-then-scope round trip and the per-prefix cache key) and atschema-scope.guard.spec.ts, which is regex over source rather than an ESLint rule — an ESLint rule can't see thatobjectType:needsscope.type()without type information, but a creative enough refactor could slip past this guard.Not in this PR
template-and-libs/nestjs-packages/e10s-client-wrapper) — I don't have access to that repo.