Skip to content

delete() has no secret masker, and the delete bag CAN carry plaintext on the in-process rollback path #2007

Closed

Description

@go-to-k

Summary

Issue #1932 item 3 added SecretMaskingContext.maskSecrets to CreateContext
and UpdateContext. DeleteContext was deliberately left without it. This
issue records the residual, because the FIRST rationale written for that
exclusion was factually wrong and the corrected one is narrower than it sounds.

The bag CAN carry plaintext (the wrong rationale)

The shipped comment originally claimed every delete() call site hands the
provider a bag read from cdkd STATE, which holds the redacted
{{resolve:...}} expression. That is false. Trace:

  1. deploy-engine.ts CREATE writes
    properties: this.propertiesToRecord(resolvedProps, result) into the
    IN-MEMORY stateResources map — resolvedProps is RESOLVED PLAINTEXT.
  2. Redaction happens at the SAVE choke point: redactStateForPersist builds a
    NEW record per resource via scrubResourceRecord and never mutates the
    in-memory one.
  3. performRollback hands that same in-memory map to replayRollback.
  4. rollback-executor.ts's reverse-replacement delete arms read
    current.properties straight off it (current = stateResources[op.logicalId]).

So an in-process rollback can hand a provider delete() a bag carrying a
concrete secret. The comment now states the narrower true thing: no provider
delete() interpolates a property value into a log line today.

Why the capability was NOT added in that PR

Not effort — correctness. Threading it is not the mechanical change it is on
the create / update side:

  • The plaintext in current.properties was resolved by the deploy, whose
    bag lives in DeployEngine.perResourceSecrets.
  • RollbackExecutorContext has no secrets field.
  • The executor's own per-op map is built by re-resolving
    previousState.properties — the previous generation.

A delete-side masker bound to that per-op map would therefore miss exactly the
value it exists to catch whenever the two generations differ (a rotated secret,
or a property only the new generation carries). That is protection which fences
nothing, and it is worse than no capability at all, because its presence stops
the next author looking.

What doing it properly needs

  1. A secrets field on RollbackExecutorContext, populated from
    DeployEngine.perResourceSecrets in rollbackExecutorContext(), keyed per
    logical id — with a decision about which bag wins for an op whose previous
    and current generations resolved different values.
  2. DeleteContext extends SecretMaskingContext. DeleteContext lives in
    src/provisioning/region-check.ts while SecretMaskingContext lives in
    src/types/resource.ts, which imports DeleteContext back — so the shared
    base likely has to move to the src/deployment/secret-redaction.ts leaf
    (zero imports) to avoid a type-level cycle.
  3. Threading at the 10 plaintext-capable delete sites (5 in
    deploy-engine.ts, 5 in rollback-executor.ts).
    destroy-runner.ts's single site reads state from S3, which IS redacted, so
    it has nothing to mask.

Interim rule (already in the code and docs)

SecretMaskingContext and .claude/rules/providers.md both state it as a MUST:
before adding any delete() log line that interpolates a property value, thread
the capability first. Do not read the absence of maskSecrets on
DeleteContext as evidence that a delete bag is safe to print.

Note #1997's provider list was audited for create / update log lines only, not
for delete ones.

Session-fit: next (not this session) — needs new cross-component plumbing
(RollbackExecutorContext ← DeployEngine.perResourceSecrets) plus a design
call on which generation's bag wins, and it has zero consumers today /
Effort: ~3 h+ (the plumbing, a type move to break the cycle, 10 threading
points, unit matrix, and a review round)

Consumers found later, folded here rather than filed separately

The missing secrets field on RollbackExecutorContext (item 1 above) is the
precondition for more than the delete-side masker this issue was filed for.
Each row below is a distinct consumer blocked on that same plumbing.

  • rollback-executor.ts nested-stack parameter collapse -- DONE, and the
    premise recorded here when it was folded in was WRONG.
    It was filed as
    blocked on item 1 above; it was not. secrets is hoisted to the top of the
    revert arm and already in scope, with the resolved plaintext and the
    expression-bearing JOURNAL both in hand, so no new plumbing was needed --
    and item 1's "wrong generation" objection does not apply, because a rollback
    WANTS the previous generation's expressions, which is the generation it is
    reverting to. Fixed in the fix(secret-redaction): two child Parameters resolving to ONE plaintext collapse onto a single expression, so a nested-stack leaf persists its siblings version stage #2291 PR: the three resolution arms
    that feed the four withCurrentResourceSecrets binders now record from the
    journal under STATE_DERIVED_RULES, DESIRED side only. Left here rather
    than deleted so the wrong premise is visible instead of quietly removed.

  • s3-directory-bucket-provider.ts notEmptyRefusalMessage (the delete-path not-empty refusal): renders aws s3 rm s3://<recorded bucket name> with no masker, so a secret-derived bucket name prints on destroy. Found by the fix(provisioning): the ASG / ELBv2 deletion-protection remedy pastes a pre-rotation secret-derived name unmasked #4339 sweep of every renderDisableCommand caller. It is a delete-path site, so it waits on this issue's DeleteContext masker. Low severity: destroy-time, and only for a secret-derived directory bucket name.

Activity

  1. go-to-k commented on Aug 26, 2026

    @go-to-k
    OwnerAuthor

    Concrete instance found while landing #2178: custom-resource-provider.ts's DELETE-arm payload refusal

    Filing this as a checklist row here rather than as its own issue — the root cause is exactly the one
    this issue already owns (DeleteContext carries no maskSecrets, and the delete bag CAN carry
    plaintext), so a separate issue would split it.

    Dup-check: searched the open issue list for an existing delete-path masker root cause
    (mask / secret / redact / delete-path / DeleteContext / custom resource title scan over
    all open issues). HIT: this issue. #2179 is the destroy-path events store, a different sink;
    #2177 is the create/update logger.* sweep, which its own body scopes away from delete.

    The site

    CustomResourceProvider.delete() builds its handler request with
    ResourceProperties: this.stringifyProperties(properties) — i.e. the delete bag, which per this
    issue's trace is RESOLVED PLAINTEXT on the in-process rollback path. A handler routinely echoes
    ResourceProperties back in its response Data, and parseLambdaPayload stringifies that echo
    into its "Invalid Lambda response payload format" refusal when the payload does not match the
    expected shape.

    Before #2178 that refusal was handed maskerOrIdentity(undefined) on the delete path — an identity
    masker, which is precisely the "protection which fences nothing ... worse than no capability at all,
    because its presence stops the next author looking" this issue's own rationale section warns about.
    #2178 removed it: the constant is now DELETE_PATH_UNMASKED = undefined, the refusal is split into
    a masked create/update arm and an explicitly unmasked DELETE arm, and the unmasked arm is recorded
    as a re-audited EXEMPT row in scripts/check-provider-secret-mask.ts. That row fails the build
    the moment the site becomes masked, so closing this issue automatically forces the exemption out.

    One correction to this issue's body

    The comment now states the narrower true thing: no provider delete() interpolates a property
    value into a log line today.

    That is still true of LOG lines, but it is now worth stating more precisely, because the
    custom-resource delete arm does interpolate a delete-derived value into a THROWN message:

    • the value is the handler's echoed payload, which is delete-bag-derived;
    • it reaches no logger today, because the sole caller wraps the parse in
      try { … } catch { this.logger.debug('Lambda payload parse failed for …') }, which discards the
      error object without reading error.message;
    • so exposure today is nil, and it is one logger.debug(error.message) away from being a live
      leak
      . Swallowing the parse failure is the only thing holding the line, and that is a smell in
      its own right rather than a safety property anyone chose.

    What closing this issue should also do

    Add to the "threading at the 10 plaintext-capable delete sites" checklist:

    • custom-resource-provider.ts: retire DELETE_PATH_UNMASKED, thread the real capability into
      invokeCustomResourceWithRetry / getCustomResourceResponse / parseLambdaPayload, collapse
      the two-arm refusal back into one masked arm, and drop the custom-resource-provider.ts
      parsed row from EXEMPT in scripts/check-provider-secret-mask.ts (the exemption audit
      will fail until it is dropped, so this cannot be forgotten).
    • While there: decide whether that bare catch should surface the parse failure at all. If it
      ever starts logging error.message, the delete arm leaks — the critic classifies per SITE,
      not per PATH, so a single masked-looking site would cover both paths and hide it.
  2. added
    severity:mediumUndone: a capability is missing but a workaround exists, or it only shows under one condition
    effort:largeA new integ fixture must be written, or a behavior change / schema bump needing its own PR
    on Sep 5, 2026
  3. 68 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort:largeA new integ fixture must be written, or a behavior change / schema bump needing its own PRseverity:mediumUndone: a capability is missing but a workaround exists, or it only shows under one condition

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions