fix: Quote app-env values in deploy-vps #56

ahmad · 2026-09-07T18:23:42Z

ahmad commented

2026-09-07 18:23:42 +00:00

Issue

Closes #54

Problem

deploy-vps writes app-env lines verbatim into the host .env and the remote step shell-sources it, so a value containing $, a backtick, \ or " is silently altered — or, for a backtick, executed on the host. The action's own DEPLOY_DOMAIN="…" lines have the same shape, and every consumer has to know to quote correctly (expiro works around it with single quotes).

Solution

The action now single-quotes every value it writes — its own lines and each app-env value — so the sourcing shell reads back exactly the bytes that were passed. A value the consumer already wrapped in matching quotes is written as given (the consumer owns that quoting), which keeps expiro's live workaround deploying unchanged. A line whose key is not a valid variable name fails the deploy, naming only the key. The README stops teaching the KEY="…" shape.

Review notes

Start with the "Write the runtime environment file" step. The sourcing shell is the effective parser — compose also reads the file, but the sourced process environment takes precedence in compose interpolation. Verified with a local matrix that extracts the exact run: block from the YAML and round-trips hostile bytes through set -a; . ./file (17/17, plus a negative control showing the old shape corrupts); details in the plan comment on #54.

Risks and trade-offs

Honouring pre-quoted values means a consumer who double-quotes keeps the old expansion semantics for that value — deliberate, documented, and what makes the change safe to roll out without coordinating consumers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

### Issue Closes #54 ### Problem deploy-vps writes app-env lines verbatim into the host `.env` and the remote step shell-sources it, so a value containing `$`, a backtick, `\` or `"` is silently altered — or, for a backtick, executed on the host. The action's own `DEPLOY_DOMAIN="…"` lines have the same shape, and every consumer has to know to quote correctly (expiro works around it with single quotes). ### Solution The action now single-quotes every value it writes — its own lines and each app-env value — so the sourcing shell reads back exactly the bytes that were passed. A value the consumer already wrapped in matching quotes is written as given (the consumer owns that quoting), which keeps expiro's live workaround deploying unchanged. A line whose key is not a valid variable name fails the deploy, naming only the key. The README stops teaching the `KEY="…"` shape. ### Review notes Start with the "Write the runtime environment file" step. The sourcing shell is the effective parser — compose also reads the file, but the sourced process environment takes precedence in compose interpolation. Verified with a local matrix that extracts the exact `run:` block from the YAML and round-trips hostile bytes through `set -a; . ./file` (17/17, plus a negative control showing the old shape corrupts); details in the plan comment on #54. ### Risks and trade-offs Honouring pre-quoted values means a consumer who double-quotes keeps the old expansion semantics for that value — deliberate, documented, and what makes the change safe to roll out without coordinating consumers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

ahmad added 1 commit

2026-09-07 18:23:42 +00:00

fix: Quote every value written to the host env file

PR / validate (pull_request) Successful in 17s

Details

a676e1b580

The remote step shell-sources .env, so the old KEY="value" shape left
dollar signs, backticks and backslashes live in the host shell: a
generated secret containing any of them was silently altered, and a
backtick executed on the host. Single-quote every value instead — the
action's own lines and each app-env value — so the container sees the
bytes that were passed.

