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

Do not use eval function in piwik.js (for CSP) #5960

Closed
ondrejmirtes opened this Issue Aug 9, 2014 · 13 comments

Comments

Projects
None yet
3 participants
@ondrejmirtes

ondrejmirtes commented Aug 9, 2014

In order to implement CSP on my website, I disallowed 'unsafe-eval' in script-src directive. But I cannot track visitors with Piwik, because piwik.js tracking code contains call to eval function twice.

Can you consider rewriting the code so it does not use eval? (Which is a bad idea nevertheless.)

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 20, 2014

Member

One of the two eval is used in the JSON2 implementation. We can't change that unfortunately.

Member

mattab commented Aug 20, 2014

One of the two eval is used in the JSON2 implementation. We can't change that unfortunately.

@mattab mattab added the wontfix label Aug 20, 2014

@mattab mattab closed this Aug 21, 2014

@ondrejmirtes

This comment has been minimized.

Show comment
Hide comment
@ondrejmirtes

ondrejmirtes Aug 24, 2014

What is the included JSON2 library for, since all current browsers support JSON.parse and JSON.stringify?

ondrejmirtes commented Aug 24, 2014

What is the included JSON2 library for, since all current browsers support JSON.parse and JSON.stringify?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 25, 2014

Member

@ondrejmirtes it's for all "non-current" browsers that are still tracked by Piwik.

Member

mattab commented Aug 25, 2014

@ondrejmirtes it's for all "non-current" browsers that are still tracked by Piwik.

@ondrejmirtes

This comment has been minimized.

Show comment
Hide comment
@ondrejmirtes

ondrejmirtes Aug 25, 2014

So what about eliminating the other (second) eval call that's not part of the JSON2 library and using native JSON.parse/stringify where available? That way, it would be possible to remove 'unsafe-eval' clausule from my CSP string and still track overwhelming majority of users.

I personally don't care about supporting ancient browsers (the whole caniuse overview table is green) and would rather prefer a more secure experience for the majority of users. I suspect most of the developers interested in using Piwik platform would take the same stance.

ondrejmirtes commented Aug 25, 2014

So what about eliminating the other (second) eval call that's not part of the JSON2 library and using native JSON.parse/stringify where available? That way, it would be possible to remove 'unsafe-eval' clausule from my CSP string and still track overwhelming majority of users.

I personally don't care about supporting ancient browsers (the whole caniuse overview table is green) and would rather prefer a more secure experience for the majority of users. I suspect most of the developers interested in using Piwik platform would take the same stance.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 25, 2014

Member

I personally don't care about supporting ancient browsers

Most people care about tracking as many visits as possible, so I'm afraid it's not as simple as it sounds. Maybe you can create a new issue for this specific change?

Member

mattab commented Aug 25, 2014

I personally don't care about supporting ancient browsers

Most people care about tracking as many visits as possible, so I'm afraid it's not as simple as it sounds. Maybe you can create a new issue for this specific change?

@ondrejmirtes

This comment has been minimized.

Show comment
Hide comment
@ondrejmirtes

ondrejmirtes Aug 25, 2014

Well, people who don't setup CSP header would still be able to track them, it's only that in my server configuration, the minority of browsers that would trigger the eval call would not get tracked.

And I realized that the browsers that support CSP also probably have native JSON implementation, so the number of untracked visitors would be zero.

ondrejmirtes commented Aug 25, 2014

Well, people who don't setup CSP header would still be able to track them, it's only that in my server configuration, the minority of browsers that would trigger the eval call would not get tracked.

And I realized that the browsers that support CSP also probably have native JSON implementation, so the number of untracked visitors would be zero.

@ondrejmirtes

This comment has been minimized.

Show comment
Hide comment
@ondrejmirtes

ondrejmirtes Aug 25, 2014

My whole motivation behind this issue is to encourage Piwik to stay on the cutting edge of web standards and security because I really like it and want to promote it in the community. So please think about some solutions to enable using strict CSP settings while tracking with Piwik.

ondrejmirtes commented Aug 25, 2014

My whole motivation behind this issue is to encourage Piwik to stay on the cutting edge of web standards and security because I really like it and want to promote it in the community. So please think about some solutions to enable using strict CSP settings while tracking with Piwik.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 25, 2014

Member

@ondrejmirtes sounds good, thanks! Could you maybe create a new issue with more specific scope than "do not use eval"?

For example: Make Piwik compatible with strict CSP settings ?

Member

mattab commented Aug 25, 2014

@ondrejmirtes sounds good, thanks! Could you maybe create a new issue with more specific scope than "do not use eval"?

For example: Make Piwik compatible with strict CSP settings ?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Feb 17, 2015

Member

I wonder why this issue is closed as it is not fixed? For example we could use https://bestiejs.github.io/json3/ which does not use eval. I just decided to reopen it :)

Member

tsteur commented Feb 17, 2015

I wonder why this issue is closed as it is not fixed? For example we could use https://bestiejs.github.io/json3/ which does not use eval. I just decided to reopen it :)

@tsteur tsteur reopened this Feb 17, 2015

@tsteur tsteur removed the wontfix label Feb 17, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Feb 17, 2015

Member

FYI: I tried to replace JSON2 with JSON3 but I can't get JSON3 to work with JSLint . Meaning no matter which options I set JSLint always fails and it seems to be not possible to ignore one block of code for JSLint. We should replace JSLint at some point with JSHint which would exactly solve this.

Member

tsteur commented Feb 17, 2015

FYI: I tried to replace JSON2 with JSON3 but I can't get JSON3 to work with JSLint . Meaning no matter which options I set JSLint always fails and it seems to be not possible to ignore one block of code for JSLint. We should replace JSLint at some point with JSHint which would exactly solve this.

@tsteur tsteur added this to the Short term milestone Feb 17, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Feb 17, 2015

Member

If someone manages to make Piwik.js work with JSHint we could replace JSON2 with JSON3 and remove all evals. See #7232

Member

tsteur commented Feb 17, 2015

If someone manages to make Piwik.js work with JSHint we could replace JSON2 with JSON3 and remove all evals. See #7232

@mattab mattab changed the title from Do not use eval function in JavaScript to Do not use eval function in piwik.js (for CSP) Apr 9, 2015

@mattab mattab added Task and removed Bug labels Apr 9, 2015

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Apr 9, 2015

Member

since the goal of this issue is really to add CSP support to piwik.js I suggest we mark as duplicate of: #1542

next step: #7232

Member

mattab commented Apr 9, 2015

since the goal of this issue is really to add CSP support to piwik.js I suggest we mark as duplicate of: #1542

next step: #7232

@mattab mattab closed this Apr 9, 2015

@mattab mattab added the duplicate label Apr 9, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 2, 2015

Member

Reopen this one since eval !== CSP

Member

tsteur commented Oct 2, 2015

Reopen this one since eval !== CSP

@tsteur tsteur reopened this Oct 2, 2015

@tsteur tsteur modified the milestones: 2.15.0, Short term Oct 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment