fix(auth): gate activity notes and last-scan data server-side - #2807
Merged
Conversation
Asset and audit activity loaders gated on the entity permission rather than the note permission, then fetched and returned every note in the page payload. The note check ran client-side only, so roles without it received the data and only the UI hid it. The same shape existed on the asset overview loader, which returned last-scan data containing personal information with the only scan:read check running in the component. Splits the authorization half out of #2800 so it is not blocked by the consumable work under review there. No behavioural overlap between the two.
🩺 React Doctor — webappFindings on the files changed by this PR:
|
Contributor
WalkthroughThis change adds server-side permission gating for asset scans, asset activity notes, and audit activity notes. It also hides Activity tabs when note-read permission is absent and adds tests for the new authorization paths. ChangesAuthorization enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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: 109d028573
ℹ️ 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".
Gating on note:read first would 403 a deep link to an asset or audit living in another of the user's workspaces, before the resolver could hand off to the switch-workspace path. Assets and audits have that affordance; bookings, whose pattern this copied, does not. Now gates the entity permission, resolves the record, then gates the note permission before any note is fetched. Both properties hold: the deep link still hands off, and notes never reach the payload without the right to read them. Adds a test pinning the call order, verified to fail when the gates are swapped.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.ts (2)
102-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
loaderArgstype-safe.Line 108 casts the complete loader argument object through
unknown. If the loader adds a required context member or changes its parameter contract, this fixture will still compile and fail only at runtime. Use a typed fixture factory that satisfiesParameters<typeof loader>[0]instead.As per coding guidelines, define the correct type or narrow genuinely dynamic values instead of using
unknownas a type escape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/routes/_layout`+/assets.$assetId.activity.test.server.ts around lines 102 - 109, Update the loaderArgs fixture factory to construct a value typed as Parameters<typeof loader>[0] without casting through unknown. Define or reuse the correct loader-argument type and ensure its context, request, and params satisfy the loader contract so required contract changes fail at compile time.Source: Coding guidelines
188-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection without an unchecked cast.
Lines 193-195 cast the
unknownrejection to a response-wrapper shape. Use a runtime matcher or type guard so the test validates the actual rejection shape.Proposed test simplification
- const thrown = await loader(loaderArgs()).then( - () => null, - (caught: unknown) => caught - ); - - expect((thrown as { init?: { status?: number } })?.init?.status).toBe( - 403 - ); + await expect(loader(loaderArgs())).rejects.toMatchObject({ + init: { status: 403 }, + });As per coding guidelines, narrow genuinely dynamic values instead of using unchecked type assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/routes/_layout`+/assets.$assetId.activity.test.server.ts around lines 188 - 195, Update the rejection assertion around loader to avoid casting thrown to an assumed response shape; use an appropriate runtime matcher or type guard to narrow the unknown value before checking its init.status equals 403. Preserve the existing behavior of asserting the loader rejects with the expected forbidden response.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.activity.test.server.ts:
- Around line 102-109: Update the loaderArgs fixture factory to construct a
value typed as Parameters<typeof loader>[0] without casting through unknown.
Define or reuse the correct loader-argument type and ensure its context,
request, and params satisfy the loader contract so required contract changes
fail at compile time.
- Around line 188-195: Update the rejection assertion around loader to avoid
casting thrown to an assumed response shape; use an appropriate runtime matcher
or type guard to narrow the unknown value before checking its init.status equals
403. Preserve the existing behavior of asserting the loader rejects with the
expected forbidden response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a9d496c-0bf0-4221-99f2-fbb658666a1a
📒 Files selected for processing (3)
apps/webapp/app/routes/_layout+/assets.$assetId.activity.test.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.activity.tsxapps/webapp/app/routes/_layout+/audits.$auditId.activity.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/_layout+/assets.$assetId.activity.tsx
DonKoko
added a commit
that referenced
this pull request
Aug 5, 2026
The activity-note and last-scan authorization work now lives in #2807, so it is removed here rather than carrying the same nine files across two open PRs. This also clears the two ESLint errors blocking CI. The rule they trip, local-rules/no-test-files-in-routes, landed on main after this branch was cut, so the two route test suites went red on a rebase rather than on any change here. assets.$assetId.overview.tsx was the only file carrying both halves. It returns to its pre-authz state except for the one consumable line, which is kept: consumptionType passed through to QuantityCustodyList.
DonKoko
previously approved these changes
Aug 5, 2026
added 3 commits
August 5, 2026 17:03
Main added an ESLint rule banning test files under app/routes — Vite's dev-server warmup pulls every file there into the client graph, so a co-located route test importing a *.server module breaks pnpm webapp:dev. Moves both activity loader tests to test/routes-tests/ and switches their route imports to the ~/routes alias. Also renames them from .test.server.ts to .test.ts. Main removed the **/*.test.server.[jt]s include pattern from vitest.config.ts, so files with that spelling are silently never collected. Verified both suites run and pass from the new location.
…o fix/activity-and-scan-authz
DonKoko
approved these changes
Aug 5, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server-side authorization gap: three loaders fetched permission-gated data and returned it in the page payload, with the permission check running only in the component. The UI hid the data; the response body still contained it.
Split out of #2800. That PR bundles this with the ONE_WAY consumable work, which is under review. There is no behavioural overlap between the two halves, so this one should not wait. #2800 keeps the consumable change.
The defect
routes/_layout+/assets.$assetId.activity.tsxgated its loader onasset:read, fetched every note, and returned them. Thenote:readcheck ran client-side, swapping the notes for an empty state. Roles that holdasset:readbut notnote:readtherefore received the notes in the response and only React hid them.routes/_layout+/audits.$auditId.activity.tsxhad the identical shape withauditNote:read. Not a live exposure — every role currently holds that permission, so nothing leaks today. It is the same defect one permission edit away from mattering, and it is fixed the same way.The intended rule was already established elsewhere in the codebase: all four activity CSV exports require the note permission, and the bookings and locations activity pages gate their loaders on
bookingNote:read/locationNote:read. Assets and audits were the outliers.What changed
note:read. The parent route already enforcesasset:readfor the page, so the child requires only the permission covering the data it returns. Notes are never fetched without it.auditNote:read. The orthogonalrequireAuditAssigneeForBaseSelfServicerestriction is untouched, with a test pinning that it still runs.The sweep
Grepping every loader that returns data the component hides behind
userHasPermissionturned up two further instances.Fixed here — asset overview
lastScan. The loader returned scan data that includes personal information, with the onlyscan:readcheck running client-side. Because that is personal data rather than workspace content, it is fixed here rather than filed.The gate is extracted into
getLastScanForViewer(modules/scan/service.server.ts) so the data cannot be fetched without the check — a caller can no longer forget. It returnsnullfor an unauthorized viewer, the same shape as "never scanned", so callers need no special case. Extracting it also makes it directly unit-testable, which an inline check in that very large loader would not be.One further instance was identified during the sweep. Its correct server-side gate depends on an org-level visibility setting rather than a flat permission, so fixing it changes visible behaviour and is being handled separately as a product decision.
Tests
assets.$assetId.activity.test.server.ts— drives the realRole2PermissionMapthrough the loader. Asserts the gate asks fornote:readrather thanasset:read, that a denied request throws a 403 andgetPaginatedAndFilterableAssetNotesis never called, and that a permitted role still receives its notes. Asserting against the real matrix rather than restating it means the test speaks up if the matrix changes.audits.$auditId.activity.test.server.ts— same, plus a test pinning that the assignee restriction survives the entity switch, plus one documenting every role currently holdingauditNote:readso a change to that set is caught.last-scan-for-viewer.test.ts— runs the realhasPermissionandparseScanDataagainst the real matrix with only the DB read mocked. A viewer withoutscan:readgetsnulland the scan row is never queried; a permitted viewer still gets the parsed scan.Verification
pnpm turbo typecheck— 4/4 successfulpnpm webapp:lint— 0 errors (5 warnings, all pre-existing, none in touched files)Full
pnpm webapp:validatewas not run to completion locally: the machine was carrying a load average above 150 on 8 cores, under which vitest suites die during collect with hook timeouts and the failing set differs between runs on identical code. Each leg is green run separately. Worth a clean CI run before merge.Summary by CodeRabbit