fix: Parse OpenBao responses with node, not jq #58

ahmad · 2026-09-07T22:45:46Z

ahmad commented

2026-09-07 22:45:46 +00:00

Issue

Closes #57

Problem

openbao-fetch installs jq (and curl if missing) with an in-job apt-get. On 2026-09-07 that failed whole jobs in three repositories in one evening — and the trigger is now permanent: the default job container is bullseye-based and bullseye-security's final InRelease expired past Debian 11's LTS end.

Solution

Both JSON parses (AppRole token extract, kv-to-KEY=VALUE render) become node -e one-liners. Every job container in this fleet is a node image (the runner's default label maps to node:20-bullseye; declared containers are node:22-bookworm), so nothing is installed at runtime. The apt fallback shrinks to curl-only last-resort; a missing node fails fast naming the requirement. One deliberate behaviour change: a response without a secret payload (permission denied, HTML error page) now fails the step, where jq rendered an empty file and exited 0.

Review notes

One step in one file plus a README sentence. Parsers were round-tripped locally against fixtures: token extracted; no-auth login response takes the existing login-failed path; kv render matches jq's stringification for string/number/bool/empty values; malformed and permission-denied responses exit nonzero.

Risks and trade-offs

A consumer on a non-node image would now fail fast with a named requirement instead of quietly apt-installing — no such consumer exists in the fleet.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

### Issue Closes #57 ### Problem openbao-fetch installs `jq` (and `curl` if missing) with an in-job `apt-get`. On 2026-09-07 that failed whole jobs in three repositories in one evening — and the trigger is now permanent: the default job container is bullseye-based and bullseye-security's final `InRelease` expired past Debian 11's LTS end. ### Solution Both JSON parses (AppRole token extract, kv-to-KEY=VALUE render) become `node -e` one-liners. Every job container in this fleet is a node image (the runner's default label maps to `node:20-bullseye`; declared containers are `node:22-bookworm`), so nothing is installed at runtime. The apt fallback shrinks to curl-only last-resort; a missing `node` fails fast naming the requirement. One deliberate behaviour change: a response without a secret payload (permission denied, HTML error page) now fails the step, where `jq` rendered an empty file and exited 0. ### Review notes One step in one file plus a README sentence. Parsers were round-tripped locally against fixtures: token extracted; no-auth login response takes the existing login-failed path; kv render matches jq's stringification for string/number/bool/empty values; malformed and permission-denied responses exit nonzero. ### Risks and trade-offs A consumer on a non-node image would now fail fast with a named requirement instead of quietly apt-installing — no such consumer exists in the fleet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TpKJno2d3VeZeEyV3sJbeq

ahmad added 1 commit

2026-09-07 22:45:46 +00:00

fix: Parse OpenBao responses with node, not jq

PR / validate (pull_request) Successful in 18s

Details

abbd0dbde6

openbao-fetch installed jq with an in-job apt-get, so an expired mirror
release file failed whole jobs across three repositories in one evening
— and bullseye-security's InRelease is now expired for good. Every job
container in this fleet is a node image, so node parses the JSON and
nothing is installed at runtime; the apt fallback shrinks to curl-only.
A response without a secret payload now fails the step instead of
rendering an empty file.

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

ahmad commented

2026-09-07 22:48:18 +00:00

Self-verification against the acceptance criteria:

Fetch succeeds with apt unavailable or failing — the step no longer touches apt when curl is present, which it is in every fleet job container (node Debian images ship it); the only remaining apt path is the curl-only last resort. Parsing needs node, guaranteed by the runner's default label (node:20-bullseye, read from the runner config) and by every declared container (node:22-bookworm).
Consumers unchanged on the happy path — inputs and outputs are identical; the parsers were round-tripped against fixtures (token extract; no-auth → existing login-failed path; kv render byte-matching jq's stringification for string/number/bool/empty). The one deliberate divergence — a payload-less response fails the step instead of writing an empty file — is stated in the PR body.
check-actions.py passes — locally (openbao-fetch/action.yml: ok) and in CI: PR / validate green on abbd0db, read back from the commit status.

Marking ready.

Self-verification against the acceptance criteria: 1. **Fetch succeeds with apt unavailable or failing** — the step no longer touches apt when `curl` is present, which it is in every fleet job container (node Debian images ship it); the only remaining apt path is the curl-only last resort. Parsing needs `node`, guaranteed by the runner's default label (`node:20-bullseye`, read from the runner config) and by every declared container (`node:22-bookworm`). 2. **Consumers unchanged on the happy path** — inputs and outputs are identical; the parsers were round-tripped against fixtures (token extract; no-auth → existing login-failed path; kv render byte-matching jq's stringification for string/number/bool/empty). The one deliberate divergence — a payload-less response fails the step instead of writing an empty file — is stated in the PR body. 3. **check-actions.py passes** — locally (`openbao-fetch/action.yml: ok`) and in CI: `PR / validate` green on `abbd0db`, read back from the commit status. Marking ready.

ahmad changed title from ~~WIP: fix: Parse OpenBao responses with node, not jq~~ to fix: Parse OpenBao responses with node, not jq

2026-09-07 22:48:18 +00:00

ahmad_bot requested changes

2026-09-07 23:20:14 +00:00

ahmad_bot left a comment

Requesting changes on one point: the two node -e readers concatenate raw Buffer chunks into a string, which silently corrupts any non-ASCII byte sequence that lands on a chunk boundary. Everything else here checks out.

Verified before this review

Every runner label maps to the same image — ubuntu-latest:docker://node:20-bullseye,ubuntu-22.04:...,linux:...,self-hosted:... in forgejo/templates/docker-compose.yml.j2 — so there is no host-mode label, and the new command -v node fail-fast cannot strand a job that used to work. Consumers that declare container: node:22-bookworm are covered too.
curl ships in node:20-bullseye (buildpack-deps base), jq does not. That is exactly why the old code reached for apt-get, and it makes the remaining curl-only fallback effectively dead code in this fleet — the right shape for a last resort.
This closes the openbao-fetch half of the bullseye-security exposure from #57: this step no longer runs apt-get update on the happy path, so an expired mirror can no longer fail a job that only wanted to read a secret.
CI is green on abbd0db (PR / validate, checked in its own step).
Behaviour change on a missing .data.data is real and is an improvement: jq wrote an empty file and exited 0, node throws and set -euo pipefail fails the step.

The one finding is inline.

Requesting changes on one point: the two `node -e` readers concatenate raw `Buffer` chunks into a string, which silently corrupts any non-ASCII byte sequence that lands on a chunk boundary. Everything else here checks out. **Verified before this review** - Every runner label maps to the same image — `ubuntu-latest:docker://node:20-bullseye,ubuntu-22.04:...,linux:...,self-hosted:...` in `forgejo/templates/docker-compose.yml.j2` — so there is no host-mode label, and the new `command -v node` fail-fast cannot strand a job that used to work. Consumers that declare `container: node:22-bookworm` are covered too. - `curl` ships in `node:20-bullseye` (buildpack-deps base), `jq` does not. That is exactly why the old code reached for `apt-get`, and it makes the remaining curl-only fallback effectively dead code in this fleet — the right shape for a last resort. - This closes the `openbao-fetch` half of the `bullseye-security` exposure from #57: this step no longer runs `apt-get update` on the happy path, so an expired mirror can no longer fail a job that only wanted to read a secret. - CI is green on `abbd0db` (`PR / validate`, checked in its own step). - Behaviour change on a missing `.data.data` is real and is an improvement: `jq` wrote an empty file and exited 0, `node` throws and `set -euo pipefail` fails the step. The one finding is inline.

openbao-fetch/action.yml Outdated

					
				@ -53,3 +60,3 @@

				        token=$(printf '{"role_id":"%s","secret_id":"%s"}' "$OB_ROLE_ID" "$OB_SECRET_ID" \

				          | curl -sf $k -X POST -d @- "$OB_ADDR/v1/auth/approle/login" \

				          | jq -r '.auth.client_token // empty')

				          | node -e 'let d="";process.stdin.on("data",c=>d+=c);process.stdin.on("end",()=>{const t=JSON.parse(d).auth&&JSON.parse(d).auth.client_token;if(t)process.stdout.write(t)})')

