-
Notifications
You must be signed in to change notification settings - Fork 739
Fixed XSS #339
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
Fixed XSS #339
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.
Hey thanks for the PR, but the code quality is not up to the mark. The indentation, variable declarations can be better
example.html
Outdated
@@ -0,0 +1,34 @@ | |||
<html> |
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.
Can you remove this file?
src/js/utils/axis-chart-utils.js
Outdated
}catch(e){ | ||
} |
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.
A blank catch
is not desired.
src/js/utils/axis-chart-utils.js
Outdated
var temp = data.datasets[i]['values']; | ||
for(var j=0;j<temp.length;j++){ | ||
if(typeof(temp[j]) == "string"){ | ||
data.datasets[i]['values'][j] = String(temp[j]).replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">").replace(/"/g, """).replace(/'/g, "'"); | ||
} | ||
} |
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.
The loop is really inefficient, let's just check if the item is a valid number or not, there's a utility for that
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.
Yes it check whether its number or not, But actual problem is not does work properly. even Strings are inserted into charts and that leads to XSS
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.
even Strings are inserted into charts
The values should always be numbers, strings shouldn't be allowed. For labels a fix for this was already made here
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.
There's a line in axis-chart-utils
that does check if the value in the dataset is actually a number or not. The fix can be implemented there instead of looping again
vals = vals.map(val => (!isNaN(val) ? val : 0));
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.
Address the comments and let me know
Got It, but changing this vals does not reflect in the actual dataset that's the problem of this entirely. That the only reason why I created a separate loop for updating the actual dataset; |
Patching the loop will be a better option |
Thanks @Aravindha1234u for the fix! |
@JamieSlome can we track this fix bounty on behalf of 418sec#2? |
@adam-nygate - the disclosure bounty and fix bounty have both been rewarded here. Great work all! 🍰 |
Explanation About What Code Achieves:
Screenshots/GIFs:
Exploit

Fix

Steps To Test:
"<img src=x "
and place it invalues: [25, "<img src=x "