Repository navigation
fix(app-restart): mask any Name=value whose name holds a secret word, so --constant X.Password=v no longer prints (TK-003) - #403
Merged
Conversation
…rd=v (TK-003) Field report from a five-app lab project (Mendix 11.15, mxcli v0.26.0). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
MendixMau
added a commit
that referenced
this pull request
Oct 15, 2026
…7 to TK-046) (#400) Reviewed head: 62525b3 Verdict: CHANGES (one false cross-reference, two lines) PR #400 learn(bug-logs): ten ledger entries TK-037 to TK-046 + one on-demand skill line Framework (RELEASE-PROCESS 4a) - Process: confirmed. No gate, step or artifact; ledger text and one skill line. - Dependencies: confirmed. bug-logs/mxcli-bugs.md, skills/learned-constants-and-secrets.md, CHANGELOG.md; no new artifact read. - Context cost: confirmed. learned-constants-and-secrets is ondemand (skill-routing.tsv:308); ledger is read via bug-lookup.sh. - Behaviour: confirmed. NOT RUN, on-demand text only, reason given. - Runtime: confirmed. n/a, no script changed. 5 of 5 rows confirmed. Correctness - bug-logs/mxcli-bugs.md:7142 (TK-044) says "skills/testing-shape.md names the flags". It does not: testing-shape.md has no --app-port/--admin-port on this head, and the PR body says that line was deliberately left out pending an A/B. CHANGELOG.md:24 repeats it ("skill line + ledger"). - Other cross-references hold: testing-shape.md:236/394 has the scheduled-events paragraph (TK-043), learned-sidebar-collapse-icons.md exists (TK-037), decompose-app.md names fix widgets (TK-042). - Wording is generic ("a five-app lab project"); no client or brand names. Owners are right: mxcli for 037-044, Business Events module for 045, Mendix runtime for 046 (NOTE-, not BUG-). - bin/check-pr-discipline.sh origin/master: clean. bug-lookup.sh --index lists all ten entries. Risk / regression - Text only; nothing executes. Low. Readers: agents via bug-lookup.sh and the secrets skill. - Cross-PR: the new secrets line says pass credentials with --constant Module.Name=value. Until #403 lands, bin/app-restart.sh prints that value in clear on its start: line. Merge #403 first or together. Exact change - mxcli-bugs.md:7142: "Toolkit side: skills/testing-shape.md does not name the flags yet (waits on a behaviour A/B). bin/test-scratch.sh still takes its lock on the fixed 8081/8091; ..." - CHANGELOG.md:24: replace "skill line + ledger" with "ledger only; skill line waits on an A/B". --- re-review --- Reviewed head: b5bc764 Verdict: APPROVE PR #400 round 2: re-check of the one CHANGES finding (TK-044 false cross-reference). Delta 62525b3..b5bc764: 2 files, 2 lines (bug-logs/mxcli-bugs.md:7142, CHANGELOG.md:24). - mxcli-bugs.md:7142 now reads "ledger only for now; a skills/testing-shape.md line naming the flags waits on a behaviour A/B (baseline file)". Matches the head: testing-shape.md has no --app-port/--admin-port. RESOLVED. - CHANGELOG.md:24 now "ledger only, the skill line waits on an A/B". RESOLVED. - Nothing else changed; Impact rows from round 1 still hold (5 of 5). - bin/check-pr-discipline.sh origin/master: clean. Carry-over (not blocking): merge #403 first or together, since the secrets line recommends --constant and bin/app-restart.sh still echoes it until #403 lands.
4 tasks done
MendixMau
added a commit
that referenced
this pull request
Oct 16, 2026
…Credential (follow-up to #403) (#406) Independent review: APPROVE (round 2 at b4e74d2). # Review round 2: PR #406 (app-restart mask follow-up) Head b4e74d2 (new commits 7a98507, b4e74d2 on top of 48a15af), reviewed in a detached worktree at /tmp/rv-406b (removed after). Read-only. ## Earlier findings | # | Round-1 finding | Status | |---|---|---| | 1 | Env-prefixed `--cmd` collapsed to `bash -c DB_PASSWORD=****` | **Fixed.** Pass 1 is skipped for the argument after `-c`. `bash -c 'DB_PASSWORD=x ./mxcli run --local --watch -p F.mpr'` now shows `bash -c 'DB_PASSWORD=**** ./mxcli run --local --watch -p F.mpr'`. Tests added on both the dry-run and refuse path. | | 2 | Runtime row wrong (~0.8 s per restart) | **Fixed, and the cost is cut.** Flag check and sed gate are now a bash `case` glob (`mask_has_secret`), no fork unless an argument holds a secret word. My measurement (bash 3.2, 10-call average): plain 12-arg argv master 24 ms / r1 380 ms / now 2 ms; argv with two secrets master 16 ms / r1 285 ms / now 35 ms. The Impact row ("master ~53 ms, r1 ~590 ms, now ~56 ms, includes bash start") is consistent in shape and honest. | | 3 | Argv test was tautological | **Fixed.** A real launch with the fake mxcli writes its received argv to `.fakeargv`; the test asserts it is byte-identical to the raw argv (spaced secret, `--db-password 'zqx pw9'`, Pwd), that stdout hides the secrets, and that `start.cmd` keeps the raw secrets. | | 4 | Master failure count (said 7/8) | **Fixed.** PR body says branch tests vs master's script give PASS=88 FAIL=8; I measured exactly 88/8, all 8 being the new spaced/Pwd/Credential/real-launch/refuse cases. | ## New code: glob-gated sed - **Gate cannot drop a secret the sed would have caught.** `mask_has_secret` uses the same six words as `_w`, both letter-by-letter case-insensitive, both substring. If the glob misses, the sed had nothing to match either. Probed (bash 3.2.57, BSD sed): `PASSWORD=`, `password=`, `Password=`, `--DB-PASSWORD "two words"`, `--db-PassWord=abc`, `Mod.CREDENTIAL=`, `Mod.pwd=`, `MX_TOKEN=`, `DB_PASSWORD="two words"`, `"Mod.PASSWORD=two words"` and `'Mod.password=two words'` inside a quoted `--cmd`: all masked. - **Executed command and start.cmd stay raw.** `mask()` is still only called in `$(...)` (lines 271, 291); exec and `$REC` use raw `ARGV`. Now proven by the real-launch test, not just by reading. - **New, minor: `-c` is treated as a shell flag anywhere in argv, not just after a shell.** For the recorded-argv paths (`/proc/cmdline`, `ps`, `start.cmd`) an argv such as `./mxcli run -c --db-password sekrit` now displays `sekrit` (master and r1 mask it: the argument after `-c` skips the flag check, so `prev_secret` is never set). `mxcli run` has no `-c` shorthand today (only `--configuration`), so this is not reachable with a real mxcli argv. Advisory: set `prev_c` only when the argument before `-c` is a shell (`bash`, `sh`, `*/bash`), or keep the flag check for the `-c` argument and only drop pass 1. - Cosmetic, unchanged from round 1: in a `--cmd` string, pass 4 re-matches `****"` and eats the closing quote (`"Mod.ApiKey=**** -p F.mpr`); also eats a trailing `;` (`export DB_PWD=**** ./mxcli`). No leak; master did not mask `export DB_PWD=abc;` at all. - Known, acknowledged: names without a secret word (`Mod.Salt`), embedded quotes, `--db-password2 v` (digit in flag name, same on master), `--no-password-prompt -p F.mpr` swallowing `-p`. ## Tests and discipline - `PATH=/tmp/shim:$PATH bash tests/wave2/test-app-restart.sh` (no-op setsid shim, 600 s alarm): **PASS=96 FAIL=0**, 94 s, no hang. Matches the PR body. - Branch tests vs master's `app-restart.sh`: PASS=88 FAIL=8 (matches body). The env-prefix test passes on master, correctly: it guards the r1 regression. - `bin/check-pr-discipline.sh origin/master`: clean. - CHANGELOG bullet updated: "22 cases, 8 of them failing on master", real-launch test and `--cmd` env case described. Credited. - Impact block: Process none, Dependencies (3 files, no shared helper), Context cost none, Behaviour n/a, Runtime measured. All accurate. ## Portability Ran under /bin/bash 3.2.57. New code uses only `case` globs with bracket classes (`[!-A-Za-z_]`, leading `-` after `!` is literal), `local`, `${out# }`. No `${,,}`, `mapfile`, `[[ =~ ]]`. Fewer forks also helps Git Bash. ## Verdict APPROVE Advisory (not blocking): 1. Gate the `-c` shell-string handling on the previous argument being a shell, so a non-shell argv with `-c --secret-flag v` still masks `v`. 2. Stop pass 4's `[^ ]+` from eating a closing `"` / `'` or `;` in a `--cmd` string (display copy-paste only).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Direct lane
What changed and why (one paragraph): app-restart.sh masked only values after a password/secret/token/key-named flag, so
--constant Module.ApiPassword=value(flag name--constant) reached the screen and transcripts in clear. A second sed pass now masks the value of anyName=valuewhose Name contains one of those words. Display only; the command that runs is unchanged. Note:test-app-restart.shneeds asetsidshim on macOS (fails 35 cases on master without one); results were taken with a no-op shim.Impact
Field evidence — defect TK-003 of a five-app lab field report (Mendix 11.15, mxcli v0.26.0, toolkit d16f578); reproduced against master 354669e by the scoped test.
CHANGELOG.mdline appended, crediting the sourceFix: a second sed pass masks the value of any Name=value whose Name contains password/secret/token/key, keeping the flag pass (covers
--db-password vand=v). Tests: tests/wave2/test-app-restart.sh adds 5 cases (Password constant, Token constant, --db-password both forms, non-secret constant stays readable); 74 pass, 0 fail with a setsid shim (the masking cases were vacuous before the shim because the stand-in app never started; they pass non-vacuously now).🤖 Generated with Claude Code