Same setEncoding("utf8") fix as on the render line below. A Vault token is ASCII, so this one cannot corrupt today; worth changing anyway so the two readers do not diverge and nobody has to work out which of them was safe.

Minor, non-blocking: JSON.parse(d) runs twice here. It only matters because the second parse of a large response is wasted work, but a single const r=JSON.parse(d) reads the same and does half the work.

Same `setEncoding("utf8")` fix as on the render line below. A Vault token is ASCII, so this one cannot corrupt today; worth changing anyway so the two readers do not diverge and nobody has to work out which of them was safe. Minor, non-blocking: `JSON.parse(d)` runs twice here. It only matters because the second parse of a large response is wasted work, but a single `const r=JSON.parse(d)` reads the same and does half the work.

openbao-fetch/action.yml Outdated

					
				@ -60,2 +67,3 @@

				        # unlike jq, which rendered it as empty output and exit 0.

				        curl -sf $k -H "X-Vault-Token: $token" "$OB_ADDR/v1/$data_path" \

				          | jq -r '.data.data | to_entries[] | "\(.key)=\(.value)"' > "$OB_OUT"

				          | node -e 'let d="";process.stdin.on("data",c=>d+=c);process.stdin.on("end",()=>{for(const [k,v] of Object.entries(JSON.parse(d).data.data))process.stdout.write(k+"="+v+"\n")})' > "$OB_OUT"

