Skip to content

security: require opt-in for automatic npm restore to avoid build-time script execution #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ANcpLua wants to merge 1 commit into
base: main
Choose a base branch
from
Open

security: require opt-in for automatic npm restore to avoid build-time script execution #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Build/Common/Npm.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project>

<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"/>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Allow project-level opt-in with Directory.Build.props imports

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 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep explicit /t:NpmInstall working without the new opt-in

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 👍 / 👎.

</ItemGroup>

<!-- Compute additional metadata for the NpmPackageFile items -->
Expand Down
43 changes: 37 additions & 6 deletions tests/ANcpLua.Sdk.Tests/SdkTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,8 @@ public async Task SetTargetFramework(string propName, string version)
[Fact]
public async Task NpmInstall()
{
await using var project = CreateProject(SdkWebName);
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");
Comment on lines +1005 to +1006
Copy link

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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();

Comment on lines +1005 to +1006
Copy link

@coderabbitai coderabbitai bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


project.AddFile("package.json", """
{
Expand All @@ -1029,7 +1030,8 @@ public async Task NpmInstall()
[Fact]
public async Task NpmRestore()
{
await using var project = CreateProject(SdkWebName);
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");

project.AddFile("package.json", """
{
Expand All @@ -1052,10 +1054,36 @@ public async Task NpmRestore()
}

[Fact]
public async Task Npm_Dotnet_Build_sln()
public async Task NpmRestore_DefaultAutoDetectionDisabled()
{
await using var project = CreateProject(SdkWebName);

project.AddFile("package.json", """
{
"name": "sample",
"version": "1.0.0",
"private": true,
"devDependencies": {
"is-number": "7.0.0"
}
}
""");

var result = await project
.AddSource("Program.cs", "Console.WriteLine();")
.RestoreAsync();

Assert.Equal(0, result.ExitCode);
Assert.False(File.Exists(project.RootFolder / "package-lock.json"));
Assert.False(File.Exists(project.RootFolder / "node_modules" / ".npm-install-stamp"));
}

[Fact]
public async Task Npm_Dotnet_Build_sln()
Copy link

@coderabbitai coderabbitai bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

{
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");

var slnFile = project.AddFile("sample.slnx", """
<Solution>
<Project Path="sample.csproj" />
Expand Down Expand Up @@ -1087,7 +1115,8 @@ public async Task Npm_Dotnet_Build_sln()
[InlineData("publish")]
public async Task Npm_Dotnet_sln(string command)
{
await using var project = CreateProject(SdkWebName);
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");

var slnFile = project.AddFile("sample.slnx", """
<Solution>
Expand Down Expand Up @@ -1170,7 +1199,8 @@ public async Task NpmRestore_MultipleFiles()
[Fact]
public async Task Npm_Dotnet_Build_RestoreLockedMode_Fail()
{
await using var project = CreateProject(SdkWebName);
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");

project.AddFile("package.json", """
{
Expand All @@ -1195,7 +1225,8 @@ public async Task Npm_Dotnet_Build_RestoreLockedMode_Fail()
[InlineData("/p:ContinuousIntegrationBuild=true")]
public async Task Npm_Dotnet_Build_Ci_Success(string command)
{
await using var project = CreateProject(SdkWebName);
await using var project = CreateProject(SdkWebName)
.WithProperty("EnableDefaultNpmPackageFile", "true");

project.AddFile("package.json", """
{
Expand Down
Loading