-
Notifications
You must be signed in to change notification settings - Fork 220
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 href props with xss vulnerability V2 #1000
Conversation
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.
Looking good so far.
1000th issue / PR! Cool milestone 😄
I think you need a npm run format
src/components/nav/NavLink.js
Outdated
@@ -1,9 +1,11 @@ | |||
import React, {useEffect, useState} from 'react'; | |||
import React, {useEffect, useState, useMemo} from 'react'; |
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.
useMemo
not used in this file afaict
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.
Right - I didn't do npm run format
yet. Still lots of files to change :-)
5ad2be3
to
08e85c6
Compare
08e85c6
to
8b55738
Compare
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.
Great stuff, thanks @AnnMarieW!
I made a few small edits:
- Used
pytest.mark.parameterize
inside the integration tests, I think it makes it a little easier to see what is going on in the test + that the same check is being performed for multiple components. - Some small changes to prop destructuring, e.g. where we pull out
setProps
we no longer need to callomit
and so on.
Sanitize html props that are vulnerable to xss vulnerability if user data is inserted.
This is the new version based on comments in #999
Let's keep this as a draft until plotly/dash#2743 is resolved.