Skip to content

feat(xss-context): add example for context sensitive XSS encoding issues#115

Merged
lirantal merged 2 commits into
OWASP:masterfrom
lirantal:xss/context-sensitive
Sep 9, 2018
Merged

feat(xss-context): add example for context sensitive XSS encoding issues#115
lirantal merged 2 commits into
OWASP:masterfrom
lirantal:xss/context-sensitive

Conversation

@lirantal
Copy link
Copy Markdown
Collaborator

@lirantal lirantal commented Sep 8, 2018

Adding a case where we use the same variable as a URL for a hyperlink. While the variable is encoded for HTML for the input element, it isn't suitable for the URL entry.

owasp node js goat project context sensitive xss

@lirantal lirantal self-assigned this Sep 8, 2018
@binarymist
Copy link
Copy Markdown
Collaborator

Just looking at the file changes alone, I don't see any issues.

@lirantal
Copy link
Copy Markdown
Collaborator Author

lirantal commented Sep 9, 2018

Cool, will go ahead and merge.

@lirantal lirantal merged commit 86d2c6a into OWASP:master Sep 9, 2018
Copy link
Copy Markdown
Member

@ckarande ckarande left a comment

Choose a reason for hiding this comment

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

Minor suggestions about FIXME instructions

Comment thread app/routes/profile.js
doc.firstNameSafeString = ESAPI.encoder().encodeForHTML(doc.firstName)
// fix it by replacing the above with another template variable that is used for
// the context of a URL in a link header
// doc.firstNameSafeString = ESAPI.encoder().encodeForURL(urlInput)
Copy link
Copy Markdown
Member

@ckarande ckarande Sep 10, 2018

Choose a reason for hiding this comment

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

It is explained in the comment lines above, but to be clear about steps to fix, could line 26 use a different variable name, doc.firstNameSafeURLString, for example.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes makes sense!

Comment thread app/views/profile.html
</div>
<input type="hidden" name="_csrf" value="{{csrftoken}}" />
<button type="submit" class="btn btn-default" name="submit">Submit</button>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a comment showing the fix in the template to use the different variable that is escaped for the URL context.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

👍

@ckarande
Copy link
Copy Markdown
Member

@lirantal Thanks for this addition. It highlights the importance of context sensitive encoding. I liked your creativity to come up with a way to demonstrate it on the profile page. If you have a few minutes, can you also add a few lines explaining this issue in the Tutorial as well; otherwise it may go unnoticed by the users.

@lirantal
Copy link
Copy Markdown
Collaborator Author

@ckarande yes of course, I'll open a follow-up PR with all the suggested improvements.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants