Fix: unguarded js method and trusted publishing npm#29
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdded error handling to JavaScript file system operations in the detector and report modules to gracefully handle failures. Modified CI workflows to remove push-based triggers and adjusted path filters. Updated JavaScript README to document Node version requirement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/js/src/detector.ts (1)
129-131:⚠️ Potential issue | 🟡 MinorUnguarded
fs.statSync— inconsistent with the rest of the hardening in this PR.
fs.existsSync→fs.statSyncis a TOCTOU: ifpackage.jsonis removed (or a permission changes) between the two calls,statSyncwill throw and crashdetectEnvironment— exactly the scenario the new try/catch on line 107 is meant to prevent fornodeModulesPath.🛡️ Proposed fix
- if (fs.existsSync(packageJsonPath)) { - const pkgStat = fs.statSync(packageJsonPath); - isOutdated = pkgStat.mtime.getTime() > modified.getTime(); - } + try { + const pkgStat = fs.statSync(packageJsonPath); + isOutdated = pkgStat.mtime.getTime() > modified.getTime(); + } catch { + // package.json unavailable or removed — leave isOutdated as false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js/src/detector.ts` around lines 129 - 131, The call to fs.statSync for packageJsonPath is unguarded and can throw on TOCTOU races; update detectEnvironment to wrap the stat call in a try/catch (similar to the nodeModulesPath handling) around const pkgStat = fs.statSync(packageJsonPath) and the subsequent isOutdated assignment so a thrown error is caught and handled (e.g., leave isOutdated false or log/debug and continue) rather than letting detectEnvironment crash; reference packageJsonPath, pkgStat, isOutdated, and detectEnvironment when making the change.
🧹 Nitpick comments (1)
packages/js/src/detector.ts (1)
122-123:?? nullis dead code onstat.birthtime/stat.mtime.
fs.Stats.birthtimeandfs.Stats.mtimeare alwaysDateobjects — they are neverundefinedornull, so the nullish-coalescing fallback can never trigger. On filesystems that don't track creation time, Node.js setsbirthtimeto the epoch (1970-01-01) rather thannull.♻️ Suggested cleanup
- const created = stat.birthtime ?? null; - const modified = stat.mtime ?? null; + const created = stat.birthtime; + const modified = stat.mtime;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js/src/detector.ts` around lines 122 - 123, The nullish-coalescing fallbacks are unnecessary because fs.Stats.birthtime and fs.Stats.mtime are always Date objects; update the code that assigns created and modified (the const created = stat.birthtime ?? null; and const modified = stat.mtime ?? null; in detector.ts) to directly use stat.birthtime and stat.mtime (remove the "?? null") so the values remain Date instances and avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/js/package.json`:
- Around line 35-37: The package.json currently sets "engines" -> "node":
">=20.19.0", which is a semver-breaking floor that will reject Node 18/19 users;
either revert to a wider range (e.g., >=18 || >=20.19.0 or >=18.0.0) if you
intend to keep supporting Node 18/19, or if the Node 20.19.0-only requirement is
intentional, bump the package "version" and add an explicit note in the
changelog documenting the breaking change and rationale; locate the "engines"
entry and update it, and update the package.json "version" and the project
changelog accordingly (reference keys: "engines", "node": ">=20.19.0",
"version", and the changelog file).
---
Outside diff comments:
In `@packages/js/src/detector.ts`:
- Around line 129-131: The call to fs.statSync for packageJsonPath is unguarded
and can throw on TOCTOU races; update detectEnvironment to wrap the stat call in
a try/catch (similar to the nodeModulesPath handling) around const pkgStat =
fs.statSync(packageJsonPath) and the subsequent isOutdated assignment so a
thrown error is caught and handled (e.g., leave isOutdated false or log/debug
and continue) rather than letting detectEnvironment crash; reference
packageJsonPath, pkgStat, isOutdated, and detectEnvironment when making the
change.
---
Nitpick comments:
In `@packages/js/src/detector.ts`:
- Around line 122-123: The nullish-coalescing fallbacks are unnecessary because
fs.Stats.birthtime and fs.Stats.mtime are always Date objects; update the code
that assigns created and modified (the const created = stat.birthtime ?? null;
and const modified = stat.mtime ?? null; in detector.ts) to directly use
stat.birthtime and stat.mtime (remove the "?? null") so the values remain Date
instances and avoid dead code.
1 participant
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes