Harden Node archive Starlark generation#2101
Conversation
|
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. |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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);
}
}
}| console.error(err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
|
Superseded by #2102 after the CLA check on this PR did not update cleanly. |
This hardens the Node archive updater by validating release metadata before generation and consistently quoting generated Starlark strings.
Changes:
x.y.zallowlist.Local checks run: