8000 Fixed XSS by Aravindha1234u · Pull Request #339 · frappe/charts · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 2 commits into from
May 3, 2021
Merged

Fixed XSS #339

merged 2 commits into from
May 3, 2021

Conversation

Aravindha1234u
Copy link
Contributor
Explanation About What Code Achieves:
Screenshots/GIFs:

Exploit
image

Fix
image

image

Steps To Test:

Copy link
Contributor
@scmmishra scmmishra left a 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>
Copy link
Contributor

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?

Comment on lines 46 to 47
}catch(e){
}
Copy link
Contributor

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.

Comment on lines 39 to 44
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, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;").replace(/'/g, "&#039;");
}
}
Copy link
Contributor

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

Copy link
Contributor Author
@Aravindha1234u Aravindha1234u Apr 30, 2021

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

Copy link
Contributor

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

Copy link
Contributor
@scmmishra scmmishra left a 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));

Copy link
Contributor
@scmmishra scmmishra left a 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

@Aravindha1234u
Copy link
Contributor Author
Aravindha1234u commented Apr 30, 2021

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));

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;

@scmmishra
Copy link
Contributor

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

@scmmishra scmmishra merged commit 6703002 into frappe:master May 3, 2021
@scmmishra
Copy link
Contributor

Thanks @Aravindha1234u for the fix!

@adam-nygate
Copy link

@JamieSlome can we track this fix bounty on behalf of 418sec#2?

@JamieSlome
Copy link
Contributor

@adam-nygate - the disclosure bounty and fix bounty have both been rewarded here.

Great work all! 🍰

scmmishra pushed a commit that referenced this pull request May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0