fix: reject env-file process controls - #971
Conversation
|
@codex review Please review the exact current head: |
![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: fd96efa598
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR hardens the GitHub Action’s env-files feature by preventing repository-controlled .env files from setting process-control environment variables that could influence Node/npm startup or executable resolution before Promptfoo runs.
Changes:
- Introduces
src/utils/env.tsto load.envfiles into an isolated object, validate keys, then apply values to the target environment while preserving “later file wins”. - Updates
src/main.tsto use the new guarded loader instead of callingdotenv.config()directly. - Updates docs/metadata (
README.md,action.yml) and regenerates the bundleddist/index.js; adds regression tests for forbidden variables.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/env.ts | New isolated .env parsing + forbidden-key validation + controlled application to process.env. |
| src/main.ts | Switches env loading to loadEnvironmentFile() and removes direct dotenv usage. |
| README.md | Clarifies that env-files rejects process-control variables. |
| action.yml | Clarifies that env-files rejects process-control variables. |
| tests/main.test.ts | Adds tests for forbidden variable rejection (case-insensitive) and later-file-wins behavior. |
| dist/index.js | Regenerated bundle reflecting the new env-loading implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up hardening on top of the env-file blocklist, closing the gaps found while reviewing the change: - git controls: add a GIT_ prefix (GIT_SSH_COMMAND, GIT_EXTERNAL_DIFF, GIT_PROXY_COMMAND, GIT_CONFIG_COUNT/KEY_n/VALUE_n, ...). The action runs `git diff` through simple-git after loading env-files and that child inherits process.env, so git controls share the Node/npm trust boundary. - interpreter injection: reject PERL5OPT, PYTHONHOME, PYTHONEXECUTABLE, PYTHONSTARTUP, RUBYOPT. Module search-path vars (PYTHONPATH/RUBYLIB/ PERL5LIB) are intentionally still allowed — real providers need them and they grant nothing beyond code the reviewed config already runs. - document why validation lives at the untrusted-file boundary (the action and child both read merged values from process.env; the final child env legitimately inherits the trusted runner's PATH/NODE_OPTIONS). - tests: new __tests__/env.test.ts exercises findForbiddenEnvFileKey and loadEnvironmentFile against real dotenv parsing and real files (the existing suite mocks dotenv), covering isolation-on-reject, later-file- wins, case-insensitivity, prefix rules, and load errors; extend the integration matrix with git/interpreter cases and a later-file rejection. 282 tests, 100% coverage. - README: document the workflow-env migration path for proxy/CA/process controls and that ordinary application vars still pass through. - regenerate dist with the lockfile's esbuild 0.28.1 (the committed bundle was built with 0.28.0 and would otherwise fail check-dist). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c20b8a5515
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…v-startup-injection # Conflicts: # dist/index.js.map
…ist) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54c852b161
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…-pr-971-019f62c3 # Conflicts: # dist/index.js.map
|
@codex review Please re-review the latest head |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
| throw new PromptfooActionError( | ||
| 'Invalid config directory: the config must stay within the working directory.', | ||
| ErrorCodes.INVALID_CONFIGURATION, | ||
| 'Use a readable config file and directory within the working directory.', | ||
| ); |
| throw new PromptfooActionError( | ||
| 'Invalid config directory: the config must stay within the working directory.', | ||
| ErrorCodes.INVALID_CONFIGURATION, | ||
| 'Use a readable config file and directory within the working directory.', | ||
| ); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c678b2573
ℹ️ 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".
| (configGlobRepositoryRoot !== undefined && | ||
| file.startsWith(configGlobRepositoryRoot)), |
There was a problem hiding this comment.
When config is a root-level glob such as *.yaml or eval*.yaml, directoryPrefix becomes empty, so configGlobRepositoryRoot is ''; because every string satisfies file.startsWith(''), any changed file in the repository is treated as a config change. In workflows that use a root config glob with prompts filtering, unrelated changes like docs/readme.md will now force a full Promptfoo evaluation instead of skipping, which can spend provider/API time unexpectedly. Restrict this fallback to root-level candidates or match the configured glob rather than using an empty prefix.
Useful? React with 👍 / 👎.
| if (typeof envPath === 'string') { | ||
| if (envPath.includes(',')) { | ||
| markUnsafeDependency(); | ||
| } else { | ||
| processFilePath(envPath); |
There was a problem hiding this comment.
When a config sets commandLineOptions.envPath, this branch only records that file as a dependency; Promptfoo later loads that env file from the config during eval, outside the action's loadEnvironmentFile forbidden-key checks. In a PR that changes the config to point at .env.evil, values such as OPENAI_BASE_URL, AWS_*, or other process/provider controls can still reach the Promptfoo process and redirect trusted credentials despite the new env-file hardening. Validate these config-referenced env files before invoking Promptfoo, or reject config-level envPath in this action.
Useful? React with 👍 / 👎.
| const hasParentAfterMagic = pathParts | ||
| .slice(firstMagicPart) | ||
| .some((part) => part === '..'); | ||
| if (hasCrossDirectoryAlternative || hasParentAfterMagic) { |
There was a problem hiding this comment.
This only treats a literal .. segment as parent traversal after the first glob token, but glob syntax like [.][.] or @(.)@(.) can also match ... For a config dependency such as file://providers/[.][.]/shared/*.py, the action records only the providers/ sentinel, so a change to shared/foo.py can be missed and the evaluation skipped even though Promptfoo expands the dependency. Detect segments whose glob can match .. (as validatePromptGlob does) or conservatively return the workspace sentinel.
Useful? React with 👍 / 👎.
| if (extractNestedFileReferences(httpConfig.body)) { | ||
| markUnsafeDependency(); |
There was a problem hiding this comment.
For HTTP providers whose request body contains normal Promptfoo variables, such as body: { prompt: "{{prompt}}" }, extractNestedFileReferences returns true solely because it sees the template delimiters, so this marks the whole config unsafe and returns the workspace sentinel. In workflows that otherwise monitor prompt/config files, any unrelated PR change then triggers full prompt evaluation even though the body template is just runtime data, not an untracked file dependency; only fail closed here for actual templated file references or executable hooks.
Useful? React with 👍 / 👎.
| (/[\\/]/.test(rawPath) && | ||
| !path.extname(rawPath) && | ||
| !path.extname(candidatePath)) | ||
| ) { | ||
| markUnsafeDependency(); |
There was a problem hiding this comment.
Promptfoo supports external prompt references such as langfuse://... and portkey://..., but this extensionless-path fallback treats those URL schemes as local executable-style prompt paths whenever they contain slashes and no extension, marking the entire workspace as a dependency. With remote config-defined prompts, unrelated file changes now force full evaluations instead of respecting the monitored prompt/config scope; skip recognized non-file URL schemes before applying this local-path heuristic.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Substantial, careful security hardening: repository .env files are now parsed into an isolated object, screened against a large process-control denylist (Node/npm/loader/proxy/etc.) before any value reaches process.env or the npx promptfoo child, and npx is isolated from a repo .npmrc via --prefix. The threat model is sound and the denylist is thorough. My comments are about minimality and test quality, not correctness of the gate:
- The containment check (
resolveContainedEnvFile) runs up to three times per env file with identical inputs, and re-implements the existingisPathInsidehelper twice inline with redundantrealpathSync().toString()/path.resolve()wrappers. - Several denylist entries and a loop guard are dead code (see inline).
- The broadened masking scans all of
process.envandsetSecrets any secret-named value — safe direction, but a repo-controlled env file can register a 1–2 char value as a global log mask; a min-length guard fixes that. - Two new tests don't exercise what they claim: the
__proto__test asserts a dotenv state real dotenv can't produce, and thenpx --prefixisolation test only asserts argv shape on a mock.
None block the security fix; addressing the simplifications would meaningfully shrink the diff.
Merge/stacking note: Base main; #972 is stacked on this branch. Conflicts only with #976/#972 on README's trailing section.
Additional findings (not on changed lines, noted here):
-
🟡 P3 · simplification — Containment validation runs up to 3× per env file; two of the three calls are pure duplicates — in
src/main.ts(nearresolveContainedEnvFile(envFilePath);)This call discards its result, then the following
return resolveContainedEnvFile(effectivePath)re-validates the identical path (when no vault is selectedeffectivePath === envFilePath). The two load sites then validate a third time paths that are already the validated return values from this map. That triples theexistsSync+ 2×realpathSyncsyscall work and obscures where the single trust-boundary check actually lives.
Minimal fix: validate once when building the list (keep only return resolveContainedEnvFile(effectivePath);) and pass the already-validated paths to loadEnvironmentFile directly.
-
🔵 nit · simplification — Redundant length guard around a for…of loop — in
src/main.ts(nearif (explicitEnvFiles.length > 0) {)Iterating an empty array is already a no-op, so this guard adds two lines and a nesting level for no behavior. Delete the wrapper and keep the bare
for…of.
| ) { | ||
| throw new Error('Config path escapes the workspace'); | ||
| } | ||
| realRoots.workspaceRoot ??= path.resolve( |
There was a problem hiding this comment.
🟡 P3 · simplification — resolveContainedEnvFile re-implements isPathInside twice, with redundant realpath wrappers
src/utils/config.ts already exports the exact three-clause containment check this inlines twice (lexical + realpath): relative === '..' || startsWith('..'+sep) || isAbsolute. Also path.resolve(fs.realpathSync(x).toString()) is doubly redundant — realpathSync(string) already returns an absolute canonical string, so both .toString() and the path.resolve() wrapper are no-ops.
Minimal fix: move/export isPathInside to a shared util and reduce this to two if (!isPathInside(base, p)) throw … checks using fs.realpathSync(...) directly (~35 lines → ~12).
| 'CC', | ||
| 'CC_HOST', | ||
| 'CC_TARGET', | ||
| 'CGO_CFLAGS', |
There was a problem hiding this comment.
🟡 P3 · simplification — Five denylist entries are dead — already covered by FORBIDDEN_ENV_FILE_PREFIXES
FORBIDDEN_ENV_FILE_PREFIXES contains 'CGO_' and 'OTEL_EXPORTER_OTLP_', and findForbiddenEnvFileKey ORs the set against the prefixes. So 'CGO_CFLAGS', 'CGO_CPPFLAGS', 'CGO_CXXFLAGS', 'CGO_LDFLAGS', and 'OTEL_EXPORTER_OTLP_ENDPOINT' in the set can never be the reason a key is rejected. In a 300-line denylist the comment says should stay "easy to audit," delete these five lines so each rule has one home.
| workingDirectory: string, | ||
| pattern: string, | ||
| realRoots: { workspaceRoot?: string; workingDirectory?: string }, | ||
| ): void { |
There was a problem hiding this comment.
🟡 P3 · security — Broad masking lets a repo-controlled env file register a short value as a global log mask
maskApiKeys() is re-invoked after merging the implicit .env and each explicit env file, scanning all of process.env and calling core.setSecret(value) for any secret-named key. A *_TOKEN/*_SECRET/*_API_KEY key is not in the process-control denylist (those are legitimate app credentials), so an attacker's .env with e.g. X_SECRET=1 passes validation and setSecret('1') masks every 1 in the rest of the step log (eval output, cache metrics, even the action's own error messages). Over-masking is the safe direction, but this is new repo-triggerable log-integrity surface.
Minimal fix: gate the scanned branch on a minimum length (GitHub's own guidance), e.g. if (value && value.length >= 8) apiKeys.push(value);. Keep the explicit action-input keys masked unconditionally.
|
|
||
| await run(); | ||
|
|
||
| const output = [ |
There was a problem hiding this comment.
🟡 P3 · tests — npx --prefix isolation test is tautological — it asserts argv shape on a mock
The test only checks args.slice(0,2) equals ['--prefix', <actionDir>] against the mocked @actions/exec; it verifies nothing about whether a repo .npmrc is actually bypassed or whether npx still resolves promptfoo with the flag inserted. The one line every run flows through has no coverage that could fail if it broke. Consider an opt-in integration test that runs the real npx --prefix <actionDir> promptfoo@<pinned> --version from a cwd containing a poisoned .npmrc, or at minimum rename the test so it doesn't claim to verify isolation.
| mockOctokit.paginate.mockResolvedValue([{ filename: 'README.md' }]); | ||
| mockGlob.sync.mockReturnValue(['prompts/first.txt']); | ||
| mockConfig.extractFileDependencies.mockReturnValue([ | ||
| 'data/context\n::error::forged.json', |
There was a problem hiding this comment.
🟡 P3 · tests — proto test asserts a dotenv state real dotenv can't produce; actual behavior is untested and contradicts the README
This test uses Object.defineProperty to force a mocked dotenv.config to emit an own enumerable __proto__ key, then asserts the run fails. Real dotenv 17.4.2 (pinned) never does this: obj['__proto__'] = 'string' is a silent no-op, so a .env with __proto__=x is silently dropped and the run proceeds — not "rejected before merging" as the README security note implies. Tellingly, the real-parser matrix in env.test.ts covers constructor/prototype but omits __proto__ (it would fail). Add a real-dotenv test asserting the silent-drop behavior and correct the README, or rename this test to state it guards a hypothetical parser.
| return filePath.split(path.sep).join('/'); | ||
| } | ||
|
|
||
| function isPathInside(baseDir: string, targetPath: string): boolean { |
There was a problem hiding this comment.
🔵 nit · simplification — maskApiKeys re-masks the 12 static input keys on every call
maskApiKeys() runs once at startup and again after every env-file load, rebuilding the 12-element static-input array and re-setSecreting values that can't have changed — duplicate ::add-mask:: commands. Mask the static input keys once outside the closure; keep only the process.env rescan inside.
- maskApiKeys now only registers secret-shaped values of >=8 chars. A repository-controlled env file could otherwise set a secret-named variable to a 1-2 char value and turn core.setSecret into a global log mask that garbles the action's own diagnostics. - Remove CGO_CFLAGS/CGO_CPPFLAGS/CGO_CXXFLAGS/CGO_LDFLAGS and OTEL_EXPORTER_OTLP_ENDPOINT from FORBIDDEN_ENV_FILE_KEYS; they are already covered by the CGO_ and OTEL_EXPORTER_OTLP_ prefixes. - Drop the redundant `if (explicitEnvFiles.length > 0)` guard around a for...of loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c341031140
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const hasExtension = | ||
| extension.length > 0 && /^[A-Za-z0-9]+$/.test(extension); | ||
| if (/[\\/*?{}[\]]/.test(reference)) { | ||
| return !/\s/.test(reference) || /[\\/]/.test(reference) || hasExtension; |
There was a problem hiding this comment.
When a config-defined inline prompt is a short punctuation-only string such as prompts: ['Continue?'] or {json}, this heuristic classifies it as a path glob just because it contains glob metacharacters and no whitespace. processPromptReference() then records the config directory (or ./ when the config is at the repo root) as a dependency, so unrelated file changes under that directory force a full evaluation and can spend provider/API time unexpectedly; require a path signal such as a slash, scheme, or extension before treating prompt text as a file reference.
Useful? React with 👍 / 👎.
Review + fix applied (
|
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: cc37973274
ℹ️ 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".
| setupCacheEnvironment, | ||
| } from './utils/cache'; | ||
| import { extractFileDependencies } from './utils/config'; | ||
| import { loadEnvironmentFile } from './utils/env'; |
There was a problem hiding this comment.
The action executes dist/index.js, but both dist/index.js and dist/index.js.map are byte-for-byte identical to the parent commit, so published users still run the old direct dotenv.config() path and none of this process-control validation takes effect. Unlike the earlier stale-source-map report, the fresh evidence at this SHA is that the executable bundle itself is also unchanged; regenerate and commit the complete dist/ output.
AGENTS.md reference: AGENTS.md:L96-L99
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: cc37973274
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.
| } else { | ||
| core.info(`Successfully loaded ${envFilePath}`); | ||
| } | ||
| loadEnvironmentFile(envFilePath); |
There was a problem hiding this comment.
When a workflow selects a checkout-controlled env-files file, rebuild dist/index.js before shipping this change. action.yml runs that bundle, but this commit leaves its blob byte-for-byte identical to the parent; it still loads directly into process.env, so this new guard never executes. A same-repository contributor can set PATH to a checked-in fake npx or use NODE_OPTIONS to execute code with job credentials. I reproduced the PATH attack. Unlike the earlier stale-source-map comment, the fresh evidence is that the executable JS itself is stale.
Useful? React with 👍 / 👎.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ac1d966ed
ℹ️ 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".
| 'APPDATA', | ||
| 'BASH_ENV', |
There was a problem hiding this comment.
When a selected env file sets AWS_CONFIG_FILE or AWS_SHARED_CREDENTIALS_FILE to a checkout-controlled INI containing credential_process, this denylist accepts the variable and loadEnvironmentFile() forwards it to the Promptfoo child. For evaluations using an AWS provider's default credential chain, the AWS SDK can consequently execute the configured command; reject these credential-configuration path controls before merging the file environment.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
| 'PERL5OPT', | ||
| 'PYTHONEXECUTABLE', |
There was a problem hiding this comment.
When env-files contains PROMPTFOO_CACHE_PATH and the action's cache-path input is omitted, this list accepts the value, after which setupCacheEnvironment() retains it and the CI path in src/main.ts passes it to cleanupOldCache(). That function recursively deletes files older than seven days and later writes a manifest, so a repository-controlled env file can redirect cache filesystem operations outside the intended directory; reject this key or validate and confine the resulting path.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
| // GIT_PROXY_COMMAND, GIT_CONFIG_COUNT/KEY_n/VALUE_n, ...). The action shells out | ||
| // to `git` via simple-git after loading these files, and that child inherits | ||
| // process.env, so git controls belong in the same trust boundary as Node/npm. | ||
| const FORBIDDEN_ENV_FILE_PREFIXES = ['GIT_', 'NPM_CONFIG_']; |
There was a problem hiding this comment.
.npmrc
When working-directory contains a checked-in .npmrc, rejecting NPM_CONFIG_* environment variables does not protect the later npx invocation because it runs with that directory as its cwd. npm's bundled npmrc documentation states that a per-project .npmrc at the project root sets project configuration, while the npm exec documentation states that requested packages absent locally are fetched remotely; a PR can therefore set registry to an attacker-controlled service and cause npx promptfoo@<version> to download and execute a replacement package before evaluation. Run npm with a trusted project/config location or reject checkout-controlled npm configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c35d3240d
ℹ️ 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 normalizedKey = key.toUpperCase(); | ||
| return ( | ||
| FORBIDDEN_ENV_FILE_KEYS.has(normalizedKey) || |
There was a problem hiding this comment.
On Linux and macOS, environment-variable names are case-sensitive, so ordinary application settings such as path=/api/v1 or home=local do not control PATH or HOME; uppercasing every key nevertheless classifies them as forbidden and aborts the action. This regresses valid env-files usage on POSIX runners; apply case-insensitive matching only on Windows or only for controls whose consumers explicitly accept lowercase aliases.
Useful? React with 👍 / 👎.
| 'PATH', | ||
| 'PATHEXT', | ||
| 'PERL5OPT', | ||
| 'PROMPTFOO_CACHE_PATH', |
There was a problem hiding this comment.
When a workflow sets PROMPTFOO_PASS_RATE_THRESHOLD in its trusted environment and loads a checked-in env file, that file can override the threshold because this action-control variable is not rejected. The action later reads the overridden value in parsePromptfooPassRateThreshold() and forwards it to Promptfoo, so a PR can set it to 0 and make an evaluation that should fail the trusted threshold pass; reject this variable alongside the other protected PROMPTFOO_* controls.
Useful? React with 👍 / 👎.
| } else { | ||
| core.info(`Successfully loaded ${envFilePath}`); | ||
| } | ||
| loadEnvironmentFile(envFilePath); |
There was a problem hiding this comment.
When the workflow supplies a trusted PROMPTFOO_API_KEY, a selected repository env file can set PROMPTFOO_REMOTE_API_BASE_URL to an attacker-controlled server because that routing variable passes this validation. The subsequent authentication branch calls validatePromptfooApiKey(promptfooApiKey, getApiHost()), which sends the trusted key as a Bearer token to that URL; reject this host override from repository env files so a PR cannot exfiltrate the workflow credential before evaluation starts.
Useful? React with 👍 / 👎.
Summary
dist/bundleSecurity impact
Fixes Codex Security finding
csf_c3d60ff5bf8dd2257cd727d9/ occurrenceocc_4b19ce5c6afabbbe9b229aae.Previously, a contributor-controlled env file selected by
env-filescould setNODE_OPTIONSorPATH. The action loaded it directly intoprocess.envand forwarded the environment tonpx promptfoo, allowing repository code to run before Promptfoo evaluated the reviewed configuration.The new boundary validates every selected file before any of its values can affect the action or child process.
Compatibility
Ordinary application variables, including
NODE_ENV, still reach Promptfoo. Multiple selected files retain later-file-wins semantics. Trusted job-level process controls are not changed.Validation
npx, with no startup observation or evaluation outputNODE_OPTIONS,PATH,NODE_PATH, npm config, loader, proxy, and config-home variantsnpm run allpasses: 9 test files, 251 tests, 100% coveragenpm run packageleaves the working tree clean