Skip to content

fix(evals): screen every segment of a name, not only the last - #2949

Open
ntdatt812 wants to merge 1 commit into
garrytan:mainfrom
ntdatt812:fix/hermetic-env-credential-segment-screen
Open

ntdatt812 wants to merge 1 commit into
garrytan:mainfrom
ntdatt812:fix/hermetic-env-credential-segment-screen

Conversation

@ntdatt812

Copy link
Copy Markdown

Follow-up to the credential screen adapted from #2636 and merged as b9706f3 (#2942, v1.88.1.0) — thank you for the credit there. One residual hole in it.

The hole

The screen reads a single segment:

// test/helpers/hermetic-env.ts
!CREDENTIAL_SUFFIXES.has(k.slice(k.lastIndexOf('_') + 1).toUpperCase())

A trailing qualifier moves the credential word off the end and the name is admitted again. That qualifier is the ordinary spelling once there is more than one of something. Running the merged buildHermeticEnv on b9706f3:

ok  credential GITHUB_TOKEN                     dropped
ok  credential GITHUB_APP_PRIVATE_KEY           dropped
XX  credential GITHUB_APP_PRIVATE_KEY_BASE64    ADMITTED
XX  credential GITHUB_PRIVATE_KEY_PEM           ADMITTED
XX  credential GITHUB_TOKEN_1                   ADMITTED
XX  credential GITHUB_TOKEN_GHES                ADMITTED
XX  credential GITHUB_SECRET_VALUE              ADMITTED

credentials leaked: 5   metadata lost: 0

GITHUB_APP_PRIVATE_KEY_BASE64 is the same PEM the screen already rejects under its unqualified name, just base64-encoded — which is how a GitHub App key is normally carried in an environment variable in the first place.

The change

isCredentialShapedName tests every underscore-separated segment.

Segments rather than substrings, deliberately: GITHUB_PATH is documented runner metadata and contains PAT, and GITHUB_TOKENIZER and GITHUB_KEYRING are already pinned in the suite as names that must survive. A substring screen takes all three; the mutation table below pins that.

The screen still governs prefix rules only. An exact ALLOW_EXACT name and a runner's extraAllow re-admit their credentials exactly as before — GEMINI_* carrying GEMINI_API_KEY is unchanged, and there is a new test for the qualified form of that opt-in.

Checked against the full documented GITHUB_* runner set (the 33 names your own test enumerates) plus the EVALS_ knobs: no segment of any of them is a credential word, so nothing that was metadata stops being metadata.

CREDENTIAL_SUFFIXES is renamed CREDENTIAL_SEGMENTS, since "suffix" is no longer what it means.

Coverage

Four tests added to test/helpers/hermetic-env.test.ts, and the launched-child regression in test/helpers/session-runner.test.ts grows two credential names and one near-miss metadata name — a predicate test alone would not have shown this reaching a child.

mutation caught by
back to the tail segment only trailing qualifier · isCredentialShapedName reads segments
substring instead of segment your existing metadata test · near-miss metadata names still pass
no screen at all 4 tests
screen extraAllow too your existing runner-admissions test
first segment only 5 tests

bun test test/helpers/hermetic-env.test.ts: 33 pass. Three failures in that file are Windows-only and identical with the change stashed (split CEO artifact Read scope ×2, Design artifact Read scope — EPERM on symlink).

One thing I could not run here. The launched-child regression cannot execute on Windows on either side of this change: the fixture claude is written with a #!/usr/bin/env node shebang, so Windows cannot launch it and the child's stdout is a shell error rather than JSON. I verified the table I added to it by driving hermeticChildEnv() — the same builder that test's child env comes from — over the same names, and all eleven match the asserted values. CI runs the real thing.

The credential screen on the `GITHUB_`/`EVALS_` prefix rules reads one
segment:

    !CREDENTIAL_SUFFIXES.has(k.slice(k.lastIndexOf('_') + 1).toUpperCase())

A trailing qualifier moves the credential word off the end, and the name is
admitted again. That is the ordinary spelling once there is more than one of
something, so these all reach the hermetic child today:

    GITHUB_APP_PRIVATE_KEY_BASE64   the PEM, base64-encoded
    GITHUB_PRIVATE_KEY_PEM          the PEM
    GITHUB_TOKEN_1                  a numbered token
    GITHUB_TOKEN_GHES               an enterprise-server token
    GITHUB_CLIENT_SECRET_VALUE      an OAuth app secret

`isCredentialShapedName` now tests every underscore-separated segment.
Segments rather than substrings, because `GITHUB_PATH` is documented runner
metadata and contains PAT, and `GITHUB_TOKENIZER` and `GITHUB_KEYRING` are
already pinned as names that must survive.

The screen still governs prefix rules only: an exact `ALLOW_EXACT` name and a
runner's `extraAllow` re-admit their credentials as before.

Checked against the full documented `GITHUB_*` runner set plus the `EVALS_`
knobs: none of them has a credential word in any segment, so nothing that
was metadata stops being metadata.
@trunk-io

trunk-io Bot commented Sep 24, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

garrytan added a commit that referenced this pull request Oct 3, 2026
…g/upgrade/type --selector, BROWSER.md type and profiles, AGENTS.md conventions, CONTRIBUTING #2949, touchfiles for the wave's new lib/bin files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant