Conversation
The read() function in the modern API's journey.acl.ts checked only for the existence of a userJourney record, not the role value. This allowed users with inviteRequested role to open the journey editor via direct URL (passing the Read check) while failing all downstream Update/Manage checks — resulting in a blank canvas and plausible stats errors. Fix read() to explicitly check for owner/editor journey roles and manager/member team roles, matching the pattern used by update(), manage(), and extract() in the same file. Add inviteRequested test cases across all actions. Closes NES-1481 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughReplace a permissive existence check in journey ACL read() with explicit role checks: allow only UserJourneyRole.owner/editor or UserTeamRole.manager/member. Add tests including an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 5ffb77d
☁️ Nx Cloud last updated this comment at |
…se test - Revert callback params from uj/ut back to userJourney/userTeam to match the established convention in all sibling functions - Add blank line between .find() calls for structural consistency - Add dual-role edge case test: inviteRequested + team member Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md`:
- Around line 203-210: The plan shows `status: completed` but the acceptance
checklist items (the unchecked boxes like "`inviteRequested` users receive
FORBIDDEN when querying `adminJourney`", "`inviteRequested` users see the
`AccessDenied` component`, etc.) are still unchecked; update the plan so the
status and checklist are consistent by either checking each completed box in the
checklist or adding a brief note under the checklist explaining why certain
items remain unchecked (e.g., pending manual verification or covered by other
tests), and ensure the `status: completed` line accurately reflects the final
state.
- Around line 85-100: Update the plan snippet to match the merged implementation
by using the long parameter names and role checks actually present: replace the
callback param examples `uj`/`ut` with `userJourney`/`userTeam` and ensure the
snippet shows the explicit role checks `hasJourneyReadAccess` and
`hasTeamReadAccess` (based on `UserJourneyRole.owner|editor` and
`UserTeamRole.manager|member`) so it mirrors the code pattern used in
`update()`, `create()`, `manage()`, and `extract()` and removes the outdated
`uj`/`ut` example in the “After (fixed)” section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00299e8b-5fa3-4922-9049-3d54f12aa174
📒 Files selected for processing (3)
apis/api-journeys-modern/src/schema/journey/journey.acl.spec.tsapis/api-journeys-modern/src/schema/journey/journey.acl.tsdocs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md
docs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md
Show resolved
Hide resolved
docs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md
Show resolved
Hide resolved
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
…le-bypasses-journey-read-acl
…le-bypasses-journey-read-acl
The read() function in the modern API's journey.acl.ts checked only for the existence of a userJourney record, not the role value. This allowed users with inviteRequested role to open the journey editor via direct URL (passing the Read check) while failing all downstream Update/Manage checks — resulting in a blank canvas and plausible stats errors.
Fix read() to explicitly check for owner/editor journey roles and manager/member team roles, matching the pattern used by update(), manage(), and extract() in the same file. Add inviteRequested test cases across all actions.
Related to NES-1481
Summary by CodeRabbit
Bug Fixes
Tests
Documentation