broker: add Slack message broker — Phase 1 (Cloudflare Worker)#65
broker: add Slack message broker — Phase 1 (Cloudflare Worker)#65benvinegar merged 3 commits intomainfrom
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR implements Phase 1 of the Slack broker — a self-contained Cloudflare Worker that routes messages between Slack workspaces and individual baudbot servers with end-to-end encryption. Key Changes:
Security Highlights:
Minor Issue:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Slack
participant Broker as Cloudflare Worker<br/>(Slack Broker)
participant Server as Baudbot Server
Note over User,Server: OAuth Installation Flow
User->>Broker: GET /slack/oauth/install
Broker->>Slack: Redirect to OAuth authorize
Slack->>User: Authorization page
User->>Slack: Approve app
Slack->>Broker: GET /slack/oauth/callback?code=...
Broker->>Slack: Exchange code for bot token
Slack-->>Broker: bot_token + team_id
Broker->>Broker: Generate auth_code, hash with SHA-256
Broker-->>User: Display auth_code (for server setup)
Note over User,Server: Server Registration
Server->>Broker: POST /api/register<br/>(workspace_id, server_pubkey, auth_code)
Broker->>Broker: Verify auth_code hash
Broker->>Broker: Activate workspace (status: pending → active)
Broker-->>Server: broker_pubkey, broker_signing_pubkey
Note over Slack,Server: Inbound: Slack → Server (Sealed Box)
Slack->>Broker: POST /slack/events<br/>(event payload, HMAC signature)
Broker->>Broker: Verify HMAC-SHA256 signature
Broker->>Broker: Encrypt with crypto_box_seal<br/>(server_pubkey, ephemeral keypair)
Broker->>Broker: Sign envelope with Ed25519
Broker->>Server: POST callback_url<br/>(encrypted event + signature)
Server->>Server: Verify broker signature
Server->>Server: Decrypt with crypto_box_seal_open<br/>(ONLY server can decrypt)
Note over Server,Slack: Outbound: Server → Slack (Authenticated Box)
Server->>Server: Encrypt body with crypto_box<br/>(broker_pubkey, server_secretkey)
Server->>Server: Sign request with Ed25519
Server->>Broker: POST /api/send<br/>(workspace_id, encrypted_body, signature)
Broker->>Broker: Verify server signature
Broker->>Broker: Decrypt body transiently
Broker->>Slack: POST chat.postMessage<br/>(plaintext message)
Broker->>Broker: Zero plaintext from memory
Broker-->>Server: Success response
Last reviewed commit: ec1b3b1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
slack-broker/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "tweetnacl": "^1.0.3", | ||
| "tweetnacl-util": "^0.15.1" |
There was a problem hiding this comment.
tweetnacl-util is listed as a dependency but not imported anywhere in the codebase
| "tweetnacl-util": "^0.15.1" | |
| "tweetnacl": "^1.0.3" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: slack-broker/package.json
Line: 15
Comment:
`tweetnacl-util` is listed as a dependency but not imported anywhere in the codebase
```suggestion
"tweetnacl": "^1.0.3"
```
How can I resolve this? If you propose a fix, please make it concise.![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
baudbot-agent
left a comment
baudbot-agent
left a comment
There was a problem hiding this comment.
Fresh-Eyes Code Review: slack-broker/
Reviewed the entire slack-broker/ Cloudflare Worker — crypto, API endpoints, Slack integration, routing, and test suite. All 54 tests pass. Overall this is solid, well-structured code with good security fundamentals. Below are my findings.
🔴 CRITICAL
1. Registration allows server hijacking — auth_code_hash not cleared after use
File: src/api/register.ts (lines 93-107) + src/routing/registry.ts
After successful registration, activateWorkspace() does NOT clear the auth_code_hash from the workspace record. This means:
- An attacker who obtains the auth code (e.g., user shares it insecurely, shoulder surfing, clipboard history) can re-register at any time — even after the legitimate server is active — replacing
server_pubkey,server_signing_pubkey, andserver_callback_url. - The legitimate server is silently disconnected with no notification.
- All inbound events now flow to the attacker's server encrypted with the attacker's public key.
The auth code is a one-time credential that should be invalidated on first use.
Fix: Clear auth_code_hash (or set to empty string) after successful activation in activateWorkspace(). Also consider rejecting registration when the workspace is already active unless the current server explicitly unregisters first.
2. Re-OAuth overwrites active workspace — bot token and auth_code replaced
File: src/slack/oauth.ts (line 138) → src/routing/registry.ts createPendingWorkspace()
If someone completes the OAuth flow for a workspace that already has status: "active", createPendingWorkspace() unconditionally overwrites the entire record — including bot_token, auth_code_hash, and crucially resetting status to "pending". This:
- Breaks the active server's connection (events stop forwarding).
- Replaces the bot token, potentially with a token from a different Slack app install.
- Gives the new OAuth completer a fresh auth_code to register their own server.
Any Slack workspace admin (not just the one who originally installed) can trigger this.
Fix: Check if the workspace is already active in the OAuth callback. If so, either reject the re-install or require the current server to unregister first. At minimum, preserve the existing server linkage.
3. Sealed box nonce derivation is incompatible with libsodium
File: src/crypto/seal.ts (lines 28-34)
The nonce is derived using SHA-512(ephemeral_pk || recipient_pk) truncated to 24 bytes. The actual libsodium crypto_box_seal uses BLAKE2B(ephemeral_pk || recipient_pk) (specifically crypto_generichash with 24-byte output).
This means:
- A baudbot server using actual libsodium (e.g.,
libsodium.js, Python's PyNaCl, Go'sgolang.org/x/crypto/nacl) to callcrypto_box_seal_openwill fail to decrypt messages from this broker. - The spec says "All encryption uses libsodium" but the implementation is not interoperable.
This isn't a security flaw per se (SHA-512 truncated to 24 bytes is a fine nonce derivation), but it's a correctness bug that will cause real-world failures when the server-side client is implemented.
Fix: Either:
- (a) Use BLAKE2B for nonce derivation to match libsodium (requires adding a BLAKE2B implementation — not in tweetnacl), or
- (b) Document prominently that this is a custom sealed-box variant, NOT libsodium-compatible
crypto_box_seal, and ensure the server client uses the same SHA-512 derivation. Update doc comments and README claims accordingly.
I'd recommend (a) for interoperability. Consider using libsodium-wrappers-sumo which works in Workers and provides crypto_box_seal directly.
🟡 IMPORTANT
4. Canonicalization delimiter injection — pipe character not validated
Files: src/crypto/verify.ts (lines 63-67, 76-80)
The canonical signing format uses | as a delimiter:
"workspace_id|timestamp|encrypted_payload_base64"
"workspace_id|action|timestamp|encrypted_body_base64"
Neither workspace_id nor action are validated to exclude |. A malicious workspace_id like T123|chat.postMessage|0| could forge a canonical form that collides with a different legitimate request, potentially allowing signature reuse across different contexts.
Slack team IDs don't contain |, so this is more of a defense-in-depth issue — but the register endpoint doesn't validate workspace_id format.
Fix: Validate that workspace_id matches the Slack team ID format (/^T[A-Z0-9]+$/) at registration time. Or use a length-prefixed encoding instead of delimiters.
5. hashAuthCode uses unsalted SHA-256 — vulnerable to precomputation
File: src/routing/registry.ts (lines 116-122)
The auth code hash is SHA-256(authCode) with no salt. Auth codes are 64 hex chars (32 bytes of entropy), so rainbow tables aren't practical. However, this is a bad pattern:
- If KV data is ever exposed (breach, admin access, Cloudflare incident), the unsalted hash allows offline brute-force.
- More practically: it allows timing-based confirmation — an attacker who knows a candidate auth code can hash it and compare against the stored hash (if they gain KV read access).
The 256-bit entropy of the auth code makes this academic rather than exploitable, but using HMAC-SHA256 with a secret as the key (e.g., HMAC(SLACK_SIGNING_SECRET, authCode)) would be trivial and strictly better.
Fix: Use HMAC-SHA256 keyed with a broker secret instead of bare SHA-256.
6. Bot token stored in plaintext in KV
File: src/routing/registry.ts (lines 65, 80)
The comment says "Cloudflare KV is encrypted at rest" which is true, but this only protects against physical disk access at Cloudflare's data centers. The bot token is readable in plaintext by:
- Anyone with Cloudflare dashboard access
wrangler kv:key getCLI- Any Worker code with the KV binding
The spec mentions "bot_token (encrypted at rest in KV)" but the implementation stores it as a plaintext JSON field. For the security model this broker advertises, the token should be encrypted with the broker's key before storage.
Fix: Encrypt the bot_token with nacl.secretbox using a key derived from BROKER_PRIVATE_KEY before storing in KV. Decrypt on read in getWorkspace().
7. forwardEvent does not validate server_url is HTTPS
File: src/routing/forward.ts
The register endpoint validates that server_callback_url uses HTTPS (in register.ts line 82), which is good. However, forwardEvent does NOT re-validate this. If the URL in KV is somehow modified (KV tampering, race condition during update), the broker would send the encrypted envelope over plain HTTP.
This is defense-in-depth since KV tampering would be a much larger issue, but a one-line check is cheap insurance.
Fix: Add if (!workspace.server_url.startsWith('https://')) return { ok: false, error: "invalid server URL" }; early in forwardEvent.
8. No rate limiting on any endpoint
Files: src/index.ts, src/api/register.ts, src/api/send.ts
There is zero rate limiting. The /api/register endpoint is particularly sensitive — an attacker can brute-force auth codes (64 hex chars makes this infeasible, but the pattern is concerning). The /api/send endpoint could be abused to spam Slack channels if a server's signing key is compromised.
The spec lists rate limiting as Phase 3, which is fine for MVP, but it should be noted as a pre-production requirement.
9. zeroBytes is ineffective in JavaScript
File: src/crypto/box.ts (lines 73-75)
arr.fill(0) does zero the Uint8Array contents, but JavaScript/V8's garbage collector may have already copied the data elsewhere in memory (e.g., during JSON.parse, string operations, V8 internal buffers). The decodeUTF8(decryptedBytes) on line 131 of send.ts creates a JavaScript string which is immutable and cannot be zeroed. The JSON.parse on line 136 creates more string copies.
This isn't fixable in JavaScript — it's a fundamental limitation. The zeroBytes call is still worth keeping as a best-effort measure, but the security claim "zeroes the plaintext from memory immediately after posting" is misleading. The plaintext string from JSON.parse will persist until GC collects it.
Fix: Update documentation and comments to say "best-effort memory cleanup" rather than implying deterministic zeroing. Consider noting this limitation in the README's security properties section.
🟢 MINOR
10. X25519 and Ed25519 derived from same seed
File: src/index.ts (lines 69-70)
Using the same 32-byte BROKER_PRIVATE_KEY as both an X25519 secret key and an Ed25519 seed is mathematically safe (the derived public keys will differ, and the algorithms operate on different curves), but it's an unusual pattern that makes key management more fragile. If the seed is compromised, both encryption and signing are compromised simultaneously.
Fix: Consider using two separate secrets, or at minimum document this design choice prominently.
11. Missing test coverage for several edge cases
Files: test/*.test.ts
Gaps I noticed:
- No tests for
handleSlackEvent(the main event handler function) — onlyverifySlackSignatureis tested directly - No tests for
handleSendorhandleRegister(the HTTP handler functions) — only the underlying crypto/registry functions - No tests for OAuth flow (handleOAuthInstall, handleOAuthCallback)
- No test that the
url_verificationchallenge response works correctly - No negative test for
sealedBoxDecryptwith wrong public key but correct private key (tests wrong keypair, but not mismatched pk/sk) - No test that
boxEncrypt→boxDecryptround-trip works across different process boundaries (simulating server→broker)
The tests are good unit tests for the crypto and registry layers, but there are no handler-level integration tests.
12. callSlackApi does not validate the method parameter
File: src/slack/api.ts (line 86)
The Slack API method name is interpolated directly into a URL: `https://slack.com/api/${method}`. While this is only called from executeSlackAction which validates the action, the function itself could be misused if called directly.
Fix: Validate method against an allowlist, or make the function non-exported (it's already module-private via callSlackApi not being exported, so this is fine as-is — disregard).
13. Missing Content-Type validation on POST endpoints
Files: src/api/register.ts, src/api/send.ts, src/slack/events.ts
POST endpoints call request.json() without checking Content-Type. Not a security issue since request.json() will throw on non-JSON regardless, but adding a Content-Type check produces clearer error messages.
14. OAuth redirect_uri derived from request.url
File: src/slack/oauth.ts (lines 58-59, 112)
The redirect_uri is derived from request.url, which in Workers comes from the incoming request's Host header. An attacker could manipulate the Host header to point the redirect to a different origin. In Cloudflare Workers with custom domains, the Host header is validated by Cloudflare, so this is safe in practice. But if deployed behind a proxy that allows host spoofing, it could be exploited.
Fix: Consider hardcoding the base URL or deriving it from a config variable rather than the request.
ℹ️ NOTES
15. Slack event retries not deduplicated
Slack retries events after 3 seconds if it doesn't get a 200 response in time. The broker ACKs immediately and forwards via waitUntil, so retries are unlikely. But if the Worker cold-starts slowly or KV is slow, Slack might retry. The server will receive duplicate encrypted events. This is expected behavior (spec says "no queuing") but worth documenting for server implementers.
16. setTimeout in Workers
src/routing/forward.ts uses setTimeout + AbortController for fetch timeout. This works in Cloudflare Workers, but AbortSignal.timeout(10_000) (available since compatibility_date 2024-12-30) would be cleaner and avoids the need for manual cleanup.
17. KV namespace interface re-declared
The KVNamespace interface is declared both in src/index.ts and src/routing/registry.ts. These should share a single type definition.
18. Spec deviation: /api/heartbeat and /api/react and /api/update not implemented
The spec lists POST /api/heartbeat, POST /api/react, and POST /api/update as separate endpoints. The implementation handles reactions.add and chat.update through /api/send with different action values — which is cleaner. Heartbeat is not implemented. These are likely Phase 2/3 items but should be called out.
Summary
| Severity | Count |
|---|---|
| 🔴 CRITICAL | 3 |
| 🟡 IMPORTANT | 6 |
| 🟢 MINOR | 5 |
| ℹ️ NOTE | 4 |
Must fix before merge: #1 (auth_code reuse), #2 (re-OAuth overwrites active workspace), #3 (sealed box incompatibility).
Strong recommendations: #4 (delimiter injection), #6 (bot token encryption), #9 (document zeroBytes limitations).
The crypto primitives are correctly used, the architecture matches the spec well, and the code quality is high. The main gaps are in the registration/lifecycle security rather than the cryptographic layer.
Security Review Fixes — All 9 Issues Addressed🔴 Critical (3/3)1. Auth code reuse allows server hijacking ✅
2. Re-OAuth overwrites active workspace ✅
3. Sealed box nonce uses SHA-512 instead of BLAKE2B ✅
🟡 Important (6/6)4. Pipe delimiter injection in canonicalization ✅
5. hashAuthCode uses unsalted SHA-256 ✅
6. Bot token stored in plaintext in KV ✅
7. forwardEvent doesn't re-validate HTTPS ✅
8. Rate limiting noted as missing (Phase 3) ✅
9. zeroBytes is misleading in docs ✅
Tests
Files Changed13 files, +298 / -85 lines |
Self-contained Cloudflare Worker at slack-broker/ that routes messages between Slack workspaces and individual baudbot servers with end-to-end encryption. Crypto layer: - crypto_box_seal (sealed boxes) for inbound Slack→server path - crypto_box (authenticated encryption) for outbound server→Slack path - Ed25519 signatures on all envelopes for authentication - All primitives via tweetnacl (lightweight, no native deps, Workers-compatible) Routing: - KV-backed workspace registry (workspace_id → server config) - Sealed box encrypt + forward to server callback URL - 10s timeout, graceful error handling, no message queuing Slack integration: - Events API handler with HMAC-SHA256 signature verification - 5-minute replay protection on all timestamps - OAuth install flow with state parameter + auth code generation - Slack Web API helpers (postMessage, reactions.add, chat.update) API endpoints: - POST /api/register — server registration with auth code verification - DELETE /api/register — server unlink with signature verification - POST /api/send — outbound encrypted message → decrypt → post to Slack - GET /api/broker-pubkey — public key distribution Security: - Broker CANNOT decrypt inbound messages (sealed box) - Outbound plaintext zeroed immediately after posting to Slack - Auth codes hashed with SHA-256 before storage - Callback URLs must use HTTPS - Constant-time signature comparison Tests: 54 tests across 3 suites (crypto, routing, integration) Docs: AGENTS.md and architecture.md updated with slack-broker layout
Addresses review feedback — encoding utilities are implemented in src/util/encoding.ts, tweetnacl-util was never imported.
…to, token encryption Critical fixes: 1. Auth code reuse: clear auth_code_hash after activation, reject re-registration of already-active workspaces (409 Conflict) 2. Re-OAuth overwrites active workspace: check workspace status in OAuth callback, reject re-install when server is active 3. Sealed box nonce: switch from SHA-512 to libsodium-wrappers-sumo with native crypto_box_seal (BLAKE2B nonce, spec-compliant) Important fixes: 4. Pipe delimiter injection: validate workspace_id matches /^T[A-Z0-9]+$/ 5. Unsalted hash: switch hashAuthCode from SHA-256 to HMAC-SHA256 keyed with BROKER_PRIVATE_KEY 6. Bot token plaintext in KV: encrypt with nacl.secretbox using key derived from BROKER_PRIVATE_KEY, decrypt on read 7. HTTPS enforcement: add protocol check in forwardEvent() 8. Rate limiting: add TODO comment noting Phase 3 requirement 9. zeroBytes docs: clarify best-effort cleanup, JS string limitation Tests: 54 → 64 (10 new tests covering all security changes)
benvinegar
force-pushed
the
feat/slack-broker
branch
from
66ff0fe to
52bf2d7
Compare
2 participants
Summary
Self-contained Cloudflare Worker (
slack-broker/) that routes messages between Slack workspaces and individual baudbot servers with end-to-end encryption. Deployable viacd slack-broker && wrangler deploy.Implements Phase 1 from
docs/slack-broker-spec.md: core broker skeleton, crypto layer, OAuth install flow, inbound/outbound paths, server registration, and tests.What's Included
Crypto Layer (
src/crypto/)seal.ts—crypto_box_sealsealed boxes for inbound (Slack→server) encryption. Broker encrypts with server pubkey, CANNOT decrypt.box.ts—crypto_boxauthenticated encryption for outbound (server→Slack). Broker decrypts transiently to post to Slack, then zeroes plaintext.verify.ts— Ed25519 signatures for envelope authentication. Deterministic canonicalization prevents field-ordering attacks.Slack Integration (
src/slack/)events.ts— Slack Events API handler with HMAC-SHA256 signature verification and 5-minute replay protectionoauth.ts— OAuth install flow (redirect → callback → store bot token → return auth code)api.ts— Slack Web API helpers:chat.postMessage,reactions.add,chat.updateRouting (
src/routing/)registry.ts— KV-backed workspace registry (workspace_id → {server_url, server_pubkey, bot_token, status})forward.ts— Encrypt event with sealed box → POST to server callback URL (10s timeout, no queuing)API Endpoints (
src/api/)register.ts— Server registration with auth code verification + HTTPS-only callback URLs; unregister with signature verificationsend.ts— Outbound: receive encrypted reply → verify server signature → decrypt → post to Slack → zero plaintextRouter (
src/index.ts)Routes all requests, derives broker keypairs from a single 32-byte seed secret.
GET/healthGET/api/broker-pubkeyPOST/slack/eventsGET/slack/oauth/installGET/slack/oauth/callbackPOST/api/registerDELETE/api/registerPOST/api/sendSecurity Properties
Tests
54 tests across 3 suites, all passing:
crypto.test.ts(24 tests) — sealed box encrypt/decrypt, authenticated box, signatures, encoding, edge casesrouting.test.ts(16 tests) — KV registry CRUD, auth code hashing, event forwarding with mocked fetchintegration.test.ts(14 tests) — end-to-end inbound/outbound flows, Slack signature verification, registration cycle, replay protectionDocs Updated
AGENTS.md/CLAUDE.md— addedslack-broker/to repo layoutdocs/architecture.md— added slack-broker to source tree diagramDependencies
tweetnacl— libsodium-compatible crypto (lightweight, no native deps, Workers-compatible)tweetnacl-util— encoding utilities@cloudflare/workers-types— TypeScript types (dev)vitest— test runner (dev)wrangler— Cloudflare Workers CLI (dev)