Security fixes manual#51
Open
Satishpethani92 wants to merge 7 commits into
Open
Conversation
Addresses the critical and high-severity findings documented in the April 2026
security audit. Full per-file breakdown is in SECURITY_FIXES.md.
Highlights:
- IPFS KYC upload rewritten with server-issued nonce + file-hash binding
so a captured signature can no longer be reused on any other file (M-9).
- Hardened CSP: removed 'unsafe-eval' and http:// sources in production,
added HSTS preload and upgrade-insecure-requests.
- express-rate-limit wired into auth / tx / search / upload / mutation
routers via helpers/rateLimiters.js (M-2).
- /verifyTx now parses the raw tx, extracts the EIP-155 chainId from v,
and rejects unprotected or cross-chain transactions (H-7).
- /candidates/search escapes regex metacharacters via lodash.escaperegexp
and caps query length to defeat ReDoS (H-1); /listByHash enforces CSV
string format and caps list length.
- All escape() calls replaced with strict UUID-v4 validation; every
console.trace(e) stack-trace leak removed.
- Error middleware rewritten to sanitize client-facing responses while
preserving full-fidelity server logs (M-4).
- MongoDB connection now reads MONGO_URI/DB_URI env vars so authenticated
URIs stay out of config files; /api-docs gated behind basic auth in
production.
- Committed secrets removed: sslcert/server.key, sslcert/server.crt,
travis.pem.enc. .gitignore expanded to block re-introduction.
- Dockerfile upgraded from EOL node:16.16.0 to node:20.18.0 with a
multi-stage build and non-root runtime user.
- xdc3 pinned from "latest" to 1.3.13416; lodash bumped to 4.17.21;
express-fileupload moved to runtime deps.
Breaking change: the old /api/ipfs/addKYC body schema is no longer accepted.
Clients must call /api/ipfs/requestKYCNonce first, sign the returned
message (which includes sha256(file) at submit time), and pass the nonce
alongside the signed message on upload. See SECURITY_FIXES.md for details.
Operational follow-ups still required (out-of-band):
- Rotate TLS certificate issued against the removed server.key
- Rotate any credentials protected by travis.pem.enc
- Enable MongoDB authentication and set MONGO_URI in production env
Made-with: Cursor
… CVEs Reachability analysis of the 120 npm audit findings against the production Node server. Categorizes advisories into server-runtime (61), browser-bundle-only (24), and dev/build-only (14); traces every entry to its top-level requirer; intersects with the set of packages the server actually require()s. Non-destructive HTTP probes against master.xinfin.network confirm the live site is still running the pre-fix upstream (signed QR message has no id binding, /api/ipfs/requestKYCNonce returns 404, Swagger UI at /api-docs/ is ungated, 60 consecutive /api/auth/generateLoginQR calls return 200 with no ratelimit-* headers, raw library errors leak). Of the 61 server-runtime CVEs, only two (elliptic signature malleability and qs query-string DoS) have a plausible live-exploit path, and both are mitigated by the fixes in this branch. The real live risk today is the 8 P0/P1 application-level findings from the re-audit, all of which this branch's commit 3731be5 closes. Made-with: Cursor
XinFinOrg#44-XinFinOrg#48 Builds on top of upstream XinFinOrg#44, XinFinOrg#45, XinFinOrg#46, XinFinOrg#47, XinFinOrg#48 (already merged into master). Every helper they added (toHexAddress, normalizeValue, unauthorized, multi-format signer recovery, x-kyc-* header fallbacks, ALLOWED_SCHEMES URL allowlist, sort- field allowlist, single-helmet structure, function-based CORS + error handler, default-rewards structure) is preserved. Fixes found during the local end-to-end pass: F-1 Dockerfile build: - npm install --legacy-peer-deps --ignore-scripts in both stages: skips sha3 native compilation that fails on Node 20. - Replace final chown -R masternode:masternode /app with COPY --chown on each stage and a tiny chown for tmp/sslcert. The recursive chown over node_modules was hanging on overlay/WSL2 filesystems. F-2 app/components/Setting.vue (XSS-class adjacent / wallet UX): encodeURIComponent (not encodeURI) for both message and submitURL when constructing xdcchain://login URIs. The server-issued message now contains "id=<uuid>" (audit fix H-2 / login-QR session binding) and encodeURI does not escape "=", so the mobile wallet URL parser was splitting that token into the wrong query keys. F-3 app/components/candidates/Apply.vue (KYC client): Rewire uploadKYC() to the two-step flow: 1. Compute sha256(file) in the browser via window.crypto.subtle. 2. POST /api/ipfs/requestKYCNonce to mint a one-time nonce. 3. Sign exactly "[XDCmaster KYC <nonce>] Upload <fileHash> for <acct>" and submit signature + nonce + file to /api/ipfs/addKYC. Replaces the timestamp-only message that PR XinFinOrg#44 wired (which still allowed any captured signed message to be paired with any other file within the 5-minute window). x-kyc-* header set is preserved. F-4 apis/auth.js + models/mongodb/signature.js (lateral-takeover fix): Single-use binding of signedId -> signedAddress. The previous lookup was by signedAddress alone, which let a second wallet that scanned the victim's QR with their own client overwrite the same login session and hijack the SPA polling /getLoginResult. Now: - signedId carries a unique+sparse index (atomic guarantee under concurrent inserts). - verifyLogin rejects re-claim by a different signer and treats a same-signer retry as idempotent success. - 24h TTL on the row is refreshed on every successful login. Ops note: existing deployments must drop the old non-unique index before bouncing — see SECURITY_FIXES.md "Pre-deploy migration note". F-5 app/app.js (production blank-page fix): Switch the root Vue instance from template:'<App/>' to render: h => h(App). Webpack 5 + terser was tree-shaking the Vue 2.7 runtime template compiler in the production build, leaving the root instance unable to resolve <App/> and rendering an empty comment node. Render functions are statically analysable and never DCE'd. F-6 package.json (runtime dependency): Move connect-flash from devDependencies to dependencies. It is required at runtime by index.js (app.use(flash())) and the Docker image was crashing on first request without it. Also includes: - SECURITY_FIXES.md: appended "Follow-up E2E pass" section (F-1..F-6), the Signature.signedId index migration note for ops, and the rationale for not adopting PR XinFinOrg#44's optional x-api-key bypass on /addKYC. - test-harness/soak.sh: parametrised 5-minute soak harness used during the local pass (no secrets, all knobs env-overridable). - Webpack production bundles regenerated to pick up F-3 and F-5; old fingerprinted bundles removed accordingly. Verification performed locally: - npm install --legacy-peer-deps --ignore-scripts (clean) - ESLint on every changed JS / Vue file (0 errors) - vue-template-compiler on Apply.vue + Setting.vue (0 errors) - Webpack production build (devtool:false, no source maps emitted) - Headless-Chromium walk of the SPA: login QR + KYC + voting flows - E2E suite for /api/auth/* and /api/ipfs/* with real EIP-191 signatures - E2E suite for /api/voters/verifyTx with EIP-155 chainId binding - docker build + docker run against local Mongo, smoke-hit /api/config - 5-minute soak (8 workers, mixed read endpoints): stable mem, no leaks - npm test (Truffle): pre-existing Node 20 incompatibility, unchanged Made-with: Cursor
… major, 5 correctness, 9 nits) Follow-up to PR XinFinOrg#49 covering every comment from the automated review: Critical - apis/ipfs.js: rewrite KYC pin to use ipfs-http-client@40+'s Promise API (the previous callback signature silently never fired and hung the request). Defer the atomic nonce consume until after the IPFS pin succeeds so a transient pinning failure no longer wastes the user's single-use nonce. Normalised v40/v50 result shapes; cleanup wrapped in finally{}. Major - apis/auth.js: require canonical "[XDCmaster <iso>] Login id=<uuid>" shape on /verifyLogin so attackers can't strip the prefix to skip the TTL window. Reject unparseable timestamps. - apis/ipfs.js: drop req.query fallbacks for account/signedMessage/ nonce — those leak credentials into nginx access logs, browser history and Referer headers. Body and x-kyc-* headers only. - index.js: DEBUG_REQUESTS now keys off the file-wide IS_PRODUCTION fail-secure check (was inverted on mainnet|testnet|devnet) and REQUEST_TRACE is opt-in ('1') instead of opt-out. SPA fallback uses the same check for consistency. Correctness - apis/voters.js: generateQR catch now next(e) so sanitizeForClient runs (was sending raw e.message). Replaced hand-rolled extractChainIdFromTx with ethereumjs-tx@1's built-in getChainId() (verified semantically identical: 0 for v=27|28 / missing v). - app/components/candidates/Apply.vue: explicit window.crypto.subtle availability check before digest() so plain-HTTP deployments fail with a readable toast instead of an opaque TypeError. - models/mongodb/index.js: convert callback mongoose.connect to Promise form + connection.on('error') listener so connection failures surface and post-connect blips don't get swallowed. - models/mongodb/signature.js: drop redundant index:true (unique already creates the index) — was emitting duplicate-index warnings. - models/mongodb/ipfsNonce.js: nonce now required:true (a missing- field doc could match the consume CAS); same redundant-index cleanup. Nits - .env.example: NODE_ENV=development by default with comment that prod must override it (avoids tripping Swagger gating on local boot). - helpers/rateLimiters.js: remove dead message field on authLimiter that the custom handler always overrode. - Dockerfile: move USER masternode + chown /app ahead of npm install so we never run a recursive chown over node_modules. Comment now matches behaviour. - apis/candidates.js: keep the doubled UUID validation as defense in depth with an explanatory comment. - middlewares/error.js: tighten sanitizeForClient — strip Windows paths, multi-segment Unix paths, and dangling 'at' tokens, then gate the result through an explicit allowlist regex (/, <>, {}, quotes, backticks excluded). Verified against 31 real validator/ throw messages (all pass verbatim) and 8 dangerous shapes (all neutralised to 'Error' or path-stripped). - SECURITY_FIXES.md: escape pipe in v=27|28 inline code so the markdown table row renders. - test-harness/soak.sh: fix misleading 'authLimiter' label — the soak only hits read-side endpoints (readLimiter at 240 req/min/IP). - sslcert/README.md, LIVE_EXPOSURE_REPORT.md: add language identifiers (bash/http/text) on every fenced code block (MD040). Also rebuilt the production webpack bundle so the Apply.vue secure-context guard ships in build/. Lint passes clean on every edited file (npm run lint). Documented every finding + fix in SECURITY_FIXES.md under a new "Code-review follow-ups — 2026-04-27" section. Made-with: Cursor
security: re-audit P0/P1 remediations + E2E follow-up (builds on XinFinOrg#44-XinFinOrg#48)
Security hardening, dependency upgrades, and Solidity contract updates with successful verification.
📝 WalkthroughWalkthroughThis PR implements a comprehensive security hardening audit across the XDC MasterNode App, featuring nonce-based KYC flow replay protection, UUID-v4 validation on authentication endpoints, rate limiting infrastructure, Smart contract Solidity 0.8 migration, Docker multi-stage hardening with non-root user, TLS key removal after an exposure incident, frontend wallet provider restrictions, and production-gated Content Security Policy enforcement. ChangesBackend API Security & Nonce-Based KYC
Smart Contract Solidity 0.8 Migration
Infrastructure & Deployment
Frontend Security
Dependency & Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apis/voters.js (1)
209-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
amountshould be string-coerced or validated before.replace().The validation chain on
/verifyTxdoesn't constrainreq.body.amount, so a non-string payload ({"amount": 5}or{"amount": [...]}) reachesreq.body.amount.replace(/,/g, '')and throwsTypeError: ... .replace is not a function. The outer catch sanitizes the response, so this isn't exploitable beyond a noisy 500, but it's a trivial guard to add.🛡️ Proposed fix
- const amount = (req.body.amount) - ? new BigNumber(req.body.amount.replace(/,/g, '')).toString(10) - : undefined + const amount = (req.body.amount) + ? new BigNumber(String(req.body.amount).replace(/,/g, '')).toString(10) + : undefinedAlternatively, add
check('amount').optional().isString().isLength({ max: 64 })to the validation chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/voters.js` around lines 209 - 237, The /verifyTx handler (router.post('/verifyTx')) is calling req.body.amount.replace(...) without ensuring amount is a string; add validation and/or coercion to avoid TypeError: ensure the request validation chain for this route includes check('amount').optional().isString().isLength({ max: 64 }) (or equivalent) so non-string amounts are rejected, and/or coerce amount to a string before calling replace (e.g. String(req.body.amount).replace(...)) where the BigNumber conversion is done; update the code around the amount assignment and BigNumber(...) usage to rely on the validated/coerced value.apis/ipfs.js (1)
24-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen
accountformat validation in/requestKYCNonce.
toHexAddressonly returns an empty string whenaccountis missing or a non-string; for a non-empty garbage value like"abc"it returns"abc"and theif (!toHexAddress(account))guard passes. The signature step in/addKYCwill reject the garbage, so this isn't a security hole, but it lets an unauthenticated caller spamIpfsNoncedocuments (5-minute TTL bounds the damage but also lengthens it). A one-line hex-format check rejects this at the front door and yields a clearer error.🛡️ Proposed fix
- const account = normalizeValue(req.body.account).toLowerCase() - if (!toHexAddress(account)) { - return badRequest(res, 'invalid_account') - } + const account = normalizeValue(req.body.account).toLowerCase() + const accountHex = toHexAddress(account) + if (!/^0x[0-9a-f]{40}$/.test(accountHex)) { + return badRequest(res, 'invalid_account') + }Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/ipfs.js` around lines 24 - 29, The toHexAddress helper currently accepts non-hex garbage like "abc" and causes /requestKYCNonce to accept invalid accounts; update validation so only valid 20-byte hex addresses are allowed: modify toHexAddress (or add a short validator used by /requestKYCNonce) to normalize XDC prefixes to 0x and then require the result match /^0x[0-9a-f]{40}$/i (return empty string or falsy on failure), and use that stricter check in the /requestKYCNonce handler before creating IpfsNonce documents to reject malformed accounts up-front.contracts/XDCValidator.sol (2)
83-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConstructor does not validate that
_candidatesand_capsarrays are the same length.If
_caps.length < _candidates.length, the constructor panics mid-loop with an out-of-bounds revert (Solidity 0.8.x array bounds check), leaving no contract deployed but potentially wasting significant gas on a large candidate set. An explicitrequiremakes the intent clear and the failure mode immediate and readable.🛠️ Proposed fix
constructor ( address[] memory _candidates, uint256[] memory _caps, address _firstOwner, uint256 _minCandidateCap, uint256 _minVoterCap, uint256 _maxValidatorNumber, uint256 _candidateWithdrawDelay, uint256 _voterWithdrawDelay ) { + require(_candidates.length == _caps.length, "XDCValidator: array length mismatch"); minCandidateCap = _minCandidateCap;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/XDCValidator.sol` around lines 83 - 108, The constructor currently iterates _candidates and indexes _caps without checking lengths, causing an out-of-bounds revert if _caps.length < _candidates.length; add an explicit require at the start of the constructor that validates _candidates.length == _caps.length (and optionally > 0 if desired) so failures are immediate and clear; update the constructor (referencing the constructor, _candidates, _caps, and the loop that sets validatorsState and voters) to perform this check before any state mutations or the for-loop.
194-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
delete blockNumbers[_index]leaves a zero sentinel in the array;getWithdrawBlockNumbers()permanently accumulates zero entries.Like the
candidatesissue above,delete withdrawsState[msg.sender].blockNumbers[_index]zeroes the slot without shrinking the array. Every successfully completed withdrawal adds a permanent zero to the user'sblockNumbersarray. WhileonlyValidWithdrawrejects_blockNumber == 0, the array still grows without bound, increasing gas for any off-chain iteration.🛠️ Proposed fix — swap-and-pop in `withdraw()`
- delete withdrawsState[msg.sender].blockNumbers[_index]; + uint256 lastIdx = withdrawsState[msg.sender].blockNumbers.length - 1; + withdrawsState[msg.sender].blockNumbers[_index] = withdrawsState[msg.sender].blockNumbers[lastIdx]; + withdrawsState[msg.sender].blockNumbers.pop();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/XDCValidator.sol` around lines 194 - 200, The withdraw() currently uses delete withdrawsState[msg.sender].blockNumbers[_index] which leaves a zero sentinel and lets the array grow with zeros; update withdraw() to perform a swap-and-pop on withdrawsState[msg.sender].blockNumbers: read the array, if _index is not the last index assign the last element into blockNumbers[_index], then pop the last element (and only delete the mapping slot if needed), ensuring you handle the case where the array length is 1; reference withdraw(), withdrawsState[msg.sender].blockNumbers, and onlyValidWithdraw to locate and modify the logic so getWithdrawBlockNumbers() no longer accumulates zero entries.
🧹 Nitpick comments (4)
Dockerfile (1)
23-23: ⚡ Quick winPrefer
npm cifor deterministic dependency resolution.Line 23 and Line 51 currently use
npm install; switching tonpm ci(with existing flags) gives tighter lockfile reproducibility for a security-focused image build.♻️ Suggested change
-RUN npm install --legacy-peer-deps --ignore-scripts +RUN npm ci --legacy-peer-deps --ignore-scripts ... -RUN npm install --omit=dev --legacy-peer-deps --ignore-scripts \ +RUN npm ci --omit=dev --legacy-peer-deps --ignore-scripts \ && npm cache clean --forceAlso applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 23, Replace the non-deterministic install command "RUN npm install --legacy-peer-deps --ignore-scripts" with "RUN npm ci --legacy-peer-deps --ignore-scripts" to use the lockfile for reproducible installs; update the other identical RUN npm install occurrences in the Dockerfile as well and ensure a package-lock.json is present in the build context so npm ci can run successfully.index.js (1)
178-179: 💤 Low valueDuplicate error middleware registration.
require('./middlewares/error')isapp.use'd twice (line 179 and line 192). Only the first one is reachable fornext(err)propagation from the API/sitemap routes — the second is dead code because Express stops at the first error handler that responds. Not a functional bug, but it's confusing and easy to drift out of sync if the file evolves.♻️ Proposed cleanup
-// error handler -app.use(require('./middlewares/error')) - app.get('*', function (req, res) { // Use the same fail-secure check as the rest of the file — anything // that isn't a recognised dev/local NODE_ENV serves the production // bundle from build/. const p = !IS_PRODUCTION ? path.resolve(__dirname, 'index.html') : path.resolve(__dirname, './build', 'index.html') return res.sendFile(p) }) // error handler app.use(require('./middlewares/error'))Also applies to: 191-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 178 - 179, Remove the duplicate registration of the error middleware so there is only one app.use(require('./middlewares/error')) call; locate the two occurrences that register the middleware (the earlier app.use(require('./middlewares/error')) after route/middleware setup and the later identical call) and delete the later one to avoid dead/duplicate error-handler registration and keep the single error handler (require('./middlewares/error')) as the canonical final error middleware.middlewares/error.js (1)
26-28: ⚡ Quick winComment claims full-fidelity logging but only the message string is logged.
The comment says "Log at full fidelity, but never send raw stack/paths back to the client." but
logger.warn('request %s %s failed: %s', ..., message)discardserr.stackand any structured fields — making post-mortem debugging significantly harder for true 500s. The client-side sanitization is still preserved by passingmessageonly into theres.jsonpayload; the logger can safely receive the full error.♻️ Proposed fix to log full error while keeping client output sanitized
- // Log at full fidelity, but never send raw stack/paths back to the client. - if (status !== 401 && status !== 403) { - logger.warn('request %s %s failed: %s', req.method, req.originalUrl, message) - } + // Log at full fidelity (incl. stack), but never send raw stack/paths to client. + if (status !== 401 && status !== 403) { + logger.warn('request %s %s failed: %s', req.method, req.originalUrl, + (err && err.stack) ? err.stack : message) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middlewares/error.js` around lines 26 - 28, The logger currently records only the sanitized message in the warning (see the logger.warn call in middlewares/error.js), so capture and log the full error object/stack there while continuing to send only the sanitized message to the client; update the logger.warn invocation to include err and/or err.stack (or the full error as a structured field) alongside req.method and req.originalUrl so you get full-fidelity logs for post-mortem debugging but leave the response body using only the sanitized message.package.json (1)
84-84: ⚡ Quick winUpdate
uuidfrom v3.3.2 to a current stable version (v14.0.0+).While UUID v4 validation is handled by
express-validator, uuid v3.3.2 is outdated (released ~2018). The package has known CVEs (CVE-2026-41988, CVE-2026-41907) affecting UUID v3, v5, and v6 generation with external buffers. Current version is v14.0.0 (2026).If uuid is used for generation beyond v4, upgrading is essential. The v4() API is stable across versions, but code using deep imports (
require('uuid/v4')) must migrate to named exports:const { v4: uuidv4 } = require('uuid')(required since v8.0.0).♻️ Proposed update
- "uuid": "^3.3.2", + "uuid": "^14.0.0",Verify and update any
require('uuid/v4')orfrom 'uuid/v4'imports to use named exports instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 84, Update the obsolete "uuid" dependency from "^3.3.2" to a current stable version (e.g. "^14.0.0") in package.json and run install; then search the codebase for legacy deep-imports like "require('uuid/v4')" or "from 'uuid/v4'" and replace them with the modern named-export usage (e.g. import/require the package and use the named v4 export such as v4 or uuidv4) so generation calls keep working after the upgrade.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/default.json`:
- Line 33: The runtime configuration currently allows falling back to db.uri
when MONGO_URI is absent; change the config loader to fail fast in production by
validating process.env.MONGO_URI and throwing or exiting with a clear error if
it's missing when NODE_ENV === "production". Locate the configuration
bootstrap/loader (e.g., functions named loadConfig, getDatabaseUri, or the
module that reads config/default.json and process.env) and add a check that
enforces presence of MONGO_URI in production, preventing use of db.uri fallback
and surfacing an explicit error message indicating MONGO_URI is required in
production.
In `@contracts/XDCValidator.sol`:
- Around line 175-192: The resign() implementation currently uses delete
candidates[i], leaving zero-address holes and causing unbounded growth and
expensive scans; replace that with swap-and-pop: inside resign() when you find
index i where candidates[i] == _candidate, set candidates[i] =
candidates[candidates.length - 1] (if i != lastIndex), then call
candidates.pop() to shrink the array, and then break; ensure candidateCount is
decremented as already done, and handle the case where the removed element was
the last element (just pop). Update only the candidates array manipulation in
resign() (function name: resign, variable: candidates) to perform swap-and-pop
so the array stays compact and removals are O(1).
In `@helpers.js`:
- Around line 13-18: The comment warns but doesn't prevent use of
HDWalletProvider; add a runtime guard around the exported HDWalletProvider in
helpers.js so it cannot be constructed/used in unsafe runtimes (e.g., if
process.env.NODE_ENV === 'production' or when a new env var like
DISABLE_MNEMONIC_LOGIN is true) — implement this by replacing the direct
export/constructor reference to HDWalletProvider with a small guard wrapper that
throws an explicit error (including guidance to use safer auth) when attempted
in production/when the env flag is set, otherwise returns/forwards to the
original HDWalletProvider; reference the HDWalletProvider symbol and the export
in helpers.js so callers fail fast instead of silently allowing mnemonic usage.
In `@index.js`:
- Around line 73-75: The CSP config uses upgradeInsecureRequests: true which
breaks helmet >=4; update the production branch where the object spread uses
IS_PRODUCTION to set upgradeInsecureRequests to an empty array ([]) instead of
true so the directive is iterable. Locate the object that conditionally spreads
...(IS_PRODUCTION ? { upgradeInsecureRequests: true } : {}) in index.js and
replace the boolean value with an empty array, ensuring the rest of the CSP
object structure remains unchanged.
In `@SECURITY_FIXES.md`:
- Around line 56-72: Remove or update the stale C-4 entry in the "Known
remaining work" section that claims "C-4 (Solidity 0.4.21 in
`contracts/XDCValidator.sol`)" is outstanding; instead mark it as completed or
delete the bullet, since the "Final Security Hardening" table (row 18) already
documents contracts upgraded to 0.8.x—edit the "Known remaining work" block to
reflect the current state and keep C-4 consistent with the Final Security
Hardening table.
---
Outside diff comments:
In `@apis/ipfs.js`:
- Around line 24-29: The toHexAddress helper currently accepts non-hex garbage
like "abc" and causes /requestKYCNonce to accept invalid accounts; update
validation so only valid 20-byte hex addresses are allowed: modify toHexAddress
(or add a short validator used by /requestKYCNonce) to normalize XDC prefixes to
0x and then require the result match /^0x[0-9a-f]{40}$/i (return empty string or
falsy on failure), and use that stricter check in the /requestKYCNonce handler
before creating IpfsNonce documents to reject malformed accounts up-front.
In `@apis/voters.js`:
- Around line 209-237: The /verifyTx handler (router.post('/verifyTx')) is
calling req.body.amount.replace(...) without ensuring amount is a string; add
validation and/or coercion to avoid TypeError: ensure the request validation
chain for this route includes check('amount').optional().isString().isLength({
max: 64 }) (or equivalent) so non-string amounts are rejected, and/or coerce
amount to a string before calling replace (e.g.
String(req.body.amount).replace(...)) where the BigNumber conversion is done;
update the code around the amount assignment and BigNumber(...) usage to rely on
the validated/coerced value.
In `@contracts/XDCValidator.sol`:
- Around line 83-108: The constructor currently iterates _candidates and indexes
_caps without checking lengths, causing an out-of-bounds revert if _caps.length
< _candidates.length; add an explicit require at the start of the constructor
that validates _candidates.length == _caps.length (and optionally > 0 if
desired) so failures are immediate and clear; update the constructor
(referencing the constructor, _candidates, _caps, and the loop that sets
validatorsState and voters) to perform this check before any state mutations or
the for-loop.
- Around line 194-200: The withdraw() currently uses delete
withdrawsState[msg.sender].blockNumbers[_index] which leaves a zero sentinel and
lets the array grow with zeros; update withdraw() to perform a swap-and-pop on
withdrawsState[msg.sender].blockNumbers: read the array, if _index is not the
last index assign the last element into blockNumbers[_index], then pop the last
element (and only delete the mapping slot if needed), ensuring you handle the
case where the array length is 1; reference withdraw(),
withdrawsState[msg.sender].blockNumbers, and onlyValidWithdraw to locate and
modify the logic so getWithdrawBlockNumbers() no longer accumulates zero
entries.
---
Nitpick comments:
In `@Dockerfile`:
- Line 23: Replace the non-deterministic install command "RUN npm install
--legacy-peer-deps --ignore-scripts" with "RUN npm ci --legacy-peer-deps
--ignore-scripts" to use the lockfile for reproducible installs; update the
other identical RUN npm install occurrences in the Dockerfile as well and ensure
a package-lock.json is present in the build context so npm ci can run
successfully.
In `@index.js`:
- Around line 178-179: Remove the duplicate registration of the error middleware
so there is only one app.use(require('./middlewares/error')) call; locate the
two occurrences that register the middleware (the earlier
app.use(require('./middlewares/error')) after route/middleware setup and the
later identical call) and delete the later one to avoid dead/duplicate
error-handler registration and keep the single error handler
(require('./middlewares/error')) as the canonical final error middleware.
In `@middlewares/error.js`:
- Around line 26-28: The logger currently records only the sanitized message in
the warning (see the logger.warn call in middlewares/error.js), so capture and
log the full error object/stack there while continuing to send only the
sanitized message to the client; update the logger.warn invocation to include
err and/or err.stack (or the full error as a structured field) alongside
req.method and req.originalUrl so you get full-fidelity logs for post-mortem
debugging but leave the response body using only the sanitized message.
In `@package.json`:
- Line 84: Update the obsolete "uuid" dependency from "^3.3.2" to a current
stable version (e.g. "^14.0.0") in package.json and run install; then search the
codebase for legacy deep-imports like "require('uuid/v4')" or "from 'uuid/v4'"
and replace them with the modern named-export usage (e.g. import/require the
package and use the named v4 export such as v4 or uuidv4) so generation calls
keep working after the upgrade.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2549c617-7607-46fb-9f6f-3f5c29a1f601
⛔ Files ignored due to path filters (4)
build/app.d6ee356cd701e09ed0fb.js.mapis excluded by!**/*.mapbuild/node-vendor.d4d835193df6f3eb9196.js.mapis excluded by!**/*.mapbuild/runtime.04647efee1a41c5e77ac.js.mapis excluded by!**/*.mapbuild/style.9b2a3757cae03d13690f.js.mapis excluded by!**/*.map
📒 Files selected for processing (46)
.env.example.gitignoreDockerfileSECURITY_FIXES.mdapis/auth.jsapis/candidates.jsapis/index.jsapis/ipfs.jsapis/voters.jsapp/app.jsapp/components/Setting.vueapp/components/candidates/Apply.vuebuild/app.51a7fb9d79b73238fbbd.jsbuild/app.d6ee356cd701e09ed0fb.jsbuild/index.htmlbuild/node-vendor.6a24ea3423f3be77c51c.js.LICENSE.txtbuild/node-vendor.8099f31c7c5e3ac37d5a.jsbuild/node-vendor.8099f31c7c5e3ac37d5a.js.LICENSE.txtbuild/node-vendor.d4d835193df6f3eb9196.jsbuild/runtime.04647efee1a41c5e77ac.jsbuild/runtime.f6886b9416edacf69bd3.jsbuild/style.85c65ce027f58e54d212.jsbuild/style.9b2a3757cae03d13690f.jsbuild/vendor.d9e3b7255f6258bc8ca1.jsbuild/vendor.dc6e866f897f3bdbedc9.jsconfig/default.jsonconfig/local.jsoncontracts/BlockSigner.solcontracts/Migrations.solcontracts/XDCRandomize.solcontracts/XDCValidator.solcontracts/libs/SafeMath.solhelpers.jshelpers/rateLimiters.jsindex.jsmiddlewares/error.jsmodels/mongodb/index.jsmodels/mongodb/ipfsNonce.jsmodels/mongodb/signature.jspackage.jsonsslcert/README.mdsslcert/server.crtsslcert/server.keytest-harness/soak.shtravis.pem.enctruffle-config.js
💤 Files with no reviewable changes (4)
- sslcert/server.crt
- sslcert/server.key
- build/vendor.d9e3b7255f6258bc8ca1.js
- build/runtime.04647efee1a41c5e77ac.js
| "db": { | ||
| "uri": "mongodb://mongodb:27017/governance" | ||
| }, | ||
| "_comment_db": "Prefer setting MONGO_URI environment variable for authenticated connections (e.g. MONGO_URI=mongodb://user:pass@host:port/db)", |
There was a problem hiding this comment.
Make MONGO_URI mandatory in production instead of advisory-only.
Line 33 is documentation-only; it does not prevent production from using db.uri fallback. Enforce fail-fast behavior in runtime config loading for production environments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/default.json` at line 33, The runtime configuration currently allows
falling back to db.uri when MONGO_URI is absent; change the config loader to
fail fast in production by validating process.env.MONGO_URI and throwing or
exiting with a clear error if it's missing when NODE_ENV === "production".
Locate the configuration bootstrap/loader (e.g., functions named loadConfig,
getDatabaseUri, or the module that reads config/default.json and process.env)
and add a check that enforces presence of MONGO_URI in production, preventing
use of db.uri fallback and surfacing an explicit error message indicating
MONGO_URI is required in production.
Comment on lines
175
to
192
| function resign(address _candidate) public onlyOwner(_candidate) onlyCandidate(_candidate) { | ||
| validatorsState[_candidate].isCandidate = false; | ||
| candidateCount = candidateCount.sub(1); | ||
| candidateCount = candidateCount - 1; | ||
| for (uint256 i = 0; i < candidates.length; i++) { | ||
| if (candidates[i] == _candidate) { | ||
| delete candidates[i]; | ||
| break; | ||
| } | ||
| } | ||
| uint256 cap = validatorsState[_candidate].voters[msg.sender]; | ||
| validatorsState[_candidate].cap = validatorsState[_candidate].cap.sub(cap); | ||
| validatorsState[_candidate].cap = validatorsState[_candidate].cap - cap; | ||
| validatorsState[_candidate].voters[msg.sender] = 0; | ||
| // refunding after resigning X blocks | ||
| uint256 withdrawBlockNumber = candidateWithdrawDelay.add(block.number); | ||
| withdrawsState[msg.sender].caps[withdrawBlockNumber] = withdrawsState[msg.sender].caps[withdrawBlockNumber].add(cap); | ||
| uint256 withdrawBlockNumber = candidateWithdrawDelay + block.number; | ||
| withdrawsState[msg.sender].caps[withdrawBlockNumber] = withdrawsState[msg.sender].caps[withdrawBlockNumber] + cap; | ||
| withdrawsState[msg.sender].blockNumbers.push(withdrawBlockNumber); | ||
| emit Resign(msg.sender, _candidate); | ||
| } |
There was a problem hiding this comment.
delete candidates[i] leaves a zero-address hole; the candidates array grows unboundedly and the linear scan becomes increasingly expensive.
In Solidity 0.8.x, delete array[i] zeroes the slot but does not reduce the array length. After each resign():
getCandidates()returnsaddress(0)entries alongside live candidates, breaking any caller that iterates the result.- The
candidates.lengthdiverges fromcandidateCount. - The
resign()search loop is O(N) over an array that only ever grows, so gas cost increases monotonically with network activity.
The idiomatic fix is swap-and-pop, which maintains a compact array at O(1) removal cost.
🛠️ Proposed fix — swap-and-pop in `resign()`
- for (uint256 i = 0; i < candidates.length; i++) {
- if (candidates[i] == _candidate) {
- delete candidates[i];
- break;
- }
- }
+ for (uint256 i = 0; i < candidates.length; i++) {
+ if (candidates[i] == _candidate) {
+ candidates[i] = candidates[candidates.length - 1];
+ candidates.pop();
+ break;
+ }
+ }Note: swap-and-pop changes the order of the candidates array. If ordering is a protocol invariant (e.g., the on-chain order determines validator slot assignment), the loop should instead shift elements left and pop, at higher gas cost. Verify the XDC consensus layer's ordering requirement before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/XDCValidator.sol` around lines 175 - 192, The resign()
implementation currently uses delete candidates[i], leaving zero-address holes
and causing unbounded growth and expensive scans; replace that with
swap-and-pop: inside resign() when you find index i where candidates[i] ==
_candidate, set candidates[i] = candidates[candidates.length - 1] (if i !=
lastIndex), then call candidates.pop() to shrink the array, and then break;
ensure candidateCount is decremented as already done, and handle the case where
the removed element was the last element (just pop). Update only the candidates
array manipulation in resign() (function name: resign, variable: candidates) to
perform swap-and-pop so the array stays compact and removals are O(1).
Comment on lines
+13
to
+18
| /** | ||
| * SECURITY WARNING (Audit Finding C-3): | ||
| * This provider stores the mnemonic and derived private keys in memory. | ||
| * This is insecure if the browser environment is compromised (e.g. via XSS). | ||
| * The 'PrivateKey/MNEMONIC' login path has been disabled in the UI to mitigate this. | ||
| */ |
There was a problem hiding this comment.
Add an enforceable runtime block, not only a warning comment.
Line 13–18 documents the risk, but HDWalletProvider is still usable/exported. Add a hard guard (at least in production) to prevent accidental reintroduction of mnemonic/private-key login flows.
🔒 Suggested hard guard
function HDWalletProvider (
mnemonic,
provider_url,
address_index = 0,
num_addresses = 1,
wallet_hdpath = "m/44'/551'/0'/0/"
) {
+ if (process.env.NODE_ENV === 'production') {
+ throw new Error('HDWalletProvider is disabled in production due to in-memory key exposure risk')
+ }
this.mnemonic = mnemonic🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helpers.js` around lines 13 - 18, The comment warns but doesn't prevent use
of HDWalletProvider; add a runtime guard around the exported HDWalletProvider in
helpers.js so it cannot be constructed/used in unsafe runtimes (e.g., if
process.env.NODE_ENV === 'production' or when a new env var like
DISABLE_MNEMONIC_LOGIN is true) — implement this by replacing the direct
export/constructor reference to HDWalletProvider with a small guard wrapper that
throws an explicit error (including guidance to use safer auth) when attempted
in production/when the env flag is set, otherwise returns/forwards to the
original HDWalletProvider; reference the HDWalletProvider symbol and the export
in helpers.js so callers fail fast instead of silently allowing mnemonic usage.
Comment on lines
+73
to
75
| // helmet v3 expects `true` (or omit the key) to emit this directive. | ||
| ...(IS_PRODUCTION ? { upgradeInsecureRequests: true } : {}) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the installed helmet version and confirm whether `true` is a valid
# upgradeInsecureRequests value for that version.
fd -t f -d 3 'package.json' | xargs -I{} sh -c '
echo "--- {} ---"
jq -r ".dependencies.helmet // .devDependencies.helmet // empty" "{}"
' 2>/dev/null
fd -t f -d 4 'package-lock.json' | head -1 | xargs -I{} jq -r '.packages | to_entries | map(select(.key | test("node_modules/helmet$"))) | .[].value.version' "{}" 2>/dev/null
fd -t f -d 4 'yarn.lock' | head -1 | xargs -I{} grep -A1 '^"helmet@\|^helmet@' "{}" 2>/dev/nullRepository: XinFinOrg/MasterNode-App
Length of output: 1929
🌐 Web query:
What value does helmet 4.x and helmet 5.x accept for the upgradeInsecureRequests CSP directive?
💡 Result:
Helmet 4.x and 5.x accept the same values for the upgradeInsecureRequests (or "upgrade-insecure-requests") CSP directive: an empty array [] to enable the directive (emits "upgrade-insecure-requests" with no value), null to disable it, or omission (inherits default empty array) [1][2][3][4][5]. Non-empty arrays of strings are also possible but not standard for this directive, which takes no arguments [6][7]. This behavior is consistent across versions, including the current latest (8.1.0 as of 2025), with no breaking changes to this directive in 5.x [8][9][10][4]. Official docs show examples like upgradeInsecureRequests: [] for enabling and "upgrade-insecure-requests": null for disabling in development [2][5].
Citations:
- 1: https://www.npmjs.com/package/helmet/v/4.0.0
- 2: https://helmetjs.github.io/
- 3: https://github.com/helmetjs/helmet/blob/master/middlewares/content-security-policy/README.md
- 4: https://www.npmjs.com/package/helmet?activeTab=versions
- 5: https://github.com/helmetjs/helmet/blob/main/middlewares/content-security-policy/README.md
- 6: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy/upgrade-insecure-requests
- 7: https://www.npmjs.com/package/helmet-csp
- 8: https://github.com/helmetjs/helmet/blob/main/CHANGELOG.md
- 9: https://github.com/helmetjs/helmet/blob/20fae0d8225783fd53449a782cf92901ec201ea6/CHANGELOG.md
- 10: https://www.npmjs.com/package/helmet/v/5.0.0?activeTab=readme
Use upgradeInsecureRequests: [] instead of true — helmet ≥4 rejects true as invalid.
The upgradeInsecureRequests CSP directive in helmet 4+ requires an empty array [], not a boolean. Helmet will throw TypeError: directiveValue is not iterable at startup when given true (the code iterates over the value). This branch executes only in production, so the crash occurs on boot in the live environment.
The empty array form [] is the canonical directive syntax across all modern helmet versions and works safely across helmet-csp v2/v3+.
Fix
- // helmet v3 expects `true` (or omit the key) to emit this directive.
- ...(IS_PRODUCTION ? { upgradeInsecureRequests: true } : {})
+ ...(IS_PRODUCTION ? { upgradeInsecureRequests: [] } : {})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // helmet v3 expects `true` (or omit the key) to emit this directive. | |
| ...(IS_PRODUCTION ? { upgradeInsecureRequests: true } : {}) | |
| } | |
| ...(IS_PRODUCTION ? { upgradeInsecureRequests: [] } : {}) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@index.js` around lines 73 - 75, The CSP config uses upgradeInsecureRequests:
true which breaks helmet >=4; update the production branch where the object
spread uses IS_PRODUCTION to set upgradeInsecureRequests to an empty array ([])
instead of true so the directive is iterable. Locate the object that
conditionally spreads ...(IS_PRODUCTION ? { upgradeInsecureRequests: true } :
{}) in index.js and replace the boolean value with an empty array, ensuring the
rest of the CSP object structure remains unchanged.
Comment on lines
+56
to
+72
| ## Known remaining work (not in this branch) | ||
|
|
||
| These were out of scope for a minimally-invasive security fix PR but should be | ||
| scheduled as follow-ups: | ||
|
|
||
| - **C-1 / C-3 (private key & mnemonic entered in browser, HDWalletProvider in | ||
| memory).** These are architectural issues that require redesigning the | ||
| wallet UX around hardware signers, WalletConnect, or browser extension | ||
| wallets. Cannot be fixed without UI rework. | ||
| - **C-4 (Solidity 0.4.21 in `contracts/XDCValidator.sol`).** Requires a | ||
| contract upgrade path and on-chain migration. | ||
| - **Nonce-based CSP.** The current CSP still allows `'unsafe-inline'` for | ||
| scripts and styles. Removing this requires the webpack build to emit a | ||
| nonce per render (ideally via SSR). Tracked as a follow-up task. | ||
| - **Transitive `npm audit` issues** (206 advisories from deep deps). Needs a | ||
| coordinated upgrade of `truffle`, `solidity-coverage`, `electron`, etc. | ||
|
|
There was a problem hiding this comment.
Stale "Known remaining work" entry: C-4 was resolved in this same PR.
Line 65–66 lists C-4 (XDCValidator.sol Solidity 0.4.21 upgrade) as still outstanding, but the "Final Security Hardening" table on line 201 explicitly documents C-4 as completed (row 18: contracts upgraded to 0.8.x). This contradiction may mislead the ops team into scheduling work that is already done.
📝 Proposed fix
-- **C-4 (Solidity 0.4.21 in `contracts/XDCValidator.sol`).** Requires a
- contract upgrade path and on-chain migration.
+- ~~**C-4 (Solidity 0.4.21 in `contracts/XDCValidator.sol`).**~~ *Resolved in
+ the "Final Security Hardening" pass (2026-05-06) — all contracts upgraded to
+ 0.8.x. An on-chain migration/deployment is still required for the live
+ network.*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY_FIXES.md` around lines 56 - 72, Remove or update the stale C-4 entry
in the "Known remaining work" section that claims "C-4 (Solidity 0.4.21 in
`contracts/XDCValidator.sol`)" is outstanding; instead mark it as completed or
delete the bullet, since the "Final Security Hardening" table (row 18) already
documents contracts upgraded to 0.8.x—edit the "Known remaining work" block to
reflect the current state and keep C-4 consistent with the Final Security
Hardening table.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔐 Security & Dependency Updates
⚙️ Upgrades & Improvements
Upgraded Solidity contracts to v0.8.x and successfully compiled with solc 0.8.19 (verified)
Upgraded dependencies:
Summary by CodeRabbit
Release Notes
New Features
Security Fixes
Infrastructure