fix(auth): require authentication on bibles and USFM routes#189
Conversation
Bibles and USFM route groups declared no middleware, so the global pass-through authenticate middleware left them effectively public (full bible CRUD and per-project USFM export/download open to anyone). - Add denyUntilAdminRole() placeholder middleware to role-auth.ts (403; swap for requirePermission(<admin>) when the admin role lands). - Bibles: reads require authenticateUser; writes (POST/PATCH/DELETE) are flat-denied via denyUntilAdminRole() per reviewer decision (modification is an admin-only ability, no admin role exists yet). - USFM: all 5 routes require authenticateUser (general auth check). Per reviewer feedback 2026-06-18. Project-scoped USFM authz and the bible-texts/books read-auth decision are tracked in the proposal docs. Verified: typecheck, lint, and the 78-test suite pass.
Capture the four code-audit follow-ups and the reviewer decision on each: auth on open domains (ticket 1, partially implemented here), export error UX + Azure logging (2), async export pipeline hardening (3), and concurrent-editor warning QA (4).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesAuth Enforcement on Open API Domains
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/domains/bibles/bibles.route.ts`:
- Line 23: The secured Bible routes that include the authenticateUser middleware
can now return 403 Forbidden responses (for inactive accounts or admin
placeholders), but these 403 response schemas are not declared in the route
specifications, creating a contract drift between the OpenAPI documentation and
actual runtime behavior. For each route specification that has authenticateUser
middleware (at lines 23, 54, 88, 123, 161, and 202), add a 403 Forbidden
response schema to the responses object to document that these endpoints can
return this status code to clients.
In `@src/domains/usfm/usfm.route.ts`:
- Line 72: The USFM routes currently only enforce authentication via the
authenticateUser middleware, but lack authorization checks to verify users can
access the specific project. On all five routes referenced (lines 72, 86, 110,
126, and 143 where middleware arrays are defined), extend the middleware array
to include additional middleware or checks that validate project-scoped
permissions and enforce project policy checks. Ensure authenticated users can
only access projects within their organization by adding authorization
validation that verifies the user has the appropriate permissions for the
requested project before allowing the operation to proceed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2eb70b26-21b1-4c65-a7d5-44ec74f65aed
📒 Files selected for processing (8)
docs/proposals/security-and-export-hardening/01-auth-open-domains.mddocs/proposals/security-and-export-hardening/02-export-error-ux-and-logging.mddocs/proposals/security-and-export-hardening/03-async-export-pipeline-hardening.mddocs/proposals/security-and-export-hardening/04-concurrent-editor-warning-qa.mddocs/proposals/security-and-export-hardening/README.mdsrc/domains/bibles/bibles.route.tssrc/domains/usfm/usfm.route.tssrc/middlewares/role-auth.ts
The bibles routes can now return 403 (inactive account via authenticateUser on all routes; admin placeholder via denyUntilAdminRole() on writes), but the OpenAPI specs declared no 403, mirroring the same 401 spec/behaviour drift called out in the PR. Add a FORBIDDEN response to all six secured routes, matching the projects.route.ts pattern. Refs: #189
There was a problem hiding this comment.
Pull request overview
Secures previously-public Bibles and USFM export/download endpoints by adding per-route authentication middleware, and introduces a temporary “admin-only” hard-deny middleware for Bible write operations until an admin role/permission is available.
Changes:
- Added
denyUntilAdminRole()placeholder middleware (always 403) inrole-auth.ts. - Required
authenticateUseron all USFM export/job/download routes. - Required
authenticateUserfor Bible reads and hard-denied Bible writes viadenyUntilAdminRole(). - Added a set of security/export hardening proposal docs under
docs/proposals/security-and-export-hardening/.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/middlewares/role-auth.ts | Adds denyUntilAdminRole() placeholder middleware for admin-only actions. |
| src/domains/usfm/usfm.route.ts | Adds authenticateUser middleware to USFM export/job/download routes. |
| src/domains/bibles/bibles.route.ts | Adds authenticateUser to reads and denyUntilAdminRole() to writes. |
| docs/proposals/security-and-export-hardening/README.md | Introduces proposal index/overview and ticket list. |
| docs/proposals/security-and-export-hardening/01-auth-open-domains.md | Documents ticket 1 decisions/implementation and follow-ups. |
| docs/proposals/security-and-export-hardening/02-export-error-ux-and-logging.md | Documents ticket 2 scope/acceptance criteria. |
| docs/proposals/security-and-export-hardening/03-async-export-pipeline-hardening.md | Documents ticket 3 async pipeline defects and fixes. |
| docs/proposals/security-and-export-hardening/04-concurrent-editor-warning-qa.md | Documents ticket 4 QA plan and potential concurrency concerns. |
Comments suppressed due to low confidence (4)
src/domains/bibles/bibles.route.ts:89
- This route now requires
authenticateUser, but the documented OpenAPI responses omit 403 (e.g., inactive user). Add a 403 response to keep the spec aligned with actual behavior.
if (result.ok) return c.json(result.data, HttpStatusCodes.OK);
return c.json({ message: result.error.message }, getHttpStatus(result.error) as never);
});
src/domains/bibles/bibles.route.ts:124
- This route now has
denyUntilAdminRole()middleware, which will always return 403 for authenticated callers (andauthenticateUsercan also return 403 for inactive users). The OpenAPI spec currently doesn’t document any 403 response for this endpoint.
server.openapi(getBiblesByLanguageIdRoute, async (c) => {
const { languageId } = c.req.valid('param');
const result = await bibleService.getBiblesByLanguageId(languageId);
src/domains/bibles/bibles.route.ts:165
- This endpoint now hard-denies via
denyUntilAdminRole()(403), but 403 isn’t documented in the OpenAPI responses. That makes the API contract inaccurate for clients and generated docs.
summary: 'Create a new bible',
description: 'Creates a new bible with the provided data',
});
server.openapi(createBibleRoute, async (c) => {
src/domains/bibles/bibles.route.ts:206
- This endpoint now always returns 403 for authenticated callers via
denyUntilAdminRole(), but the OpenAPIresponsesdon’t include a 403 entry.
'Internal server error'
),
},
summary: 'Update bible by ID',
description: 'Updates a specific bible with the provided data',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
The five USFM export routes now run authenticateUser, which can return 401 (no user) and 403 (inactive account), but their OpenAPI responses maps omitted both. Add UNAUTHORIZED/FORBIDDEN entries to each route using the file's local errorSchema to keep the generated contract in sync with runtime behavior. Also make denyUntilAdminRole self-consistent: throw 401 when there is no user in context (instead of always 403), matching its docstring and the requirePermission/requireSelf guards in the same file. Refs: #189
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domains/bibles/bibles.route.ts (1)
135-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick win403 docs on write routes omit the inactive-account forbidden path.
With
authenticateUserfirst in the chain, write endpoints can return 403 for both inactive accounts and admin-only restriction. Documenting only the admin reason creates API contract drift for clients handling 403 causes.Suggested minimal doc fix
- 'Restricted to administrators' + 'Forbidden (inactive account or restricted to administrators)'Apply to the 403 response descriptions in create/update/delete bible routes.
Also applies to: 177-199, 222-237
🤖 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 `@src/domains/bibles/bibles.route.ts` around lines 135 - 155, The 403 FORBIDDEN response documentation in the denyUntilAdminRole() middleware routes is incomplete. Update the description for the HttpStatusCodes.FORBIDDEN response in the route definitions to indicate that a 403 can be returned for two different reasons: either because the user account is inactive (from authenticateUser middleware) or because the user lacks the required admin role (from denyUntilAdminRole middleware). Apply this fix to all affected write routes (create, update, and delete bible routes) to ensure the API contract accurately reflects all possible 403 causes.
🤖 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.
Inline comments:
In `@docs/proposals/security-and-export-hardening/01-auth-open-domains.md`:
- Around line 53-57: The bulk-texts endpoint decision is incomplete and needs
resolution. Currently it's marked as "public + rate-limited pending decision"
which is insufficient security. You must either: (1) add authenticateUser to the
POST /bibles/{bibleId}/bulk-texts endpoint and move it behind authentication if
anonymous pre-cache access is not actually needed, or (2) if anonymous access
for mobile pre-caching IS intentional, explicitly document this use case and the
associated data exposure risks in the proposal. Do not leave the endpoint in an
undefined state with only rate-limiting as protection.
In
`@docs/proposals/security-and-export-hardening/02-export-error-ux-and-logging.md`:
- Around line 24-28: Before aligning the USFM export error responses with the
standard error envelope, verify frontend compatibility by checking the
fluent-web application and any other consumers of the USFM export endpoints in
src/domains/usfm/usfm.route.ts to ensure they can handle the new { message }
response format instead of the current { error, details } format. Additionally,
review all integration tests that interact with the USFM export routes
(particularly those testing error cases at lines 191–196, 211–217, 289–296) to
identify and update any assertions or expectations that depend on the old error
response structure, ensuring no tests break after the contract change is
implemented.
In
`@docs/proposals/security-and-export-hardening/03-async-export-pipeline-hardening.md`:
- Around line 21-26: The issue confirms that the separate tmpfs instances for
API and worker services prevent the API from accessing files written by the
worker, causing 404 errors on the downloads endpoint. To fix this, modify the
docker-compose configuration to replace the isolated tmpfs mounts at
/app/exports with a shared persistent volume mounted in both the API and worker
services, or alternatively implement S3/Azure Blob storage with signed URLs for
the downloads endpoint. Update the export pipeline logic to write exports to the
shared storage location instead of the isolated tmpfs, ensuring both services
can access the same exported files.
In
`@docs/proposals/security-and-export-hardening/04-concurrent-editor-warning-qa.md`:
- Around line 28-31: Add a security follow-up task item to this proposal
document that specifically addresses the missing authorization check in the
presence routes. The follow-up should note that the POST/DELETE endpoints for
/chapter-assignments/{chapterAssignmentId}/presence currently lack the
requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT) check
and must be updated to include this authorization check to align with the
editor-state routes pattern and prevent translators from probing presence
information across arbitrary chapter assignments.
In `@src/domains/usfm/usfm.route.ts`:
- Line 72: All five routes in the file that include the authenticateUser
middleware (at lines 72, 86, 110, 126, and 143) are missing UNAUTHORIZED and
FORBIDDEN response declarations in their OpenAPI responses objects. For each of
these five routes, add the UNAUTHORIZED (401) and FORBIDDEN (403) status codes
to the responses object to properly document that authentication failures are
possible outcomes. This ensures the OpenAPI contract accurately reflects the
authentication requirements and allows clients to handle these error cases
correctly.
---
Outside diff comments:
In `@src/domains/bibles/bibles.route.ts`:
- Around line 135-155: The 403 FORBIDDEN response documentation in the
denyUntilAdminRole() middleware routes is incomplete. Update the description for
the HttpStatusCodes.FORBIDDEN response in the route definitions to indicate that
a 403 can be returned for two different reasons: either because the user account
is inactive (from authenticateUser middleware) or because the user lacks the
required admin role (from denyUntilAdminRole middleware). Apply this fix to all
affected write routes (create, update, and delete bible routes) to ensure the
API contract accurately reflects all possible 403 causes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b011cf49-2352-406a-82f8-623f58a3832b
📒 Files selected for processing (8)
docs/proposals/security-and-export-hardening/01-auth-open-domains.mddocs/proposals/security-and-export-hardening/02-export-error-ux-and-logging.mddocs/proposals/security-and-export-hardening/03-async-export-pipeline-hardening.mddocs/proposals/security-and-export-hardening/04-concurrent-editor-warning-qa.mddocs/proposals/security-and-export-hardening/README.mdsrc/domains/bibles/bibles.route.tssrc/domains/usfm/usfm.route.tssrc/middlewares/role-auth.ts
The USFM routes documented 401/403 with the local errorSchema
({ error, details? }), but those statuses are thrown by authenticateUser
as HTTPExceptions and the global handler (server.ts) serializes them as
{ message }. So the documented shape didn't match what clients receive.
Switch all five routes' 401/403 to stoker's createMessageObjectSchema,
matching the bibles routes and the actual response. USFM was the only
domain with this mismatch.
Addresses review feedback from @Joel-Joseph-George on #189.
…#312) useExportUsfm omitted credentials: 'include' — the only API call in the app that did. It worked only because the export endpoint was unauthenticated; once fluent-api requires auth (eten-tech-foundation/fluent-api#189) the request would 401 and the editor export would break. - Add credentials: 'include' so the session cookie is sent. - Parse the JSON error body (supports both { message } and the USFM route's { error } shape) and surface it, so ExportProjectDialog shows the actual reason (e.g. 'Invalid book IDs') instead of a hardcoded 'Export Failed'. Refs eten-tech-foundation/fluent-api#195
After #189 the USFM routes required authentication but no project/org check, so any logged-in user could export another organization's translated content by guessing a projectUnitId. - Add requireProjectUnitAccess middleware: resolves projectUnitId -> project -> org and applies ProjectPolicy.read, mirroring translated-verse-auth.middleware.ts. Returns 404 (not 403) on denial to avoid enumeration, and stashes the resolved project on the context. - Apply it to the three /project-units/{projectUnitId}/usfm* routes (books, sync export, async export). /jobs and /downloads are async-only; binding them to an owner is tracked in #196. - Document each route's 404 as { message } (createMessageObjectSchema), matching what the middleware/global handler returns. - Sync handler now reads the project name from the context instead of re-querying via getProjectName, removing a redundant query and its duplicate { error } 404. Closes #198.
* feat(usfm): scope export endpoints to the caller's project/org After #189 the USFM routes required authentication but no project/org check, so any logged-in user could export another organization's translated content by guessing a projectUnitId. - Add requireProjectUnitAccess middleware: resolves projectUnitId -> project -> org and applies ProjectPolicy.read, mirroring translated-verse-auth.middleware.ts. Returns 404 (not 403) on denial to avoid enumeration, and stashes the resolved project on the context. - Apply it to the three /project-units/{projectUnitId}/usfm* routes (books, sync export, async export). /jobs and /downloads are async-only; binding them to an owner is tracked in #196. - Document each route's 404 as { message } (createMessageObjectSchema), matching what the middleware/global handler returns. - Sync handler now reads the project name from the context instead of re-querying via getProjectName, removing a redundant query and its duplicate { error } 404. Closes #198. * fix(usfm): enforce positive integer projectUnitId in access guard Reject non-integer values (1.5, Infinity, NaN, 0, negatives) before any DB query, matching the route's z.coerce.number().int().positive() schema. Refs: #206
Closes #194
Summary
The
biblesandusfmroute groups declared nomiddleware, so the globalpass-through
authenticatemiddleware left them effectively public — fullBible CRUD (incl. create/update/delete of shared source data) and per-project
USFM export/download were reachable without authentication. This closes those
holes following the established
authenticateUser→requirePermission→policy pattern used by the projects/users/chapter-assignments domains.
Found in a code audit of
fluent-web+fluent-api(2026-06-18); scoped perreviewer feedback.
What changed
role-auth.ts— addeddenyUntilAdminRole()placeholder middleware (403).One-line swap to
requirePermission(<admin permission>)once the admin rolelands.
GET /bibles,/bibles/{id},/bibles/language/{id})require
authenticateUser; writes (POST/PATCH/DELETE) are flat-deniedvia
denyUntilAdminRole()(modification is an admin-only ability; no admin roleexists yet).
authenticateUser(general auth check).Reviewer decisions encoded here (2026-06-18)
the admin role exists.
Securing
POST /project-units/{id}/usfmwill break the editor's exportunless
fluent-websends credentials.useExportUsfm.tsis the only fetch in theweb app missing
credentials: 'include'— it works today only because theendpoint is open. Ship the
fluent-webcredentials fix with, or ahead of, thisPR. Tracked as ticket 2 below.
Open follow-ups (documented, not in this PR)
See
docs/proposals/security-and-export-hardening/(added in this branch):content, so a logged-in user from another org can still export a project they
aren't a member of. Recommend a
requireProjectUnitAccessmiddleware mirroringtranslated-verse-auth.middleware.ts.endpoint is documented "no auth required" (possible mobile precache). Decide
whether to lock down or rate-limit.
(ticket 3), concurrent-editor warning QA (ticket 4).
Test plan
npm run typechecknpm run lintnpm run test— 78/78 passPOST /biblesreturns 403fluent-websends credentialsSummary by CodeRabbit
Release Notes
Security Updates
Documentation