fix: improve code generation safety#11
Conversation
374d58c to
0238ddd
Compare
There was a problem hiding this comment.
Code Review
This pull request primarily enhances the security and robustness of the code generation and interpretation logic. It achieves this by consistently using JSON.stringify for string values and property names in CodeGenerator, AssignmentNode, ConstantNode, FunctionNode, and SymbolNode to prevent code injection vulnerabilities. Additionally, validation for the factory option has been added to the Interpreter constructor. The review feedback points out two critical security vulnerabilities: potential prototype pollution in AssignmentNode.js and information exposure in SymbolNode.js if __proto__ is accessed, both with suggested fixes to explicitly disallow such operations. The package-lock.json updates reflect changes in development dependency declarations, adding peer: true to several entries.
|
|
||
| module.exports = function (node) { | ||
| return 'scope["' + node.name + '"] = ' + this.next(node.expr) | ||
| return 'scope[' + JSON.stringify(node.name) + '] = ' + this.next(node.expr) |
There was a problem hiding this comment.
While this change prevents code injection, it is still vulnerable to prototype pollution. An assignment where node.name is __proto__ can modify the prototype of the scope object. This can lead to security vulnerabilities like remote code execution. It's crucial to forbid assignments to __proto__.
| return 'scope[' + JSON.stringify(node.name) + '] = ' + this.next(node.expr) | |
| return node.name === '__proto__' ? 'throw new Error("Assignment to __proto__ is not allowed.")' : 'scope[' + JSON.stringify(node.name) + '] = ' + this.next(node.expr); |
| module.exports = function (node) { | ||
| const id = node.name | ||
| return '$$mathCodegen.getProperty("' + id + '", scope, ns)' | ||
| return '$$mathCodegen.getProperty(' + JSON.stringify(id) + ', scope, ns)' |
There was a problem hiding this comment.
Accessing __proto__ can leak Object.prototype. This information exposure might not be intended and could provide valuable information to an attacker for chaining vulnerabilities. It's safer to explicitly disallow access to __proto__.
| return '$$mathCodegen.getProperty(' + JSON.stringify(id) + ', scope, ns)' | |
| return id === '__proto__' ? 'throw new Error("Access to __proto__ is not allowed.")' : '$$mathCodegen.getProperty(' + JSON.stringify(id) + ', scope, ns)'; |
No description provided.