feat(node-message-broker): container-facing message + participant endpoints#15
Conversation
… endpoints
Implement the SDK-compatible container API, verified against the legacy
node-message-broker server and the python-sdk client:
- POST /analyses/:id/messages send to recipient node ids (202, empty)
- POST /analyses/:id/messages/broadcast fan out to all participants (202, empty)
- GET /analyses/:id/participants bare array of { nodeId, nodeType }
- GET /analyses/:id/participants/self { nodeId, nodeType }, 404 if absent
`message` is an opaque JSON payload relayed verbatim — serialized, then sealed
per recipient under their public key and tagged with metadata.analysisId on the
Hub wire. `recipients` carry participant node ids; an unknown recipient is a 400.
Inbound delivery stays webhook-push (no pull endpoint), matching the contract.
Every analysis-scoped route is gated by the ANALYSIS_SELF_MESSAGE_BROKER_USE
capability (request permission checker) and an analysis-ownership check.
Also wire CryptoService into the components module and add the core send
orchestration (dispatchAnalysisMessage / broadcastAnalysisMessage) it builds on,
with core dispatch tests and controller tests against in-memory fakes.
|
Warning Review limit reached
More reviews will be available in 35 minutes and 48 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds analysis message send and broadcast endpoints, participant lookup endpoints, crypto-backed fan-out dispatch, new container wiring for crypto and core-client dependencies, and unit tests for dispatch and controller behavior. ChangesNode message broker analysis messaging
Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalysisMessageController
participant dispatchAnalysisMessage
participant IParticipantResolver
participant ICryptoService
participant IHubClient
Client->>AnalysisMessageController: POST /analyses/:id/messages
AnalysisMessageController->>dispatchAnalysisMessage: dispatchAnalysisMessage(...)
dispatchAnalysisMessage->>IParticipantResolver: resolve(analysisId)
dispatchAnalysisMessage->>ICryptoService: seal(data, recipientPublicKey)
dispatchAnalysisMessage->>IHubClient: send({ metadata.analysisId })
AnalysisMessageController-->>Client: 202 Accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
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 (2)
apps/node-message-broker/src/adapters/http/controllers/messages/module.ts (2)
138-162: 🔒 Security & Privacy | 🟠 MajorSubscription CRUD endpoints are missing the
authorizegate.
subscribe,listSubscriptions, andunsubscribeapply onlyForceLoggedInMiddlewareand never callawait this.authorize(event, analysisId), unlikesend,broadcast,listParticipants, andgetSelfParticipant. This allows any authenticated client to register, enumerate, or delete webhook subscriptions for anyanalysisIdthey do not own (IDOR) and bypasses theANALYSIS_SELF_MESSAGE_BROKER_USEcapability check. This also contradicts the class-level documentation stating that all analysis-scoped routes are gated byauthorize.Add
await this.authorize(event, analysisId)at the start ofsubscribe,listSubscriptions, andunsubscribe.🤖 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 `@apps/node-message-broker/src/adapters/http/controllers/messages/module.ts` around lines 138 - 162, The subscription endpoints are missing the same analysis authorization gate used by other routes in this controller. Update the MessageBrokerController methods subscribe, listSubscriptions, and unsubscribe to call await this.authorize(event, analysisId) at the start, matching send, broadcast, listParticipants, and getSelfParticipant so analysis-scoped webhook CRUD is properly restricted.
198-203: 🔒 Security & Privacy | 🟡 MinorValidate
webhookUrlto prevent SSRF attacksThe
requireWebhookUrlmethod accepts any non-empty string, which is then passed directly tofetch()inapps/node-message-broker/src/adapters/delivery/memory.tswithout validation. This allows attackers to register internal URLs (e.g.,http://169.254.169.254,http://localhost) or non-HTTP schemes, leading to Server-Side Request Forgery (SSRF).Update
requireWebhookUrlto:
- Parse the URL using
new URL()to ensure validity.- Enforce
httporhttpsschemes only.- Reject private/internal IP addresses and localhost.
Example fix
protected requireWebhookUrl(data: WebhookSubscriptionBody): string { if (!data || typeof data.webhookUrl !== 'string' || data.webhookUrl.length === 0) { throw new BadRequestError('A webhookUrl is required.'); } let parsedUrl: URL; try { parsedUrl = new URL(data.webhookUrl); } catch { throw new BadRequestError('Invalid webhookUrl format.'); } if (!['http:', 'https:'].includes(parsedUrl.protocol)) { throw new BadRequestError('webhookUrl must use http or https scheme.'); } // Block private/internal IPs and localhost const ip = parsedUrl.hostname; if (ip === 'localhost' || ip.startsWith('127.') || ip === '169.254.169.254') { throw new BadRequestError('webhookUrl cannot point to internal hosts.'); } return data.webhookUrl; }🤖 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 `@apps/node-message-broker/src/adapters/http/controllers/messages/module.ts` around lines 198 - 203, Update requireWebhookUrl in the messages module so it no longer accepts arbitrary non-empty strings before being used by fetch() in memory delivery. Parse data.webhookUrl with new URL(), reject invalid URLs with BadRequestError, allow only http: and https: schemes, and block localhost plus private/internal IP targets before returning the URL. Use the requireWebhookUrl method name and WebhookSubscriptionBody to locate the validation logic.
🧹 Nitpick comments (3)
apps/node-message-broker/test/unit/adapters/http/messages-controller.spec.ts (2)
183-223: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd denied-path coverage for the other analysis-scoped routes.
This suite only proves capability/ownership enforcement on
send().broadcast(),listParticipants(), andgetSelfParticipant()are part of the same analysis-scoped surface, so a regression there could expose message fan-out or participant topology without tripping these tests.🤖 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 `@apps/node-message-broker/test/unit/adapters/http/messages-controller.spec.ts` around lines 183 - 223, Add denied-path tests for the other analysis-scoped controller methods, not just send(). In MessagesControllerSpec, extend the setup to cover unauthorized/ownership failures for broadcast(), listParticipants(), and getSelfParticipant(), asserting they reject or return the expected denial behavior when the caller lacks access to the analysis. Use the existing controller, resolver, and event setup to verify these routes enforce the same capability checks as send() and do not leak recipients or participant data.
218-223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the 404 contract explicitly.
This test only matches
/self participant/i, so a generic 400/500 with the same message would still pass. The contract here is specifically404, so the spec should assert the HTTP-specific error type or response status as well.🤖 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 `@apps/node-message-broker/test/unit/adapters/http/messages-controller.spec.ts` around lines 218 - 223, The getSelfParticipant spec currently only checks the error message, so it can pass for the wrong HTTP status. Update the test in messages-controller.spec.ts to assert the HTTP-specific 404 contract for getSelfParticipant, using the controller’s error type/response status in addition to the self participant message. Keep the check anchored on getSelfParticipant and the “no self participant exists” case so the intended 404 behavior is verified explicitly.apps/node-message-broker/src/core/messaging/dispatch.ts (1)
58-67: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffFan-out is not atomic — a single recipient failure leaves partial deliveries.
Promise.allrejects on the first failingcrypto.seal/hub.send, but the other in-flight sends still complete. The caller (controller) only returns202on full success, so a partial failure surfaces as an error to the container even though some recipients already received the message, with no record of which ids succeeded. For an at-least-once relay this is often acceptable, but consider documenting the semantics or usingPromise.allSettledto surface per-recipient outcomes.🤖 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 `@apps/node-message-broker/src/core/messaging/dispatch.ts` around lines 58 - 67, The fan-out in dispatch’s message send flow is using Promise.all, so one recipient failure can hide which recipients already succeeded while still causing the whole operation to reject. Update the dispatch logic around the recipients map in dispatch.ts to either use Promise.allSettled and return per-recipient outcomes (including which clientIds succeeded or failed) or explicitly document that partial deliveries are expected. Make sure the behavior is clear at the dispatch/send boundary and, if you keep failure-tolerant delivery, preserve enough result detail for the controller or caller to report partial success.
🤖 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 `@apps/node-message-broker/src/adapters/http/controllers/messages/module.ts`:
- Around line 138-162: The subscription endpoints are missing the same analysis
authorization gate used by other routes in this controller. Update the
MessageBrokerController methods subscribe, listSubscriptions, and unsubscribe to
call await this.authorize(event, analysisId) at the start, matching send,
broadcast, listParticipants, and getSelfParticipant so analysis-scoped webhook
CRUD is properly restricted.
- Around line 198-203: Update requireWebhookUrl in the messages module so it no
longer accepts arbitrary non-empty strings before being used by fetch() in
memory delivery. Parse data.webhookUrl with new URL(), reject invalid URLs with
BadRequestError, allow only http: and https: schemes, and block localhost plus
private/internal IP targets before returning the URL. Use the requireWebhookUrl
method name and WebhookSubscriptionBody to locate the validation logic.
---
Nitpick comments:
In `@apps/node-message-broker/src/core/messaging/dispatch.ts`:
- Around line 58-67: The fan-out in dispatch’s message send flow is using
Promise.all, so one recipient failure can hide which recipients already
succeeded while still causing the whole operation to reject. Update the dispatch
logic around the recipients map in dispatch.ts to either use Promise.allSettled
and return per-recipient outcomes (including which clientIds succeeded or
failed) or explicitly document that partial deliveries are expected. Make sure
the behavior is clear at the dispatch/send boundary and, if you keep
failure-tolerant delivery, preserve enough result detail for the controller or
caller to report partial success.
In
`@apps/node-message-broker/test/unit/adapters/http/messages-controller.spec.ts`:
- Around line 183-223: Add denied-path tests for the other analysis-scoped
controller methods, not just send(). In MessagesControllerSpec, extend the setup
to cover unauthorized/ownership failures for broadcast(), listParticipants(),
and getSelfParticipant(), asserting they reject or return the expected denial
behavior when the caller lacks access to the analysis. Use the existing
controller, resolver, and event setup to verify these routes enforce the same
capability checks as send() and do not leak recipients or participant data.
- Around line 218-223: The getSelfParticipant spec currently only checks the
error message, so it can pass for the wrong HTTP status. Update the test in
messages-controller.spec.ts to assert the HTTP-specific 404 contract for
getSelfParticipant, using the controller’s error type/response status in
addition to the self participant message. Keep the check anchored on
getSelfParticipant and the “no self participant exists” case so the intended 404
behavior is verified explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44ca2c89-5aa9-45e3-afac-f552d2c9cb5a
📒 Files selected for processing (14)
apps/node-message-broker/src/adapters/http/controllers/messages/module.tsapps/node-message-broker/src/adapters/http/controllers/messages/types.tsapps/node-message-broker/src/app/modules/components/constants.tsapps/node-message-broker/src/app/modules/components/module.tsapps/node-message-broker/src/app/modules/http/controller.tsapps/node-message-broker/src/app/modules/http/module.tsapps/node-message-broker/src/core/messaging/dispatch.tsapps/node-message-broker/src/core/messaging/index.tsapps/node-message-broker/src/core/messaging/types.tsapps/node-message-broker/test/unit/adapters/http/messages-controller.spec.tsapps/node-message-broker/test/unit/core/messaging/dispatch.spec.tsapps/node-message-broker/test/unit/core/messaging/fake-crypto-service.tsapps/node-message-broker/test/unit/core/messaging/fake-hub-client.tsapps/node-message-broker/test/unit/core/messaging/fake-participant-resolver.ts
… zod Replace the hand-rolled requireRecipients / requireMessage / requireWebhookUrl guards in the message controller with validup `Container` validators backed by zod schemas, matching the Hub's validation convention and the local config validator. - AnalysisMessageValidator: `recipients` (non-empty node-id array, gated to a `send` group so a broadcast validates `message` alone) and `message` (opaque non-null JSON — object, array, or scalar — relayed verbatim). Unknown top-level keys are stripped. - WebhookSubscriptionValidator: a required, absolute `webhookUrl`. Validation failures throw a ValidupError that the already-mounted error middleware renders as a 400 with issue details. Adds a focused validator spec alongside the controller tests.
1 participant
Summary
Implements the container-facing message + participant API for the node message-broker
(Plan 013 Track B, slice S5 — issue #8). The HTTP surface was verified by reading the
legacy
PrivateAIM/node-message-brokerserver and thePrivateAIM/python-sdk(
flamesdk) client, so the SDK needs no change.Endpoints
POST /analyses/:id/messages{ recipients: nodeId[], message: <JSON> }202, empty bodyPOST /analyses/:id/messages/broadcast{ message: <JSON> }202, empty bodyGET /analyses/:id/participants[{ nodeId, nodeType }]GET /analyses/:id/participants/self{ nodeId, nodeType },404if absent(Webhook-subscription CRUD was already present; inbound delivery stays webhook-push — there
is no pull endpoint in the contract.)
Behaviour
messageis an opaque JSON payload relayed verbatim — serialized, sealed perrecipient under their public key, and tagged with
metadata.analysisIdon the Hub wire.The SDK embeds its own
metaenvelope (id/sender/category/…) insidemessage, sothe broker never mints ids or wraps the payload.
recipientscarry participant node ids; an unknown recipient is rejected with400.nodeId/nodeType— the internalclientId/publicKeynever leak to containers (covered by a test).ANALYSIS_SELF_MESSAGE_BROKER_USEcapability (request permission checker) plus ananalysis-ownership check. This is stricter than the legacy server (which only required an
authenticated identity); it is the node-side analysis policy introduced in feat(node-message-broker): analysis authorization — scope binding + capability check #13, and the
analysis client satisfies it transparently.
Internals
CryptoServiceinto the components DI module (it had been unwired since S1).core/messaging/—dispatchAnalysisMessage/broadcastAnalysisMessage(resolve participants → seal per recipient → one Hubsendeach), kept port-only so it is testable against in-memory fakes.
Tests
67 unit tests pass (+4 core dispatch, +9 controller) against in-memory fakes of the
participant resolver, crypto service, hub client, and analysis-client lookup. Lint,
typecheck, and the tsdown build are clean.
Closes #8. Supersedes #7 (folded into the S3/S5 authorization design).
Summary by CodeRabbit
New Features
Bug Fixes