🪪 fix: Scope Message Conversation Access#13183
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Fixes a cross-user message injection/IDOR path by ensuring message persistence and retrieval are consistently scoped to the authenticated user and to a single, validated conversationId (preferring the URL param and rejecting mismatches).
Changes:
- Enforce
conversationIdconsistency across URL/body/nested payload invalidateMessageReqand persist the validated URLconversationIdon POST. - Scope message reads (routes, BaseClient history load, thread gap sync) by authenticated user.
- Scope
saveConvomessage aggregation (getMessages) by user and add/adjust regression tests for the above behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/methods/conversation.ts | Scope saveConvo message aggregation query by user. |
| packages/data-schemas/src/methods/conversation.spec.ts | Update expectations to assert user-scoped getMessages query. |
| api/server/services/Threads/manage.js | Scope thread gap DB message reads by authenticated user. |
| api/server/routes/messages.js | Scope message reads by user; force POST persistence to use URL conversationId. |
| api/server/routes/tests/messages-delete.spec.js | Add route-level regression tests for POST persistence and user-scoped reads. |
| api/server/middleware/validateMessageReq.js | Reject mismatched conversationId values (URL/body/nested) before DB access. |
| api/server/middleware/tests/validateMessageReq.spec.js | Add tests for mismatch rejection and ownership validation path. |
| api/app/clients/BaseClient.js | Scope loadHistory DB reads by this.user. |
| api/app/clients/specs/BaseClient.test.js | Add regression test asserting loadHistory uses user-scoped reads. |
Comments suppressed due to low confidence (1)
api/server/routes/messages.js:317
db.getMessages()returns an array; when no record matches, it returns[]which is truthy, soif (!message)won’t trigger and the route responds200with an empty array. Consider selecting the first result (e.g.,const message = (await db.getMessages(...))?.[0];) and checking for absence (if (!message)) so missing/unauthorized messages correctly return404and the response shape is consistent.
const { conversationId, messageId } = req.params;
const message = await db.getMessages(
{ conversationId, messageId, user: req.user.id },
'-_id -__v -user',
);
if (!message) {
return res.status(404).json({ error: 'Message not found' });
}
res.status(200).json(message);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
GitNexus: 🚀 deployedThe |
* fix: Scope message conversation access * style: Format message route query
* fix: Scope message conversation access * style: Format message route query
* fix: Scope message conversation access * style: Format message route query
I fixed a cross-user message injection path by making conversation ownership checks and message persistence use the same validated conversation ID.
conversationIdvalues before the message route reaches database writes.POST /api/messages/:conversationIdto persist the validated URLconversationIdinstead of trusting the request body.saveConvomessage aggregation.Change Type
Testing
npm ci --ignore-scripts --no-audit --no-fundto install dependencies for this worktree.npm run build:data-provider,npm --workspace packages/data-schemas run build, andnpm --workspace packages/api run buildso local workspace package entrypoints resolve during tests.npm --workspace api test -- server/middleware/__tests__/validateMessageReq.spec.js server/routes/__tests__/messages-delete.spec.js app/clients/specs/BaseClient.test.js --runInBand.npm --workspace packages/data-schemas run test:ci -- src/methods/conversation.spec.ts --runInBand.Test Configuration:
mongodb-memory-servervia targeted Jest suitesChecklist