Phase 1: Critical Security Refactors

[devin-ai-integration]

Phase 1: Critical Security Refactors

Summary

This PR implements Phase 1 of the comprehensive security refactoring plan for nodejs-goof, addressing critical security vulnerabilities while maintaining application functionality. The changes include:

1. Updated 7 vulnerable dependencies to fix known security issues (prototype pollution, RCE, XSS, DoS, buffer exposure)
2. Fixed template injection vulnerability in /account_details endpoint via strict field allowlisting
3. Fixed XSS vulnerability in admin.ejs by properly escaping output
4. Fixed open redirect vulnerability by validating redirect URLs are local-only
5. Fixed ReDoS vulnerabilities by removing problematic validator options and replacing unsafe string operations
6. Fixed prototype pollution in chat API by replacing _.merge() with explicit property copying

Dependency Updates

• lodash: 4.17.4 → 4.17.21 (fixes prototype pollution, command injection)
• ejs: 1.0.0 → 3.1.10 (fixes RCE, XSS, DoS vulnerabilities)
• marked: 0.3.5 → 12.0.2 (fixes XSS)
• mongoose: 4.2.4 → 6.12.3 (fixes buffer memory exposure, maintains callback compatibility)
• validator: 13.5.2 → 13.11.0 (latest version with security fixes)
• express-fileupload: 0.0.5 → 1.5.0 (fixes DoS, prototype pollution)
• adm-zip: 0.5.2 → 0.5.16 (latest with Zip Slip fixes)

Note on Mongoose: Upgraded to 6.12.3 instead of 8.x because Mongoose 8 removed callback support, which would require extensive code refactoring beyond Phase 1 scope. Version 6.12.3 includes all critical security fixes while maintaining backward compatibility.

Code Security Fixes

routes/index.js:

• adminLoginSuccess(): Validates redirectPage only accepts local URLs (/ prefix, not //)
• save_account_details(): Implements strict allowlisting of 5 expected fields, adds 1000-char length limits, removes ReDoS-vulnerable allow_display_name option and validator.rtrim()
• chat.add(): Replaces unsafe _.merge() with explicit property copying using Object.create(null) and field allowlisting

views/admin.ejs:

• Changed <%- redirectPage %> to <%= redirectPage %> to properly escape output and prevent XSS

Review & Testing Checklist for Human

⚠️ 3 Critical Items to Verify:

• Test all modified endpoints manually - I verified the app starts successfully, but did NOT manually test the specific endpoints that were changed:

  • POST /account_details with various input payloads (normal, malicious, edge cases)
  • Admin login with different redirectPage values (local paths, external URLs, malicious payloads)
  • Chat API POST with prototype pollution payloads ({"message": {"__proto__": {"polluted": true}}})

• Review Mongoose 6.12.3 vs 8.x trade-off - I chose 6.12.3 to avoid breaking changes with callback-based code throughout the application. Verify this trade-off is acceptable for your security requirements. Mongoose 8.x would require refactoring all database operations to use promises/async-await.

• Verify allowlisted fields don't break legitimate use cases - The save_account_details function now only accepts 5 fields: email, firstname, lastname, country, phone. Confirm no other fields should be accepted. Also verify the 1000-char length limit is reasonable.

Recommended Test Plan

1. Start the application: npm start
2. Test account details endpoint:
   • Navigate to account details page
   • Submit valid profile information → should succeed
   • Try submitting with extra fields like {"layout": "../../malicious"} → should be ignored
3. Test admin login:
   • Try login with redirectPage=/admin → should redirect to /admin
   • Try login with redirectPage=https://evil.com → should redirect to /admin (not external)
4. Test chat API:
   • Send valid message → should work
   • Try prototype pollution payload → should not pollute Object.prototype
5. Run CI checks to see if any unexpected failures occur

Notes

• Application starts successfully with all dependency updates applied
• 127 vulnerabilities still reported by Snyk (expected for this intentionally vulnerable demo app)
• Many remaining vulnerabilities are in dev dependencies (tap, nyc) or transitive dependencies
• Some vulnerabilities (like ejs@0.8.8 in ejs-locals) remain because updating would break compatibility
• This is Phase 1 only - additional phases will address code quality, infrastructure, and testing

Link to Devin run: https://app.devin.ai/sessions/5e811e3857f641d39e6cb78961f1eb7f
Requested by: @bnob-ship-it

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring