Bump express from 4.21.2 to 5.1.0 #9188
Conversation
WalkthroughUpdated Express from 4.21.2 to 5.1.0 in package.json. Test server replaced body-parser with Express built-in parsers and now explicitly ends POST responses. An integration test mock handler uses Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TestServer as Test Server\n(src/test/framework/server.js)
participant MetricsMock as Metrics Mock\n(test_cli_diagnose)
rect rgb(240,248,255)
Note left of Client: POST request (test payload)
Client-->>TestServer: POST / (JSON body)
activate TestServer
TestServer->>TestServer: express.json() parses body
TestServer->>TestServer: handler logs req.body
TestServer-->>Client: res.end() (empty response)
deactivate TestServer
end
rect rgb(245,255,240)
Note left of Client: GET /metrics (test)
Client-->>MetricsMock: GET /metrics
activate MetricsMock
MetricsMock->>MetricsMock: prepare metrics_obj_mock
MetricsMock-->>Client: res.json(metrics_obj_mock)
deactivate MetricsMock
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential focus areas:
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
liranmauda
force-pushed
the
liran-bump-express
branch
from
7bd9d6c to
f1f9c5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/server/web_server.js (2)
164-164: Minor nit: prefer redirect(status, url) or set trust proxyCurrent usage is fine, but consider one of:
- Use res.redirect(302, url) directly for clarity.
- If behind proxies, app.set('trust proxy', true) and rely on req.secure or req.protocol rather than inspecting X-Forwarded-Proto manually.
- return res.status(302).redirect('https://' + host + req.originalUrl); + return res.redirect(302, 'https://' + host + req.originalUrl);
328-336: Error handler tidy-up looks good; minor hardening suggestionRefactor is solid. Consider ensuring e.message is a string (avoid non-string payloads leaking) and avoid echoing raw messages in HTML in prod if they can be influenced by user input.
- <h2>${e.message}</h2> + <h2>${String(e.message || 'Unexpected Error')}</h2>Also applies to: 340-340
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/server/web_server.js(8 hunks)src/test/framework/server.js(0 hunks)src/test/integration_tests/nc/cli/test_cli_diagnose.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/framework/server.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit Configuration File
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/nc/cli/test_cli_diagnose.test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (7)
src/test/integration_tests/nc/cli/test_cli_diagnose.test.js (1)
93-93: Good switch to res.json()Using res.json(metrics_obj_mock) is the idiomatic and safer choice (sets headers, handles serialization). LGTM.
src/server/web_server.js (6)
95-96: LGTM: secure context update formattingMinor cleanup looks good; no behavior change.
208-209: LGTM: explicit 200 and payloadClearer than implicit defaults.
223-224: LGTM: simplified return shapeConcise and readable.
242-252: LGTM: early returns with sendStatus(500)Consistent and prevents fallthrough.
311-311: LGTM: single send with explicit statusStraightforward and correct.
376-376: LGTM: concise early returns in version comparatorClearer logic; no behavior change.
Also applies to: 386-386, 395-395
liranmauda
force-pushed
the
liran-bump-express
branch
from
f1f9c5d to
adb6978
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/server/web_server.js (3)
148-166: Avoid open-redirect and Host-header poisoning in HTTPS redirectUsing
req.get('Host')directly enables host-header attacks and open redirects. Preferreq.secure(withapp.set('trust proxy', true)) andreq.hostname.function https_redirect_handler(req, res, next) { - const fwd_proto = req.get('X-Forwarded-Proto'); - if (fwd_proto === 'http') { - const host = req.get('Host'); - return res.status(302).redirect('https://' + host + req.originalUrl); - } - return next(); + // With trust proxy enabled, req.secure reflects x-forwarded-proto + if (!req.secure) { + const url = `https://${req.hostname}${req.originalUrl}`; + return res.redirect(302, url); + } + next(); }Add before any middleware in setup:
app.set('trust proxy', true);
220-227: Avoid double-ending responses in get_version_handlerCurrent flow can call
res.send()thenres.end()again. Set status first and send once.// Suggested replacement (outside changed hunk for clarity) async function get_version_handler(req, res) { if (config.NOOBAA_VERSION_AUTH_ENABLED && !http_utils.authorize_bearer(req, res)) return; const { status, version } = await getVersion(req.path); return res.status(status).send(version || ''); }
370-401: Harden is_latest_version: normalize inputs and use dbg instead of console.logGuard against missing env var, ensure string inputs, and use
dbglogging.function is_latest_version(query_version) { - const srv_version = process.env.CURRENT_VERSION; - console.log('Checking version', query_version, 'against', srv_version); - - if (query_version === srv_version) return true; + const srv_version = process.env.CURRENT_VERSION; + dbg.log1('Checking version', query_version, 'against', srv_version); + if (!srv_version || !query_version) return true; + if (String(query_version) === String(srv_version)) return true; @@ - if (parseInt(srv_version_parts[i], 10) > parseInt(query_version_parts[i], 10)) return false; + if (parseInt(srv_version_parts[i], 10) > parseInt(query_version_parts[i], 10)) return false; @@ - if (srv_version_parts.length > query_version_parts.length) return false; + if (srv_version_parts.length > query_version_parts.length) return false;
♻️ Duplicate comments (2)
src/server/web_server.js (2)
168-182: Bug: reading query string from req.params[0] breaks on Express 5; use req.query.curr
req.params[0]matches a path wildcard, not the query string. This will cause/get_latest_version?curr=1.2.3to return 400 most of the time.function get_latest_version_handler(req, res) { - if (req.params[0] && req.params[0].includes('&curr=')) { - try { - const query_version = req.params[0].substring(req.params[0].indexOf('&curr=') + 6); - let ret_version = ''; - if (!is_latest_version(query_version)) { - ret_version = config.on_premise.base_url + process.env.CURRENT_VERSION + '/' + config.on_premise.nva_part; - } - return res.status(200).send({ version: ret_version }); - } catch (err) { - // noop - } - } - return res.status(400).send({}); + const query_version = req.query?.curr; + if (!query_version || typeof query_version !== 'string') return res.sendStatus(400); + try { + let ret_version = ''; + if (!is_latest_version(String(query_version))) { + ret_version = `${config.on_premise.base_url}${process.env.CURRENT_VERSION}/${config.on_premise.nva_part}`; + } + return res.status(200).send({ version: ret_version }); + } catch (err) { + dbg.warn('get_latest_version_handler failed', err); + return res.sendStatus(500); + } }Optional: remove the wildcard from the route to avoid confusion:
- Current (Line 120):
app.get('/get_latest_version*', get_latest_version_handler);- Suggested:
app.get('/get_latest_version', get_latest_version_handler);I can add/adjust integration tests to assert 200/400 behavior for
/get_latest_version?curr=....
184-204: Validate inputs, remove console.log, and wire body parsers (Express 5)
console.logis inconsistent withdbg.*.- Validate
module_name/levelas non-empty strings.- Express 5 does not bundle body parsing;
req.bodywill be undefined without parsers.async function set_log_level_handler(req, res) { - const module_name = req.params.module || req.query.module || req.body.module; - const level = req.params.level || req.query.level || req.body.level; - - console.log('req.module', module_name, 'req.level', level); - if (!module_name || !level) { - return res.status(400).end(); - } - - dbg.log0('Change log level requested for', module_name, 'to', level); + const module_name = req.params?.module || req.query?.module || req.body?.module; + const level = req.params?.level || req.query?.level || req.body?.level; + if (typeof module_name !== 'string' || !module_name || + typeof level !== 'string' || !level) { + return res.sendStatus(400); + } + dbg.log0('Change log level requested for', module_name, 'to', level); dbg.set_module_level(level, module_name); @@ - res.sendStatus(200); + return res.sendStatus(200); }Add body parsers early in setup (before routes that read
req.body):app.use(express.json()); app.use(express.urlencoded({ extended: false }));#!/bin/bash # Verify body parsers are configured and find req.body usages rg -n "express\\.json|express\\.urlencoded|bodyParser" src/server/web_server.js rg -n "req\\.body" -n
🧹 Nitpick comments (3)
src/server/web_server.js (3)
91-97: Nit: honorCipherOrder is not used by setSecureContext()
honorCipherOrderis a server option; it has no effect when passed tohttps_server.setSecureContext(...). Safe to omit for clarity.- https_server.setSecureContext({ ...updated_cert_info.cert, honorCipherOrder: true }); + https_server.setSecureContext({ ...updated_cert_info.cert });
309-311: Nit: explicitly set content-type to HTMLBe explicit since the payload contains
<br>tags.- res.status(200).send(nsfs_report); + res.status(200).type('html').send(nsfs_report);
324-340: Error handler: standardize logging and simplify status handling
- Prefer
dbg.erroroverconsole.error.- Status normalization looks good; consider setting before building the HTML.
// Outside changed lines, but recommended: dbg.error('ERROR:', err);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/server/web_server.js(8 hunks)src/test/framework/server.js(0 hunks)src/test/integration_tests/nc/cli/test_cli_diagnose.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/framework/server.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/test/integration_tests/nc/cli/test_cli_diagnose.test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/server/web_server.js (1)
206-209: LGTM: get_log_level_handler response200 + JSON object is fine.
liranmauda
force-pushed
the
liran-bump-express
branch
from
adb6978 to
50a7474
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/server/web_server.js (2)
120-121: Simplify the route; use query parameters instead of a wildcardThe handler should read the query (req.query.curr). The trailing * is unnecessary and misleading.
- app.get('/get_latest_version*', get_latest_version_handler); + app.get('/get_latest_version', get_latest_version_handler);
111-116: Enable body parsers before routes that read req.bodyExpress 5 doesn’t include body parsing by default. Since set_log_level_handler reads req.body, parse bodies early.
app.use(express_morgan_logger(dev_mode ? 'dev' : 'combined')); + // Parse request bodies before any route that accesses req.body + app.use(express.json()); + app.use(express.urlencoded({ extended: false })); app.use(https_redirect_handler);
♻️ Duplicate comments (2)
src/server/web_server.js (2)
169-182: Bug: reading query from req.params[0] breaks on Express 5; use req.query.curr and proper error codesreq.params[0] captures wildcard path, not the query string. This will sporadically 400 even for valid requests. Also, swallowing errors with a noop hides server problems.
Apply:
-function get_latest_version_handler(req, res) { - if (req.params[0] && req.params[0].includes('&curr=')) { - try { - const query_version = req.params[0].substring(req.params[0].indexOf('&curr=') + 6); - let ret_version = ''; - if (!is_latest_version(query_version)) { - ret_version = config.on_premise.base_url + process.env.CURRENT_VERSION + '/' + config.on_premise.nva_part; - } - return res.status(200).send({ version: ret_version }); - } catch (err) { - // noop - } - } - return res.status(400).send({}); -} +function get_latest_version_handler(req, res) { + const query_version = req.query?.curr; + if (typeof query_version !== 'string' || !query_version) { + return res.sendStatus(400); + } + try { + let ret_version = ''; + if (!is_latest_version(String(query_version))) { + ret_version = `${config.on_premise.base_url}${process.env.CURRENT_VERSION}/${config.on_premise.nva_part}`; + } + return res.status(200).send({ version: ret_version }); + } catch (err) { + dbg.warn('get_latest_version_handler failed', err); + return res.sendStatus(500); + } +}To verify tests cover this endpoint and its 200/400 behavior after the change, run:
#!/bin/bash set -euo pipefail # Find existing tests or callers of the endpoint rg -n "get_latest_version" src/test || true rg -n "/get_latest_version" || true # Check for any lingering uses of req.params\\[0\\] patterns rg -n "req\\.params\\s*\\[\\s*0\\s*\\]" src || true
184-204: set_log_level_handler: avoid crash on req.body access, validate inputs, and remove console.logAccessing req.body.module when no body parser is configured throws. Also, prefer dbg logging and stronger validation.
async function set_log_level_handler(req, res) { - const module_name = req.params.module || req.query.module || req.body.module; - const level = req.params.level || req.query.level || req.body.level; - - console.log('req.module', module_name, 'req.level', level); - if (!module_name || !level) { - return res.status(400).end(); - } - - dbg.log0('Change log level requested for', module_name, 'to', level); - dbg.set_module_level(level, module_name); + const module_name = req.params?.module || req.query?.module || req.body?.module; + const level = req.params?.level || req.query?.level || req.body?.level; + + if (typeof module_name !== 'string' || !module_name || + typeof level !== 'string' || !level) { + return res.sendStatus(400); + } + + dbg.log0('Change log level requested for', module_name, 'to', level); + dbg.set_module_level(level, module_name); @@ - res.sendStatus(200); + return res.sendStatus(200); }Also ensure body parsers are configured before routes using req.body (see next comment).
🧹 Nitpick comments (3)
src/test/framework/server.js (1)
26-29: Return an explicit status in the POST handler for test stabilityres.end() works, but tests are more robust with an explicit 2xx status. Consider 204 or echoing the body.
app.post("*", function(req, res) { console.log(req.body); - res.end(); + res.sendStatus(204); });src/server/web_server.js (2)
95-96: Nit: honorCipherOrder is not used by setSecureContexthonorCipherOrder is a tls server option, not part of the secure context. Keep it on server creation; drop it here to avoid confusion.
- https_server.setSecureContext({ ...updated_cert_info.cert, honorCipherOrder: true }); + https_server.setSecureContext(updated_cert_info.cert);
370-401: Prefer dbg logging over console and harden version parsingOptional cleanup:
- Replace console.log/error with dbg logging for consistency.
- Guard parseInt/Number conversions to avoid NaN pitfalls on non-numeric parts (e.g., prerelease tags).
- console.log('Checking version', query_version, 'against', srv_version); + dbg.log1('Checking version', query_version, 'against', srv_version); @@ - if (parseInt(srv_version_parts[i], 10) > parseInt(query_version_parts[i], 10)) return false; - if (parseInt(srv_version_parts[i], 10) < parseInt(query_version_parts[i], 10)) { - console.error('BUG?! Queried version (', query_version, ') is higher than server version(', - srv_version, ') ! How can this happen?'); + const a = Number(srv_version_parts[i]); + const b = Number(query_version_parts[i]); + if (!Number.isFinite(a) || !Number.isFinite(b)) break; // bail on non-numeric segment + if (a > b) return false; + if (a < b) { + dbg.warn('Queried version higher than server version', { query_version, srv_version }); return true; } @@ - if (srv_version_parts.length > query_version_parts.length) return false; + if (srv_version_parts.length > query_version_parts.length) return false; if (srv_version_parts.length < query_version_parts.length) { - console.error('BUG?! Queried version (', query_version, ') is higher than server version(', - srv_version, '), has more tailing parts! How can this happen?'); + dbg.warn('Queried version has more tailing parts than server version', { query_version, srv_version }); return true; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/server/web_server.js(8 hunks)src/test/framework/server.js(1 hunks)src/test/integration_tests/nc/cli/test_cli_diagnose.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/nc/cli/test_cli_diagnose.test.js
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit Configuration File
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/framework/server.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (6)
src/test/framework/server.js (1)
23-24: Express 5 built-in body parsing — LGTMGood swap from body-parser to app.use(express.urlencoded(...)) and app.use(express.json()).
src/server/web_server.js (5)
163-164: Explicit 302 redirect — LGTMMaking the 302 explicit is fine and keeps intent clear.
208-209: get_log_level_handler response consolidation — LGTM
223-228: getVersion: concise return — LGTM
239-252: oauth_authorise_handler: concise error paths — LGTMUsing res.sendStatus(500) clarifies control-flow and avoids double-ends.
309-311: metrics_nsfs_stats_handler: single 200 send — LGTM
liranmauda
force-pushed
the
liran-bump-express
branch
from
50a7474 to
f7d06d4
Compare
liranmauda
force-pushed
the
liran-bump-express
branch
2 times, most recently
from
de75ded to
4a862ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/framework/server.js (1)
26-29: Be explicit with response status.Avoid relying on implicit 200 + empty body; make intent clear (200 OK or 204 No Content).
- res.end(); + // If callers expect a bodyless success, prefer explicit status: + res.sendStatus(204); + // or, if tests expect 200: + // res.sendStatus(200);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/test/framework/server.js(1 hunks)src/test/integration_tests/nc/cli/test_cli_diagnose.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/test/integration_tests/nc/cli/test_cli_diagnose.test.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/framework/server.js
🧬 Code graph analysis (1)
src/test/framework/server.js (1)
src/test/integration_tests/nc/cli/test_cli_diagnose.test.js (2)
app(91-91)express(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/test/framework/server.js (1)
23-24: Set explicit body-size limits on the test server parsersDefaults are small and can cause 413s for larger payloads; set explicit limits in src/test/framework/server.js (lines 23–24).
Suggested change:
-app.use(express.urlencoded({ extended: true })); -app.use(express.json()); +app.use(express.urlencoded({ extended: true, limit: '5mb' })); +app.use(express.json({ limit: '5mb' }));If tests POST raw text (repo contains many occurrences of ContentType: 'text/plain'), also add:
app.use(express.text({ limit: '5mb', type: 'text/plain' }));
- Bump express from 4.21.2 to 5.1.0 Signed-off-by: liranmauda <[email protected]>
liranmauda
force-pushed
the
liran-bump-express
branch
from
4a862ce to
1b2ac32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
92-92: Addenginesfield to enforce minimum Node.js version for Express 5 compatibility.Express 5.0 requires Node.js v18 or newer. The package.json currently lacks an
enginesfield to enforce this constraint at install time. The migration to Express 5 appears complete—the codebase correctly usesexpress.json()instead of body-parser and contains no incompatible APIs.Add the following field after line 3 to enforce the required Node.js version:
"name": "noobaa-core", "version": "5.21.0", + "engines": { + "node": ">=18" + }, "license": "SEE LICENSE IN LICENSE",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/test/framework/server.js(1 hunks)src/test/integration_tests/nc/cli/test_cli_diagnose.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/nc/cli/test_cli_diagnose.test.js
- src/test/framework/server.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
2 participants
Explain the Changes
Summary by CodeRabbit
New Features
Chores
Tests
Documentation