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

Eliminating evals #228

Merged
merged 1 commit into from
Apr 15, 2021
Merged

Eliminating evals #228

merged 1 commit into from
Apr 15, 2021

Conversation

KOLANICH
Copy link
Contributor

No description provided.

@jstasiak
Copy link
Contributor

Hey, thank you for the contribution. I'm happy to merge this if you modify the commit message to say this is being done to simplify things and because eval() is not actually needed here, not because it's a security risk (because it's not in this case).

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Apr 14, 2021

say this is being done to simplify things and because eval() is not actually needed here, not because it's a security risk (because it's not in this case).

Actually it is, even if it isn't. Because

  1. eval when used where it is really needed is really a security risk;
  2. usage of eval where it is really unneeded is also a security risk, because it disallows to apply a policy "no evals allowed in the code base", which should be understood as "no eval-related security risk is allowed in the code base". And not being able to avoid security risk is a security risk itself.

@jstasiak
Copy link
Contributor

Without going into details I respectfully disagree. I won't merge this change with the current description unless a plausible attack scenario (that's thwarted by this change) is presented.

@KOLANICH
Copy link
Contributor Author

I have fixed the commit describtion.

@jstasiak jstasiak changed the title Eliminating evals, which are security risk Eliminating evals Apr 15, 2021
@jstasiak jstasiak merged commit e84688f into netaddr:master Apr 15, 2021
@jstasiak
Copy link
Contributor

Thank you!

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.

None yet

2 participants