security: re-audit P0/P1 remediations + E2E follow-up (builds on #44-#48)#49
Open
xcantera wants to merge 4 commits intoXinFinOrg:masterfrom
Open
security: re-audit P0/P1 remediations + E2E follow-up (builds on #44-#48)#49xcantera wants to merge 4 commits intoXinFinOrg:masterfrom
xcantera wants to merge 4 commits intoXinFinOrg:masterfrom
Conversation
added 3 commits
April 24, 2026 20:14
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
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThis comprehensive security-hardening PR introduces environment-based configuration, multi-stage Docker builds with Node 20 and a non-root user, refactors QR login and KYC upload workflows to prevent replay attacks via UUID and nonce binding, centralizes rate limiting across APIs, hardens file uploads and headers, removes TLS keys from version control, and sanitizes error messages in production. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Browser
participant Server as Server
participant DB as MongoDB
participant Blockchain as Blockchain
rect rgba(100, 200, 150, 0.5)
Note over User,Blockchain: QR Login Flow with Replay Prevention
User->>Client: Request login QR
Client->>Server: GET /api/auth/generateLoginQR
Server->>DB: Generate UUID, create record
Server-->>Client: Return QR with message "Login id=<UUID> [XDCmaster <timestamp>]"
Client->>User: Display QR code
User->>Client: Scan QR in wallet
Client->>Blockchain: Sign message with private key
Client->>Server: POST /api/auth/verifyLogin {id, message, signature, signer}
Server->>Server: Validate UUID v4 format
Server->>Server: Extract & validate timestamp from message
Server->>Server: Reject if timestamp beyond LOGIN_SIGNATURE_TTL_MS
Server->>Server: Verify signature matches message/signer
Server->>DB: Query existing QR by signedId (idempotent check)
alt QR already used by same signer
Server-->>Client: Return success (idempotent retry)
else First use or different signer
Server->>DB: Upsert QR with signedId, reject if duplicate
Server-->>Client: Return success
end
end
sequenceDiagram
actor User
participant Client as Browser
participant Server as Server
participant DB as MongoDB
participant IPFS as IPFS
participant Signer as Wallet
rect rgba(100, 150, 200, 0.5)
Note over User,Signer: Two-Step KYC Nonce-Bound Protocol
User->>Client: Select KYC PDF file
Client->>Client: Compute SHA-256 file hash
Client->>Server: POST /api/ipfs/requestKYCNonce {account}
Server->>DB: Generate UUID nonce, store as unconsumed
Server-->>Client: Return {nonce, message template with nonce/TTL, fileHashInstructions}
Client->>Signer: Request signature of {nonce, fileHash, account}
Signer-->>Client: Return signed message
Client->>Server: POST /api/ipfs/addKYC {account, nonce, signedMessage, file}
Server->>Server: Validate nonce is UUIDv4
Server->>Server: Compute SHA-256 of uploaded bytes
Server->>Server: Reconstruct expected message from {nonce, fileHash, account}
Server->>Server: Recover signer from signature, verify matches account
Server->>DB: findOneAndUpdate nonce to consumed (atomic, TTL check)
alt Nonce expired or already consumed
Server-->>Client: Reject (reuse prevented)
else Valid & unconsumed
Server->>IPFS: Upload verified file bytes
Server-->>Client: Return {ipfsHash, fileHash}
end
end
sequenceDiagram
participant Client as Client
participant ReadLimiter as Read Limiter
participant AuthLimiter as Auth Limiter
participant Router as Route Handler
participant RateLimitStore as Store
rect rgba(200, 150, 100, 0.5)
Note over Client,RateLimitStore: Centralized Rate Limiting Stack
Client->>ReadLimiter: Request to /api/voters/getList
ReadLimiter->>RateLimitStore: Check request count in window
alt Requests within limit
RateLimitStore-->>ReadLimiter: Allow
ReadLimiter->>Router: Pass through
Router-->>Client: 200 OK
else Limit exceeded
RateLimitStore-->>ReadLimiter: Reject
ReadLimiter-->>Client: 429 Too many requests
end
Client->>AuthLimiter: Request to /api/auth/verifyLogin
AuthLimiter->>RateLimitStore: Check stricter auth window/max
alt Within stricter limits
AuthLimiter->>Router: Pass through
Router-->>Client: 200/401/other
else Limit exceeded
AuthLimiter-->>Client: 429 Too many requests
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 12
🧹 Nitpick comments (9)
build/index.html (1)
1-1: Build artifact — committingbuild/keeps webpack output under version control.Just noting that
build/index.htmland the hashed bundles are regenerated on every production build, so every PR that touches frontend code will produce noisy diffs and merge conflicts here. If the deploy pipeline already runswebpack --mode=production, consider adding/build/to.gitignoreand shipping these artifacts only via CI/CD. Out of scope for this PR — flagging only because.gitignoreis also being updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/index.html` at line 1, The PR accidentally commits generated frontend artifacts (build/index.html and the hashed bundles like runtime.*.js, app.*.js, vendor.*.js) which should not be versioned; remove the entire build/ directory from the repo, add build/ to .gitignore, and update CI/CD to produce and publish these artifacts during the deployment step (so consumers still get runtime.*, node-vendor.*, app.*, style.* and vendor.* bundles) to avoid noisy diffs and merge conflicts.sslcert/README.md (1)
11-14: Add a language identifier to the fenced code block.markdownlint MD040 — also gives nicer rendering on GitHub.
-``` -openssl req -x509 -newkey rsa:4096 -nodes -keyout server.key -out server.crt \ - -days 365 -subj "/CN=localhost" -``` +```bash +openssl req -x509 -newkey rsa:4096 -nodes -keyout server.key -out server.crt \ + -days 365 -subj "/CN=localhost" +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sslcert/README.md` around lines 11 - 14, Update the fenced code block in README.md that contains the openssl command to include a language identifier; replace the opening triple backticks with ```bash so the block starting with "openssl req -x509 -newkey rsa:4096 -nodes -keyout server.key -out server.crt \ ..." is changed to a bash fenced block for proper markdownlint (MD040) and GitHub rendering.helpers/rateLimiters.js (1)
34-38: Deadmessagefield onauthLimiter.Because
buildLimiterinstalls a customhandlerthat always responds with{ status: 429, error: { message: 'Too many requests, please retry later.' } }, themessage: 'Too many authentication attempts.'here is never sent. Either remove it for consistency with the other limiters, or fold it into the handler if you actually want a different copy on this surface.♻️ Suggested cleanup
const authLimiter = buildLimiter({ windowMs: WINDOW_15_MIN, - max: 60, - message: 'Too many authentication attempts.' + max: 60 })Or, if a distinct message is desired, override the handler per-limiter:
-const authLimiter = buildLimiter({ - windowMs: WINDOW_15_MIN, - max: 60, - message: 'Too many authentication attempts.' -}) +const authLimiter = buildLimiter({ + windowMs: WINDOW_15_MIN, + max: 60, + handler: (req, res) => res.status(429).json({ + status: 429, + error: { message: 'Too many authentication attempts.' } + }) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/rateLimiters.js` around lines 34 - 38, The authLimiter config sets message: 'Too many authentication attempts.' which is never used because buildLimiter installs a global custom handler that always returns the standard 429 payload; update authLimiter by either removing the dead message property for consistency with other limiters, or if you need a custom auth-specific message, modify authLimiter to supply a per-limiter handler that returns your custom payload (referencing authLimiter and buildLimiter/handler to locate the code) so the auth message is actually sent.Dockerfile (1)
41-48: Comment and implementation disagree:chown -R node_modulesis exactly what the comment claims to avoid.The block comment on lines 41–44 says you’re using
COPY --chown=...to skip the slow recursive chown overnode_moduleson overlay FS, butnpm installhere runs as root (noUSERswitch yet), sonode_modulesends up root-owned and line 48 then performschown -R masternode:masternode node_modules— the very operation the comment says is being avoided. On a multi-million-filenode_modulesthis is the slow path the comment promises you don’t pay.Two cheap ways to actually realize the optimization:
♻️ Option A — install as the unprivileged user (preferred)
-COPY --from=build --chown=masternode:masternode /app/package*.json ./ -RUN npm install --omit=dev --legacy-peer-deps --ignore-scripts \ - && npm cache clean --force \ - && chown -R masternode:masternode node_modules +COPY --from=build --chown=masternode:masternode /app/package*.json ./ +USER masternode +RUN npm install --omit=dev --legacy-peer-deps --ignore-scripts \ + && npm cache clean --force(Move the existing
USER masternodedirective on line 73 up here, and ensure/appis writable bymasternode— which it already is given themkdir/chownyou do fortmpandsslcert.)♻️ Option B — install in the build stage and copy with --chown
Run
npm prune --production(or a separatenpm ci --omit=dev) in the build stage and copy the resultingnode_moduleswith--chown=masternode:masternode, eliminating the runtimenpm installentirely.If the goal is just code, not perf, then please update the comment to match what the code actually does so the next person isn’t misled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 41 - 48, The comment claims we avoid a recursive chown by using COPY --chown, but the current runtime steps run npm install as root and then run chown -R masternode:masternode node_modules, which negates that optimization; fix by either (A) moving the USER masternode directive up before the runtime RUN npm install so npm install runs as masternode (ensure /app is writable), or (B) perform production install/prune in the build stage and COPY --from=build the node_modules with --chown=masternode:masternode to the final image removing the runtime npm install and chown, and if you intentionally keep the current root-install-then-chown behavior, update the comment to reflect that fact instead of claiming the chown is avoided (referencing RUN npm install, chown -R masternode:masternode node_modules, USER masternode, and COPY --from=build --chown).apis/candidates.js (1)
938-999: UUID validation is doubled — keep one or note why both are needed.
query('id').isUUID(4)on lines 939 and 989 already enforces strict v4 syntax, so the innerisValidUuid(id)checks on lines 952 and 997 only fire if a previous middleware forgets to short-circuit on validator errors. Not harmful — the redundancy gives belt-and-braces — but you can drop the inner check to keep one source of truth, or keep it and add a brief comment that it’s a defense-in-depth guard against future bypasses of the validator stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/candidates.js` around lines 938 - 999, The code redundantly validates UUIDs with both express-validator (query('id').isUUID(4)) and the custom isValidUuid checks inside the handlers; remove the inner runtime checks to keep a single source of truth: delete the isValidUuid(id) guard in the verifyScannedQR route handler (the block referencing isValidUuid(id) around where id is set) and the similar isValidUuid(messId) guard in the getSignature handler, relying on validationResult() from express-validator to enforce format; if you prefer defense-in-depth instead, keep the checks but add a concise comment above the isValidUuid calls stating they are intentional extra guards.index.js (1)
177-185: Same NODE_ENV-shape mismatch on the SPA static-file fallback.
process.env.NODE_ENV === 'development'on line 179 won’t matchdev,test, orlocal— the dev-modeindex.htmlwould only be served whenNODE_ENVis exactly'development'. Not a security issue (it just falls back to./build/index.html), but for consistency consider using the same fail-secure pattern as elsewhere:- if (process.env.NODE_ENV === 'development') { + if (!IS_PRODUCTION) { p = path.resolve(__dirname, 'index.html') } else { p = path.resolve(__dirname, './build', 'index.html') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 177 - 185, The SPA fallback currently serves dev index.html only when process.env.NODE_ENV === 'development', which misses other non-production env names; update the app.get('*') logic (the handler that sets p and calls res.sendFile) to use a fail-secure pattern used elsewhere — either treat anything not equal to 'production' as dev (e.g., process.env.NODE_ENV !== 'production') or explicitly check an allow-list (e.g., ['development','dev','local','test'].includes(process.env.NODE_ENV)) — then resolve p via path.resolve(__dirname, 'index.html') for dev envs and path.resolve(__dirname, './build','index.html') for production before calling res.sendFile(p).LIVE_EXPOSURE_REPORT.md (1)
192-266: Add language identifiers to the fenced code blocks (markdownlint MD040).The plain
```fences on lines 192, 210, 218, 234, and 262 trip MD040 in CI. Most of them are HTTP responses / shell session output — the convention istextorhttp/console:-``` +```http content-security-policy: default-src 'self'; ...-``` +```text 200 200 200 200 200 ...Apply the same to the M-4 / M-6 / reverse-proxy blocks. Pure formatting; no behavior change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LIVE_EXPOSURE_REPORT.md` around lines 192 - 266, Update the Markdown fenced code blocks to include language identifiers to satisfy MD040: change the content-security-policy block to ```http, the back-to-back request status line block (the repeated "200 200 ...") to ```text, the M-4 example request/response blocks (the POST/HTTP 400 and POST/HTTP 406 JSON responses) to ```http, the M-6 OpenAPI route listing block to ```text or ```http (prefer ```http if shown as response body), and the reverse-proxy header block to ```http so CI linting passes; locate these blocks by their contents (e.g., the content-security-policy snippet, the repeated 200 statuses, the POST /api/auth/verifyLogin examples, the /api-docs route list, and the x-powered-by/server headers) and replace the triple-backtick fences accordingly.middlewares/error.js (1)
39-46:sanitizeForClientregex covers the common cases but not every stack-frame shape.The two patterns catch
…/foo.js[:line:col]paths andat funcName ( … )frames, which is the right 80% solution. Things that still pass through:
- Native / anonymous frames:
at Object.<anonymous>(no parens, no path).- V8-style frames without a function name:
at /app/foo.js:12:34(covered by the path regex but the surroundingattext remains).- Windows-style paths (
C:\app\foo.js) — not relevant for the Alpine deployment but easy to forget.For a stricter posture (and given that
err.messagecontent is largely operational and rarely useful to API clients), you could go further and just return a single generic string in production:♻️ Stricter alternative
-function sanitizeForClient (msg) { - if (typeof msg !== 'string') return 'Error' - return msg - .replace(/\/[^\s'"`]+\.(js|ts|vue)(:\d+:\d+)?/g, '') - .replace(/\s+at\s+[^\s]+\s+\([^)]*\)/g, '') - .trim() || 'Error' +// Allowlist of safe, user-actionable messages; everything else collapses to a +// generic string so library internals never reach the client. +const SAFE_MESSAGE = /^[A-Za-z0-9 ,.'!?:;()\-_/]{1,200}$/ +function sanitizeForClient (msg) { + if (typeof msg !== 'string') return 'Error' + const trimmed = msg.trim() + return SAFE_MESSAGE.test(trimmed) ? trimmed : 'Error' +}The current implementation is fine to ship; this is a hardening tightener if you want to close the long tail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middlewares/error.js` around lines 39 - 46, The sanitizeForClient function currently misses some stack-frame shapes; update sanitizeForClient to more aggressively strip remaining stack/frame artifacts by: normalizing and removing Windows-style paths (C:\...), stripping any leading "at ..." lines (including frames like "at Object.<anonymous>" and bare "at /path:line:col"), removing bare function-only frames and any leftover parenthesized sources, and collapsing the result to a safe fallback ('Error'); optionally, if process.env.NODE_ENV === 'production', always return the generic 'Error' string to ensure no internal details leak. Ensure you modify the sanitizeForClient function to apply these broader regexes/normalizations and the optional production shortcut.apis/voters.js (1)
27-45: Useethereumjs-tx's built-ingetChainId()instead of hand-parsing the v byte.
ethereumjs-txv1.3.7 exposesTransaction.prototype.getChainId()which returns0for unprotected transactions (v∈{27,28}) and the EIP-155 chainId otherwise—exactly the semantics yourextractChainIdFromTxfunction recreates. Delegating to the library eliminates the v-byte parsing logic, reducing surface for edge cases (Buffer-vs-string conversion, leading zeros, sign handling) and ensures correctness if the library's internals change.Suggested refactor
let parsedTx try { parsedTx = new EthereumTx(serializedTx) } catch (e) { throw Error('rawTx is not a valid RLP-encoded transaction') } const expectedChainId = parseInt(config.get('blockchain.networkId')) -const txChainId = extractChainIdFromTx(parsedTx) +const txChainId = parsedTx.getChainId() // 0 = unprotected, rejected by the check below if (!txChainId || txChainId !== expectedChainId) { throw Error(`rawTx chainId ${txChainId} does not match expected ${expectedChainId}`) }You can drop
extractChainIdFromTxentirely once migrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/voters.js` around lines 27 - 45, The custom extractChainIdFromTx function duplicates logic already provided by ethereumjs-tx; replace uses of extractChainIdFromTx with the library call Transaction.prototype.getChainId (i.e., call tx.getChainId() on the Transaction instance) and remove the extractChainIdFromTx helper; ensure any callers that expected null/0 semantics map tx.getChainId() results directly (it returns 0 for unprotected txs) and remove Buffer/string parsing and try/catch logic tied to extractChainIdFromTx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 31-32: The .env.example currently sets NODE_ENV=production which
will cause developers who copy it to unintentionally enable production-only
behavior governed by IS_PRODUCTION (HSTS preload, CSP changes, sanitized errors,
stricter rate limits, and Swagger gating); change the example to
NODE_ENV=development, add a short comment that production should be used only
for deployments and that SWAGGER_USER/SWAGGER_PASS must be set when
NODE_ENV=production, and optionally mention how to toggle IS_PRODUCTION for
CI/containers so local dev keeps verbose errors and Swagger access.
In `@apis/auth.js`:
- Around line 59-71: The current verification allows messages that contain
"id=<id>" but omit or fail to parse the `[XDCmaster <ts>]` prefix, letting
attackers bypass the TTL; update the verification in the function that checks
`message`, `id` and `tsMatch` so that the `[XDCmaster <timestamp>]` prefix is
required and its timestamp must parse to a number within LOGIN_SIGNATURE_TTL_MS
(i.e., if `tsMatch` is missing or Date.parse(ts) is NaN or outside the TTL,
throw an error like 'login signature expired/invalid'); ensure the check
enforces the canonical format emitted by `generateLoginQR` (match the exact
prefix and the `id=<id>` occurrence produced by that routine) so only correctly
formatted, fresh signatures pass.
In `@apis/ipfs.js`:
- Around line 129-143: The addKYC route currently reads credentials from
req.query (account, signedMessage, nonce) which can leak secrets; update the
router.post('/addKYC') handler to stop using req.query and only obtain account,
signedMessage and nonce from req.body and the x-kyc-* headers (via
normalizeValue) while keeping the same validation (presence checks,
UUID_V4_REGEX for nonce) and file upload check; adjust any variable
initialization that references req.query to use only req.body || req.headers and
leave normalizeValue, UUID_V4_REGEX and the unauthorized/badRequest branches
unchanged.
- Around line 196-210: The xinFinClient.add call uses an old callback pattern
and must be converted to the Promise/async-iterable API: await/for-await the
result of xinFinClient.add (or addAll) with fileBuffer, not a Node-style
callback; check the returned object fields (cid/path/size) not ipfsHash[0].hash,
defensively verify the result is defined before accessing properties, and wrap
the await in try/catch to return a 500 on failure; ensure uploaded.tempFilePath
is removed in a finally block (or after successful upload) to avoid leaking temp
files; also defer consuming the nonce (the code that atomically consumes the
nonce) until after the IPFS upload succeeds so a failed upload doesn't waste the
nonce. Use the existing symbols xinFinClient.add, fileBuffer,
uploaded.tempFilePath, and fileHash to locate and update the logic.
In `@apis/voters.js`:
- Around line 219-226: The catch block in generateQR currently writes e.message
directly to the response (res.send({...})), bypassing the centralized sanitizer
in middlewares/error.js; change the catch to call next(e) (or rethrow) instead
of sending the raw error so the existing error middleware can sanitize file
paths/stack traces before responding; also scan this file for any other catch
blocks that do res.send({error:{message: e.message}}) and convert them to use
next(e) for consistency with the rest of the routes.
In `@app/components/candidates/Apply.vue`:
- Around line 578-584: Before attempting to hash the uploaded file in Apply.vue
(the block using this.KYC.file.arrayBuffer() and window.crypto.subtle.digest to
compute fileHash), add an explicit check for window.crypto &&
window.crypto.subtle and early-fail with a clear user-facing error/toast (and
return) if absent; this prevents calling window.crypto.subtle.digest when
undefined, avoids consuming the nonce/continuing the flow, and surfaces a
helpful message instead of the generic TypeError.
In `@index.js`:
- Around line 85-99: DEBUG_REQUESTS is using the wrong env check and will be
true in mainnet/testnet/devnet; change its definition to reuse the existing
IS_PRODUCTION flag (invert it) so the request-logging middleware only runs when
not in non-production environments, e.g. derive DEBUG_REQUESTS from
IS_PRODUCTION and REQUEST_TRACE, and optionally make REQUEST_TRACE an explicit
opt-in (check for === '1' rather than !== '0') so the middleware around
logger.debug (the app.use block) only activates when intended.
In `@models/mongodb/index.js`:
- Around line 23-33: Replace the callback-style mongoose.connect(mongoUri, opts,
callback) with the promise-based form: call mongoose.connect(mongoUri, opts) and
handle success/failure with .then/.catch or await inside an async init flow so
connection errors are caught before models load; on failure call
logger.error(...) including the actual error and then process.exit(1). Update
the module bootstrap (or export an async init function) so that mongoose.connect
is awaited (using the same options: useNewUrlParser, useUnifiedTopology,
serverSelectionTimeoutMS) and ensure any code that registers/uses models runs
only after the awaited connect resolves.
In `@models/mongodb/ipfsNonce.js`:
- Around line 13-20: The IpfsNonce schema's nonce field should be made required
and the redundant index removed to avoid duplicate-index warnings and prevent
null-value uniqueness collisions: update the Schema definition for IpfsNonce so
the nonce property includes required: true and removes index: true (keep unique:
true), i.e., change the nonce field in the IpfsNonce schema to { type: String,
unique: true, required: true } to ensure validation rejects missing nonces and
eliminate the duplicate schema index warning.
In `@models/mongodb/signature.js`:
- Around line 8-11: The schema change to make signedId sparse won't update
existing MongoDB indexes; add a one-shot migration that drops the old signedId_1
index on the signatures collection and then calls
mongoose.model('Signature').syncIndexes() (run-once during deployment) so
MongoDB recreates the index as unique+sparse; implement the drop-and-sync logic
using the db.collection('signatures').dropIndex('signedId_1').catch(() => {})
pattern and then call syncIndexes(), and remove redundant index: true flags from
the signedAddress and signedId field definitions in the Signature model to avoid
duplicate/unused index declarations.
In `@SECURITY_FIXES.md`:
- Line 20: The table row in SECURITY_FIXES.md is broken by an unescaped pipe
inside the inline code span `v=27|28`; update that cell to escape the pipe so
GFM doesn't treat it as a column separator—for example replace the inline code
content with a safe alternative such as using the HTML entity or escaping the
pipe (e.g., `v=27|28` or `v=27\|28`) in the same cell; ensure the rest of
the row text referencing `/verifyTx` and `apis/voters.js` remains unchanged so
the table renders with the correct four columns.
In `@test-harness/soak.sh`:
- Line 65: Update the misleading echo in soak.sh so the 429 count is attributed
to readLimiter (not authLimiter); change the message for the variable limited to
reference readLimiter and optionally txLimiter (if /api/voters/* is included)
and list the endpoints under test (/api/config, /, /api/candidates,
/api/transactions) to make it clear where the 429s originate; locate the echo
line that prints "429 : $limited ..." and update its text to mention
readLimiter (and txLimiter only as a conditional note).
---
Nitpick comments:
In `@apis/candidates.js`:
- Around line 938-999: The code redundantly validates UUIDs with both
express-validator (query('id').isUUID(4)) and the custom isValidUuid checks
inside the handlers; remove the inner runtime checks to keep a single source of
truth: delete the isValidUuid(id) guard in the verifyScannedQR route handler
(the block referencing isValidUuid(id) around where id is set) and the similar
isValidUuid(messId) guard in the getSignature handler, relying on
validationResult() from express-validator to enforce format; if you prefer
defense-in-depth instead, keep the checks but add a concise comment above the
isValidUuid calls stating they are intentional extra guards.
In `@apis/voters.js`:
- Around line 27-45: The custom extractChainIdFromTx function duplicates logic
already provided by ethereumjs-tx; replace uses of extractChainIdFromTx with the
library call Transaction.prototype.getChainId (i.e., call tx.getChainId() on the
Transaction instance) and remove the extractChainIdFromTx helper; ensure any
callers that expected null/0 semantics map tx.getChainId() results directly (it
returns 0 for unprotected txs) and remove Buffer/string parsing and try/catch
logic tied to extractChainIdFromTx.
In `@build/index.html`:
- Line 1: The PR accidentally commits generated frontend artifacts
(build/index.html and the hashed bundles like runtime.*.js, app.*.js,
vendor.*.js) which should not be versioned; remove the entire build/ directory
from the repo, add build/ to .gitignore, and update CI/CD to produce and publish
these artifacts during the deployment step (so consumers still get runtime.*,
node-vendor.*, app.*, style.* and vendor.* bundles) to avoid noisy diffs and
merge conflicts.
In `@Dockerfile`:
- Around line 41-48: The comment claims we avoid a recursive chown by using COPY
--chown, but the current runtime steps run npm install as root and then run
chown -R masternode:masternode node_modules, which negates that optimization;
fix by either (A) moving the USER masternode directive up before the runtime RUN
npm install so npm install runs as masternode (ensure /app is writable), or (B)
perform production install/prune in the build stage and COPY --from=build the
node_modules with --chown=masternode:masternode to the final image removing the
runtime npm install and chown, and if you intentionally keep the current
root-install-then-chown behavior, update the comment to reflect that fact
instead of claiming the chown is avoided (referencing RUN npm install, chown -R
masternode:masternode node_modules, USER masternode, and COPY --from=build
--chown).
In `@helpers/rateLimiters.js`:
- Around line 34-38: The authLimiter config sets message: 'Too many
authentication attempts.' which is never used because buildLimiter installs a
global custom handler that always returns the standard 429 payload; update
authLimiter by either removing the dead message property for consistency with
other limiters, or if you need a custom auth-specific message, modify
authLimiter to supply a per-limiter handler that returns your custom payload
(referencing authLimiter and buildLimiter/handler to locate the code) so the
auth message is actually sent.
In `@index.js`:
- Around line 177-185: The SPA fallback currently serves dev index.html only
when process.env.NODE_ENV === 'development', which misses other non-production
env names; update the app.get('*') logic (the handler that sets p and calls
res.sendFile) to use a fail-secure pattern used elsewhere — either treat
anything not equal to 'production' as dev (e.g., process.env.NODE_ENV !==
'production') or explicitly check an allow-list (e.g.,
['development','dev','local','test'].includes(process.env.NODE_ENV)) — then
resolve p via path.resolve(__dirname, 'index.html') for dev envs and
path.resolve(__dirname, './build','index.html') for production before calling
res.sendFile(p).
In `@LIVE_EXPOSURE_REPORT.md`:
- Around line 192-266: Update the Markdown fenced code blocks to include
language identifiers to satisfy MD040: change the content-security-policy block
to ```http, the back-to-back request status line block (the repeated "200 200
...") to ```text, the M-4 example request/response blocks (the POST/HTTP 400 and
POST/HTTP 406 JSON responses) to ```http, the M-6 OpenAPI route listing block to
```text or ```http (prefer ```http if shown as response body), and the
reverse-proxy header block to ```http so CI linting passes; locate these blocks
by their contents (e.g., the content-security-policy snippet, the repeated 200
statuses, the POST /api/auth/verifyLogin examples, the /api-docs route list, and
the x-powered-by/server headers) and replace the triple-backtick fences
accordingly.
In `@middlewares/error.js`:
- Around line 39-46: The sanitizeForClient function currently misses some
stack-frame shapes; update sanitizeForClient to more aggressively strip
remaining stack/frame artifacts by: normalizing and removing Windows-style paths
(C:\...), stripping any leading "at ..." lines (including frames like "at
Object.<anonymous>" and bare "at /path:line:col"), removing bare function-only
frames and any leftover parenthesized sources, and collapsing the result to a
safe fallback ('Error'); optionally, if process.env.NODE_ENV === 'production',
always return the generic 'Error' string to ensure no internal details leak.
Ensure you modify the sanitizeForClient function to apply these broader
regexes/normalizations and the optional production shortcut.
In `@sslcert/README.md`:
- Around line 11-14: Update the fenced code block in README.md that contains the
openssl command to include a language identifier; replace the opening triple
backticks with ```bash so the block starting with "openssl req -x509 -newkey
rsa:4096 -nodes -keyout server.key -out server.crt \ ..." is changed to a bash
fenced block for proper markdownlint (MD040) and GitHub rendering.
🪄 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: a330a2e8-fbb9-466d-9cc2-9388febe62df
⛔ 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 (38)
.env.example.gitignoreDockerfileLIVE_EXPOSURE_REPORT.mdSECURITY_FIXES.mdapis/auth.jsapis/candidates.jsapis/index.jsapis/ipfs.jsapis/voters.jsapp/app.jsapp/components/Setting.vueapp/components/candidates/Apply.vuebuild/app.b6d16146611612e84979.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.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.enc
💤 Files with no reviewable changes (4)
- sslcert/server.crt
- sslcert/server.key
- build/vendor.d9e3b7255f6258bc8ca1.js
- build/runtime.04647efee1a41c5e77ac.js
Comment on lines
+8
to
+11
| // unique+sparse keeps the lateral-takeover fix atomic: once any wallet has | ||
| // bound itself to a signedId the underlying unique-index error fires for | ||
| // every other concurrent wallet trying to claim the same QR session. | ||
| signedId: { type: String, unique: true, sparse: true, index: true }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no migration script for the signedId index ships in this PR.
fd -t f -e js -e ts . | xargs rg -nP '(syncIndexes|dropIndex).*signedId|signedId.*sparse' 2>/dev/null
rg -nP 'syncIndexes\s*\(' --type=js -C2Repository: XinFinOrg/MasterNode-App
Length of output: 172
🏁 Script executed:
# Check for migration files or scripts
fd -t f \( -name '*migrat*' -o -name '*index*' \) . | head -20Repository: XinFinOrg/MasterNode-App
Length of output: 299
🏁 Script executed:
# Look for SECURITY_FIXES.md or similar documentation
fd -t f \( -name 'SECURITY*' -o -name 'CHANGELOG*' -o -name 'MIGRATIONS*' \) .Repository: XinFinOrg/MasterNode-App
Length of output: 299
🏁 Script executed:
# Read the full signature.js schema context
wc -l models/mongodb/signature.jsRepository: XinFinOrg/MasterNode-App
Length of output: 99
🏁 Script executed:
# Search for syncIndexes or index management in the entire codebase
rg -n 'syncIndexes|ensureIndex|createIndex|dropIndex' -t js -t ts --max-count=20Repository: XinFinOrg/MasterNode-App
Length of output: 50
🏁 Script executed:
# Check if there are any initialization or startup scripts that manage indexes
rg -n 'Signature.*index|index.*Signature' -t js -t ts -C2 --max-count=15Repository: XinFinOrg/MasterNode-App
Length of output: 50382
🏁 Script executed:
# Read the full signature.js file
cat -n models/mongodb/signature.jsRepository: XinFinOrg/MasterNode-App
Length of output: 937
🏁 Script executed:
# Search for migration or script directories
find . -type d \( -name migration -o -name migrations -o -name scripts -o -name db \) 2>/dev/null | head -20Repository: XinFinOrg/MasterNode-App
Length of output: 137
🏁 Script executed:
# More targeted search for index operations in the codebase
rg 'dropIndex|syncIndexes|createIndex' --type js --type ts 2>/dev/null | head -20Repository: XinFinOrg/MasterNode-App
Length of output: 50
🏁 Script executed:
# Check git log to understand what changed in signature.js
git log --oneline models/mongodb/signature.js 2>/dev/null | head -10Repository: XinFinOrg/MasterNode-App
Length of output: 156
🏁 Script executed:
# List migration files
ls -la migrations/ 2>/dev/null || echo "migrations directory not readable"Repository: XinFinOrg/MasterNode-App
Length of output: 326
🏁 Script executed:
# Search for migration files by pattern
find migrations -type f \( -name '*.js' -o -name '*.json' \) 2>/dev/null | head -20Repository: XinFinOrg/MasterNode-App
Length of output: 135
🏁 Script executed:
# Check mongoose configuration for autoIndex settings
rg 'autoIndex|useCreateIndex|collection.*index' -t js -t ts --max-count=15Repository: XinFinOrg/MasterNode-App
Length of output: 50382
🏁 Script executed:
# Search for any index-related initialization or schema setup
rg 'Signature.*Schema|Signature.*model|syncIndexes|ensureIndexes' -t js -t ts --max-count=15Repository: XinFinOrg/MasterNode-App
Length of output: 50382
Production needs an explicit index migration — schema change alone won't flip existing index to sparse.
mongoose 5.x will not modify an already-existing index in MongoDB to add sparse: true; it only creates indexes that don't yet exist (and only when useCreateIndex is on, which is set in models/mongodb/index.js). On any environment that already ran with the previous non-sparse definition, the live signedId_1 index will remain non-sparse, so:
- The lateral-takeover comment's atomicity guarantee actually relies on the old non-sparse unique index (which still rejects duplicate non-null values), but
- Any historical row with
signedId: null(or any future doc inserted beforesignedIdis set) will now collide on the unique constraint, which the new schema implies should be allowed.
No MongoDB migration script exists in the repository. Ship a one-shot migration alongside this PR (drop signedId_1, then let mongoose / syncIndexes() recreate it as unique+sparse) and document it in SECURITY_FIXES.md:
await db.collection('signatures').dropIndex('signedId_1').catch(() => {})
await mongoose.model('Signature').syncIndexes()Minor: with unique: true the explicit index: true is redundant on both signedAddress and signedId and can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/mongodb/signature.js` around lines 8 - 11, The schema change to make
signedId sparse won't update existing MongoDB indexes; add a one-shot migration
that drops the old signedId_1 index on the signatures collection and then calls
mongoose.model('Signature').syncIndexes() (run-once during deployment) so
MongoDB recreates the index as unique+sparse; implement the drop-and-sync logic
using the db.collection('signatures').dropIndex('signedId_1').catch(() => {})
pattern and then call syncIndexes(), and remove redundant index: true flags from
the signedAddress and signedId field definitions in the Signature model to avoid
duplicate/unused index declarations.
… 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
Author
Follow-up commit
|
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
1 participant
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.
Context
This PR is the result of an external security re-audit of
master.xinfin.network/MasterNode-Appperformed 2026-04-24, followed by an end-to-end local reproduction pass on 2026-04-27.It builds strictly on top of the security work already merged in #44, #45, #46, #47, #48. Every helper, allowlist, signer-recovery shim, fallback header, CSP directive, and CORS handler that those PRs introduced is preserved — this PR adds another layer rather than rewriting them. Where one of those layers had a residual issue we found during reproduction, the change is described explicitly below.
We're submitting this publicly because the fork has been published while the audit was running, the underlying fixes (login QR replay, KYC file substitution, CSP downgrade, MongoDB without auth, etc.) overlap with the recently merged hardening work, and there is no
SECURITY.md/ private security advisory channel set up on the repo. If you'd prefer the rest of the audit notes (SECURITY_FIXES.md,LIVE_EXPOSURE_REPORT.md) handled privately, happy to coordinate offline before merge — let me know.What's preserved from upstream
index.jsframeguard: deny,referrerPolicy: strict-origin-when-cross-origin,hidePoweredBy, base CSPindex.jsconnectSrc/imgSrcfor runtime needsindex.jshelmet({...})structure, Google Analytics origins (scriptSrc/imgSrc/connectSrc)index.jsapis/candidates.jsALLOWED_SCHEMESURL allowlist,validateUrl(...),web3.eth.accounts.recoverowner check, signed-message + nonce update flowapis/candidates.jsapis/voters.jsALLOWED_SORT_FIELDS+normalizeSortFieldapis/ipfs.jstoHexAddress,normalizeValue,unauthorized, multi-format signer recovery (plain / utf8ToHex), `x-kyc-accountapp/components/candidates/Apply.vuesignerAccountxdc→0x normalisation,personal.signwitheth.signfallback,x-kyc-*headersWhat's new in this PR
Re-audit (P0 / P1) — already merged on this branch in earlier commits
apis/auth.js generateLoginQR); SPA passes it through to/verifyLogin;Setting.vueswitched toencodeURIComponentbecause the message now contains=(would otherwise be split into the wrong query keys by mobile wallets).chainIdbinding on/api/voters/verifyTx: parses the legacy raw txvbyte and rejects unprotected (v in {27,28}) or wrong-chain transactions.middlewares/error.jsreturns generic envelopes in production, full stack only behindIS_DEV_ENV.console.logper request gated behindDEBUG_REQUESTSand routed through winston so client IPs / origins / referers don't hit prod stdout./api-docsrequires HTTP Basic Auth fromSWAGGER_USER/SWAGGER_PASSin production; disabled outright if either is unset.express-fileuploadhardening:useTempFiles, hard 10 MiBfileSize,files: 1,abortOnLimit,safeFileNames,preserveExtension: 4.POST /api/ipfs/requestKYCNonce— mint a uuid bound toaccount, TTL 5 min.[XDCmaster KYC <nonce>] Upload <fileHash> for <account>.POST /api/ipfs/addKYC— server atomically consumes the nonce, recomputessha256(file), reconstructs the message, recovers the signer (still using Fixed the security issues #44's multi-format fallback), and rejects on any mismatch.helpers/rateLimiters.jsapplied to login, signature, and write endpoints (usesapp.set('trust proxy', ...)from env so we don't bucket every real client into the proxy IP).script-srcdrops'unsafe-eval'; HSTS preload +upgrade-insecure-requestsonly whenIS_PRODUCTION. Dev still ships'unsafe-eval'+ws:so webpack-dev-server HMR works.E2E follow-up pass (this commit,
61fad27)Dockerfile:npm install --legacy-peer-deps --ignore-scriptsin both stages (skips thesha3native compile that fails on Node 20); replace the finalchown -R masternode:masternode /appwithCOPY --chown=masternode:masternodeper stage + a tinychownfortmp/sslcert. Was hanging for several minutes on overlay/WSL2 filesystems because of the recursive chown overnode_modules.app/components/Setting.vue:encodeURIComponentfor bothmessageandsubmitURL(see H-2 above).app/components/candidates/Apply.vue:uploadKYC()rewired to the two-step flow described in M-9./verifyLogin: discovered during the live E2E pass. The previous lookup wasfindOneAndUpdate({ signedAddress }), so a second wallet that scanned the victim's QR with their own client could overwrite the same login session and hijack the SPA polling/getLoginResult. Fix:models/mongodb/signature.js:signedIdcarriesunique: true, sparse: true(atomic guarantee under concurrent inserts).apis/auth.js verifyLogin: rejects re-claim by a different signer; treats a same-signer retry as idempotent success.app/app.js: switch the root Vue instance fromtemplate: '<App/>'torender: 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.package.json: moveconnect-flashfromdevDependenciestodependencies. It's required at runtime byindex.js(app.use(flash())) and the production Docker image was crashing on first request without it.Deliberate departure from #44
PR #44 added an
x-api-keyshared-secret bypass on/addKYCfor trusted server-to-server uploads. We dropped that bypass: a single shared key short-circuits all the per-upload signature checks, and a static secret stored in config + travelling in plaintext over a header is a strictly worse attack surface than what the per-request flow defends against. Happy to add it back behindhelmet-style strict opt-in (e.g. only mounted whenconfig.security.ipfsUploadApiKeyis set andprocess.env.NODE_ENV === 'development') if there's a deployment that needs it — please let us know the use case and we'll wire that up in a follow-up commit.Files in this PR
Pre-deploy migration note for ops
The
Signaturecollection's existingsignedId_1index is non-unique. After deploy, drop it and let Mongoose recreate the newunique+sparseone. If you can't take a brief lock window, this works:Verification performed locally
npm install --legacy-peer-deps --ignore-scripts(clean)vue-template-compileronApply.vue+Setting.vue(0 errors)devtool: false, no.mapfiles emitted)/api/auth/*and/api/ipfs/*with real EIP-191 signatures (incl. nonce replay, file-hash mismatch, lateral takeover)/api/voters/verifyTxwith real EIP-155 signed legacy txs (correct chainId, wrong chainId, unprotected v∈{27,28})docker build+docker runagainst local Mongo, smoke-hit/api/configand a rate-limited endpointnpm test(Truffle): pre-existing Node 20 incompatibility (TypeError: Cannot read properties of undefined (reading 'type')) — not introduced by this PR; nocontracts/,test/, ortruffle-config.jsfiles modified.Out of scope
npm auditreports 120 transitive vulnerabilities (42 critical, 38 high), almost all from@walletconnect/*,truffle-hdwallet-provider,elliptic,axios,web3,ethers. Bumping these majors would break the wallet integration contract surface — tracked under "known remaining work" inSECURITY_FIXES.md. Live-exposure analysis (LIVE_EXPOSURE_REPORT.md) shows none are exploitable at the masterchain.xinfin.network surface area, but the dependency upgrade should still be tackled as its own piece of work.Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Security Enhancements
Documentation