🐛 Fixed missing permission checks in paginated query hooks #25656
Conversation
Contributors viewing their user profile menu saw a permission error because useBrowseInvites() was making API calls without checking user permissions. The createQuery hook correctly disabled queries when users lacked permissions, but createInfiniteQuery and createPaginatedQuery were missing this check.
WalkthroughAdds permission gating across three API query hooks: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/admin-x-framework/src/utils/api/hooks.ts (1)
121-126: Critical: Conditional hook call violates React's Rules of Hooks.Same issue as in
createPaginatedQuery(line 73). The conditional call tousePermissionviolates React's Rules of Hooks.Apply this diff to call the hook unconditionally:
- const hasPermission = options.permissions ? usePermission(options.permissions) : true; + const permissionCheck = usePermission(options.permissions); + const hasPermission = options.permissions ? permissionCheck : true;
🧹 Nitpick comments (1)
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx (1)
376-402: Consider adding explicit "with permission" tests for consistency.While the existing tests implicitly cover the "has permission" scenario via the default admin user in
beforeEach,createPaginatedQueryandcreateInfiniteQueryonly have explicit "no permission" tests. For consistency with thecreateQuerytests (lines 183-236), consider adding explicit "with permission" tests similar to lines 211-236.This is optional since the functionality is already tested implicitly, but explicit tests would improve clarity and maintainability.
Also applies to: 455-485
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-framework/src/utils/api/hooks.ts(2 hunks)apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-05T16:42:12.989Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25356
File: apps/admin/test-utils/fixtures/query-client.tsx:17-35
Timestamp: 2025-11-05T16:42:12.989Z
Learning: In apps/admin/test-utils/fixtures/query-client.tsx, the createTestQueryClient function is intentionally duplicated from admin-x-framework to reduce external dependencies in the admin app's test utilities.
Applied to files:
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx
📚 Learning: 2025-11-10T17:07:54.169Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25377
File: apps/admin/src/ember-bridge/EmberBridge.test.tsx:146-174
Timestamp: 2025-11-10T17:07:54.169Z
Learning: In apps/admin/src/ember-bridge/EmberBridge.tsx, the useEmberAuthSync hook invalidates all React Query caches whenever an emberAuthChange event is emitted, regardless of the isAuthenticated field value in the event payload. The isAuthenticated field exists in the event type but is not currently used by the handler.
Applied to files:
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx
📚 Learning: 2025-11-10T11:30:41.316Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25357
File: apps/admin/test-utils/test-helpers.ts:6-12
Timestamp: 2025-11-10T11:30:41.316Z
Learning: In apps/admin/test-utils/test-helpers.ts, the waitForQuerySettled helper is intentionally designed to timeout for idle/disabled queries. It should only treat queries as settled when they reach a terminal state (isSuccess or isError) and are not fetching. This ensures tests properly wait for active queries to complete.
Applied to files:
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx
🧬 Code graph analysis (1)
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx (3)
apps/admin-x-framework/src/api/currentUser.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/test/utils/mockFetch.ts (1)
withMockFetch(7-23)apps/admin-x-framework/src/utils/api/hooks.ts (3)
createQuery(35-61)createPaginatedQuery(63-105)createInfiniteQuery(118-145)
🪛 Biome (2.1.2)
apps/admin-x-framework/src/utils/api/hooks.ts
[error] 73-73: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 121-121: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Admin-X Settings tests
- GitHub Check: ActivityPub tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push
🔇 Additional comments (2)
apps/admin-x-framework/test/unit/utils/api/hooks.test.tsx (2)
8-62: Well-structured test setup for permission testing.The mock setup for
useCurrentUseris clean and thebeforeEach/afterEachpattern ensures tests have a consistent default admin user while allowing individual tests to override it for specific scenarios.
183-236: Comprehensive permission test coverage for createQuery.These tests properly verify both the denied and granted permission scenarios, checking that no API calls are made when permissions are insufficient and that requests proceed normally when authorized.
| const hasPermission = options.permissions ? usePermission(options.permissions) : true; | ||
|
|
||
| const result = useQuery<ResponseData>({ | ||
| enabled: hasPermission, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/admin-x-framework/src/utils/api/hooks.ts | head -130Repository: TryGhost/Ghost
Length of output: 6438
🏁 Script executed:
find . -name "*usePermission*" -o -name "*usePermissions*" | head -20Repository: TryGhost/Ghost
Length of output: 174
🏁 Script executed:
cat -n apps/admin-x-framework/src/hooks/usePermissions.tsRepository: TryGhost/Ghost
Length of output: 552
🏁 Script executed:
cat -n apps/admin-x-framework/test/unit/hooks/usePermissions.test.tsRepository: TryGhost/Ghost
Length of output: 6090
🏁 Script executed:
rg "usePermission\(" apps/admin-x-framework/src --context=3 | head -50Repository: TryGhost/Ghost
Length of output: 2102
Critical: Conditional hook call violates React's Rules of Hooks.
The pattern options.permissions ? usePermission(options.permissions) : true calls the usePermission hook conditionally, which violates React's Rules of Hooks. Hooks must be called unconditionally in the same order on every render.
usePermission requires a string[] parameter and cannot accept undefined. The correct fix is to call the hook unconditionally with a fallback for when permissions are not provided:
- const hasPermission = options.permissions ? usePermission(options.permissions) : true;
+ const hasPermission = options.permissions ? usePermission(options.permissions) : usePermission([]);This issue appears in three locations:
- Line 41 in
createQuery(called inline in theuseQueryobject) - Line 73 in
createPaginatedQuery - Line 121 in
createInfiniteQuery
All three instances should be fixed together for consistency. Note that line 41's inline usage is additionally problematic—it should be extracted to a separate statement like lines 73 and 121.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasPermission = options.permissions ? usePermission(options.permissions) : true; | |
| const result = useQuery<ResponseData>({ | |
| enabled: hasPermission, | |
| const hasPermission = options.permissions ? usePermission(options.permissions) : usePermission([]); | |
| const result = useQuery<ResponseData>({ | |
| enabled: hasPermission, |
🧰 Tools
🪛 Biome (2.1.2)
[error] 73-73: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In apps/admin-x-framework/src/utils/api/hooks.ts around lines 41, 73 and 121,
the code currently calls usePermission conditionally which violates React's
Rules of Hooks; instead call usePermission unconditionally with a safe fallback
(e.g. usePermission(options.permissions ?? [])) and assign the result to
hasPermission before using it in enabled; extract any inline conditional hook
calls into their own statement so each hook is invoked on every render and
ensure usePermission's parameter type is satisfied by providing an empty array
when permissions are undefined.
There was a problem hiding this comment.
It does, but its an existing pattern, so I decided to stick with it over changing the existing lines for the purpose of this PR. We can come back to these holistically after if this changeset is merged.
And the code we'd use is:
const hasPermission = usePermission(options.permissions ?? []);There was a problem hiding this comment.
@rob-ghost, that makes sense! Keeping this PR focused on adding the permission checks is a reasonable approach, especially since line 41 already uses this pattern. This aligns with a preference I've seen from kevinansfield to maintain existing patterns when fixing specific issues and address improvements separately.
Would you like me to create a follow-up issue to track fixing all three conditional hook calls (lines 41, 73, and 121) with the correct pattern usePermission(options.permissions ?? [])? That way it won't be forgotten once this PR is merged.
🧠 Learnings used
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25377
File: apps/admin/src/ember-bridge/EmberBridge.test.tsx:146-174
Timestamp: 2025-11-10T17:07:54.169Z
Learning: In apps/admin/src/ember-bridge/EmberBridge.tsx, the useEmberAuthSync hook invalidates all React Query caches whenever an emberAuthChange event is emitted, regardless of the isAuthenticated field value in the event payload. The isAuthenticated field exists in the event type but is not currently used by the handler.
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
I'd consider sneaking in that fix for the conditional hook while you're at it. Otherwise this looks good!
Updated usePermission to accept undefined/null and return true (no permissions = allow all), eliminating need for conditional hook calls in createQuery, createPaginatedQuery, and createInfiniteQuery.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/admin-x-framework/test/unit/hooks/usePermissions.test.ts (1)
99-103: Parametrized “no permissions required” test matches hook semanticsThe
it.eachover[], undefined, nullcorrectly captures the intended “no permissions required → allow all” behavior and aligns with the hook’s new early-return logic. If you want to make that contract even clearer, you could add a variant wherecurrentUserisundefinedto explicitly document that these inputs bypass user loading, but that’s optional.Also applies to: 114-116
apps/admin-x-framework/src/hooks/usePermissions.ts (1)
4-10: AlignrequiredRolestyping withUserRoleTypeto avoid misleading castThe new signature and early-return for “no permissions required” look good and match the tests. The only concern is the
role as UserRoleTypecast in the final check whilerequiredRolesis typed asstring[] | null. That assertion suggests only validUserRoleTypevalues are passed, but in practice any string is allowed and will silently be treated as a valid role string, which can hide mismatches (for example,'admin'vs'Administrator').To tighten this up, consider either:
- Changing the hook signature to
requiredRoles?: UserRoleType[] | nulland removing the cast, or- Treating
currentUserRolesasstring[]and dropping theUserRoleTypeassertion so the types honestly reflect the looser contract.Also applies to: 16-16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/admin-x-framework/src/hooks/usePermissions.ts(1 hunks)apps/admin-x-framework/src/utils/api/hooks.ts(3 hunks)apps/admin-x-framework/test/unit/hooks/usePermissions.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-framework/src/utils/api/hooks.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/admin-x-framework/test/unit/hooks/usePermissions.test.ts (1)
apps/admin-x-framework/src/hooks/usePermissions.ts (1)
usePermission(4-17)
apps/admin-x-framework/src/hooks/usePermissions.ts (2)
apps/admin-x-framework/src/api/currentUser.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/roles.ts (1)
UserRoleType(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
|
||
| const result = useQuery<ResponseData>({ | ||
| enabled: options.permissions ? usePermission(options.permissions) : true, | ||
| enabled: hasPermission, |
There was a problem hiding this comment.
Bug: Permission check can be overridden by caller's enabled option
The enabled: hasPermission check is placed before ...query in the object spread, which means if a caller passes an enabled option, it will override the permission check. For example, passing {enabled: true} when hasPermission is false would bypass the security guard and allow the API call to proceed despite the user lacking required permissions. The correct approach would be to combine both conditions: enabled: hasPermission && (query.enabled ?? true).
Additional Locations (2)
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
Nice! Added a few thoughts/questions. I don't think that should block merging this.
| export const usePermission = (requiredRoles?: string[] | null) => { | ||
| const {data: currentUser} = useCurrentUser(); | ||
|
|
||
| // No permissions required = allow all | ||
| if (!requiredRoles || requiredRoles.length === 0) { | ||
| return true; | ||
| } | ||
|
|
||
| const currentUserRoles = currentUser?.roles.map(role => role.name); | ||
| if (!currentUserRoles) { | ||
| return false; | ||
| } | ||
| return userRoles.some((role => currentUserRoles.includes(role as UserRoleType))); | ||
| return requiredRoles.some((role => currentUserRoles.includes(role as UserRoleType))); |
There was a problem hiding this comment.
Non blocking: I wonder if we should allow null values here, or if that would accidentally open up for incorrect usage of this hook?
I think I'd rather have a usePermissionIfAvailable hook or something along those lines that would wrap this and add the default allow all behavior 🤔
|
|
||
| const result = useQuery<ResponseData>({ | ||
| enabled: options.permissions ? usePermission(options.permissions) : true, | ||
| enabled: hasPermission, |
Summary
Contributors viewing their user profile menu were seeing a permission error toast because
useBrowseInvites()was making API calls without checking user permissions.Root cause: The
createQueryhook correctly disabled queries when users lacked permissions viaenabled: hasPermission, butcreateInfiniteQueryandcreatePaginatedQuerywere missing this check.Fix: Added the same permission check pattern to both hooks:
createPaginatedQuerynow checks permissions before making requestscreateInfiniteQuerynow checks permissions before making requestsNote
Adds consistent permission gating across query hooks and updates
usePermissionto allow when no roles are required, with comprehensive tests.usePermission: AcceptsrequiredRoles?: string[] | null; returnstruewhen no roles provided; refactors param name and usage.createPaginatedQuery/createInfiniteQuery: AddhasPermission = usePermission(options.permissions)and setenabled: hasPermissionto prevent unauthorized requests.createQuery: Minor refactor to reusehasPermissionforenabled.usePermissions.test.ts: Add cases for empty/undefined/null roles returningtrueand other robustness checks.utils/api/hooks.test.tsx: MockcurrentUser; add tests ensuring queries are disabled and no requests are made without required roles; add positive cases when permissions are satisfied.Written by Cursor Bugbot for commit c7556c6. This will update automatically on new commits. Configure here.