Skip to content

fix(server): unset the allow-scripts policy npx exports before installing the pinned runtime - #9403

Closed
ylcn91 wants to merge 1 commit into
pingdotgg:mainfrom
ylcn91:fix/pinned-runtime-strip-npx-allow-scripts
Closed

fix(server): unset the allow-scripts policy npx exports before installing the pinned runtime#9403
ylcn91 wants to merge 1 commit into
pingdotgg:mainfrom
ylcn91:fix/pinned-runtime-strip-npx-allow-scripts

Conversation

@ylcn91

@ylcn91 ylcn91 commented Sep 3, 2026

Copy link
Copy Markdown

What Changed

apps/server/src/cloud/pinnedRuntime.ts: the npm install --prefix <staging> child that installs the pinned runtime now runs with npm_config_allow_scripts unset. One env entry plus a comment.

apps/server/src/cloud/pinnedRuntime.test.ts: a focused test that the install runner receives the variable overridden to undefined. It fails without the fix.

Why

Fixes #9398.

npx exports its resolved allow-scripts config to child processes as npm_config_allow_scripts. The CLI inherits it and hands it to the pinned-runtime npm install. npm 12 treats an env-sourced policy like a --allow-scripts flag and refuses project-scoped installs with EALLOWSCRIPTS, so npx t3 service install and npx t3 service update fail whenever ~/.npmrc holds an allow-scripts entry. The reporter confirmed the same CLI succeeds once the variable is unset.

The runner extends the host environment and Node drops undefined entries, so the override removes the variable for the child only. The user-level .npmrc policy still applies to the install, which is the layer that should grant native builds.

This is scoped to the one variable that is proven to break the install. It is independent of #9380, which grants the native builds through the staged manifest; both can land.

Verification:

vp test run apps/server/src/cloud/pinnedRuntime.test.ts   # 6 passed

Server typecheck and lint are clean.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (no UI changes)
  • I included a video for animation/interaction changes (not applicable)

Model and harness: Claude Fable 5.1 in Claude Code.


Note

Low Risk
Narrow env override on one subprocess during pinned-runtime install; behavior is covered by a unit test and targets a known npm 12 / npx interaction.

Overview
Fixes pinned runtime installation failing when the CLI is started via npx and the host has an allow-scripts policy in ~/.npmrc (npm 12 treats npx-exported npm_config_allow_scripts like a project-scoped --allow-scripts and aborts with EALLOWSCRIPTS).

The staged npm install --prefix … for the pinned t3@<version> runtime now passes env: { npm_config_allow_scripts: undefined } so the child process does not inherit npx’s resolved policy; the process runner still merges the host environment and Node omits undefined keys, so user-level .npmrc rules remain in effect for that install.

A new pinnedRuntime.test.ts case asserts the install runner receives npm_config_allow_scripts as an own property set to undefined.

Reviewed by Cursor Bugbot for commit 70662af. Configure here.

Note

Unset npm_config_allow_scripts in installPinnedRuntime child-process env

The pinned runtime install was inheriting the npx allow-scripts policy, which npm 12 could interpret as a project-scoped install option. The fix sets npm_config_allow_scripts to undefined at the child-process environment boundary so the install does not inherit it. Adds a test in pinnedRuntime.test.ts asserting the install runner receives the undefined override.

Macroscope summarized 70662af.

Summary by CodeRabbit

  • Bug Fixes
    • Improved runtime installation reliability by preventing inherited npm script-approval settings from affecting installation processes.
    • Preserved unrelated npm configuration, such as custom registry settings, during installation.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 3, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 70662af

Macroscope's review found this PR approvable — This is a narrowly scoped fix to the pinned-runtime installation subprocess, removing only the npx-exported environment variable that causes npm 12 installation failures. Existing service behavior and unrelated environment variables remain unchanged, and the override is covered by a focused test.

You can add or adjust custom eligibility rules. Learn more.

@shivamhwp

Copy link
Copy Markdown
Collaborator

Note: GPT-6 on behalf of shivam (@shivamhwp).

The lowercase override removes that variable, but uppercase NPM_CONFIG_ALLOW_SCRIPTS remains inherited. npm treats config environment names case-insensitively, so the uppercase case can still produce EALLOWSCRIPTS. Remove the inherited allow-scripts keys regardless of casing while preserving unrelated npm config.

