fix(experiences): scope idempotency dedup to the author - #1803
fix(experiences): scope idempotency dedup to the author#1803ionfwsrijan wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughInterview-experience idempotency now scopes duplicate detection to the authenticated user or anonymous client key. MongoDB indexes match these scopes. Unit tests cover key reuse and duplicate-key race recovery. ChangesInterview idempotency scope
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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
🤖 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 `@backend/controllers/interviewExperienceController.js`:
- Line 104: The duplicate-key recovery must reuse the validated, trimmed
idempotency key and author filter instead of the raw request value. In
backend/controllers/interviewExperienceController.js:104, retain those
normalized values from the initial validation and pass them to both lookup paths
in the duplicate-key branch. In
backend/tests/interviewExperienceController.unit.test.js:230-263, add a
whitespace-padded duplicate-key race case and assert both lookups use the
trimmed key.
In `@backend/models/InterviewExperience.js`:
- Around line 101-117: Make authenticated and anonymous ownership scopes
mutually exclusive: in backend/models/InterviewExperience.js lines 101-117,
restrict the anonymous clientKey/idempotencyKey unique index to documents with
userId null; in backend/controllers/interviewExperienceController.js lines
79-82, strip caller-supplied clientKey for authenticated requests, strip
caller-supplied userId for anonymous requests, and limit anonymous lookups to
ownerless records; in backend/tests/interviewExperienceController.unit.test.js
lines 207-228, add coverage for an authenticated submission followed by an
anonymous submission reusing the same clientKey and idempotencyKey.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec2fbd5e-daeb-42a6-8885-7474b261dc88
📒 Files selected for processing (3)
backend/controllers/interviewExperienceController.jsbackend/models/InterviewExperience.jsbackend/tests/interviewExperienceController.unit.test.js
| try { | ||
| const existing = await InterviewExperience.findOne({ | ||
| idempotencyKey: req.body.idempotencyKey, | ||
| ...buildAuthorFilter(req, req.body.clientKey), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the normalized idempotency key in duplicate-key recovery.
Line 57 stores the trimmed key, but line 104 queries with the raw request value. If two concurrent requests submit " submit-key-abc12345 ", the losing request cannot find the winning record and returns HTTP 500.
backend/controllers/interviewExperienceController.js#L104-L104: retain the validated, trimmed idempotency key and author filter for use in the duplicate-key branch.backend/tests/interviewExperienceController.unit.test.js#L230-L263: add a duplicate-key race test with surrounding whitespace and assert that both lookups use the trimmed key.
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 101-104: Untrusted HTTP request data (req.body / req.query / req.params) flows into a MongoDB/Mongoose query, enabling NoSQL injection — an attacker can supply objects like {"$gt":""} or {"$where":"..."} to bypass filters or run arbitrary JavaScript. Never pass raw request data as a query object or operator value; validate and cast each field to its expected primitive type (e.g. with a schema validator), or whitelist allowed operators before querying.
Context: InterviewExperience.findOne({
idempotencyKey: req.body.idempotencyKey,
...buildAuthorFilter(req, req.body.clientKey),
})
Note: [CWE-943] Improper Neutralization of Special Elements in Data Query Logic.
(nosql-injection-mongo-request-javascript)
📍 Affects 2 files
backend/controllers/interviewExperienceController.js#L104-L104(this comment)backend/tests/interviewExperienceController.unit.test.js#L230-L263
🤖 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 `@backend/controllers/interviewExperienceController.js` at line 104, The
duplicate-key recovery must reuse the validated, trimmed idempotency key and
author filter instead of the raw request value. In
backend/controllers/interviewExperienceController.js:104, retain those
normalized values from the initial validation and pass them to both lookup paths
in the duplicate-key branch. In
backend/tests/interviewExperienceController.unit.test.js:230-263, add a
whitespace-padded duplicate-key race case and assert both lookups use the
trimmed key.
| interviewExperienceSchema.index( | ||
| { idempotencyKey: 1 }, | ||
| { userId: 1, idempotencyKey: 1 }, | ||
| { | ||
| unique: true, | ||
| partialFilterExpression: { | ||
| userId: { $type: "objectId" }, | ||
| idempotencyKey: { $type: "string", $gt: "" }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| interviewExperienceSchema.index( | ||
| { clientKey: 1, idempotencyKey: 1 }, | ||
| { | ||
| unique: true, | ||
| partialFilterExpression: { | ||
| clientKey: { $type: "string", $gt: "" }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make authenticated and anonymous ownership scopes mutually exclusive.
An authenticated request can include a valid clientKey. The controller preserves that value. The document then matches both partial indexes and an anonymous lookup with the same clientKey and idempotencyKey can return the authenticated submission. A second authenticated user can also fail the clientKey unique index despite a different userId.
backend/models/InterviewExperience.js#L101-L117: apply the anonymous unique index only to ownerless documents, such as documents whereuserIdisnull.backend/controllers/interviewExperienceController.js#L79-L82: remove caller-suppliedclientKeyfor authenticated submissions, remove caller-supplieduserIdfor anonymous submissions, and constrain anonymous lookups to anonymous records.backend/tests/interviewExperienceController.unit.test.js#L207-L228: add coverage for an authenticated submission withclientKeyfollowed by an anonymous submission using that sameclientKeyandidempotencyKey.
📍 Affects 3 files
backend/models/InterviewExperience.js#L101-L117(this comment)backend/controllers/interviewExperienceController.js#L79-L82backend/tests/interviewExperienceController.unit.test.js#L207-L228
🤖 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 `@backend/models/InterviewExperience.js` around lines 101 - 117, Make
authenticated and anonymous ownership scopes mutually exclusive: in
backend/models/InterviewExperience.js lines 101-117, restrict the anonymous
clientKey/idempotencyKey unique index to documents with userId null; in
backend/controllers/interviewExperienceController.js lines 79-82, strip
caller-supplied clientKey for authenticated requests, strip caller-supplied
userId for anonymous requests, and limit anonymous lookups to ownerless records;
in backend/tests/interviewExperienceController.unit.test.js lines 207-228, add
coverage for an authenticated submission followed by an anonymous submission
reusing the same clientKey and idempotencyKey.
|
@ionfwsrijan Address coderabbit suggestions |
2 participants
Problem
POST /api/interview-experiencesdeduplicates submissions byidempotencyKeyalone, with no scope to the submitting user or client key. Anyone who re-uses a key gets the first submitter's full submission back (the unscopedfindOne({ idempotencyKey })plus the same flaw in the11000unique-index race branch). Because the model also enforces a global unique index onidempotencyKey, a different user re-using the same key can never persist their own fresh submission — their content is silently lost.Fix
userIdwhen authenticated,clientKeyfor anonymous submissions.idempotencyKeywith author-scoped partial unique indexes (userId + idempotencyKeyandclientKey + idempotencyKey), so a different author re-using the same key gets their own new submission instead of someone else's data.Files changed
backend/controllers/interviewExperienceController.js— author-scopedfindOnefilters in the pre-create check and the11000race branch.backend/models/InterviewExperience.js— author-scoped partial unique indexes.backend/tests/interviewExperienceController.unit.test.js— updated existing assertion and added coverage for authenticated scoping, cross-author key reuse, and the race branch.Testing
cd backend && npm test: all interview experience tests pass (16/16). The only failing tests in the full suite are the pre-existingjobCache.boundedKeysfailures that also fail on cleanorigin/mainand are unrelated to this change.Closes #1795
Summary
POST /api/interview-experiences.userId.clientKey.