Actionable comments posted: 0

packages/core/src/internal/utils.ts (1)

17-23: Good centralization of eval usage, but consider additional safeguards

Centralizing eval in a single utility function is a good approach that makes it easier to audit and control its usage. The @internal marker and comments clearly indicate the purpose.

However, the function still inherits all security risks associated with eval without 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)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Reviewing files that changed from the base of the PR and between 6acaeff and 295cbc6.

• .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)

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.

packages/core/src/internal/utils.ts (1)

• doEval (20-23)

packages/core/src/internal/utils.ts (1)

• doEval (20-23)

packages/core/src/util/domUtils.ts (1)

• getTextContent (88-90)

packages/core/src/internal/utils.ts (1)

• doEval (20-23)

packages/core/src/util/domUtils.ts (1)

• getTextContent (88-90)

packages/core/src/internal/utils.ts (1)

• doEval (20-23)

packages/core/src/internal/utils.ts (1)

• doEval (20-23)

[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)

CHANGELOG.md (1)

12-13: Well-documented breaking change

The 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 rule

Adding the no-eval rule as an error is an excellent security practice that aligns with industry standards. This enforces the PR's goal of limiting eval usage across the codebase.

packages/core/src/serialization/ObjectCodec.ts (2)

27-27: Good import of centralized eval function

Importing the doEval function 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 function

Replacing the direct call to eval with the centralized doEval function maintains the existing functionality while improving code organization and security control. The conditional check on ObjectCodec.allowEval adds 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 doEval function to replace direct eval usage, aligning with the PR objective to limit eval usage.

---

161-161: Replaced direct eval with safer controlled evaluation.

The change ensures evaluation only happens when explicitly allowed and uses the centralized doEval function instead of direct eval.

---

175-175: Improved security by replacing direct eval with controlled evaluation.

Using doEval centralizes 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 doEval function import supports the PR's security objectives by providing a centralized place for evaluation.

---

197-197: Enhanced security by replacing direct eval with controlled evaluation.

Changing from direct eval to doEval helps mitigate security risks and aligns with the ESLint rule added in this PR to prevent direct eval usage.

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 direct eval with controlled function.

The change maintains compatibility with the XML-based configuration approach while improving security.

---

198-198:

Details

✅ Verification successful

Replaced direct eval with 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.ts

Length 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 using item and one using condNode) make use of the wrapper function doEval().
• 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 direct eval with controlled evaluation function.

Using doEval instead of direct eval enhances security while maintaining functionality for those who explicitly opt-in.

---

45-47:

Details

✅ Verification successful

BREAKING CHANGE: allowEval now 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 using eval() 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 via eval() 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 exploit eval(). For example, modifying client-side data attributes (e.g., data-* attributes) via browser tools lets attackers inject arbitrary code if eval() processes those values[5].

Safer Alternatives

• JSON Parsing: Use JSON.parse() instead of eval() 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 restrict eval() 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: allowEval now defaults to false

The 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 import

This 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 doEval

Replacing direct eval(edgeStyle) with doEval(edgeStyle) centralizes dynamic code evaluation to a single location in the codebase, making it easier to track and secure. This change is guarded by the isAllowEval() check which is now false by default.

---

1597-1599: Security improvement by replacing eval with doEval

Replacing direct eval(perimeter) with doEval(perimeter) follows the same security pattern applied throughout the codebase. This centralized approach improves security and maintainability.