fix: Refuse app-env values a compose parse cannot survive #61

ahmad · 2026-09-08T06:17:34Z

ahmad commented

2026-09-08 06:17:34 +00:00

Issue

Closes #60

Problem

emit_app_env escapes an embedded ' as '\'' — byte-exact for the sourcing shell, unreadable for compose's dotenv parser, which exits 1 and abandons the whole env file, failing the deploy at compose with an error pointing nowhere near the cause. Separately, the honour-consumer-quoting pass-through has had no user since the fleet-wide raw conversion closed, and its value-that-looks-quoted edge writes an unquoted line.

Solution

Validation becomes its own pass before anything is emitted: a valid key and no ' in the value, refusal naming only the key. Every value then goes through sq() uniformly — the pass-through branch is deleted. Input description and README updated (raw values, one forbidden byte).

Review notes

The verification matrix was rebuilt for the new semantics: hostile bytes minus ' round-trip byte-for-byte; a double-quoted value now keeps its quotes as literal bytes; a value containing ' (including the old single-quote workaround shape) is refused naming the key, with the value absent from both stderr and the file; and the emitted file passes a live docker compose config parse. 12/12.

Risks and trade-offs

A consumer branch still passing pre-quoted values would now get literal quote bytes (double) or a refusal (single) — the census says none exists on any main. Residual, out of scope here: the action's own inputs (grafana token et al.) go through sq() with no ' check, so the compose-side exposure persists for them; flagged on the issue for a follow-up if the review agrees.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

### Issue Closes #60 ### Problem `emit_app_env` escapes an embedded `'` as `'\''` — byte-exact for the sourcing shell, unreadable for compose's dotenv parser, which exits 1 and abandons the whole env file, failing the deploy at compose with an error pointing nowhere near the cause. Separately, the honour-consumer-quoting pass-through has had no user since the fleet-wide raw conversion closed, and its value-that-looks-quoted edge writes an unquoted line. ### Solution Validation becomes its own pass before anything is emitted: a valid key and no `'` in the value, refusal naming only the key. Every value then goes through `sq()` uniformly — the pass-through branch is deleted. Input description and README updated (raw values, one forbidden byte). ### Review notes The verification matrix was rebuilt for the new semantics: hostile bytes minus `'` round-trip byte-for-byte; a double-quoted value now keeps its quotes as literal bytes; a value containing `'` (including the old single-quote workaround shape) is refused naming the key, with the value absent from both stderr and the file; and the emitted file passes a live `docker compose config` parse. 12/12. ### Risks and trade-offs A consumer branch still passing pre-quoted values would now get literal quote bytes (double) or a refusal (single) — the census says none exists on any main. Residual, out of scope here: the action's own inputs (grafana token et al.) go through `sq()` with no `'` check, so the compose-side exposure persists for them; flagged on the issue for a follow-up if the review agrees. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

ahmad added 1 commit

2026-09-08 06:17:34 +00:00

fix: Refuse app-env values a compose parse cannot survive

PR / validate (pull_request) Successful in 16s

Details

dd0175118d

sq()'s POSIX escaping of an embedded single quote reads back exactly
in the sourcing shell but makes compose's dotenv parser abandon the
whole file. Validation now runs as its own pass before anything is
emitted and refuses such a value naming only the key. The
honour-consumer-quoting branch is deleted: the fleet passes raw values
everywhere and the branch's only remaining behaviour was a footgun.

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

ahmad commented

2026-09-08 06:20:38 +00:00

Self-verification against the acceptance criteria:

A ' value fails naming the key, no partial app-env content — matrix kill cases: refusal exits nonzero, stderr names only the key, the value appears in neither stderr nor the file, and nothing from app-env is written (validation is a separate pre-emit pass). The emitted file from a passing run parses under a live docker compose config.
Current consumers unchanged — every fleet consumer passes raw single-line values without ' (the closed census); the matrix round-trips the hostile set byte-for-byte through set -a; . ./file.
Pass-through decision recorded and enacted — deleted, with the reason in the step comment and the PR body; a double-quoted value now keeps its quotes as literal bytes (matrix-pinned).

PR / validate green on dd01751, read back from the commit status. Marking ready.

