Skip to content

fix: check role in read() to deny inviteRequested users#8896

Merged
jianwei1 merged 8 commits intomainfrom
jianweichong/nes-1481-fix-inviterequested-role-bypasses-journey-read-acl
Mar 24, 2026
Merged

fix: check role in read() to deny inviteRequested users#8896
jianwei1 merged 8 commits intomainfrom
jianweichong/nes-1481-fix-inviterequested-role-bypasses-journey-read-acl

Conversation

@jianwei1
Copy link
Contributor

@jianwei1 jianwei1 commented Mar 23, 2026

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

    • Enforced explicit role-based access for journeys: only owner/editor on a journey or manager/member via team can read; other roles (including invite-requested) are denied for read/manage/update/delete/export.
  • Tests

    • Added coverage asserting deny for invite-requested across actions and an edge-case allowing read when a team member role coexists.
  • Documentation

    • Added plan and security guidance describing the issue, root cause, and mitigation.

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>
@jianwei1 jianwei1 self-assigned this Mar 23, 2026
@linear
Copy link

linear bot commented Mar 23, 2026

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 905fa8b2-8355-4c80-9e8f-52dcb1587135

📥 Commits

Reviewing files that changed from the base of the PR and between 4be92e9 and 3a474c9.

📒 Files selected for processing (1)
  • docs/solutions/security-issues/journey-acl-read-authorization-bypass-invite-requested-role.md
✅ Files skipped from review due to trivial changes (1)
  • docs/solutions/security-issues/journey-acl-read-authorization-bypass-invite-requested-role.md

Walkthrough

Replace 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 inviteRequested fixture asserting deny for invite-requested roles (with an exception when team membership permits). Documentation added.

Changes

Cohort / File(s) Summary
ACL Implementation
apis/api-journeys-modern/src/schema/journey/journey.acl.ts
Replaced null-existence read() check with explicit role-based allow logic: permit only journey roles owner/editor or team roles manager/member.
Tests
apis/api-journeys-modern/src/schema/journey/journey.acl.spec.ts
Added journeyUserJourneyInviteRequested fixture and assertions verifying users with inviteRequested are denied for Manage, Read, Update, Delete, Export; added edge-case allowing read when team role is member.
Documentation / Plan
docs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md
Added completed plan describing the ACL fix, role-based checks, and required tests.
Documentation / Investigation
docs/solutions/security-issues/journey-acl-read-authorization-bypass-invite-requested-role.md
New security write-up describing the bypass, root cause (existence-based check), investigation, fix details, and mitigation guidance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the specific fix: replacing a permissive null-check with explicit role validation in the read() ACL function to deny users with inviteRequested role.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jianweichong/nes-1481-fix-inviterequested-role-bypasses-journey-read-acl

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Mar 23, 2026

View your CI Pipeline Execution ↗ for commit 5ffb77d

Command Status Duration Result
nx affected --target=codecov --base=a8d7e1b05ab... ✅ Succeeded 1s View ↗
nx affected --target=test --base=a8d7e1b05abe87... ✅ Succeeded 48s View ↗
nx affected --target=test --base=a8d7e1b05abe87... ✅ Succeeded 43s View ↗
nx affected --target=test --base=a8d7e1b05abe87... ✅ Succeeded 37s View ↗
nx affected --target=build --base=a8d7e1b05abe8... ✅ Succeeded 11s View ↗
nx affected --target=fetch-secrets --base=a8d7e... ✅ Succeeded <1s View ↗
nx run-many --target=prisma-generate --all --pa... ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-24 21:19:27 UTC

autofix-ci bot and others added 3 commits March 23, 2026 10:57
…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>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2bc684c and 4be92e9.

📒 Files selected for processing (3)
  • apis/api-journeys-modern/src/schema/journey/journey.acl.spec.ts
  • apis/api-journeys-modern/src/schema/journey/journey.acl.ts
  • docs/plans/2026-03-23-001-fix-invite-requested-role-bypasses-journey-read-acl-plan.md

@stage-branch-merger
Copy link

I see you added the "on stage" label, I'll get this merged to the stage branch!

@jianwei1 jianwei1 added this pull request to the merge queue Mar 24, 2026
Merged via the queue into main with commit 1fcb429 Mar 24, 2026
21 checks passed
@jianwei1 jianwei1 deleted the jianweichong/nes-1481-fix-inviterequested-role-bypasses-journey-read-acl branch March 24, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants