Greptile Summary

This PR extracts provider-specific doctor helpers (Telegram, Discord) and shared config-repair utilities (allowlist, open-policy, mutable-allowlist, empty-allowlist policy) out of the monolithic doctor-config-flow.ts into a new src/commands/doctor/providers/ and src/commands/doctor/shared/ structure. All 59 doctor tests continue to pass and no logic changes are introduced.

Key observations:

• The extraction is faithful — all moved functions are byte-for-byte equivalent to their originals.
• resolveAllowFromMode (including its type alias) is duplicated identically between shared/open-policy-allowfrom.ts and shared/allowlist-policy-repair.ts; worth extracting to avoid drift.
• shared/empty-allowlist-policy.ts imports collectTelegramGroupPolicyWarnings from providers/telegram.ts, creating a shared → providers dependency that inverts the intended layering. If other providers eventually need custom group-policy warning logic, this shared file would need to be reopened — against the stated goal of the refactor.
• Test coverage is solid across all new modules.

Confidence Score: 4/5

• Safe to merge; all moved logic is functionally identical and tests pass.
• This is a pure refactoring with no behavioral changes and comprehensive test coverage. The two flagged issues (layering inversion in empty-allowlist-policy and duplicated resolveAllowFromMode) are style/maintainability concerns rather than bugs, and neither affects runtime correctness. A 4 rather than 5 reflects that the shared→provider dependency runs counter to the architecture goal of the PR and is worth resolving before the pattern propagates.
• src/commands/doctor/shared/empty-allowlist-policy.ts (providers import), src/commands/doctor/shared/open-policy-allowfrom.ts and src/commands/doctor/shared/allowlist-policy-repair.ts (duplicated resolveAllowFromMode)

This is a comment left during a code review.
Path: src/commands/doctor/shared/empty-allowlist-policy.ts
Line: 1

Comment:
**Layering violation: `shared/` depends on `providers/`**

`shared/empty-allowlist-policy.ts` imports `collectTelegramGroupPolicyWarnings` from `../providers/telegram.js`. This inverts the intended dependency direction: shared utilities should not depend on provider-specific modules. If a future provider is added and also needs custom group-policy warning logic, this shared file would need to be reopened and extended with another provider-specific branch — exactly the tight coupling this refactor was meant to eliminate.

Consider either:
- Moving the Telegram-specific branch out of this shared function and into the Telegram provider layer (calling `collectTelegramGroupPolicyWarnings` at the call site in `doctor-config-flow.ts` before/after `collectEmptyAllowlistPolicyWarningsForAccount`), or
- Accepting a callbacks/strategy pattern here so the shared helper stays provider-agnostic.

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/commands/doctor/shared/open-policy-allowfrom.ts
Line: 4-14

Comment:
**Duplicated `resolveAllowFromMode` across two shared files**

`resolveAllowFromMode` (and its accompanying mode type) is defined identically in both `open-policy-allowfrom.ts` (as `OpenPolicyAllowFromMode`) and `allowlist-policy-repair.ts` (as `AllowFromMode`). The logic is byte-for-byte identical in both places. If the channel-to-mode mapping ever needs to change, it now has to be updated in two places.

Consider extracting it to `src/commands/doctor/shared/allow-from-mode.ts` (or similar) and importing it from both files.

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

_{Last reviewed commit: "refactor(doctor): ex..."}

🔒 Aisle Security Analysis

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

1. 🔴 SSRF / Telegram bot token exfiltration via configurable apiRoot/proxyUrl during doctor --fix username resolution

Description

The doctor --fix flow attempts to auto-resolve non-numeric Telegram allowFrom entries (e.g. @​username) by calling lookupTelegramChatId() with:

• token: resolved Telegram bot token
• apiRoot: configurable URL (channels.telegram.apiRoot / per-account override)
• proxyUrl: configurable proxy URL (channels.telegram.proxy / per-account override)

Because apiRoot is not allowlisted to Telegram-owned hosts and can be any URL, running doctor --fix on an attacker-influenced config can:

• Trigger SSRF to arbitrary hosts (including internal network endpoints) from the machine running the CLI
• Exfiltrate the bot token because the Telegram Bot API token is embedded in the request path (/bot<token>/...) inside lookupTelegramChatId().

This is especially risky in CI or other automation where secrets are present but configs may come from untrusted branches/PRs.

Vulnerable call site:

const id = await lookupTelegramChatId({
  token: account.token,
  chatId: username,
  apiRoot: account.apiRoot,
  proxyUrl: account.proxyUrl,
  network: account.network,
});

Downstream sink (token in URL path) occurs inside extensions/telegram/src/api-fetch.ts where the URL is constructed as ${apiBase}/bot${token}/getChat?....

Recommendation

Restrict where doctor --fix can send Telegram bot tokens.

Safer options (pick one):

1. Ignore custom endpoints for doctor repairs (recommended for doctor --fix):

const id = await lookupTelegramChatId({
  token: account.token,
  chatId: username,
  signal: controller.signal,
  ​// Do NOT forward apiRoot/proxyUrl from config in this auto-repair path
});

2. Allowlist Telegram API hosts + require HTTPS before forwarding apiRoot:

function isSafeTelegramApiRoot(apiRoot?: string): boolean {
  if (!apiRoot) return true;
  const u = new URL(apiRoot);
  return u.protocol === "https:" && (u.hostname === "api.telegram.org");
}

const safeApiRoot = isSafeTelegramApiRoot(account.apiRoot) ? account.apiRoot : undefined;
const id = await lookupTelegramChatId({ token: account.token, chatId: username, apiRoot: safeApiRoot });

3. If custom endpoints/proxies are needed, require an explicit CLI opt-in (e.g. --allow-custom-telegram-endpoints) and print a warning explaining that the bot token will be sent to that endpoint.

2. 🔵 Terminal/log injection via unsanitized config values in doctor output

Description

The doctor command renders user-controlled config values directly into terminal output without output neutralization.

• Values derived from configuration (e.g., allowlist entries, IDs/keys embedded in config paths) are interpolated into strings.
• These strings are printed via note(...) (Clack terminal UI) without sanitizing ANSI escape sequences or control characters.
• A malicious config value containing newlines or ANSI/OSC sequences can forge/spoof log lines, hide warnings, or manipulate terminal display in CI/local terminals.

Vulnerable code (examples):

const exampleLines = mutableAllowlistHits
  .slice(0, 8)
  .map((hit) => `- ${hit.path}: ${hit.entry}`)
  .join("\n");
...
note(
  [
    `- Found ${mutableAllowlistHits.length} mutable allowlist ...`,
    exampleLines,
    ...
  ].join("\n"),
  "Doctor warnings",
);

Related call sites introduced/expanded in this change also build output lines from raw config entries, e.g.:

• src/commands/doctor/providers/telegram.ts: changes.push(\- ${pathLabel}: resolved ${rep.from} -> ${rep.to}`)whererep.from` comes from config allowFrom entries.

Note: the repo already includes a helper that would mitigate this (sanitizeForLog in src/terminal/ansi.ts), but these interpolations do not use it.

Recommendation

Sanitize/neutralize untrusted values before interpolating them into terminal/log output.

Option A (preferred): sanitize only the untrusted parts (paths/entries coming from config):

import { sanitizeForLog } from "../terminal/ansi.js";

const exampleLines = mutableAllowlistHits
  .slice(0, 8)
  .map((hit) => `- ${sanitizeForLog(hit.path)}: ${sanitizeForLog(hit.entry)}`)
  .join("\n");

Option B: sanitize the complete message right before printing (be careful if you intentionally include ANSI formatting):

note(sanitizeForLog(message), "Doctor warnings");

Also apply the same sanitization to other doctor-generated change/warning lines that include config-derived values (e.g., Telegram rep.from, dynamic prefix/object keys).

Analyzed PR: #51753 at commit b02f620

_{Last updated on: 2026-03-21T17:40:50Z}