Self-verification against the acceptance criteria: 1. **A `'` value fails naming the key, no partial app-env content** — matrix kill cases: refusal exits nonzero, stderr names only the key, the value appears in neither stderr nor the file, and nothing from app-env is written (validation is a separate pre-emit pass). The emitted file from a passing run parses under a live `docker compose config`. 2. **Current consumers unchanged** — every fleet consumer passes raw single-line values without `'` (the closed census); the matrix round-trips the hostile set byte-for-byte through `set -a; . ./file`. 3. **Pass-through decision recorded and enacted** — deleted, with the reason in the step comment and the PR body; a double-quoted value now keeps its quotes as literal bytes (matrix-pinned). `PR / validate` green on `dd01751`, read back from the commit status. Marking ready.

ahmad changed title from ~~WIP: fix: Refuse app-env values a compose parse cannot survive~~ to fix: Refuse app-env values a compose parse cannot survive

2026-09-08 06:20:38 +00:00

ahmad_bot approved these changes

2026-09-08 06:46:45 +00:00

Dismissed

ahmad_bot left a comment

Approved. This came from my report on suhba/suhba-docs#11, so I held it to the same standard rather than a lighter one: I extracted the step's script from the action at dd01751 and ran it, including a live docker compose parse of what it emits.

Every claimed behaviour, measured

valid values ($ ` " \ empty base64+/=)   rc=0, all bytes intact
value containing '                       rc=1, "the value of PASSWORD contains a single quote…"
consumer pre-single-quoted 'SECRET'      rc=1, same refusal
invalid key                              rc=1, pre-existing guard intact
consumer pre-double-quoted "SECRET"      rc=0, quotes kept as literal bytes
empty app-env                            rc=0

On the two refusals I checked the thing that matters most: the value appears in neither stderr nor the partial file. The validation pass runs to completion before the emit pass, so .env.deploy holds only DEPLOY_DOMAIN and IMAGE_TAG when the step aborts, and the step aborts — set -euo pipefail at the top carries the function's return 1 out of the { … } > .env.deploy group. Splitting validation from emission rather than checking inline is what makes that true, and it is the right shape.

The emitted file survives a real compose parse, which is the whole point of the change:

A = plain          B = has $dollar and `backtick`
C = has "double quotes"    D = back\slash    F = base64+with/slash==

read back through docker compose config from the file the step wrote.

On deleting the pass-through, I widened the check rather than reusing the census. My earlier count covered a fixed list of thirteen repositories; this one walks every repository the bot can see — 21 of them, 11 deploy-vps consumers (Bayan/bayan-web, Bayan/kidpoints, Bayan/stem, ahmad/expiro, ahmad/larda, ahmad/namey, ahmad/portfolio, ahmad/trip, suhba/khitta, suhba/suhba-docs, suhba/suhba-web) — and finds 184 raw, 0 single-quoted, 0 double-quoted. So the branch really has no user and the behaviour change reaches nobody. One caveat the census cannot cover: that is main only, so an unmerged branch still carrying a pre-quoted block would meet the new behaviour on its first deploy after this lands — a refusal for single quotes, literal quote bytes for double.

The residual you flagged is real, and I confirmed it rather than agreeing on principle. With GRAFANA_PROM_TOKEN=tok'en:

step rc=0, no stderr
emitted line 7: GRAFANA_PROM_TOKEN='tok'\''en'
docker compose config -> rc=1, "unexpected character … in variable name"

So the action's own inputs reproduce exactly the failure this PR fixes for app-env, minus the named refusal: the step passes, and the deploy dies later at compose pointing at a line rather than a cause. DEPLOY_DOMAIN, IMAGE_TAG and COMPOSE_PROFILES are the same shape. Agreed it is out of scope for #60 — the fix is to run the same check over those four before the block, and the fact that it is a silent pass makes it worth the follow-up rather than leaving it as known-and-tolerated.

One note inline on a comment the change leaves behind.

Approved. This came from my report on `suhba/suhba-docs#11`, so I held it to the same standard rather than a lighter one: I extracted the step's script from the action at `dd01751` and ran it, including a live `docker compose` parse of what it emits. **Every claimed behaviour, measured** valid values ($ ` " \ empty base64+/=) rc=0, all bytes intact value containing ' rc=1, "the value of PASSWORD contains a single quote…" consumer pre-single-quoted 'SECRET' rc=1, same refusal invalid key rc=1, pre-existing guard intact consumer pre-double-quoted "SECRET" rc=0, quotes kept as literal bytes empty app-env rc=0 On the two refusals I checked the thing that matters most: the value appears in neither stderr nor the partial file. The validation pass runs to completion before the emit pass, so `.env.deploy` holds only `DEPLOY_DOMAIN` and `IMAGE_TAG` when the step aborts, and the step aborts — `set -euo pipefail` at the top carries the function's `return 1` out of the `{ … } > .env.deploy` group. Splitting validation from emission rather than checking inline is what makes that true, and it is the right shape. **The emitted file survives a real compose parse**, which is the whole point of the change: A = plain B = has $dollar and `backtick` C = has "double quotes" D = back\slash F = base64+with/slash== read back through `docker compose config` from the file the step wrote. **On deleting the pass-through, I widened the check rather than reusing the census.** My earlier count covered a fixed list of thirteen repositories; this one walks every repository the bot can see — 21 of them, 11 `deploy-vps` consumers (`Bayan/bayan-web`, `Bayan/kidpoints`, `Bayan/stem`, `ahmad/expiro`, `ahmad/larda`, `ahmad/namey`, `ahmad/portfolio`, `ahmad/trip`, `suhba/khitta`, `suhba/suhba-docs`, `suhba/suhba-web`) — and finds **184 raw, 0 single-quoted, 0 double-quoted**. So the branch really has no user and the behaviour change reaches nobody. One caveat the census cannot cover: that is `main` only, so an unmerged branch still carrying a pre-quoted block would meet the new behaviour on its first deploy after this lands — a refusal for single quotes, literal quote bytes for double. **The residual you flagged is real, and I confirmed it rather than agreeing on principle.** With `GRAFANA_PROM_TOKEN=tok'en`: step rc=0, no stderr emitted line 7: GRAFANA_PROM_TOKEN='tok'\''en' docker compose config -> rc=1, "unexpected character … in variable name" So the action's own inputs reproduce exactly the failure this PR fixes for `app-env`, minus the named refusal: the step passes, and the deploy dies later at compose pointing at a line rather than a cause. `DEPLOY_DOMAIN`, `IMAGE_TAG` and `COMPOSE_PROFILES` are the same shape. Agreed it is out of scope for #60 — the fix is to run the same check over those four before the block, and the fact that it is a *silent* pass makes it worth the follow-up rather than leaving it as known-and-tolerated. One note inline on a comment the change leaves behind.

deploy-vps/action.yml

					
				@ -174,3 +172,1 @@

				        # knows what (the error names only the key, never the value). A value

				        # the consumer already wrapped in matching quotes is written verbatim:

				        # the consumer owns that quoting (documented on the input).

				        # app-env lines are KEY=VALUE. The key must be a valid name and the

This new comment is accurate, but the one immediately above it (lines 160-167, on sq() itself) still ends:

# (docker compose also parses this file, but the sourced process environment
# takes precedence in its interpolation, so the shell's reading is the one
# that counts.)

That parenthetical is the belief #60 disproved. Compose's reading is not a secondary concern that the process environment papers over — a parse failure abandons the whole file before any interpolation happens, which is precisely the bug being fixed here. As it stands the file carries two comments ten lines apart that disagree about whether the compose parse matters, and the wrong one is the one attached to sq(), which is what a reader looking at the escaping will read first.

It is a comment, so nothing misbehaves at runtime — but this is the shared action every consumer's author reads to decide what is safe to pass, and that sentence is how a future change talks itself into loosening the escaping again. Worth folding into this PR since it is the same subject: replace the parenthetical with the fact, that both parsers read this file and only single quotes satisfy both, which is now why the value is checked at all.

This new comment is accurate, but the one immediately above it (lines 160-167, on `sq()` itself) still ends: # (docker compose also parses this file, but the sourced process environment # takes precedence in its interpolation, so the shell's reading is the one # that counts.) That parenthetical is the belief #60 disproved. Compose's reading is not a secondary concern that the process environment papers over — a parse failure abandons the whole file before any interpolation happens, which is precisely the bug being fixed here. As it stands the file carries two comments ten lines apart that disagree about whether the compose parse matters, and the wrong one is the one attached to `sq()`, which is what a reader looking at the escaping will read first. It is a comment, so nothing misbehaves at runtime — but this is the shared action every consumer's author reads to decide what is safe to pass, and that sentence is how a future change talks itself into loosening the escaping again. Worth folding into this PR since it is the same subject: replace the parenthetical with the fact, that both parsers read this file and only single quotes satisfy both, which is now why the value is checked at all.

ahmad changed title from ~~fix: Refuse app-env values a compose parse cannot survive~~ to WIP: fix: Refuse app-env values a compose parse cannot survive

2026-09-08 07:13:37 +00:00

ahmad added 1 commit

2026-09-08 07:14:06 +00:00

docs: Both parsers count for the env file

PR / validate (pull_request) Successful in 17s

Details

fe6f58082a

The sq() comment still claimed the sourced process environment makes
compose's reading moot — the belief #60 disproved: a compose parse
failure abandons the file before any interpolation happens.

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

ahmad dismissed ahmad_bot's review

2026-09-08 07:14:06 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

ahmad reviewed

2026-09-08 07:14:07 +00:00

ahmad left a comment

Taken — fixed in fe6f580: the sq() comment now states both readers count and why the single-quote refusal exists, instead of the precedence claim #60 disproved. Re-marking ready.

Taken — fixed in fe6f580: the sq() comment now states both readers count and why the single-quote refusal exists, instead of the precedence claim #60 disproved. Re-marking ready.

ahmad changed title from ~~WIP: fix: Refuse app-env values a compose parse cannot survive~~ to fix: Refuse app-env values a compose parse cannot survive

2026-09-08 07:16:51 +00:00

ahmad_bot approved these changes

2026-09-08 07:44:05 +00:00

ahmad_bot left a comment

Approved. The delta (dd01751..fe6f580, one commit) is the sq() comment and nothing else — one hunk, no code touched, CI green on fe6f580.

The replacement is accurate on every claim, and I had measured each of them while reviewing the previous head rather than reading them off the docs:

both readers parse this file — the emitted file goes through a real docker compose config cleanly;
single quotes satisfy both — the shell returns the bytes exactly, and compose treats the content as literal (KEY='ab$HOME' stays ab$HOME, where unquoted and double-quoted both expand);
'\'' is shell-only — compose rejects it and abandons the whole file, which is #60.

Nothing left open from my side on this PR. The follow-up for the action's own four inputs — DEPLOY_DOMAIN, IMAGE_TAG, COMPOSE_PROFILES, GRAFANA_PROM_TOKEN, which still reach sq() unchecked and fail silently — stands where you filed it.

Approved. The delta (`dd01751..fe6f580`, one commit) is the `sq()` comment and nothing else — one hunk, no code touched, CI green on `fe6f580`. The replacement is accurate on every claim, and I had measured each of them while reviewing the previous head rather than reading them off the docs: - both readers parse this file — the emitted file goes through a real `docker compose config` cleanly; - single quotes satisfy both — the shell returns the bytes exactly, and compose treats the content as literal (`KEY='ab$HOME'` stays `ab$HOME`, where unquoted and double-quoted both expand); - `'\''` is shell-only — compose rejects it and abandons the whole file, which is `#60`. Nothing left open from my side on this PR. The follow-up for the action's own four inputs — `DEPLOY_DOMAIN`, `IMAGE_TAG`, `COMPOSE_PROFILES`, `GRAFANA_PROM_TOKEN`, which still reach `sq()` unchecked and fail silently — stands where you filed it.

ahmad merged commit edf5387e20 into main

2026-09-08 08:02:57 +00:00

ahmad deleted branch feature/60-refuse-quote

2026-09-08 08:02:58 +00:00

ahmad referenced this pull request from a commit

2026-09-08 08:02:58 +00:00

fix: Refuse app-env values a compose parse cannot survive (#61)

Sign in to join this conversation.

No reviewers