security: require opt-in for automatic npm restore to avoid build-time script execution#54
security: require opt-in for automatic npm restore to avoid build-time script execution#54
Conversation
Summary by CodeRabbit
WalkthroughThe changes modify npm package file auto-detection in MSBuild by requiring an explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Comment Tip Migrating from UI to YAML configuration.Use the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the SDK by changing the default behavior of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a security concern by making automatic npm restore an opt-in feature. The change in Npm.targets is correct, and the updates to existing tests to opt-in are necessary. The addition of the NpmRestore_DefaultAutoDetectionDisabled test is excellent as it verifies the new secure-by-default behavior. I've included one suggestion to improve the maintainability of the test suite by reducing code duplication.
| await using var project = CreateProject(SdkWebName) | ||
| .WithProperty("EnableDefaultNpmPackageFile", "true"); |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, consider creating a helper method for setting up projects that require npm. This pattern of CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true") is repeated across multiple tests (NpmRestore, Npm_Dotnet_Build_sln, Npm_Dotnet_sln, Npm_Dotnet_Build_RestoreLockedMode_Fail, and Npm_Dotnet_Build_Ci_Success).
You could add a private helper method to the SdkTests class like this:
private SdkProjectBuilder CreateProjectWithNpmEnabled() =>
CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");Then, you can simplify the test setup.
await using var project = CreateProjectWithNpmEnabled();
There was a problem hiding this comment.
Pull request overview
This PR hardens the SDK’s npm restore behavior by disabling implicit package.json auto-detection, requiring an explicit opt-in to avoid unintended npm lifecycle script execution during build/restore.
Changes:
- Require
EnableDefaultNpmPackageFile=truefor the SDK to auto-include$(MSBuildProjectDirectory)/package.jsonas anNpmPackageFile. - Update npm-related integration tests to explicitly opt in where automatic npm behavior is expected.
- Add a regression test ensuring the secure default: no npm restore side-effects occur when the property is not set.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/ANcpLua.Sdk.Tests/SdkTests.cs | Opt-in existing npm tests via EnableDefaultNpmPackageFile=true and add a regression test for the secure default behavior. |
| src/Build/Common/Npm.targets | Gate default package.json detection behind EnableDefaultNpmPackageFile == 'true'. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: a83627b9da
ℹ️ 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".
|
|
||
| <ItemGroup> | ||
| <NpmPackageFile Condition="'$(EnableDefaultNpmPackageFile)' == '' AND Exists('$(MSBuildProjectDirectory)/package.json')" Include="$(MSBuildProjectDirectory)/package.json"/> | ||
| <NpmPackageFile Condition="'$(EnableDefaultNpmPackageFile)' == 'true' AND Exists('$(MSBuildProjectDirectory)/package.json')" Include="$(MSBuildProjectDirectory)/package.json"/> |
There was a problem hiding this comment.
This condition is evaluated when Npm.targets is imported, so the new opt-in only works if EnableDefaultNpmPackageFile is already set before that import. In the repo's supported SdkElementDirectoryBuildProps mode, the SDK is injected from Directory.Build.props (tests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cs:329-346) and project properties are emitted later in the generated .csproj (:357-382), so .WithProperty("EnableDefaultNpmPackageFile", "true") never repopulates @(NpmPackageFile). That leaves the Sdk100DirectoryBuildPropsTests npm cases still broken and means real projects using the Directory.Build.props import style cannot re-enable the default package.json behavior from their project file.
Useful? React with 👍 / 👎.
|
|
||
| <ItemGroup> | ||
| <NpmPackageFile Condition="'$(EnableDefaultNpmPackageFile)' == '' AND Exists('$(MSBuildProjectDirectory)/package.json')" Include="$(MSBuildProjectDirectory)/package.json"/> | ||
| <NpmPackageFile Condition="'$(EnableDefaultNpmPackageFile)' == 'true' AND Exists('$(MSBuildProjectDirectory)/package.json')" Include="$(MSBuildProjectDirectory)/package.json"/> |
There was a problem hiding this comment.
Gating the default package.json item here also empties @(NpmPackageFile) for the manual NpmInstall target (src/Build/Common/Npm.targets:43-45). Any existing CI or local workflow that explicitly runs dotnet msbuild /t:NpmInstall in a web project will no longer see the repo's package.json, so npm dependencies stop being installed unless the caller also knows to set EnableDefaultNpmPackageFile; the security concern in this change is about automatic restore/build entry points, not an explicitly requested npm install.
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ANcpLua.Sdk.Tests/SdkTests.cs`:
- Around line 1005-1006: Extract the repeated project setup into a small helper
method (e.g., CreateProjectWithDefaultNpmOptIn) that wraps
CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true");
update all callers (tests currently calling
CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true") —
references around SdkTests.cs lines near the CreateProject usage such as the
blocks using CreateProject and SdkWebName) to call the new helper instead; keep
the helper signature similar to CreateProject so it can be used in places that
currently chain additional calls, and run tests to ensure no behavior changed.
- Line 1082: The test method Npm_Dotnet_Build_sln does not follow the
MethodName_Scenario_ExpectedResult convention; rename the method to a
descriptive name that follows that pattern (for example,
BuildSolution_WithNpmAndDotnet_ReturnsSuccess or
BuildSolution_WhenUsingNpmAndDotnet_Succeeds) so the test method (originally
Npm_Dotnet_Build_sln) clearly describes the method under test, the scenario, and
the expected outcome.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a7d6e8d6-e5b4-4ad4-9fa8-e2b4af00d0de
📒 Files selected for processing (2)
src/Build/Common/Npm.targetstests/ANcpLua.Sdk.Tests/SdkTests.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.cs
⚙️ CodeRabbit configuration file
xUnit.v3 test projects using xunit.v3.mtp-v2 with AwesomeAssertions and NSubstitute. Follow Arrange-Act-Assert pattern. Use descriptive test names: MethodName_Scenario_ExpectedResult. Test async methods with async Task, not async void. Flag hardcoded test data that should come from the seeded DuckDB file (the single source of truth for demo/test data). Prefer [Theory] with [InlineData] or [MemberData] over duplicated [Fact] methods testing variations of the same behavior. Flag tests that depend on test execution order.
Files:
tests/ANcpLua.Sdk.Tests/SdkTests.cs
🔇 Additional comments (2)
src/Build/Common/Npm.targets (1)
4-4: Opt-in gate is correctly enforced for defaultpackage.jsondiscovery.Line 4 now requires explicit
EnableDefaultNpmPackageFile=truebefore default npm restore wiring is activated, which closes the implicit restore/script-execution path.tests/ANcpLua.Sdk.Tests/SdkTests.cs (1)
1057-1079: Regression test covers the secure default behavior correctly.This test validates the intended default: restore succeeds without generating
package-lock.jsonor.npm-install-stampwhen opt-in is not set.
| await using var project = CreateProject(SdkWebName) | ||
| .WithProperty("EnableDefaultNpmPackageFile", "true"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract repeated npm opt-in setup into a helper to avoid drift.
The same CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true") chain is repeated across tests; centralize it in one helper.
Refactor sketch
+ private SdkProjectBuilder CreateWebProjectWithDefaultNpmEnabled() =>
+ CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true");
- await using var project = CreateProject(SdkWebName)
- .WithProperty("EnableDefaultNpmPackageFile", "true");
+ await using var project = CreateWebProjectWithDefaultNpmEnabled();Also applies to: 1033-1034, 1084-1085, 1118-1119, 1202-1203, 1228-1229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ANcpLua.Sdk.Tests/SdkTests.cs` around lines 1005 - 1006, Extract the
repeated project setup into a small helper method (e.g.,
CreateProjectWithDefaultNpmOptIn) that wraps
CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true");
update all callers (tests currently calling
CreateProject(SdkWebName).WithProperty("EnableDefaultNpmPackageFile", "true") —
references around SdkTests.cs lines near the CreateProject usage such as the
blocks using CreateProject and SdkWebName) to call the new helper instead; keep
the helper signature similar to CreateProject so it can be used in places that
currently chain additional calls, and run tests to ensure no behavior changed.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Npm_Dotnet_Build_sln() |
There was a problem hiding this comment.
Rename this test to the required scenario/expected naming format.
Line 1082 (Npm_Dotnet_Build_sln) is not in MethodName_Scenario_ExpectedResult format.
Suggested rename
- public async Task Npm_Dotnet_Build_sln()
+ public async Task NpmRestore_DotnetBuildSln_WithDefaultNpmPackageFileEnabled_CreatesNpmArtifacts()As per coding guidelines, "Use descriptive test names: MethodName_Scenario_ExpectedResult."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task Npm_Dotnet_Build_sln() | |
| public async Task NpmRestore_DotnetBuildSln_WithDefaultNpmPackageFileEnabled_CreatesNpmArtifacts() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ANcpLua.Sdk.Tests/SdkTests.cs` at line 1082, The test method
Npm_Dotnet_Build_sln does not follow the MethodName_Scenario_ExpectedResult
convention; rename the method to a descriptive name that follows that pattern
(for example, BuildSolution_WithNpmAndDotnet_ReturnsSuccess or
BuildSolution_WhenUsingNpmAndDotnet_Succeeds) so the test method (originally
Npm_Dotnet_Build_sln) clearly describes the method under test, the scenario, and
the expected outcome.
Motivation
package.jsonand rannpm install/npm ciduringRestore, which executes npm lifecycle scripts and creates a build-time RCE risk.package.jsonfile.Description
src/Build/Common/Npm.targetsso the repositorypackage.jsonis only included whenEnableDefaultNpmPackageFileis explicitly set totrueinstead of being picked up by default.tests/ANcpLua.Sdk.Tests/SdkTests.csto opt-in by calling.WithProperty("EnableDefaultNpmPackageFile", "true")where the tests require automatic npm behavior.NpmRestore_DefaultAutoDetectionDisabledthat verifies the secure default (nopackage-lock.jsonand no npm stamp file created) whenEnableDefaultNpmPackageFileis not set.Testing
dotnet testinvocation was attempted:dotnet test tests/ANcpLua.Sdk.Tests/ANcpLua.Sdk.Tests.csproj --filter "NpmInstall|NpmRestore|Npm_Dotnet_Build_sln|Npm_Dotnet_sln|NpmRestore_MultipleFiles|Npm_Dotnet_Build_RestoreLockedMode_Fail|Npm_Dotnet_Build_Ci_Success", but it failed in this environment becausedotnetis not installed.git diffand inspecting the modified files to ensure the condition and test updates are correct.fix: require opt-in for automatic npm restore) to the branch so CI can run the updated test suite in an environment withdotnetavailable.Codex Task