fix(judge): stop env expansion from corrupting literal API keys - #156
Open
addyCooks wants to merge 3 commits into
Open
fix(judge): stop env expansion from corrupting literal API keys#156addyCooks wants to merge 3 commits into
addyCooks wants to merge 3 commits into
Conversation
expandEnvVar ran a global regex over every string in judge.json, and one of
its two branches matched a bare `$NAME` anywhere in the value. An API key
pasted verbatim from a provider console — the input path `judge configure`
asks for — was silently rewritten at load time:
sk-ant-api03-AB$CD-EF -> sk-ant-api03-AB-EF
sk-live$SECRET_PART-a -> sk-live-a
There was no escape syntax and no way to opt out. judge.json on disk stayed
correct, so inspecting the file proved nothing; the corruption happened on
every load, and the user-visible result was a provider 401 with a
correct-looking key, which sends people to debug their provider account
rather than nanotune.
Substitute only when the value is *entirely* one `${VAR}` or
`${VAR:-default}` reference — the form the docs actually promise — and drop
the unbraced branch. These values are credentials, so the rule is now exact:
a value either is a reference, or it is a literal that round-trips byte for
byte. A `${VAR}` embedded in a longer string is a literal under that rule and
is no longer expanded.
The second half is the empty-string case. An unset variable resolved to '',
and `apiKey: config.apiKey ?? 'dummy-key'` does not catch '' — `??` only
catches null/undefined — so an empty bearer token went out and came back as
the same anonymous 401. An unset reference now stays intact and
loadJudgeConfig names the variable that is missing, so the error points at
the shell rather than the provider. `??` becomes `||` at the one call site
where '' genuinely means "no key configured", which is what the local-server
`dummy-key` placeholder was always for.
Closes Nano-Collective#128
Every key shape from the issue is asserted to round-trip, table-driven so a
new shape is one line. The variables they look like — SECRET_PART, CD, A,
LEADING — are exported to a sentinel first: with them unset a surviving
expansion produces '' and the key merely looks truncated, so the test could
pass for the wrong reason. Against the pre-fix implementation 11 of these
fail, including all four literal-key cases.
Six existing tests encoded the behaviour being removed — unbraced `$MY_VAR`,
an unset variable collapsing to '', and expansion of references embedded in a
longer string. They are rewritten to assert the new contract rather than
deleted, since that contract is the fix.
loadJudgeConfig is covered end to end for the first time: the literal key
survives a real read of judge.json, the documented `${VAR}` form still
resolves, an unset variable raises an error naming it, and a default keeps
that error from firing. Set-but-empty and an empty `${VAR:-}` default are
pinned separately — both must win over the leave-it-alone fallback, which is
why the resolution order uses `??` rather than `||`.
env-substitution.ts 97.22%; judge.ts 71.93% -> 74.53%.
The page promised `${ENV_VAR}` support without saying where the boundary
was, which is how a literal key containing `$` came to be treated as a
reference. Spell out that only a whole-value reference is substituted, show
the shapes side by side including one that is now deliberately literal, and
show the error a user gets when the variable is unset.
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.
Description
expandEnvVarran a global regex over every string injudge.json, and one ofits two branches matched a bare
$NAMEanywhere in the value. An API key pastedverbatim from a provider console the input path
judge configureasks forwas silently rewritten at load time. The file on disk stayed correct, so
inspecting it proved nothing; the corruption happened on every load.
Substitution now fires only when a value is entirely one
${VAR}or${VAR:-default}reference — the form the docs promise and the unbracedbranch is gone. These values are credentials, so the rule is exact: a value
either is a reference, or it is a literal that round-trips byte for byte.
An unset reference is left intact rather than collapsing to
'', andloadJudgeConfigraises an error naming the variable. Leaving the placeholderalone is not by itself enough to make the failure name the variable without
the check the literal
${VAR}just reaches the provider and returns the sameanonymous 401.
apiKey: config.apiKey ?? 'dummy-key'becomes||at the onecall site where
''genuinely means "no key configured", which is what thelocal-server placeholder was always for.
Closes #128
Type of Change
Testing
Automated Tests
pnpm test:allcompletes successfully)Manual Testing
nanotune initnanotune datacommands (add/import/list/validate)nanotune trainnanotune exportnanotune benchmarkChecklist
pnpm format)