Skip to content
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

Sanitize keys before confirmation modal. #1558

Merged
merged 1 commit into from Apr 30, 2021
Merged

Sanitize keys before confirmation modal. #1558

merged 1 commit into from Apr 30, 2021

Conversation

bamnet
Copy link
Member

@bamnet bamnet commented Apr 30, 2021

Without sanitization, these are susceptible to an varying degrees of XSS attack depending on the input. When the values are displayed in the twitter-bootstrap-confirmation modal (I don't fully understand how it works) the JS is rendered and executed. The user.name fields are the most concerning for instances that allow public registration, but we sanitize everywhere this pattern is used to be safe.

Kudos to Ben Shaw (@sudonoodle) for finding this vulnerability!

@bamnet bamnet requested a review from zr2d2 April 30, 2021 03:28
@bamnet
Copy link
Member Author

bamnet commented Apr 30, 2021

@zr2d2 mind taking a look and making sure I've not messed too much up?

Copy link
Member

@zr2d2 zr2d2 left a comment

Choose a reason for hiding this comment

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

These changes look good, but should we also be sanitizing user input before we put it into the database? I feel like not storing it in the database would be safer

@bamnet
Copy link
Member Author

bamnet commented Apr 30, 2021

While it would be safest, I considered that wouldn't resolve the problem if anyone had already put some XSS-triggering content in place and was just waiting for the right user, like an admin, to trigger it.

Names may also involve characters that the sanitize() function messes up, maybe bobby; drop tables has a sister <script>

@zr2d2
Copy link
Member

zr2d2 commented Apr 30, 2021

I suppose that's true, it does offer more protection if upgrading an existing deploy

@zr2d2 zr2d2 merged commit 1b34c37 into master Apr 30, 2021
1 check passed
@zr2d2 zr2d2 deleted the xss branch April 30, 2021 23:45
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.

None yet

2 participants