-
Notifications
You must be signed in to change notification settings - Fork 1.9k
JavaScript: Add library for HTML sanitizers #46
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
Conversation
xiemaisi
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.
LGTM, but perhaps could do with a change note. The name based matching is probably fine; it's no worse than what we do for sensitive expressions.
| abstract DataFlow::Node getInput(); | ||
| } | ||
|
|
||
| private class DefaultHtmlSanitizerCall extends HtmlSanitizerCall { |
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.
May be worth adding a comment (even though it's private), explaining that this models popular npm packages as well as home-made sanitizers.
37bcc05 to
8074786
Compare
|
Addressed comments and rebased to avoid conflict in the change log. |
xiemaisi
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.
LGTM
Add `unique` wrapper to `AstNode::getParent()`
Change build to simplify sembuild integration
Add models for composite actions and reusable workflows sinks
Adds
HtmlSanitizerCall, which models a call to a function whose purpose is to sanitize HTML.CodeInjectionhas been updated to use theHtmlSanitizerCallclass for additional taint steps. Unilke the previous version of this PR, no other queries are affected, since it cost more than expected.I ran
CodeInjectionon default slugs to force evaluation ofHtmlSanitizerCall. The performance results are noisy but after re-running the slowest bunch it looks okay.@max you had some concerns about the name-based matching in the standard library. We could move that part back into
CodeInjectionif you prefer.