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

Fix regular expressions in isValidXss #2671

Closed
wants to merge 10 commits into from
Closed

Conversation

ZeroCho
Copy link

@ZeroCho ZeroCho commented Jan 16, 2020

Fix for #2670
following XSS Cheatsheet
also fixed JavaScript string problem in #2646 #2663 only preventing "javascript:"

It's impossible for current simple regex to prevent all kind of XSS attacks. But I tried to prevent most frequent & famous events for attacking, without saying no to "only=true"'.

My opinion is, although it is good to prevent XSS by checking URL, normal users should not suffer from this function. Current regex is still too loose.

return xssRegex.test(requestURL);
var xssEventRegex = /(\b)on(click|error|load|mouse\w+|key\w+|focus\w?|blur|change|input|drag\w?|resize|dbclick|contextmenu|drop|select|message|scroll)=/;
var xssJSRegex = /javascript:|(<\s*)(\/*)script/gi;
return xssJSRegex.test(requestURL) || xssEventRegex.test(requestURL);
Copy link
Author

Choose a reason for hiding this comment

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

separated into two regex for readablity

@Kolobok12309
Copy link

You forgot touch events

@ZeroCho ZeroCho requested a review from yasuf January 16, 2020 16:29
@ZeroCho
Copy link
Author

ZeroCho commented Jan 16, 2020

@Kolobok12309 Good point. Between huge amounts of events, I just want to add serious and frequently used events first. Touch series should be included

@Kolobok12309
Copy link

Kolobok12309 commented Jan 16, 2020

Flags only in xssJsRegexp, onMouseMove= is dismatched by xssEventRegexp
And only now find, <script> equal <\script>

P.S. Thanks for this PR:D

@yasuf
Copy link
Collaborator

yasuf commented Jan 16, 2020

thanks @ZeroCho appreciate the highly requested fix

@ZeroCho
Copy link
Author

ZeroCho commented Jan 16, 2020

@Kolobok12309 Fixed case insensitive issue
Can you please explain <script> equal <\script> in detail?

@Kolobok12309
Copy link

@ZeroCho Sorry, i'm stupid, i test it in console and forget js ignoring useless guarding))

Thanks for PR again

@ZeroCho
Copy link
Author

ZeroCho commented Jan 17, 2020

@yasuf Please tell me, if something more have to be changed or improved!

@chinesedfan chinesedfan changed the title Fix: #2670 Fix regular expressions in isValidXss Jan 20, 2020
Copy link
Collaborator

@chinesedfan chinesedfan left a comment

Choose a reason for hiding this comment

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

Thanks for your fix. But my opinion is the same with #2447 (comment).

XSS check is out of the scope of axios, and regular expressions can't find every possible case, i.e. using encoding. The most important problem is that it's overkill. I recommend to simply revert corresponding commit, as well as removing isValidXss.

@chinesedfan
Copy link
Collaborator

Closed in favor of #2679. Thanks to @ZeroCho all the same.

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants