🔒 Aisle Security Analysis

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

1. 🟡 Unsalted truncated SHA-256 used as "redaction" enables identifier re-identification and cross-run correlation in logs

Description

redactIdentifier() is used in new observation logging to “redact” values like auth profileId, provider request_id, and raw error/fingerprint fields. However, the implementation is a deterministic unsalted SHA-256 prefix.

Implications:

• Re-identification / dictionary attacks for low-entropy identifiers: values with small or predictable domains (e.g., profile IDs like openai:default, phone numbers, emails, common IDs) can be brute-forced offline by anyone who can read the logs.
• Cross-tenant / cross-environment correlation: because there is no keyed salt/pepper, the same identifier will hash to the same value across deployments, enabling correlation if logs are aggregated or shared.
• Membership testing: logging rawErrorHash of the unredacted raw error payload allows a party with log access to test whether a guessed string/error content occurred, even if previews are redacted.

New call sites include:

• src/agents/auth-profiles/state-observation.ts (logs profileId: redactIdentifier(profileId, { len: 12 }))
• src/agents/pi-embedded-error-observation.ts (logs rawErrorHash, requestIdHash, rawErrorFingerprint using redactIdentifier(..., { len: 12 }))
• src/agents/pi-embedded-runner/run/failover-observation.ts (logs hashed profileId)

Vulnerable core code:

return `sha256:${sha256HexPrefix(trimmed, opts?.len ?? 12)}`;

This is not reversible, but it is not a secure redaction primitive for identifiers because it is unkeyed and deterministic.

Recommendation

Use a keyed construction (HMAC) with a deployment secret (pepper) so log readers cannot brute-force identifiers, while preserving stability for correlation within the same deployment.

Suggested implementation:

import crypto from "node:crypto";

const LOG_REDACTION_KEY = process.env.LOG_REDACTION_KEY; // 32+ bytes recommended

export function redactIdentifier(value: string | undefined, opts?: { len?: number; purpose?: string }): string {
  const trimmed = value?.trim();
  if (!trimmed) return "-";
  const len = Number.isFinite(opts?.len) ? Math.max(8, Math.floor(opts!.len!)) : 24;
  const purpose = opts?.purpose ?? "id";

  if (!LOG_REDACTION_KEY) {
    ​// safer default than unkeyed hashes: avoid pretending to redact
    return "redacted";
  }

  const digest = crypto
    .createHmac("sha256", LOG_REDACTION_KEY)
    .update(`${purpose}:`)
    .update(trimmed)
    .digest("hex")
    .slice(0, len);

  return `hmac256:${digest}`;
}

Additionally:

• Consider a per-tenant key/salt if you want to prevent cross-tenant correlation inside shared log systems.
• Consider increasing the default prefix length to reduce collisions (e.g., 24+ hex chars).

2. 🟡 CPU DoS risk from unbounded provider error redaction/parsing in observation logging

Description

The new observation sanitization path performs synchronous parsing and multi-pass regex redaction over the entire raw provider error string before truncation, allowing very large error payloads to consume substantial CPU and block the Node.js event loop.

Key points:

• buildApiErrorObservationFields() trims the full rawError, then calls parseApiErrorInfo(trimmed) (JSON parsing) and redactObservationText(trimmed) (regex redaction) before truncating for preview output [src/agents/pi-embedded-error-observation.ts:100-134].
• redactObservationText() forces redaction by calling redactSensitiveText() with default + operator-configured + extra regex patterns [src/agents/pi-embedded-error-observation.ts:42-56].
• redactSensitiveText() applies patterns sequentially; for large inputs it chunks replacements (replacePatternBounded) but still processes the full string for every pattern [src/logging/redact.ts:126-139] and [src/logging/redact-bounded.ts:9-25].
• Upstream error strings can embed full HTTP response bodies (untrusted and potentially large). Example: Ollama throws an error containing await response.text() [src/agents/ollama-stream.ts:489-492], which flows into AssistantMessage.errorMessage and then into these observation logs.

Impact:

• A large error payload (e.g., multi-megabyte HTML/text body) can trigger expensive synchronous work during error handling/logging (JSON.parse + N regex passes), leading to degraded throughput or temporary unavailability (denial of service).
• Operator-configured patterns are compiled via a heuristic “safe regex” check, but that check is not a full guarantee against expensive regex behavior, and the multi-pass approach still scales with O(input_size * pattern_count).

Vulnerable code (full-input parsing + redaction before truncation):

const parsed = parseApiErrorInfo(trimmed);
...
const redactedRawPreview = replaceRequestIdPreview(redactObservationText(trimmed), requestId);
...
rawErrorPreview: truncateForObservation(redactedRawPreview, RAW_ERROR_PREVIEW_MAX_CHARS),

Recommendation

Add guardrails so preview-generation never processes unbounded strings synchronously.

Recommended changes:

1. Cap input length before JSON parsing/redaction (keep full hash separately):

const MAX_REDACT_INPUT = 32_768; // or lower
const trimmed = rawError?.trim();
if (!trimmed) return {};

const redactInput = trimmed.length > MAX_REDACT_INPUT ? trimmed.slice(0, MAX_REDACT_INPUT) : trimmed;

​// Only parse/redact the bounded prefix used for previews/fingerprints
const parsed = parseApiErrorInfo(redactInput);
const redactedRawPreview = replaceRequestIdPreview(redactObservationText(redactInput), requestId);

2. Optionally redact both head and tail windows (tokens/IDs sometimes appear at the end), e.g. head(16k)+"…"+tail(16k).

3. Avoid synchronous config file IO per call: cache readLoggingConfig() results with a TTL / watch-based reload, or pass resolved patterns in from a higher layer.

4. Constrain operator-configured patterns: limit count/length; reject patterns that fail compileSafeRegex; consider running them only on bounded windows.

3. 🟡 Console log suppression can be triggered by untrusted runId/sessionId prefix "probe-"

Description

shouldSuppressProbeConsoleLine() suppresses console info/warn output for agent/embedded* and model-fallback* subsystems when either:

• structured metadata contains runId/sessionId starting with probe-, or
• the message contains runId=probe- / sessionId=probe-

This creates a log-evasion/suppression primitive if runId/sessionId is influenced by external inputs.

Relevant code (console suppression):

const runLikeId = typeof params.meta?.runId === "string"
  ? params.meta.runId
  : typeof params.meta?.sessionId === "string"
    ? params.meta.sessionId
    : undefined;
if (runLikeId?.startsWith("probe-")) {
  return true;
}
return /(sessionId|runId)=probe-/.test(params.message);

[src/logging/subsystem.ts:253-283]

Trust boundary / attacker control:

• Gateway-facing runs set runId from the client-controlled idempotencyKey (used as runId downstream) and pass it into the embedded runner / model fallback paths, which now emit suppressed warnings when that runId begins with probe-.

Impact:

• In non-verbose mode, a remote client can choose an idempotency key like probe-... to suppress console visibility of warn/info lines in these subsystems (including new failover/model-fallback decision warnings).
• File logs are written before suppression ([src/logging/subsystem.ts:336-352]), but many deployments rely primarily on stdout/stderr aggregation; if file logging is disabled/silent, suppression can effectively drop these warnings entirely.

Why this is security-relevant:

• Reduces operator visibility into failovers/model-fallback events and related warning signals, potentially aiding abuse/evasion during an attack or reducing detection/forensics (log suppression).

Recommendation

Do not use user-controlled identifiers (runId/sessionId) or message regexes as the switch for suppressing logs.

Recommended mitigations (pick one):

1. Explicit internal flag: require a dedicated boolean like meta.isProbe === true to suppress, and ensure only internal probe codepaths set it.

function shouldSuppressProbeConsoleLine({ level, subsystem, meta }: {...}): boolean {
  if (isVerbose() || level === "error" || level === "fatal") return false;
  const isProbeSuppressedSubsystem = /* same as today */;
  if (!isProbeSuppressedSubsystem) return false;

  return meta?.isProbe === true;
}

2. Server-side generated runIds: in network-facing entrypoints, generate runId server-side and keep client idempotency keys separate (do not reuse them as runId).

3. If you must keep prefix-based behavior, normalize/validate external IDs at ingress so untrusted callers cannot claim probe-... prefixes (e.g., reject or rewrite such prefixes for inbound runId/sessionId).

Also remove the message-regex fallback (/(sessionId|runId)=probe-/) or ensure only structured metadata can activate suppression, to avoid future cases where untrusted strings influence suppression.

4. 🔵 Log forging / terminal escape injection via unsanitized runId/provider/model in consoleMessage (failover observation)

Description

consoleMessage is composed by interpolating runId, provider, and model without neutralizing control characters (e.g. \n, \r, \t, \x1b).

Because consoleMessage is written to the console as a raw string (see writeConsoleLine() in src/logging/subsystem.ts), an attacker who can influence runId (or other interpolated fields) can:

• Forge additional log lines by injecting \n/\r (log injection / log forging)
• Inject terminal control sequences (e.g. ESC[ …) to clear/hide output or create misleading console output

Concrete input control:

• runId for embedded runs is ultimately sourced from gateway ingress idempotencyKey (network-provided) and is only validated as a non-empty string (no character allowlist), then propagated into embedded agent logging.

Vulnerable code:

consoleMessage:
  `embedded run failover decision: runId=${normalizedBase.runId ?? "-"} ... ` +
  `... provider=${normalizedBase.provider}/${normalizedBase.model} ...`

A safer precedent exists in this repo: src/agents/model-fallback-observation.ts uses sanitizeForLog() for interpolated provider/model values to prevent CWE-117 issues.

Recommendation

Sanitize every untrusted value before interpolating into consoleMessage (at least runId, provider, model, and any free-text).

Use the existing sanitizeForLog() helper:

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

const safeRunId = normalizedBase.runId ? sanitizeForLog(normalizedBase.runId) : "-";
const safeProvider = sanitizeForLog(normalizedBase.provider);
const safeModel = sanitizeForLog(normalizedBase.model);

consoleMessage:
  `embedded run failover decision: runId=${safeRunId} stage=${normalizedBase.stage} decision=${decision} ` +
  `reason=${reasonText} provider=${safeProvider}/${safeModel} profile=${profileText}`

Optionally, add a defense-in-depth layer in src/logging/subsystem.ts to sanitize the message before applying Chalk formatting (do not sanitize after formatting or you’ll strip intended color codes).

5. 🔵 Log forging / terminal escape injection via unsanitized runId/provider in consoleMessage (auth profile failure state logging)

Description

consoleMessage in logAuthProfileFailureStateChange interpolates runId and provider without stripping control characters / ANSI escape sequences.

• runId can originate from gateway ingress idempotencyKey (network-provided). It is only constrained to a non-empty string at the schema layer and is passed through to embedded runner logging.
• createSubsystemLogger ultimately writes the console line using console.log/warn/error without removing newlines or terminal control sequences. This enables CWE-117 style log forging and terminal escape injection.

Vulnerable code:

consoleMessage:
  `auth profile failure state updated: runId=${params.runId ?? "-"} profile=${safeProfileId} provider=${params.provider} ` +
  `reason=${params.reason} window=${windowType} reused=${String(windowReused)}`,

Although profileId is hashed via redactIdentifier, the other interpolated fields are not sanitized (contrast with sanitizeForLog() usage in model-fallback-observation.ts).

Recommendation

Sanitize interpolated values in consoleMessage using the existing helper sanitizeForLog().

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

const safeRunId = params.runId ? sanitizeForLog(params.runId) : "-";
const safeProvider = sanitizeForLog(params.provider);

consoleMessage:
  `auth profile failure state updated: runId=${safeRunId} profile=${safeProfileId} provider=${safeProvider} ` +
  `reason=${params.reason} window=${windowType} reused=${String(windowReused)}`

Defense-in-depth: consider sanitizing non-JSON console messages centrally in formatConsoleLine before applying Chalk formatting.

Analyzed PR: #41023 at commit dff074a

_{Last updated on: 2026-03-09T18:09:55Z}

Greptile Summary

This PR adds structured observation logs across the embedded agent error-handling path (lifecycle end, failover decisions, auth-profile state updates, and model-fallback decisions) and replaces always-on raw provider payload logging with sanitized preview/hash/fingerprint fields. The runId is threaded through to each new observation event, and the consoleMessage separation in subsystem.ts cleanly routes compact human-readable lines to the console while keeping full structured payloads in file logs.

Key findings:

• Model-fallback log level (model-fallback-observation.ts:49): logModelFallbackDecision emits at info while all other observation functions in this PR emit at warn. In environments where the file log level is warn or higher, model-fallback decisions will be silently absent while failover, auth-profile, and lifecycle events remain visible — breaking the end-to-end jq-based correlation the PR advertises.

• Field name inconsistency in previousAttempts (model-fallback-observation.ts:52–65): Top-level error fields are manually renamed to errorPreview/errorHash/errorFingerprint, but inside each entry of previousAttempts the buildTextObservationFields spread keeps the raw textPreview/textHash/textFingerprint names. Consumers correlating attempts by fingerprint must account for two different field names for the same concept.

• The cron/isolated-agent/run.ts change (using sessionId as the registerAgentRunContext key and the related unbounded runContextById growth for CLI-cron paths) was flagged in prior review threads and remains unresolved.

Confidence Score: 3/5

• Safe to merge for runtime behavior (no logic changes), but the observation log schema has two consistency defects and an unresolved map-growth issue in the cron path that should be addressed before the log format is treated as stable.
• Runtime behavior is untouched — all changes are in logging/observation code and cannot cause regressions for users. However, the log schema shipped by this PR has a field-name inconsistency (errorFingerprint vs textFingerprint across attempt levels) and a log-level gap (info instead of warn for model-fallback decisions) that will require a follow-up fix once downstream consumers start relying on the advertised jq filters. Additionally, the runContextById unbounded growth issue for CLI cron runs flagged in a prior thread remains unresolved.
• Pay close attention to src/agents/model-fallback-observation.ts (log level and field naming) and src/cron/isolated-agent/run.ts (map growth / runId semantics).

_{Last reviewed commit: 6fdc781}

@greptileai review
@codex review

@greptileai review
@codex review

Codex Review: Didn't find any major issues. Swish!

@greptileai review
@codex review

Split into stacked drafts:

• Agents: add embedded error observations #41336
• Agents: add fallback error observations #41337
• Logging: harden probe suppression for observations #41338

Leaving #41023 as draft for reference while the stack takes over review.

Superseded by stacked draft PRs: #41336, #41337, #41338.