redactSecrets fails open for role('secret') fields reachable through union/intersect/tuple/transform/lazy schemas

[nectopower]

Summary

packages/settings/settings/src/redact.ts is the code that strips role('secret') fields from a settings value before it crosses a wire boundary — the package README states "every wire surface MUST pass it." The walker only recurses through object, dict, and array schema nodes. Any other schemastery type falls into the default branch, which returns the value verbatim, with no error and nothing recorded — the exact behavior the file's own comment already flags:

default:
  // TODO(settings-wire-redaction): Fail closed instead — a secret reachable
  // only through a union, intersection, or transform is returned verbatim
  // here, with nothing recording that it was missed.
  return value

Scope is bigger than the TODO says

Cross-checking against vendor/schemastery/src/index.ts, the container types are: object, dict, array, tuple, union, intersect, transform, lazy (everything else — string, number, boolean, const, any, bitset, function, is, never — is a true leaf with no child schema, so passthrough there is safe). The TODO names union, intersect, and transform, but tuple and lazy have the identical gap and aren't mentioned:

• tuple carries member schemas in .list, exactly like union/intersect — a secret at a fixed tuple position leaks the same way.
• lazy (used for self-referential/recursive schemas — e.g. a tree-shaped config) resolves through a deferred .builder() — never visited by the walker at all.

Is this exploitable today?

Not in any shipped configuration as far as I checked — every current role('secret') usage in the repo (web-search-deepseek, llm-pi-ai, etc.) is a plain field on a plain object, and the multi-provider adapter configs use role('credential-ref') (an env-var name, not the secret itself) rather than nesting a real secret inside a union. So this is a latent gap, not an active leak — but it's a silent one: the CI coverage gate is per-file 100% line coverage, and the default branch is already exercised by ordinary leaf fields (z.string(), z.number()), so the branch "runs" without the dangerous scenario (a secret nested under union/tuple/transform/lazy) ever being tested. In a plugin-first architecture where third-party settings schemas are expected, this seems worth closing before it's someone's real config shape.

Suggested fix

The default branch can't simply throw on everything — that would break every leaf type. The fix needs two parts:

