env understands ${VAR}, because headers two methods away already did - #127
Conversation
…y did
`{"env": {"GITHUB_TOKEN": "${GITHUB_TOKEN}"}}` handed the child the eleven
literal characters, which a server takes for a token and fails on somewhere
far from the cause. `headers` got the expansion when HTTP servers arrived;
`env` sat two methods away and kept taking values verbatim.
That left two ways to give a subprocess a secret: write it into a file that
gets committed, or let the child inherit smith's whole environment. The
second is what #109 exists to stop, and it cannot be stopped while there is
no deliberate way to pass one — which is why this is its own change and goes
first.
One implementation serves both, rather than the same regex written twice, so
a `${VAR}` cannot come to mean two things depending on which half of an
entry it was written in. The warning names the entry now — `env
'GITHUB_TOKEN'`, `header 'Authorization'` — since both are `key: value` and
which one it was is the first thing a reader needs. Its closing words
changed from "sending it empty" to "using an empty value", which reads right
for both.
Only the exact `${NAME}` shape is a reference. A bare `$`, a `$5` and a
`${not-a-name}` are left as written, and only strings are expanded at all,
because only a string can hold one: `8080` is a port. Numbers and booleans
still arrive as the strings they obviously mean.
Three of the four specs fail without the change. The fourth is the guard
against over-eager expansion, so it passes either way by design.
Closes #122
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p a claim that never happened
Review, four corrections. Three are prose, one is a hazard worth writing
down rather than fixing here.
`${GITHUB_TOKEN}` is fifteen characters, not eleven.
"The warning now names the entry rather than only the variable" never
happened: main's header warning already read `header 'Authorization'
references ${TOKEN}`. The only thing that changed for headers is the closing
clause. `env` simply had no warning before. Said that way now.
Two hazards named rather than papered over, both inherited from headers and
both sharper in `env`:
There is no escape for a literal `${NAME}` — not `$$`, not a backslash. The
one input where old and new behaviour differ is `"a\${B}"`, which used to
arrive verbatim and now loses its reference. An `env` value is far likelier
than a header to be a template that some other program means to expand
itself, so the README says there is no opt-out instead of only listing what
is not a reference.
An unset variable becomes empty rather than absent, which the issue asked
for and which is not equally harmless on both sides. An empty header is
inert; an empty environment variable is a different thing from a missing one
to the program reading it — an empty PYTHONPATH puts the working directory
on the import path. Today the inherited value is still there behind the
warning. Under #109's clear_env it will not be, and the comment says the
choice deserves revisiting there.
The header spec now pins `header 'Authorization'` too. Both sides share one
implementation, and nothing else stopped a later change from dropping the
`what` argument at that call site with the suite still green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ited Review caught the handoff note to #109 stating the opposite of the truth, in the direction that makes today's build look safer than it is. Verified before fixing, parent `PYTHONPATH=/opt/inherited-libs`: explicit {"PYTHONPATH" => ""} -> child sees [] set-but-empty: 1 same, with clear_env: true -> child sees [] {"PYTHONPATH" => nil} -> absent: <ABSENT> So an explicit entry *overrides* the inherited value rather than sitting in front of it: an empty expansion does not fall back, it replaces. And `clear_env` gives the identical result, so it takes away nothing that was softening this. What `clear_env` removes is the fallback for a variable no entry names at all — a different hazard, conflated with this one. The consequence is the opposite of what the comment implied: this is not a risk that arrives with #109, it is live now, on the current release, for anyone who writes an entry whose variable is unset. The third line is why the alternative is named too: `Process` reads a nil value as "leave it unset", so dropping the key costs widening `ServerSpec#env` to `Hash(String, String?)` and nothing else. The changelog also says what actually changed for an existing HTTP user — the closing words of the warning — since that is the only behaviour change in this PR for anyone not using `env`, and a runbook grepping for the old string deserves the pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two from the final review round.
"Costs widening `ServerSpec#env` and nothing else" was wrong twice over.
`spawn_server`'s signature is annotated `Hash(String, String)` one file away
and would have to widen with it — a reader taking "nothing else" literally
meets a compile error in exactly the place this change's own premise warns
about, a rule that drifted because its two halves live two methods apart.
And the typing is the easy half. `expand_vars` returns a `String`, so it
cannot report "this whole value was one unset reference" — only a value that
is nothing but that could become nil, while `"a${UNSET}b"` has to stay a
string. Deciding what a partial reference means is the work. A paragraph
that exists so #109 need not rediscover the option should be right about
what it would cost.
The separator I reported as added last round went in one paragraph too late,
so the hazard block still opened as a continuation of a sentence about
argument naming.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review durch einen Reviewer-Agenten, drei Runden: APPROVE WITH NITS, dann REQUEST CHANGES, dann APPROVE. Alle Befunde betrafen Prosa und Dokumentation — der ausführbare Code ist seit dem ersten Commit byteidentisch. Der Befund, der die zweite Runde gekostet hatMein Kommentar zur Gefahr „leer statt abwesend" behauptete, hinter der leer expandierten Variablen stehe heute noch der geerbte Wert, und Ein expliziter Die Folge ist das Gegenteil dessen, was dort stand: das Risiko entsteht nicht erst mit #109, es ist mit diesem PR live. Ein Kommentar, der als Übergabe an #109 gedacht ist, hätte dort mit falscher Prämisse gelegen. Korrigiert im Code und auf #109, wo ich denselben Fehler gepostet hatte. Zwei Gefahren benannt statt behobenKein Escape für ein literales „Nicht gesetzt" wird „leer" — vom Issue so verlangt, aber bei Nach #109 weitergereichtZwei Befunde, die nicht diesen PR betreffen, aber dort scharf werden — beide auf #109 notiert:
Was gegengeprüft und sauber warKein Leck-Pfad. Keine Injektion. Ein Newline im expandierten Wert spaltet keine zweite Variable ab; Verhaltenskompatibilität bewiesen, nicht angenommen. 28 Eingabeformen gegen Der Der Header-Spec ist mutationsgetestet. Schwächt man die Aufrufstelle auf
|
1 participant
Closes #122
{"env": {"GITHUB_TOKEN": "${GITHUB_TOKEN}"}}handed the child eleven literal characters. A server takes that for a token and fails somewhere far from the cause.The drift
headersgot${VAR}expansion when HTTP servers arrived (server_config.cr:263).env, two methods away, kept callingstring_map, which takes values verbatim (:286). Same file, same shape of config, different rules — and nothing said so.That left exactly two ways to give a stdio server a secret: write it into a file that gets committed, or rely on the child inheriting smith's entire environment (
protocol.cr:206spawns withoutclear_env). The second is what #109 exists to stop, and it cannot be stopped while there is no deliberate way to pass one — which is why this is its own change and goes first. Doing #109's step 2 before this would break every server that relies on an inherited variable, with no replacement to offer.One implementation, not the same regex twice
expand_varsis now shared byexpand_headersand the newexpand_env, so a${VAR}cannot come to mean two things depending on which half of an entry it was written in.The warning names the entry rather than the kind —
env 'GITHUB_TOKEN',header 'Authorization'— because both arekey: valueand which one it was is the first thing somebody reading it needs. Its closing words changed from "sending it empty" to "using an empty value", which reads right for both; the existing header spec asserts the variable name andnot set, both untouched.What is deliberately not expanded
${VAR};8080is a port. Numbers and booleans still arrive as the strings they obviously mean, so"PORT": 8080needs no quoting — that behaviour came fromstring_mapand is kept.${NAME}shape. A bare$, a$5, a${not-a-name}are left as written.$is legal and common in a value.Not in this PR
clear_env: trueand the base allowlist — #109's step 2, with its open question about a transition period. This is the prerequisite, nothing more.Specs
Four. Three fail without the change (expansion, expansion mid-value, the unset-variable warning); the fourth is the guard against over-eager expansion and passes either way by design — which is what a guard is for.
crystal spec: 1432 examples, 0 failures.crystal tool format --check: clean.🤖 Generated with Claude Code