fix: address review — add Greek symbols, copy global_dict per call, validate variable names

Author: sebastiondev

src/qwed_new/core/safe_parser.py

@@ -65,16 +65,52 @@
     re.IGNORECASE,
 )
 
+# Regex for validating variable names passed to Symbol().
+# Allows single-letter, Greek names, and conventional multi-letter math
+# variable names.  Must start with a letter and contain only alphanumerics
+# and underscores, with a reasonable length cap.
+_SAFE_VARIABLE_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_]{0,49}$")
+
+
+def validate_variable_name(name: str) -> None:
+    """
+    Validate a user-supplied variable name before it reaches Symbol().
+
+    Applies the same length cap, denylist, and character-set checks that
+    safe_parse_expr applies to full expressions, keeping the hardened
+    boundary consistent across all user-controlled string inputs.
+
+    Raises:
+        ValueError: If the name is invalid or contains dangerous patterns.
+    """
+    if not isinstance(name, str):
+        raise ValueError("Variable name must be a string")
+
+    stripped = name.strip()
+    if not stripped:
+        raise ValueError("Variable name must not be empty")
+
+    if not _SAFE_VARIABLE_RE.match(stripped):
+        raise ValueError(
+            "Variable name must start with a letter, contain only "
+            "alphanumerics/underscores, and be at most 50 characters"
+        )
+
+    if _DANGEROUS_PATTERNS.search(stripped):
+        raise ValueError("Variable name contains disallowed constructs")
+
 
 def _build_safe_local_dict(extra_symbols: Optional[Dict[str, Any]] = None) -> dict:
     """
     Build the allow-listed local namespace for parse_expr.
 
     Only mathematical symbols, constants, functions, and the internal
     sympy types that parse_expr's transformations emit are included.
+    Includes common Greek-letter and multi-letter symbolic variable names
+    used in standard mathematical and scientific notation.
     """
     safe = {
-        # Common symbolic variables
+        # Common single-letter symbolic variables
         "x": Symbol("x"),
         "y": Symbol("y"),
         "z": Symbol("z"),
@@ -91,6 +127,40 @@ def _build_safe_local_dict(extra_symbols: Optional[Dict[str, Any]] = None) -> di
         "u": Symbol("u"),
         "v": Symbol("v"),
         "w": Symbol("w"),
+        # Greek-letter symbolic variables (common in verification workloads)
+        "alpha": Symbol("alpha"),
+        "beta": Symbol("beta"),
+        "gamma": Symbol("gamma"),
+        "delta": Symbol("delta"),
+        "epsilon": Symbol("epsilon"),
+        "zeta": Symbol("zeta"),
+        "eta": Symbol("eta"),
+        "theta": Symbol("theta"),
+        "iota": Symbol("iota"),
+        "kappa": Symbol("kappa"),
+        "mu": Symbol("mu"),
+        "nu": Symbol("nu"),
+        "xi": Symbol("xi"),
+        "omicron": Symbol("omicron"),
+        "rho": Symbol("rho"),
+        "sigma": Symbol("sigma"),
+        "tau": Symbol("tau"),
+        "upsilon": Symbol("upsilon"),
+        "phi": Symbol("phi"),
+        "chi": Symbol("chi"),
+        "psi": Symbol("psi"),
+        "omega": Symbol("omega"),
+        # Capital Greek letters commonly used as symbols
+        "Alpha": Symbol("Alpha"),
+        "Beta": Symbol("Beta"),
+        "Gamma": Symbol("Gamma"),
+        "Delta": Symbol("Delta"),
+        "Theta": Symbol("Theta"),
+        "Lambda": Symbol("Lambda"),
+        "Sigma": Symbol("Sigma"),
+        "Phi": Symbol("Phi"),
+        "Psi": Symbol("Psi"),
+        "Omega": Symbol("Omega"),
         # Mathematical constants
         "pi": pi,
         "e": E,
@@ -141,7 +211,9 @@ def _build_safe_local_dict(extra_symbols: Optional[Dict[str, Any]] = None) -> di
     return safe
 
 
-# Pre-built global dict that strips builtins
+# Pre-built global dict that strips builtins.
+# IMPORTANT: A shallow copy is made per invocation (see safe_parse_expr)
+# to prevent cross-call mutation by SymPy transformations.
 _SAFE_GLOBAL_DICT: dict = {"__builtins__": {}}
 
 
@@ -192,7 +264,7 @@ def safe_parse_expr(
         return parse_expr(
             stripped,
             local_dict=local_dict,
-            global_dict=_SAFE_GLOBAL_DICT,
+            global_dict=dict(_SAFE_GLOBAL_DICT),
             transformations=transformations,
         )
     except Exception as exc:

src/qwed_new/core/verifier.py

@@ -18,7 +18,7 @@
     diff, integrate, limit, oo,
     simplify, expand
 )
-from qwed_new.core.safe_parser import safe_parse_expr, SAFE_TRANSFORMATIONS
+from qwed_new.core.safe_parser import safe_parse_expr, validate_variable_name, SAFE_TRANSFORMATIONS
 from typing import Any, Dict, List, Optional
 from decimal import Decimal, ROUND_HALF_UP
 from dataclasses import dataclass
@@ -282,6 +282,7 @@ def verify_derivative(
         """
         try:
             expr = safe_parse_expr(expression)
+            validate_variable_name(variable)
             var = Symbol(variable)
             expected_expr = safe_parse_expr(expected)
 
@@ -329,6 +330,7 @@ def verify_integral(
         """
         try:
             expr = safe_parse_expr(expression)
+            validate_variable_name(variable)
             var = Symbol(variable)
             expected_expr = safe_parse_expr(expected)
 
@@ -386,6 +388,7 @@ def verify_limit(
         """
         try:
             expr = safe_parse_expr(expression)
+            validate_variable_name(variable)
             var = Symbol(variable)
             expected_expr = safe_parse_expr(expected)
 

tests/security/test_safe_parser.py

@@ -6,7 +6,7 @@
 """
 
 import pytest
-from qwed_new.core.safe_parser import safe_parse_expr
+from qwed_new.core.safe_parser import safe_parse_expr, validate_variable_name
 
 
 class TestSafeParseExprBlocksCodeExecution:
@@ -81,6 +81,32 @@ def test_valid_expression_parses(
         assert str(result) == expected_str
 
 
+class TestSafeParseExprMultiLetterSymbols:
+    """Verify that multi-letter symbolic variable names parse correctly."""
+
+    @pytest.mark.parametrize(
+        "expression,expected_str",
+        [
+            ("alpha + beta", "alpha + beta"),
+            ("theta**2", "theta**2"),
+            ("sin(phi)", "sin(phi)"),
+            ("gamma * delta", "delta*gamma"),
+            ("epsilon + tau", "epsilon + tau"),
+            ("sigma * omega", "omega*sigma"),
+            ("Lambda + Omega", "Lambda + Omega"),
+        ],
+    )
+    def test_greek_letter_variables_parse(
+        self, expression: str, expected_str: str
+    ) -> None:
+        result = safe_parse_expr(expression)
+        assert str(result) == expected_str
+
+    def test_mixed_single_and_greek_variables(self) -> None:
+        result = safe_parse_expr("x + alpha")
+        assert str(result) == "alpha + x"
+
+
 class TestSafeParseExprInputValidation:
     """Verify input validation guards."""
 
@@ -95,3 +121,52 @@ def test_rejects_non_string(self) -> None:
     def test_rejects_oversized_input(self) -> None:
         with pytest.raises(ValueError, match="too long"):
             safe_parse_expr("x + " * 2000)
+
+
+class TestSafeParseExprGlobalDictIsolation:
+    """Verify that _SAFE_GLOBAL_DICT is not mutated across calls."""
+
+    def test_global_dict_not_shared_between_calls(self) -> None:
+        """Parse two different expressions and confirm no cross-contamination."""
+        safe_parse_expr("x + 1")
+        safe_parse_expr("alpha + beta")
+        # If global_dict were shared mutably, SymPy transformations could
+        # leak symbols from one call into another. The shallow-copy fix
+        # prevents this. We just verify no exception is raised and both
+        # parse independently.
+        result = safe_parse_expr("y + 2")
+        assert str(result) == "y + 2"
+
+
+class TestValidateVariableName:
+    """Verify variable name validation for calculus methods."""
+
+    @pytest.mark.parametrize(
+        "name",
+        ["x", "y", "theta", "alpha", "x1", "var_name"],
+    )
+    def test_valid_variable_names_accepted(self, name: str) -> None:
+        # Should not raise
+        validate_variable_name(name)
+
+    @pytest.mark.parametrize(
+        "name",
+        [
+            "",           # empty
+            "   ",        # whitespace only
+            "123",        # starts with digit
+            "__import__", # dunder pattern
+            "a" * 51,    # too long
+            "os",         # denylist hit
+            "sys",        # denylist hit
+            "eval",       # denylist hit
+            "exec",       # denylist hit
+        ],
+    )
+    def test_invalid_variable_names_rejected(self, name: str) -> None:
+        with pytest.raises(ValueError):
+            validate_variable_name(name)
+
+    def test_rejects_non_string_variable(self) -> None:
+        with pytest.raises(ValueError, match="must be a string"):
+            validate_variable_name(123)  # type: ignore[arg-type]