Skip to content

fix: example of xss in bad context using a dedicated profile field#203

Merged
lirantal merged 1 commit intoOWASP:masterfrom
lirantal:fix/xss-context
Aug 3, 2020
Merged

fix: example of xss in bad context using a dedicated profile field#203
lirantal merged 1 commit intoOWASP:masterfrom
lirantal:fix/xss-context

Conversation

@lirantal
Copy link
Copy Markdown
Collaborator

@lirantal lirantal commented Jul 9, 2020

Why this PR

The example of using proper context to perform output encoding is incorrect in terms of bad code, and example reference:

  • bad code: line 26 of profile.js referenced a doc.doc.firstName.. but doc.doc is incorrect and never really did work anyway. Also the firstNameSafeString template variable in the view at profile.html was never assigned.
  • example reference: the example relied on the firstName with an attempt to first encode it, and then the FIXME hint to encode it properly for HTML but the problem is that even if you encode it (either way), it still causes an XSS because other views use it, such as the drop-down profile name etc.

This change

This PR fixes the issue by introducing another profile field website that is never used anywhere, and can be mitigated easily with encoding, but if done improperly can still introduce an XSS, and the FIXME clause of proper context encoding then makes sense.

@lirantal lirantal added the bug label Jul 9, 2020
@lirantal lirantal requested review from UlisesGascon and ckarande July 9, 2020 07:05
@lirantal lirantal self-assigned this Jul 9, 2020
@lirantal lirantal merged commit a512124 into OWASP:master Aug 3, 2020
Copy link
Copy Markdown
Collaborator

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Looks great @lirantal! Just update the broken test and we fix the tests 💪

@lirantal
Copy link
Copy Markdown
Collaborator Author

lirantal commented Aug 4, 2020

Thanks!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants