Skip to content

fix(P2-T7): implement stricter page binding validation for files #214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
2witstudios merged 3 commits into from
Jan 15, 2026
+89 −39
Merged

fix(P2-T7): implement stricter page binding validation for files #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86bca2e
fix(P2-T7): implement stricter page binding validation for files
claude Jan 15, 2026
8ac1a79
fix(tests): add missing filePages mock to enforced-file-repository tests
2witstudios Jan 15, 2026
d47d55f
fix(tests): improve test robustness for CI environment
2witstudios Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/lib/src/__tests__/encryption-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,15 @@ describe('encryption-utils', () => {
const encrypted = await encrypt(testData)
const parts = encrypted.split(':')

// Corrupt the auth tag - flip all bits in the first byte
// Corrupt the auth tag - invert all bytes to guarantee different value
const authTagHex = parts[2]
const corruptedAuthTag = 'FF' + authTagHex.substring(2)
const corruptedAuthTag = authTagHex
.split('')
.map((c) => {
const val = parseInt(c, 16)
return (15 - val).toString(16) // Invert each hex digit
})
.join('')
parts[2] = corruptedAuthTag
const corrupted = parts.join(':')

Expand Down
20 changes: 14 additions & 6 deletions packages/lib/src/__tests__/notifications.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
deleteNotification,
getUnreadCount
} from '../notifications'
import { db, users } from '@pagespace/db'
import { db, users, eq } from '@pagespace/db'
import { factories } from '@pagespace/db/test/factories'

// Mock fetch for broadcast testing
Expand All @@ -20,11 +20,9 @@ describe('notifications', () => {
let testPage: Awaited<ReturnType<typeof factories.createPage>>

beforeEach(async () => {
// Clean up ALL test data first (ensures clean slate)
// Deleting users cascades to drives, pages, notifications
await db.delete(users)

// Create fresh test data (factories use unique IDs)
// Create fresh test data with unique IDs (factories use cuid2)
// NOTE: We don't delete ALL users as that can cause race conditions
// with other test files. Instead we rely on unique IDs per test run.
testUser = await factories.createUser()
otherUser = await factories.createUser()
testDrive = await factories.createDrive(testUser.id)
Expand All @@ -35,6 +33,16 @@ describe('notifications', () => {
;(global.fetch as any).mockResolvedValue({ ok: true, json: async () => ({}) })
})

afterEach(async () => {
// Clean up only the test data we created (by cascade from users)
if (testUser?.id) {
await db.delete(users).where(eq(users.id, testUser.id))
}
if (otherUser?.id) {
await db.delete(users).where(eq(users.id, otherUser.id))
}
})

describe('createNotification', () => {
it('creates a notification successfully', async () => {
const notification = await createNotification({
Expand Down
19 changes: 19 additions & 0 deletions packages/lib/src/repositories/__tests__/enforced-file-repository.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ vi.mock('@pagespace/db', () => ({
files: {
findFirst: vi.fn(),
},
filePages: {
findFirst: vi.fn(),
},
},
update: vi.fn(() => ({
set: vi.fn(() => ({
Expand All @@ -35,7 +38,9 @@ vi.mock('@pagespace/db', () => ({
})),
},
files: { id: 'id' },
filePages: { fileId: 'fileId', pageId: 'pageId' },
eq: vi.fn((a, b) => ({ type: 'eq', a, b })),
and: vi.fn((...args) => ({ type: 'and', conditions: args })),
}));

// Mock permissions
Expand Down Expand Up @@ -314,6 +319,14 @@ describe('EnforcedFileRepository', () => {
const mockFile = createMockFile({ driveId: 'drive-123' });

vi.mocked(db.query.files.findFirst).mockResolvedValue(mockFile);
// Mock file-page link exists (file is linked to the bound page)
vi.mocked(db.query.filePages.findFirst).mockResolvedValue({
fileId: 'file-123',
pageId: 'page-123',
linkedBy: null,
linkedAt: new Date(),
linkSource: null,
});
vi.mocked(getUserDrivePermissions).mockResolvedValue(createMockDrivePermissions());

const file = await repo.getFile('file-123');
Expand All @@ -335,6 +348,8 @@ describe('EnforcedFileRepository', () => {
const mockFile = createMockFile({ driveId: 'drive-123' });

vi.mocked(db.query.files.findFirst).mockResolvedValue(mockFile);
// Mock no file-page link exists (file is NOT linked to this page)
vi.mocked(db.query.filePages.findFirst).mockResolvedValue(undefined);

const result = await repo.getFile('file-123');

Expand All @@ -353,6 +368,8 @@ describe('EnforcedFileRepository', () => {
const mockFile = createMockFile({ driveId: 'drive-123' });

vi.mocked(db.query.files.findFirst).mockResolvedValue(mockFile);
// Mock no file-page link exists (file is NOT linked to this page)
vi.mocked(db.query.filePages.findFirst).mockResolvedValue(undefined);

await repo.getFile('file-123');

Expand All @@ -373,6 +390,8 @@ describe('EnforcedFileRepository', () => {
const mockFile = createMockFile({ driveId: 'drive-123' });

vi.mocked(db.query.files.findFirst).mockResolvedValue(mockFile);
// Mock no file-page link exists (file is NOT linked to this page)
vi.mocked(db.query.filePages.findFirst).mockResolvedValue(undefined);

const result = await repo.getFile('file-123');

Expand Down
21 changes: 13 additions & 8 deletions packages/lib/src/repositories/enforced-file-repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module @pagespace/lib/repositories/enforced-file-repository
*/

import { db, files, eq } from '@pagespace/db';
import { db, files, filePages, eq, and } from '@pagespace/db';
import { EnforcedAuthContext } from '../permissions/enforced-context';
import { getUserDrivePermissions } from '../permissions/permissions-cached';
import { loggers } from '../logging/logger-config';
Expand Down Expand Up @@ -102,7 +102,7 @@ export class EnforcedFileRepository {

// 4. Check resource binding
// Token may be bound to a specific file, drive, or page
if (!this.isResourceAccessAllowed(file)) {
if (!(await this.isResourceAccessAllowed(file))) {
loggers.api.warn('File access denied: resource binding mismatch', {
fileId,
userId: this.ctx.userId,
Expand Down Expand Up @@ -195,9 +195,10 @@ export class EnforcedFileRepository {
* A token can be bound to:
* - A specific file (must match exactly)
* - A specific drive (file must be in that drive)
* - A specific page (file must be linked to that page via filePages)
* - No binding (unrestricted)
*/
private isResourceAccessAllowed(file: { id: string; driveId: string }): boolean {
private async isResourceAccessAllowed(file: { id: string; driveId: string }): Promise<boolean> {
const binding = this.ctx.resourceBinding;

// No binding = unrestricted
Expand All @@ -212,10 +213,14 @@ export class EnforcedFileRepository {
case 'drive':
return binding.id === file.driveId;
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;
Comment on lines 215 to +223
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

default:
// Unknown binding type - deny by default
return false;
Expand Down Expand Up @@ -244,7 +249,7 @@ export class EnforcedFileRepository {
}

// Check resource binding
if (!this.isResourceAccessAllowed(file)) {
if (!(await this.isResourceAccessAllowed(file))) {
loggers.api.warn('File update denied: resource binding mismatch', {
fileId,
userId: this.ctx.userId,
Expand Down
58 changes: 35 additions & 23 deletions plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,12 +1090,14 @@ describe('Sessions Schema', () => {
```

**Acceptance Criteria:**
- [ ] Schema deployed to all environments
- [ ] Indexes verified efficient
- [ ] Relations to users table working
- [x] Schema deployed to all environments
- [x] Indexes verified efficient
- [x] Relations to users table working

**Dependencies:** None

**Status:** ✅ COMPLETED (2026-01-13)

---

### P2-T2: Opaque Token Generation
Expand Down Expand Up @@ -1166,13 +1168,15 @@ describe('Opaque Token Generation', () => {
```

**Acceptance Criteria:**
- [ ] Tokens have 256 bits of entropy
- [ ] Format is parseable and type-identifiable
- [ ] Hashing is consistent
- [ ] Format validation is robust
- [x] Tokens have 256 bits of entropy
- [x] Format is parseable and type-identifiable
- [x] Hashing is consistent
- [x] Format validation is robust

**Dependencies:** None

**Status:** ✅ COMPLETED (2026-01-13)

---

### P2-T3: Session Service Implementation
Expand Down Expand Up @@ -1358,13 +1362,15 @@ describe('Session Service', () => {
```

**Acceptance Criteria:**
- [ ] Session creation validates user exists
- [ ] Token validation is comprehensive
- [ ] Revocation is immediate
- [ ] tokenVersion changes invalidate all sessions
- [x] Session creation validates user exists
- [x] Token validation is comprehensive
- [x] Revocation is immediate
- [x] tokenVersion changes invalidate all sessions

**Dependencies:** P2-T1, P2-T2

**Status:** ✅ COMPLETED (2026-01-13)

---

### P2-T4: Dual-Mode Authentication (Migration Support)
Expand Down Expand Up @@ -1446,13 +1452,15 @@ describe('Dual Mode Authentication', () => {
```

**Acceptance Criteria:**
- [ ] Both token types validated correctly
- [ ] Legacy usage tracked in audit log
- [ ] Migration metrics available
- [ ] No functionality regression
- [x] Both token types validated correctly (N/A - clean cutover, no dual mode needed)
- [x] Legacy usage tracked in audit log (N/A - clean cutover)
- [x] Migration metrics available (N/A - clean cutover)
- [x] No functionality regression

**Dependencies:** P2-T3

**Status:** ⏭️ SKIPPED (2026-01-13) - Clean cutover completed, dual-mode not needed

---

### P2-T5: Enforced Auth Context
Expand Down Expand Up @@ -1525,13 +1533,15 @@ describe('Enforced Auth Context', () => {
```

**Acceptance Criteria:**
- [ ] No way to create context without validated session
- [ ] Context is frozen/immutable
- [ ] Scope checking is comprehensive
- [ ] Resource binding enforced
- [x] No way to create context without validated session
- [x] Context is frozen/immutable
- [x] Scope checking is comprehensive
- [x] Resource binding enforced

**Dependencies:** P2-T3

**Status:** ✅ COMPLETED (2026-01-13)

---

### P2-T6: Update Processor Auth Middleware
Expand Down Expand Up @@ -1618,13 +1628,15 @@ describe('Processor Auth Middleware', () => {
```

**Acceptance Criteria:**
- [ ] All processor endpoints use new middleware
- [ ] Both token types work during migration
- [ ] EnforcedAuthContext available on all requests
- [ ] Scope checking enforced
- [x] All processor endpoints use new middleware
- [x] Both token types work during migration (N/A - clean cutover, opaque tokens only)
- [x] EnforcedAuthContext available on all requests
- [x] Scope checking enforced

**Dependencies:** P2-T4, P2-T5

**Status:** ✅ COMPLETED (2026-01-13)

---

### P2-T7: Enforced Repository Pattern
Expand Down