|
Looks like there is no CI running on the 4.17 branch anymore as the migration to github actions has been done only in master... |
|
The regex is of ~10k in size, and is still incomplete. Example: > var 𝐀 = 1
undefined
> re.test('𝐀')
false |
|
@Alan-Liang I indeed took the ES 5 regex, not the ES6 one (which allowed even more chars. I'll let the maintainers decide what they want to allow for this |
|
Would it be OK to just ban the following characters: Is there any current ES proposal that extends this list of characters? |
|
What do you think about only accepting ASCII characters for the variable name, and instead of throwing an error, just defaulting to the Any change here to fix this CVE would be considered breaking, but I don't want to introduce additional explicit errors. /cc @jdalton @falsyvalues for any inputs. |
|
@bnjmnt4n defaulting to |
|
Yeah, the generated template function would be broken, but it would be a runtime error instead of an error during _.template. However, now that I think about it, it's probably fine to throw an error at "compile time" since there's a likelihood that our Function call would do that also, so keeping the error throwing behaviour should be fine. |
|
And throwing a compile-time error at least allows to throw a meaningful error, explaining what went wrong. |
2312507 to
1560d21
Compare
|
I updated the PR to use a much smaller regex forbidding some offending chars instead of the huge ES identifier regex |
|
Main problem is that we don't know what users put there, its configuration layer. Honestly I'm not really sure why this is even reported as @bnjmnt4n I agree we should avoid throwing new errors and go with default path. Maybe it would be easier to use negative check and detect characters that cannot be part of variable name in ES6. |
|
@falsyvalues lodash cannot enforce that no user-controlled values is ever used by projects when passing the variable.
having a variable name or not having one produce a different template compilation, for a different kind of usage. You cannot consider the
This has now been done (with a set of specific forbidden chars) to avoid the huge regexp. |
Sorry, but I don't know what are you trying to say. To which part of comment are you referring? |
|
@falsyvalues that was the answer to the first paragraph of your previous comment. |
|
I think the list of forbidden characters used is a good compromise between having to embed a large ES identifier regex, and maximizing back-compat. This does mean that we might have to update the regex in the future if there are any changes to allowed ECMAScript syntax. |
|
Thanks for the PR, merged in 3469357. |
| @@ -14865,6 +14875,8 @@ | |||
| var variable = hasOwnProperty.call(options, 'variable') && options.variable; | |||
| if (!variable) { | |||
| source = 'with (obj) {\n' + source + '\n}\n'; | |||
| } else if (reForbiddenIdentifierChars.test(variable)) { | |||
There was a problem hiding this comment.
I noticed there is no check that variable is typeof 'string'
There was a problem hiding this comment.
RegExp.prototype.test coerces its argument to a string before making matches.
Prevent command injection through `_.template`'s `variable` option Closes lodash#5085.
This fixes the issue disclosed at https://app.snyk.io/vuln/SNYK-JS-LODASH-1040724
For now, I used a regexp allowing any ES5 identifier for the variable name. The advantage of that is that it avoids BC breaks if some code uses weird variable names. The drawback is that the regexp is big.
If reducing the range of allowed variable names is OK, a smaller regex could be used.