Skip to content

Preserve environment values containing spaces in the launcher carry list#1468

Open
Moon-Knight13 wants to merge 2 commits into
felddy:developfrom
Moon-Knight13:fix/launcher-env-word-splitting
Open

Preserve environment values containing spaces in the launcher carry list#1468
Moon-Knight13 wants to merge 2 commits into
felddy:developfrom
Moon-Knight13:fix/launcher-env-word-splitting

Conversation

@Moon-Knight13

@Moon-Knight13 Moon-Knight13 commented Jul 5, 2026

Copy link
Copy Markdown

🗣 Description

src/launcher.sh builds the clean-environment carry list as a
single string and expands it unquoted into env -i. Any
passlisted variable whose value contains a space word-splits
at exec time.

This PR builds the carry set as a bash array instead, so each
NAME=VALUE reaches env as a single argument. The
${arr[@]+"${arr[@]}"} expansion guards the empty-array case
under set -o nounset on bash < 4.4. ENV_VAR_PASSLIST_REGEX is
unchanged.

A second, related commit widens the CONTAINER_VERBOSE
env-dump redaction in src/entrypoint.sh: the current pattern
(.PASSWORD|KEY.)=.* only matches the keyword adjacent to =,
so values like FOUNDRY_PASSWORD_SALT (and any
_SECRET/_TOKEN) print in clear. The pattern now matches
PASSWORD|KEY|SECRET|TOKEN|SALT anywhere in the variable name.
The commits are independent and cherry-pickable if you'd
prefer to take only one.

💭 Motivation and context

With e.g. NODE_OPTIONS='--max-old-space-size=4096
--enable-source-maps' (matches the ^NODE_.+$ passlist), env
receives --enable-source-maps as a standalone argument and
treats it as the program to execute, failing with env:
'--enable-source-maps': No such file or directory. The exec
never reaches Node and the container crash-loops through the
backoff handler.

Resolves #1467

🧪 Testing

  • Reproduced the crash-loop against
    ghcr.io/felddy/foundryvtt:release with the spaced
    NODE_OPTIONS above (log output attached to Container crash-loops when a carried env var contains spaces (e.g. NODE_OPTIONS with two flags) #1467).
  • Verified the array-based carry loop delivers spaced values
    as a single intact variable, and the empty-carry edge case
    under set -o nounset.
  • Redaction pattern verified against FOUNDRY_PASSWORD_SALT,
    NGROK_AUTH_TOKEN, MY_SECRET, plus a non-sensitive HOME
    control (only the control prints its value).
  • The passlist tests in tests/test_launcher.py parse
    ENV_VAR_PASSLIST_REGEX, which is unchanged; shellcheck
    reports no new findings on either file.
  • Ran the repo's pre-commit shellcheck and shfmt hooks
    (same versions/flags) on both changed files: clean. Full
    pytest suite: 36/36 pass on this branch.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope
    creep!
  • I have read the CONTRIBUTING document.
  • All new and existing tests pass.
  • All relevant type-of-change labels have been added.

The carry list was expanded unquoted into 'env -i', so any passlisted
variable whose value contains a space breaks the exec command line.
Example: NODE_OPTIONS="--max-old-space-size=4096 --enable-source-maps"
splits into 'NODE_OPTIONS=--max-old-space-size=4096' plus a stray
'--enable-source-maps', which env treats as the program to run — exec
fails and the container crash-loops through backoff instead of starting
Foundry.

Build the carry set as a bash array instead; each NAME=VALUE reaches
env as one argument. The ${arr[@]+...} expansion guards the
empty-array case under 'set -o nounset' on bash <4.4.

Repro:
  NODE_OPTIONS='--a --b' bash -c '<old carry loop>; env -i $LIST env'
  -> env: '--b': No such file or directory
The redaction regex '(.*PASSWORD|KEY.*)=.*' misses variables where the
sensitive token is not adjacent to '=': FOUNDRY_PASSWORD_SALT, and any
*_SECRET/*_TOKEN names, are printed in clear in the CONTAINER_VERBOSE
environment dump. Match the sensitive keyword anywhere in the variable
name instead, and add SECRET/TOKEN/SALT to the list.
@Moon-Knight13 Moon-Knight13 requested a review from felddy as a code owner July 5, 2026 08:58
Copilot AI review requested due to automatic review settings July 5, 2026 08:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an environment-carry bug in src/launcher.sh where passlisted variables whose values contain spaces were being word-split when invoking env -i, causing env to misinterpret parts of the value as the command to execute (e.g., NODE_OPTIONS with multiple flags). It also improves sensitive-environment redaction in verbose startup logging in src/entrypoint.sh.

Changes:

  • Switch the environment “carry list” in src/launcher.sh from a single unquoted string to a bash array so NAME=VALUE pairs (including spaced values) are passed to env as intact arguments.
  • Add an empty-array-safe array expansion for compatibility with set -o nounset behavior on older bash versions.
  • Widen CONTAINER_VERBOSE env-dump redaction in src/entrypoint.sh to redact variables whose names contain sensitive keywords anywhere (PASSWORD/KEY/SECRET/TOKEN/SALT).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/launcher.sh Builds the carry set as a bash array to preserve env var values containing spaces when calling env -i.
src/entrypoint.sh Expands verbose env redaction to catch sensitive keywords anywhere in the variable name.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@felddy felddy self-assigned this Jul 11, 2026

@felddy felddy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please rebase and we'll get this merged. Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Container crash-loops when a carried env var contains spaces (e.g. NODE_OPTIONS with two flags)

3 participants