Remix.run Logo
minus7 5 hours ago

The `eval` alone should be enough of a red flag

jeltz 4 hours ago | parent | next [-]

Yeah, I would have loved to see an example where it was not obvious that there is an exploit. Where it would be possible for a reviewer to actually miss it.

godelski 2 hours ago | parent | prev | next [-]

I'm not a JS person, but taking the line at face value shouldn't it to nothing? Which, if I understand correctly, should never be merged. Why would you merge no-ops?

kordlessagain 5 hours ago | parent | prev [-]

No it’s not.

simonreiff 5 hours ago | parent | next [-]

OWASP disagrees: See https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Securi..., listing `eval()` first in its small list of examples of "JavaScript functions that are dangerous and should only be used where necessary or unavoidable". I'm unaware of any such uses, myself. I can't think of any scenario where I couldn't get what I wanted by using some combination of `vm`, the `Function` constructor, and a safe wrapper around `JSON.parse()` to do anything I might have considered doing unsafely with `eval()`. Yes, `eval()` is a blatant red flag and definitely should be avoided.

jacquesm 4 hours ago | parent | prev | next [-]

While there are valid use cases for eval they are so rare that it should be disabled by default and strongly discouraged as a pattern. Only in very rare cases is eval the right choice and even then it will be fraught with risk.

godelski 2 hours ago | parent | prev | next [-]

The parent didn't say "there's no legitimate uses of eval", they said "using eval should make people pay more attention." A red flag is a warning. An alert. Not a signal saying "this is 100% no doubt malicious code."

Yes, it's a red flag. Yes, there's legitimate uses. Yes, you should always interrogate evals more closely. All these are true

pavel_lishin 5 hours ago | parent | prev | next [-]

When is an eval not at least a security "code smell"?

SahAssar 5 hours ago | parent | prev [-]

It really is. There are very few proper use-cases for eval.

nswango 4 hours ago | parent [-]

For a long time the standard way of loading JSON was using eval.

bawolff 2 hours ago | parent | next [-]

Not that long, browsers implemented JSON.parse() back in 2009. JSON was only invented back in 2001 and took a while to become popular. It was a very short window more than a decade ago when eval made sense here.

Eval for json also lead to other security issues like XSSI.

creatonez 5 minutes ago | parent [-]

Problem is, it took until around 2016 for IE6 to be fully dead, so people continued to justify these hacks for a long time. Horrifying times.

creatonez 15 minutes ago | parent | prev | next [-]

For IE7 support, yes... https://caniuse.com/?search=JSON.parse

_flux 3 hours ago | parent | prev [-]

And why do we not anymore make use of it, but instead implemented separate JSON loading functionality in JavaScript? Can you think of any reasons beyond performance?

bawolff 2 hours ago | parent | next [-]

I'd be surprised if there is a performance benefit of processing json with eval(). Browsers optimize the heck out of JSON.

fhars 18 minutes ago | parent [-]

You are arguing against the opposite of what the comment you answered to said.

bulbar 3 hours ago | parent | prev [-]

Why did you opt in for such a comment while a straight forward response without belittling tone would have achieved the same?

_flux 3 hours ago | parent [-]

I actually gave it some thought. I had written the actual reason first, but I realized that the person I was responding to must know this, yet keeps arguing in that eval is just fine.

I would say they are arguing that in bad faith, so I wanted to enter a dialogue where they are either forced to agree, or more likely, not respond at all.