-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Manipulation: Detect sneaky no-content replaceWith input #2223
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
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.
Or
if ( arg && (arg[ 0 ] || arg.nodeType) && typeof arg !== "string" ) {
return this;
}
// Force removal if there was no new content (e.g., from empty arguments)
return this.remove();Which i actually like better, since remove would not be called as much
|
I believe this will still fail on |
|
@gibson042 could you post an jsfiddle? |
|
Perhaps i didn't get your example, since this - htmlPrefilter = jQuery.htmlPrefilter
jQuery.htmlPrefilter = function( html ) {
// Remove <script> and <del> elements
return htmlPrefilter.apply( this, arguments )
.replace( /<(script|del)(?=[\s>])[\w\W]*?<\/\1\s*>/ig, "" );
};
$("div").replaceWith(function() {
return "<script>alert(1)<\/script>";
});with conjunction with #2203 works for me |
|
Your example works only because the replacer function returns non-empty. I put together a jsFiddle, and actually uncovered (and fixed) a bug in #2206: http://jsfiddle.net/cdhxqwk4/. Observe how this PR erroneously removes all the original elements. I just want to stop being so damn clever with |
|
@gibson042 this is a lot of code, could you provide simplified example? Just like we ask other people to do?
Yeah, your way is more robust and this logic is vague at best, but #2206 complicates a lot in order to fix a very-very edge case. |
|
Just a gist or PR with tests would do |
|
Sorry; I thought the fiddle was a clear comparison between the two PRs that demonstrated errors in this one. Straightforward tests are already at https://github.com/jquery/jquery/pull/2206/files#diff-328a9410239da61336e3662fdbfe9bf6 As for the rest, |
|
Okay, although those are really weird test-cases, i fully agree with your point. |
Alternative to #2206
-3 bytes to gzipped size