-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix xss website property v2 #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix xss website property v2 #362
Conversation
borhamsakr-cell
left a comment
There was a problem hiding this 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:
websitevalue → assigned tobankAccparameterbankAccvalue → assigned tobankRoutingparameterbankRoutingvalue → assigned tocallbackparametercallbackfunction → 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:
- Add
websiteto thereq.bodydestructuring assignment (line 47) - Update
app/data/profile-dao.jsto accept and persist thewebsiteparameter - 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.
|
hii @borhamsakr-cell, thank you for the review i will fix this issue by tommorow |
No description provided.