fix(nextly): fail closed on an unread role set, and retire derived key grants - #1794
Conversation
…y grants Follow-up to #1783, whose last review round landed after it merged. The facade resolved roles through `listRoleSlugsForUser`, which degrades a failed query to an empty set. That is the safe direction for a rule granting on a role and the wrong one for a rule withholding on it: `user.role !== "suspended"` admits a caller the database declined to answer for, which is the same empty-role grant #1783 exists to remove arriving by another door. Its strict sibling exists for exactly this and says so in its own documentation. A throw refuses the plugin's call, which is the correct direction: a decision taken on roles nobody read is not a decision. An API key's grants are DERIVED from the same RBAC rows and cached for five minutes under the key's id, and nothing retired them when a ROLE changed. `UserRoleService` evicts when a role is assigned to or removed from a USER; the role services only call `invalidatePermissionCache`, which knows nothing about keys. So revoking a role's inherited `super-admin` left that user's key holding the whole catalogue for the rest of the TTL, and changing a role's permissions left a role-based key holding the old set, which a comment in `UserRoleService` claimed was handled elsewhere and was not. Fixed by reversing the direction rather than by enumerating keys, which the permissions module cannot do and which a path written later would have to remember: `invalidatePermissionCache` counts its invalidations, and the key cache refuses an entry resolved under an older count. Blunt on purpose. A role change is rare and re-resolving a key's grants is a couple of indexed queries; a stale grant is the catalogue in the hands of somebody who no longer holds the role that granted it. Two suites had mocked `services/lib/permissions` as closed literals and refused the new import; both derive from the real module now, and their comments say why. Controls: a caller whose role lookup throws is refused, and a case asserting the STRICT resolver was called and the swallowing one was not, since a facade wired to either would pass the first alone. On a real database, a key holding a catalogue permission its owner's own role does not grant, which keeps holding it after a silent revocation and loses it once the role change is announced, by either hint, while its own role's grant survives so the difference is a re-resolve and not a wipe.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b62cbb8e34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * couple of indexed queries; a stale grant is the whole catalogue in the hands | ||
| * of somebody who no longer holds the role that granted it. | ||
| */ | ||
| let rbacRevisionCounter = 0; |
There was a problem hiding this comment.
In a multi-instance or serverless deployment, this module-level counter changes only in the process that handles the role mutation. Another instance with a cached API-key grant keeps its unchanged local revision, so the comparison in resolveApiKeyGrants continues accepting the revoked grants for the full five-minute TTL. Use a shared generation/invalidation mechanism, or avoid retaining these authorization results across requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and deliberately NOT fixed here. Leaving this thread open because it is a decision rather than a patch.
What is true: the counter is module-level, so a role mutation handled by one instance never reaches another, and that instance keeps serving its cached grants for the rest of the five-minute TTL.
What is also true, and is why this is not a regression: every cache the counter governs is already process-local and always has been. superAdminCache, the Tier-1 permission map and _apiKeyPermissionsCache are module-level Maps, and invalidatePermissionCache has only ever cleared the ones in the process that ran it. A role change in instance A has never evicted instance B permission cache either. The revision makes the in-process case correct and inherits the existing boundary rather than creating one.
There IS a shared tier, and it is not a signal: PermissionCacheService tombstones rows in a database table, which every instance then re-reads. The equivalent for key grants would be a generation row read on every key authentication, which puts a query on the hot path this cache exists to remove. That is a real trade and it is the founder call, not mine to slip into a follow-up.
Three options as I see them, with the cost of each:
- leave it, bounded by the TTL, and shorten the TTL if five minutes is too long for a revoked super-admin;
- stop caching the CATALOGUE branch specifically, so the most privileged credential is re-resolved every request while ordinary keys keep the cache. Bounds the worst case to the 60-second super-admin cache rather than five minutes, at one indexed select per request for those keys;
- a shared generation row, correct across instances, at one read per key authentication.
I have written up the trade for the founder and will implement whichever is chosen. The code now says plainly that the mechanism is process-local, so nobody reads it as more than it is.
There was a problem hiding this comment.
Correct, and deliberately still open. Leaving this thread unresolved is the honest state: the condition you describe stands, and the fix for it is a separate change.
What has changed here is the bound. The stored tier's default life drops from a day to five minutes (PERMISSION_CACHE_TTL_SECONDS still sets it), because until the signal is shared the TTL is the only thing limiting how long an instance that did not handle the change goes on serving what it stored, and a day is not a bound worth having on a revoked grant. Five minutes matches the window an API key's copied grants already carry, so the two tiers expire on the same order rather than one outliving the other by a factor of three hundred.
What has not changed is the mechanism, and the reason is worth stating rather than implying it was overlooked. The real answer is a generation stored beside the rows: one counter in the database, read about once a second per instance rather than per request, stamped on every cached entry and compared on every cache write. That turns "stale until the TTL expires" into "stale for about a second" at a cost the cache still pays for itself against. It needs a schema change and a migration, which is its own pull request rather than a late addition to this one.
Everything single-process in this round is fixed: the zero-role write is verified like its sibling, the clearing window is closed, and the batch is scoped to its operation.
One correction to my earlier reading of this finding. I had described the exposure as limited to the API key grants cache. Your second finding on the shared write is a wider case than that, and it is the one that moved the TTL.
|
While you are in |
…n honestly Three of the four findings in this round, each real. `PermissionService.updatePermission`, `deletePermissionById` and `deletePermission` called no invalidation at all, and neither existing one can express what they do: a permission row belongs to no user and no role, so there is no id to scope by. A role-based key therefore kept a renamed slug and a super-admin's key kept a deleted grant until their entries aged out. `invalidateAllPermissionCaches` clears the process-local tiers, tombstones the shared one through a new `PermissionCacheService.invalidateAll`, and advances the revision; all three mutations call it after a successful write. The revision was read AFTER the grant queries, so an invalidation landing while they were in flight was stamped onto rows read under the old one, and the next request reused grants the change was meant to retire for the whole TTL. It is captured before the reads now, which leaves such an entry already behind when it is written. The strict resolver propagates the driver's exception by design, and this facade had no catch around it, so a plugin route saw a raw database error with no `code` to branch on. Wrapped in `NextlyError.internal`, cause kept. The refusal is unchanged; only its shape is. The fourth finding, that this counter is process-local, is answered on the thread rather than here: every cache it governs is already process-local, the shared tier this module has is a database table rather than a signal, and a shared generation read per request would put a query on the hot path the cache exists to remove. That is a design decision, not a patch. Controls. A permission-row invalidation retires a key's catalogue grant. For the race, a resolution is started, the caches are invalidated while it is in flight, a new permission row is added, and the next read must SEE it — observed on the catalogue rather than on the super-admin answer, which has a cache of its own and made the first version of this case measure the wrong mechanism. Its control is the same sequence without the race, where the row must NOT appear, since otherwise the case passes on an implementation that caches nothing. And the refusal is typed, with a second case on the cause, since an implementation that threw a fresh error and discarded the original passes the first alone.
The case for a permission change called `invalidateAllPermissionCaches` directly, so it proved the primitive works and said nothing about whether anything calls it. Measured: removing the call from the delete path left it green, which is a fixture that never reaches the mechanism it is named for. The new case deletes through `PermissionService.deletePermissionById` and requires the key's grants to lose the row. The direct case stays, because the two claims are different and only one of them is about the service.
|
Heads-up on |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e57826d14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
…ation Three findings, all real, and the third is the same race one level in: it defeats the fix from the last round on its own. `isSuperAdmin` awaits two queries and then writes its cache, so a lookup started before a demotion completes after it and puts `true` back into the map the invalidation had just cleared. The next key request then correctly rejects the outer entry, consumes that stale inner one, and caches catalogue-wide grants under the NEW revision for another five minutes. Two instances of one shape is a coincidence; three is the design. So the rule is stated once, as `resolvedUnderCurrentRevision`, and asked at every cache write here rather than patched per cache: the super-admin answer, both tiers behind `hasPermission`, and the key grants already fixed. The database tier was the worst of them, because its entries are shared across instances and live for a day, so a decision written after a tombstone outlived everything else. Nine methods write permission rows and none invalidated anything. Review found them three at a time, twice, which is what a rule with nothing enforcing it looks like, so there is a check now. It reads the SYNTAX TREE: its first version found method bodies by counting braces from the signature, and a signature carrying an object type gave it the return type's brace, so `ensurePermission` mutates the table twice and read as compliant. That version also named six writers. The tree version named nine, and the three nobody had mentioned are `markOrphanedPermissions`, `normalizeReversedSlugs` and `returnPermissionToPresets` — the first of which decides whether a row is in the catalogue at all. The fourth finding, that the counter is process-local, is still open on the thread. It is a design decision with a cost either way, and the founder has the options. Controls: a lookup in flight across a revocation must not restore the answer, asserted both on the map directly and through the key grants that consume it; and the writer check names its nine rather than counting them, with a case requiring the tree to have parsed anything at all.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3393e336e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…on push but not with main The first commit said pull_request.base.sha is fixed when the pull request is opened and never moves. Measured since: GitHub sets it at opening (the base tip, #1804: 0121364) and again on each push (the merge-base then: #1796 ebf8f78, #1803 da323d5 after a merge of main). What it never does is follow the base branch, and the merge ref the audit reads is rebuilt against the live base — so the drift is everything main gained since the last push, which a long queue makes likely. The fix is unchanged; the comment now says that. The first commit's body also cited #1794 as passing and then failing with nothing pushed between; a push landed at 14:24Z and the failing run is on it, so that example is withdrawn.
… file
Five findings, all real, and two of them were about this branch's own controls.
The writer check named two service files, so it reported a third as compliant
without opening it — the failure it exists to catch, one level up. It reads the
package's whole source tree now, and immediately found two more writers in
`role-permission-service.ts`. It also resolves one file's own helpers as a fixed
point, because a writer is allowed to delegate: four seeder passes share one
"did this write anything" helper rather than asking it four times, and a check
that only looked for a direct call would have called all four of them silent.
It accepts either invalidator too: a scoped `invalidatePermissionCache({ roleId })`
is right where the write is additive and confined to one role, and demanding the
table-wide form there would turn a correct cheap call into an unfiltered rewrite.
The table-wide tombstone was paid per permission. A seeder ensures one at a
time, so a single new collection rewrote and locked the whole cache table once
per permission, each pass expiring rows the previous had already expired. Each
seeding method runs as one sweep now: the revision still advances per write, so
nothing in flight can file a result as current, and only the table write is
deferred. The flush happens on the way out even when the batch throws, since a
partial write still changed rows.
The Tier-2 lookup is awaited, so an invalidation can land while it is
outstanding; the row it returns predates the change and was being promoted into
Tier 1 for that tier's whole life, having just been tombstoned. It recomputes
instead. The fire-and-forget Tier-2 write had the mirror of that: checked only
before launching, it could land behind a tombstone with a fresh expiry, into the
tier that is shared between instances and lives for a day. It re-checks after
the upsert and tombstones this user's rows if the revision moved, so either the
invalidation caught the row or this does.
Both delete paths in `PermissionService` reached the same statement by different
routes and now share one, which is also where the invalidation lives, and the
seeder's four passes share the helper above. The comment on the writer check
describes the invariant rather than how it was arrived at.
Controls: the sweep advances the revision per write while deferring the flush,
clears by the time the batch returns, and flushes when the batch throws.
… as it was at the last push (#1804) * fix(ci): the hygiene gate diffs against the base branch's tip, not the sha the pr was opened on The fallow action scopes its audit to files changed since github.event.pull_request.base.sha unless told otherwise. GitHub records that sha when the pull request is opened and never moves it, while the merge ref the job checks out is rebuilt against the live base branch — so every file main gained while a pull request was open read as changed by it, and functions its author never touched (listEntries, generateCollectionUpdate, invalidatePermissionCache on #1787, which touches none of those files) failed the gate as introduced. The same pull request passed at 13:46Z and failed at 14:29Z with nothing pushed in between. The step now names origin/<base_ref>, as the envelope step in the same job already did; on the merge ref the three-dot diff has the live tip as its merge-base, so the scope is exactly the pull request. Ledger: task:ci-hygiene-gate-diffs-against-current-main, implementing finding:fallow-ci-blames-a-pr-for-main-moving. * fix(ci): say when github sets a pull request's base sha, which moves on push but not with main The first commit said pull_request.base.sha is fixed when the pull request is opened and never moves. Measured since: GitHub sets it at opening (the base tip, #1804: 0121364) and again on each push (the merge-base then: #1796 ebf8f78, #1803 da323d5 after a merge of main). What it never does is follow the base branch, and the merge ref the audit reads is rebuilt against the live base — so the drift is everything main gained since the last push, which a long queue makes likely. The fix is unchanged; the comment now says that. The first commit's body also cited #1794 as passing and then failing with nothing pushed between; a push landed at 14:24Z and the failing run is on it, so that example is withdrawn.
The two cases added for the tier-2 promotion re-check could not run: their own control showed the shared tier was never consulted, and a probe found why — `setCachedPermission` does not take effect under `createTestNextly`, where the table is created and a write followed by a read returns null. A green from a case that cannot reach its branch is worse than no case, so they are removed and the branch says in the source that it is uncovered, and why. The predicate it uses is covered by the sweep and race cases; the call site is not.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bea55b2e62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!resolvedUnderCurrentRevision(resolvedUnder)) { | ||
| await service.invalidateByUser(userId); |
There was a problem hiding this comment.
Fresh evidence beyond the acknowledged API-key limitation is that this post-upsert check still compares only the writer process's module-local revision. If instance B mutates RBAC and tombstones the shared database cache while instance A's stale upsert is pending, A's revision never changes, so this check accepts the write and recreates an authorization result with a fresh 24-hour expiry that every instance can consume. The database write needs shared generation/CAS ordering rather than a process-local comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is the finding that changed how the round was scoped, so thank you for separating it from the one above.
You are right that this is not the same as the acknowledged API-key limitation. That one is a derived cache in this process; this is a write into the tier every instance reads, carrying a fresh expiry, ordered by a counter the writing process alone can see. A process-local comparison cannot order it, and no amount of re-checking that counter will.
It is not fixed here, and the thread stays open for that reason rather than because it is disputed. The remedy is the shared generation described on the decision thread: a counter stored beside the rows and compared on write, which is a schema change and its own pull request. Until then the bound is the TTL, and this finding is why it moved from a day to five minutes: a stale answer resurrected into the shared tier could previously outlive its cause by a day, and now cannot outlive it by more than five minutes.
Worth saying about the process-local check you are looking at, since it is easy to read as an attempt at the ordering you describe: it is not one. It closes a race within one process, where the invalidation and the pending write are genuinely both visible, and it is correct for that. It was never going to order two instances, and the comment above it now says so.
… caches A permission row belongs to no user and no role, so neither scoped invalidation can express a change to one. Every writer therefore carried its own table-wide invalidation, which is a step each new writer has to remember and whose omission is invisible where it happens: the writer's own tests assert on rows, not on what still answers from cache. `writingPermissions` makes the two acts one. The table arrives as the callback's parameter, so a write is written inside the gate as a matter of course, and the guard holds every other write in the package to the same rule by asking a property of the write itself rather than recognising the method around it. Where the write is inside a transaction the gate wraps the transaction, so the caches are retired after the commit rather than before it. Three further cases where a retired answer could come back: - the zero-role denial was stored without the verification its sibling branch does, so a first role granted while that write was in flight left a stored `false` for a permission the user now holds; - clearing the shared tier is itself an awaited write, and a check beginning and finishing inside that window read a row the clearing had not reached and promoted it into a tier that outlives the clearing; - the seeding batch was counted for the process, so a revocation raised while any seeder was awaiting was deferred until that seeder finished. It is scoped to the operation now, which is what `AsyncLocalStorage` is for. The stored tier's default life drops from a day to five minutes. It is shared between instances and the signal that retires it is held in memory, so the TTL is the only bound on how long an instance that did not handle the change goes on serving what it stored. Storing that signal beside the rows is the fix for that, and is not in this change.
Two cases, and the second is what makes the first mean anything. Holding a batch open and raising an unrelated revocation from outside it distinguishes a batch scoped to its operation from one counted for the process; on its own it is also satisfied by a sweep that defers nothing at all, so the control asserts that the batch's own writes are still coalesced. Verified by reproducing the process-wide read: the first case fails and the control stays green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32e428c77a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… plugin call does
The strict resolver only ran where `authenticatedScope.roles` was absent, and an
API-key request always populates that scope: a read-only or full-access key
resolved its owner's roles through the swallowing lookup, so a failed query
arrived as an empty set in the very field every later role rule reads directly.
Nothing downstream can tell that from an owner who holds no roles, and a rule
that WITHHOLDS on one — `!roles.includes("suspended")` — then authorizes the
request it exists to refuse. The door was closed on one path and left open on
the reachable one.
Both paths now ask `listRoleSlugsForUserOrRefuse`. Written once because it is
one question: asked separately, the two would drift, and a caller left on the
swallowing resolver is invisible, since an empty role set is an ordinary answer.
Its contract is tested against the real function rather than through either
caller's mock, and without mocking anything: an empty id returns before any
query, and a lookup with no adapter registered fails for real. An earlier
version of that file mocked the strict lookup through the module's exports,
which does not intercept a call the wrapper makes by its internal name — it
passed the real lookup straight through while appearing to control it.
The documented default for PERMISSION_CACHE_TTL_SECONDS follows the code to 300
seconds, and says why the value is what bounds cross-instance staleness.
…at it refuses The refusal case mocked whichever resolver the service called, so it passed on either wiring: the swallowing sibling rejects too, once a test tells it to. Reverting the fix left it green. Both resolvers are mocked now and the case asserts the door, which is the property — an unreadable role set refusing rather than arriving as none.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af545ee5ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.logger.error("Failed to invalidate the whole permission cache", { | ||
| error: String(error), | ||
| }); | ||
| return 0; |
There was a problem hiding this comment.
If the table-wide cache update fails after a permission-row mutation has committed, returning 0 makes flushPermissionCaches treat the tombstone as complete and release its invalidation guard. The mutation still reports success, but the old unexpired database-cache row remains; a subsequent permission check can read and promote that stale grant for up to the five-minute TTL. Preserve a failed/invalidating state or otherwise ensure stale shared entries cannot be consumed after this failure.
Useful? React with 👍 / 👎.
| * old set, which the comment in `UserRoleService` said was handled | ||
| * elsewhere and was not. |
There was a problem hiding this comment.
This comment records that a separate UserRoleService comment previously claimed the behavior was handled elsewhere, rather than describing only the current revision invariant. Rewrite it to state directly that role and permission changes retire derived API-key grants.
AGENTS.md reference: AGENTS.md:L287-L289
Useful? React with 👍 / 👎.
1 participant
Follow-up to #1783. Its last review round found two more P1s and landed on the branch after the pull request had already merged, so neither reached
main— verified by content:maincarries the catalogue branch, the inherited-super-admin resolver, the key's own roles and the super-admin cache eviction, and neither of the two below.An unread role set must refuse, not grant
The facade resolved roles through
listRoleSlugsForUser, which catches a failed RBAC query and returns[]. That is the safe direction for a rule that GRANTS on a role and the wrong one for a rule that WITHHOLDS on one:A caller the database declined to answer for arrives roleless and satisfies it. That is the same empty-role grant #1783 exists to remove, arriving through a different door.
listRoleSlugsForUserStrictexists for exactly this and says so in its own documentation, which is what makes this a miss rather than a judgement call. The facade now uses it, so a lookup that could not run refuses the plugin's call: a decision taken on roles nobody read is not a decision.The REST path still resolves through the swallowing version (
resolveRoleSlugs). That is a wider change with a blast radius on every request, and it belongs to whoever owns that path rather than to this follow-up, but it is the same exposure and worth someone's decision.A key's grants must be retired when the roles behind them change
An API key's grants are derived from the same RBAC rows and cached for five minutes under the key's id. Nothing retired them when a role changed:
UserRoleServiceevicts when a role is assigned to or removed from a userinvalidatePermissionCache, which knows nothing about keysSo revoking a role's inherited
super-adminleft that user's key holding the entire catalogue for the rest of the TTL. The same gap left arole-basedkey holding a permission set its role no longer has — which a comment inUserRoleServiceasserts is "invalidated via RolePermissionService instead", and that service only callsinvalidatePermissionCache. The comment has been wrong since it was written.Fixed by reversing the direction rather than enumerating keys. The permissions module cannot find a derived cache —
api-key-servicealready imports it, so it cannot be imported back — and it has no way to learn which keys a role reaches. SoinvalidatePermissionCachecounts its invalidations and the key cache refuses an entry resolved under an older count. Neither module enumerates the other's keys, and a path written later is covered without having to remember.Blunt on purpose: a role change is rare and re-resolving a key's grants is a couple of indexed queries, while a stale grant is the catalogue in the hands of somebody who no longer holds the role that granted it.
Controls
Three mutations, each red: the swallowing resolver restored, the revision check removed from the cache hit, and the invalidation no longer counting.
Two things worth knowing, both found by a control
The first version of the integration suite used
harness.getService("apiKeyService"), which the test harness resolves throughnextly/testing— the built package. Itsservices/lib/permissionsis a different module instance from the one the test imports, with its own caches and its own counter, so the two could never agree and the suite measured exactly that. It builds the service from source now.The cases were then order-dependent through the module-level caches, which outlive a harness while the database does not: a shared key id handed one case's resolved grants to the next case's first read, and a shared user id did the same for the super-admin answer. Both are unique per case now.
Two unrelated suites had mocked
services/lib/permissionsas closed literals and refused the new import. Both derive from the real module now, and their comments say why — that is the second time this session a literal mock broke a suite with nothing to do with the change.Verification
pnpm test940 files passing,pnpm check-typesclean inpackages/nextly, the key-scope and cache integration suites green.