[codex] Harden compatibility CI isolation#48
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughHarden compatibility tests: detect and fail on GitHub Actions runtime credentials at test start, sanitize child-process environments, run tests inside a sandboxed action with job-level empty permissions, and update two dependency pins. ChangesSecurity Hardening for Compatibility Tests
Dependency Version Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SegaraRai
force-pushed
the
codex/harden-compatibility-ci
branch
2 times, most recently
from
d2f4747 to
e6351da
Compare
SegaraRai
force-pushed
the
codex/harden-compatibility-ci
branch
from
e6351da to
3c29867
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
examples/compat/scripts/command.ts (1)
10-22: ⚡ Quick winShare the Actions runtime-key list with
security.ts.This re-declares the same
ACTIONS_*keys the startup guard uses. If those lists drift, the parent process and child-process scrubber will enforce different credential policies. Export the runtime-key constant fromsecurity.tsand extend it here with the extra package-manager tokens.Possible direction
+import { FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS } from "./security.ts"; + const BLOCKED_CHILD_ENV_KEYS = new Set([ - "ACTIONS_CACHE_SERVICE_V2", - "ACTIONS_CACHE_URL", - "ACTIONS_ID_TOKEN_REQUEST_TOKEN", - "ACTIONS_ID_TOKEN_REQUEST_URL", - "ACTIONS_RESULTS_URL", - "ACTIONS_RUNTIME_TOKEN", - "ACTIONS_RUNTIME_URL", + ...FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS, "GH_TOKEN", "GITHUB_TOKEN", "NODE_AUTH_TOKEN", "NPM_TOKEN", ]);// examples/compat/scripts/security.ts export const FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS = [ // ... ] as const;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/compat/scripts/command.ts` around lines 10 - 22, Replace the duplicated ACTIONS_* list by importing the canonical array from security.ts and extending it with the package-manager tokens: import FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS from security.ts, then build BLOCKED_CHILD_ENV_KEYS as new Set([...FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS, "GH_TOKEN", "GITHUB_TOKEN", "NODE_AUTH_TOKEN", "NPM_TOKEN"]). Ensure you reference the existing symbol BLOCKED_CHILD_ENV_KEYS in this file and the exported FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS symbol in security.ts so both parent and child scrubbing use the same runtime-key source..github/workflows/ci.yml (1)
225-228: ⚡ Quick winUse the official
setup-vpGitHub Action instead of piping the install script.
curl ... | bashmakes this job depend on whateverviteplus.dev/install.shserves at runtime, weakening reproducibility. Vite+ provides an official GitHub Action,voidzero-dev/setup-vp, designed for CI environments that supports version pinning:- uses: voidzero-dev/setup-vp@v1 with: version: "1.2.3" # Pin to a specific versionThis is the documented best practice for CI installations and avoids the security and reproducibility concerns of piping scripts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 225 - 228, Replace the insecure curl|bash installer with the official GitHub Action by removing the lines that call the remote install script and source "$HOME/.vite-plus/env" and instead use the voidzero-dev/setup-vp action in the workflow; ensure you call the action (voidzero-dev/setup-vp@v1) and pass a pinned version (e.g., version: "1.2.3") so subsequent steps like invoking vp env use "$(cat .node-version)" continue to work against a reproducible, pinned Vite+ installation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/compat/scripts/security.ts`:
- Around line 12-14: The filter for presentKeys currently treats only truthy
values as present, so empty strings are missed; update the predicate used with
FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS (where presentKeys is computed) to check for
presence using process.env[key] !== undefined (or similar explicit undefined
check) instead of relying on truthiness, so variables like
ACTIONS_RUNTIME_TOKEN="" are considered present by the guard.
In `@package.json`:
- Line 33: Replace the unpublished version spec for the wasm-pack dependency in
package.json by changing the "wasm-pack" entry (currently "^0.15.0") to the
official stable release "0.14.0" (or "^0.14.0" if you want caret semantics) so
the project uses the published stable wasm-pack version.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 225-228: Replace the insecure curl|bash installer with the
official GitHub Action by removing the lines that call the remote install script
and source "$HOME/.vite-plus/env" and instead use the voidzero-dev/setup-vp
action in the workflow; ensure you call the action (voidzero-dev/setup-vp@v1)
and pass a pinned version (e.g., version: "1.2.3") so subsequent steps like
invoking vp env use "$(cat .node-version)" continue to work against a
reproducible, pinned Vite+ installation.
In `@examples/compat/scripts/command.ts`:
- Around line 10-22: Replace the duplicated ACTIONS_* list by importing the
canonical array from security.ts and extending it with the package-manager
tokens: import FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS from security.ts, then build
BLOCKED_CHILD_ENV_KEYS as new Set([...FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS,
"GH_TOKEN", "GITHUB_TOKEN", "NODE_AUTH_TOKEN", "NPM_TOKEN"]). Ensure you
reference the existing symbol BLOCKED_CHILD_ENV_KEYS in this file and the
exported FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS symbol in security.ts so both parent
and child scrubbing use the same runtime-key source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad8e20f6-a309-4e46-9ad7-551be32ecc7c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/ci.ymlexamples/compat/scripts/command.tsexamples/compat/scripts/runner.tsexamples/compat/scripts/security.tspackage.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/compat/scripts/security.ts (1)
12-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck env key presence by definition, not truthiness.
Line 13 treats only truthy values as present, so
ACTIONS_RUNTIME_TOKEN=""bypasses the guard. Use an explicitundefinedcheck to enforce the intended fail-fast behavior.Proposed fix
export function assertNoActionsRuntimeCredentials(): void { const presentKeys = FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS.filter((key) => { - return process.env[key]; + return process.env[key] !== undefined; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/compat/scripts/security.ts` around lines 12 - 14, The presentKeys calculation is checking truthiness which allows empty-string env values to pass; change the filter in the FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS check to test for undefined explicitly (e.g., using process.env[key] !== undefined or typeof process.env[key] !== 'undefined') so that keys set to an empty string are treated as present and trigger the guard; update the expression that defines presentKeys accordingly (referencing FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS and the presentKeys variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@examples/compat/scripts/security.ts`:
- Around line 12-14: The presentKeys calculation is checking truthiness which
allows empty-string env values to pass; change the filter in the
FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS check to test for undefined explicitly (e.g.,
using process.env[key] !== undefined or typeof process.env[key] !== 'undefined')
so that keys set to an empty string are treated as present and trigger the
guard; update the expression that defines presentKeys accordingly (referencing
FORBIDDEN_ACTIONS_RUNTIME_ENV_KEYS and the presentKeys variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc1facce-2cf7-4a42-bb0d-12c64cdf43e6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
examples/compat/scripts/command.tsexamples/compat/scripts/security.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/compat/scripts/command.ts
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.
Summary
vp run test:compatwith a clean environment and make the compat runner fail if GitHub Actions runtime cache credentials are presentValidation
vp run checknode ./examples/compat/compat-matrix.ts --listACTIONS_RUNTIME_TOKENis presentSummary by CodeRabbit
Chores
Tests