Repository navigation
Conversation
Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| req.on('close', () => sseClients.delete(res)); | ||
| // Account-scoped streams share the same owner check as message reads/sends. | ||
| const conversationStreams = require('./lib/conversationStreams'); | ||
| app.get('/events', chatAuth, (_req, res) => { |
| const payload = `event: ${event}\ndata: ${JSON.stringify(data)}\n\n`; | ||
| clients.forEach(res => res.write(payload)); | ||
| } | ||
| app.get('/stream/:conversationId', chatAuth, asyncHandler(conversationStreams.open)); |
| // 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 for this, Nirmal. Locking /stream and /events behind an account token plus conversation ownership closes a real hole: on main, anyone with a conversation ID can read its live chat. Registering the stream before the ownership read, revoking even pending streams on delete, closing on token expiry, and using the same 404 for foreign, legacy and missing chats are all careful work. The tests are strong: they caught 31 of the 40 bugs we planted.
What I tested:
- 5154bdf via merge ref cfc4f03, plus a fresh merge onto current main 7524c48. That merge has the same code as your new head 4571d3a; only the order of two CONFORMANCE.md sections differs.
- Results: backend 37/37 node --test and 73/73 Jest, chat-ui 13/13, vite build, frontend lint, test:planner and next build all pass. The two new backend test files also passed 8 more reruns.
Requesting changes because of one merge blocker:
- The CodeQL gate fails with 5 high js/missing-rate-limiting alerts: server.js:67, :70 and :83, conversationRoutes.js:8 and communicationRoutes.js:8. Main's ruleset blocks merges on these. Two are real:
- /uploads now scans the whole Messages table on every request (message.js:69-84).
- /stream has no cap on open streams per user.
Please add express-rate-limit after chatAuth, keyed by req.user.userId, at least on /uploads and /stream. Cap open streams per user in conversationStreams.open (429 above about 5, decremented in cleanup()), and drop chatAuth from the 410 /events route. The alerts that moved over from main (#5, #15, #16) can be rate-limited too or dismissed by a maintainer with a reason.
Should fix (fine as follow-ups):
-
Attachment lookup: serve files at /conversations/:id/attachments/:name behind conversationOwner and Query that one partition, or add a filePath GSI.
-
Sign-in state: nothing calls setAccountToken yet. Please show a visible "sign in" state on 401 in Sidebar.jsx:29-36 and ChatWindow instead of only logging.
Nits:
-
Merge order: main only allows squash or rebase merges, so once #191 lands this branch will conflict in CONFORMANCE.md and conversation-ownership.md. Please rebase then so the PR holds only the stream commit. While you're there, rename "A1/A2" to #191/#192 in authenticated-streams.md:3, :44 and conversation-ownership.md:47-48.
-
Flaky test: conversationStreams.test.js:29 can fail near a second boundary. Sign with exp = Math.ceil(Date.now()/1000) + 1 and raise the 1500 ms timeout above 2 s.
-
Attachment names with %, # or ? can't be downloaded (accountClient.mjs:43-50). Validate the raw name and request '/uploads/' + encodeURIComponent(name).
-
A frame over 1 MiB makes the client reconnect every second. Treat it as a terminal error instead.
-
Older issues for separate tickets, not this PR:
- Deleting a chat with more than 25 messages leaves them behind (message.js:43-63).
- Each token and message is sent twice on the same stream (communication_controller.js:13-29).
- The ownership script isn't run in CI yet.
I'll re-test once the rebased head is up.
Apply account request budgets and cap pending plus active streams at five per account. Release capacity on expiry, disconnect and deletion, and return 410 for the retired global feed without authentication. Carry shared ownership and attachment fixes. Stop oversized-frame retries while preserving valid frames coalesced in one network chunk. Add regression coverage and update deployment documentation.
094bc9a to
fc1c498
Compare
What the change is
Protects live chat updates with account authentication and conversation ownership checks. Removes the unrestricted
/eventsfeed.Why it is needed
The current stream opens before authentication. Someone who knows a conversation ID can subscribe to its private updates.
How the change works
The server checks the account token and owner before opening the stream. It closes streams when the token expires, the client disconnects, or the conversation is deleted locally.
The browser uses streaming fetch to send the token in an Authorization header. It stops the previous stream on account changes and stops retrying when authorization fails. Tokens stay out of URLs and browser storage.
This PR depends on #191 and currently includes its changes. Review #191 first and deploy both together. The host must supply a login token. Stream invalidation is local to one server process; this does not add cross-server revocation.
Screenshot of the actual UI after a live message arrives. Demo responses are synthetic; the stream request was checked for the account Authorization header.
Where it is involved
Agrinet's
/stream/:conversationIdendpoint,backend/lib/conversationStreams.js, conversation deletion, andfrontend/chat-uistreaming client, chat window, and sidebar. Stream behavior and limits.Verified locally: 37 Node tests, 73 Jest tests, 13 chat UI tests, the UI build, and DynamoDB ownership checks passed. Coverage includes access denial, expiry, fragmented events, account changes, and deletion races. Hosted workflows await maintainer approval.