fix(P2-T7): implement stricter page binding validation for files #214
Conversation
- Add filePages lookup for page-bound tokens instead of just checking drive membership - Make isResourceAccessAllowed async to support database query - Update plan.md Phase 2 acceptance criteria to reflect completion status
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughConverts resource-binding checks in the enforced file repository from synchronous to asynchronous, adds DB-backed validation for page-type bindings via the Changes
Sequence Diagram(s)(Skipped — changes are focused and do not introduce a new multi-component flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-14T14:54:45.713ZApplied to files:
🧬 Code graph analysis (1)packages/lib/src/repositories/__tests__/enforced-file-repository.test.ts (1)
⏰ 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). (2)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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
🤖 Fix all issues with AI agents
In `@packages/lib/src/repositories/enforced-file-repository.ts`:
- Around line 215-223: The const filePageLink declared in the switch case 'page'
can leak into other cases; wrap the case body in its own block (add { ... }
around the existing case 'page' statements) so filePageLink (and any other local
const/let) is scoped to that case; keep the existing logic using
db.query.filePages.findFirst with where: and(eq(filePages.fileId, file.id),
eq(filePages.pageId, binding.id)) and return filePageLink !== undefined inside
the new block.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/src/repositories/enforced-file-repository.tsplan.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
packages/lib/src/repositories/enforced-file-repository.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
packages/lib/src/repositories/enforced-file-repository.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
packages/lib/src/repositories/enforced-file-repository.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 199
File: packages/lib/src/repositories/enforced-file-repository.ts:214-218
Timestamp: 2026-01-14T04:56:49.682Z
Learning: Page-bound tokens in PageSpace are primarily used for editor operations and already have page-level access verified through getUserAccessLevel(), which mitigates the security impact of the current permissive page binding check in EnforcedFileRepository.
📚 Learning: 2026-01-14T04:56:49.682Z
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 199
File: packages/lib/src/repositories/enforced-file-repository.ts:214-218
Timestamp: 2026-01-14T04:56:49.682Z
Learning: Page-bound tokens in PageSpace are primarily used for editor operations and already have page-level access verified through getUserAccessLevel(), which mitigates the security impact of the current permissive page binding check in EnforcedFileRepository.
Applied to files:
packages/lib/src/repositories/enforced-file-repository.ts
📚 Learning: 2025-12-22T20:04:40.910Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission functions from `pagespace/lib/permissions` (e.g., `getUserAccessLevel`, `canUserEditPage`) instead of implementing permission logic locally
Applied to files:
packages/lib/src/repositories/enforced-file-repository.ts
📚 Learning: 2025-12-23T18:49:41.966Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : For database access, always use Drizzle client from `pagespace/db`: `import { db, pages } from 'pagespace/db';`
Applied to files:
packages/lib/src/repositories/enforced-file-repository.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access
Applied to files:
packages/lib/src/repositories/enforced-file-repository.ts
🧬 Code graph analysis (1)
packages/lib/src/repositories/enforced-file-repository.ts (2)
packages/db/src/index.ts (3)
db(20-20)and(8-8)eq(8-8)packages/db/src/schema/storage.ts (1)
filePages(23-38)
🪛 Biome (2.1.2)
packages/lib/src/repositories/enforced-file-repository.ts
[error] 217-222: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: Unit Tests
🔇 Additional comments (5)
packages/lib/src/repositories/enforced-file-repository.ts (3)
10-10: LGTM!The import correctly adds
filePages,eq, andandfrom@pagespace/db, which are necessary for the new page binding validation query.
103-112: LGTM!The async resource binding check is properly awaited, and the existing security pattern of returning
null(rather than throwing) to prevent file enumeration is preserved.
250-259: LGTM!The async resource binding check in
getFileForUpdatecorrectly mirrors the pattern ingetFile, maintaining consistent security behavior across both code paths.plan.md (2)
1093-1099: LGTM!The P2-T1 acceptance criteria correctly reflects the completed sessions schema deployment. Status update is consistent with the broader Phase 2 completion noted in the document.
1751-1763: LGTM!The P2-T7 acceptance criteria and status are consistent with the code changes. The stricter page binding validation via
filePageslookup directly supports the "Resource binding enforced" criterion.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| case 'page': | ||
| // Page binding requires checking file-page associations | ||
| // For now, we allow if the file is in the same drive | ||
| // TODO: Add filePages lookup for stricter validation | ||
| return this.ctx.driveId !== undefined && this.ctx.driveId === file.driveId; | ||
| // Page binding: file must be linked to the bound page via filePages table | ||
| const filePageLink = await db.query.filePages.findFirst({ | ||
| where: and( | ||
| eq(filePages.fileId, file.id), | ||
| eq(filePages.pageId, binding.id) | ||
| ), | ||
| }); | ||
| return filePageLink !== undefined; |
There was a problem hiding this comment.
Wrap switch case declaration in a block scope.
The const filePageLink declaration inside the switch case can leak to other cases, which Biome correctly flags. Wrap the case body in a block to restrict scope.
🔧 Proposed fix
case 'page':
- // Page binding: file must be linked to the bound page via filePages table
- const filePageLink = await db.query.filePages.findFirst({
- where: and(
- eq(filePages.fileId, file.id),
- eq(filePages.pageId, binding.id)
- ),
- });
- return filePageLink !== undefined;
+ {
+ // Page binding: file must be linked to the bound page via filePages table
+ const filePageLink = await db.query.filePages.findFirst({
+ where: and(
+ eq(filePages.fileId, file.id),
+ eq(filePages.pageId, binding.id)
+ ),
+ });
+ return filePageLink !== undefined;
+ }📝 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.
| case 'page': | |
| // Page binding requires checking file-page associations | |
| // For now, we allow if the file is in the same drive | |
| // TODO: Add filePages lookup for stricter validation | |
| return this.ctx.driveId !== undefined && this.ctx.driveId === file.driveId; | |
| // Page binding: file must be linked to the bound page via filePages table | |
| const filePageLink = await db.query.filePages.findFirst({ | |
| where: and( | |
| eq(filePages.fileId, file.id), | |
| eq(filePages.pageId, binding.id) | |
| ), | |
| }); | |
| return filePageLink !== undefined; | |
| case 'page': | |
| { | |
| // Page binding: file must be linked to the bound page via filePages table | |
| const filePageLink = await db.query.filePages.findFirst({ | |
| where: and( | |
| eq(filePages.fileId, file.id), | |
| eq(filePages.pageId, binding.id) | |
| ), | |
| }); | |
| return filePageLink !== undefined; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 217-222: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In `@packages/lib/src/repositories/enforced-file-repository.ts` around lines 215 -
223, The const filePageLink declared in the switch case 'page' can leak into
other cases; wrap the case body in its own block (add { ... } around the
existing case 'page' statements) so filePageLink (and any other local const/let)
is scoped to that case; keep the existing logic using
db.query.filePages.findFirst with where: and(eq(filePages.fileId, file.id),
eq(filePages.pageId, binding.id)) and return filePageLink !== undefined inside
the new block.
Code reviewFound 1 issue:
Fix: Add db: {
query: {
files: { findFirst: vi.fn() },
filePages: { findFirst: vi.fn() }, // Add this
},
}And mock 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The test mock was missing db.query.filePages, causing TypeError when tests hit the 'case page' branch in isResourceAccessAllowed(). Added: - filePages to db.query mock - filePages and 'and' to mock exports - Proper filePages.findFirst mocking in page-bound test cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add missing filePages mock properties (linkedBy, linkedAt, linkSource) - Fix flaky encryption auth tag test by inverting all hex digits instead of just first byte - Fix flaky notification tests by using targeted cleanup instead of deleting all users Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
drive membership
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.