fix(presence): require participation + serialize first-editor check (#197) - #202
Merged
Conversation
…outes
The POST/DELETE /chapter-assignments/{id}/presence routes were gated only
by authenticateUser + requirePermission(CONTENT_UPDATE), so any translator
could register presence on — and read concurrent-editor info for — any
chapterAssignmentId, leaking editor identity across assignments.
Add requireChapterAssignmentAccess(IS_PARTICIPANT) to both routes, matching
the editor-state routes, and document the resulting 403/404 responses (the
403 from requirePermission was previously undocumented too).
Part of #197 (the presence-route authz item; the 2nd-editor-warning QA and
first-editor-race items need a running 2-user environment).
Contributor
📝 WalkthroughWalkthroughThe presence routes add ChangesChapter Assignment Presence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/chapter-assignments/presence/chapter-assignments-presence.route.ts`:
- Around line 33-52: The responses object in the route is missing documentation
for the 400 BAD_REQUEST response that the requireChapterAssignmentAccess
middleware can return when the chapter assignment ID parameter is invalid or
missing. Add a new entry to the responses object using
HttpStatusCodes.BAD_REQUEST as the key, with jsonContent wrapping
createMessageObjectSchema to document the error response with an appropriate
message describing the invalid parameter condition.
🪄 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: ba9b7977-e5aa-4dba-bbcb-2382c00d939c
📒 Files selected for processing (1)
src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts
…ry lock upsertAndQueryFirstEditor pruned, upserted, and selected the first editor in one READ COMMITTED transaction without serializing per chapter. Two near-simultaneous first registrations could each miss the other's uncommitted row and both conclude they are first, so the second user briefly saw no concurrent-editor warning (self-correcting on the next heartbeat). startedAt is fixed per user, so the common 'one already present' case was already correct — only the both-fresh race was affected. Take a transaction-scoped pg_advisory_xact_lock keyed on chapterAssignmentId at the top of the transaction so registrations for the same chapter run strictly one at a time. Uncontended acquisition is negligible; contention only occurs on simultaneous same-chapter opens. Part of #197 (the first-editor-race item; the 2nd-editor-warning QA still needs a running 2-user environment to confirm).
henrique221
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/chapter-assignments/presence/chapter-assignments-presence.repository.ts`:
- Around line 19-24: The first-editor lookup in
chapter-assignments-presence.repository.ts is non-deterministic when multiple
rows share the same startedAt value. Update the ordering used by the
first-editor selection query so it includes a stable secondary sort key, such as
userId, alongside startedAt in the relevant repository method (the one that
reads active_chapter_editors and determines firstEditor). This will make tie
cases deterministic without changing the existing advisory-lock flow or insert
logic.
🪄 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: 2fed6d46-bb7b-443f-9cee-b9bcef851271
📒 Files selected for processing (1)
src/domains/chapter-assignments/presence/chapter-assignments-presence.repository.ts
There was a problem hiding this comment.
Pull request overview
This PR addresses two items from #197 in the chapter-assignment presence feature: tightening record-level authorization on the presence routes to prevent IDOR-style access, and eliminating a rare race in determining the “first editor” during near-simultaneous presence registrations.
Changes:
- Added
requireChapterAssignmentAccess(IS_PARTICIPANT)to the presencePOST/DELETEroutes to ensure only chapter participants can register/remove presence and observe concurrent-editor info. - Documented the new
403/404responses in the OpenAPI route definitions for presence endpoints. - Serialized “first editor” determination by taking a transaction-scoped PostgreSQL advisory lock per
chapterAssignmentIdinsideupsertAndQueryFirstEditor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts |
Adds participant-level access control middleware and documents 403/404 outcomes for presence routes. |
src/domains/chapter-assignments/presence/chapter-assignments-presence.repository.ts |
Adds a per-chapter transaction-scoped advisory lock to prevent race conditions in first-editor selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…deterministic Add BAD_REQUEST to both presence routes' OpenAPI responses since requireChapterAssignmentAccess runs before the zod validators in @hono/zod-openapi and can return 400 for an invalid chapterAssignmentId. Add userId as a stable secondary sort key to the first-editor lookup so startedAt ties resolve deterministically, complementing the advisory lock. Refs: #202
AnuMonachan
approved these changes
Jun 29, 2026
kaseywright
approved these changes
Jun 29, 2026
henrique221
enabled auto-merge (squash)
June 29, 2026 20:27
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts (1)
36-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMirror the positive-integer guard in
src/domains/chapter-assignments/chapter-assignment-auth.middleware.ts.
createRoutemiddleware runs before therequest.paramsschema here, so-1and1.5can reach the lookup path and return404instead of the documented400on both presence routes.Proposed fix in `chapter-assignment-auth.middleware.ts`
const chapterAssignmentId = Number(c.req.param(paramName)); -if (!chapterAssignmentId || Number.isNaN(chapterAssignmentId)) { - return c.json({ message: 'Missing chapter assignment ID' }, HttpStatusCodes.BAD_REQUEST); +if (!Number.isInteger(chapterAssignmentId) || chapterAssignmentId <= 0) { + return c.json({ message: 'Invalid chapter assignment ID' }, HttpStatusCodes.BAD_REQUEST); }🤖 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/chapter-assignments/presence/chapter-assignments-presence.route.ts` around lines 36 - 44, The presence routes are allowing non-positive or non-integer chapter assignment IDs to reach the lookup path because the middleware in chapter-assignment-auth.middleware.ts does not mirror the positive-integer guard before createRoute request param validation. Update the chapter assignment ID check in chapter-assignment-auth.middleware.ts so it rejects values like -1 and 1.5 early with the documented 400 behavior, and make sure the same guard is applied consistently through the route flow used by the chapter-assignments-presence.route.ts endpoints.
🤖 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.
Outside diff comments:
In
`@src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts`:
- Around line 36-44: The presence routes are allowing non-positive or
non-integer chapter assignment IDs to reach the lookup path because the
middleware in chapter-assignment-auth.middleware.ts does not mirror the
positive-integer guard before createRoute request param validation. Update the
chapter assignment ID check in chapter-assignment-auth.middleware.ts so it
rejects values like -1 and 1.5 early with the documented 400 behavior, and make
sure the same guard is applied consistently through the route flow used by the
chapter-assignments-presence.route.ts endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12343fb4-e466-4fbc-9ebc-2e2c8072dd0c
📒 Files selected for processing (2)
src/domains/chapter-assignments/presence/chapter-assignments-presence.repository.tssrc/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts
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
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.
Two #197 presence fixes. (The remaining #197 item — QA that the 2nd-editor warning fires for a real 2nd user — needs a running 2-user environment.)
1. Authz: require chapter-assignment participation (IDOR fix)
The
POST/DELETE /chapter-assignments/{chapterAssignmentId}/presenceroutes were gated only byauthenticateUser+requirePermission(CONTENT_UPDATE), with no record-level check — so any translator could register presence on, and read concurrent-editor info for, anychapterAssignmentId, leaking who's editing what across assignments.requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT)to both routes, matching the sibling editor-state routes (404 to non-participants).403/404responses.2. Serialize the first-editor check (race fix)
upsertAndQueryFirstEditorpruned, upserted, and selected the first editor in one READ COMMITTED transaction with no per-chapter serialization. Two near-simultaneous first registrations could each miss the other's uncommitted row and both conclude they're first → the 2nd user briefly saw no warning (self-correcting on the next 30s heartbeat). Only the both-fresh case was affected;startedAtis fixed per user, so the common "one already present" case was already correct.pg_advisory_xact_lock(chapterAssignmentId)at the top of the transaction so same-chapter registrations run one at a time. (FOR UPDATEwouldn't fix the both-fresh case — there are no existing rows to lock — but the advisory lock does.) Uncontended cost is negligible.Test plan
npm run typechecknpm run lintnpm run test— 80/80 pass/presenceroutesSummary by CodeRabbit