Repository navigation
fix(nextly): a collection field rule reads the grants the caller arrived with - #1851
mobeenabdullah wants to merge 1 commit into
Conversation
|
@codex review |
|
Warning Review limit reachedNext included review available in 11 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 (2)
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 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. |
![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: de2ceb1df6
ℹ️ 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".
| const KEY_WITH_GRANT = { | ||
| actorType: "apiKey" as const, | ||
| permissions: [`update-${SLUG}`, `create-${SLUG}`, "write-secret"], | ||
| }; |
There was a problem hiding this comment.
This fixture hand-assembles only stored permission slugs and then has the field rule look for write-secret. A real authenticated key scope carries grant rows, so ruleFacingPermissions converts that grant to the rule-facing spelling secret:write; the current fixture instead exercises the rowless/mock fallback and would stay green if the production conversion were bypassed. Build the scope with apiKeyScope and grant rows, and make the rule assert the resource:action spelling, so this security regression test represents an actual API-key request.
AGENTS.md reference: packages/nextly/AGENTS.md:L40-L45
Useful? React with 👍 / 👎.
| data: finalData, | ||
| operation: "update", | ||
| user: params.user, | ||
| authenticatedScope: params.authenticatedScope, |
There was a problem hiding this comment.
All five newly added authenticatedScope arguments are bare additions; the surrounding comments describe field stripping, overrides, and document IDs, but not why the API-key scope must accompany this pass or what authority error omission causes. Add a code-local what-and-why comment to these forwards, as required for every code change.
AGENTS.md reference: AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
| const created = await h.createEntry( | ||
| { | ||
| collectionName: SLUG, | ||
| routeAuthorized: true, |
There was a problem hiding this comment.
This create case calls CollectionsHandler.createEntry, whose non-transactional create pipeline already forwarded authenticatedScope through all three field-access passes in the parent commit. It therefore passes both before and after this change and never reaches the newly edited transactional create pass at line 8552. Exercise createEntryInTransaction or the bulk-create path and confirm that test fails when the new forwarding is removed, so the added create assertion provides evidence for the changed code rather than an already-working path.
AGENTS.md reference: AGENTS.md:L189-L191
Useful? React with 👍 / 👎.
|
Superseded by #1844, which merged carrying this same change: every collection field-access call site on Splitting it out was the right call at the time, when #1844 was being reworked and I did not want a clean measured fix held behind a subtle one. #1844 merging made that moot. The tests in this branch are worth keeping, so they come back with the follow-up rather than being lost here. |
1 participant
Split out of #1844, which is being reworked. This half is independent, measured, and has drawn no findings across four review rounds there.
A field rule was answered on the wrong caller's authority
A field
accessrule receivespermissionsandroles, and it is asking about the caller. An API key carries the grants stamped on the key, deliberately narrower than the database roles of whoever owns it, because that is the point of scoping a key. None of the collection write paths passed that scope to the field-level pass, so the rules were answered from the key owner's roles instead.Measured on
main, sqlite. A field whose rule is({ permissions }) => permissions.includes("write-secret"), written through an API key stamped with exactly that grant:The key holds the grant the rule asks for, the write is stripped anyway, and the call reports success. It reads as a permission bug in both directions: a correctly scoped key cannot write its own field, and a key owned by a privileged user is judged on authority the key was never given.
Every collection write path passes
params.authenticatedScopenow, on create and on update alike. It was already available on all five call sites and already used elsewhere in the same methods; only the field pass was missed. The Singles path has done this since #1833.This is not a promote-only problem. Four of the five sites are ordinary create and update.
Tests
Three, in
field-rules-read-the-callers-own-grants.integration.test.ts:main:expected 'before' to be 'after'.maintoo, which is what a guard should do.Verification
pnpm --filter nextly check-typesclean.