fix(greenfield): isolate sealer verification environment - #112
Conversation
📝 WalkthroughWalkthroughVerification now runs in per-run temporary homes with staged Corepack pnpm. Workspace configuration is validated before offline frozen dependency setup, and sealing records the dedicated setup observation. Tests cover credential isolation, hook suppression, Corepack staging, and fail-closed config dependency cases. ChangesCandidate verification isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant seal
participant runCommand
participant CorepackCache
participant pnpm
participant Evidence
seal->>runCommand: execute verification command
runCommand->>CorepackCache: stage pinned pnpm in temporary HOME
runCommand->>pnpm: run with filtered environment
pnpm-->>runCommand: return command result
runCommand->>Evidence: record isolated command observation
seal->>pnpm: run offline frozen dependency setup
pnpm-->>seal: return setup timing and exit details
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 589540b209
ℹ️ 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 result = spawnSync( | ||
| 'pnpm', | ||
| ['install', '--frozen-lockfile', '--offline', '--ignore-scripts', '--ignore-pnpmfile'], |
There was a problem hiding this comment.
When pnpm on PATH is a Corepack shim and the owner has COREPACK_ENABLE_UNSAFE_CUSTOM_URLS=1, a candidate can change its packageManager to pnpm@<URL> and this setup will download and execute that candidate-selected package manager with the owner's complete environment and HOME before --offline or the script-suppression flags reach pnpm. I checked the installed Corepack parseSpec implementation: its custom-URL guard is explicitly disabled by that inherited variable. Validate the repository's exact package-manager pin and invoke a trusted binary under a sanitized setup environment instead, or a candidate can read/exfiltrate owner credentials during sealing.
AGENTS.md reference: AGENTS.md:L64-L64
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/seal-candidate.mjs (1)
303-325: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument why this call deliberately inherits the owner environment.
Unlike
runCommand, thisspawnSyncintentionally omitsenvandshellso the owner's registry/cache config is available, and safety rests entirely on the fixed argv (--offline --frozen-lockfile --ignore-scripts --ignore-pnpmfile) plus theconfigDependenciesguard. That invariant is invisible here; a one-line comment makes it much harder for a later change to addshell: trueor an interpolated argument and silently hand the owner environment to candidate-controlled code.🤖 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 `@scripts/seal-candidate.mjs` around lines 303 - 325, In dependencySetupObservation, add a concise comment before the spawnSync call documenting that it intentionally inherits the owner environment and omits env and shell; state that safety depends on the fixed offline, frozen-lockfile, ignore-scripts, and ignore-pnpmfile arguments plus assertSetupDoesNotUseConfigDependencies. Keep the implementation unchanged.scripts/seal-candidate.test.mjs (3)
319-355: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo positive control for a valid
pnpm-workspace.yaml.All five cases assert rejection; none assert that a normal workspace file still seals. That gap is what lets the over-broad root-key check in
scripts/seal-candidate.mjs(Lines 293-300) reject idiomatic YAML unnoticed. Add a case with a workspace file whosepackagessequence entries sit at column zero and assertseal.valid === true.🤖 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 `@scripts/seal-candidate.test.mjs` around lines 319 - 355, Add a positive-control test near the existing config-dependency rejection cases, using the fixture helper with a valid pnpm-workspace.yaml containing a top-level packages sequence whose entries are at column zero, and assert that the resulting seal has valid === true. Keep the test focused on confirming idiomatic workspace YAML is accepted.
55-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
pnpm installhere can reach the network and repeats the version pin.Line 74 omits
--offline, so on a cold store this lockfile generation performs registry resolution and the test becomes network-dependent even though the dependency isfile:-local.--offline(or--prefer-offline) keeps it hermetic. Thepnpm@11.9.0literal at Line 64 also duplicates the pin inscripts/seal-candidate.mjs; hoisting it to a single constant read from the rootpackage.jsonavoids drift across both files.🤖 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 `@scripts/seal-candidate.test.mjs` around lines 55 - 75, Update addLockedLocalDependency to run pnpm install with --offline so lockfile generation remains network-independent. Remove the duplicated pnpm@11.9.0 literal by sourcing the package-manager version from the root package.json through a shared constant or equivalent used by both seal-candidate test and implementation flows.
248-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests fail for environmental reasons, not defects.
Two ambient assumptions:
- Lines 255 and 287 hard-assert that a Corepack pnpm payload is already cached in the owner's home. On a fresh checkout or a CI image that has never run
corepack, both tests fail even though the sealer is correct. Staging the payload is a precondition, not the behavior under test — prefert.skip()when the source directory is absent, or provision it explicitly.- Line 251 does
join(environmentValue('HOME'), '.cache')and Line 284 doesjoin(environmentValue('HOME'), ...)with no fallback; an unsetHOMEthrowsTypeErrorfrompath.join.stageCorepackPnpminscripts/seal-candidate.mjsguards this with?? tmpdir(). Line 284 also ignoresXDG_CACHE_HOME, diverging from Line 251 and from the source lookup order.🤖 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 `@scripts/seal-candidate.test.mjs` around lines 248 - 291, Update the Corepack cache setup in both tests to resolve the source using XDG_CACHE_HOME first, then HOME with a tmpdir fallback, matching stageCorepackPnpm. Treat a missing pnpm payload as an unmet test precondition by skipping the test rather than asserting existsSync true; adjust the test callbacks as needed to access the skip mechanism while preserving the existing staging and credential assertions.
🤖 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 `@scripts/seal-candidate.mjs`:
- Around line 239-301: Update assertSetupDoesNotUseConfigDependencies in
scripts/seal-candidate.mjs (lines 239-301) to skip root-indentation lines that
are exactly "-" or begin with "- " before calling yamlRootMappingKey, while
retaining fail-closed handling for ambiguous mapping keys. Add a positive
regression case in scripts/seal-candidate.test.mjs (lines 319-355) with
packages: and column-zero sequence entries, asserting seal.valid is true and
setupError is absent.
---
Nitpick comments:
In `@scripts/seal-candidate.mjs`:
- Around line 303-325: In dependencySetupObservation, add a concise comment
before the spawnSync call documenting that it intentionally inherits the owner
environment and omits env and shell; state that safety depends on the fixed
offline, frozen-lockfile, ignore-scripts, and ignore-pnpmfile arguments plus
assertSetupDoesNotUseConfigDependencies. Keep the implementation unchanged.
In `@scripts/seal-candidate.test.mjs`:
- Around line 319-355: Add a positive-control test near the existing
config-dependency rejection cases, using the fixture helper with a valid
pnpm-workspace.yaml containing a top-level packages sequence whose entries are
at column zero, and assert that the resulting seal has valid === true. Keep the
test focused on confirming idiomatic workspace YAML is accepted.
- Around line 55-75: Update addLockedLocalDependency to run pnpm install with
--offline so lockfile generation remains network-independent. Remove the
duplicated pnpm@11.9.0 literal by sourcing the package-manager version from the
root package.json through a shared constant or equivalent used by both
seal-candidate test and implementation flows.
- Around line 248-291: Update the Corepack cache setup in both tests to resolve
the source using XDG_CACHE_HOME first, then HOME with a tmpdir fallback,
matching stageCorepackPnpm. Treat a missing pnpm payload as an unmet test
precondition by skipping the test rather than asserting existsSync true; adjust
the test callbacks as needed to access the skip mechanism while preserving the
existing staging and credential assertions.
🪄 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 Plus
Run ID: 16034a77-87d0-4d07-b488-102a3cfb77f9
📒 Files selected for processing (2)
scripts/seal-candidate.mjsscripts/seal-candidate.test.mjs
| function yamlRootMappingKey(line) { | ||
| const mapping = line.trimEnd(); | ||
| if (['?', '&', '*', '!', '{', '[', '-'].includes(mapping[0])) return null; | ||
| if (mapping.startsWith('"')) { | ||
| let escaped = false; | ||
| for (let index = 1; index < mapping.length; index += 1) { | ||
| const character = mapping[index]; | ||
| if (escaped) { | ||
| escaped = false; | ||
| continue; | ||
| } | ||
| if (character === '\\') { | ||
| escaped = true; | ||
| continue; | ||
| } | ||
| if (character !== '"') continue; | ||
| if (!/^\s*:/.test(mapping.slice(index + 1))) return null; | ||
| try { | ||
| return JSON.parse(mapping.slice(0, index + 1)); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| if (mapping.startsWith("'")) { | ||
| for (let index = 1; index < mapping.length; index += 1) { | ||
| if (mapping[index] !== "'") continue; | ||
| if (mapping[index + 1] === "'") { | ||
| index += 1; | ||
| continue; | ||
| } | ||
| if (!/^\s*:/.test(mapping.slice(index + 1))) return null; | ||
| return mapping.slice(1, index).replaceAll("''", "'"); | ||
| } | ||
| return null; | ||
| } | ||
| const separator = mapping.indexOf(':'); | ||
| if (separator <= 0) return null; | ||
| const key = mapping.slice(0, separator).trim(); | ||
| return key && key !== '<<' && !/\s/.test(key) ? key : null; | ||
| } | ||
|
|
||
| function assertSetupDoesNotUseConfigDependencies(repository) { | ||
| const workspace = join(repository, 'pnpm-workspace.yaml'); | ||
| if (!existsSync(workspace)) return; | ||
| const lines = readFileSync(workspace, 'utf8') | ||
| .split('\n') | ||
| .map((line) => ({ line, indentation: line.match(/^\s*/)[0].length })) | ||
| .filter( | ||
| ({ line }) => line.trim() && !line.trimStart().startsWith('#') && !/^(---|\.\.\.)\s*(?:#.*)?$/.test(line.trim()), | ||
| ); | ||
| if (lines.length === 0) return; | ||
| const rootIndentation = Math.min(...lines.map(({ indentation }) => indentation)); | ||
| for (const { line, indentation } of lines) { | ||
| if (indentation !== rootIndentation) continue; | ||
| const key = yamlRootMappingKey(line.slice(rootIndentation)); | ||
| if (key === null) | ||
| fail('OWNER_DECISION_REQUIRED: dependency setup cannot safely inspect an ambiguous top-level pnpm workspace key'); | ||
| if (key === 'configDependencies') | ||
| fail('OWNER_DECISION_REQUIRED: dependency setup cannot inherit owner environment with configDependencies'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Over-broad root-key classification in the workspace guard, unguarded by any accept-path test. yamlRootMappingKey returns null for any root-indentation line beginning with -, but YAML allows block sequence entries at the parent key's indentation (packages: followed by - 'packages/*' at column zero), so idiomatic pnpm-workspace.yaml files abort sealing with OWNER_DECISION_REQUIRED. The regression suite only exercises rejection paths, which is why the false positive is invisible.
scripts/seal-candidate.mjs#L239-L301: skip root-indentation lines that start with-(or are exactly-) before callingyamlRootMappingKey; they cannot be mapping keys, soconfigDependenciesdetection stays fail-closed.scripts/seal-candidate.test.mjs#L319-L355: add a positive case writing a workspace file withpackages:and its sequence entries at column zero, assertingseal.valid === trueand nosetupError.
📍 Affects 2 files
scripts/seal-candidate.mjs#L239-L301(this comment)scripts/seal-candidate.test.mjs#L319-L355
🤖 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 `@scripts/seal-candidate.mjs` around lines 239 - 301, Update
assertSetupDoesNotUseConfigDependencies in scripts/seal-candidate.mjs (lines
239-301) to skip root-indentation lines that are exactly "-" or begin with "- "
before calling yamlRootMappingKey, while retaining fail-closed handling for
ambiguous mapping keys. Add a positive regression case in
scripts/seal-candidate.test.mjs (lines 319-355) with packages: and column-zero
sequence entries, asserting seal.valid is true and setupError is absent.
1 participant
Purpose
Fix the delivery-package sealer so arbitrary verification commands cannot inherit or log owner-home credentials.
Scope
--frozen-lockfile --offline --ignore-scripts --ignore-pnpmfile)configDependencies, including quoted, decoded, and ambiguous YAML formsExact candidate evidence
589540b20949b44f5dd0d59bb2827d1d8ad861db64da452bd7d584c8b14dcbb7c26f2eb6554b9de94b067f46d3141f741c95966d92eee50abb67be3a/7c37ce192d905e5afd4a2bf2a108a0d890397db8/private/tmp/jig-seal-isolated-home-589540b2/envelope.json(valid: true; setup, diff check, andpnpm checkeach passed once)This is a prerequisite correction only; it enables no provider, effect, consumer, or public surface.
Summary by CodeRabbit
Security Enhancements
Reliability