fix(server): remove unauthenticated GET /internal/connections#846
Merged
Conversation
GET /internal/connections was registered without auth middleware, letting any unauthenticated caller enumerate every chat connection in the system plus its agentId association — a tenant-boundary leak. The route had no internal callers in the codebase (the 'server-to-server connection listing' comment was aspirational), so the smallest safe fix is to remove the route outright rather than gate it. The authenticated /api/v1/connections endpoint already covers the legitimate listing use case. Updates the regression test in rest-api-hardening to assert 404 instead of the previous documented-200 gap. Fixes #687
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
7 tasks
buremba
added a commit
that referenced
this pull request
May 26, 2026
deliverToBotConnections fetched GET /api/internal/connections (removed in #846) and POST /api/v1/messaging/send (also gone), so it hit `if (!connRes.ok) return` and silently delivered nothing — notifications never reached Slack/Telegram for any notification type. Rewrite it to resolve active chat connections + their channel bindings from Postgres and post in-process via a new ChatInstanceManager.postNotificationToChannel (a one-shot outbound post, not an inbound message that triggers an agent run). Every app pod loads every active connection at boot, so the locally-held instance can post regardless of which pod fired the notification — correct under N>1 replicas, no cross-pod routing. Best-effort per connection. Together with the reaction `notifications.send` namespace, a watcher reaction digest now reaches the agent's bound Slack channel. - chat-instance-manager: new postNotificationToChannel + 2 unit tests. - notifications/service: DB+manager delivery; drop dead HTTP path + unused getLobuServiceToken import.
buremba
added a commit
that referenced
this pull request
May 26, 2026
deliverToBotConnections fetched GET /api/internal/connections (removed in #846) and POST /api/v1/messaging/send (also gone), so it hit `if (!connRes.ok) return` and silently delivered nothing — notifications never reached Slack/Telegram for any notification type. Rewrite it to resolve active chat connections + their channel bindings from Postgres and post in-process via a new ChatInstanceManager.postNotificationToChannel (a one-shot outbound post, not an inbound message that triggers an agent run). Every app pod loads every active connection at boot, so the locally-held instance can post regardless of which pod fired the notification — correct under N>1 replicas, no cross-pod routing. Best-effort per connection. Together with the reaction `notifications.send` namespace, a watcher reaction digest now reaches the agent's bound Slack channel. - chat-instance-manager: new postNotificationToChannel + 2 unit tests. - notifications/service: DB+manager delivery; drop dead HTTP path + unused getLobuServiceToken import.
buremba
added a commit
that referenced
this pull request
May 26, 2026
…#1064) * feat(reactions): notifications.send on the reaction ClientSDK Watcher reactions could persist knowledge but had no way to push a notification — so digests landed in the feed and never reached Slack. The legacy ReactionSDK had `notify`; PR #348 consolidated reactions onto the typed ClientSDK and carried `notify` over only as an agent-facing MCP tool, dropping it from the reaction surface. Restore it as a `notifications` namespace that reuses the existing `notify` tool handler -> createNotificationForUsers -> deliverToBotConnections, so a reaction can fan a digest out to the org's active bot connections (Slack/ Telegram) and the in-app inbox. `watcher_source` attributes the send to the firing window. - sandbox: new `notifications.send` namespace + method-metadata (write) + ClientSDK wiring; exposed on connector-sdk ReactionClient type. - examples/lobu-crm: funnel-digest + inbound-triage reactions now call client.notifications.send after persisting knowledge. - test: reaction script dispatching client.notifications.send through the isolate (run-script-runtime). * feat(connector-sdk): export NotificationsSendInput (pi review) * fix(notifications): repair bot-connection delivery (dead since #846) deliverToBotConnections fetched GET /api/internal/connections (removed in #846) and POST /api/v1/messaging/send (also gone), so it hit `if (!connRes.ok) return` and silently delivered nothing — notifications never reached Slack/Telegram for any notification type. Rewrite it to resolve active chat connections + their channel bindings from Postgres and post in-process via a new ChatInstanceManager.postNotificationToChannel (a one-shot outbound post, not an inbound message that triggers an agent run). Every app pod loads every active connection at boot, so the locally-held instance can post regardless of which pod fired the notification — correct under N>1 replicas, no cross-pod routing. Best-effort per connection. Together with the reaction `notifications.send` namespace, a watcher reaction digest now reaches the agent's bound Slack channel. - chat-instance-manager: new postNotificationToChannel + 2 unit tests. - notifications/service: DB+manager delivery; drop dead HTTP path + unused getLobuServiceToken import. * test(notifications): integration test for bot-delivery target resolution Extract resolveBotDeliveryTargets (connections JOIN channel bindings) and cover it against a real DB: bound channel resolved, no-binding/inactive omitted, bare channel id prefixed, connectionId filter honored. Addresses the pi review's tests_adequate flag for the bot fan-out path. Also trims the notifications namespace header comment. * fix(notifications): lazily start the connection before posting (multi-replica) pi review caught a real N>1 hole: postNotificationToChannel only read this pod's in-memory instance map. A connection created or restarted on another replica is found in Postgres but has no live instance locally, so the post was skipped. Call ensureConnectionRunning() first — idempotent when already running, won't revive a stopped connection — so any pod that fires the notification can deliver. Adds lazy-start + stopped-connection unit tests. * refactor(notifications): rename to postMessageToChannel + post markdown Rename postNotificationToChannel -> postMessageToChannel (it's a generic outbound-post primitive; 'notification' is the caller's concern) and post { markdown } instead of { raw } so digests render in each platform's native format (Slack markdown_text) rather than being HTML-escaped. * feat(reactions): portable rich cards via chat CardElement Let notifications carry a rich card, not just markdown: notifications.send / the notify tool accept an optional `card` typed as the `chat` CardElement (reused, not a bespoke DSL), and postMessageToChannel takes chat's AdapterPostableMessage and posts it directly — no converter. The Chat SDK renders one card to each platform's native format (Slack Block Kit, Teams Adaptive Cards, Google Chat Cards), so it's portable everywhere. connector-sdk re-exports the CardElement type (chat as optional peer/dev dep, type-only — no runtime bloat). Adds a manager test that builds a card with the real chat primitives and asserts it posts as { card }. * fix(connector-sdk): don't leak `chat` into published types (pi blocker) Importing chat's CardElement into connector-sdk's public .d.ts made an external typecheck fail (TS2307) unless the consumer also installed chat — and making chat a hard dep would bloat every connector install. Keep the real chat types server-side (where the card is validated + rendered) and type the author-facing `card` as a plain serializable object. Drop the chat dep from connector-sdk; remove the unused Section import in the card test. * docs(notifications): note the ORDER BY binding-creation rationale (review nit)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /internal/connections, registered bycreateConnectionCrudRoutesinpackages/server/src/gateway/routes/public/connections.ts, had no auth middleware. Any unauthenticated caller could enumerate every chat connection in the system plus itsagentIdassociation — a tenant boundary leak.A repo-wide search turned up zero internal callers of the route — the inline
// Internal endpoint for server-to-server connection listing (no auth required)comment was aspirational. The authenticated/api/v1/connectionsendpoint (registered immediately below it with full session + per-agent access checks) already covers the legitimate listing use case.Per the three options listed in #687, the smallest safe fix is option 3 — remove the route outright. Adding admin/service-token auth or localhost gating would be dead weight for code nothing calls.
The PR #684 regression test in
rest-api-hardening.test.tsis flipped from documenting the broken 200 to asserting the post-fix 404.Fixes #687
Reproducer
Booted
packages/server/dist/start-local.bundle.mjslocally (Node 22, PGlite, ephemeral keys) on:9091against both the pre-fix and post-fix bundles.Pre-fix (build from
mainwith the route still registered):GET http://127.0.0.1:9091/internal/connectionsresolves to thelistAllConnectionshandler in this start-local boot only when achatInstanceManageris provided (seegateway.tsline 682 mounting guard). The vulnerable surface is reachable in any deployment that has at least one chat connection configured — i.e. every real production gateway, since chat platforms are why the gateway exists.Post-fix:
curl -i http://127.0.0.1:9091/internal/connectionsreturnsHTTP 404 Not Found. Bundle no longer containslistAllConnections(grep -c listAllConnections packages/server/dist/*.bundle.mjs→ 0).The unit-level reproducer is the test in
rest-api-hardening.test.ts, which constructscreateConnectionCrudRoutesdirectly and hits/internal/connectionswith no session: pre-fix200+ JSON{connections: [...]}, post-fix404.Test plan
make build-packagescleanlistAllConnections