Skip to content

fix(security): keep the MID password out of argv in get_mid_url.sh - #2

Merged
jschuller merged 5 commits into
mainfrom
fix/review-2026-10-17
Oct 19, 2026
Merged

jschuller merged 5 commits into
mainfrom
fix/review-2026-10-17

Conversation

@jschuller

Copy link
Copy Markdown
Owner

Fixes the high finding from the 2026-10-17 repo review, plus the medium and low findings in the same script.

Findings

Severity Finding Fix
High scripts/get_mid_url.sh took --password on argv, which contradicts the script's own "never in argv" promise. The password shows up in ps(1) and in shell history. --password and --password=… are refused. The password comes only from MID_INSTANCE_PASSWORD, set in an env file or in the environment.
Medium The curl --config line broke on passwords containing " or \. A newline in a password could inject extra config lines. \ and " are escaped for curl's config syntax. Line breaks in credentials are refused.
Medium The env-file parser deleted every quote character in a value and ignored export KEY= lines. Only one pair of matching surrounding quotes is removed, and export is accepted. Otherwise values are read literally, the same way docker run --env-file reads them.
Medium A non-JSON response (a hibernating PDI, a login page) ended in a Python traceback. It now fails with a clear error. Null values and unexpected response shapes are handled too.
Low An option given without a value exited silently. It now fails with "--x needs a value".
Low The instance-supplied buildstamp went into a URL unvalidated. It is restricted to [A-Za-z0-9._-]. A rejected value is printed with %q.
Medium (docs) CLAUDE.md said --recipe downloads and unpacks the recipe. It prints the URL; the docs now say so.

Behaviour change: MID_INSTANCE_URL, MID_INSTANCE_USERNAME and MID_INSTANCE_PASSWORD now come from one source:

  1. the --env-file, if one is given;
  2. otherwise the environment, if any of the three is set there;
  3. otherwise .env.

The settings are never mixed across sources. Before, the script read only the env file. README, CHANGELOG and CLAUDE.md are updated.

Verification

  • tests/get_mid_url_test.sh: 28 cases, with a stubbed curl that records the --config text sent on stdin. They pass on macOS /bin/bash 3.2 and on bash 5.2 with GNU sed.
  • shellcheck is clean on 0.9.0 (the version on the CI runner) and on 0.10.0.
  • Escaping was also checked byte for byte with a real curl, using fake values, against a local server that decodes the Basic auth header.
  • An independent review found that environment variables overrode an explicit --env-file, so values from two sources could mix. That is fixed in 8cb5019 and re-reviewed.
  • New CI (.github/workflows/ci.yml) runs shellcheck and the tests. It uses a read-only token, and checkout is pinned by SHA.
  • Vendor-signed files (asset/, Dockerfile*, META-INF, mid.env) are untouched.

Not verified

  • No run against a live instance. The tests stub curl.

The upstream vendor findings (asset/download.sh unquoted wget, the signature substring match, the MD5 of the env) are not addressed here, because those files are vendor-signed.

CRB2205 and others added 5 commits October 18, 2026 19:37
The script promised credentials never reach argv, but offered --password, which puts the
password in ps(1) output and shell history. --password is now refused with a pointer to
MID_INSTANCE_PASSWORD (environment first, then the env file).

Same script, same review:
- escape \ and " for curl's --config syntax and refuse line breaks, so a password cannot
  break the quoting or inject config lines
- strip one pair of surrounding quotes instead of every quote; accept `export KEY=`;
  drop a trailing comment on unquoted values
- fail with a message when an option has no value
- a non-JSON response (hibernating PDI, login page) gives a clear error, not a traceback;
  null property values are tolerated
- restrict the instance-supplied buildstamp to [A-Za-z0-9._-] before it becomes a URL

tests/get_mid_url_test.sh covers all of these with a stubbed curl.

Co-Authored-By: Claude <noreply@anthropic.com>
…ehaviour

Co-Authored-By: Claude <noreply@anthropic.com>
Read-only token; checkout pinned to v7.0.1 by SHA.

Co-Authored-By: Claude <noreply@anthropic.com>
Review follow-up. Reading the environment first, per key, let values mix: with
MID_INSTANCE_URL exported and --env-file pointing at another instance's file, that
file's credentials went to the exported host. A stale exported password also beat an
edited file. All three settings now come from one source: the --env-file if given, else
the environment if any of them is set, else .env.

Env-file values are literal again, as docker run --env-file reads them (only `export `
and one pair of matching surrounding quotes are removed), so a password containing " #"
is no longer cut short. Oddly shaped JSON gets the clear error instead of a traceback,
and a rejected buildstamp is printed with %q.

Co-Authored-By: Claude <noreply@anthropic.com>
@jschuller
jschuller merged commit be4e1fd into main Oct 19, 2026
1 check passed
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.

2 participants