A CI job passes an untrusted commit author name through an environment variable, then a later step builds a JSON webhook payload from it — what is still wrong?

## Taint does not end at the shell The environment-variable handoff is the right fix for one specific sink: the shell that parses the step's script. Once teams learn it, a predictable failure follows — the value is treated as "cleaned" and then flows onward. But a value is never clean in the abstract. It is safe *for a given parser*, and every parser it meets afterwards is a new decision. In this job the sequence is: the engine sets `AUTHOR` in the environment (safe), the shell expands `"$AUTHOR"` (safe), and the script pastes the expansion into a JSON document (not safe). The shell hands the bytes to the next program exactly as they arrived, which is what you asked for; the JSON parser on the other end then reads the attacker's quote character as the end of a string. ## What the injected value can do An author name of `x", "channel": "incident-war-room", "text": "all clear` turns a one-field message into a document with keys the pipeline never intended. Depending on the receiver, that means: - **Rewriting the message body**, so the recorded deployment says something false. - **Overriding a routing field** — a channel, a recipient, an environment name — so the record goes somewhere nobody watches, or somewhere it will cause noise and be muted. - **Setting a field the receiver acts on.** Deployment and chat webhooks frequently accept fields that trigger behaviour: a mention that pages an on-call engineer, a link that renders, a flag that marks a deploy approved or a run successful. - **Breaking the request entirely**, which is the benign case, and the one that gets the bug found by accident. Most duplicate keys are resolved last-wins by common parsers, but you should not reason about which one wins. Assume the attacker can set any field in the document. ## Why this is worse than "a cosmetic notification" The reflex is to say a chat message is not a security boundary. The asset here is not confidentiality or availability — it is **audit truth**. The deploy notification, the release record and the change log are what people reconstruct history from during an incident and what an auditor samples to show that changes were authorised. An attacker who can write arbitrary content into that stream can: - make a real deployment look like a routine one, or attribute it to someone else, defeating non-repudiation; - insert a plausible-looking entry for a change that never happened; - push the true entry out of view among forged ones. And the attacker position is cheap: an outside contributor sets the git author name locally, on any commit, with no account privileges at all. It travels through review — reviewers read diffs, not author fields — into the default branch, and the deploy job reads it there. ## The general rule and the concrete fix **Never build a structured document by string concatenation.** Not JSON, not YAML, not a query, not a manifest, not HTML in a build report. Use a serializer that takes the value as a discrete input and encodes it: - pass the value as a named argument to a JSON-building command-line tool rather than pasting it into a heredoc; - or write a few lines in whatever language the pipeline already has, building an object and serializing it; - or, where the shape is genuinely known and narrow, validate the value against an allowlist pattern at the boundary and reject anything else. Escaping quotes yourself is the wrong answer for the same reason it was wrong at the shell layer: you now own a model of another parser's grammar, including its escape sequences, unicode handling and control characters. ## Finding the other sinks When you fix one of these, look for the rest, because the same value usually has several destinations. Common secondary sinks in build pipelines: | Sink | What the value becomes | | --- | --- | | A JSON or YAML body built by pasting | Document structure | | A generated script or config file | Code, at the moment something runs it | | A query sent to a build database | Query syntax | | An HTML build report | Markup, then script in a reviewer's browser | | A container image tag or label | An argument to later tooling, parsed downstream | | A log line ingested by a log pipeline | A forged event in the log store | | A file path or directory name | A path, with traversal available | A review heuristic that catches most of it: grep the pipeline for every place an untrusted variable appears next to a quote character or a concatenation operator, then ask what parses the result. ## In an interview The answer that earns the question is "the handoff protects the shell, not the next parser" followed by naming the specific parser. Then get the asset right: this is an integrity and non-repudiation problem in the audit trail, not a leaked secret, and saying so shows you can reason about impact when nothing confidential moves.