Please cover the child process environment at the actual ProcessRunner boundary, including the uppercase case. The current test only checks the input object passed to a mocked runner.

…ling the pinned runtime

npx exports its resolved allow-scripts config to child processes as
npm_config_allow_scripts. The CLI inherits it and passes it on to the
`npm install --prefix <staging>` child that installs the pinned runtime.
npm 12 treats an env-sourced policy like a --allow-scripts flag and
refuses project-scoped installs with EALLOWSCRIPTS, so `npx t3 service
install` and `npx t3 service update` fail whenever the user's ~/.npmrc
holds an allow-scripts entry.

Unset every inherited spelling of the variable on the install runner.
npm matches config variable names case-insensitively, so an uppercase
NPM_CONFIG_ALLOW_SCRIPTS trips the same check; the runner extends the
host env and Node drops undefined entries, so the child npm sees none
of them while the rest of its configuration is untouched. The
user-level .npmrc policy still applies.

The test spawns a real child through the ProcessRunner with the exact
environment the installer passes and asserts on what that child sees.

Fixes pingdotgg#9398
@ylcn91

ylcn91 commented Sep 11, 2026

Copy link
Copy Markdown
Author

Right, npm reads config variable names case-insensitively, so the lowercase override alone still let NPM_CONFIG_ALLOW_SCRIPTS through. 146b851 enumerates the inherited environment and unsets every key matching npm_config_allow[-_]scripts regardless of case, on both the npm run and the pnpm dlx fallback; other npm config is untouched.

The test now goes through the real ProcessRunner and NodeChildProcessSpawner: it spawns a child with the exact environment the installer hands npm and asserts on what that child sees, with both spellings set and npm_config_registry preserved. It fails against main.

@ylcn91
ylcn91 force-pushed the fix/pinned-runtime-strip-npx-allow-scripts branch from 70662af to 146b851 Compare September 11, 2026 04:56
@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:XS 0-9 changed lines (additions + deletions). labels Sep 11, 2026
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: cd4e61c5-d778-4e70-8c89-98c962aa9fb8

📥 Commits

Reviewing files that changed from the base of the PR and between 57aee3e and 146b851.

📒 Files selected for processing (2)
  • apps/server/src/cloud/pinnedRuntime.test.ts
  • apps/server/src/cloud/pinnedRuntime.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The pinned runtime installer removes inherited npm allow-scripts environment variables from direct and fallback installations. A test captures the child environment and confirms both spellings are removed while the npm registry setting remains.

Changes

Pinned runtime environment

Layer / File(s) Summary
Sanitize install environment
apps/server/src/cloud/pinnedRuntime.ts, apps/server/src/cloud/pinnedRuntime.test.ts
The installer removes case-insensitive npm_config_allow_scripts variants before direct npm and pnpm-based fallback installations. The test verifies removal and preservation of npm_config_registry.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 146b8

Pinned runtime installation now avoids the inherited npm policy conflict without removing unrelated user configuration. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary change: removing the inherited npx allow-scripts policy before installing the pinned runtime.
Description check ✅ Passed The description includes the required What Changed, Why, and Checklist sections. It explains the npm 12 failure, the scoped environment change, testing, and the absence of UI changes. The description …
Linked Issues check ✅ Passed PR #9403 satisfies the core coding objective in issue #9398. pinnedRuntime.ts removes inherited npm_config_allow-scripts and npm_config_allow_scripts keys without regard to case, for both npm an…
Out of Scope Changes check ✅ Passed The changed files contain only the pinned-runtime environment sanitization and its focused test. Both changes directly support issue #9398. No unrelated behavior or file changes are identified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

⚠️ This pull request shows signs of AI-generated slop (ai_padded_prose). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.


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

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #11510 (and the SEA/archive stack ending in #11659).

Pinned runtimes are no longer installed via npm install under npx, so unsetting npm_config_allow_scripts for that child is obsolete. Closing in favor of main; leftover #9398 closed as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npx t3 service update fails with EALLOWSCRIPTS because npx exports npm_config_allow_scripts to the pinned runtime install

3 participants