Skip to content

Conversation

@Adarshkumar0509
Copy link

No description provided.

Copy link

@borhamsakr-cell borhamsakr-cell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review Summary

Risk Level: HIGH

This PR attempts to fix the broken XSS example (issue #239) but introduces a critical bug that breaks parameter passing to the database layer. The changes are incomplete and will not properly persist the website field.


Findings

[High] Incomplete Fix - Website Field Not Extracted from Request

Location: app/routes/profile.js:47-52

Issue: The website field is not destructured from req.body, causing it to be undefined when passed to the update function.

Current Code:

const {
    firstName,
    lastName,
    ssn,
    dob,
    address,
    bankAcc,
    bankRouting
} = req.body;

Impact: The website field will always be undefined, so it cannot be saved to the database.

Required Fix:

const {
    firstName,
    lastName,
    ssn,
    dob,
    address,
    website,  // Add this line
    bankAcc,
    bankRouting
} = req.body;

[High] Parameter Position Mismatch - Breaks Database Persistence

Location: app/routes/profile.js:90

Issue: The profile.updateUser() method signature in app/data/profile-dao.js does not accept a website parameter. Adding website at position 7 shifts all subsequent parameters, causing:

  • website value → assigned to bankAcc parameter
  • bankAcc value → assigned to bankRouting parameter
  • bankRouting value → assigned to callback parameter
  • callback function → passed beyond expected parameters

DAO Signature:

updateUser = (userId, firstName, lastName, ssn, dob, address, bankAcc, bankRouting, callback)

Impact: Banking information (account and routing numbers) will not be saved correctly, potentially causing data corruption. The callback may not execute properly, breaking the update flow.

Required Fix: Update the DAO method signature in app/data/profile-dao.js to accept and persist the website parameter:

this.updateUser = (userId, firstName, lastName, ssn, dob, address, website, bankAcc, bankRouting, callback) => {
    const user = {};
    // ... existing code ...
    if (website) {
        user.website = website;
    }
    // ... rest of the method
}

[High] Missing Website Field in Error Response

Location: app/routes/profile.js:72

Issue: When bank routing validation fails, the website field is added to the render context but was never extracted from req.body (see first finding). This will display undefined in the form.

Impact: User's website input will be lost on validation error, poor user experience and potential data loss.

Fix: Ensure website is properly extracted from req.body as noted in the first finding.


Recommendations

Priority 1 - Required for this PR:

  1. Add website to the req.body destructuring assignment (line 47)
  2. Update app/data/profile-dao.js to accept and persist the website parameter
  3. Test the complete flow: input → validation → persistence → retrieval → display

Priority 2 - Complete the XSS Example Fix:
4. Update app/views/profile.html line 78 to use {{website}} instead of {{firstNameSafeString}} in the href attribute (this completes the XSS demonstration)
5. Verify displayProfile properly encodes the website field (currently at line 26 - this intentionally uses wrong encoding context for training)

Testing Checklist:

  • Website field value is extracted from form submission
  • Website value persists to database correctly
  • Website value displays when profile is reloaded
  • Banking fields (account/routing) still save correctly
  • Error handling preserves website value
  • XSS payload in website field demonstrates the encoding context vulnerability

Context

This PR addresses issue #239 which identified that the website field introduced in PR #203 was not being properly saved or retrieved. The XSS vulnerability using encodeForHTML instead of encodeForURL is intentional as this is a training application (OWASP NodeGoat).

Note: This is a training application designed to demonstrate vulnerabilities, but the database persistence layer should still function correctly to demonstrate the XSS issue.

@Adarshkumar0509
Copy link
Author

hii @borhamsakr-cell, thank you for the review i will fix this issue by tommorow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants