Skip to content

fix(update): isolate pnpm read probes from projects - #641

Closed
luvs01 wants to merge 4 commits into
devfrom
codex/propose-fix-for-untrusted-project-hooks
Closed

luvs01 wants to merge 4 commits into
devfrom
codex/propose-fix-for-untrusted-project-hooks

Conversation

@luvs01

@luvs01 luvs01 commented Sep 26, 2026 •

Copy link
Copy Markdown
Owner

Motivation

  • Prevent untrusted project code (.pnpmfile.cjs) from being executed during automatic/package-read-only pnpm probes launched by the proxy.
  • The refresh scheduler and registry/owner discovery could spawn pnpm from an attacker-controlled working directory and inherit project hooks and environment.
  • Introduce a minimal, low-risk isolation for read-only checks that preserves existing owner-bound executable selection and behavior for non-pnpm installers.

Description

  • Add src/update/pnpm-read-policy.ts which exposes PNPM_READ_CWD (a trusted working directory) and pnpmReadEnvironment() that forces npm_config_ignore_pnpmfile=true while normalizing conflicting env casing.
  • Use the read-policy when spawning pnpm for owner discovery and registry queries by setting cwd: PNPM_READ_CWD and env: pnpmReadEnvironment(...) in src/update/index.ts and src/update/async-check.ts, preserving the existing unprivilegedOwnershipMutationEnvironment semantics otherwise.
  • Update tests/update/update-refresh.test.ts to add regression coverage asserting the isolation behavior and that the spawned child receives the trusted cwd and the hook-suppression env.
  • Document the security boundary change in structure/ops/service-and-sidecars.md to note that read-only pnpm probes run from the installed update module directory with project pnpmfiles disabled.

Testing

  • Ran the focused update refresh tests with bun test tests/update/update-refresh.test.ts, which passed (29 tests in that file).
  • Ran type checking and broader repository checks with bun x tsc --noEmit, bun run structure:check, and bun run privacy:scan, which completed without errors.
  • Ran broader test invocations exercised during validation (full bun test and layout/file-size guards referenced above) and observed no regressions in the exercised suites.

Codex Task


Devin Review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown

✅ Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository: luvs01/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d1e67032-a2a5-41f2-a9dc-aa5f34e5127c


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Sep 26, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

…o pnpm

- async-check: only pnpm probes run from PNPM_READ_CWD; npm/bun keep the
  caller's working directory so project registry settings still apply.
- index.ts checkUpdatePackageIntegrity: run the integrity registry probe
  with the same isolation as the version lookup.
- ocx.mjs: apply the policy to the launcher's pnpm owner-discovery,
  version, integrity, and update-transaction probes via a shared
  src/update/pnpm-read-policy.mjs (Node-loadable, .d.mts types).
- index.ts runOwnedPnpm: isolate owner-bound pnpm probes (launcher
  re-read and transaction-internal list/root/bin reads).
- tests: cover the synchronous owner-discovery, registry, and integrity
  spawn options plus the non-pnpm cwd passthrough.

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
devin-ai-integration[bot]

This comment was marked as resolved.

PNPM_READ_CWD sits inside the installed package, so a pnpm add -g child
kept a working-directory handle in the tree pnpm must replace, blocking
removal on Windows. pnpmCommandCwd now sends mutation commands
(add/install/update/remove/uninstall) to PNPM_MUTATION_CWD while read
probes keep the isolated package-dir cwd; applied in the launcher
update callback and runOwnedPnpm.

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
devin-ai-integration[bot]

This comment was marked as resolved.

The mutation-cwd tests checked the classifier table but not the
commands the update transaction actually issues. runPnpmGlobalUpdate is
now exercised end-to-end for both the install and rollback paths with a
recording runPnpm, and every issued command must classify to its
intended cwd — reads inside the package, mutations in the neutral
directory.

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
@luvs01

luvs01 commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

이관됨: lidge-jun#6048

@luvs01

luvs01 commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

동일 수정이 상류 저장소에 제출되어 이 포크 PR의 목적은 달성됐습니다.

@luvs01 luvs01 closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant