Workflow secrets are text-substituted into the command string, so leaking one is the natural thing to write#217
○Openccantynz opened this issue 5d ago
ccantynzAuthorcommented 5d ago
The trap
${{ secrets.NAME }} is resolved by text substitution into the step's command string (src/lib/workflow-runner.ts:116, substitution at ~:514–520), not passed to the child process as an environment variable.
The containment side of this is deliberate and good: workflow-runner.ts:105 isolates the child environment so the platform's own secrets never reach step processes. That part should not change.
The problem is what it makes callers write. A user wiring up a deploy key naturally writes:
...and the secret is now in the command line. Anything that echoes the command, any step under set -x, and any curl -v puts the plaintext key straight into the run log, which is readable by anyone with repo read access.
The safe form requires knowing about the trap — pipe via stdin, or assign to a shell variable on its own line. The dangerous form is the obvious one, and it looks completely correct.
Why it's worth acting on
This came from an integrator (Zoobicon) actively designing a deploy workflow that holds a vpk_ key. They caught it only because it was flagged in conversation; their first design interpolated the key directly. Their words: "the current shape makes leaking a key the natural thing to write and safety the thing you have to know about."
Nobody has leaked a key yet as far as I know. This is a "well-placed to catch someone" report, not an incident.
Options, roughly in order of appeal
Env-var passing as the default. Inject declared secrets into the step's environment (Bun.spawn's env) and let ${{ secrets.NAME }} resolve to $NAME, keeping text substitution only where a value must appear inline. Fixes the trap for the common case without the author having to know anything. Needs care to stay compatible with the existing environment-isolation guarantee.
Log redaction. Scan step output for known secret values and mask them before persisting. Cheap, defence-in-depth, but strictly a mitigation — the value still transits argv, which is visible to other processes on the box via /proc.
Docs + a lint. Warn when a workflow interpolates a secret into a run: string. Weakest, but a real improvement over silence, and it composes with either of the above.
(1) and (2) together are the honest fix. (3) alone would be settling.
Not doing this unilaterally
Changing how secrets reach step processes touches the CI security model and could break existing workflows that rely on inline substitution. That is an owner decision, not a drive-by change — filing it so it doesn't evaporate into a chat log.
Found while answering integration questions that also produced PR #5559.
c comment · e edit title · x close/reopen · ? shortcuts
The trap
${{ secrets.NAME }}is resolved by text substitution into the step's command string (src/lib/workflow-runner.ts:116, substitution at ~:514–520), not passed to the child process as an environment variable.The containment side of this is deliberate and good:
workflow-runner.ts:105isolates the child environment so the platform's own secrets never reach step processes. That part should not change.The problem is what it makes callers write. A user wiring up a deploy key naturally writes:
- run: curl -H "Authorization: Bearer ${{ secrets.DEPLOY_KEY }}" https://target/deploy...and the secret is now in the command line. Anything that echoes the command, any step under
set -x, and anycurl -vputs the plaintext key straight into the run log, which is readable by anyone with repo read access.The safe form requires knowing about the trap — pipe via stdin, or assign to a shell variable on its own line. The dangerous form is the obvious one, and it looks completely correct.
Why it's worth acting on
This came from an integrator (Zoobicon) actively designing a deploy workflow that holds a
vpk_key. They caught it only because it was flagged in conversation; their first design interpolated the key directly. Their words: "the current shape makes leaking a key the natural thing to write and safety the thing you have to know about."Nobody has leaked a key yet as far as I know. This is a "well-placed to catch someone" report, not an incident.
Options, roughly in order of appeal
Bun.spawn'senv) and let${{ secrets.NAME }}resolve to$NAME, keeping text substitution only where a value must appear inline. Fixes the trap for the common case without the author having to know anything. Needs care to stay compatible with the existing environment-isolation guarantee./proc.run:string. Weakest, but a real improvement over silence, and it composes with either of the above.(1) and (2) together are the honest fix. (3) alone would be settling.
Not doing this unilaterally
Changing how secrets reach step processes touches the CI security model and could break existing workflows that rely on inline substitution. That is an owner decision, not a drive-by change — filing it so it doesn't evaporate into a chat log.
Found while answering integration questions that also produced PR #5559.