🔒 Aisle Security Analysis

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

1. 🟡 Under-redaction of sensitive objects due to overly permissive SecretRef detection

Description

src/config/redact-snapshot.ts introduces a new isSecretRefShape() type guard that treats any object with string source and string id as a SecretRef. This is weaker than the actual SecretRef schema/type in the codebase and can cause partial redaction of non‑SecretRef objects.

Why this can leak secrets:

• Real SecretRef is defined as exactly { source: "env"|"file"|"exec", provider: string, id: string } and is validated with strict schemas (.strict()) (see src/config/types.secrets.ts and src/config/zod-schema.core.ts).
• However, isSecretRefShape() accepts objects with any source string and no provider requirement, and it does not reject extra keys.
• When a path is marked sensitive===true and the value is an object, redaction now does:
  • If isSecretRefShape(value) → redact only id (preserve the rest)
  • Else → collect all nested strings and replace the entire object with the sentinel

This creates a regression for sensitive fields that allow arbitrary objects. For example, channels.googlechat.serviceAccount is marked sensitive and allows an arbitrary object (z.record(z.string(), z.unknown())) in addition to SecretRef. If a user stores a service account-like object that happens to include top-level source and id keys plus other secret material (e.g., private_key), the new logic will:

• redact only id
• keep other properties intact in the structured response
• and (because collectSensitiveStrings() also short-circuits on the same shape) fail to collect/replace those nested secret strings in result.raw

Vulnerable code:

function isSecretRefShape(value: Record<string, unknown>) {
  return typeof value.source === "string" && typeof value.id === "string";
}

​// ...
if (hints[candidate]?.sensitive === true && !Array.isArray(value)) {
  const objectValue = value as Record<string, unknown>;
  if (isSecretRefShape(objectValue)) {
    result[key] = redactSecretRef(objectValue, values); // only redacts id
  } else {
    collectSensitiveStrings(objectValue, values);
    result[key] = REDACTED_SENTINEL;
  }
}

This is an under-redaction / sensitive data exposure risk whenever a sensitive object field can legitimately contain non-SecretRef objects with source and id keys (even coincidentally).

Recommendation

Tighten SecretRef detection to match the actual SecretRef contract, and avoid short-circuiting on lookalike objects.

Recommended changes:

1. Replace isSecretRefShape() with the existing strict guard from src/config/types.secrets.ts (or equivalent strict logic):

• require source to be one of env|file|exec
• require non-empty provider
• require no extra keys (or explicitly handle extras)

2. If an object has source/id but is not a strict SecretRef (e.g., missing provider or has extra keys), treat it as a normal sensitive object:

• recurse with collectSensitiveStrings()
• redact the entire object (REDACTED_SENTINEL)

Example hardening:

import { isSecretRef, type SecretRef } from "./types.secrets.js";

function isStrictSecretRef(value: unknown): value is SecretRef {
  return isSecretRef(value);
}

​// redactObjectWithLookup:
if (hints[candidate]?.sensitive === true && !Array.isArray(value)) {
  if (isStrictSecretRef(value)) {
    result[key] = redactSecretRef(value, values);
  } else {
    collectSensitiveStrings(value, values);
    result[key] = REDACTED_SENTINEL;
  }
}

​// collectSensitiveStrings: optionally keep special casing only for strict SecretRef
if (isStrictSecretRef(obj)) {
  if (!isEnvVarPlaceholder(obj.id)) values.push(obj.id);
  return;
}

This prevents lookalike objects from being partially redacted and ensures result.raw redaction also covers all nested secret strings.

2. 🟡 Prototype pollution via restoreRedactedValues assigning attacker-controlled keys ("proto"/"constructor"/"prototype")

Description

restoreRedactedValues*() deep-walks an attacker-controlled object (e.g. parsed from JSON5 submitted to config.set/config.apply/config.patch, and now also parsed from snapshot.raw via shouldFallbackToStructuredRawRedaction) and copies keys into a fresh {} result object without blocking prototype keys.

Because result is a normal object, assigning to result["__proto__"] will invoke the __proto__ setter and mutate the prototype of result (prototype pollution on the returned/restored config object). This can introduce inherited properties that downstream code may read via property access or in checks, leading to unexpected behavior and potential validation/logic bypasses.

