Description

monitorWebSocket() logs WebSocket start/close errors using formatFeishuWsErrorForLog(err). The sanitizer attempts to redact common secret fields, but the allowlist of keys is incomplete for Feishu/Lark credentials.

In particular, Feishu commonly uses the field name app_access_token, but the redaction regex only matches tenant_access_token, access_token, etc. As a result, an SDK error message that includes app_access_token=<token> (or similar) will be logged in plaintext.

• Input: err (from wsClient.start() / wsClient.close()), potentially containing HTTP request/response details and tokens
• Sink: runtime.error(...) at the retry log line
• Impact: Secret/token disclosure into logs (often shipped to centralized logging/monitoring), enabling impersonation and unauthorized API calls

Vulnerable code:

.replace(
  ​/\b((?:app[_-]?secret|tenant[_-]?access[_-]?token|access[_-]?token|refresh[_-]?token|token|secret|password)\s*[:=]\s*)[^\s&;,]+/gi,
  "$1[redacted]",
)

This does not match app_access_token.

Recommendation

Expand and harden redaction to cover Feishu/Lark token field names (including app_access_token) and prefer a more general approach when possible.

Example fix (add missing key and broaden matching for token-like keys):

const redacted = singleLine
  ​// ...existing replaces...
  .replace(
    ​/\b((?:app[_-]?access[_-]?token|app[_-]?secret|tenant[_-]?access[_-]?token|access[_-]?token|refresh[_-]?token|token|secret|password)\s*[:=]\s*)[^\s&;,]+/gi,
    "$1[redacted]",
  );

Additional hardening ideas:

• Redact common header forms beyond Bearer (e.g., Authorization: Basic ..., X-Token: ...).
• Consider redacting any key that ends with token/secret/password in JSON-like strings.
• Keep truncation after redaction (already done) and add tests for app_access_token leakage.

Description

formatFeishuWsErrorForLog() performs multiple full-string passes (character normalization + several global regex replacements) on Error.message/String(err) before applying any length limit.

• Input: err from wsClient.start()/wsClient.close() failures can contain large, upstream-controlled strings (e.g., embedded HTTP responses, stack traces, or payloads).
• Processing: Array.from(raw)…join("") and chained .replace(/.../g, ...) each scan/allocate over the full message.
• Only after all passes is the output truncated to FEISHU_WS_LOG_ERROR_MAX_LENGTH (500).

An attacker who can trigger failures that include very large messages can force expensive CPU work and large temporary allocations, degrading availability.

Vulnerable code:

const raw = err instanceof Error ? err.message || err.name : String(err);
const singleLine = Array.from(raw, (char) => { /* ... */ }).join("");
const redacted = singleLine
  .replace(/.../g, ...)
  .replace(/.../g, ...)
  .replace(/.../g, ...)
  .replace(/.../g, ...)
  .replace(/\s+/g, " ")
  .trim();
​// truncation happens only here
return `${redacted.slice(0, FEISHU_WS_LOG_ERROR_MAX_LENGTH)}...`;

Recommendation

Apply a hard cap to the input string before expensive transformations, and avoid repeated full-string allocations.

Example (truncate early, then redact):

const MAX = FEISHU_WS_LOG_ERROR_MAX_LENGTH;
const OVERSCAN = 200; // small buffer so redaction still works around boundaries

function formatFeishuWsErrorForLog(err: unknown): string {
  const raw0 = err instanceof Error ? (err.message || err.name) : String(err);
  const raw = raw0.length > MAX + OVERSCAN ? raw0.slice(0, MAX + OVERSCAN) : raw0;

  ​// normalize without Array.from (fewer allocations)
  const singleLine = raw.replace(/[\x00-\x1F\x7F]/g, " ");

  const redacted = singleLine
    .replace(/:\/\/[^:@\/\s]+:[^@\/\s]+@/g, "://[redacted]@")
    .replace(/\b(authorization\s*[:=]\s*Bearer\s+)[^\s,;]+/gi, "$1[redacted]")
    .replace(/\b(Bearer\s+)[A-Za-z0-9._~+\/-]+=*/g, "$1[redacted]")
    .replace(/\b((?:app[_-]?secret|tenant[_-]?access[_-]?token|access[_-]?token|refresh[_-]?token|token|secret|password)\s*[:=]\s*)[^\s&;,]+/gi, "$1[redacted]")
    .replace(/\s+/g, " ")
    .trim();

  if (!redacted) return "unknown error";
  return redacted.length <= MAX ? redacted : `${redacted.slice(0, MAX)}...`;
}

This bounds worst-case runtime/memory even if upstream error messages are huge.

Description

The new reconnect/cleanup logic in monitorWebSocket() uses global Maps (wsClients, botOpenIds, botNames) keyed only by accountId. The helper cleanupFeishuWsClient() unconditionally deletes these entries for an accountId regardless of whether the wsClient being cleaned up is still the one currently registered.

If monitorWebSocket() is ever started concurrently for the same accountId (e.g., duplicate monitor start, overlapping restarts, or a previous loop iteration still unwinding while a new one begins), one instance can:

• call wsClients.set(accountId, wsClient) for the new client
• while another instance (or earlier attempt) calls cleanupFeishuWsClient({ accountId, wsClient: oldClient })
• which then executes wsClients.delete(accountId) and clears bot identity state

This can orphan a live WebSocket client (it remains connected but is no longer tracked/closeable via stopFeishuMonitorState()), and can cause repeated leaked connections and state corruption, leading to resource exhaustion / denial of service over time.

Vulnerable code:

function cleanupFeishuWsClient(...) {
  ...
  wsClients.delete(accountId);
  botOpenIds.delete(accountId);
  botNames.delete(accountId);
}

Recommendation

Make cleanup conditional on ownership so one monitor instance cannot delete another instance’s state.

Option A (compare-and-delete): only delete if the current map entry matches the client being cleaned up.

function cleanupFeishuWsClient({ accountId, wsClient, error }: { ... }) {
  if (wsClient) {
    try { wsClient.close(); } catch (err) { error(...); }
    if (wsClients.get(accountId) === wsClient) {
      wsClients.delete(accountId);
    }
  }
  ​// Only clear bot identity if this monitor instance is the active one,
  ​// or move bot identity lifecycle management out of WS cleanup.
  if (wsClients.has(accountId) === false) {
    botOpenIds.delete(accountId);
    botNames.delete(accountId);
  }
}

Option B (generation token / per-account mutex): store {client, generation} and only cleanup when generations match, or ensure only one monitorWebSocket() runs per account by guarding starts with a per-account lock/singleton promise.