feat!: limit usage of the eval function#736
Conversation
`StylesheetCodec` no longer call the function by default. The eslint configuration now includes a new rule that prevents new calls to the function. Introduce an internal function for indirection of the `eval` function. This change generates fewer warnings when using bundlers such as Rollup. Be aware that this is a temporary workaround, not a final solution. BREAKING CHANGES: `StylesheetCodec.allowEval` is now set to `false` by default to prevent unwanted use of the eval function, as it carries a possible security risk.
WalkthroughThis pull request enforces stricter security measures regarding the use of Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant doEval
participant JS_Eval
Caller->>doEval: Call doEval(expression)
doEval->>JS_Eval: Evaluate expression using native eval
JS_Eval-->>doEval: Return evaluation result
doEval-->>Caller: Return result
Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/internal/utils.ts (1)
17-23: Good centralization of eval usage, but consider additional safeguardsCentralizing
evalin a single utility function is a good approach that makes it easier to audit and control its usage. The@internalmarker and comments clearly indicate the purpose.However, the function still inherits all security risks associated with
evalwithout additional safeguards.Consider enhancing this function in the future with:
- Input validation to restrict what expressions can be evaluated
- A more detailed comment documenting safe usage patterns
- Potentially adding a sandbox or other security measures
/** * @internal */ export const doEval = (expression: string): any => { + // TODO: Add validation to restrict what expressions can be evaluated + // For security best practices, this function should: + // 1. Validate input expressions against a safe pattern + // 2. Consider using more secure alternatives like JSON.parse where possible + // 3. Only be used for trusted inputs with no user-controlled data // eslint-disable-next-line no-eval -- valid here as we want this function to be the only place in the codebase that uses eval return eval(expression); };🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.eslintrc.json(1 hunks)CHANGELOG.md(1 hunks)packages/core/src/editor/EditorPopupMenu.ts(3 hunks)packages/core/src/internal/utils.ts(1 hunks)packages/core/src/serialization/ObjectCodec.ts(2 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(3 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(3 hunks)packages/core/src/view/GraphView.ts(3 hunks)packages/core/src/view/geometry/node/StencilShape.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/core/src/editor/EditorPopupMenu.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-27T09:34:54.875Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
🧬 Code Definitions (5)
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
packages/core/src/internal/utils.ts (1)
doEval(20-23)
packages/core/src/editor/EditorPopupMenu.ts (2)
packages/core/src/internal/utils.ts (1)
doEval(20-23)packages/core/src/util/domUtils.ts (1)
getTextContent(88-90)
packages/core/src/serialization/ObjectCodec.ts (2)
packages/core/src/internal/utils.ts (1)
doEval(20-23)packages/core/src/util/domUtils.ts (1)
getTextContent(88-90)
packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
packages/core/src/internal/utils.ts (1)
doEval(20-23)
packages/core/src/view/GraphView.ts (1)
packages/core/src/internal/utils.ts (1)
doEval(20-23)
🪛 Biome (1.9.4)
packages/core/src/internal/utils.ts
[error] 22-22: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (18)
CHANGELOG.md (1)
12-13: Well-documented breaking changeThe addition of this breaking change information is clear and helpful, providing users with an understanding of both the change and the security rationale behind it.
.eslintrc.json (1)
36-37: Good addition of security-enhancing lint ruleAdding the
no-evalrule as an error is an excellent security practice that aligns with industry standards. This enforces the PR's goal of limitingevalusage across the codebase.packages/core/src/serialization/ObjectCodec.ts (2)
27-27: Good import of centralized eval functionImporting the
doEvalfunction from the internal utils module aligns with the security goal of centralizing and controlling eval usage.
823-825: Good replacement of direct eval with centralized functionReplacing the direct call to
evalwith the centralizeddoEvalfunction maintains the existing functionality while improving code organization and security control. The conditional check onObjectCodec.allowEvaladds an extra layer of security by ensuring evaluation only happens when explicitly enabled.packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (3)
27-27: Improved security through centralized evaluation.Added import of
doEvalfunction to replace directevalusage, aligning with the PR objective to limitevalusage.
161-161: Replaced directevalwith safer controlled evaluation.The change ensures evaluation only happens when explicitly allowed and uses the centralized
doEvalfunction instead of directeval.
175-175: Improved security by replacing directevalwith controlled evaluation.Using
doEvalcentralizes the evaluation logic and allows for better security control and monitoring.packages/core/src/view/geometry/node/StencilShape.ts (2)
36-36: Added safer evaluation function import.The
doEvalfunction import supports the PR's security objectives by providing a centralized place for evaluation.
197-197: Enhanced security by replacing directevalwith controlled evaluation.Changing from direct
evaltodoEvalhelps mitigate security risks and aligns with the ESLint rule added in this PR to prevent directevalusage.packages/core/src/editor/EditorPopupMenu.ts (3)
27-27: Added import for secure evaluation function.The added import supports the PR's objective to limit and control the use of
eval.
321-321: Enhanced security by replacing directevalwith controlled function.The change maintains compatibility with the XML-based configuration approach while improving security.
198-198:Details
✅ Verification successful
Replaced direct
evalwith controlled evaluation.This change improves security while maintaining the functionality required for XML-based configuration.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining direct eval calls in this file rg -i "eval\(" packages/core/src/editor/EditorPopupMenu.tsLength of output: 202
Confirmed use of controlled evaluation in EditorPopupMenu.ts
- The verification confirms that there are no direct invocations of
eval()—both instances (one usingitemand one usingcondNode) make use of the wrapper functiondoEval().- This controlled evaluation aligns with our current approach for XML-based configuration, as per the team's decision.
packages/core/src/serialization/codecs/StylesheetCodec.ts (3)
25-25: Added import for secure evaluation function.This import supports the PR's security objectives by centralizing evaluation logic.
166-166: Replaced directevalwith controlled evaluation function.Using
doEvalinstead of directevalenhances security while maintaining functionality for those who explicitly opt-in.
45-47:Details
✅ Verification successful
BREAKING CHANGE:
allowEvalnow defaults to false.This is an important security improvement that matches the PR objectives. By defaulting to
false, the system now requires explicit opt-in for evaluation of potentially unsafe code.
🌐 Web query:
What are the security risks of using eval in JavaScript?💡 Result:
The
eval()function in JavaScript allows dynamic code execution from strings, but introduces significant security risks that require careful handling. Its ability to execute arbitrary code makes it vulnerable to exploitation if inputs aren't properly controlled. Below are the primary security concerns and their implications.Code Injection Attacks
eval()can execute malicious code if inputs originate from untrusted sources like user inputs, cookies, or API responses. Attackers may inject commands to steal data, hijack sessions, or deploy malware[1][3]. For example, a text field usingeval()on user input could enable Cross-Site Scripting (XSS) attacks if exploited[3][5].Scope Pollution
Evaluated code runs in the current scope, allowing access to and modification of global variables. This exposes sensitive data and creates unpredictable side effects, such as overwriting critical functions or variables[1][2]. For instance, a malicious script viaeval()could redefine authentication logic or leak environment variables[2][3].Performance and Maintenance Issues
- Slow Execution:
eval()parses and interprets code at runtime, bypassing JavaScript engine optimizations and slowing down applications[1][2][4].- Debugging Complexity: Errors in dynamically evaluated code lack clear stack traces, complicating troubleshooting[1][4].
Unintended Code Execution
Even "sanitized" inputs can exploiteval(). For example, modifying client-side data attributes (e.g.,data-*attributes) via browser tools lets attackers inject arbitrary code ifeval()processes those values[5].Safer Alternatives
- JSON Parsing: Use
JSON.parse()instead ofeval()for data serialization[2].- Function Constructor: Create scoped functions via
new Function()to limit variable access[4].- Static Analysis Tools: Detect insecure
eval()usage with tools like Codiga, which flags risky patterns in codebases[1].While
eval()has niche uses, prioritizing alternatives and rigorously validating inputs minimizes exposure to code injection, data breaches, and system compromises[1][2][3]. Always restricteval()to controlled, non-user-facing contexts when unavoidable.Citations:
- 1: https://www.codiga.io/blog/javascript-eval-best-practices/
- 2: https://www.lenovo.com/us/en/glossary/eval/
- 3: https://wpengine.com/resources/javascript-security/
- 4: https://dev.to/amitkhonde/eval-is-evil-why-we-should-not-use-eval-in-javascript-1lbh
- 5: https://forum.freecodecamp.org/t/question-the-security-risk-of-eval/670388
Security Enhancement Notice:
allowEvalnow defaults to falseThe updated default value effectively mitigates the significant security risks associated with
eval(), such as code injection, scope pollution, and performance issues. By requiring an explicit opt-in for execution of dynamic code, this breaking change aligns with our security objectives and reduces the attack surface.
- File:
packages/core/src/serialization/codecs/StylesheetCodec.ts(Lines 45-47)- Change Summary:
- The property is now defined as:
/** * @default false */ static allowEval = false;- This change prevents inadvertent execution of potentially unsafe code unless explicitly permitted by the developer.
packages/core/src/view/GraphView.ts (3)
49-49: Good addition of the doEval importThis import adds the centralized evaluation utility that replaces direct eval calls, aligning with the PR's security objectives.
1330-1332: Security improvement by replacing eval with doEvalReplacing direct
eval(edgeStyle)withdoEval(edgeStyle)centralizes dynamic code evaluation to a single location in the codebase, making it easier to track and secure. This change is guarded by theisAllowEval()check which is now false by default.
1597-1599: Security improvement by replacing eval with doEvalReplacing direct
eval(perimeter)withdoEval(perimeter)follows the same security pattern applied throughout the codebase. This centralized approach improves security and maintainability.
StylesheetCodecno longer call the function by default.The eslint configuration now includes a new rule that prevents new calls to the function.
Introduce an internal function for indirection of the
evalfunction. This change generates fewer warnings when using bundlers such as Rollup.Be aware that this is a temporary workaround, not a final solution.
BREAKING CHANGES:
StylesheetCodec.allowEvalis now set tofalseby default to prevent unwanted use of the eval function, as it carries a possible security risk.Notes
Covers #722
eslint rule: https://eslint.org/docs/latest/rules/no-eval
Summary by CodeRabbit