security: fix 9 review findings — auth code reuse, OAuth hijack, crypto, 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)

Author: Baudbot

slack-broker/README.md

@@ -113,7 +113,7 @@ Supported actions: `chat.postMessage`, `reactions.add`, `chat.update`.
 
 | Primitive | Use | Library |
 |-----------|-----|---------|
-| `crypto_box_seal` (X25519 + XSalsa20-Poly1305) | Inbound: Slack → server | tweetnacl |
+| `crypto_box_seal` (X25519 + XSalsa20-Poly1305) | Inbound: Slack → server | libsodium-wrappers-sumo |
 | `crypto_box` (X25519 + XSalsa20-Poly1305) | Outbound: server → Slack | tweetnacl |
 | Ed25519 | Envelope signatures | tweetnacl |
 | HMAC-SHA256 | Slack request verification | Web Crypto API |
@@ -133,13 +133,15 @@ Supported actions: `chat.postMessage`, `reactions.add`, `chat.update`.
 - ✅ Server authenticates broker (broker signs envelopes)
 - ✅ Broker authenticates server (server signs outbound requests)
 - ✅ Replay protection (timestamps + nonces on all messages)
-- ✅ Auth code verification for server registration
+- ✅ Auth code verification for server registration (one-time use, HMAC-SHA256)
+- ✅ Bot tokens encrypted at rest in KV (nacl.secretbox)
 - ❌ Perfect forward secrecy (would need session keys — future enhancement)
+- ❌ Rate limiting (Phase 3 — pre-production requirement)
 
 ### What the Broker Can See
 
 - Routing metadata: workspace_id, channel, thread_ts, timestamps
-- Outbound message content: **transiently** (decrypted in memory to post to Slack, then zeroed)
+- Outbound message content: **transiently** (decrypted in memory to post to Slack, then best-effort zeroed — JS strings from JSON.parse cannot be deterministically zeroed, only the underlying Uint8Array buffer is cleared)
 
 ### What the Broker Cannot See
 

slack-broker/package-lock.json

@@ -11,6 +11,7 @@
     "typecheck": "tsc --noEmit"
   },
   "dependencies": {
+    "libsodium-wrappers-sumo": "^0.8.2",
     "tweetnacl": "^1.0.3"
   },
   "devDependencies": {

slack-broker/package.json

@@ -73,6 +73,12 @@ export async function handleRegister(
     return jsonResponse({ ok: false, error: "missing required fields" }, 400);
   }
 
+  // Validate workspace_id matches Slack team ID format to prevent
+  // pipe-delimiter injection in canonicalized signatures.
+  if (!/^T[A-Z0-9]+$/.test(body.workspace_id)) {
+    return jsonResponse({ ok: false, error: "invalid workspace_id format" }, 400);
+  }
+
   // Validate callback URL
   try {
     const url = new URL(body.server_callback_url);
@@ -89,8 +95,18 @@ export async function handleRegister(
     return jsonResponse({ ok: false, error: "workspace not found — complete OAuth install first" }, 404);
   }
 
-  // Verify auth code
-  const providedHash = await hashAuthCode(body.auth_code);
+  // Reject re-registration of already-active workspaces.
+  // The current server must unregister first (DELETE /api/register).
+  if (workspace.status === "active") {
+    return jsonResponse({ ok: false, error: "workspace already active — unregister the current server first" }, 409);
+  }
+
+  // Verify auth code (must not be empty — cleared after first successful registration)
+  if (!workspace.auth_code_hash) {
+    return jsonResponse({ ok: false, error: "auth code already consumed — re-install the Slack app to generate a new one" }, 403);
+  }
+
+  const providedHash = await hashAuthCode(body.auth_code, env.BROKER_PRIVATE_KEY);
   if (providedHash !== workspace.auth_code_hash) {
     return jsonResponse({ ok: false, error: "invalid auth code" }, 403);
   }

slack-broker/src/api/register.ts

@@ -29,7 +29,7 @@
 import { boxDecrypt, zeroBytes } from "../crypto/box.js";
 import { verify, canonicalizeOutbound } from "../crypto/verify.js";
 import { decodeBase64, decodeUTF8 } from "../util/encoding.js";
-import { getWorkspace } from "../routing/registry.js";
+import { getWorkspace, decryptBotToken } from "../routing/registry.js";
 import { postMessage, addReaction, updateMessage } from "../slack/api.js";
 import type { Env } from "../index.js";
 
@@ -140,9 +140,12 @@ export async function handleSend(
 
   // Execute the Slack API call
   try {
+    // Decrypt the bot token (encrypted at rest in KV)
+    const botToken = decryptBotToken(workspace.bot_token, env.BROKER_PRIVATE_KEY);
+
     const result = await executeSlackAction(
       body.action as SlackAction,
-      workspace.bot_token,
+      botToken,
       body.routing,
       decryptedBody,
     );

slack-broker/src/api/send.ts

@@ -74,8 +74,13 @@ export function boxDecrypt(
 }
 
 /**
- * Zero out a Uint8Array to minimize plaintext residence in memory.
- * Call this after posting the decrypted content to Slack.
+ * Best-effort memory cleanup — zeroes a Uint8Array to reduce plaintext
+ * residence in memory. Call this after posting the decrypted content to Slack.
+ *
+ * NOTE: This only zeroes the Uint8Array buffer. JS strings derived from the
+ * buffer (e.g. via TextDecoder or JSON.parse) are immutable and cannot be
+ * deterministically zeroed — they remain in memory until garbage collected.
+ * This is a limitation of the JS runtime, not a bug.
  */
 export function zeroBytes(arr: Uint8Array): void {
   arr.fill(0);

slack-broker/src/crypto/box.ts

@@ -4,63 +4,44 @@
  * The broker encrypts with the server's public key. Only the server's
  * private key can decrypt. The broker CANNOT decrypt sealed boxes.
  *
- * Uses tweetnacl's box.keyPair + secretbox under the hood to implement
- * the libsodium crypto_box_seal pattern:
- *   1. Generate an ephemeral X25519 keypair
- *   2. Compute shared secret: ECDH(ephemeral_sk, recipient_pk)
- *   3. Derive nonce from ephemeral_pk + recipient_pk
- *   4. Encrypt payload with crypto_box using the shared secret
- *   5. Output: ephemeral_pk || ciphertext
+ * Uses libsodium's native crypto_box_seal / crypto_box_seal_open for
+ * interoperability with standard libsodium implementations on the server.
+ * The nonce is derived using BLAKE2B(ephemeral_pk || recipient_pk) as
+ * per the libsodium spec.
  */
 
-import nacl from "tweetnacl";
-import { decodeBase64, encodeBase64 } from "../util/encoding.js";
+import _sodium from "libsodium-wrappers-sumo";
+import { encodeBase64, decodeBase64 } from "../util/encoding.js";
 
 /** Length of an X25519 public key in bytes. */
 const PUBLIC_KEY_BYTES = 32;
 
-/**
- * Derive a nonce from the ephemeral public key and recipient public key.
- * Uses the first 24 bytes of SHA-512(ephemeral_pk || recipient_pk).
- */
-async function deriveNonce(
-  ephemeralPk: Uint8Array,
-  recipientPk: Uint8Array,
-): Promise<Uint8Array> {
-  const input = new Uint8Array(PUBLIC_KEY_BYTES * 2);
-  input.set(ephemeralPk, 0);
-  input.set(recipientPk, PUBLIC_KEY_BYTES);
-  const hash = await crypto.subtle.digest("SHA-512", input);
-  return new Uint8Array(hash).slice(0, nacl.box.nonceLength);
+/** Ensure libsodium is initialized before use. */
+async function sodium(): Promise<typeof _sodium> {
+  await _sodium.ready;
+  return _sodium;
 }
 
 /**
- * Encrypt a message using a sealed box (crypto_box_seal equivalent).
+ * Encrypt a message using a sealed box (crypto_box_seal).
  *
- * Returns base64-encoded ciphertext: ephemeral_pk (32 bytes) || box output.
+ * Returns base64-encoded ciphertext (ephemeral_pk || box output).
  * Only the holder of `recipientPublicKey`'s corresponding private key can decrypt.
+ *
+ * Uses libsodium's native implementation with BLAKE2B nonce derivation
+ * for interoperability with standard libsodium on the server side.
  */
 export async function sealedBoxEncrypt(
   plaintext: Uint8Array,
   recipientPublicKey: Uint8Array,
 ): Promise<string> {
-  const ephemeral = nacl.box.keyPair();
-  const nonce = await deriveNonce(ephemeral.publicKey, recipientPublicKey);
-  const ciphertext = nacl.box(plaintext, nonce, recipientPublicKey, ephemeral.secretKey);
-
-  if (!ciphertext) {
-    throw new Error("sealedBoxEncrypt: encryption failed");
-  }
-
-  // Output: ephemeral_pk || ciphertext
-  const sealed = new Uint8Array(PUBLIC_KEY_BYTES + ciphertext.length);
-  sealed.set(ephemeral.publicKey, 0);
-  sealed.set(ciphertext, PUBLIC_KEY_BYTES);
+  const s = await sodium();
+  const sealed = s.crypto_box_seal(plaintext, recipientPublicKey);
   return encodeBase64(sealed);
 }
 
 /**
- * Decrypt a sealed box (crypto_box_seal_open equivalent).
+ * Decrypt a sealed box (crypto_box_seal_open).
  *
  * Used on the SERVER side (not in the broker for inbound messages).
  * Included here for testing and for potential future use.
@@ -70,20 +51,16 @@ export async function sealedBoxDecrypt(
   recipientPublicKey: Uint8Array,
   recipientSecretKey: Uint8Array,
 ): Promise<Uint8Array> {
+  const s = await sodium();
   const sealed = decodeBase64(sealedBase64);
 
-  if (sealed.length < PUBLIC_KEY_BYTES + nacl.box.overheadLength) {
+  if (sealed.length < PUBLIC_KEY_BYTES + s.crypto_box_MACBYTES) {
     throw new Error("sealedBoxDecrypt: ciphertext too short");
   }
 
-  const ephemeralPk = sealed.slice(0, PUBLIC_KEY_BYTES);
-  const ciphertext = sealed.slice(PUBLIC_KEY_BYTES);
-  const nonce = await deriveNonce(ephemeralPk, recipientPublicKey);
-  const plaintext = nacl.box.open(ciphertext, nonce, ephemeralPk, recipientSecretKey);
-
-  if (!plaintext) {
+  try {
+    return s.crypto_box_seal_open(sealed, recipientPublicKey, recipientSecretKey);
+  } catch {
     throw new Error("sealedBoxDecrypt: decryption failed — invalid key or corrupted data");
   }
-
-  return plaintext;
 }

slack-broker/src/crypto/seal.ts

@@ -84,6 +84,10 @@ export default {
       _ctx: ctx,
     };
 
+    // TODO(Phase 3): Add rate limiting before production deployment.
+    // Per-workspace and per-IP limits on /api/send, /api/register, and /slack/events.
+    // Cloudflare Rate Limiting rules or a KV-based token bucket are both viable.
+
     // Route requests
     try {
       // Health check

slack-broker/src/index.ts

@@ -50,6 +50,16 @@ export async function forwardEvent(
     return { ok: false, error: "workspace missing server configuration" };
   }
 
+  // Enforce HTTPS for all server callback URLs
+  try {
+    const url = new URL(workspace.server_url);
+    if (url.protocol !== "https:") {
+      return { ok: false, error: "server URL must use HTTPS" };
+    }
+  } catch {
+    return { ok: false, error: "invalid server URL" };
+  }
+
   // Serialize and encrypt
   const plaintext = encodeUTF8(JSON.stringify(event));
   const serverPubkey = decodeBase64(workspace.server_pubkey);