A value the consumer already wrapped in matching quotes is written as
given, keeping existing workarounds (ahmad/expiro#54) deploying
unchanged. A line whose key is not a valid variable name now fails the
deploy, naming only the key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

ahmad commented

2026-09-07 18:25:09 +00:00

Self-verification against the acceptance criteria, via the local matrix (extracts the exact run: block from the YAML in this branch and round-trips it through set -a; . ./file, the remote step's consumption shape):

Hostile bytes preserved — app-env values containing $, backticks, \, ", ' (also $(…), ${…}, leading/trailing spaces, empty) read back byte-identical after sourcing. 10/10 comparisons pass; a negative control confirms the old KEY="…" shape corrupts the same bytes (pa$$w0rd\echo BOOM`→paw0rdBOOM`).
Consumer-supplied quotes defined and documented — a value wrapped in matching quotes is written verbatim (not doubly quoted) and sources back to the unquoted content; documented on the input and in the README. Bonus guard: a line without = or with an invalid key fails the deploy, naming only the key — verified the rejection message does not contain the value.
The action's own lines — DEPLOY_DOMAIN=ex$mple.com and IMAGE_TAG=v1.0.0-(backtick)whoami(backtick) survive byte-for-byte through the same path.

CI is green on this head. Marking ready.

Self-verification against the acceptance criteria, via the local matrix (extracts the exact `run:` block from the YAML in this branch and round-trips it through `set -a; . ./file`, the remote step's consumption shape): 1. **Hostile bytes preserved** — app-env values containing `$`, backticks, `\`, `"`, `'` (also `$(…)`, `${…}`, leading/trailing spaces, empty) read back byte-identical after sourcing. 10/10 comparisons pass; a negative control confirms the old `KEY="…"` shape corrupts the same bytes (`pa$$w0rd\`echo BOOM\`` → `pa<pid>w0rdBOOM`). 2. **Consumer-supplied quotes defined and documented** — a value wrapped in matching quotes is written verbatim (not doubly quoted) and sources back to the unquoted content; documented on the input and in the README. Bonus guard: a line without `=` or with an invalid key fails the deploy, naming only the key — verified the rejection message does not contain the value. 3. **The action's own lines** — `DEPLOY_DOMAIN=ex$mple.com` and `IMAGE_TAG=v1.0.0-`(backtick)`whoami`(backtick) survive byte-for-byte through the same path. CI is green on this head. Marking ready.

ahmad changed title from ~~WIP: fix: Quote app-env values in deploy-vps~~ to fix: Quote app-env values in deploy-vps

2026-09-07 18:25:10 +00:00

ahmad_bot approved these changes

2026-09-07 18:46:06 +00:00

ahmad_bot left a comment

Approved. The quoting is correct — I round-tripped it rather than reading it — and the shape of the change is right for a shared action. One finding about how much of #54 this actually closes today, inline.

sq() does what it claims. I extracted the function and round-tripped hostile values through printf → file → set -a; . file:

  OK   a$B
  OK   back`tick`
  OK   quote"dq
  OK   single'q
  OK   back\slash
  OK   pg://u:p$w'd@h/db
  OK   plain

Every one comes back byte-identical, including a realistic Postgres URL carrying both $ and '. The '\'' replacement is the right idiom and the one people usually get subtly wrong. The old double-quoted shape, as a control, does not survive the same test — $B disappears and a backtick runs.

The shell around it is sound too, and the details that would have broken it are handled: the step is shell: bash so ${v//…} and local are available; set -euo pipefail is on, so emit_app_env's return 1 genuinely fails the step rather than writing a partial .env; and [ -z "${APP_ENV}" ] && return 0 is exempt from set -e because it is not the final command in its && list — which is the one place this pattern usually goes wrong.

Failing on a malformed key, and naming only the key, is the right call. A line that is not KEY=VALUE would otherwise be sourced as whatever it happens to be, and echoing the value back would put a secret in the job log.

Finding — the passthrough leaves four of six consumers exactly where they were. Inline.

CI green on a676e1b5 (PR / validate).

Approved. The quoting is correct — I round-tripped it rather than reading it — and the shape of the change is right for a shared action. One finding about how much of `#54` this actually closes today, inline. **`sq()` does what it claims.** I extracted the function and round-tripped hostile values through `printf` → file → `set -a; . file`: ``` OK a$B OK back`tick` OK quote"dq OK single'q OK back\slash OK pg://u:p$w'd@h/db OK plain ``` Every one comes back byte-identical, including a realistic Postgres URL carrying both `$` and `'`. The `'\''` replacement is the right idiom and the one people usually get subtly wrong. The old double-quoted shape, as a control, does not survive the same test — `$B` disappears and a backtick runs. **The shell around it is sound too**, and the details that would have broken it are handled: the step is `shell: bash` so `${v//…}` and `local` are available; `set -euo pipefail` is on, so `emit_app_env`'s `return 1` genuinely fails the step rather than writing a partial `.env`; and `[ -z "${APP_ENV}" ] && return 0` is exempt from `set -e` because it is not the final command in its `&&` list — which is the one place this pattern usually goes wrong. **Failing on a malformed key, and naming only the key, is the right call.** A line that is not `KEY=VALUE` would otherwise be sourced as whatever it happens to be, and echoing the value back would put a secret in the job log. **Finding — the passthrough leaves four of six consumers exactly where they were.** Inline. CI green on `a676e1b5` (`PR / validate`).

deploy-vps/action.yml

					
				@ -158,0 +185,4 @@

				              echo "app-env: not a KEY=VALUE line with a valid key: '${key}'" >&2

				              return 1

				            fi

				            case "${val}" in

This branch is what makes the change safe to roll out — and also what means most consumers gain nothing from it yet.

I checked how every consumer currently passes app-env on main:

repo	shape	effect after this
`suhba/suhba-web`	raw (`KEY=${{ … }}`)	newly protected by `sq`
`ahmad/expiro`	single-quoted	verbatim, and single quotes are already safe
`ahmad/portfolio`	double-quoted	verbatim — unchanged, still expands
`bayan/kidpoints`	double-quoted	unchanged
`ahmad/trip`	double-quoted	unchanged
`suhba/khitta`	double-quoted	unchanged

The four in the bottom rows carry exactly the values this is about — JWT_SECRET, MINIO_SECRET_KEY, MONGO_URL, ADMIN_PASSWORD, FORGEJO_OIDC_CLIENT_SECRET, DATABASE_URL, TURNSTILE_SECRET_KEY, MAILTRAP_TOKEN — generated credentials, which is where $ and backticks actually come from. And they use the double-quoted shape because it is what the README taught until this PR.

So the risks section is right that honouring pre-quoted values makes the rollout safe, and it understates the consequence: #54's hazard stays open wherever the old shape is still in use, which is most places, and nothing surfaces that. Someone reading "deploy-vps quotes app-env values itself" would reasonably conclude the fleet is covered.

I would not change the passthrough — breaking those four is worse — but it is worth making the gap visible rather than waiting for a rotation to produce a mystery. One line here turns it into something each repo sees on its next deploy:

              \"*\") echo "::warning::app-env: ${key} arrived double-quoted; the value is written verbatim and the host shell will expand \$, backticks and backslashes in it. Pass it raw to have deploy-vps quote it." >&2
                     printf '%s=%s\n' "${key}" "${val}" ;;

with the single-quoted case staying silent, since it is already correct. That, plus a follow-up to convert the four repos, is what actually closes #54 — this PR is the half that makes those conversions safe to do one at a time.

This branch is what makes the change safe to roll out — and also what means most consumers gain nothing from it yet. I checked how every consumer currently passes `app-env` on `main`: | repo | shape | effect after this | |---|---|---| | `suhba/suhba-web` | raw (`KEY=${{ … }}`) | **newly protected** by `sq` | | `ahmad/expiro` | single-quoted | verbatim, and single quotes are already safe | | `ahmad/portfolio` | double-quoted | verbatim — **unchanged, still expands** | | `bayan/kidpoints` | double-quoted | unchanged | | `ahmad/trip` | double-quoted | unchanged | | `suhba/khitta` | double-quoted | unchanged | The four in the bottom rows carry exactly the values this is about — `JWT_SECRET`, `MINIO_SECRET_KEY`, `MONGO_URL`, `ADMIN_PASSWORD`, `FORGEJO_OIDC_CLIENT_SECRET`, `DATABASE_URL`, `TURNSTILE_SECRET_KEY`, `MAILTRAP_TOKEN` — generated credentials, which is where `$` and backticks actually come from. And they use the double-quoted shape because it is what the README taught until this PR. So the risks section is right that honouring pre-quoted values makes the rollout safe, and it understates the consequence: `#54`'s hazard stays open wherever the old shape is still in use, which is most places, and nothing surfaces that. Someone reading "deploy-vps quotes app-env values itself" would reasonably conclude the fleet is covered. I would not change the passthrough — breaking those four is worse — but it is worth making the gap visible rather than waiting for a rotation to produce a mystery. One line here turns it into something each repo sees on its next deploy: ```bash \"*\") echo "::warning::app-env: ${key} arrived double-quoted; the value is written verbatim and the host shell will expand \$, backticks and backslashes in it. Pass it raw to have deploy-vps quote it." >&2 printf '%s=%s\n' "${key}" "${val}" ;; ``` with the single-quoted case staying silent, since it is already correct. That, plus a follow-up to convert the four repos, is what actually closes `#54` — this PR is the half that makes those conversions safe to do one at a time.

ahmad reviewed

2026-09-07 19:13:50 +00:00

ahmad left a comment

Agreed, and making the gap concrete rather than visible-in-prose: after this merges and the tag moves, I am filing one conversion issue per remaining double-quoting consumer — ahmad/portfolio, bayan/kidpoints, ahmad/trip, suhba/khitta — to pass raw values, with ahmad/expiro#65 (drop the single-quote workaround) as the template. The workspace knowledge file already records that this PR alone is not "the fleet is covered"; the four issues are what closes that gap issue by issue.