From 5f953355c211f3ca3159fa899028b0b5371602ad Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 08:55:13 -0600 Subject: [PATCH 01/17] Add safe settings inspection boundary --- bin/fm-brief.sh | 12 +- bin/fm-secrets.py | 403 +++++++++++++++++++++++++++++++++++++++ bin/fm-secrets.sh | 52 +++++ tests/fm-brief.test.sh | 43 ++++- tests/fm-secrets.test.sh | 149 +++++++++++++++ 5 files changed, 655 insertions(+), 4 deletions(-) create mode 100755 bin/fm-secrets.py create mode 100755 bin/fm-secrets.sh create mode 100755 tests/fm-secrets.test.sh diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 3b6797eb224..86e940f690d 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -490,12 +490,18 @@ IFS= read -r -d '' TASK_SECTION <<'EOF' || true EOF TASK_SECTION=${TASK_SECTION%$'\n'} -# One shared string keeps the ship and scout infrastructure rule identical. +# Shared strings keep the ship and scout safety rules identical. # Rule 2 governs file edits, so it does not prohibit pool administration. # The secondmate charter deliberately omits this rule because a secondmate # legitimately allocates and returns slots for crewmates in its own home. +FM_SECRETS_TOOL=$(shell_quote "$FM_ROOT/bin/fm-secrets.sh") +# shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. +SHARED_SETTINGS_RULE=$(printf '%s\n' \ + "7. Use \`$FM_SECRETS_TOOL\` for every settings file or service environment; its \`--help\` owns the safe operations and limits." \ + ' Never use `cat`, `sed`, `nl`, or `grep` to read settings values, never read `/proc/*/environ`, and never run `systemctl show Environment` directly.') + IFS= read -r -d '' SHARED_INFRA_RULE <<'EOF' || true -7. Never administer infrastructure that every lane shares. Two things are shared: +8. Never administer infrastructure that every lane shares. Two things are shared: - The `no-mistakes` daemon - one instance serving every lane/home, so stopping, restarting, or updating it kills other lanes' in-flight pipeline runs; only firstmate manages the daemon. Before you append `blocked:` about the pipeline, run `no-mistakes daemon status` and @@ -564,6 +570,7 @@ The report is the only thing that survives, so anything worth keeping must be in append \`needs-decision [at=]: {summary of options}\` and stop. Firstmate will reply with the decision. A decision or blocker you opened stays open until a \`resolved\` line carrying its exact key lands; a later \`done:\` or \`working:\` line never closes it, even when the answer is what started that work. Firstmate's reply normally writes that closing line at answer time; when a blocker or wait clears WITHOUT a firstmate reply, append \`resolved [at=]: {how it cleared}\` yourself (same \`[key=]\` if you opened it with one) as you resume. +$SHARED_SETTINGS_RULE $SHARED_INFRA_RULE $INBOX_SECTION @@ -645,6 +652,7 @@ $RULE1 $ASK_USER_BLOCK A decision or blocker you opened stays open until a \`resolved\` line carrying its exact key lands; a later \`done:\` or \`working:\` line never closes it, even when the answer is what started that work. Firstmate's reply normally writes that closing line at answer time; when a blocker or wait clears WITHOUT a firstmate reply, append \`resolved [at=]: {how it cleared}\` yourself (same \`[key=]\` if you opened it with one) as you resume. +$SHARED_SETTINGS_RULE $SHARED_INFRA_RULE $INBOX_SECTION diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py new file mode 100755 index 00000000000..e4b2e130713 --- /dev/null +++ b/bin/fm-secrets.py @@ -0,0 +1,403 @@ +#!/usr/bin/env python3 +"""Implementation for fm-secrets.sh. + +The shell entry point owns the public help and invocation contract. This helper +keeps secret-bearing data inside one process and never includes a value in an +error or diagnostic. +""" + +from __future__ import annotations + +import glob +import os +import re +import shlex +import signal +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Iterable, Sequence + + +NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +ASSIGNMENT_RE = re.compile( + r"[ \t]*(?:export[ \t]+)?([A-Za-z_][A-Za-z0-9_]*)[ \t]*=[ \t]*" +) +URL_PASSWORD_RE = re.compile( + rb"([A-Za-z][A-Za-z0-9+.-]*://[^\s/@:]+:)([^\s/@]+)(@[^\s]+)" +) +MIN_SCRUB_BYTES = 6 + + +class SecretToolError(Exception): + """An error whose message is guaranteed not to contain a setting value.""" + + +@dataclass(frozen=True) +class Assignment: + name: str + value: str + + +def _line_end(text: str, start: int) -> int: + end = text.find("\n", start) + return len(text) if end < 0 else end + + +def _line_number(text: str, position: int) -> int: + return text.count("\n", 0, position) + 1 + + +def _decode_double_quoted(raw: str) -> str: + decoded: list[str] = [] + index = 0 + escapes = {"n": "\n", "r": "\r", "t": "\t", "\\": "\\", '"': '"'} + while index < len(raw): + char = raw[index] + if char == "\\" and index + 1 < len(raw): + following = raw[index + 1] + if following in escapes: + decoded.append(escapes[following]) + index += 2 + continue + decoded.append(char) + index += 1 + return "".join(decoded) + + +def _quoted_value(text: str, start: int, quote: str, path: str) -> tuple[str, int]: + cursor = start + 1 + raw: list[str] = [] + while cursor < len(text): + char = text[cursor] + if char == quote: + if quote == '"': + backslashes = 0 + check = cursor - 1 + while check >= start and text[check] == "\\": + backslashes += 1 + check -= 1 + if backslashes % 2: + raw.append(char) + cursor += 1 + continue + value = "".join(raw) + if quote == '"': + value = _decode_double_quoted(value) + return value, cursor + 1 + raw.append(char) + cursor += 1 + raise SecretToolError( + f"{path}: unterminated quoted value at line {_line_number(text, start)}" + ) + + +def _strip_unquoted_comment(value: str) -> str: + for index, char in enumerate(value): + if char == "#" and (index == 0 or value[index - 1].isspace()): + return value[:index].rstrip() + return value.rstrip() + + +def _unquoted_value(text: str, start: int) -> tuple[str, int]: + pieces: list[str] = [] + cursor = start + while True: + end = _line_end(text, cursor) + piece = text[cursor:end] + trimmed = piece.rstrip() + trailing = len(trimmed) - len(trimmed.rstrip("\\")) + if trailing % 2 == 1 and end < len(text): + pieces.append(trimmed[:-1]) + cursor = end + 1 + continue + pieces.append(piece) + return _strip_unquoted_comment("".join(pieces)), end + + +def parse_env_file(path: str) -> list[Assignment]: + try: + text = Path(path).read_text(encoding="utf-8", errors="surrogateescape") + except OSError as exc: + raise SecretToolError(f"cannot read settings file: {path}") from exc + + assignments: list[Assignment] = [] + position = 0 + while position < len(text): + end = _line_end(text, position) + line = text[position:end] + stripped = line.lstrip(" \t\r") + if not stripped or stripped.startswith("#"): + position = end + (end < len(text)) + continue + + match = ASSIGNMENT_RE.match(line) + if match is None: + position = end + (end < len(text)) + continue + + value_start = position + match.end() + if value_start < len(text) and text[value_start] in ("'", '"'): + value, consumed = _quoted_value( + text, value_start, text[value_start], path + ) + next_end = _line_end(text, consumed) + else: + value, next_end = _unquoted_value(text, value_start) + assignments.append(Assignment(match.group(1), value)) + position = next_end + (next_end < len(text)) + return assignments + + +def unique_names(assignments: Iterable[Assignment]) -> list[str]: + seen: set[str] = set() + names: list[str] = [] + for assignment in assignments: + if assignment.name not in seen: + seen.add(assignment.name) + names.append(assignment.name) + return names + + +def final_values(assignments: Iterable[Assignment]) -> dict[str, str]: + return {assignment.name: assignment.value for assignment in assignments} + + +def validate_names(names: Sequence[str]) -> None: + if not names: + raise SecretToolError("at least one setting name is required") + for name in names: + if NAME_RE.fullmatch(name) is None: + raise SecretToolError(f"invalid setting name: {name}") + + +def systemctl_property(unit: str, property_name: str) -> str: + try: + result = subprocess.run( + [ + "systemctl", + "show", + "--no-pager", + f"--property={property_name}", + "--value", + "--", + unit, + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + text=True, + ) + except OSError as exc: + raise SecretToolError("systemctl is unavailable") from exc + if result.returncode != 0: + raise SecretToolError(f"cannot inspect systemd unit: {unit}") + return result.stdout.strip() + + +def process_environment_names(pid_text: str) -> set[str] | None: + if not pid_text.isdigit() or pid_text == "0": + return None + try: + environ = Path(f"/proc/{pid_text}/environ").read_bytes() + except OSError: + return None + names: set[str] = set() + for field in environ.split(b"\0"): + raw_name, separator, _value = field.partition(b"=") + if not separator: + continue + try: + name = raw_name.decode("ascii") + except UnicodeDecodeError: + continue + if NAME_RE.fullmatch(name): + names.add(name) + return names + + +def environment_declaration_names(raw: str) -> set[str]: + try: + words = shlex.split(raw, posix=True) + except ValueError as exc: + raise SecretToolError("systemd Environment= data could not be parsed safely") from exc + names: set[str] = set() + for word in words: + name, separator, _value = word.partition("=") + if separator and NAME_RE.fullmatch(name): + names.add(name) + return names + + +def environment_file_specs(raw: str) -> list[tuple[str, bool]]: + try: + words = shlex.split(raw, posix=True) + except ValueError as exc: + raise SecretToolError("systemd EnvironmentFile= data could not be parsed safely") from exc + specs: list[tuple[str, bool]] = [] + for word in words: + if word.startswith("(ignore_errors="): + if specs and word == "(ignore_errors=yes)": + path, _ignore_errors = specs[-1] + specs[-1] = (path, True) + continue + ignore_errors = False + if word.startswith("-"): + word = word[1:] + ignore_errors = True + if word: + specs.extend((path, ignore_errors) for path in (glob.glob(word) or [word])) + return specs + + +def service_names(unit: str) -> set[str]: + if not unit or unit.startswith("-"): + raise SecretToolError("a valid systemd unit name is required") + running = process_environment_names(systemctl_property(unit, "MainPID")) + if running is not None: + return running + + names = environment_declaration_names(systemctl_property(unit, "Environment")) + files = environment_file_specs(systemctl_property(unit, "EnvironmentFiles")) + for path, ignore_errors in files: + try: + names.update(unique_names(parse_env_file(path))) + except SecretToolError as exc: + if ignore_errors: + continue + raise SecretToolError( + "cannot inspect a required systemd EnvironmentFile safely" + ) from exc + return names + + +def command_names(args: Sequence[str]) -> int: + if len(args) != 1: + raise SecretToolError("usage: fm-secrets.sh names ") + for name in unique_names(parse_env_file(args[0])): + print(name) + return 0 + + +def command_has(args: Sequence[str]) -> int: + if args and args[0] == "--service": + if len(args) < 3: + raise SecretToolError( + "usage: fm-secrets.sh has --service ..." + ) + present = service_names(args[1]) + requested = list(args[2:]) + else: + if len(args) < 2: + raise SecretToolError( + "usage: fm-secrets.sh has ..." + ) + present = set(unique_names(parse_env_file(args[0]))) + requested = list(args[1:]) + validate_names(requested) + for name in requested: + print(f"{name}={'yes' if name in present else 'no'}") + return 0 + + +def known_scrubbers(assignments: Iterable[Assignment]) -> list[tuple[bytes, bytes]]: + scrubbers: dict[bytes, bytes] = {} + for assignment in assignments: + encoded = assignment.value.encode("utf-8", errors="surrogateescape") + replacement = f"".encode("ascii") + if len(encoded) >= MIN_SCRUB_BYTES: + scrubbers.setdefault(encoded, replacement) + for match in URL_PASSWORD_RE.finditer(encoded): + password = match.group(2) + if len(password) >= MIN_SCRUB_BYTES: + scrubbers.setdefault(password, replacement) + return sorted(scrubbers.items(), key=lambda item: len(item[0]), reverse=True) + + +def scrub_output(data: bytes, scrubbers: Sequence[tuple[bytes, bytes]]) -> bytes: + for value, replacement in scrubbers: + data = data.replace(value, replacement) + + def redact_url_password(match: re.Match[bytes]) -> bytes: + password = match.group(2) + if len(password) < MIN_SCRUB_BYTES: + return match.group(0) + return match.group(1) + b"" + match.group(3) + + return URL_PASSWORD_RE.sub(redact_url_password, data) + + +def command_run(args: Sequence[str]) -> int: + if len(args) < 5 or args[1] != "--only": + raise SecretToolError( + "usage: fm-secrets.sh run --only NAME[,NAME...] -- " + ) + try: + delimiter = args.index("--", 3) + except ValueError as exc: + raise SecretToolError("run requires -- before the command") from exc + if delimiter != 3 or delimiter + 1 >= len(args): + raise SecretToolError( + "usage: fm-secrets.sh run --only NAME[,NAME...] -- " + ) + + requested = [name.strip() for name in args[2].split(",") if name.strip()] + validate_names(requested) + assignments = parse_env_file(args[0]) + values = final_values(assignments) + missing = [name for name in requested if name not in values] + if missing: + raise SecretToolError("settings file is missing requested names: " + ",".join(missing)) + + child_env = os.environ.copy() + for name in values: + child_env.pop(name, None) + for name in requested: + child_env[name] = values[name] + + try: + child = subprocess.run( + list(args[delimiter + 1 :]), + env=child_env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + ) + except OSError as exc: + raise SecretToolError("command could not be started") from exc + + scrubbers = known_scrubbers(assignments) + sys.stdout.buffer.write(scrub_output(child.stdout, scrubbers)) + sys.stderr.buffer.write(scrub_output(child.stderr, scrubbers)) + sys.stdout.buffer.flush() + sys.stderr.buffer.flush() + if child.returncode < 0: + return 128 + abs(child.returncode) + return child.returncode + + +def main(argv: Sequence[str]) -> int: + if not argv: + raise SecretToolError("run fm-secrets.sh --help for usage") + command, args = argv[0], argv[1:] + if command == "names": + return command_names(args) + if command == "has": + return command_has(args) + if command == "run": + return command_run(args) + raise SecretToolError(f"unknown subcommand: {command}") + + +if __name__ == "__main__": + try: + sys.exit(main(sys.argv[1:])) + except SecretToolError as error: + print(f"fm-secrets: {error}", file=sys.stderr) + sys.exit(2) + except KeyboardInterrupt: + os.kill(os.getpid(), signal.SIGINT) + except Exception: + print("fm-secrets: operation failed safely without exposing settings", file=sys.stderr) + sys.exit(1) diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh new file mode 100755 index 00000000000..bf6fbbc1ac3 --- /dev/null +++ b/bin/fm-secrets.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# fm-secrets.sh - the only worker-facing interface for inspecting settings. +# +# Subcommands: +# names +# Print only assignment names, one per line. +# has ... +# has --service ... +# Print NAME=yes or NAME=no without printing any setting value. +# A running service's main-process environment is authoritative when it +# is readable; otherwise the unit's EnvironmentFile= and Environment= +# declarations are inspected. +# run --only NAME[,NAME...] -- +# Remove every variable named by the file from the inherited environment, +# add only the selected names, run the command, and scrub its stdout and +# stderr. Every file value at least 6 bytes long is replaced with +# . Passwords at least 6 bytes long in URL userinfo are +# scrubbed too. Values shorter than 6 bytes are deliberately not scrubbed +# because replacing common short strings would corrupt ordinary output. +# The child's exit status is preserved. +# +# Env files accept leading whitespace, an optional export prefix, comments, +# matching single or double quotes, and quoted values that span lines. +# Mechanics and limits are owned by this help text; worker briefs point here. +# +# Usage: +# fm-secrets.sh names +# fm-secrets.sh has ... +# fm-secrets.sh has --service ... +# fm-secrets.sh run --only NAME[,NAME...] -- +set -eu + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +usage() { + awk ' + NR == 1 { next } + /^#/ { sub(/^# ?/, ""); print; next } + { exit } + ' "$0" +} + +case "${1:-}" in + -h|--help) usage; exit 0 ;; +esac + +if ! command -v python3 >/dev/null 2>&1; then + echo "fm-secrets: python3 is required" >&2 + exit 1 +fi + +exec python3 "$SCRIPT_DIR/fm-secrets.py" "$@" diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index 418dd3a33ca..557d49303b5 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -1283,8 +1283,8 @@ test_crewmate_scaffolds_forbid_pool_administration() { # One shared string, not two copies: the emitted rule must be byte-identical # across the ship and scout scaffolds so a later edit cannot fix one and miss # the other. - ship_rule=$(awk '/^7\. Never administer/,/^$/' "$home/data/brief-pool-no-mistakes/brief.md") - scout_rule=$(awk '/^7\. Never administer/,/^$/' "$brief") + ship_rule=$(awk '/^8\. Never administer/,/^$/' "$home/data/brief-pool-no-mistakes/brief.md") + scout_rule=$(awk '/^8\. Never administer/,/^$/' "$brief") [ -n "$ship_rule" ] || fail "ship brief emitted no shared-infrastructure rule to compare" [ "$ship_rule" = "$scout_rule" ] \ || fail "ship and scout shared-infrastructure rules have drifted apart" @@ -1307,6 +1307,44 @@ test_crewmate_scaffolds_forbid_pool_administration() { pass "fm-brief.sh: every crewmate scaffold forbids administering the shared worktree pool" } +test_crewmate_scaffolds_require_the_settings_tool() { + local home id brief mode ship_rule scout_rule + home="$TMP_ROOT/settings-rule-home" + mkdir -p "$home/data" + + for mode in no-mistakes direct-PR local-only; do + id="brief-settings-$mode" + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" alpha --mode "$mode" >/dev/null 2>&1 \ + || fail "fm-brief.sh --mode $mode exited non-zero" + brief="$home/data/$id/brief.md" + assert_grep 'bin/fm-secrets.sh' "$brief" "$mode ship brief omitted the settings tool" + assert_grep "$ROOT/bin/fm-secrets.sh" "$brief" \ + "$mode ship brief did not render the firstmate-owned tool's absolute path" + # shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. + assert_grep 'never read `/proc/*/environ`' "$brief" \ + "$mode ship brief did not prohibit direct process-environment reads" + # shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. + assert_grep 'never run `systemctl show Environment` directly' "$brief" \ + "$mode ship brief did not prohibit direct systemd environment reads" + done + + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-settings-scout alpha --scout >/dev/null 2>&1 \ + || fail "fm-brief.sh --scout exited non-zero" + brief="$home/data/brief-settings-scout/brief.md" + ship_rule=$(awk '/^7[.] Use /,/^$/' "$home/data/brief-settings-no-mistakes/brief.md") + scout_rule=$(awk '/^7[.] Use /,/^$/' "$brief") + [ -n "$ship_rule" ] || fail "ship brief emitted no shared settings rule to compare" + [ "$ship_rule" = "$scout_rule" ] \ + || fail "ship and scout shared settings rules have drifted apart" + # shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. + assert_grep 'Never use `cat`, `sed`, `nl`, or `grep`' "$brief" \ + "scout brief omitted the direct settings-file read prohibition" + # shellcheck disable=SC2016 # Backtick-wrapped option is literal brief text. + assert_grep 'its `--help` owns' "$brief" "scout brief did not point to the mechanics owner" + + pass "fm-brief.sh: every crewmate scaffold requires the safe settings tool" +} + test_script_parses test_no_heredoc_in_command_substitution test_help_includes_entire_header @@ -1341,3 +1379,4 @@ test_branch_prefix_is_refused_where_it_does_not_apply test_branch_prefix_value_is_validated test_branch_prefix_command_is_shell_safe test_crewmate_scaffolds_forbid_pool_administration +test_crewmate_scaffolds_require_the_settings_tool diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh new file mode 100755 index 00000000000..05300eb9fc1 --- /dev/null +++ b/tests/fm-secrets.test.sh @@ -0,0 +1,149 @@ +#!/usr/bin/env bash +# Behavior tests for the settings inspection and scrub boundary. +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +TMP_ROOT=$(fm_test_tmproot fm-secrets) +TOOL="$ROOT/bin/fm-secrets.sh" +ENV_FILE="$TMP_ROOT/settings.env" +FAKEBIN="$TMP_ROOT/fakebin" +mkdir -p "$FAKEBIN" + +FAKE_URL_PASSWORD=fake_url_password_20260924 +FAKE_QUOTED=fake_quoted_value_20260924 +FAKE_MULTI_A=fake_multiline_first_20260924 +FAKE_MULTI_B=fake_multiline_second_20260924 +FAKE_NON_INJECTED=fake_non_injected_value_20260924 +FAKE_PROCESS=fake_process_value_20260924 +FAKE_INLINE=fake_inline_value_20260924 + +printf '%s\n' \ + '# synthetic settings - never real credentials' \ + " export INDENTED_URL = \"postgres://fake_user:${FAKE_URL_PASSWORD}@fake.example/db\" # trailing comment" \ + "QUOTED_VALUE='${FAKE_QUOTED}'" \ + "MULTI_VALUE=\"${FAKE_MULTI_A}" \ + 'LOOKS_LIKE_A_NAME=still_part_of_the_value' \ + "${FAKE_MULTI_B}\"" \ + "URL_PASSWORD=${FAKE_URL_PASSWORD}" \ + "NON_INJECTED_VALUE=${FAKE_NON_INJECTED}" \ + 'SHORT_VALUE=abc' > "$ENV_FILE" + +assert_no_fake_secret() { + local output=$1 context=$2 marker + for marker in \ + "$FAKE_URL_PASSWORD" "$FAKE_QUOTED" "$FAKE_MULTI_A" "$FAKE_MULTI_B" \ + "$FAKE_NON_INJECTED" "$FAKE_PROCESS" "$FAKE_INLINE"; do + assert_not_contains "$output" "$marker" "$context leaked a synthetic secret byte sequence" + done +} + +test_names_and_has_never_print_values() { + local names has expected + names=$($TOOL names "$ENV_FILE" 2>&1) || fail "names failed" + expected=$(printf '%s\n' INDENTED_URL QUOTED_VALUE MULTI_VALUE URL_PASSWORD NON_INJECTED_VALUE SHORT_VALUE) + [ "$names" = "$expected" ] || fail "names did not parse supported env syntax: $names" + assert_no_fake_secret "$names" "names" + assert_not_contains "$names" 'LOOKS_LIKE_A_NAME' \ + "names treated a quoted multiline value as a new assignment" + + has=$($TOOL has "$ENV_FILE" INDENTED_URL MULTI_VALUE ABSENT_SETTING 2>&1) || fail "has failed" + expected=$(printf '%s\n' 'INDENTED_URL=yes' 'MULTI_VALUE=yes' 'ABSENT_SETTING=no') + [ "$has" = "$expected" ] || fail "has returned unexpected booleans: $has" + assert_no_fake_secret "$has" "has" + pass "fm-secrets: names and has expose names and booleans only" +} + +test_run_scrubs_all_file_values_and_preserves_status() { + local output rc + # shellcheck disable=SC2016 # The child expands only its deliberately injected environment. + output=$(SHORT_VALUE=ambient-value FM_FAKE_LITERAL="$FAKE_NON_INJECTED" $TOOL run "$ENV_FILE" \ + --only INDENTED_URL,QUOTED_VALUE,MULTI_VALUE,URL_PASSWORD -- \ + sh -c ' + printf "%s\n" "$QUOTED_VALUE" + printf "%s\n" "$MULTI_VALUE" >&2 + printf "postgres://another-user:%s@another.example/db\n" "$URL_PASSWORD" + printf "%s\n" "$INDENTED_URL" + printf "%s\n" "$FM_FAKE_LITERAL" + printf "not-selected=%s\n" "${SHORT_VALUE-unset}" + exit 37 + ' 2>&1) + rc=$? + expect_code 37 "$rc" "run must preserve the child exit status" + assert_no_fake_secret "$output" "run" + assert_contains "$output" '' "run did not scrub an injected quoted value" + assert_contains "$output" '' "run did not scrub an injected multiline value" + assert_contains "$output" '' "run did not scrub a password in URL userinfo" + assert_contains "$output" '' \ + "run did not scrub a file value that was outside --only" + assert_contains "$output" 'not-selected=unset' "run injected a file setting outside --only" + pass "fm-secrets: run scrubs stdout and stderr and preserves child status" +} + +test_service_has_reads_process_environment_without_values() { + local service_pid output expected + env FM_FAKE_PROCESS_SETTING="$FAKE_PROCESS" sleep 30 & + service_pid=$! + trap 'kill "$service_pid" 2>/dev/null || true; wait "$service_pid" 2>/dev/null || true; fm_test_cleanup' EXIT + + # shellcheck disable=SC2016 # The generated stub expands this at execution time. + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "%s\n" "${FM_TEST_SERVICE_PID:?}" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_SERVICE_PID="$service_pid" \ + $TOOL has --service fake-running.service FM_FAKE_PROCESS_SETTING ABSENT_SETTING 2>&1) \ + || fail "service has failed for a running process" + expected=$(printf '%s\n' 'FM_FAKE_PROCESS_SETTING=yes' 'ABSENT_SETTING=no') + [ "$output" = "$expected" ] || fail "service has returned unexpected process booleans: $output" + assert_no_fake_secret "$output" "service process has" + + kill "$service_pid" 2>/dev/null || true + wait "$service_pid" 2>/dev/null || true + trap fm_test_cleanup EXIT + pass "fm-secrets: service has inspects a running process without exposing values" +} + +test_service_has_falls_back_to_unit_settings() { + local unit_env output expected + unit_env="$TMP_ROOT/unit.env" + printf '%s\n' "UNIT_FILE_SETTING=${FAKE_QUOTED}" > "$unit_env" + # shellcheck disable=SC2016 # The generated stub expands these at execution time. + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "0\n" ;;' \ + ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ + ' *--property=Environment*) printf "INLINE_SETTING=%s\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ + $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING ABSENT_SETTING 2>&1) \ + || fail "service has failed for unit declarations" + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'ABSENT_SETTING=no') + [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" + assert_no_fake_secret "$output" "service declaration has" + pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" +} + +test_help_owns_the_scrub_limit() { + local help + help=$($TOOL --help) || fail "--help failed" + assert_contains "$help" 'at least 6 bytes long' "help omitted the scrub threshold" + assert_contains "$help" 'shorter than 6 bytes' "help omitted the short-value limit" + assert_contains "$help" 'exit status is preserved' "help omitted child status behavior" + pass "fm-secrets: help documents the scrub boundary" +} + +test_names_and_has_never_print_values +test_run_scrubs_all_file_values_and_preserves_status +test_service_has_reads_process_environment_without_values +test_service_has_falls_back_to_unit_settings +test_help_owns_the_scrub_limit From 9003711b18b59a725728b8670e9afca6ad77e649 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:05:18 -0600 Subject: [PATCH 02/17] no-mistakes(review): Harden settings redaction and worker isolation --- bin/fm-brief.sh | 16 +++++++++------- bin/fm-secrets.py | 26 ++++++++++++++++---------- bin/fm-secrets.sh | 14 ++++++++------ tests/fm-brief.test.sh | 14 ++++++++++---- tests/fm-secrets.test.sh | 32 ++++++++++++++++++++++++++------ 5 files changed, 69 insertions(+), 33 deletions(-) diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 86e940f690d..a17213082f6 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -327,6 +327,12 @@ shell_quote() { printf "'" } +FM_SECRETS_TOOL=$(shell_quote "$FM_ROOT/bin/fm-secrets.sh") +# shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. +SHARED_SETTINGS_RULE=$(printf '%s\n' \ + "7. Use \`$FM_SECRETS_TOOL\` for every settings file or service environment; its \`--help\` owns the safe operations and limits." \ + ' Never use `cat`, `sed`, `nl`, or `grep` to read settings values, never read `/proc/*/environ`, and never run `systemctl show Environment` directly.') + STATUS_FILE=$(shell_quote "$STATE/$ID.status") # The worker's status command: the plain append always carries the line, then # the opt-in fleet ledger (docs/fleet-ledger.md) records it at once, costing one @@ -431,6 +437,8 @@ When a keyed phase ends without another reportable state, append \`resolved [key The main firstmate's answer normally writes that closing line at answer time; when a blocker or wait clears WITHOUT an answer from the main firstmate, append \`resolved [at=]: {how it cleared}\` yourself (keyed with \`[key=]\` if you opened it with one) as your domain resumes. Routine internal supervision, heartbeats, retries, and crewmate churn stay inside your own home and must not touch that status file. +$SHARED_SETTINGS_RULE + # Definition of done You are persistent by default. Do not exit just because your queue is empty. On startup and restart, run normal firstmate bootstrap and recovery through \`bin/fm-session-start.sh\` for your own home, but only to RECONCILE work that is already yours: in-flight crewmates, tracked backlog items, and durable watches recorded in this home. @@ -492,14 +500,8 @@ TASK_SECTION=${TASK_SECTION%$'\n'} # Shared strings keep the ship and scout safety rules identical. # Rule 2 governs file edits, so it does not prohibit pool administration. -# The secondmate charter deliberately omits this rule because a secondmate +# The secondmate charter deliberately omits the infrastructure rule because a secondmate # legitimately allocates and returns slots for crewmates in its own home. -FM_SECRETS_TOOL=$(shell_quote "$FM_ROOT/bin/fm-secrets.sh") -# shellcheck disable=SC2016 # Backtick-wrapped commands are literal brief text. -SHARED_SETTINGS_RULE=$(printf '%s\n' \ - "7. Use \`$FM_SECRETS_TOOL\` for every settings file or service environment; its \`--help\` owns the safe operations and limits." \ - ' Never use `cat`, `sed`, `nl`, or `grep` to read settings values, never read `/proc/*/environ`, and never run `systemctl show Environment` directly.') - IFS= read -r -d '' SHARED_INFRA_RULE <<'EOF' || true 8. Never administer infrastructure that every lane shares. Two things are shared: - The `no-mistakes` daemon - one instance serving every lane/home, so stopping, restarting, or diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index e4b2e130713..8205d01fc6e 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -28,6 +28,7 @@ rb"([A-Za-z][A-Za-z0-9+.-]*://[^\s/@:]+:)([^\s/@]+)(@[^\s]+)" ) MIN_SCRUB_BYTES = 6 +SECRET_NAME_RE = re.compile(r"PASS|PWD|SECRET|TOKEN|KEY|PIN|CREDENTIAL|AUTH", re.IGNORECASE) class SecretToolError(Exception): @@ -164,6 +165,10 @@ def final_values(assignments: Iterable[Assignment]) -> dict[str, str]: return {assignment.name: assignment.value for assignment in assignments} +def is_secret_name(name: str) -> bool: + return SECRET_NAME_RE.search(name) is not None + + def validate_names(names: Sequence[str]) -> None: if not names: raise SecretToolError("at least one setting name is required") @@ -306,12 +311,11 @@ def known_scrubbers(assignments: Iterable[Assignment]) -> list[tuple[bytes, byte for assignment in assignments: encoded = assignment.value.encode("utf-8", errors="surrogateescape") replacement = f"".encode("ascii") - if len(encoded) >= MIN_SCRUB_BYTES: + if len(encoded) >= MIN_SCRUB_BYTES or is_secret_name(assignment.name): scrubbers.setdefault(encoded, replacement) for match in URL_PASSWORD_RE.finditer(encoded): password = match.group(2) - if len(password) >= MIN_SCRUB_BYTES: - scrubbers.setdefault(password, replacement) + scrubbers.setdefault(password, replacement) return sorted(scrubbers.items(), key=lambda item: len(item[0]), reverse=True) @@ -320,9 +324,6 @@ def scrub_output(data: bytes, scrubbers: Sequence[tuple[bytes, bytes]]) -> bytes data = data.replace(value, replacement) def redact_url_password(match: re.Match[bytes]) -> bytes: - password = match.group(2) - if len(password) < MIN_SCRUB_BYTES: - return match.group(0) return match.group(1) + b"" + match.group(3) return URL_PASSWORD_RE.sub(redact_url_password, data) @@ -350,9 +351,12 @@ def command_run(args: Sequence[str]) -> int: if missing: raise SecretToolError("settings file is missing requested names: " + ",".join(missing)) - child_env = os.environ.copy() - for name in values: - child_env.pop(name, None) + child_env = { + name: value + for name, value in os.environ.items() + if name in {"PATH", "HOME", "USER", "LOGNAME", "LANG", "TERM", "TMPDIR", "SHELL", "PWD"} + or name.startswith("LC_") + } for name in requested: child_env[name] = values[name] @@ -367,7 +371,9 @@ def command_run(args: Sequence[str]) -> int: except OSError as exc: raise SecretToolError("command could not be started") from exc - scrubbers = known_scrubbers(assignments) + scrubbers = known_scrubbers( + [*assignments, *(Assignment(name, value) for name, value in child_env.items() if is_secret_name(name))] + ) sys.stdout.buffer.write(scrub_output(child.stdout, scrubbers)) sys.stderr.buffer.write(scrub_output(child.stderr, scrubbers)) sys.stdout.buffer.flush() diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh index bf6fbbc1ac3..1c81c38b4ed 100755 --- a/bin/fm-secrets.sh +++ b/bin/fm-secrets.sh @@ -11,12 +11,14 @@ # is readable; otherwise the unit's EnvironmentFile= and Environment= # declarations are inspected. # run --only NAME[,NAME...] -- -# Remove every variable named by the file from the inherited environment, -# add only the selected names, run the command, and scrub its stdout and -# stderr. Every file value at least 6 bytes long is replaced with -# . Passwords at least 6 bytes long in URL userinfo are -# scrubbed too. Values shorter than 6 bytes are deliberately not scrubbed -# because replacing common short strings would corrupt ordinary output. +# Inherit only PATH, HOME, USER, LOGNAME, LANG, LC_*, TERM, TMPDIR, SHELL, +# and PWD, add only the selected names, run the command, and scrub its +# stdout and stderr. Every file value at least 6 bytes long is replaced +# with . Values of PASS, PWD, SECRET, TOKEN, KEY, PIN, +# CREDENTIAL, or AUTH settings and URL userinfo passwords are scrubbed at +# any length. Other values shorter than 6 bytes are deliberately not +# scrubbed because replacing common short strings would corrupt ordinary +# output. # The child's exit status is preserved. # # Env files accept leading whitespace, an optional export prefix, comments, diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index 557d49303b5..8ded2182971 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -1308,7 +1308,7 @@ test_crewmate_scaffolds_forbid_pool_administration() { } test_crewmate_scaffolds_require_the_settings_tool() { - local home id brief mode ship_rule scout_rule + local home id brief mode ship_rule scout_rule secondmate_rule home="$TMP_ROOT/settings-rule-home" mkdir -p "$home/data" @@ -1331,8 +1331,8 @@ test_crewmate_scaffolds_require_the_settings_tool() { FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-settings-scout alpha --scout >/dev/null 2>&1 \ || fail "fm-brief.sh --scout exited non-zero" brief="$home/data/brief-settings-scout/brief.md" - ship_rule=$(awk '/^7[.] Use /,/^$/' "$home/data/brief-settings-no-mistakes/brief.md") - scout_rule=$(awk '/^7[.] Use /,/^$/' "$brief") + ship_rule=$(awk '/^7[.] Use / { print; getline; print }' "$home/data/brief-settings-no-mistakes/brief.md") + scout_rule=$(awk '/^7[.] Use / { print; getline; print }' "$brief") [ -n "$ship_rule" ] || fail "ship brief emitted no shared settings rule to compare" [ "$ship_rule" = "$scout_rule" ] \ || fail "ship and scout shared settings rules have drifted apart" @@ -1342,7 +1342,13 @@ test_crewmate_scaffolds_require_the_settings_tool() { # shellcheck disable=SC2016 # Backtick-wrapped option is literal brief text. assert_grep 'its `--help` owns' "$brief" "scout brief did not point to the mechanics owner" - pass "fm-brief.sh: every crewmate scaffold requires the safe settings tool" + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-settings-secondmate --secondmate --no-projects >/dev/null 2>&1 \ + || fail "fm-brief.sh --secondmate exited non-zero" + secondmate_rule=$(awk '/^7[.] Use / { print; getline; print }' "$home/data/brief-settings-secondmate/brief.md") + [ "$ship_rule" = "$secondmate_rule" ] \ + || fail "secondmate charter omitted or changed the shared settings rule" + + pass "fm-brief.sh: every worker scaffold requires the safe settings tool" } test_script_parses diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 05300eb9fc1..93f5ac8a808 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -18,6 +18,8 @@ FAKE_MULTI_B=fake_multiline_second_20260924 FAKE_NON_INJECTED=fake_non_injected_value_20260924 FAKE_PROCESS=fake_process_value_20260924 FAKE_INLINE=fake_inline_value_20260924 +FAKE_PIN=12345 +FAKE_AMBIENT_KEY=fake_ambient_typesafe_key_20260924 printf '%s\n' \ '# synthetic settings - never real credentials' \ @@ -28,13 +30,15 @@ printf '%s\n' \ "${FAKE_MULTI_B}\"" \ "URL_PASSWORD=${FAKE_URL_PASSWORD}" \ "NON_INJECTED_VALUE=${FAKE_NON_INJECTED}" \ + "PIN=${FAKE_PIN}" \ 'SHORT_VALUE=abc' > "$ENV_FILE" assert_no_fake_secret() { local output=$1 context=$2 marker for marker in \ "$FAKE_URL_PASSWORD" "$FAKE_QUOTED" "$FAKE_MULTI_A" "$FAKE_MULTI_B" \ - "$FAKE_NON_INJECTED" "$FAKE_PROCESS" "$FAKE_INLINE"; do + "$FAKE_NON_INJECTED" "$FAKE_PROCESS" "$FAKE_INLINE" "$FAKE_PIN" \ + "$FAKE_AMBIENT_KEY"; do assert_not_contains "$output" "$marker" "$context leaked a synthetic secret byte sequence" done } @@ -42,7 +46,7 @@ assert_no_fake_secret() { test_names_and_has_never_print_values() { local names has expected names=$($TOOL names "$ENV_FILE" 2>&1) || fail "names failed" - expected=$(printf '%s\n' INDENTED_URL QUOTED_VALUE MULTI_VALUE URL_PASSWORD NON_INJECTED_VALUE SHORT_VALUE) + expected=$(printf '%s\n' INDENTED_URL QUOTED_VALUE MULTI_VALUE URL_PASSWORD NON_INJECTED_VALUE PIN SHORT_VALUE) [ "$names" = "$expected" ] || fail "names did not parse supported env syntax: $names" assert_no_fake_secret "$names" "names" assert_not_contains "$names" 'LOOKS_LIKE_A_NAME' \ @@ -58,17 +62,18 @@ test_names_and_has_never_print_values() { test_run_scrubs_all_file_values_and_preserves_status() { local output rc # shellcheck disable=SC2016 # The child expands only its deliberately injected environment. - output=$(SHORT_VALUE=ambient-value FM_FAKE_LITERAL="$FAKE_NON_INJECTED" $TOOL run "$ENV_FILE" \ - --only INDENTED_URL,QUOTED_VALUE,MULTI_VALUE,URL_PASSWORD -- \ + output=$(SHORT_VALUE=ambient-value $TOOL run "$ENV_FILE" \ + --only INDENTED_URL,QUOTED_VALUE,MULTI_VALUE,URL_PASSWORD,PIN -- \ sh -c ' printf "%s\n" "$QUOTED_VALUE" printf "%s\n" "$MULTI_VALUE" >&2 printf "postgres://another-user:%s@another.example/db\n" "$URL_PASSWORD" printf "%s\n" "$INDENTED_URL" - printf "%s\n" "$FM_FAKE_LITERAL" + printf "%s\n" "$1" printf "not-selected=%s\n" "${SHORT_VALUE-unset}" + printf "pin=%s\n" "$PIN" exit 37 - ' 2>&1) + ' -- "$FAKE_NON_INJECTED" 2>&1) rc=$? expect_code 37 "$rc" "run must preserve the child exit status" assert_no_fake_secret "$output" "run" @@ -78,9 +83,22 @@ test_run_scrubs_all_file_values_and_preserves_status() { assert_contains "$output" '' \ "run did not scrub a file value that was outside --only" assert_contains "$output" 'not-selected=unset' "run injected a file setting outside --only" + assert_contains "$output" '' "run did not scrub a short secret-bearing value" pass "fm-secrets: run scrubs stdout and stderr and preserves child status" } +test_run_excludes_and_scrubs_ambient_secret_settings() { + local output + # shellcheck disable=SC2016 # The child expands only its deliberately injected environment. + output=$(TYPESAFE_API_KEY="$FAKE_AMBIENT_KEY" $TOOL run "$ENV_FILE" --only PIN -- \ + sh -c 'printf "ambient=%s\n" "${TYPESAFE_API_KEY-unset}"; printf "pin=%s\n" "$PIN"' 2>&1) \ + || fail "run failed while excluding an ambient secret setting" + assert_no_fake_secret "$output" "run ambient settings" + assert_contains "$output" 'ambient=unset' "run inherited an ambient secret setting" + assert_contains "$output" '' "run did not scrub the requested short PIN" + pass "fm-secrets: run excludes ambient secrets and scrubs short secret names" +} + test_service_has_reads_process_environment_without_values() { local service_pid output expected env FM_FAKE_PROCESS_SETTING="$FAKE_PROCESS" sleep 30 & @@ -138,12 +156,14 @@ test_help_owns_the_scrub_limit() { help=$($TOOL --help) || fail "--help failed" assert_contains "$help" 'at least 6 bytes long' "help omitted the scrub threshold" assert_contains "$help" 'shorter than 6 bytes' "help omitted the short-value limit" + assert_contains "$help" 'URL userinfo passwords' "help omitted the URL password exception" assert_contains "$help" 'exit status is preserved' "help omitted child status behavior" pass "fm-secrets: help documents the scrub boundary" } test_names_and_has_never_print_values test_run_scrubs_all_file_values_and_preserves_status +test_run_excludes_and_scrubs_ambient_secret_settings test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_help_owns_the_scrub_limit From 5db5b97193b23f900b7b7a984874b6656297a4a3 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:07:47 -0600 Subject: [PATCH 03/17] no-mistakes(review): Skip empty secret scrubbers --- bin/fm-secrets.py | 2 +- tests/fm-secrets.test.sh | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 8205d01fc6e..72c2d9e56fe 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -311,7 +311,7 @@ def known_scrubbers(assignments: Iterable[Assignment]) -> list[tuple[bytes, byte for assignment in assignments: encoded = assignment.value.encode("utf-8", errors="surrogateescape") replacement = f"".encode("ascii") - if len(encoded) >= MIN_SCRUB_BYTES or is_secret_name(assignment.name): + if encoded and (len(encoded) >= MIN_SCRUB_BYTES or is_secret_name(assignment.name)): scrubbers.setdefault(encoded, replacement) for match in URL_PASSWORD_RE.finditer(encoded): password = match.group(2) diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 93f5ac8a808..28f802fa866 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -31,6 +31,7 @@ printf '%s\n' \ "URL_PASSWORD=${FAKE_URL_PASSWORD}" \ "NON_INJECTED_VALUE=${FAKE_NON_INJECTED}" \ "PIN=${FAKE_PIN}" \ + 'TOKEN=' \ 'SHORT_VALUE=abc' > "$ENV_FILE" assert_no_fake_secret() { @@ -46,7 +47,7 @@ assert_no_fake_secret() { test_names_and_has_never_print_values() { local names has expected names=$($TOOL names "$ENV_FILE" 2>&1) || fail "names failed" - expected=$(printf '%s\n' INDENTED_URL QUOTED_VALUE MULTI_VALUE URL_PASSWORD NON_INJECTED_VALUE PIN SHORT_VALUE) + expected=$(printf '%s\n' INDENTED_URL QUOTED_VALUE MULTI_VALUE URL_PASSWORD NON_INJECTED_VALUE PIN TOKEN SHORT_VALUE) [ "$names" = "$expected" ] || fail "names did not parse supported env syntax: $names" assert_no_fake_secret "$names" "names" assert_not_contains "$names" 'LOOKS_LIKE_A_NAME' \ @@ -99,6 +100,14 @@ test_run_excludes_and_scrubs_ambient_secret_settings() { pass "fm-secrets: run excludes ambient secrets and scrubs short secret names" } +test_run_ignores_empty_secret_values() { + local output + output=$($TOOL run "$ENV_FILE" --only TOKEN -- printf x 2>&1) \ + || fail "run failed with an empty secret setting" + [ "$output" = x ] || fail "run let an empty secret setting corrupt command output: $output" + pass "fm-secrets: empty secret settings do not corrupt output" +} + test_service_has_reads_process_environment_without_values() { local service_pid output expected env FM_FAKE_PROCESS_SETTING="$FAKE_PROCESS" sleep 30 & @@ -164,6 +173,7 @@ test_help_owns_the_scrub_limit() { test_names_and_has_never_print_values test_run_scrubs_all_file_values_and_preserves_status test_run_excludes_and_scrubs_ambient_secret_settings +test_run_ignores_empty_secret_values test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_help_owns_the_scrub_limit From 1c8ef5f07e2f9b0d303d46259702791abdbd1565 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:12:18 -0600 Subject: [PATCH 04/17] no-mistakes(review): Scrub empty-username URL passwords --- bin/fm-secrets.py | 2 +- tests/fm-secrets.test.sh | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 72c2d9e56fe..1ca069b56b2 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -25,7 +25,7 @@ r"[ \t]*(?:export[ \t]+)?([A-Za-z_][A-Za-z0-9_]*)[ \t]*=[ \t]*" ) URL_PASSWORD_RE = re.compile( - rb"([A-Za-z][A-Za-z0-9+.-]*://[^\s/@:]+:)([^\s/@]+)(@[^\s]+)" + rb"([A-Za-z][A-Za-z0-9+.-]*://[^\s/@:]*:)([^\s/@]+)(@[^\s]+)" ) MIN_SCRUB_BYTES = 6 SECRET_NAME_RE = re.compile(r"PASS|PWD|SECRET|TOKEN|KEY|PIN|CREDENTIAL|AUTH", re.IGNORECASE) diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 28f802fa866..a66aae58f76 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -108,6 +108,19 @@ test_run_ignores_empty_secret_values() { pass "fm-secrets: empty secret settings do not corrupt output" } +test_run_scrubs_empty_username_url_passwords() { + local env_file output + env_file="$TMP_ROOT/empty-username.env" + printf '%s\n' 'DATABASE_URL=postgres://:12345@db/x' > "$env_file" + output=$($TOOL run "$env_file" --only DATABASE_URL -- \ + sh -c 'password=${DATABASE_URL#*://:}; printf "%s\n" "${password%@*}"' 2>&1) \ + || fail "run failed with an empty URL username" + assert_not_contains "$output" '12345' "run leaked an empty-username URL password" + assert_contains "$output" '' \ + "run did not scrub an empty-username URL password" + pass "fm-secrets: run scrubs empty-username URL passwords" +} + test_service_has_reads_process_environment_without_values() { local service_pid output expected env FM_FAKE_PROCESS_SETTING="$FAKE_PROCESS" sleep 30 & @@ -174,6 +187,7 @@ test_names_and_has_never_print_values test_run_scrubs_all_file_values_and_preserves_status test_run_excludes_and_scrubs_ambient_secret_settings test_run_ignores_empty_secret_values +test_run_scrubs_empty_username_url_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_help_owns_the_scrub_limit From 5918778bb073f7707113e03be0647e3fa6bbdd75 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:16:36 -0600 Subject: [PATCH 05/17] no-mistakes(review): Scrub decoded URLs and unset service variables --- bin/fm-secrets.py | 13 +++++++++++++ tests/fm-secrets.test.sh | 23 +++++++++++++++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 1ca069b56b2..109e1daccc1 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -18,6 +18,7 @@ from dataclasses import dataclass from pathlib import Path from typing import Iterable, Sequence +from urllib.parse import unquote_to_bytes NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") @@ -235,6 +236,14 @@ def environment_declaration_names(raw: str) -> set[str]: return names +def unset_environment_names(raw: str) -> set[str]: + try: + words = shlex.split(raw, posix=True) + except ValueError as exc: + raise SecretToolError("systemd UnsetEnvironment= data could not be parsed safely") from exc + return {word.partition("=")[0] for word in words if NAME_RE.fullmatch(word.partition("=")[0])} + + def environment_file_specs(raw: str) -> list[tuple[str, bool]]: try: words = shlex.split(raw, posix=True) @@ -274,6 +283,9 @@ def service_names(unit: str) -> set[str]: raise SecretToolError( "cannot inspect a required systemd EnvironmentFile safely" ) from exc + names.difference_update( + unset_environment_names(systemctl_property(unit, "UnsetEnvironment")) + ) return names @@ -316,6 +328,7 @@ def known_scrubbers(assignments: Iterable[Assignment]) -> list[tuple[bytes, byte for match in URL_PASSWORD_RE.finditer(encoded): password = match.group(2) scrubbers.setdefault(password, replacement) + scrubbers.setdefault(unquote_to_bytes(password), replacement) return sorted(scrubbers.items(), key=lambda item: len(item[0]), reverse=True) diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index a66aae58f76..3ed028c827a 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -121,6 +121,19 @@ test_run_scrubs_empty_username_url_passwords() { pass "fm-secrets: run scrubs empty-username URL passwords" } +test_run_scrubs_url_decoded_passwords() { + local env_file output + env_file="$TMP_ROOT/encoded-password.env" + printf '%s\n' 'DATABASE_URL=postgres://user:a%20b@db/x' > "$env_file" + output=$($TOOL run "$env_file" --only DATABASE_URL -- \ + python3 -c 'from os import environ; from urllib.parse import unquote; print(unquote(environ["DATABASE_URL"].split("@", 1)[0].rsplit(":", 1)[1]))' 2>&1) \ + || fail "run failed with a percent-encoded URL password" + assert_not_contains "$output" 'a b' "run leaked a decoded URL password" + assert_contains "$output" '' \ + "run did not scrub a decoded URL password" + pass "fm-secrets: run scrubs decoded URL passwords" +} + test_service_has_reads_process_environment_without_values() { local service_pid output expected env FM_FAKE_PROCESS_SETTING="$FAKE_PROCESS" sleep 30 & @@ -152,22 +165,23 @@ test_service_has_reads_process_environment_without_values() { test_service_has_falls_back_to_unit_settings() { local unit_env output expected unit_env="$TMP_ROOT/unit.env" - printf '%s\n' "UNIT_FILE_SETTING=${FAKE_QUOTED}" > "$unit_env" + printf '%s\n' "UNIT_FILE_SETTING=${FAKE_QUOTED}" 'UNSET_FILE_SETTING=value' > "$unit_env" # shellcheck disable=SC2016 # The generated stub expands these at execution time. printf '%s\n' \ '#!/usr/bin/env bash' \ 'case "$*" in' \ ' *--property=MainPID*) printf "0\n" ;;' \ ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ - ' *--property=Environment*) printf "INLINE_SETTING=%s\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ + ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ + ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ - $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING ABSENT_SETTING 2>&1) \ + $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING ABSENT_SETTING 2>&1) \ || fail "service has failed for unit declarations" - expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'ABSENT_SETTING=no') + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'ABSENT_SETTING=no') [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" assert_no_fake_secret "$output" "service declaration has" pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" @@ -188,6 +202,7 @@ test_run_scrubs_all_file_values_and_preserves_status test_run_excludes_and_scrubs_ambient_secret_settings test_run_ignores_empty_secret_values test_run_scrubs_empty_username_url_passwords +test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_help_owns_the_scrub_limit From de0af3b76b1c4e995807e7dd09b4885eda0772e0 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:21:52 -0600 Subject: [PATCH 06/17] no-mistakes(review): Preserve exact systemd unset assignments --- bin/fm-secrets.py | 39 +++++++++++++++++++++++++++------------ tests/fm-secrets.test.sh | 14 +++++++++----- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 109e1daccc1..c3aa442962a 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -4,6 +4,7 @@ The shell entry point owns the public help and invocation contract. This helper keeps secret-bearing data inside one process and never includes a value in an error or diagnostic. +Byte-exact parsing and scrubbing deliberately use Python, following existing bin-helper precedent. """ from __future__ import annotations @@ -223,25 +224,30 @@ def process_environment_names(pid_text: str) -> set[str] | None: return names -def environment_declaration_names(raw: str) -> set[str]: +def environment_declaration_assignments(raw: str) -> list[Assignment]: try: words = shlex.split(raw, posix=True) except ValueError as exc: raise SecretToolError("systemd Environment= data could not be parsed safely") from exc - names: set[str] = set() + assignments: list[Assignment] = [] for word in words: - name, separator, _value = word.partition("=") + name, separator, value = word.partition("=") if separator and NAME_RE.fullmatch(name): - names.add(name) - return names + assignments.append(Assignment(name, value)) + return assignments -def unset_environment_names(raw: str) -> set[str]: +def unset_environment_assignments(raw: str) -> list[tuple[str, str | None]]: try: words = shlex.split(raw, posix=True) except ValueError as exc: raise SecretToolError("systemd UnsetEnvironment= data could not be parsed safely") from exc - return {word.partition("=")[0] for word in words if NAME_RE.fullmatch(word.partition("=")[0])} + unsets: list[tuple[str, str | None]] = [] + for word in words: + name, separator, value = word.partition("=") + if NAME_RE.fullmatch(name): + unsets.append((name, value if separator else None)) + return unsets def environment_file_specs(raw: str) -> list[tuple[str, bool]]: @@ -272,21 +278,30 @@ def service_names(unit: str) -> set[str]: if running is not None: return running - names = environment_declaration_names(systemctl_property(unit, "Environment")) + assignments = environment_declaration_assignments( + systemctl_property(unit, "Environment") + ) files = environment_file_specs(systemctl_property(unit, "EnvironmentFiles")) for path, ignore_errors in files: try: - names.update(unique_names(parse_env_file(path))) + assignments.extend(parse_env_file(path)) except SecretToolError as exc: if ignore_errors: continue raise SecretToolError( "cannot inspect a required systemd EnvironmentFile safely" ) from exc - names.difference_update( - unset_environment_names(systemctl_property(unit, "UnsetEnvironment")) + unsets = unset_environment_assignments( + systemctl_property(unit, "UnsetEnvironment") ) - return names + return { + assignment.name + for assignment in assignments + if not any( + assignment.name == name and (value is None or assignment.value == value) + for name, value in unsets + ) + } def command_names(args: Sequence[str]) -> int: diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 3ed028c827a..04f78b12d7d 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -165,23 +165,27 @@ test_service_has_reads_process_environment_without_values() { test_service_has_falls_back_to_unit_settings() { local unit_env output expected unit_env="$TMP_ROOT/unit.env" - printf '%s\n' "UNIT_FILE_SETTING=${FAKE_QUOTED}" 'UNSET_FILE_SETTING=value' > "$unit_env" + printf '%s\n' \ + "UNIT_FILE_SETTING=${FAKE_QUOTED}" \ + 'UNSET_FILE_SETTING=value' \ + 'EXACT_FILE_MATCH=actual' \ + 'EXACT_FILE_KEEP=actual' > "$unit_env" # shellcheck disable=SC2016 # The generated stub expands these at execution time. printf '%s\n' \ '#!/usr/bin/env bash' \ 'case "$*" in' \ ' *--property=MainPID*) printf "0\n" ;;' \ ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ - ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ - ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING\n" ;;' \ + ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=actual\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ + ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ - $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING ABSENT_SETTING 2>&1) \ + $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP ABSENT_SETTING 2>&1) \ || fail "service has failed for unit declarations" - expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'ABSENT_SETTING=no') + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'ABSENT_SETTING=no') [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" assert_no_fake_secret "$output" "service declaration has" pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" From c0f37c31f98d2c771cc9518d6361e391f4cc9de4 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:25:02 -0600 Subject: [PATCH 07/17] no-mistakes(review): Resolve final systemd fallback values --- bin/fm-secrets.py | 9 +++++---- tests/fm-secrets.test.sh | 11 ++++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index c3aa442962a..da446727f89 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -294,12 +294,13 @@ def service_names(unit: str) -> set[str]: unsets = unset_environment_assignments( systemctl_property(unit, "UnsetEnvironment") ) + values = {assignment.name: assignment.value for assignment in assignments} return { - assignment.name - for assignment in assignments + name + for name, value in values.items() if not any( - assignment.name == name and (value is None or assignment.value == value) - for name, value in unsets + name == unset_name and (unset_value is None or value == unset_value) + for unset_name, unset_value in unsets ) } diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 04f78b12d7d..d74160ce4fb 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -169,23 +169,24 @@ test_service_has_falls_back_to_unit_settings() { "UNIT_FILE_SETTING=${FAKE_QUOTED}" \ 'UNSET_FILE_SETTING=value' \ 'EXACT_FILE_MATCH=actual' \ - 'EXACT_FILE_KEEP=actual' > "$unit_env" + 'EXACT_FILE_KEEP=actual' \ + 'OVERRIDDEN_SETTING=remove' > "$unit_env" # shellcheck disable=SC2016 # The generated stub expands these at execution time. printf '%s\n' \ '#!/usr/bin/env bash' \ 'case "$*" in' \ ' *--property=MainPID*) printf "0\n" ;;' \ ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ - ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=actual\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ - ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other\n" ;;' \ + ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=actual OVERRIDDEN_SETTING=keep\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ + ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other OVERRIDDEN_SETTING=remove\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ - $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP ABSENT_SETTING 2>&1) \ + $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP OVERRIDDEN_SETTING ABSENT_SETTING 2>&1) \ || fail "service has failed for unit declarations" - expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'ABSENT_SETTING=no') + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'OVERRIDDEN_SETTING=no' 'ABSENT_SETTING=no') [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" assert_no_fake_secret "$output" "service declaration has" pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" From b156d983cc9953eee94c70e794770ae2a66e0965 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:30:43 -0600 Subject: [PATCH 08/17] no-mistakes(review): Parse systemd EnvironmentFile escapes faithfully --- bin/fm-secrets.py | 86 +++++++++++++++++++++++++++++++++++++++- tests/fm-secrets.test.sh | 9 +++-- 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index da446727f89..10830768b8f 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -26,6 +26,7 @@ ASSIGNMENT_RE = re.compile( r"[ \t]*(?:export[ \t]+)?([A-Za-z_][A-Za-z0-9_]*)[ \t]*=[ \t]*" ) +SYSTEMD_ASSIGNMENT_RE = re.compile(r"[ \t]*([A-Za-z_][A-Za-z0-9_]*)[ \t]*=[ \t]*") URL_PASSWORD_RE = re.compile( rb"([A-Za-z][A-Za-z0-9+.-]*://[^\s/@:]*:)([^\s/@]+)(@[^\s]+)" ) @@ -153,6 +154,89 @@ def parse_env_file(path: str) -> list[Assignment]: return assignments +def _systemd_unquoted_value(text: str, start: int, path: str) -> tuple[str, int]: + pieces: list[str] = [] + cursor = start + while True: + end = _line_end(text, cursor) + piece = text[cursor:end].rstrip(" \t\r") + trailing = len(piece) - len(piece.rstrip("\\")) + if trailing % 2 and end < len(text): + pieces.append(piece[:-1]) + cursor = end + 1 + continue + pieces.append(piece) + break + value = "".join(pieces) + decoded: list[str] = [] + cursor = 0 + while cursor < len(value): + if value[cursor] == "\\": + if cursor + 1 == len(value): + raise SecretToolError(f"{path}: invalid unquoted escape") + cursor += 1 + decoded.append(value[cursor]) + cursor += 1 + return "".join(decoded), end + + +def _systemd_double_quoted_value(text: str, start: int, path: str) -> tuple[str, int]: + decoded: list[str] = [] + cursor = start + 1 + while cursor < len(text): + char = text[cursor] + if char == '"': + return "".join(decoded), cursor + 1 + if char == "\\" and cursor + 1 < len(text): + following = text[cursor + 1] + if following == "\n": + cursor += 2 + continue + if following in '"\\`$': + decoded.append(following) + cursor += 2 + continue + decoded.append(char) + cursor += 1 + raise SecretToolError(f"{path}: unterminated double-quoted value") + + +def parse_systemd_environment_file(path: str) -> list[Assignment]: + try: + text = Path(path).read_text(encoding="utf-8") + except UnicodeDecodeError as exc: + raise SecretToolError("systemd EnvironmentFile is not valid UTF-8") from exc + except OSError as exc: + raise SecretToolError(f"cannot read settings file: {path}") from exc + + assignments: list[Assignment] = [] + position = 0 + while position < len(text): + end = _line_end(text, position) + line = text[position:end] + stripped = line.lstrip(" \t\r") + if not stripped or stripped.startswith(("#", ";")): + position = end + (end < len(text)) + continue + match = SYSTEMD_ASSIGNMENT_RE.match(line) + if match is None: + position = end + (end < len(text)) + continue + value_start = position + match.end() + if value_start < len(text) and text[value_start] == "'": + value, consumed = _quoted_value(text, value_start, "'", path) + elif value_start < len(text) and text[value_start] == '"': + value, consumed = _systemd_double_quoted_value(text, value_start, path) + else: + value, consumed = _systemd_unquoted_value(text, value_start, path) + trailing_end = _line_end(text, consumed) + if text[consumed:trailing_end].strip(" \t\r"): + raise SecretToolError("systemd EnvironmentFile has unsupported trailing data") + assignments.append(Assignment(match.group(1), value)) + position = trailing_end + (trailing_end < len(text)) + return assignments + + def unique_names(assignments: Iterable[Assignment]) -> list[str]: seen: set[str] = set() names: list[str] = [] @@ -284,7 +368,7 @@ def service_names(unit: str) -> set[str]: files = environment_file_specs(systemctl_property(unit, "EnvironmentFiles")) for path, ignore_errors in files: try: - assignments.extend(parse_env_file(path)) + assignments.extend(parse_systemd_environment_file(path)) except SecretToolError as exc: if ignore_errors: continue diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index d74160ce4fb..903495a32cc 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -170,7 +170,8 @@ test_service_has_falls_back_to_unit_settings() { 'UNSET_FILE_SETTING=value' \ 'EXACT_FILE_MATCH=actual' \ 'EXACT_FILE_KEEP=actual' \ - 'OVERRIDDEN_SETTING=remove' > "$unit_env" + 'OVERRIDDEN_SETTING=remove' \ + 'ESCAPED_UNSET=foo\ bar' > "$unit_env" # shellcheck disable=SC2016 # The generated stub expands these at execution time. printf '%s\n' \ '#!/usr/bin/env bash' \ @@ -178,15 +179,15 @@ test_service_has_falls_back_to_unit_settings() { ' *--property=MainPID*) printf "0\n" ;;' \ ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=actual OVERRIDDEN_SETTING=keep\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ - ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other OVERRIDDEN_SETTING=remove\n" ;;' \ + ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other OVERRIDDEN_SETTING=remove \"ESCAPED_UNSET=foo bar\"\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ - $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP OVERRIDDEN_SETTING ABSENT_SETTING 2>&1) \ + $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP OVERRIDDEN_SETTING ESCAPED_UNSET ABSENT_SETTING 2>&1) \ || fail "service has failed for unit declarations" - expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'OVERRIDDEN_SETTING=no' 'ABSENT_SETTING=no') + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'OVERRIDDEN_SETTING=no' 'ESCAPED_UNSET=no' 'ABSENT_SETTING=no') [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" assert_no_fake_secret "$output" "service declaration has" pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" From 7bd2e9a4709e4ba98e71afd267d5b88ab1141651 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:35:44 -0600 Subject: [PATCH 09/17] no-mistakes(review): Reject malformed settings files safely --- bin/fm-secrets.py | 13 ++++++++++++ tests/fm-secrets.test.sh | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 10830768b8f..908fb0c21ec 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -147,6 +147,11 @@ def parse_env_file(path: str) -> list[Assignment]: text, value_start, text[value_start], path ) next_end = _line_end(text, consumed) + trailing = text[consumed:next_end].strip(" \t\r") + if trailing and not trailing.startswith("#"): + raise SecretToolError( + f"{path}: trailing data after quoted value at line {_line_number(text, consumed)}" + ) else: value, next_end = _unquoted_value(text, value_start) assignments.append(Assignment(match.group(1), value)) @@ -208,6 +213,14 @@ def parse_systemd_environment_file(path: str) -> list[Assignment]: raise SecretToolError("systemd EnvironmentFile is not valid UTF-8") from exc except OSError as exc: raise SecretToolError(f"cannot read settings file: {path}") from exc + if any( + char == "\0" + or char == "\ufeff" + or 0xFDD0 <= ord(char) <= 0xFDEF + or ord(char) & 0xFFFF in {0xFFFE, 0xFFFF} + for char in text + ): + raise SecretToolError("systemd EnvironmentFile contains disallowed characters") assignments: list[Assignment] = [] position = 0 diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 903495a32cc..187de8280bf 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -108,6 +108,19 @@ test_run_ignores_empty_secret_values() { pass "fm-secrets: empty secret settings do not corrupt output" } +test_run_rejects_trailing_quoted_data() { + local env_file output rc + env_file="$TMP_ROOT/trailing-quoted.env" + printf '%s\n' 'TOKEN="abc"suffix' > "$env_file" + output=$($TOOL run "$env_file" --only TOKEN -- printf x 2>&1) + rc=$? + expect_code 2 "$rc" "run must reject trailing quoted data" + assert_not_contains "$output" 'abc' "run exposed a malformed quoted value" + assert_contains "$output" 'trailing data after quoted value' \ + "run did not explain malformed quoted data safely" + pass "fm-secrets: run rejects trailing quoted data" +} + test_run_scrubs_empty_username_url_passwords() { local env_file output env_file="$TMP_ROOT/empty-username.env" @@ -193,6 +206,37 @@ test_service_has_falls_back_to_unit_settings() { pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" } +test_service_has_rejects_invalid_environment_file_characters() { + local env_file invalid output rc + env_file="$TMP_ROOT/invalid-unit.env" + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "0\n" ;;' \ + ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ + ' *--property=Environment*) printf "\n" ;;' \ + ' *--property=UnsetEnvironment*) printf "\n" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + for invalid in nul bom noncharacter; do + case "$invalid" in + nul) printf 'TOKEN=value\0' > "$env_file" ;; + bom) printf '\357\273\277TOKEN=value\n' > "$env_file" ;; + noncharacter) printf 'TOKEN=value\357\267\220\n' > "$env_file" ;; + esac + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$env_file" \ + $TOOL has --service fake-invalid.service TOKEN 2>&1) + rc=$? + expect_code 2 "$rc" "$invalid EnvironmentFile must fail closed" + assert_not_contains "$output" 'value' "$invalid EnvironmentFile exposed its value" + assert_contains "$output" 'cannot inspect a required systemd EnvironmentFile safely' \ + "$invalid EnvironmentFile did not explain the safe refusal" + done + pass "fm-secrets: service has rejects invalid EnvironmentFile characters" +} + test_help_owns_the_scrub_limit() { local help help=$($TOOL --help) || fail "--help failed" @@ -207,8 +251,10 @@ test_names_and_has_never_print_values test_run_scrubs_all_file_values_and_preserves_status test_run_excludes_and_scrubs_ambient_secret_settings test_run_ignores_empty_secret_values +test_run_rejects_trailing_quoted_data test_run_scrubs_empty_username_url_passwords test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings +test_service_has_rejects_invalid_environment_file_characters test_help_owns_the_scrub_limit From 49af36bf11ae1e907eb15fd0a578f1b3a488ef1e Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:42:59 -0600 Subject: [PATCH 10/17] no-mistakes(review): Report unread service variables as unknown --- bin/fm-secrets.py | 36 +++++++++++++++++++++++++++++------- bin/fm-secrets.sh | 5 +++-- tests/fm-secrets.test.sh | 26 ++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 908fb0c21ec..be16e90061d 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -368,12 +368,20 @@ def environment_file_specs(raw: str) -> list[tuple[str, bool]]: return specs -def service_names(unit: str) -> set[str]: +def pass_environment_names(raw: str) -> set[str]: + try: + words = shlex.split(raw, posix=True) + except ValueError as exc: + raise SecretToolError("systemd PassEnvironment= data could not be parsed safely") from exc + return {word for word in words if NAME_RE.fullmatch(word)} + + +def service_presence(unit: str, requested: Sequence[str]) -> dict[str, str]: if not unit or unit.startswith("-"): raise SecretToolError("a valid systemd unit name is required") running = process_environment_names(systemctl_property(unit, "MainPID")) if running is not None: - return running + return {name: "yes" if name in running else "no" for name in requested} assignments = environment_declaration_assignments( systemctl_property(unit, "Environment") @@ -392,7 +400,7 @@ def service_names(unit: str) -> set[str]: systemctl_property(unit, "UnsetEnvironment") ) values = {assignment.name: assignment.value for assignment in assignments} - return { + present = { name for name, value in values.items() if not any( @@ -400,6 +408,18 @@ def service_names(unit: str) -> set[str]: for unset_name, unset_value in unsets ) } + unconditionally_unset = {name for name, value in unsets if value is None} + unknown = pass_environment_names(systemctl_property(unit, "PassEnvironment")) + return { + name: ( + "yes" + if name in present + else "unknown" + if name in unknown and name not in values and name not in unconditionally_unset + else "no" + ) + for name in requested + } def command_names(args: Sequence[str]) -> int: @@ -416,18 +436,20 @@ def command_has(args: Sequence[str]) -> int: raise SecretToolError( "usage: fm-secrets.sh has --service ..." ) - present = service_names(args[1]) requested = list(args[2:]) + validate_names(requested) + presence = service_presence(args[1], requested) else: if len(args) < 2: raise SecretToolError( "usage: fm-secrets.sh has ..." ) - present = set(unique_names(parse_env_file(args[0]))) requested = list(args[1:]) - validate_names(requested) + validate_names(requested) + present = set(unique_names(parse_env_file(args[0]))) + presence = {name: "yes" if name in present else "no" for name in requested} for name in requested: - print(f"{name}={'yes' if name in present else 'no'}") + print(f"{name}={presence[name]}") return 0 diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh index 1c81c38b4ed..1f9c71f2258 100755 --- a/bin/fm-secrets.sh +++ b/bin/fm-secrets.sh @@ -6,10 +6,11 @@ # Print only assignment names, one per line. # has ... # has --service ... -# Print NAME=yes or NAME=no without printing any setting value. +# Print NAME=yes, NAME=no, or NAME=unknown without any setting value. # A running service's main-process environment is authoritative when it # is readable; otherwise the unit's EnvironmentFile= and Environment= -# declarations are inspected. +# declarations are inspected. Manager values named by PassEnvironment= +# are reported as unknown rather than read. # run --only NAME[,NAME...] -- # Inherit only PATH, HOME, USER, LOGNAME, LANG, LC_*, TERM, TMPDIR, SHELL, # and PWD, add only the selected names, run the command, and scrub its diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 187de8280bf..c83da014fa8 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -193,6 +193,7 @@ test_service_has_falls_back_to_unit_settings() { ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ ' *--property=Environment*) printf "INLINE_SETTING=%s UNSET_INLINE_SETTING=value EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=actual OVERRIDDEN_SETTING=keep\n" "${FM_TEST_INLINE_VALUE:?}" ;;' \ ' *--property=UnsetEnvironment*) printf "UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH=actual EXACT_FILE_KEEP=other EXACT_INLINE_MATCH=actual EXACT_INLINE_KEEP=other OVERRIDDEN_SETTING=remove \"ESCAPED_UNSET=foo bar\"\n" ;;' \ + ' *--property=PassEnvironment*) printf "\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" @@ -216,6 +217,7 @@ test_service_has_rejects_invalid_environment_file_characters() { ' *--property=EnvironmentFiles*) printf "%s (ignore_errors=no)\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ ' *--property=Environment*) printf "\n" ;;' \ ' *--property=UnsetEnvironment*) printf "\n" ;;' \ + ' *--property=PassEnvironment*) printf "\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" @@ -237,6 +239,28 @@ test_service_has_rejects_invalid_environment_file_characters() { pass "fm-secrets: service has rejects invalid EnvironmentFile characters" } +test_service_has_marks_unread_passed_environment_unknown() { + local output expected + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "0\n" ;;' \ + ' *--property=EnvironmentFiles*) printf "\n" ;;' \ + ' *--property=Environment*) printf "\n" ;;' \ + ' *--property=UnsetEnvironment*) printf "\n" ;;' \ + ' *--property=PassEnvironment*) printf "TYPESAFE_API_KEY\n" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + output=$(PATH="$FAKEBIN:$PATH" $TOOL has --service fake-stopped.service \ + TYPESAFE_API_KEY ABSENT_SETTING 2>&1) \ + || fail "service has failed with PassEnvironment" + expected=$(printf '%s\n' 'TYPESAFE_API_KEY=unknown' 'ABSENT_SETTING=no') + [ "$output" = "$expected" ] || fail "service has misreported passed environment: $output" + pass "fm-secrets: service has marks unread passed environment unknown" +} + test_help_owns_the_scrub_limit() { local help help=$($TOOL --help) || fail "--help failed" @@ -244,6 +268,7 @@ test_help_owns_the_scrub_limit() { assert_contains "$help" 'shorter than 6 bytes' "help omitted the short-value limit" assert_contains "$help" 'URL userinfo passwords' "help omitted the URL password exception" assert_contains "$help" 'exit status is preserved' "help omitted child status behavior" + assert_contains "$help" 'NAME=unknown' "help omitted indeterminate service presence" pass "fm-secrets: help documents the scrub boundary" } @@ -257,4 +282,5 @@ test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_service_has_rejects_invalid_environment_file_characters +test_service_has_marks_unread_passed_environment_unknown test_help_owns_the_scrub_limit From 9d9c1e761b5d93b025b5f50de4d0b0e83fa30c74 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:49:47 -0600 Subject: [PATCH 11/17] no-mistakes(review): Harden selected secret output scrubbing --- bin/fm-secrets.py | 45 +++++++++++++++++++++++++--------------- bin/fm-secrets.sh | 9 ++++---- tests/fm-secrets.test.sh | 45 ++++++++++++++++++++++++++++++++-------- 3 files changed, 69 insertions(+), 30 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index be16e90061d..7f578c63c6e 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -368,14 +368,6 @@ def environment_file_specs(raw: str) -> list[tuple[str, bool]]: return specs -def pass_environment_names(raw: str) -> set[str]: - try: - words = shlex.split(raw, posix=True) - except ValueError as exc: - raise SecretToolError("systemd PassEnvironment= data could not be parsed safely") from exc - return {word for word in words if NAME_RE.fullmatch(word)} - - def service_presence(unit: str, requested: Sequence[str]) -> dict[str, str]: if not unit or unit.startswith("-"): raise SecretToolError("a valid systemd unit name is required") @@ -409,14 +401,11 @@ def service_presence(unit: str, requested: Sequence[str]) -> dict[str, str]: ) } unconditionally_unset = {name for name, value in unsets if value is None} - unknown = pass_environment_names(systemctl_property(unit, "PassEnvironment")) return { name: ( "yes" if name in present - else "unknown" - if name in unknown and name not in values and name not in unconditionally_unset - else "no" + else "no" if name in unconditionally_unset else "unknown" ) for name in requested } @@ -453,12 +442,18 @@ def command_has(args: Sequence[str]) -> int: return 0 -def known_scrubbers(assignments: Iterable[Assignment]) -> list[tuple[bytes, bytes]]: +def known_scrubbers( + assignments: Iterable[Assignment], selected_names: Sequence[str] = () +) -> list[tuple[bytes, bytes]]: scrubbers: dict[bytes, bytes] = {} for assignment in assignments: encoded = assignment.value.encode("utf-8", errors="surrogateescape") replacement = f"".encode("ascii") - if encoded and (len(encoded) >= MIN_SCRUB_BYTES or is_secret_name(assignment.name)): + if encoded and ( + assignment.name in selected_names + or len(encoded) >= MIN_SCRUB_BYTES + or is_secret_name(assignment.name) + ): scrubbers.setdefault(encoded, replacement) for match in URL_PASSWORD_RE.finditer(encoded): password = match.group(2) @@ -477,6 +472,20 @@ def redact_url_password(match: re.Match[bytes]) -> bytes: return URL_PASSWORD_RE.sub(redact_url_password, data) +def scrub_stream_boundary( + stdout: bytes, stderr: bytes, scrubbers: Sequence[tuple[bytes, bytes]] +) -> tuple[bytes, bytes]: + stdout = scrub_output(stdout, scrubbers) + stderr = scrub_output(stderr, scrubbers) + for value, replacement in scrubbers: + for split in range(1, len(value)): + if stdout.endswith(value[:split]) and stderr.startswith(value[split:]): + stdout = stdout[:-split] + replacement + stderr = stderr[len(value) - split :] + break + return stdout, stderr + + def command_run(args: Sequence[str]) -> int: if len(args) < 5 or args[1] != "--only": raise SecretToolError( @@ -520,10 +529,12 @@ def command_run(args: Sequence[str]) -> int: raise SecretToolError("command could not be started") from exc scrubbers = known_scrubbers( - [*assignments, *(Assignment(name, value) for name, value in child_env.items() if is_secret_name(name))] + [*assignments, *(Assignment(name, value) for name, value in child_env.items() if is_secret_name(name))], + requested, ) - sys.stdout.buffer.write(scrub_output(child.stdout, scrubbers)) - sys.stderr.buffer.write(scrub_output(child.stderr, scrubbers)) + stdout, stderr = scrub_stream_boundary(child.stdout, child.stderr, scrubbers) + sys.stdout.buffer.write(stdout) + sys.stderr.buffer.write(stderr) sys.stdout.buffer.flush() sys.stderr.buffer.flush() if child.returncode < 0: diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh index 1f9c71f2258..4cb9b007312 100755 --- a/bin/fm-secrets.sh +++ b/bin/fm-secrets.sh @@ -9,13 +9,14 @@ # Print NAME=yes, NAME=no, or NAME=unknown without any setting value. # A running service's main-process environment is authoritative when it # is readable; otherwise the unit's EnvironmentFile= and Environment= -# declarations are inspected. Manager values named by PassEnvironment= -# are reported as unknown rather than read. +# declarations are inspected. Values that could come from another +# systemd environment source are reported as unknown rather than read. # run --only NAME[,NAME...] -- # Inherit only PATH, HOME, USER, LOGNAME, LANG, LC_*, TERM, TMPDIR, SHELL, # and PWD, add only the selected names, run the command, and scrub its -# stdout and stderr. Every file value at least 6 bytes long is replaced -# with . Values of PASS, PWD, SECRET, TOKEN, KEY, PIN, +# stdout and stderr. Every nonempty selected value and every other file +# value at least 6 bytes long is replaced with . Values of +# PASS, PWD, SECRET, TOKEN, KEY, PIN, # CREDENTIAL, or AUTH settings and URL userinfo passwords are scrubbed at # any length. Other values shorter than 6 bytes are deliberately not # scrubbed because replacing common short strings would corrupt ordinary diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index c83da014fa8..a05fdea7b8c 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -108,6 +108,30 @@ test_run_ignores_empty_secret_values() { pass "fm-secrets: empty secret settings do not corrupt output" } +test_run_scrubs_short_selected_values() { + local env_file output + env_file="$TMP_ROOT/short-selected.env" + printf '%s\n' 'OTP=12345' > "$env_file" + output=$($TOOL run "$env_file" --only OTP -- sh -c 'printf "%s\\n" "$OTP"' 2>&1) \ + || fail "run failed with a short selected value" + assert_not_contains "$output" '12345' "run leaked a short selected value" + assert_contains "$output" '' "run did not scrub a short selected value" + pass "fm-secrets: run scrubs short selected values" +} + +test_run_scrubs_stream_boundary() { + local env_file output + env_file="$TMP_ROOT/stream-boundary.env" + printf '%s\n' 'TOKEN=foobar' > "$env_file" + output=$($TOOL run "$env_file" --only TOKEN -- \ + sh -c 'printf foo; printf bar >&2' 2>&1) \ + || fail "run failed while scrubbing a stream boundary" + assert_not_contains "$output" 'foobar' "run leaked a value split across output streams" + assert_contains "$output" '' \ + "run did not scrub a value split across output streams" + pass "fm-secrets: run scrubs stream-boundary values" +} + test_run_rejects_trailing_quoted_data() { local env_file output rc env_file="$TMP_ROOT/trailing-quoted.env" @@ -201,7 +225,7 @@ test_service_has_falls_back_to_unit_settings() { output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$unit_env" FM_TEST_INLINE_VALUE="$FAKE_INLINE" \ $TOOL has --service fake-stopped.service UNIT_FILE_SETTING INLINE_SETTING UNSET_FILE_SETTING UNSET_INLINE_SETTING EXACT_FILE_MATCH EXACT_FILE_KEEP EXACT_INLINE_MATCH EXACT_INLINE_KEEP OVERRIDDEN_SETTING ESCAPED_UNSET ABSENT_SETTING 2>&1) \ || fail "service has failed for unit declarations" - expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=no' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=no' 'EXACT_INLINE_KEEP=yes' 'OVERRIDDEN_SETTING=no' 'ESCAPED_UNSET=no' 'ABSENT_SETTING=no') + expected=$(printf '%s\n' 'UNIT_FILE_SETTING=yes' 'INLINE_SETTING=yes' 'UNSET_FILE_SETTING=no' 'UNSET_INLINE_SETTING=no' 'EXACT_FILE_MATCH=unknown' 'EXACT_FILE_KEEP=yes' 'EXACT_INLINE_MATCH=unknown' 'EXACT_INLINE_KEEP=yes' 'OVERRIDDEN_SETTING=unknown' 'ESCAPED_UNSET=unknown' 'ABSENT_SETTING=unknown') [ "$output" = "$expected" ] || fail "service has returned unexpected unit booleans: $output" assert_no_fake_secret "$output" "service declaration has" pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" @@ -239,7 +263,7 @@ test_service_has_rejects_invalid_environment_file_characters() { pass "fm-secrets: service has rejects invalid EnvironmentFile characters" } -test_service_has_marks_unread_passed_environment_unknown() { +test_service_has_marks_unmodeled_environment_unknown() { local output expected printf '%s\n' \ '#!/usr/bin/env bash' \ @@ -248,23 +272,24 @@ test_service_has_marks_unread_passed_environment_unknown() { ' *--property=EnvironmentFiles*) printf "\n" ;;' \ ' *--property=Environment*) printf "\n" ;;' \ ' *--property=UnsetEnvironment*) printf "\n" ;;' \ - ' *--property=PassEnvironment*) printf "TYPESAFE_API_KEY\n" ;;' \ ' *) exit 64 ;;' \ 'esac' > "$FAKEBIN/systemctl" chmod +x "$FAKEBIN/systemctl" output=$(PATH="$FAKEBIN:$PATH" $TOOL has --service fake-stopped.service \ - TYPESAFE_API_KEY ABSENT_SETTING 2>&1) \ - || fail "service has failed with PassEnvironment" - expected=$(printf '%s\n' 'TYPESAFE_API_KEY=unknown' 'ABSENT_SETTING=no') - [ "$output" = "$expected" ] || fail "service has misreported passed environment: $output" - pass "fm-secrets: service has marks unread passed environment unknown" + NOTIFY_SOCKET ABSENT_SETTING 2>&1) \ + || fail "service has failed with unmodeled environment" + expected=$(printf '%s\n' 'NOTIFY_SOCKET=unknown' 'ABSENT_SETTING=unknown') + [ "$output" = "$expected" ] || fail "service has misreported unmodeled environment: $output" + pass "fm-secrets: service has marks unmodeled environment unknown" } test_help_owns_the_scrub_limit() { local help help=$($TOOL --help) || fail "--help failed" assert_contains "$help" 'at least 6 bytes long' "help omitted the scrub threshold" + assert_contains "$help" 'Every nonempty selected value' \ + "help omitted selected-value scrubbing" assert_contains "$help" 'shorter than 6 bytes' "help omitted the short-value limit" assert_contains "$help" 'URL userinfo passwords' "help omitted the URL password exception" assert_contains "$help" 'exit status is preserved' "help omitted child status behavior" @@ -276,11 +301,13 @@ test_names_and_has_never_print_values test_run_scrubs_all_file_values_and_preserves_status test_run_excludes_and_scrubs_ambient_secret_settings test_run_ignores_empty_secret_values +test_run_scrubs_short_selected_values +test_run_scrubs_stream_boundary test_run_rejects_trailing_quoted_data test_run_scrubs_empty_username_url_passwords test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_service_has_rejects_invalid_environment_file_characters -test_service_has_marks_unread_passed_environment_unknown +test_service_has_marks_unmodeled_environment_unknown test_help_owns_the_scrub_limit From aca68d83c2e1f5d93f88c6b93abe2af0f7818f80 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:54:11 -0600 Subject: [PATCH 12/17] no-mistakes(review): Isolate selected commands from terminals --- bin/fm-secrets.py | 3 +++ bin/fm-secrets.sh | 1 + tests/fm-secrets.test.sh | 43 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index 7f578c63c6e..f6b0e8b3232 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -521,8 +521,11 @@ def command_run(args: Sequence[str]) -> int: child = subprocess.run( list(args[delimiter + 1 :]), env=child_env, + stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + start_new_session=True, + close_fds=True, check=False, ) except OSError as exc: diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh index 4cb9b007312..e387278f022 100755 --- a/bin/fm-secrets.sh +++ b/bin/fm-secrets.sh @@ -14,6 +14,7 @@ # run --only NAME[,NAME...] -- # Inherit only PATH, HOME, USER, LOGNAME, LANG, LC_*, TERM, TMPDIR, SHELL, # and PWD, add only the selected names, run the command, and scrub its +# The command has no controlling terminal and reads stdin from /dev/null. # stdout and stderr. Every nonempty selected value and every other file # value at least 6 bytes long is replaced with . Values of # PASS, PWD, SECRET, TOKEN, KEY, PIN, diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index a05fdea7b8c..e114d2ffc18 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -132,6 +132,46 @@ test_run_scrubs_stream_boundary() { pass "fm-secrets: run scrubs stream-boundary values" } +test_run_detaches_terminal() { + local env_file output rc + env_file="$TMP_ROOT/terminal.env" + printf '%s\n' 'TOKEN=fake_terminal_secret_20260925' > "$env_file" + output=$(python3 - "$TOOL" "$env_file" <<'PY' +import errno +import os +import sys + +tool, env_file = sys.argv[1:] +pid, terminal = os.forkpty() +if pid == 0: + os.execv(tool, [tool, "run", env_file, "--only", "TOKEN", "--", "sh", "-c", 'printf %s "$TOKEN" > /dev/tty; printf %s "$TOKEN" >&0']) + +chunks = [] +while True: + try: + chunk = os.read(terminal, 4096) + except OSError as error: + if error.errno == errno.EIO: + break + raise + if not chunk: + break + chunks.append(chunk) +os.close(terminal) +_pid, status = os.waitpid(pid, 0) +os.write(1, b"".join(chunks)) +if os.WIFEXITED(status): + sys.exit(os.WEXITSTATUS(status)) +sys.exit(128 + os.WTERMSIG(status)) +PY +) + rc=$? + expect_code 0 "$rc" "run must isolate terminal output" + assert_not_contains "$output" 'fake_terminal_secret_20260925' \ + "run leaked a selected value through a terminal descriptor" + pass "fm-secrets: run detaches terminal descriptors" +} + test_run_rejects_trailing_quoted_data() { local env_file output rc env_file="$TMP_ROOT/trailing-quoted.env" @@ -290,6 +330,8 @@ test_help_owns_the_scrub_limit() { assert_contains "$help" 'at least 6 bytes long' "help omitted the scrub threshold" assert_contains "$help" 'Every nonempty selected value' \ "help omitted selected-value scrubbing" + assert_contains "$help" 'no controlling terminal' \ + "help omitted terminal isolation" assert_contains "$help" 'shorter than 6 bytes' "help omitted the short-value limit" assert_contains "$help" 'URL userinfo passwords' "help omitted the URL password exception" assert_contains "$help" 'exit status is preserved' "help omitted child status behavior" @@ -303,6 +345,7 @@ test_run_excludes_and_scrubs_ambient_secret_settings test_run_ignores_empty_secret_values test_run_scrubs_short_selected_values test_run_scrubs_stream_boundary +test_run_detaches_terminal test_run_rejects_trailing_quoted_data test_run_scrubs_empty_username_url_passwords test_run_scrubs_url_decoded_passwords From 8ffd26ec7a40c72ed152fd8dc501394defab626a Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 09:58:21 -0600 Subject: [PATCH 13/17] no-mistakes(review): Fail closed on malformed optional environment files --- bin/fm-secrets.py | 11 +++++++++-- tests/fm-secrets.test.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index f6b0e8b3232..ad01498ebc4 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -380,11 +380,18 @@ def service_presence(unit: str, requested: Sequence[str]) -> dict[str, str]: ) files = environment_file_specs(systemctl_property(unit, "EnvironmentFiles")) for path, ignore_errors in files: + if ignore_errors: + try: + Path(path).stat() + except FileNotFoundError: + continue + except OSError as exc: + raise SecretToolError( + "cannot inspect a required systemd EnvironmentFile safely" + ) from exc try: assignments.extend(parse_systemd_environment_file(path)) except SecretToolError as exc: - if ignore_errors: - continue raise SecretToolError( "cannot inspect a required systemd EnvironmentFile safely" ) from exc diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index e114d2ffc18..737170b3beb 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -303,6 +303,37 @@ test_service_has_rejects_invalid_environment_file_characters() { pass "fm-secrets: service has rejects invalid EnvironmentFile characters" } +test_service_has_only_ignores_missing_optional_environment_files() { + local env_file missing_file output rc + env_file="$TMP_ROOT/optional-unit.env" + missing_file="$TMP_ROOT/missing-optional-unit.env" + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "0\n" ;;' \ + ' *--property=EnvironmentFiles*) printf -- "-%s\n" "${FM_TEST_UNIT_ENV:?}" ;;' \ + ' *--property=Environment*) printf "TOKEN=value\n" ;;' \ + ' *--property=UnsetEnvironment*) printf "\n" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$missing_file" \ + $TOOL has --service fake-optional.service TOKEN 2>&1) \ + || fail "service has failed with a missing optional EnvironmentFile" + [ "$output" = 'TOKEN=yes' ] || fail "service has did not ignore a missing optional EnvironmentFile: $output" + + printf 'TOKEN=value\377\n' > "$env_file" + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENV="$env_file" \ + $TOOL has --service fake-optional.service TOKEN 2>&1) + rc=$? + expect_code 2 "$rc" "service has must reject a malformed optional EnvironmentFile" + assert_not_contains "$output" 'value' "optional EnvironmentFile failure exposed its value" + assert_contains "$output" 'cannot inspect a required systemd EnvironmentFile safely' \ + "service has did not fail closed for a malformed optional EnvironmentFile" + pass "fm-secrets: service has only ignores missing optional EnvironmentFiles" +} + test_service_has_marks_unmodeled_environment_unknown() { local output expected printf '%s\n' \ @@ -352,5 +383,6 @@ test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings test_service_has_rejects_invalid_environment_file_characters +test_service_has_only_ignores_missing_optional_environment_files test_service_has_marks_unmodeled_environment_unknown test_help_owns_the_scrub_limit From 94aa18fbc037dbd4380cb332732e3800b12ab215 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 10:04:52 -0600 Subject: [PATCH 14/17] no-mistakes(document): Document fm-secrets worker boundary --- bin/fm-secrets.sh | 4 ++-- docs/scripts.md | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/fm-secrets.sh b/bin/fm-secrets.sh index e387278f022..87dcfda3eb4 100755 --- a/bin/fm-secrets.sh +++ b/bin/fm-secrets.sh @@ -14,8 +14,8 @@ # run --only NAME[,NAME...] -- # Inherit only PATH, HOME, USER, LOGNAME, LANG, LC_*, TERM, TMPDIR, SHELL, # and PWD, add only the selected names, run the command, and scrub its -# The command has no controlling terminal and reads stdin from /dev/null. -# stdout and stderr. Every nonempty selected value and every other file +# stdout and stderr. The command has no controlling terminal and reads stdin +# from /dev/null. Every nonempty selected value and every other file # value at least 6 bytes long is replaced with . Values of # PASS, PWD, SECRET, TOKEN, KEY, PIN, # CREDENTIAL, or AUTH settings and URL userinfo passwords are scrubbed at diff --git a/docs/scripts.md b/docs/scripts.md index 68e1072082d..22faf863789 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -34,6 +34,7 @@ The shared no-mistakes gate refusal for fleet lifecycle entrypoints is summarize | `fm-captain-hold.sh` | Hold tasks for the captain, record the captain's answers, gate investigation completion, and report record divergence between the status log and the backlog | | `fm-decision-hold.sh` | One-release compatibility shim mapping the retired decision commands onto fm-captain-hold.sh | | `fm-brief.sh` | Scaffold ship (explicit `--mode`, plus the project's registered `--forge`), scout, secondmate-charter, and Herdr-lab briefs, with Captain's intent and Firstmate spec subsections on ship/scout | +| `fm-secrets.sh` | Worker-facing settings inspection and command-output scrub boundary | | [`fm-dod-lib.sh`](../bin/fm-dod-lib.sh) | Own ship/scout worker role scope, ship definitions of done, the named-head reachability gate on ship `done:` acceptance, and the no-mistakes `--intent` contract | | `fm-brief-heading-lib.sh` | Single owner of reading a brief's sections, shared by the `--intent` contract, spawn and promotion validation, and `fm-dispatch-resolve.sh` | | `fm-herdr-lab.sh` | Provision and guardedly operate an isolated, never-default Herdr lab session | From 18963c7afa84767f630e037cea56fe3629a03e8d Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 10:06:10 -0600 Subject: [PATCH 15/17] no-mistakes(lint): Suppress intentional ShellCheck test literals --- tests/fm-secrets.test.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 737170b3beb..335dec88f6f 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -112,6 +112,7 @@ test_run_scrubs_short_selected_values() { local env_file output env_file="$TMP_ROOT/short-selected.env" printf '%s\n' 'OTP=12345' > "$env_file" + # shellcheck disable=SC2016 # The child expands only its deliberately injected environment. output=$($TOOL run "$env_file" --only OTP -- sh -c 'printf "%s\\n" "$OTP"' 2>&1) \ || fail "run failed with a short selected value" assert_not_contains "$output" '12345' "run leaked a short selected value" @@ -189,6 +190,7 @@ test_run_scrubs_empty_username_url_passwords() { local env_file output env_file="$TMP_ROOT/empty-username.env" printf '%s\n' 'DATABASE_URL=postgres://:12345@db/x' > "$env_file" + # shellcheck disable=SC2016 # The child extracts its deliberately injected URL password. output=$($TOOL run "$env_file" --only DATABASE_URL -- \ sh -c 'password=${DATABASE_URL#*://:}; printf "%s\n" "${password%@*}"' 2>&1) \ || fail "run failed with an empty URL username" @@ -274,6 +276,7 @@ test_service_has_falls_back_to_unit_settings() { test_service_has_rejects_invalid_environment_file_characters() { local env_file invalid output rc env_file="$TMP_ROOT/invalid-unit.env" + # shellcheck disable=SC2016 # This writes literal child-shell source. printf '%s\n' \ '#!/usr/bin/env bash' \ 'case "$*" in' \ @@ -307,6 +310,7 @@ test_service_has_only_ignores_missing_optional_environment_files() { local env_file missing_file output rc env_file="$TMP_ROOT/optional-unit.env" missing_file="$TMP_ROOT/missing-optional-unit.env" + # shellcheck disable=SC2016 # This writes literal child-shell source. printf '%s\n' \ '#!/usr/bin/env bash' \ 'case "$*" in' \ From 993b28af32642c5f90d0c3e93e795281b08e22dc Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 10:46:56 -0600 Subject: [PATCH 16/17] Forward signals to settings child process group --- bin/fm-secrets.py | 31 +++++++++++++++++++++--- tests/fm-secrets.test.sh | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index ad01498ebc4..ac685a66603 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -524,8 +524,26 @@ def command_run(args: Sequence[str]) -> int: for name in requested: child_env[name] = values[name] + child: subprocess.Popen[bytes] | None = None + received_signal: int | None = None + previous_handlers: dict[int, signal.Handlers] = {} + + def forward_signal(signum: int, _frame: object) -> None: + nonlocal received_signal + if received_signal is None: + received_signal = signum + if child is None: + return + try: + os.killpg(child.pid, signum) + except ProcessLookupError: + pass + + forwarded_signals = (signal.SIGHUP, signal.SIGINT, signal.SIGTERM) try: - child = subprocess.run( + for signum in forwarded_signals: + previous_handlers[signum] = signal.signal(signum, forward_signal) + child = subprocess.Popen( list(args[delimiter + 1 :]), env=child_env, stdin=subprocess.DEVNULL, @@ -533,20 +551,27 @@ def command_run(args: Sequence[str]) -> int: stderr=subprocess.PIPE, start_new_session=True, close_fds=True, - check=False, ) + if received_signal is not None: + forward_signal(received_signal, None) + stdout_bytes, stderr_bytes = child.communicate() except OSError as exc: raise SecretToolError("command could not be started") from exc + finally: + for signum, previous_handler in previous_handlers.items(): + signal.signal(signum, previous_handler) scrubbers = known_scrubbers( [*assignments, *(Assignment(name, value) for name, value in child_env.items() if is_secret_name(name))], requested, ) - stdout, stderr = scrub_stream_boundary(child.stdout, child.stderr, scrubbers) + stdout, stderr = scrub_stream_boundary(stdout_bytes, stderr_bytes, scrubbers) sys.stdout.buffer.write(stdout) sys.stderr.buffer.write(stderr) sys.stdout.buffer.flush() sys.stderr.buffer.flush() + if received_signal is not None: + return 128 + received_signal if child.returncode < 0: return 128 + abs(child.returncode) return child.returncode diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index 335dec88f6f..a14b167d972 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -173,6 +173,56 @@ PY pass "fm-secrets: run detaches terminal descriptors" } +test_run_forwards_termination_to_child_group() { + local env_file pid_file output_file wrapper_pid child_pid rc attempt child_alive + env_file="$TMP_ROOT/signal.env" + pid_file="$TMP_ROOT/signal-child.pid" + output_file="$TMP_ROOT/signal-output" + printf '%s\n' 'TOKEN=fake_signal_secret_20260925' > "$env_file" + + # shellcheck disable=SC2016 # The child expands its PID and positional argument. + $TOOL run "$env_file" --only TOKEN -- \ + sh -c 'printf "%s\n" "$$" > "$1"; sleep 30' _ "$pid_file" \ + > "$output_file" 2>&1 & + wrapper_pid=$! + + attempt=0 + while [ ! -s "$pid_file" ] && [ "$attempt" -lt 100 ]; do + sleep 0.05 + attempt=$((attempt + 1)) + done + if [ ! -s "$pid_file" ]; then + kill -TERM "$wrapper_pid" 2>/dev/null || true + wait "$wrapper_pid" 2>/dev/null || true + fail "run child did not publish its PID" + fi + child_pid=$(sed -n '1p' "$pid_file") + sleep 0.1 + + kill -TERM "$wrapper_pid" || fail "could not terminate run wrapper" + wait "$wrapper_pid" + rc=$? + + child_alive=yes + attempt=0 + while [ "$attempt" -lt 40 ]; do + if ! kill -0 "$child_pid" 2>/dev/null; then + child_alive=no + break + fi + sleep 0.05 + attempt=$((attempt + 1)) + done + if [ "$child_alive" = yes ]; then + kill -TERM -- "-$child_pid" 2>/dev/null || true + fail "run left its detached child process group alive after termination" + fi + + expect_code 143 "$rc" "run must report conventional SIGTERM status" + assert_no_fake_secret "$(< "$output_file")" "run signal forwarding" + pass "fm-secrets: run forwards termination to its child process group" +} + test_run_rejects_trailing_quoted_data() { local env_file output rc env_file="$TMP_ROOT/trailing-quoted.env" @@ -381,6 +431,7 @@ test_run_ignores_empty_secret_values test_run_scrubs_short_selected_values test_run_scrubs_stream_boundary test_run_detaches_terminal +test_run_forwards_termination_to_child_group test_run_rejects_trailing_quoted_data test_run_scrubs_empty_username_url_passwords test_run_scrubs_url_decoded_passwords From b66a85caa693b47cca0ad3e88ee7d406c82c2706 Mon Sep 17 00:00:00 2001 From: Curtis Kormos Date: Fri, 25 Sep 2026 10:52:12 -0600 Subject: [PATCH 17/17] no-mistakes(review): Order wildcard EnvironmentFile expansions deterministically --- bin/fm-secrets.py | 2 +- tests/fm-secrets.test.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/bin/fm-secrets.py b/bin/fm-secrets.py index ac685a66603..94824fa4587 100755 --- a/bin/fm-secrets.py +++ b/bin/fm-secrets.py @@ -364,7 +364,7 @@ def environment_file_specs(raw: str) -> list[tuple[str, bool]]: word = word[1:] ignore_errors = True if word: - specs.extend((path, ignore_errors) for path in (glob.glob(word) or [word])) + specs.extend((path, ignore_errors) for path in (sorted(glob.glob(word)) or [word])) return specs diff --git a/tests/fm-secrets.test.sh b/tests/fm-secrets.test.sh index a14b167d972..f5d2100aabe 100755 --- a/tests/fm-secrets.test.sh +++ b/tests/fm-secrets.test.sh @@ -323,6 +323,32 @@ test_service_has_falls_back_to_unit_settings() { pass "fm-secrets: service has safely reads EnvironmentFile and Environment declarations" } +test_service_has_orders_wildcard_environment_files() { + local unit_envs output + unit_envs="$TMP_ROOT/wildcard-unit-envs" + mkdir -p "$unit_envs" + printf '%s\n' 'TOKEN=remove' > "$unit_envs/20.env" + printf '%s\n' 'TOKEN=keep' > "$unit_envs/10.env" + # shellcheck disable=SC2016 # The generated stub expands this at execution time. + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'case "$*" in' \ + ' *--property=MainPID*) printf "0\n" ;;' \ + ' *--property=EnvironmentFiles*) printf "%s/*\n" "${FM_TEST_UNIT_ENVS:?}" ;;' \ + ' *--property=Environment*) printf "\n" ;;' \ + ' *--property=UnsetEnvironment*) printf "TOKEN=remove\n" ;;' \ + ' *) exit 64 ;;' \ + 'esac' > "$FAKEBIN/systemctl" + chmod +x "$FAKEBIN/systemctl" + + output=$(PATH="$FAKEBIN:$PATH" FM_TEST_UNIT_ENVS="$unit_envs" \ + $TOOL has --service fake-wildcard.service TOKEN 2>&1) \ + || fail "service has failed for wildcard EnvironmentFiles" + [ "$output" = 'TOKEN=unknown' ] \ + || fail "service has did not apply wildcard EnvironmentFiles in order: $output" + pass "fm-secrets: service has orders wildcard EnvironmentFiles" +} + test_service_has_rejects_invalid_environment_file_characters() { local env_file invalid output rc env_file="$TMP_ROOT/invalid-unit.env" @@ -437,6 +463,7 @@ test_run_scrubs_empty_username_url_passwords test_run_scrubs_url_decoded_passwords test_service_has_reads_process_environment_without_values test_service_has_falls_back_to_unit_settings +test_service_has_orders_wildcard_environment_files test_service_has_rejects_invalid_environment_file_characters test_service_has_only_ignores_missing_optional_environment_files test_service_has_marks_unmodeled_environment_unknown