fix: protect Promptfoo auth environment - #972
Conversation
|
@codex review Please review the exact current head: |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
This PR hardens the GitHub Action’s .env loading boundary so repository-controlled env files cannot override Promptfoo authentication settings (API key / remote API base URL) prior to authentication validation, addressing the reported security finding.
Changes:
- Parse each configured env file into an isolated object, validate it doesn’t contain protected Promptfoo auth variables, then apply values to
process.env. - Document the new constraint for
env-filesinREADME.mdandaction.yml. - Regenerate the bundled
dist/index.jsand add tests covering protected-key rejection and ordinary variable forwarding.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main.ts | Loads .env files into an isolated object, rejects protected auth keys, then applies safe values. |
| tests/main.test.ts | Adds test coverage for protected auth key rejection and allowed env propagation. |
| README.md | Clarifies that Promptfoo auth variables must be set via the workflow environment, not .env files. |
| action.yml | Updates env-files input description to match the new security boundary. |
| dist/index.js | Rebuilt bundle reflecting the updated env-file handling logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Consolidate the auth-boundary protection into the shared env-file loader introduced for the process-control fix, instead of a parallel inline check in main.ts. loadEnvironmentFile now also rejects PROMPTFOO_API_KEY and PROMPTFOO_REMOTE_API_BASE_URL (case-insensitively) with an auth-specific message, so a checked-in file cannot pair an inherited credential with an attacker-chosen host that the preflight would send the bearer token to. - add FORBIDDEN_AUTH_KEYS + findForbiddenAuthKey to env.ts; check it in loadEnvironmentFile after the process-control check - unit tests (real dotenv) for findForbiddenAuthKey and auth-key isolation - integration tests: reject both auth vars, forward non-auth PROMPTFOO_ settings, preserve trusted workflow authentication - document the auth-variable rejection in the README env-files note Stacked on the process-control fix (#971); no duplicate env-loading path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
23a7306 to
8da37f4
Compare
💡 Codex Reviewpromptfoo-action/src/utils/env.ts Lines 27 to 28 in 8da37f4  Block LD_AUDIT from env files
When promptfoo-action/src/utils/env.ts Line 30 in 8da37f4  Block NODE_TLS_REJECT_UNAUTHORIZED from env files
When a selected repository promptfoo-action/src/utils/env.ts Lines 135 to 136 in 8da37f4 Keep env files from overriding Promptfoo thresholds
When a workflow sets ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
…njection' into mdangelo/codex/author-pr-972-019f62c3
| const cachedConfig = loadedConfigs.get(filePath); | ||
| if (cachedConfig !== undefined) { | ||
| return cachedConfig; | ||
| } |
| } catch (error) { | ||
| if (String(error).includes('resolved path')) { | ||
| requiresFullEvaluation = true; | ||
| } | ||
| if (!warnedUnsafeDependency) { |
![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: 259c1b4c27
ℹ️ 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".
| const resolvedEntry = path.isAbsolute(entry) | ||
| ? entry | ||
| : path.resolve(path.dirname(lexicalConfigPath), entry); |
There was a problem hiding this comment.
When the selected config lives below working-directory, this resolves a relative commandLineOptions.envPath against the config file directory. The action later runs Promptfoo with cwd: workingDirectory and passes the original config, so Promptfoo loads the raw relative env path from the runtime cwd; for configs/promptfooconfig.yaml with envPath: .env.prod, the preflight validates configs/.env.prod while the child loads ./.env.prod. If the cwd file contains a forbidden endpoint/cache/privacy variable, it bypasses the new preflight and reaches Promptfoo unvetted.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
This adds a separate FORBIDDEN_AUTH_KEYS set + findForbiddenAuthKey() to reject PROMPTFOO_API_KEY and PROMPTFOO_REMOTE_API_BASE_URL from repository env files. The intent is right, but the implementation is almost entirely redundant with the process-control denylist it sits next to: PROMPTFOO_REMOTE_API_BASE_URL is already in FORBIDDEN_ENV_FILE_KEYS, and findForbiddenEnvFileKey runs first in loadEnvironmentFile, so the new auth branch is unreachable for that key. The only net-new behavior is rejecting PROMPTFOO_API_KEY — achievable by adding one entry to the existing set. The new set, function, throw block, and test describe are all dead weight, and the tests that claim to cover the fix actually pass on the base branch. I'd collapse this into a one-line addition (details inline).
Merge/stacking note: Stacked on #971 (base = mdangelo/codex/sec-env-startup-injection), not a conflict — but given the redundancy, consider folding the one-line change into #971 rather than shipping a separate PR.
Additional findings (not on changed lines, noted here):
-
🟠 P2 · simplification — The entire auth apparatus reduces to adding one entry to the existing denylist — in
src/utils/env.ts(nearconst FORBIDDEN_AUTH_KEYS = new Set([)findForbiddenEnvFileKeyruns beforefindForbiddenAuthKeyinloadEnvironmentFile, andPROMPTFOO_REMOTE_API_BASE_URLis already inFORBIDDEN_ENV_FILE_KEYS— so the only keyfindForbiddenAuthKeycan ever return isPROMPTFOO_API_KEY, and it's a line-for-line re-implementation offindForbiddenEnvFileKey's mechanism. The whole behavioral change is: rejectPROMPTFOO_API_KEYfrom env files.
Minimal fix: add 'PROMPTFOO_API_KEY', to FORBIDDEN_ENV_FILE_KEYS (alphabetically, before 'PROMPTFOO_CACHE_PATH'); delete FORBIDDEN_AUTH_KEYS, findForbiddenAuthKey, the second throw block, and the findForbiddenAuthKey describe in env.test.ts. If you want an auth-specific hint, append it to the existing error's resolution text. (This also removes the PROMPTFOO_REMOTE_API_BASE_URL duplication the Copilot thread already flagged.)
-
🟠 P2 · tests — The auth-rejection tests don't exercise the new path — they pass on the base branch — in
__tests__/env.test.ts(neartest('rejects a protected auth variable without leaking any value', () => {)This test (and the
test.eachinmain.test.ts) rejectPROMPTFOO_REMOTE_API_BASE_URL, but that key is caught by the pre-existingfindForbiddenEnvFileKeycheck that runs first, so the message is the oldsets forbidden process-control variable …, not the newsets protected authentication variable …. The assertions only check the key name appears, so they pass with all of this PR'senv.tschanges reverted. The one key that actually reaches the new branch —PROMPTFOO_API_KEY— is exercised only via mocked-dotenv tests. Worse, the PR removedPROMPTFOO_API_KEYfrom thepreserves trusted application credentialsfixture, which was the only real-file test feeding that key throughloadEnvironmentFile.
Minimal fix: write PROMPTFOO_API_KEY into the .env fixture and assert the message contains protected authentication variable + code === ErrorCodes.INVALID_CONFIGURATION, so the test fails without the change.
PROMPTFOO_API_KEY and PROMPTFOO_REMOTE_API_BASE_URL are already both in FORBIDDEN_ENV_FILE_KEYS, so findForbiddenAuthKey (and its separate throw) is fully dead code — findForbiddenEnvFileKey already rejects both. Delete the parallel apparatus and fold the authentication hint into the shared error's help text. Repoint the real-dotenv rejection test at PROMPTFOO_API_KEY. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review + fix applied (
|
Correction on the stacking note aboveMy earlier comment said to "watch for a small conflict when rebasing #972 onto #971 … should merge cleanly." That was wrong, and GitHub now shows this PR as CONFLICTING / DIRTY with its base — so I want to give you an accurate picture. This PR's base is So:
Net: treat my commit as a reviewable improvement to #972's head; the base/divergence is a separate reconciliation task on your side. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac084f29f1
ℹ️ 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".
| const forbiddenAuthKey = findForbiddenAuthKey(fileEnvironment); | ||
| if (forbiddenAuthKey) { |
There was a problem hiding this comment.
For workflows consuming the packaged action, action.yml executes dist/index.js, but this commit does not update any dist/ files and the checked-in bundle still loads env files directly without findForbiddenAuthKey or the new protected-authentication rejection. Consequently, the source tests pass while released/tagged action users remain able to set these authentication variables through env-files; regenerate and commit the bundle.
AGENTS.md reference: AGENTS.md:L96-L98
Useful? React with 👍 / 👎.
| const forbiddenAuthKey = findForbiddenAuthKey(fileEnvironment); | ||
| if (forbiddenAuthKey) { |
There was a problem hiding this comment.
When the checkout contains the implicit workingDirectory/.env or a config selects commandLineOptions.envPath, Promptfoo loads that repository-controlled file itself, bypassing this guard because the only loadEnvironmentFile call in this exact commit is in the action's explicit env-files loop. With a trusted workflow PROMPTFOO_API_KEY, such a file can set PROMPTFOO_REMOTE_API_BASE_URL; the action first validates against the trusted host and then launches Promptfoo with --share, allowing the child to send the inherited key to the repository-selected host. Unlike the earlier path-resolution comment, the fresh evidence here is that this tree contains no implicit/config env preflight at all.
Useful? React with 👍 / 👎.
| const FORBIDDEN_AUTH_KEYS = new Set([ | ||
| 'PROMPTFOO_API_KEY', | ||
| 'PROMPTFOO_REMOTE_API_BASE_URL', | ||
| ]); |
There was a problem hiding this comment.
Even an explicit env-files file that reaches this check can still redirect an inherited Promptfoo credential because the protected set contains only these two names. Promptfoo also honors PROMPTFOO_CLOUD_API_URL as a cloud API destination, so a repository file can set that variable while the trusted workflow supplies PROMPTFOO_API_KEY; the action's getApiHost() ignores this alias and validates against the default host, but the subsequently launched --share process inherits the attacker-selected cloud endpoint. Add all Promptfoo API-routing aliases to the protected set rather than allowing every other PROMPTFOO_ variable through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: ac084f29f1
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| const FORBIDDEN_AUTH_KEYS = new Set([ | ||
| 'PROMPTFOO_API_KEY', | ||
| 'PROMPTFOO_REMOTE_API_BASE_URL', | ||
| ]); |
There was a problem hiding this comment.
For workflows invoking the action normally via uses:, action.yml executes dist/index.js, not this source. This commit leaves that bundle byte-for-byte unchanged, and its env-files path still calls dotenv.config({ override: true }) without findForbiddenAuthKey. Therefore, when a secret-bearing workflow selects a repository env file and sharing remains enabled, a contributor can set only PROMPTFOO_REMOTE_API_BASE_URL; the bundle then sends the inherited PROMPTFOO_API_KEY to that host during validation. Regenerate and commit dist/ so the shipped entrypoint enforces this guard.
Useful? React with 👍 / 👎.
…njection' into mdangelo/audit-security-pr-972
…njection' into mdangelo/audit-security-pr-972
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9450959603
ℹ️ 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".
| for (const selectedPath of configuredPath.split(',')) { | ||
| const trimmedPath = selectedPath.trim(); | ||
| if (trimmedPath) { | ||
| environmentPaths.add(path.resolve(workingDirectory, trimmedPath)); |
There was a problem hiding this comment.
When the config is below workingDirectory, Promptfoo's resolveConfigs() resolves each relative commandLineOptions.envPath against the config file's directory before evaluation, while this line checks it against the working directory. For example, configs/promptfooconfig.yaml with envPath: .env.prod causes the guard to inspect ./.env.prod, but Promptfoo subsequently loads configs/.env.prod; protected routing variables in the latter therefore bypass preflight and can redirect an inherited Promptfoo key. The fresh evidence relative to the earlier thread is its verified resolver behavior, which shows that the mismatch is the inverse of the previously reported one.
Useful? React with 👍 / 👎.
| }) as { | ||
| commandLineOptions?: { envPath?: string | string[] }; | ||
| } | null; | ||
| const selectedPaths = config?.commandLineOptions?.envPath; |
There was a problem hiding this comment.
When a YAML or JSON config obtains commandLineOptions.envPath through Promptfoo's supported local $ref or extended-config resolution, this reads only the raw top-level object and never traverses the referenced config. Promptfoo later dereferences that config and loads its selected env file, so a referenced file can select an env file containing PROMPTFOO_REMOTE_API_BASE_URL or another protected routing variable without this preflight examining it. Resolve the same bounded local config graph that Promptfoo will evaluate before collecting environment paths.
Useful? React with 👍 / 👎.
| if (fs.existsSync(environmentPath)) { | ||
| loadEnvironmentFile(environmentPath, {}); |
There was a problem hiding this comment.
When trusted workflow state sets DOTENV_KEY and the checkout contains .env.vault without a plaintext .env, dotenv selects and decrypts the vault file directly, but this existence check prevents loadEnvironmentFile() from running at all. Promptfoo subsequently performs its own dotenv startup and loads the vault, so a protected routing variable stored there can bypass the guard and be paired with an inherited PROMPTFOO_API_KEY; the same issue applies to a configured .env.production.vault when envPath names the absent .env.production. Check the vault companion selected by dotenv rather than conditioning preflight solely on the plaintext path.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| preflightPromptfooEnvironmentFiles(configAbsolutePath, workingDirectory); |
There was a problem hiding this comment.
When prompts is configured and a PR changes only workingDirectory/.env or a file selected by commandLineOptions.envPath, neither file is included by extractFileDependencies(), so the unchanged-prompt branch returns before this new preflight call. The check therefore succeeds without evaluating an input that changes Promptfoo's results and without rejecting newly introduced protected authentication settings, allowing such a repository change to merge and break later evaluations. Add these environment paths to dependency relevance before deciding to skip.
Useful? React with 👍 / 👎.
| for (const selectedPath of configuredPath.split(',')) { | ||
| const trimmedPath = selectedPath.trim(); | ||
| if (trimmedPath) { | ||
| environmentPaths.add(path.resolve(workingDirectory, trimmedPath)); |
There was a problem hiding this comment.
When a static YAML config computes commandLineOptions.envPath through Promptfoo templating, this loop treats the template itself as a literal filename while Promptfoo later renders it before loading the environment. For example, env: { PICK: '.env.capture' } with envPath: '{{ env.PICK }}' makes this inspect a nonexistent {{ env.PICK }} path, after which Promptfoo loads .env.capture; protected routing variables in that file can therefore reach the child unchecked. Reject computed env paths or resolve them using the same trusted configuration phase as Promptfoo.
Useful? React with 👍 / 👎.
| for (const selectedPath of configuredPath.split(',')) { | ||
| const trimmedPath = selectedPath.trim(); | ||
| if (trimmedPath) { | ||
| environmentPaths.add(path.resolve(workingDirectory, trimmedPath)); |
There was a problem hiding this comment.
When repository-controlled config uses an absolute envPath or a traversal such as ../../../../dev/zero, path.resolve() accepts it without checking either lexical or real-path containment, and the subsequent dotenv call synchronously opens that runner path. This lets a pull request make the action read files or special devices outside the checkout, including causing a pre-evaluation hang or memory exhaustion; validate containment, symlink targets, and regular-file bounds before reading.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4e75e703e
ℹ️ 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".
| ): void { | ||
| const environmentPaths = new Set([path.join(workingDirectory, '.env')]); | ||
|
|
||
| if (/\.(?:json|ya?ml)$/i.test(configPath) && fs.existsSync(configPath)) { |
There was a problem hiding this comment.
When the action uses a supported executable config such as promptfooconfig.js or .ts, this extension gate skips the config entirely—the new test at __tests__/env.test.ts:245-251 explicitly preserves that behavior. Such a config can export commandLineOptions: { envPath: '.env.capture' }; Promptfoo then loads that file, while this preflight checks only the implicit .env, allowing protected routing variables in the selected file to reach the child alongside a trusted PROMPTFOO_API_KEY. Reject executable configs when they cannot be inspected safely, or resolve their environment selection through a trusted equivalent of Promptfoo's loader.
Useful? React with 👍 / 👎.
…njection' into mdangelo/audit-security-pr-972 # Conflicts: # dist/index.js # dist/index.js.map # src/utils/env.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c5bc85d45
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| if (isStaticConfig && fs.existsSync(configPath)) { |
There was a problem hiding this comment.
When config is a Promptfoo-supported glob such as configs/*.yaml, the name passes the static-extension test but fs.existsSync(configPath) is false, so no selected config is inspected. Promptfoo subsequently expands the glob and can load an envPath containing a protected routing variable after the action has validated an inherited PROMPTFOO_API_KEY, allowing the child’s authenticated share request to use the unchecked destination. Reject config patterns or expand and inspect every matched config before authentication.
Useful? React with 👍 / 👎.
Summary
commandLineOptions.envPathin YAML/JSON, including bounded local$refchains, fragments, nested configs, and in-workspace symlinks; fail closed for executable/dynamic/ambiguous configs, unsafe paths, and resource-amplification casesdist/bundle from the trusted check-dist artifactSecurity impact
Fixes Codex Security finding
csf_28f8d46cff789d7927863877/ occurrenceocc_4d36979898e538bc09fb79fd.A contributor-controlled environment source could replace the remote API or provider host while an inherited credential remained present. The action now validates every repository-selected environment source before authentication or child execution, requires protected credentials and their destinations to come from trusted workflow state, and prevents relevant config/env changes from being incorrectly skipped.
The config preflight intentionally rejects JavaScript/TypeScript configs, config globs/templates, ambiguous extended refs, additional implicit
promptfooconfig.*/redteam.*configs, escaping symlinks/traversal, non-regular or oversized inputs, and unsafe YAML/ref forms. Static YAML/JSON and ordinary application variables remain supported.Compatibility and overlap
Trusted custom Promptfoo hosts remain supported. Later selected env files retain their documented override behavior for ordinary application variables, and unrelated changes still skip cleanly.
The bounded dependency-relevance work overlaps the config/dependency lanes in #974, #979, and #981; those branches should restack/fix-forward rather than reintroducing the earlier skip behavior. General URL/redirect/SSRF hardening remains separately tracked.
Validation
npm run all: 10 test files, 1,341 tests, 100% statements/branches/functions/linesdist/comes from the GitHub check-dist artifact using the locked dependencies; no Socket bypass or dependency overlay