d += c appends a Buffer to a string, which decodes each chunk independently as UTF-8. A multi-byte character split across two chunks decodes as two replacement characters on either side of the seam, and the corrupted value is written into .env with the step still exiting 0 — the same silent-wrong-file failure mode this PR set out to remove, just moved from "empty" to "subtly wrong".

I confirmed both halves of this against the exact pipeline in this diff:

# writer flushes mid-character (what a network-fed curl does), 2KB payload
SMTP_PASS=caf<ef><bf><bd><ef><bf><bd>        # as written here
SMTP_PASS=café                              # with setEncoding("utf8")

and again with cat on a large payload, where the character straddles the 64KiB read boundary exactly — byte 65534 and 65536 are fine, byte 65535 corrupts. So this is not only a >64KiB concern: curl reading from a socket flushes in pieces of whatever arrives, so a small response can split anywhere.

Fix, one token:

| node -e 'process.stdin.setEncoding("utf8");let d="";process.stdin.on("data",c=>d+=c);process.stdin.on("end",()=>{for(const [k,v] of Object.entries(JSON.parse(d).data.data))process.stdout.write(k+"="+v+"\n")})' > "$OB_OUT"

With the encoding set, node's StringDecoder holds the incomplete sequence back until the next chunk completes it. jq had no such problem, so this is a regression against what it replaces, and it lands in secret values — a corrupted password fails at the app, far from here.

`d += c` appends a `Buffer` to a string, which decodes **each chunk independently as UTF-8**. A multi-byte character split across two chunks decodes as two replacement characters on either side of the seam, and the corrupted value is written into `.env` with the step still exiting 0 — the same silent-wrong-file failure mode this PR set out to remove, just moved from "empty" to "subtly wrong". I confirmed both halves of this against the exact pipeline in this diff: # writer flushes mid-character (what a network-fed curl does), 2KB payload SMTP_PASS=caf<ef><bf><bd><ef><bf><bd> # as written here SMTP_PASS=café # with setEncoding("utf8") and again with `cat` on a large payload, where the character straddles the 64KiB read boundary exactly — byte 65534 and 65536 are fine, byte 65535 corrupts. So this is not only a >64KiB concern: `curl` reading from a socket flushes in pieces of whatever arrives, so a small response can split anywhere. Fix, one token: ``` | node -e 'process.stdin.setEncoding("utf8");let d="";process.stdin.on("data",c=>d+=c);process.stdin.on("end",()=>{for(const [k,v] of Object.entries(JSON.parse(d).data.data))process.stdout.write(k+"="+v+"\n")})' > "$OB_OUT" ``` With the encoding set, node's `StringDecoder` holds the incomplete sequence back until the next chunk completes it. `jq` had no such problem, so this is a regression against what it replaces, and it lands in secret values — a corrupted password fails at the app, far from here.

ahmad changed title from ~~fix: Parse OpenBao responses with node, not jq~~ to WIP: fix: Parse OpenBao responses with node, not jq

2026-09-07 23:43:44 +00:00

ahmad added 1 commit

2026-09-07 23:44:38 +00:00

fix: Decode stdin as UTF-8 in both response readers

PR / validate (pull_request) Successful in 17s

Details

15148e2a55

Concatenating raw Buffer chunks decodes each chunk independently, so a
multi-byte character split across a chunk boundary corrupts silently —
in secret values, with exit 0. setEncoding lets the string decoder hold
the incomplete sequence until the next chunk completes it. The token
reader also parses the response once instead of twice.

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

ahmad reviewed

2026-09-07 23:44:38 +00:00

ahmad left a comment

Fixed in 15148e2 — setEncoding("utf8") on both readers, and the token reader now parses once (const r=JSON.parse(d)), taking the non-blocking note too. Reproduced your seam case before fixing: a split-write harness that delivers the payload in two chunks with the boundary inside the character shows the old reader emitting caf�� (and mangling a 4-byte emoji) while the fixed reader returns both byte-for-byte — the broken variant is kept in the harness as the kill control. Good catch: this was a real regression against jq, in exactly the values that matter.

Fixed in 15148e2 — `setEncoding("utf8")` on both readers, and the token reader now parses once (`const r=JSON.parse(d)`), taking the non-blocking note too. Reproduced your seam case before fixing: a split-write harness that delivers the payload in two chunks with the boundary inside the character shows the old reader emitting `caf��` (and mangling a 4-byte emoji) while the fixed reader returns both byte-for-byte — the broken variant is kept in the harness as the kill control. Good catch: this was a real regression against jq, in exactly the values that matter.

ahmad changed title from ~~WIP: fix: Parse OpenBao responses with node, not jq~~ to fix: Parse OpenBao responses with node, not jq

2026-09-07 23:46:47 +00:00

ahmad_bot approved these changes

2026-09-08 00:15:25 +00:00

ahmad_bot left a comment

Approved. Re-reviewed the delta only (abbd0db..15148e2, one commit): setEncoding("utf8") added to both readers, and the token reader now parses once instead of twice. That is exactly the fix, and I re-ran the cases that produced the original finding against the new one-liners:

The 2KB payload whose writer flushes mid-character — the case that made this more than a large-response concern — now yields SMTP_PASS=café.
The 64KiB boundary case with the character straddling byte 65535, previously two replacement characters, now round-trips.
Token reader unchanged in behaviour where it matters: a normal response yields the token; {"errors":[...]}, auth without client_token, and auth: null all write nothing and exit 0, so [ -n "$token" ] still produces the "AppRole login failed" message rather than a stack trace. Collapsing the double parse into const r did not change that — r.auth && r.auth.client_token short-circuits the same way.
The deliberate new failure still fires: a payload with no .data.data exits 1, and under set -euo pipefail that fails the step.
CI green on 15148e2 (PR / validate), checked in its own step.

Everything from the first review still holds: node is guaranteed by every runner label resolving to node:20-bullseye and by the declared node:22-bookworm containers, curl ships in those images, and the remaining apt fallback is a last resort that should not fire in this fleet.

Worth stating because another PR is waiting on it: merging this moves v3 automatically — release.yml force-moves the major tag to the new semver tag — so every consumer picks the fix up on its next run with no change on their side. That retires openbao-fetch as an apt site fleet-wide, which is what ahmad/expiro#70 needs for its two deploy jobs.

One non-blocking note inline.

Approved. Re-reviewed the delta only (`abbd0db..15148e2`, one commit): `setEncoding("utf8")` added to both readers, and the token reader now parses once instead of twice. That is exactly the fix, and I re-ran the cases that produced the original finding against the new one-liners: - The 2KB payload whose writer flushes mid-character — the case that made this more than a large-response concern — now yields `SMTP_PASS=café`. - The 64KiB boundary case with the character straddling byte 65535, previously two replacement characters, now round-trips. - Token reader unchanged in behaviour where it matters: a normal response yields the token; `{"errors":[...]}`, `auth` without `client_token`, and `auth: null` all write nothing and exit 0, so `[ -n "$token" ]` still produces the "AppRole login failed" message rather than a stack trace. Collapsing the double parse into `const r` did not change that — `r.auth && r.auth.client_token` short-circuits the same way. - The deliberate new failure still fires: a payload with no `.data.data` exits 1, and under `set -euo pipefail` that fails the step. - CI green on `15148e2` (`PR / validate`), checked in its own step. Everything from the first review still holds: `node` is guaranteed by every runner label resolving to `node:20-bullseye` and by the declared `node:22-bookworm` containers, `curl` ships in those images, and the remaining apt fallback is a last resort that should not fire in this fleet. Worth stating because another PR is waiting on it: merging this moves `v3` automatically — `release.yml` force-moves the major tag to the new semver tag — so every consumer picks the fix up on its next run with no change on their side. That retires `openbao-fetch` as an apt site fleet-wide, which is what `ahmad/expiro#70` needs for its two deploy jobs. One non-blocking note inline.

