Skip to content

Harden Node archive Starlark generation#2101

Closed
yoshihiro999 wants to merge 1 commit into
GoogleContainerTools:mainfrom
yoshihiro999:harden-node-archive-generation
Closed

Harden Node archive Starlark generation#2101
yoshihiro999 wants to merge 1 commit into
GoogleContainerTools:mainfrom
yoshihiro999:harden-node-archive-generation

Conversation

@yoshihiro999

Copy link
Copy Markdown

This hardens the Node archive updater by validating release metadata before generation and consistently quoting generated Starlark strings.

Changes:

  • Validate Node.js versions from the release index with a conservative x.y.z allowlist.
  • Validate archive suffixes, distroless architecture names, and SHA256 values before writing Starlark.
  • Quote generated Starlark string values instead of interpolating raw metadata into string literals.
  • Add a small regression test for invalid metadata values and Starlark string escaping.

Local checks run:

node knife.d/update_node_archives_test.js
node --check knife.d/update_node_archives.js
node --check knife.d/update_node_archives_test.js
bash -n knife
git diff --check

@google-cla

google-cla Bot commented Jun 2, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces input validation and safe Starlark string escaping to the Node.js archive update script (knife and knife.d/update_node_archives.js), along with a new test suite to verify these validation rules. The feedback highlights two main areas for improvement: first, ensuring that the script fails fast if any expected Node.js version/architecture checksum fails to fetch, preventing a downstream TypeError; second, refactoring the asynchronous file writing operations to use synchronous methods (fs.writeFileSync) to guarantee that write errors are properly propagated and do not cause silent failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

process.exit(1);
}
versions = process.argv[2].split(",");
await fetchChecksums();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If fetchChecksums fails to retrieve the checksum for any architecture (e.g., due to a network error or a 404 response), it logs the error but does not throw. This leaves nodeVersions[nodeVersion][key] undefined, which subsequently causes a silent TypeError when trying to access arch.suffix on line 219.

To make the script robust and fail-fast with a clear error message, we should validate that all expected version/architecture combinations were successfully fetched immediately after calling fetchChecksums().

  await fetchChecksums();
  for (const version of versions) {
    for (const arch of architectures) {
      const major = parseInt(version.split(".")[0]);
      if (major > 22 && arch === "arm") {
        continue;
      }
      if (!nodeVersions[version] || !nodeVersions[version][arch]) {
        throw new Error("Failed to fetch checksum for Node.js version " + version + " on " + arch);
      }
    }
  }

Comment on lines 285 to 287
console.error(err);
}
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The file writing operation fs.writeFile is asynchronous and its callback only logs the error without propagating it or exiting with a non-zero code. Furthermore, because main is an async function but does not await the file writing, the function resolves immediately, and the process could exit before the file is fully written or fail silently on write errors.

Since this is a build/update script, using synchronous file writing (fs.writeFileSync) is much simpler and safer as it will naturally throw errors and cause the process to exit with a non-zero code.

Consider refactoring the file writing to use fs.writeFileSync:

  fs.writeFileSync("private/extensions/node.bzl", nodeArchives);

Similarly, the test data file writing on line 245 should also be refactored to fs.writeFileSync for the same reasons.

@yoshihiro999

Copy link
Copy Markdown
Author

Superseded by #2102 after the CLA check on this PR did not update cleanly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant