Repository navigation
Conversation
Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Middleware | ||
| app.use(authMiddleware); | ||
| app.use('/uploads', express.static(path.join(__dirname, 'uploads'))); | ||
| app.use('/uploads', chatAuth, asyncHandler(require('./middleware/attachmentOwner')), express.static(path.join(__dirname, 'uploads'), { |
Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
NetworkTheoryAppliedResearchInstitute
left a comment
NetworkTheoryAppliedResearchInstitute
left a comment
There was a problem hiding this comment.
Thanks, @nirmalhk7. This closes a real hole. On main, anyone with the shared browser key could read, rename or delete any conversation and download any upload by its ID. The design is solid:
- the owner comes only from the verified JWT
- DynamoDB conditions act as a second check
- foreign, legacy and missing IDs all get the same 404
- uploads are served as downloads
The tests are strong, and the docs are candid about the limits.
What I tested
I tested eae4e15 on GitHub's merge ref (on cf9299f) and on a local merge into current main (7524c48). Your new head, 583b23b, has the same code: compared with my merge, only the order of the CONFORMANCE.md notes differs. All of these pass:
- backend: 37 node --test and 60/60 Jest
- chat-ui: 10/10 and the vite build
- Next frontend: lint, test:planner and next build
I also planted 27 regressions, and the tests caught 22.
Blocking
-
The required CodeQL check fails with 3 high js/missing-rate-limiting alerts, and the main ruleset blocks merges on high alerts:
- #16 backend/server.js:135
- #15 backend/routes/communicationRoutes.js:8
- #5 backend/routes/conversationRoutes.js:8
#5 and #15 already exist on main. #16 is new and worth fixing: every GET /uploads/ now runs a full Scan of the Messages table (attachmentOwner.js:10 → message.js:69-84). Fix: add express-rate-limit, keyed on req.user.userId with an IP fallback, to the /conversations, /messages and /uploads mounts. That clears all three alerts. An index on file.path can follow later.
Non-blocking (fine as follow-ups)
- backend/models/message.js:29-36: upload names are Date.now() + '_' + the file name, and access now depends on that name being unique. Two same-name uploads in one millisecond overwrite each other, and both owners can download the result. Use randomUUID() + a safe extension, keep the original name in the message, and write with the 'wx' flag. Bonus: this also closes path-traversal alert #13 at line 31.
- conversationAccess.test.js: add tests that an unreferenced upload and an upload from a deleted conversation both return 404. Optionally, add '/uploads/%2e%2e', '/uploads/..%5cx' and request('GET','//evil.example/x') cases to accountClient.test.js.
- frontend/chat-ui/src/accountClient.mjs:4-9: setting the same token again still clears chat state and rejects in-flight requests. Return early if the token hasn't changed.
- conversation.js:66-80 and message.js:54-63 (pre-existing): deleting a chat with more than 25 messages fails because of the batchWrite limit. If you touch this, delete the messages before the conversation row so a failed delete can be retried.
- conversation-ownership.md: a read-only count of ownerless rows, plus one sentence saying legacy rows were a shared pool, would help operators.
- Nothing in chat-ui calls setAccountToken yet, so chat calls return 401 with only a console error. A 'sign in to use chat' state is a good next step, done together with a CORS fix.
Note for maintainers
Merge this with #192 (or merge #192 alone, since it contains this commit) with no deploy in between, because /stream is still open. The CI and CodeQL runs on 583b23b are waiting for approval to run.
Thanks again for the careful work.
…ersation-ownership Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com> # Conflicts: # CONFORMANCE.md # backend/models/message.js
Rate-limit private chat routes by verified account. Use exclusive UUID upload names and query owned conversation partitions for downloads. Preserve state for unchanged tokens and show sign-in guidance on 401. Add regression coverage and document legacy data checks and rollout.
6693301 to
bc2f387
Compare
What the change is
Restricts private conversations, messages, and attachment downloads to the account that owns them. Switching accounts clears the previous account's chat history, draft, and selected file.
Why it is needed
An authenticated caller can currently access another member's chat by supplying its ID. Shared API keys also do not identify an individual owner, and browser state can carry private content across account changes.
How the change works
The backend verifies the account token, records ownership when a conversation is created, and checks it on reads and changes. Shared keys alone cannot access private chats. Legacy chats without an owner stay inaccessible until an operator explicitly assigns the rightful owner.
The browser sends account tokens in headers, downloads attachments through authenticated requests, and resets private state when the account changes. The host application must supply the login token; this does not add a login screen.
Deploy together with #192, which closes the public live-stream path.
Screenshots use synthetic demo responses in this PR's actual UI.
Account A's history and unsent draft:
After switching to account B, A's history and draft are cleared:
Where it is involved
Agrinet's conversation/message routes, ownership middleware, DynamoDB models, attachment serving, and
frontend/chat-uiaccount client, store, and attachment component. Ownership and rollout guide.Verified locally: 37 Node tests, 60 Jest tests, 10 chat UI tests, the UI build, and DynamoDB ownership checks passed. Existing account/federation hardening and large-history deletion remain separate work. Hosted workflows await maintainer approval.