Skip to content

[Bug] Refactor deploy/destroy to pass values via env vars, never interpolate into sh -c script text #4

Description

@sjohnston1972

Part of the command-injection hardening parent issue. This is the structural fix — the two validation sub-issues are defence-in-depth on top of it.

Background

custom-tools.ts runs docker run ... sh -c "<script>" where <script> is a JS template-literal string with tool inputs baked directly into it (RG names, locations, deployment names, tag pairs, regions). Because those values become part of the script text, any shell metacharacter in them is interpreted as code.

Problem / Goal

Refactor the deploy/destroy handlers so dynamic values are passed as environment variables to the container and referenced as "$VAR" inside the script — never interpolated into the script text. Docker passes -e NAME=value values as literal strings (the value is not parsed by any shell before the container's sh starts, and inside the script "$NAME" expands to the literal value, not re-parsed as code). This makes injection structurally impossible for these fields.

Where to look

  • runBicepDeploy (~750): the azCmd construction (~810-813) and tagBlock (~827-862), then the final dockerArgs (~877-894) and shellScript (~864-875).
  • runDestroy (~604): lines[] construction (~626-700) and dockerArgs (~703-719).
  • runDestroyAws (~1250): lines[] (~1273-1333) and finalArgs (~1360-1388).
  • Credentials are already passed this way (-e AZURE_CLIENT_ID=...) — follow the same shape, but note the separate secret-hygiene parent issue about passing values on the argv.

Suggested approach

  1. For each handler, move dynamic values into -e flags: e.g. -e DEPLOY_RG="${input.resource_group_name}", -e DEPLOY_LOCATION=..., -e DEPLOY_NAME=....
  2. Rewrite the script body to reference them quoted: az deployment group create --resource-group "$DEPLOY_RG" --template-file "$ENTRY" --name "$DEPLOY_NAME".
  3. For tag pairs, pass them as a delimited env var (e.g. newline-separated TAG_PAIRS) and loop in-shell, or pass each as TAG_1, TAG_2, ...; build the az tag update --tags args from the env inside the script. Do not interpolate '${k}=${v}' into the script text.
  4. For JMESPath filters in destroy: build the query from env vars inside the shell too, or keep the query construction but only after sub-issue [Bug] Validate scalar inputs (location, resource_group_name, deployment_name, region) before shell use #2 validation guarantees safe characters. Prefer building it from $VAR.
  5. Keep the entry-file /work/... path passed via env as well.

Acceptance criteria

  • The sh -c script strings for all four handlers contain no interpolated user/model input — only static text and $VAR references.
  • Legitimate deploy/destroy flows still succeed end-to-end.
  • With validation temporarily disabled, a metacharacter-laden resource_group_name is treated as an inert string (the az command errors on a bad RG name) rather than executing an injected command.

Testing

  • Snapshot/inspect the generated shellScript for a crafted input and assert no injected substring appears as executable text.
  • Re-run an existing manual-deploy/ scenario (e.g. manual-deploy/hub-spoke) to confirm legitimate deploys still work.

Out of scope

  • The regex validators (separate sub-issues) — still worth adding as belt-and-braces.
  • Secret-passing hygiene (separate parent issue).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions