Skip to content

fix(ui): correct and harden config.json generation - #6870

Merged
otavio merged 3 commits into
masterfrom
fix/stripe-publishable-key-prefix
Aug 9, 2026
Merged

fix(ui): correct and harden config.json generation#6870
otavio merged 3 commits into
masterfrom
fix/stripe-publishable-key-prefix

Conversation

@otavio

@otavio otavio commented Aug 9, 2026

Copy link
Copy Markdown
Member

What

Two independent fixes to ui/scripts/gen-config.sh, which renders the /config.json the console fetches at boot. Stripe checkout works again, and the generated document is now always valid JSON.

Requires https://github.com/shellhub-io/cloud/pull/2481. That PR passes the renamed variable through to the ui service. Merging this one alone leaves cloud deployments and local dev serving an empty key, because the compose overlay still exports the old name.

Why

gen-config.sh read the Stripe publishable key from STRIPE_PUBLISHABLE_KEY — the only unprefixed variable in the file. The shellhub-ui secret only sets SHELLHUB_STRIPE_PUBLISHABLE_KEY, so the container never received the name the script looked for, the :- fallback rendered stripePublishableKey: "", and BillingPayment.tsx passed stripe={null} into <Elements>. Stripe Elements never mounted and customers could not enter card details.

Closes #6865

Changes

  • stripe key (1572693): renamed the read to SHELLHUB_STRIPE_PUBLISHABLE_KEY, matching every other value in the file.

  • prefix guard (1572693): tests/ui/gen-config.bats asserts that every value in the script reads a SHELLHUB_-prefixed variable. This is the point of the test file — it fails the whole bug class rather than this one key. EDITION and OUTPUT are exempt because the script assigns them itself.

  • CI (1572693): a gen-config job in script-tests.yml, mirroring the existing is-latest-tag job (pinned SHAs, paths-filter, draft guard).

Unrelated pre-existing bug, second commit

4f399a701 is not part of #6865. It was found while reviewing the first commit and is kept separate so it can be reviewed, or dropped, on its own.

Values were interpolated straight into the JSON, so a quote, backslash or control character produced a config.json the console could not parse at all. The unquoted boolean fields were the sharper edge:

SHELLHUB_EDITION=community SHELLHUB_ANNOUNCEMENTS='false, "edition": "cloud"'
# -> {"edition": "community", "announcements": false, "edition": "cloud", ...}
# -> JSON.parse() yields edition === "cloud"

A second "edition" key wins at parse time, silently promoting a community deployment to cloud despite the SHELLHUB_EDITION guard rejecting exactly that value. Not a vulnerability — it needs control of the container's environment, and anyone with that can set SHELLHUB_EDITION directly — but it defeats a check the script goes out of its way to perform.

String fields are now escaped; boolean fields are validated rather than escaped, since no escaping makes a non-boolean valid in an unquoted position.

Behavioural change worth a look: SHELLHUB_ANNOUNCEMENTS or SHELLHUB_WEB_ENDPOINTS set to anything but true/false now aborts startup instead of rendering garbage. Every declared value in the repo is already true/false, and this matches how the script already treats an invalid SHELLHUB_EDITION, but an operator with a hand-rolled .env.override could hit it.

Testing

bats tests/ui/gen-config.bats

Two things are easy to get wrong here and worth a reviewer's attention:

  • json_string uses $!{N;ba}, not the more familiar :a;N;$!ba. The familiar form makes both GNU and busybox sed print-and-exit on single-line input, skipping every substitution after it and emitting unescaped output — i.e. it silently fails on the common case. Verified in alpine:3.24.1, the production base image, under busybox sh/sed/tr.
  • Escape ordering. Backslash doubling runs first, so the backslashes introduced by the later ", \t, \r and \n rules are not re-escaped.

Beyond the bats suite, the escaping was fuzzed over 200 random values drawn from quotes, backslashes, tabs, newlines and control characters, checking every output parses: zero invalid documents.

otavio added 2 commits August 7, 2026 15:33
gen-config.sh read STRIPE_PUBLISHABLE_KEY while every other value in the file reads a
SHELLHUB_* var. Deployments only ever pass the prefixed name, so the :- fallback rendered
stripePublishableKey empty and Stripe Elements failed to mount at checkout.

The new bats guard asserts every value in the script reads a SHELLHUB_-prefixed variable,
so the whole bug class fails CI rather than just this one key.

Fixes: #6865
gen-config.sh interpolated environment values straight into the JSON, so a
value carrying a quote, a backslash or a control character produced a
config.json the console could not parse at all.

The unquoted boolean fields were the sharper edge. A value such as
'false, "edition": "cloud"' appends a second "edition" key that wins at parse
time, so a community deployment could be silently promoted to cloud despite
the SHELLHUB_EDITION guard rejecting exactly that. They are now validated as
true/false rather than escaped, since neither is valid JSON otherwise.

The $!{N;ba} form in json_string is deliberate: the more familiar :a;N;$!ba
slurp makes both GNU and busybox sed print and exit on single-line input,
skipping every substitution after it and quietly emitting unescaped output.
@otavio
otavio requested review from a team as code owners August 9, 2026 01:34
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 10m 53s —— View job


Code Review Complete

Reviewed 3 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

I verified the parts flagged in the PR body as easy to get wrong, and they hold up:

  • Escape ordering & JSON-injection — backslash-doubling runs before the quote/tab/CR/newline rules, so no synthetic backslash is re-escaped and no escape can be split. Traced adversarial inputs (x", "edition": "cloud, trailing backslash, tab/CR/newline, control chars) all round-trip as data. require_bool fully closes the unquoted-boolean injection vector, and edition is safe unescaped because the case guard only admits community|enterprise|cloud after lowercasing and stripping all whitespace.
  • tr -d set — the removed ranges plus the sed-escaped \t/\n/\r together cover all of 0x00–0x1F, so no raw C0 control byte can reach the output. 0x7F/≥0x80 are JSON-legal unescaped.
  • $!{N;ba} slurp — correctly avoids the :a;N;$!ba print-and-exit-on-single-line trap, as the comment states.
  • Prefix-guard test — the grep -oE '\$\{?[A-Z_]...' pipeline correctly skips $1/$2/$!/$(...) and would catch a real unprefixed reference (e.g. a missing-underscore typo), so it's not a false green.
  • CI wiring — the new gen-config job's paths-filter matches all three changed files, so the tests actually run on this PR; the draft == false guard behaves like the sibling is-latest-tag job on push events.
  • Cross-repo — no dangling STRIPE_PUBLISHABLE_KEY (old name) reference remains anywhere in this repo; the console reads the unchanged stripePublishableKey JSON field, so no UI-side change is needed. The cloud overlay dependency is accurately disclosed in the PR body.

Non-blocking note

The gen-config CI job runs bats on ubuntu-24.04, which exercises GNU sed/coreutils — not the busybox sed/tr that ships in the alpine production image. The json_string escaping is busybox-specific in a couple of spots (the N-slurp, \n-in-pattern matching, octal tr ranges). Busybox handles all of these and you've verified against alpine:3.24.1 manually, so this is not a defect — but an alpine-image matrix leg would turn that manual check into a regression guard for free. Entirely optional.


To request another review round, comment /review.

The runner ships GNU sed and tr, but gen-config.sh escapes JSON inside
the alpine production image, where the octal tr ranges and the newline
match in the pattern space are busybox's to interpret. A green suite on
the runner said nothing about the shell that actually renders
config.json in a deployment.

Run the same suite a second time inside the image the Dockerfile names,
so an alpine bump is covered by the same guard rather than silently
widening the gap.
@otavio
otavio merged commit 6395dec into master Aug 9, 2026
39 checks passed
@otavio
otavio deleted the fix/stripe-publishable-key-prefix branch August 9, 2026 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gen-config.sh reads STRIPE_PUBLISHABLE_KEY without SHELLHUB_ prefix, breaking Stripe checkout

1 participant