fix(run)!: a variable value was supplying shell syntax, not just a value - #11
Conversation
… commands A variable supplies a value. It was supplying syntax. substitute_variables spliced the resolved value into the command text, which cli.run then executed with shell=True. Verified end to end: TARGET='safe; touch /tmp/PWNED' ran touch as a second command TARGET='$(touch /tmp/PWNED)' was evaluated by the shell The dangerous variant is an imported runbook: the payload sits in a variable's default, which mem run never displays. The listing shows `echo $TARGET` while something else executes, so reviewing a shared runbook tells the user nothing. Values now reach the child through its environment and the command runs verbatim, so the shell expands $NAME itself. Parameter expansion does not re-scan its result for operators, which makes the same payloads inert — and $API_KEY_ID stops expanding as $API_KEY followed by '_ID', because the shell tokenizes identifiers properly and textual replacement did not. Two other leaks close with it. mem printed the fully substituted command, so a secret pulled from the store — entered at a hidden prompt precisely so it would stay unseen — was echoed in cleartext and left in the scrollback. And the expanded value was a process argument, visible in ps. The preview now shows $API_TOKEN, and the value never enters argv. substitute_variables is deleted rather than fixed: nothing calls it any more, so the $$ escape bug it carried was a bug in dead code. Same commit, because they are one change: $$NAME is now stored verbatim and resolved on the way to the shell. Collapsing it at save time was lossy — the stored text became indistinguishable from a real mem variable, so exporting a runbook and importing it elsewhere made mem claim ownership of a variable the author had marked as the shell's. Also in variables.py: allow_prompt=False only short-circuited the default branch, so a variable with no source and no default still reached click.prompt and `mem run --yes` could block on a stdin nobody is watching. And credential detection offered `StrictHostKeyChecking=no` as DB_PASSWORD: the value extractor understood only --flag=value and UPPER_CASE=value, so a mixed-case option assignment was kept whole and cleared the length filter at 24 characters. Reducing any identifier=value to its right-hand side lets the existing filter reject 'no' on its own merits.
Three were stale rather than failing, and each said so through its own guard instead of passing quietly: - The vars.json permission test spied on Path.write_text, which atomic_write no longer uses. It now inspects the temporary file at the rename — the last instant it exists, and independent of which call created it. - The escape round-trip test built its input by calling process_escapes, mimicking the old save behaviour. It now uses what save actually stores. - TestSubstituteVariables is gone along with the function it covered. Its prefix-collision cases stop being mem's problem: the shell tokenizes $API_KEY_ID correctly on its own. Drops the 10 xfail markers this branch turns green.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 955c45ca9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| console.print(f" [dim]$ {safe(run_cmd)}[/]") | ||
| try: | ||
| result = sp.run(run_cmd, shell=True) | ||
| result = sp.run(run_cmd, shell=True, env=child_env) |
There was a problem hiding this comment.
When a saved command quotes a placeholder with single quotes—such as the documented/generated curl -H 'Authorization: Bearer $API_TOKEN' form—passing the value only through the environment leaves $API_TOKEN literal because /bin/sh does not perform parameter expansion inside single quotes. The previous textual substitution resolved this form, so existing runbooks now send literal placeholders instead of credentials; preserve safe placeholder resolution regardless of shell quoting or migrate the generated syntax.
Useful? React with 👍 / 👎.
| # `$API_TOKEN`, not the token. A value fetched from the store — entered with | ||
| # hidden input precisely so it would stay unseen — used to be echoed in | ||
| # plaintext to the terminal and left in the scrollback. | ||
| child_env = {**os.environ, **{name: value for name, (value, _) in resolved.items()}} |
There was a problem hiding this comment.
On Linux, an imported or explicitly declared variable such as LD_PRELOAD can now alter execution before the reviewed command starts: VarDeclaration accepts arbitrary uppercase names, declarations need not appear in the command, and a hidden default is copied into the environment used to launch /bin/sh, causing the dynamic loader to load the specified object. This restores a command-execution path through variable values even for an otherwise harmless command; prevent loader and shell-startup control variables from being forwarded directly, or use a transport namespace the shell itself does not consume.
Useful? React with 👍 / 👎.
1 participant
Fifth branch. Turns 10 more contracts green (29 xfail → 19). This is the branch
that closes the remaining command-injection and secret-leak paths.
The injection
substitute_variablesspliced the resolved value into the command text, whichcli.runthen executed withshell=True. Verified end to end against a realsentinel file:
The dangerous variant is an imported runbook: the payload lives in a
variable's
default, whichmem runnever displays. The listing showsecho $TARGETwhile something else executes, so reviewing a runbook a colleaguesent you tells you nothing.
After:
The fix is to stop substituting, not to escape harder
Values now reach the child through its environment and the command runs
verbatim, so the shell expands
$NAMEitself. Shell parameter expansion doesnot re-scan its result for operators, which makes the same payloads inert.
shlex.quotewould also have been safe, but it changes what the user wrote: avalue with spaces becomes one quoted argument whether or not that was intended.
Handing over the environment keeps the semantics the user already expects from
their shell.
A bonus that falls out of it:
$API_KEY_IDno longer expands as$API_KEYfollowed by
_ID. The shell tokenizes identifiers properly; textual replacementdid not. The whole prefix-collision class of bug disappears rather than being
worked around.
Two leaks close with it
The secret was printed. mem echoed the fully substituted command, so a value
pulled from the store — entered at a hidden prompt precisely so it would never
be seen — was written in cleartext to the terminal and left in the scrollback.
The preview now shows
$API_TOKEN.The secret was in argv. The expanded command was a process argument, visible
to
ps. It now travels in the environment instead.Also fixed
--yescould block on a prompt.allow_prompt=Falseonly short-circuited thedefault branch, so a variable with no source and no default still reached
click.promptagainst a stdin nobody is watching. In CI that is a hang, notan error.
StrictHostKeyChecking=nowas offered asDB_PASSWORD. The value extractorunderstood only
--flag=valueandUPPER_CASE=value, so a mixed-case optionassignment was kept whole and cleared the 8-character filter at 24. Reducing
any
identifier=valueto its right-hand side lets the existing filter rejectnoon its own merits — fixing the class rather than blacklisting a name.Deletions
substitute_variablesis deleted rather than fixed. Nothing calls it any more,so the
$$-escape bug it carried was a bug in dead code, and itsprefix-collision tests were pinning behaviour the shell now handles correctly.
BREAKING
$$NAMEis stored verbatim and resolved on the way to the shell, instead ofbeing collapsed to
$NAMEat save time. Collapsing early was lossy: the storedtext became indistinguishable from a real mem variable, so exporting a runbook
and importing it elsewhere made mem claim ownership of a variable the author had
explicitly marked as the shell's.
Still open
~/.mem/vars.jsonholds secrets in plaintext at 0600. Given the decision todouble down on Apple integration, the Keychain is the right home for them, and
that is its own branch — it needs a migration and a fallback for machines where
the Keychain is locked.