[Security][P1] Enforce resource binding on processor read operations #566
Conversation
…561) Fixes asymmetric enforcement where write operations enforced resourceBinding but read operations (serve, optimize, ingest) only checked userId + contentHash. Changes: - Add AuthorizationDecision type with full audit structure - Add authorizeFileAccess/assertFileAccess in new authorization service - Add requireResourceBinding middleware for fast-fail validation - Update serve.ts, optimize.ts, ingest.ts to use new authorization - Remove old checkFileAccess/assertFileAccess from rbac.ts - Add comprehensive test coverage (39 test cases) Zero-trust principles implemented: - Fail secure: unknown binding types denied - Defense in depth: middleware → auth service → repository - Audit everything: AuthorizationDecision logged for all access - Least privilege: token binding restricts to specific resources Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds resource- and page-binding middlewares that fast-fail mismatched service tokens, replaces legacy RBAC file-access with a new authorization service returning structured decisions, applies binding and read rate-limit middleware to processor routes, and adds comprehensive unit tests for binding and authorization logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BindingMW as Binding middleware
participant AuthSvc as Authorization service
participant DB as Database
participant Handler as Route handler
Client->>BindingMW: Request (service token + params/body)
BindingMW->>BindingMW: Inspect token binding
alt file-bound
BindingMW->>BindingMW: Extract contentHash (params/body)
alt match (case-insensitive)
BindingMW->>AuthSvc: Forward request with auth
else mismatch
BindingMW-->>Client: 403 Forbidden (log security warning)
end
else page-bound
BindingMW->>BindingMW: Extract pageId (params)
alt match
BindingMW->>AuthSvc: Forward request with auth
else mismatch
BindingMW-->>Client: 403 Forbidden (log security warning)
end
else unbound/other
BindingMW->>AuthSvc: Forward request with auth for full check
end
AuthSvc->>DB: Fetch file links & drive context
AuthSvc->>AuthSvc: Determine binding match type & scope
AuthSvc->>AuthSvc: Check page/drive permissions
alt allowed
AuthSvc-->>Handler: AuthorizationDecision (allowed)
Handler->>Client: 200 Processed
else denied
AuthSvc-->>Client: 403 Denied (with decision)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/processor/src/api/ingest.ts`:
- Around line 22-30: The requirePageBinding() middleware is ineffective for the
ingest route because req.params.pageId is undefined at app.use() scope; move the
page-binding enforcement into the sub-router (the route handler registered as
router.post('/by-page/:pageId', ...) or apply requirePageBinding() directly to
that route) so the middleware runs when pageId is in scope, or alternatively add
a dedicated check in the sub-router before the ingest logic (the same
binding/type/id check currently inline in apps/processor/src/api/ingest.ts) and
remove or document the misleading global middleware usage.
In `@apps/processor/src/services/authorization.ts`:
- Around line 113-115: The 'file' comparison in determineBindingMatchType
incorrectly compares binding.id to contentHash without normalization; update
determineBindingMatchType's 'file' case to normalize binding.id (e.g.,
toLowerCase or same normalization used in requireResourceBinding) before
comparing to the already-normalized contentHash so mixed-case binding IDs match
correctly; locate the switch in determineBindingMatchType and change the 'file'
branch to compare normalized binding.id with contentHash.
🧹 Nitpick comments (3)
apps/processor/src/services/__tests__/authorization.test.ts (1)
44-55:isBoundToResourcedefault is misleading for unbound token tests.
createAuthdefaultsisBoundToResource: () => truewhile also defaultingresourceBinding: undefined. Tests for unbound tokens (e.g., lines 90, 110, 124) inherit this inconsistency. If the authorization service ever delegates toisBoundToResource(), these tests would silently produce wrong results.🔧 Suggested fix
function createAuth(overrides: Partial<EnforcedAuthContext> = {}): EnforcedAuthContext { return { userId: 'user-1', userRole: 'user', resourceBinding: undefined, driveId: undefined, hasScope: () => true, isAdmin: () => false, - isBoundToResource: () => true, + isBoundToResource: () => !!overrides.resourceBinding, ...overrides, } as unknown as EnforcedAuthContext; }apps/processor/src/api/serve.ts (1)
25-25: Consider adding a type annotation todecision.
let decision;is implicitlyanyat this point. Adding a type annotation improves readability and prevents accidental misuse. As per coding guidelines, code should be explicit and never useanytypes.🔧 Suggested fix
- let decision; + let decision: Awaited<ReturnType<typeof authorizeFileAccess>> | undefined;Or import and use the
AuthorizationDecisiontype directly if exported:- let decision; + let decision: AuthorizationDecision | undefined;apps/processor/src/services/authorization.ts (1)
280-296: Defensive branch uses hardcoded'mismatch'matchType instead of the computed value.At line 284, when
scopedLinksis empty, the decision is built with a hardcoded'mismatch'matchType rather than the actualmatchTypecomputed at line 222. While this branch should be unreachable given the upstream consistency betweendetermineBindingMatchTypeandgetScopedLinks, if it ever does trigger, the audit log would show a misleadingmatchType.Suggested fix
- const decision = buildDeniedDecision('binding', binding, 'mismatch', requirement); + const decision = buildDeniedDecision('binding', binding, matchType, requirement);
| const binding = auth.resourceBinding; | ||
| if (binding?.type === 'page' && binding.id !== pageId) { | ||
| loggers.security.warn('ingest denied: page binding mismatch', { | ||
| userId: auth.userId, | ||
| requestedPageId: pageId, | ||
| boundPageId: binding.id, | ||
| }); | ||
| return res.status(403).json({ error: 'Access denied: token is bound to a different page' }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify route structure and middleware application
echo "=== Checking ingest route definition ==="
rg -n 'pageId' apps/processor/src/api/ingest.ts | head -20
echo ""
echo "=== Checking server.ts mount for ingest router ==="
rg -n -B2 -A2 'ingestRouter\|requirePageBinding' apps/processor/src/server.ts | head -30
echo ""
echo "=== Checking requirePageBinding middleware implementation ==="
rg -n -A10 'requirePageBinding' apps/processor/src/middleware/ | head -40Repository: 2witstudios/PageSpace
Length of output: 4053
🏁 Script executed:
#!/bin/bash
echo "=== Full requirePageBinding middleware implementation ==="
sed -n '94,130p' apps/processor/src/middleware/resource-binding.ts
echo ""
echo "=== Checking server.ts for ingestRouter imports and mounting ==="
grep -n 'ingest' apps/processor/src/server.ts
echo ""
echo "=== Checking full server.ts middleware chain for /api/ingest ==="
sed -n '1,100p' apps/processor/src/server.ts | tail -50Repository: 2witstudios/PageSpace
Length of output: 2930
🏁 Script executed:
#!/bin/bash
echo "=== Lines after the log in requirePageBinding ==="
sed -n '94,150p' apps/processor/src/middleware/resource-binding.tsRepository: 2witstudios/PageSpace
Length of output: 1035
The requirePageBinding() middleware in server.ts line 51 silently passes for the ingest route because pageId is not in scope at the app.use() level.
Since :pageId is defined only in the sub-router (router.post('/by-page/:pageId', ...)), the middleware receives undefined for req.params.pageId, triggering its early exit (if (!pageId) { next(); return; }). The inline check in this handler (lines 22–30) is the sole enforcement point for page-bound tokens on ingest requests.
While this doesn't create a security gap, the middleware wiring creates a false impression of defense-in-depth. Consider moving the page-binding check into the sub-router where pageId is properly in scope, or document this as a known limitation of the current middleware architecture.
🤖 Prompt for AI Agents
In `@apps/processor/src/api/ingest.ts` around lines 22 - 30, The
requirePageBinding() middleware is ineffective for the ingest route because
req.params.pageId is undefined at app.use() scope; move the page-binding
enforcement into the sub-router (the route handler registered as
router.post('/by-page/:pageId', ...) or apply requirePageBinding() directly to
that route) so the middleware runs when pageId is in scope, or alternatively add
a dedicated check in the sub-router before the ingest logic (the same
binding/type/id check currently inline in apps/processor/src/api/ingest.ts) and
remove or document the misleading global middleware usage.
There was a problem hiding this comment.
This is a known architectural limitation. The requirePageBinding() middleware receives undefined for req.params.pageId at the app.use() level because Express route params are only populated by the matched route pattern.
The inline check in the route handler (lines 22-30) is the intentional enforcement point. This provides an early-fail optimization before the expensive getPageForIngestion() DB lookup.
I've kept this as-is because:
- The security is correctly enforced at the route handler level
- Moving binding validation into middleware would require Express 5+ router merging or a custom param-parsing middleware
- The current implementation follows the pattern established in other routes (e.g., delete-file)
I can add a comment documenting this limitation if preferred.
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
Fixes CodeQL alert: "Missing rate limiting" on serve.ts - Add createRateLimiter factory for configurable rate limiters - Add rateLimitRead middleware with 1000 req/hour default - Apply rate limiter to /cache routes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix case-sensitivity in determineBindingMatchType for file bindings - Fix hardcoded 'mismatch' matchType in scopedLinks branch - Add type annotation for decision variable in serve.ts - Fix test helper isBoundToResource to match binding state Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move rate limiting from server.ts middleware chain to individual route handlers so CodeQL can detect the protection at the file level. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The rateLimitRead middleware provides rate limiting, but CodeQL doesn't recognize custom rate limiters. Adding suppression comment to document that rate limiting is properly implemented. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The closure-based approach was causing issues with the spread operator override. Using direct access to overrides.resourceBinding fixes the test. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/processor/src/api/serve.ts (1)
113-119:⚠️ Potential issue | 🟡 MinorLeaking internal error details to clients in 500 responses.
Lines 117 and 192 include
error.messagein the JSON response. In a security-focused PR, this could leak internal implementation details (DB errors, stack info, etc.) to callers.💡 Suggested fix
res.status(500).json({ error: 'Failed to serve original file', - message: error instanceof Error ? error.message : 'Unknown error' });
🤖 Fix all issues with AI agents
In `@apps/processor/src/services/authorization.ts`:
- Around line 327-343: The FileAuthorizationError thrown in assertFileAccess
currently embeds auth.userId in the error message; change this to follow the
assertDeleteFileAccess pattern by logging the denial to the security logger with
structured metadata and throwing a generic message without the userId.
Specifically, in assertFileAccess (which calls authorizeFileAccess), call
loggers.security.warn/error with an object containing userId: auth.userId,
contentHash, requirement, and any decision details, then throw new
FileAuthorizationError with a non-identifying message like "Not authorized to
view/modify file" (determine action from requirement) and include decision — do
not interpolate auth.userId into the message. Ensure the thrown
FileAuthorizationError signature and decision payload remain unchanged.
🧹 Nitpick comments (4)
.claude/ralph-loop.local.md (1)
1-9: Local workflow file included in the PR — likely should not be committed.The
.local.mdsuffix suggests this is a local-only agent configuration. Consider adding.claude/*.local.mdto.gitignoreto prevent accidental commits of developer-specific workflow files.apps/processor/src/middleware/rate-limit.ts (1)
77-105:rateLimitUploadduplicates the logic now encapsulated bycreateRateLimiter.Consider refactoring
rateLimitUploadto use the new factory to eliminate the duplicate bucket management and cleanup code:♻️ Proposed refactor
-const buckets = new Map<string, RateLimitBucket>(); const DEFAULT_LIMIT = parseInt(process.env.PROCESSOR_UPLOAD_RATE_LIMIT ?? '100', 10); const WINDOW_SECONDS = parseInt(process.env.PROCESSOR_UPLOAD_RATE_WINDOW ?? '3600', 10); -// ... getBucketKey ... +export const rateLimitUpload = createRateLimiter( + 'upload', + DEFAULT_LIMIT, + WINDOW_SECONDS, + 'Upload rate limit exceeded' +); -export function rateLimitUpload(req: Request, res: Response, next: NextFunction): void { - // ... 28 lines of duplicate logic ... -} - -// ... remove orphaned cleanup interval at bottom of file ...This would also remove the now-orphaned cleanup interval at lines 117–124.
apps/processor/src/api/serve.ts (1)
27-35: Catch-all swallows unexpected errors as 403.The outer
tryblock (line 28) catches everything fromauthorizeFileAccess, including potential database connectivity errors or unexpected runtime exceptions, and maps them uniformly to a 403. While fail-secure is the right default for authorization, this makes it harder to diagnose infrastructure issues since they'll appear as access denials rather than 500s.Consider narrowing the catch to only
FileAuthorizationError:💡 Suggested improvement
try { decision = await authorizeFileAccess(auth, contentHash, 'view'); if (!decision.allowed) { return res.status(403).json({ error: 'Access denied for requested file' }); } - } catch { - return res.status(403).json({ error: 'Access denied for requested file' }); + } catch (error) { + if (error instanceof FileAuthorizationError) { + return res.status(403).json({ error: 'Access denied for requested file' }); + } + throw error; // Let the outer catch handle infrastructure errors as 500 }That said, there's a valid argument for fail-secure here — if you prefer masking infrastructure errors for security reasons, add a comment documenting the intentional choice.
apps/processor/src/services/authorization.ts (1)
149-170: Sequential permission checks are fine for typical cardinality but worth documenting.
checkPagePermissionsiterates links sequentially, issuing onegetUserAccessLevelcall per link. For the typical case (a file linked to 1–3 pages), this is negligible. If file-to-page fan-out could grow large in the future, consider parallelizing withPromise.allor adding a short-circuit limit.
| export async function assertFileAccess( | ||
| auth: EnforcedAuthContext, | ||
| contentHash: string, | ||
| requirement: AccessRequirement | ||
| ): Promise<AuthorizationDecision> { | ||
| const decision = await authorizeFileAccess(auth, contentHash, requirement); | ||
|
|
||
| if (!decision.allowed) { | ||
| const action = requirement === 'edit' ? 'modify' : 'view'; | ||
| throw new FileAuthorizationError( | ||
| `User ${auth.userId} is not authorized to ${action} file`, | ||
| decision | ||
| ); | ||
| } | ||
|
|
||
| return decision; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check serve.ts for how assertFileAccess errors are caught and handled
fd serve.ts apps/processor/src -x cat -n {}Repository: 2witstudios/PageSpace
Length of output: 9778
🏁 Script executed:
# Search for Express error handlers that might log error messages
rg -n "error\.message|err\.message|app\.use.*error|express.*middleware" apps/processor/src --type ts -C 2 | head -50Repository: 2witstudios/PageSpace
Length of output: 3327
🏁 Script executed:
# Look for logging configuration or PII-related comments/policies
rg -n "logger|log|pii|PII|audit|userId.*log" apps/processor/src --type ts | head -30Repository: 2witstudios/PageSpace
Length of output: 3479
🏁 Script executed:
# Check if there's any documentation or configuration about logging
fd -e md -e txt -e json apps/processor | xargs rg -l "log|PII|userId" 2>/dev/null | head -10Repository: 2witstudios/PageSpace
Length of output: 730
🏁 Script executed:
# Check logger.ts for logging strategy
cat -n apps/processor/src/logger.tsRepository: 2witstudios/PageSpace
Length of output: 1995
🏁 Script executed:
# Look for other assertFileAccess or similar authorization error logging patterns
rg -n "assertFileAccess|FileAuthorizationError|userId" apps/processor/src/services --type ts -B 2 -A 2Repository: 2witstudios/PageSpace
Length of output: 26583
🏁 Script executed:
# Check if there's a centralized error logging configuration
fd -type f "log" apps/processor/src --full-pathRepository: 2witstudios/PageSpace
Length of output: 235
Follow the established pattern from assertDeleteFileAccess: log userId via structured metadata fields, not in error messages.
The error message at line 337 embeds auth.userId directly, which is fragile. While serve.ts currently catches this error before it reaches logging, the pattern contradicts the established approach in assertDeleteFileAccess (rbac.ts), which logs userId as a separate metadata field to loggers.security. Refactor to match that pattern for consistency and defensive depth.
🤖 Prompt for AI Agents
In `@apps/processor/src/services/authorization.ts` around lines 327 - 343, The
FileAuthorizationError thrown in assertFileAccess currently embeds auth.userId
in the error message; change this to follow the assertDeleteFileAccess pattern
by logging the denial to the security logger with structured metadata and
throwing a generic message without the userId. Specifically, in assertFileAccess
(which calls authorizeFileAccess), call loggers.security.warn/error with an
object containing userId: auth.userId, contentHash, requirement, and any
decision details, then throw new FileAuthorizationError with a non-identifying
message like "Not authorized to view/modify file" (determine action from
requirement) and include decision — do not interpolate auth.userId into the
message. Ensure the thrown FileAuthorizationError signature and decision payload
remain unchanged.
Testing if original tests pass CI Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/processor/src/middleware/__tests__/resource-binding.test.ts`:
- Around line 128-135: Remove the leftover debugging console.log statements in
the test: delete the lines logging 'DEBUG: auth object:', 'DEBUG:
auth.resourceBinding:', 'DEBUG: req.params:', 'DEBUG: next called:', and 'DEBUG:
statusCode' around the call to middleware(req, res, next) in
resource-binding.test.ts; ensure the test still invokes middleware(req, res,
next) and asserts expected behavior (using authObj, req, res, next, statusCode)
without printing to stdout.
apps/processor/src/middleware/__tests__/resource-binding.test.ts
Outdated
Show resolved
Hide resolved
The original createMockResponse() used JavaScript getters which don't work correctly when destructured - the getter is called once during destructuring and the primitive value is stored, rather than preserving the getter behavior. Changed to return getter functions (getStatusCode, getJsonBody) that must be called explicitly to retrieve the current values from the closure. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update suppression comment from lgtm[...] to codeql[...] syntax to resolve CodeQL rate limiting alert. Rate limiting is correctly applied via rateLimitRead middleware on each route. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Fixes #561 - Processor token scope/resource binding not consistently enforced
Problem: Asymmetric enforcement - write operations (upload, delete) enforced
resourceBindingfromEnforcedAuthContext, but read operations (serve, optimize, ingest) only checkeduserId + contentHash, allowing scoped tokens to access files beyond their intended boundary.Solution: Implement 3-layer defense-in-depth with full audit trail:
AuthorizationDecisionaudit typecheckFileAccess/assertFileAccessChanges
New Files
apps/processor/src/services/authorization.ts- New authorization service withAuthorizationDecisiontypeapps/processor/src/middleware/resource-binding.ts- Fast-fail middleware for binding validationapps/processor/src/services/__tests__/authorization.test.ts- 24 test casesapps/processor/src/middleware/__tests__/resource-binding.test.ts- 15 test casesModified Files
apps/processor/src/server.ts- Add binding middleware to routesapps/processor/src/api/serve.ts- Use new authorization serviceapps/processor/src/api/optimize.ts- Use new authorization serviceapps/processor/src/api/ingest.ts- Use new authorization service + page binding checkapps/processor/src/services/rbac.ts- Remove old functions, keep delete-file authapps/processor/src/middleware/auth.ts- Re-export ResourceBinding typeZero-Trust Principles
AuthorizationDecisionlogged for all file access attemptsTest plan
cd apps/processor && pnpm testcd apps/processor && pnpm tsc --noEmitAuthorizationDecisionstructure🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests