fix(codex): pass the OpenAI key through env, not into the run body - #1128
Conversation
tend-agent
left a comment
There was a problem hiding this comment.
The env: change is right and the empty-key branch is unchanged in shape. I confirmed the new test does what it claims: it fails on the pre-fix codex/action.yaml with exactly the assertion quoted in the PR body, and I found no other credential reaching a run: body anywhere in the repo — not in either action, not in .github/workflows/, not in the generator templates (all of those pass credentials through env:/with:). So the scope claim holds.
One inline suggestion on the guard's reach. Separately, the comment above CREDENTIAL_INPUT says a credential added later is covered "without anyone remembering this test exists" — that holds for the suffix convention but not for memory_gist_id in claude/action.yaml, which comes from a repo secret and grants read/write to the memory gist while matching none of _token|_key|secret|password. It isn't inlined today, so nothing is broken; it's just the one input the enumeration in that comment skips.
…t_id
Two gaps in `test_credential_inputs_reach_run_bodies_through_env`, both
from review.
`INPUT_REF` anchored on `${{ … }}`, so it matched only the bare reference:
`${{ inputs.x || '' }}` and `${{ format('{0}', inputs.x) }}` interpolate
into the script text identically and slipped through. Any `inputs.<name>`
in a `run:` body is necessarily a GHA expression — bash has no such
syntax — so dropping the anchors is simpler and strictly broader. Today
it adds one non-credential match repo-wide (`codex_version` in the Codex
install step), so nothing else changes.
`CREDENTIAL_INPUT` matched credentials by name suffix, which the comment
claimed covers an input added later. `memory_gist_id` is the exception:
it comes from a repo secret and grants read/write to the bot's memory
gist while matching none of `_token|_key|secret|password`. It is not
inlined today, so nothing was broken — `gist_id` joins the suffixes so
it stays that way, and the comment now names the case instead of
implying the convention is exhaustive.
Verified the guard still fails on the pre-fix `codex/action.yaml` with
the same assertion message.
|
Both points taken, in
The anchors. Replied inline — dropped as suggested. Verification
Unchanged from the original PR: no live codex session exercises the auth step here, since no |
tend-agent
left a comment
There was a problem hiding this comment.
Verified the fix and the guard independently: the env: form matches what every other input in both actions already uses, and the new test does fail on the pre-fix codex/action.yaml with exactly the assertion quoted in the PR body. One design observation, and one correction if the design stands.
The convention exists to permit a single outlier. codex_version is the only inputs.<name> that reaches a run: body anywhere in either action — github_token, openai_api_key, uv_version, prompt, model, effort, sandbox, and every input in claude/action.yaml already arrive through env:. So CREDENTIAL_INPUT is a naming convention, a twelve-line comment maintaining it, and a widening round already spent on memory_gist_id, all to leave room for one inline reference that the same env: move would remove — in the Install Codex CLI step, whose body is npm install -g "@openai/codex@${{ inputs.codex_version }}":
- name: Install Codex CLI
shell: bash
env:
CODEX_VERSION: ${{ inputs.codex_version }}
run: |
npm install -g "@openai/codex@$CODEX_VERSION"
codex --versionWith that, the guard becomes "no INPUT_REF match in any run: body" — CREDENTIAL_INPUT, its comment, and the did they rename? tripwire all delete, and no future input has to be named a particular way to be covered. CLAUDE.md, under Development: "Prefer deleting a mechanism over refining it." It also closes the substitute-then-parse hole for codex_version itself; that value is adopter-controlled through .config/tend.yaml rather than hostile, but the mechanism is the one the PR body describes. That step is outside the diff, so it isn't an inline suggestion.
If the convention stays, the memory_gist_id justification overstates what the ID grants. docs/security-model.md reads: "A secret Gist is readable to anyone who learns its URL, so the Gist ID stays out of committed public files" — read, not write; writing still needs the PAT's gist scope. Suggestion inline. Inclusion in the suffix list is still warranted on the read exposure alone.
…ed ones codex_version was the last inputs.<name> reaching an inline run: body in either action. Passing it through env: too lets the guard drop the CREDENTIAL_INPUT suffix convention, its comment, and the "did they rename?" tripwire — the rule is now a flat ban that covers any input added later regardless of what it is named.
|
Taken — Better rule than the one it replaces, for the reason you gave and one more: the suffix convention only covered a future input if whoever added it named it for what it holds, which is exactly the assumption a value like
PR body updated to describe the flat rule rather than the suffix list. |
The Codex harness's
Validate auth configuredstep interpolated the OpenAI key straight into the shell script body —if [ -z "${{ inputs.openai_api_key }}" ]. GitHub substitutes an expression into the script text before bash ever parses it, so the value isn't a string being compared; it's script. This passes the key through the step'senv:instead, which is how the Claude harness already handles all three of its credentials, and adds a test that fails on anyinputs.<name>interpolated into an inlinerun:body in either action.Found during the nightly rolling survey of
codex/action.yaml— not from an observed failure.Why it matters, and why nothing caught it
The failure. A key whose value contains a quote or a
$(…)stops being compared and starts executing, as therunneruser — which undercodexholds the real PAT and the model key in its own environment. The realistic path is a mistyped or mis-pasted secret rather than a hostile one, since the value comes from the adopter's own repo secret; the consequence is the same either way, and the step runs before any other codex step.The scope.
openai_api_keywas the only credential inlined across both actions — an outlier, not a pattern. Everything else already goes throughenv::run:body inlineclaude/action.yamlgithub_token,anthropic_api_key,claude_code_oauth_tokencodex/action.yamlgithub_token,openai_api_keyopenai_api_keyWhy no existing check saw it. actionlint doesn't read
action.yamlat all (it parses one as a malformed workflow), and the siblingtest_inline_run_bodies_pass_shellchecksubstitutes every${{ … }}for an opaque${_GHA_EXPR}before handing the body to shellcheck — deliberately, so shellcheck reports on the code rather than the placeholder, but it means the interpolation itself is invisible there.The test.
test_inputs_reach_run_bodies_through_envingenerator/tests/test_repo_pins.py, parametrized over both actions. It bans anyinputs.<name>from an inlinerun:body rather than matching credential-shaped names, so nothing depends on a future input being named for what it holds. That flat rule is only possible because the one input still reaching a body —codex_version, inInstall Codex CLI— moves toenv:here too; with it gone the earlier suffix convention and its comment delete outright.Verification.
uv run pytest— 887 passed.uv tool run pre-commit run --all-files— all 13 hooks pass. The guard fails on either pre-fix body: on the originalopenai_api_keyinline, and (checked by stashing the action) onInstall Codex CLI inlines inputs.codex_version. No live codex session exercised it: noOPENAI_API_KEYreaches this repo's runs, so the auth step's behavior on a set key is unverified here; the empty-key branch is unchanged in shape and theenv:form is whatclaude/action.yamlalready ships. Thenpm install -g "@openai/codex@$CODEX_VERSION"rewrite is likewise unexercised here — CI'stest-codex-surfacejob installs the pin through its own path, not through this step.