1. Recurse into the missing containers: tuple (per-index, like array but heterogeneous), union/intersect (merge member schemas into one synthetic shape before walking, rather than walking each member against the raw value separately — walking separately reintroduces the leak, because object's "pass through keys my own schema doesn't declare" behavior treats a sibling branch's secret field as ordinary unknown data), transform/lazy (transparent passthrough to .inner, with lazy bounded by actual value depth rather than declared-schema depth, since a self-referential lazy schema will not otherwise terminate).
2. Only then make the genuine default case (a schema type this list doesn't recognize) throw instead of silently passing the value through.

I have a working patch (with new tests for all five container types plus the fail-closed default, at 100% branch coverage on the file) if a maintainer wants to see the approach — happy to paste the diff here, since I understand external PRs aren't being accepted right now.

[argszero]

Verified every claim against rc.2 — your container-type list is exact, and this is a second independent confirmation of a finding the repo already carries.

1. Family context: this is #2445's redactSecrets finding, re-derived from source

The community security review #2445 (35 findings) flagged "redactSecrets fails open" at redact.ts:86-91. Your report independently re-derives the same gap — good triangulation. The walker really does only recurse object/dict/array (redact.ts:57-90), the default branch returns the value verbatim with the TODO comment (:86-91) naming union/intersect/transform, and tuple/lazy have the identical gap. schemastery's type discriminator confirms the container set (vendor/schemastery/src/index.ts:408-433, lazy at :525).

2. One more silent-spread risk than the TODO implies

The JSDoc above redactSecrets (:96-99) tells schema authors a secret "buried inside a union branch or transform is not reachable and must not be modeled that way." That is a behavioral contract punishing legitimate schema composition — it makes the gap self-perpetuating: a plugin author who builds a union (for a perfectly valid multi-shape setting) and declares a secret field inside one branch gets silent passthrough, and the "documented" response is to blame their modeling. The redaction contract should be structural (total over schema types), not author-discipline. Until then, role('credential-ref') (env-var name, not the secret) is the correct plugin convention — it survives every schema shape.

3. Defense-in-depth: a declared-vs-visited invariant (complementary to your two-part fix)

Your fix (recurse tuple/union/intersect/transform/lazy, then fail-closed default) closes the known types. A cheap extra layer catches unknown ones: before the value walk, run a pure schema walk (no value) that enumerates every declared secret path — recursing through ALL container types including lazy (bounded by a visited set on the builder, since self-referential schemas won't terminate otherwise). Then assert every declared path was visited during the value walk; a declared-but-unvisited path (e.g. a secret under a schemastery type added after this fix) fails loudly instead of leaking. The schema walk is trivial to make total because it binds no value; the value walk then just has to keep up. Combined with per-file 100% coverage, this turns the silent gap into a compile-time-style failure on the next container type, not the one after someone's config shape hits it.

4. Your exploitability assessment is right — and worth saying loudly

Only one real role('secret') usage exists today (web-search-deepseek apiKey at web-search-deepseek/src/index.ts:64, a plain object field), and the wire surfaces are settings.describe({ redactSecrets: true }) calls in api-proxy (api-proxy.ts:1929/3168). So: latent, not active — but the plugin-first architecture means third-party settings schemas are the expected next consumer, which is exactly the population that will hit union/tuple/lazy shapes. The 100%-line-coverage gate actively masks it: the default branch is exercised by every leaf (z.string(), z.number()), so the dangerous shape is never tested. Your five-container regression suite closes that hole in CI.

Please paste the patch — it's the family's PR-ready candidate for when the channel reopens.

[nokkies]

Your analysis matches a fix I have been running for a while, including the two node kinds the TODO omits — you are right that tuple and lazy have the identical gap and that the TODO understates its own scope.

The shape that works:

Walk the containers that can be walked. tuple recurses per index through .list; union and intersect need two passes over .list — one to enumerate every declared secret position against the original value so the set flags report what was actually held, then one applying each member's own walk, since each container walk deletes exactly the positions it declared.

Fail closed on the rest. Anything the walker cannot prove secret-free throws rather than returning the value:

default:
  if (SAFE_LEAF_TYPES.has(node.type ?? '')) return value
  throw new UnprovableSchemaError(node.type ?? 'unknown', path)

The allowlist is the important half, and it is exactly the partition you drew: string, number, boolean, bitset, const, any, never, function, is are true leaves with no child schema, so passthrough is safe there — a role('secret') on the node itself is handled before the type switch. transform, lazy, and anything unknown throw.

Failing closed rather than redacting wholesale is deliberate: a namespace whose schema hides a secret behind an unverifiable node should be refused on the wire entirely, so the schema gets restructured instead of quietly serving a value nobody has proved is clean.

On exploitability

Your assessment is right and worth keeping in the record: no shipped configuration reaches it today, because every current role('secret') is a plain field on a plain object and the multi-provider configs use role('credential-ref') rather than nesting a real secret in a union.

That is an argument about timing, not severity. The cost of the current default is that the first schema to nest a secret under a union leaks it silently, with nothing recorded — and whoever writes that schema has no reason to suspect the walker will not follow them there. Fixing it while it is still latent is much cheaper than after.

[nokkies]

A branch carrying a fix for this is available, based directly on cd5ef8148 (0.1.2-alpha.1) so it applies with git am -3:

https://github.com/nokkies/dsh-upstream-patches/tree/fix/settings-wire-redaction-fail-closed

It makes the redaction walker fail closed: it follows object/dict/array/tuple containers and every union/intersect member, and a transform, lazy, or unknown node kind now throws UnprovableSchemaError rather than passing a shape it cannot prove secret-free. The error names the node kind and path.

The commit also rewrote the package README limitation entry this lifts, but that is left out of the branch: the two READMEs have diverged into different documents, and the wording is yours to choose. The entry it answers begins "redactSecrets is not a proven wire boundary".

Offered as-is, no attribution wanted. Take, adapt, or ignore it freely.

[nokkies]

The branch is rebased onto current master.

It was cut against cd5ef81 (0.1.2-alpha.1), and this package has been restructured since, so the old tip no longer applies — git apply --3way reports conflicts in src/index.ts and tests/redact.spec.ts. It now sits directly on dd6322d, and git am -3 is clean again:

https://github.com/nokkies/dsh-upstream-patches/tree/fix/settings-wire-redaction-fail-closed

Very little moved. redact.ts, which carries the fix, had no upstream changes between alpha.1 and alpha.3, so all 102 of its lines apply unchanged. Both conflicts were imports: deepEqualJson/deepFreeze now sit beside the redact import in index.ts, and the test imported settingsNamespace, which is now the private parseSettingsNamespace — it was never used, so it is dropped rather than reinstated.

Scope is unchanged: four files, src and tests only. The README limitation entry this lifts is still left out, for the same reason as before — the wording is yours to choose.

One caveat: this was a force-update of the same branch rather than a new one, so a clone taken before today needs a re-fetch rather than a pull.

Offered as-is, no attribution wanted.