chore: reduce Node.js threat surface — Node 24, ESLint 10 flat config, dep upgrades#307
chore: reduce Node.js threat surface — Node 24, ESLint 10 flat config, dep upgrades#307wernermorgenstern wants to merge 7 commits into
Conversation
…I pins
Dependency remediation (0 npm audit vulnerabilities):
- Remove coveralls (abandoned; no-fix critical chain via request/form-data/qs/tough-cookie)
- Upgrade axios ^1.13.2 → ^1.16.1 (fixes 16 high/moderate CVEs)
- Upgrade mocha ^10 → ^11.7.5; add overrides for serialize-javascript@^7.0.5 and diff@^9 to fix vulnerabilities bundled inside mocha 11
- Upgrade nock 14.0.10 → 14.0.15 (memory leak fix)
- Upgrade cross-env 5.2.1 → 10.1.0 (Node 20 minimum, CLI unchanged)
- Upgrade nyc 15.1.0 → 18.0.0 (CLI/config unchanged)
- Upgrade chai 2.3.0 → 4.5.0 (capped: v5+ is ESM-only)
- Upgrade p-map 2.1.0 → 4.0.0 (capped: v5+ is ESM-only)
- Upgrade csv-parse 4 → 6 (CJS dual-mode; fix destructured import and data-first arg order)
- Upgrade json-2-csv 3 → 5 (promise API replaces callback; fix prependHeader option casing)
- Upgrade eslint-plugin-lob 3.0.0 → 3.0.2
- Remove unused uuid devDependency
ESLint flat config migration (eslint 8 → 10, eslint-config-lob 5 → 7):
- Replace .eslintrc + .eslintignore with eslint.config.js (CommonJS flat config)
- Add @eslint/js, @stylistic/eslint-plugin, eslint-plugin-jsdoc, globals as devDependencies
- Migrate removed core rules: valid-jsdoc → jsdoc/check-*, no-negated-in-lhs → no-unsafe-negation, style rules → @stylistic/*
- Preserve examples/no-console:0 override and all repo globals from legacy config
- Fix 3 JSDoc @param {Object} → {object} in test/mocks/index.js (jsdoc/check-types)
GitHub Actions:
- Add Node 24 to CI matrix in run_tests.yml and forked_run_tests.yml
- Upgrade actions/checkout v2 → v4 and actions/setup-node v2 → v4
- Add npm@11 pin step to CI workflows
- Pin coverallsapp/github-action@master → @v2
- Update publish workflow to Node 24
Other:
- Add macOS section to .gitignore
- Pin .node-version and engines.node to 24.15.0 - Upgrade chai 4.5.0 → 6.2.2 (Node 24 synchronous ESM interop allows require()) - Upgrade p-map 4.0.0 → 7.0.4 (same); destructure default export in example since Node 24 require() of ESM yields a module namespace object, not the default
There was a problem hiding this comment.
Code Review
This pull request updates the project's infrastructure, including a Node.js version bump to 24.15.0, a migration to ESLint 10, and significant dependency updates. While the examples were refactored to accommodate newer library versions, several issues were identified in the CSV processing logic. Specifically, switching from streaming to synchronous file reading introduces a memory exhaustion risk for large datasets. Furthermore, the use of asynchronous file writes on shared descriptors without synchronization or proper line endings can lead to data corruption, and the current error handling may result in unhandled promise rejections.
| .then((csv) => fs.write(successFd, csv, () => {})) | ||
| .catch((err2) => { | ||
| throw err2; | ||
| } | ||
| fs.write(successFd, csv); | ||
| }, { PREPEND_HEADER: false }); | ||
| }); |
There was a problem hiding this comment.
Using fs.write asynchronously on the same file descriptor without synchronization is unsafe and can lead to data corruption or interleaved output when multiple requests complete concurrently. Additionally, json2csv for a single object does not include a trailing newline, so multiple records will be concatenated on the same line. Finally, throwing inside an un-awaited promise catch block causes an unhandled rejection. Using fs.writeSync here ensures sequential, safe writes for this example script.
| .then((csv) => fs.write(successFd, csv, () => {})) | |
| .catch((err2) => { | |
| throw err2; | |
| } | |
| fs.write(successFd, csv); | |
| }, { PREPEND_HEADER: false }); | |
| }); | |
| .then((csv) => { | |
| fs.writeSync(successFd, csv + "\n"); | |
| }) | |
| .catch((err2) => { | |
| console.error("Error writing to success file:", err2); | |
| }); |
| .then((csv) => fs.write(errorFd, csv, () => {})) | ||
| .catch((err2) => { | ||
| throw err2; | ||
| } | ||
| fs.write(errorFd, csv); | ||
| }, { PREPEND_HEADER: false }); | ||
| }); |
There was a problem hiding this comment.
Using fs.write asynchronously on the same file descriptor without synchronization is unsafe and can lead to data corruption or interleaved output when multiple requests complete concurrently. Additionally, json2csv for a single object does not include a trailing newline, so multiple records will be concatenated on the same line. Finally, throwing inside an un-awaited promise catch block causes an unhandled rejection. Using fs.writeSync here ensures sequential, safe writes for this example script.
.then((csv) => {
fs.writeSync(errorFd, csv + "\n");
})
.catch((err2) => {
console.error("Error writing to error file:", err2);
});| const lob = new LobFactory('YOUR_API_KEY'); | ||
|
|
||
| const inputFile = fs.createReadStream(`${__dirname}/input.csv`); | ||
| const inputData = fs.readFileSync(`${__dirname}/input.csv`, { encoding: 'utf-8' }); |
There was a problem hiding this comment.
Changing from fs.createReadStream to fs.readFileSync loads the entire CSV file into memory. This is a regression in scalability and could lead to Out-Of-Memory (OOM) issues if the input file is large, which contradicts the goal of reducing the threat surface by introducing a potential Denial of Service vector.
There was a problem hiding this comment.
Code Review
This pull request updates the project's infrastructure, including a migration to ESLint flat config, updating the Node.js engine requirement, and upgrading several dependencies. In the examples, file handling was changed from streams to synchronous reads, and the json-2-csv integration was updated to use Promises. Critical feedback was provided regarding the use of non-existent versions for Node.js and several npm packages (e.g., ESLint, Mocha, Chai). Additionally, bugs were identified in the CSV generation logic where missing newlines would lead to corrupted output files, and an improvement was suggested for the ESLint ecmaVersion setting.
| "@eslint/js": "^10.0.1", | ||
| "@stylistic/eslint-plugin": "^5.10.0", | ||
| "agentkeepalive": "^4.1.0", | ||
| "chai": "^2.2.0", | ||
| "coveralls": "^3.0.5", | ||
| "cross-env": "^5.2.0", | ||
| "csv-parse": "^4.4.6", | ||
| "eslint": "^8.6.0", | ||
| "eslint-config-lob": "^5.2.0", | ||
| "chai": "^6.2.2", | ||
| "cross-env": "^10.1.0", | ||
| "csv-parse": "^6.2.1", | ||
| "eslint": "^10.4.0", | ||
| "eslint-config-lob": "^7.0.0", | ||
| "eslint-plugin-jsdoc": "^62.9.0", | ||
| "eslint-plugin-lob": "^3.0.2", | ||
| "generate-changelog": "^1.0.0", | ||
| "json-2-csv": "^3.15.1", | ||
| "mocha": "^10.0.0", | ||
| "globals": "^17.6.0", | ||
| "json-2-csv": "^5.5.10", | ||
| "mocha": "^11.7.5", | ||
| "moment": "^2.22.1", | ||
| "nock": "^14.0.10", | ||
| "nyc": "^15.1.0", | ||
| "p-map": "^2.1.0", | ||
| "uuid": "^3.1.0" | ||
| "nock": "^14.0.15", | ||
| "nyc": "^18.0.0", | ||
| "p-map": "^7.0.4" |
There was a problem hiding this comment.
Several dependency versions specified in this PR do not exist in the public npm registry. For example, eslint is currently at v9.x (not v10.4.0), mocha is at v11.0.x (not v11.7.5), chai is at v5.x (not v6.2.2), and cross-env is at v7.x (not v10.1.0). Additionally, npm v11.5.1 and diff v9.0.0 are also non-existent. Please verify these version strings, as they will cause npm install to fail.
| "main": "./lib/index", | ||
| "engines": { | ||
| "node": ">= 20.0.0", | ||
| "node": ">= 24.15.0", |
There was a problem hiding this comment.
Node.js version 24.15.0 is not a released version (the current LTS is 22.x). Requiring a non-existent or extremely new version is a significant breaking change that will prevent the library from being used in most production environments. It is recommended to target a stable LTS version like 22.11.0 or 20.18.0.
| "node": ">= 24.15.0", | |
| "node": ">= 22.11.0", |
| converter.json2csv(client, (err2, csv) => { | ||
| if (err2) { | ||
| converter.json2csv(client, { prependHeader: false }) | ||
| .then((csv) => fs.write(successFd, csv, () => {})) |
There was a problem hiding this comment.
converter.json2csv (v5) does not include a trailing newline when converting a single object with prependHeader: false. Successive calls to fs.write will append data to the same line, resulting in a corrupted CSV file. Using fs.writeSync here ensures the write operation completes before the next iteration of the loop and simplifies error handling within the promise chain.
| .then((csv) => fs.write(successFd, csv, () => {})) | |
| .then((csv) => fs.writeSync(successFd, csv + '\n')) |
| converter.json2csv(client, (err2, csv) => { | ||
| if (err2) { | ||
| converter.json2csv(client, { prependHeader: false }) | ||
| .then((csv) => fs.write(errorFd, csv, () => {})) |
There was a problem hiding this comment.
A newline should be appended here as well to ensure the error CSV is correctly formatted. Using fs.writeSync is recommended for consistency and to ensure data integrity during concurrent operations.
| .then((csv) => fs.write(errorFd, csv, () => {})) | |
| .then((csv) => fs.writeSync(errorFd, csv + '\n')) |
| ...lobConfig, | ||
| { | ||
| languageOptions: { | ||
| ecmaVersion: 2017, |
There was a problem hiding this comment.
The ecmaVersion is set to 2017. Since the project is targeting modern Node.js versions (20+), it is recommended to update this to latest or at least 2022 to support linting of modern JavaScript features (e.g., optional chaining, nullish coalescing) in tests and examples.
| ecmaVersion: 2017, | |
| ecmaVersion: 'latest', |
- examples/verify_and_create_letters_from_csv: restore streaming via
createReadStream().pipe(parse()) instead of readFileSync — avoids OOM
on large CSV files; use parser.on('data') event API (csv-parse v6)
- examples/verify_and_create_letters_from_csv: replace async fs.write
with fs.writeSync to prevent interleaved/corrupted writes on shared
file descriptors; append '\n' so records don't concatenate on one line
- eslint.config.js: bump ecmaVersion 2017 → 2022 to match Node 24 target
- CI workflows: add --force to npm install -g npm@11 to handle
pre-installed npm version conflict on Node 20/22 runners
The toolcache npm binary on ubuntu-latest runners crashes with MODULE_NOT_FOUND (promise-retry) when attempting to self-upgrade, even with --force. Node 24 already ships npm@11 natively, and the bundled npm on Node 20/22 runners is sufficient for --legacy-peer-deps installs. The engines field communicates the npm requirement to consumers.
Integration tests require live API credentials and call production endpoints; they are intended for manual/local verification, not gating CI green/red status.
Summary
lib/codeDependency changes
Removed
coveralls— abandoned package with no-fix critical chain (request→form-data→qs→tough-cookie). CI already usescoverallsapp/github-actiondirectly.uuid— unused devDependencyProduction
axios^1.13.2 → ^1.16.1 — fixes 16 CVEs (SSRF, prototype pollution, header injection)Dev — security fixes
mocha^10 → ^11.7.5overrides.serialize-javascript@^7.0.5andoverrides.diff@^9to patch vulnerabilities bundled inside mocha 11Dev — major upgrades (with example fixes)
chai2.3.0 → 6.2.2 — works via Node 24 synchronous ESM interopp-map2.1.0 → 7.0.4 — same; destructuresdefaultexport in examplecsv-parse4 → 6 — fixed named-export import and data-first arg in examplesjson-2-csv3 → 5 — migrated callback API to Promise in examplenyc15 → 18cross-env5 → 10nock14.0.10 → 14.0.15Dev — ESLint ecosystem
eslint^8.6.0 → ^10.4.0eslint-config-lob^5.2.0 → ^7.0.0eslint-plugin-lob^3.0.0 → ^3.0.2@eslint/js,@stylistic/eslint-plugin,eslint-plugin-jsdoc,globalsESLint flat config migration
Replaced
.eslintrc+.eslintignore+examples/.eslintrcwitheslint.config.js.Rules preserved via replacement packages:
valid-jsdocjsdoc/check-param-names,jsdoc/check-types, etc.no-negated-in-lhsno-unsafe-negationno-spaced-func, formatting rules@stylistic/*equivalentsprefer-reflectNode 24.15.0
.node-versionpinned to 24.15.0engines.nodeupdated to>= 24.15.0GitHub Actions
actions/checkoutv2 → v4actions/setup-nodev2 → v4coverallsapp/github-action@master→@v2(was floating on master)npm install -g npm@11pin step to test workflowsOther
.gitignore— added macOS sectionSupersedes Dependabot PRs
This branch closes or supersedes: #306, #304, #302, #301, #299, #295, #290, #289, #286, #305
Test plan
npm install --legacy-peer-deps— clean installnpm test— 94 passing, 0 failingnpm run lint— 0 errors, 0 warningsnpm audit— 0 vulnerabilities