openbao-fetch/action.yml

					
				@ -60,2 +67,3 @@

				        # unlike jq, which rendered it as empty output and exit 0.

				        curl -sf $k -H "X-Vault-Token: $token" "$OB_ADDR/v1/$data_path" \

				          | jq -r '.data.data | to_entries[] | "\(.key)=\(.value)"' > "$OB_OUT"

				          | node -e 'process.stdin.setEncoding("utf8");let d="";process.stdin.on("data",c=>d+=c);process.stdin.on("end",()=>{for(const [k,v] of Object.entries(JSON.parse(d).data.data))process.stdout.write(k+"="+v+"\n")})' > "$OB_OUT"

Non-blocking, and not a regression against anything that currently happens — raising it because the divergence is silent.

k+"="+v matches jq's "\(.value)" exactly for every scalar. I checked side by side:

S=str   N=42   B=true   E=   U=a=b   SP= pad    NUL=null      ← identical under both

It diverges only for structured values:

node:  OBJ=[object Object]   ARR=1,2
jq:    OBJ={"a":1}           ARR=[1,2]

kv-v2 stores whatever JSON was written, so a field written through the API as an object or array is possible even though bao kv put k=v only ever writes strings. Neither rendering produces a usable env var, so nothing is lost here — but [object Object] is the kind of value that reaches an application and gets debugged in the wrong place, whereas the JSON text at least reads as what it is.

If you want exact parity for a token's cost:

process.stdout.write(k+"="+(typeof v==="string"?v:JSON.stringify(v))+"\n")

Alternatively, rejecting a non-string value outright would be defensible too — an .env cannot represent one, so failing the step names the problem at its source. Either is better than the current silent flattening; the choice is yours and neither blocks this.

Non-blocking, and not a regression against anything that currently happens — raising it because the divergence is silent. `k+"="+v` matches `jq`'s `"\(.value)"` exactly for every scalar. I checked side by side: S=str N=42 B=true E= U=a=b SP= pad NUL=null ← identical under both It diverges only for structured values: node: OBJ=[object Object] ARR=1,2 jq: OBJ={"a":1} ARR=[1,2] kv-v2 stores whatever JSON was written, so a field written through the API as an object or array is possible even though `bao kv put k=v` only ever writes strings. Neither rendering produces a usable env var, so nothing is *lost* here — but `[object Object]` is the kind of value that reaches an application and gets debugged in the wrong place, whereas the JSON text at least reads as what it is. If you want exact parity for a token's cost: process.stdout.write(k+"="+(typeof v==="string"?v:JSON.stringify(v))+"\n") Alternatively, rejecting a non-string value outright would be defensible too — an `.env` cannot represent one, so failing the step names the problem at its source. Either is better than the current silent flattening; the choice is yours and neither blocks this.

ahmad reviewed

2026-09-08 00:43:39 +00:00

ahmad left a comment

Acknowledged on the structured-value note: agreed it is not a regression (no consumer writes objects/arrays into these secrets, and bao kv put only writes strings), and worth a look only if a future consumer stores structured values — at which point JSON.stringify per non-string value is the one-line answer. Merging as approved.

ahmad merged commit 2bccd5eff4 into main

2026-09-08 00:43:40 +00:00

ahmad deleted branch feature/57-openbao-no-apt

2026-09-08 00:43:40 +00:00

ahmad referenced this pull request from a commit

2026-09-08 00:43:41 +00:00

fix: Parse OpenBao responses with node, not jq (#58)

ahmad referenced this pull request

2026-09-08 21:37:54 +00:00

fix: A failed notification must not fail the release #65

ahmad referenced this pull request

2026-09-10 05:18:32 +00:00