🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🔵 Terminal/log injection via unsanitized Discord /gateway/bot response body snippets

Description

The Discord gateway metadata fetch now includes a trimmed snippet of the upstream HTTP response body in thrown error messages. Because the snippet is derived from untrusted input (especially when a user-configured proxy is enabled) and is only whitespace-normalized, it can still contain ANSI escape sequences / control characters.

This can lead to log/terminal injection when these errors are surfaced:

• Input: body = await response.text() reads arbitrary text from the network/proxy.
• Transformation: summarizeGatewayResponseBody() collapses \s+ but does not strip non-whitespace control bytes like ESC (\x1b) or other non-printables.
• Sink: the snippet is interpolated into Error.message and may be printed via console-based runtime logging and global error handlers (e.g., unhandled rejection / uncaught exception handlers print formatted error text to stderr).

Vulnerable code (untrusted body included in Error.message):

const summary = summarizeGatewayResponseBody(body);

if (!response.ok) {
  throw createGatewayMetadataError({
    detail: `Discord API /gateway/bot failed (${response.status}): ${summary}`,
    transient,
  });
}

Even with replace(/\s+/g, " "), an attacker-controlled upstream/proxy can include sequences like \x1b[2J (clear screen), OSC-8 hyperlinks, etc., which are not whitespace and would survive into logs/terminal output.

Recommendation

Avoid embedding raw upstream bodies in exception messages that may be logged to a terminal.

Options (prefer 1 + 2):

1. Strip ANSI escapes and control chars from the snippet before including it in any log/error message.
2. Keep body snippets out of Error.message; instead attach sanitized details as structured metadata only when verbose/debug logging is enabled.

Example sanitization:

function sanitizeForTerminal(text: string): string {
  ​// Remove C0/C1 control chars including ESC (0x1b)
  return text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
}

function summarizeGatewayResponseBody(body: string): string {
  const normalized = sanitizeForTerminal(body).trim().replace(/\s+/g, " ");
  return normalized ? normalized.slice(0, 240) : "<empty>";
}

If you need the raw body for troubleshooting, log it only under explicit debug/verbose mode and ensure it is escaped/sanitized before printing.

Analyzed PR: #44397 at commit edd17c0

_{Last updated on: 2026-03-13T05:08:07Z}

Greptile Summary

This PR hardens Discord gateway startup by replacing the raw response.json() call with a text-first approach that classifies 5xx and known upstream-proxy error bodies as transient before attempting JSON parsing, and refactors the duplicated ProxyGatewayPlugin class into a shared createGatewayPlugin factory.

Key changes:

• fetchDiscordGatewayInfo reads the response body as text, checks isTransientDiscordGatewayResponse (5xx or known upstream-connect patterns) before attempting JSON.parse, and wraps all failure paths in createGatewayMetadataError with a consistent "fetch failed" message for transient errors
• createGatewayPlugin replaces the duplicated ProxyGatewayPlugin/GatewayPlugin instantiation across three branches with a single SafeGatewayPlugin subclass
• "upstream connect error" and "disconnect/reset before headers" added to TRANSIENT_NETWORK_MESSAGE_SNIPPETS in unhandled-rejections.ts as belt-and-suspenders coverage
• Tests updated for the new ok/status/text() response shape, with two new cases covering the no-proxy path and the plain-text 503 regression

Issues found:

• response.text() on line 95 of gateway-plugin.ts is not inside a try-catch. A connection drop during body streaming would throw a raw error that bypasses both the outer fetch-call wrapper and the inner JSON.parse wrapper, potentially producing an error that does not match isWrappedFetchFailedMessage and could cause a process exit.
• The ...params.fetchInit spread on line 85 comes after the headers literal, so any headers key in fetchInit would silently overwrite the Authorization: Bot … entry. Currently safe (only dispatcher is passed), but fragile for future callers.

Confidence Score: 4/5

• Safe to merge after addressing the unguarded response.text() body-read path; all other logic is correct and well-tested.
• The fix correctly solves the reported production restart loop and the refactoring is clean with good test coverage. One logic concern — the unguarded response.text() call could allow body-read network errors to escape the wrapping logic — and one fragile header-spread ordering are the only items that lower the score from 5.
• src/discord/monitor/gateway-plugin.ts — specifically the response.text() call and the fetchInit spread ordering in fetchDiscordGatewayInfo.

This is a comment left during a code review.
Path: src/discord/monitor/gateway-plugin.ts
Line: 95

Comment:
**`response.text()` not guarded against body-read failures**

`response.text()` is called outside any try-catch block. If the connection drops while the response body is being streamed, this will throw a raw network error (e.g., `ECONNRESET`) that bypasses both the outer fetch catch (lines 80–93, which only wraps the initial request) and the inner catch (lines 115–121, which only wraps `JSON.parse`). The raw error will propagate up through `registerClient` without being wrapped in a `createGatewayMetadataError`, so it won't carry the `"fetch failed"` message that `isWrappedFetchFailedMessage` relies on. While many such errors would still be caught via their error codes in `TRANSIENT_NETWORK_CODES`, others (e.g., body-length or encoding errors) could slip through and trigger a process exit.

```suggestion
  let body: string;
  try {
    body = await response.text();
  } catch (error) {
    throw createGatewayMetadataError({
      detail: error instanceof Error ? error.message : String(error),
      transient: true,
      cause: error,
    });
  }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/discord/monitor/gateway-plugin.ts
Line: 81-86

Comment:
**`fetchInit` spread silently overwrites `Authorization` header**

Because `...params.fetchInit` is spread after `headers`, any `headers` key in `fetchInit` would silently replace the `Authorization: Bot …` entry, causing a `401` on the next gateway metadata fetch. The current callers only ever pass `{ dispatcher: fetchAgent }` so there is no immediate breakage, but the ordering is fragile.

Putting the per-call headers last (or deep-merging them) would make this safe regardless of what `fetchInit` contains:

```suggestion
    response = await params.fetchImpl(DISCORD_GATEWAY_BOT_URL, {
      ...params.fetchInit,
      headers: {
        ...(params.fetchInit?.headers as Record<string, string> | undefined),
        Authorization: `Bot ${params.token}`,
      },
    });
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: bcc50f7}

Merged via squash.

• Prepared head SHA: edd17c0effe4f90887ac94ce549f44a69fe19eb2
• Merge commit: 6d0939d84ea5e509da7a910a05db5d7dd098669a

Thanks @jalehman!