fix: security and reliability improvements - #322
Conversation
terminate() function
use python3 binary on unix systems
…ssary code in run
Update example in README.md
update node&python version
No point in -u here because we are getting the print results at the end of the program
Remove bad -u
Removes [hawk](https://github.com/mozilla/hawk). It's no longer used after updating ancestor dependency [request-json](https://github.com/hackervents/request-json). These dependencies need to be updated together. Removes `hawk` Updates `request-json` from 0.6.3 to 0.6.5 - [Release notes](https://github.com/hackervents/request-json/releases) - [Commits](https://github.com/hackervents/request-json/commits) --- updated-dependencies: - dependency-name: hawk dependency-type: indirect - dependency-name: request-json dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…yarn/hawk-and-request-json--removed Bump hawk and request-json
feat: add format
already the default
…e-22-test ci(appveyor): add Node.js 22 to test matrix
- Fix runString() using tmpdir reference instead of tmpdir() call (fixes extrabacon#320) - Replace exec() with execFile() to prevent command injection in checkSyntaxFile, getVersion, and getVersionSync - Add temp file cleanup in runString() and checkSyntax() via .finally() - Replace custom extend() with Object.assign - Re-enable getVersion/getVersionSync tests (were disabled since extrabacon#158) - Add GitHub Actions CI matrix (Node 18/20/22, Python 3.10/3.12, 3 OSes) - Update minimum Node.js engine from 0.10 to 16
f206f6c to
2feabe6
Compare
|
I took a triage pass on this because it is tied to #290. I don't think this is reviewable as one PR in its current shape: GitHub reports it as conflicting, and the diff combines several unrelated tracks: issue templates, GitHub Actions, AppVeyor, TypeScript conversion ( The pieces that look easiest to make reviewable are the security/bug fixes split one at a time with focused tests:
The |
PNHD
left a comment
There was a problem hiding this comment.
One additional correctness/concurrency blocker beyond the existing scope/conflict triage: this branch replaces the existing random temp-file suffix with Date.now() in both checkSyntax() and runString().
Two calls started within the same millisecond can therefore select the same path. For runString(), the second writeFileSync() can overwrite the first call's script before Python has consumed it; with the new .finally() cleanup, either call can also unlink the shared path while the other still needs it. checkSyntax() has the same collision class.
Please keep a collision-resistant unique suffix (the focused #330 fix preserves the existing random suffix), or use a proper unique-temp-file primitive, and add a concurrent-call regression before merging this part.
Summary
I'm interested in helping maintain this project (ref #290). This PR demonstrates the kind of improvements I'd bring:
runString()temp path bug —tmpdirwas used as a reference instead oftmpdir(), writing temp files to the wrong location (fixes the issue in Fix runString temp path to use tmpdir() and add regression test #320)checkSyntaxFile,getVersion, andgetVersionSyncusedexec()with string interpolation, which is unsafe. Replaced withexecFile()which passes args as an arrayrunString()andcheckSyntax()now clean up their temp.pyfiles via.finally()extend()helper withObject.assign, updated minimum Node.js from 0.10 to 16All 45 tests passing (43 existing + 2 re-enabled).
Test plan