Vulnerable code:

const result: Record<string, unknown> = {};
for (const [key, value] of Object.entries(incoming as Record<string, unknown>)) {
  result[key] = value; // key may be "__proto__"
  ​// ...
}

New/expanded exposure in this diff:

• shouldFallbackToStructuredRawRedaction() now does JSON5.parse(redactedRaw) and passes the parsed object to restoreRedactedValues(...), increasing the chance that a crafted config containing prototype keys triggers this behavior during config.get snapshot redaction as well.

Impact depends on where the restored object flows next, but the prototype mutation is concrete and avoidable.

Recommendation

Block prototype keys (at minimum __proto__, constructor, prototype) anywhere you copy/merge untrusted objects.

A minimal fix is to reuse the project’s existing helper (isBlockedObjectKey) and skip these keys in both restore workers:

import { isBlockedObjectKey } from "./prototype-keys.js";

const result: Record<string, unknown> = {};
for (const [key, value] of Object.entries(incoming as Record<string, unknown>)) {
  if (isBlockedObjectKey(key)) continue;
  ​// existing logic...
  result[key] = value;
}

Optionally, also create accumulator objects with Object.create(null) to remove magic __proto__ behavior entirely:

const result: Record<string, unknown> = Object.create(null);

Apply the same guard anywhere else in this file that copies arbitrary keys into {} (e.g., redaction walkers), for defense in depth.

Analyzed PR: #32174 at commit 3cc83c8

_{Last updated on: 2026-03-02T23:51:20Z}

Greptile Summary

This PR fixes a raw-redaction overlap bug where a sensitive string value (e.g. "local" as a password) appeared elsewhere as a non-sensitive field value (e.g. gateway.mode: "local"), causing naïve replaceAll to corrupt the non-sensitive field and break config round-trips. The fix adds a two-layer defence: (1) restrict sensitive-value collection for SecretRef objects to only the id field (not structural fields like source/provider), and (2) after text-based redaction, probe the result via a suppressed restoreRedactedValues + isDeepStrictEqual round-trip; if the probe fails, fall back to JSON5.stringify of the structured (path-based) redacted config.

Key changes and observations:

• isSecretRefShape is a loose duck-type guard requiring only source: string and id: string. Any object at an explicitly sensitive: true schema path that happens to carry those two fields will have only id redacted — other sensitive fields on the same object would be silently omitted from both text and structured redaction. A stricter guard (e.g. verifying no unrecognised string fields exist) would limit this to genuine SecretRef values (see inline comment).
• The fallback comparison correctly uses isDeepStrictEqual from node:util (key-order-insensitive), addressing a potential concern with JSON.stringify-based comparison.
• The suppressRestoreWarnings module-level mutable flag is safe for synchronous code but is a code smell; all callers are currently synchronous so there is no real concurrency risk.
• When the fallback fires, original JSON5 comments and formatting are lost; this is acknowledged in the PR as an acceptable trade-off.

Confidence Score: 3/5

• Safe to merge for the described overlap scenario, but the loose isSecretRefShape duck-type guard introduces a theoretical under-redaction path for non-SecretRef objects at sensitive schema paths that carry additional sensitive fields.
• The core fix (round-trip fallback probe with isDeepStrictEqual) is sound and well-tested. The SecretRef-scoped collection correctly prevents over-redaction of structural fields. However, isSecretRefShape only checks for two specific string fields and could silently under-redact other sensitive data on objects that happen to share that shape — a tightened guard would make the heuristic unambiguous. No regressions in existing test coverage were introduced.
• src/config/redact-snapshot.ts — the isSecretRefShape guard at lines 27-31 and its usages in collectSensitiveStrings and redactObjectWithLookup need attention.

_{Last reviewed commit: 3cc83c8}

Landed via temp rebase onto main.

• Gate: pnpm -s vitest run src/config/redact-snapshot.test.ts
• Land commit: 3cc83c8
• Merge commit: bd4a082

Thanks @bmendonca3!

Codex Review: Didn't find any major issues. You're on a roll.

@greptile review latest commit, please.