From dc9d4db72ca4b4ae3f96d0e6a0c27a9e38a06f61 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Mon, 8 Jun 2026 14:22:09 +0100 Subject: [PATCH 1/8] fix: restrict sympy parse_expr to prevent code execution (CWE-95) sympy parse_expr uses Python code execution internally and without restrictions on local_dict/global_dict allows arbitrary code execution through crafted math expression strings. Add safe_parse_expr wrapper that: - Validates input against a denylist of dangerous patterns (dunder attrs, import, exec, os, subprocess, etc.) - Restricts the namespace to only known-safe sympy objects (math functions, constants, and common symbolic variables) - Strips Python builtins from the global namespace - Enforces a maximum expression length Replace all bare parse_expr calls across the codebase: - src/qwed_new/api/main.py (/verify/math endpoint) - src/qwed_new/core/verifier.py (VerificationEngine) - src/qwed_new/core/batch.py (batch math verification) - src/qwed_new/core/validator.py (SemanticValidator) Signed-off-by: Sebastion --- src/qwed_new/api/main.py | 8 +- src/qwed_new/core/batch.py | 8 +- src/qwed_new/core/safe_parser.py | 199 +++++++++++++++++++++++++++++ src/qwed_new/core/validator.py | 4 +- src/qwed_new/core/verifier.py | 28 ++-- tests/security/test_safe_parser.py | 97 ++++++++++++++ tests/test_api_exceptions.py | 2 +- 7 files changed, 321 insertions(+), 25 deletions(-) create mode 100644 src/qwed_new/core/safe_parser.py create mode 100644 tests/security/test_safe_parser.py diff --git a/src/qwed_new/api/main.py b/src/qwed_new/api/main.py index 67b3162c..afd8fbfc 100644 --- a/src/qwed_new/api/main.py +++ b/src/qwed_new/api/main.py @@ -457,7 +457,7 @@ async def verify_math( try: import sympy - from sympy.parsing.sympy_parser import parse_expr + from qwed_new.core.safe_parser import safe_parse_expr from sympy import simplify, symbols, Eq, solve expression = request.get("expression") @@ -472,8 +472,8 @@ async def verify_math( left_str, right_str = expression.split("=", 1) # Parse both sides - left = parse_expr(left_str) - right = parse_expr(right_str) + left = safe_parse_expr(left_str) + right = safe_parse_expr(right_str) # Simplify and check equivalence difference = simplify(left - right) @@ -501,7 +501,7 @@ async def verify_math( if re.search(r'/\d+\(', expression.replace(" ", "")): is_ambiguous = True - parsed = parse_expr(expression_normalized) + parsed = safe_parse_expr(expression_normalized) # Check for division by zero before simplifying if "/0" in expression.replace(" ", "") or "/ 0" in expression: diff --git a/src/qwed_new/core/batch.py b/src/qwed_new/core/batch.py index 666926d7..715cbe9a 100644 --- a/src/qwed_new/core/batch.py +++ b/src/qwed_new/core/batch.py @@ -220,14 +220,14 @@ async def _verify_item( ) elif item.verification_type == VerificationType.MATH: - from sympy.parsing.sympy_parser import parse_expr + from qwed_new.core.safe_parser import safe_parse_expr from sympy import simplify expression = item.query if "=" in expression: left, right = expression.split("=", 1) - left_expr = parse_expr(left) - right_expr = parse_expr(right) + left_expr = safe_parse_expr(left) + right_expr = safe_parse_expr(right) diff = simplify(left_expr - right_expr) is_valid = diff == 0 return { @@ -236,7 +236,7 @@ async def _verify_item( "message": "Identity verified" if is_valid else "Not equal" } else: - parsed = parse_expr(expression) + parsed = safe_parse_expr(expression) simplified = simplify(parsed) return { "is_valid": False, diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py new file mode 100644 index 00000000..fab11a2b --- /dev/null +++ b/src/qwed_new/core/safe_parser.py @@ -0,0 +1,199 @@ +""" +Safe wrapper around sympy's parse_expr to prevent code execution. + +sympy.parsing.sympy_parser.parse_expr() uses Python eval() internally. +Without restrictions on local_dict/global_dict, an attacker can execute +arbitrary code via crafted math expressions (CWE-95). + +This module provides safe_parse_expr() which: +1. Validates input against a denylist of dangerous patterns +2. Restricts the eval namespace to only known-safe sympy objects +3. Strips Python builtins from the global namespace + +Usage: + from qwed_new.core.safe_parser import safe_parse_expr + expr = safe_parse_expr("x**2 + 2*x + 1") +""" + +import re +import logging +from typing import Optional, Dict, Any + +import sympy +from sympy import ( + Symbol, Integer, Float, Rational, + pi, E, oo, I, +) +from sympy.parsing.sympy_parser import ( + parse_expr, + standard_transformations, + implicit_multiplication_application, +) + +logger = logging.getLogger(__name__) + +# Transformations that enable natural math input (e.g. "2x" → "2*x") +SAFE_TRANSFORMATIONS = standard_transformations + (implicit_multiplication_application,) + +# Patterns that must never appear in math expressions. +# Checked before the string reaches parse_expr / eval. +_DANGEROUS_PATTERNS = re.compile( + r"__\w+__|" # dunder attributes (__import__, __class__, …) + r"\bimport\b|" # import keyword + r"\bexec\b|" # exec() + r"\beval\b|" # eval() + r"\bgetattr\b|" # getattr() + r"\bsetattr\b|" # setattr() + r"\bdelattr\b|" # delattr() + r"\bglobals\b|" # globals() + r"\blocals\b|" # locals() + r"\bcompile\b|" # compile() + r"\bopen\b|" # open() + r"\bbreakpoint\b|" # breakpoint() + r"\bprint\b|" # print() + r"\binput\b|" # input() + r"\bvars\b|" # vars() + r"\bdir\b|" # dir() + r"\btype\b|" # type() + r"\bsuper\b|" # super() + r"\bsubclasses\b|" # __subclasses__() + r"\bmro\b|" # mro() + r"\bbases\b|" # __bases__ + r"\bos\b|" # os module + r"\bsys\b|" # sys module + r"\bsubprocess\b", # subprocess module + re.IGNORECASE, +) + + +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. + """ + safe = { + # Common symbolic variables + "x": Symbol("x"), + "y": Symbol("y"), + "z": Symbol("z"), + "a": Symbol("a"), + "b": Symbol("b"), + "c": Symbol("c"), + "n": Symbol("n", integer=True, positive=True), + "t": Symbol("t"), + "r": Symbol("r"), + "k": Symbol("k"), + "m": Symbol("m"), + "p": Symbol("p"), + "q": Symbol("q"), + "u": Symbol("u"), + "v": Symbol("v"), + "w": Symbol("w"), + # Mathematical constants + "pi": pi, + "e": E, + "E": E, + "I": I, + "oo": oo, + # Trigonometric functions + "sin": sympy.sin, + "cos": sympy.cos, + "tan": sympy.tan, + "cot": sympy.cot, + "sec": sympy.sec, + "csc": sympy.csc, + # Inverse trigonometric + "asin": sympy.asin, + "acos": sympy.acos, + "atan": sympy.atan, + "atan2": sympy.atan2, + # Hyperbolic + "sinh": sympy.sinh, + "cosh": sympy.cosh, + "tanh": sympy.tanh, + # Logarithmic / exponential + "log": sympy.log, + "ln": sympy.log, + "exp": sympy.exp, + # Roots and absolute value + "sqrt": sympy.sqrt, + "cbrt": sympy.cbrt, + "abs": sympy.Abs, + "Abs": sympy.Abs, + # Combinatorial + "factorial": sympy.factorial, + "binomial": sympy.binomial, + # Sympy internal types emitted by standard_transformations + "Integer": Integer, + "Float": Float, + "Rational": Rational, + "Symbol": Symbol, + } + + if extra_symbols: + # Only allow Symbol instances or sympy types as overrides + for key, value in extra_symbols.items(): + if isinstance(value, (Symbol, sympy.Basic)): + safe[key] = value + + return safe + + +# Pre-built global dict that strips builtins +_SAFE_GLOBAL_DICT: dict = {"__builtins__": {}} + + +def safe_parse_expr( + expression: str, + *, + transformations=SAFE_TRANSFORMATIONS, + extra_symbols: Optional[Dict[str, Any]] = None, +) -> sympy.Basic: + """ + Safely parse a mathematical expression string into a sympy expression. + + Raises ValueError if the expression contains dangerous patterns or + cannot be parsed. + + Args: + expression: The math expression string to parse. + transformations: sympy transformations to apply (default includes + implicit multiplication). + extra_symbols: Additional Symbol mappings to include in the + local namespace. + + Returns: + A sympy expression object. + + Raises: + ValueError: If the expression is rejected by safety checks or + cannot be parsed. + """ + if not isinstance(expression, str): + raise ValueError("Expression must be a string") + + stripped = expression.strip() + if not stripped: + raise ValueError("Expression must not be empty") + + # Length limit to prevent resource exhaustion + if len(stripped) > 5000: + raise ValueError("Expression too long (max 5000 characters)") + + # Deny-list check: reject expressions with dangerous patterns + if _DANGEROUS_PATTERNS.search(stripped): + raise ValueError("Expression contains disallowed constructs") + + local_dict = _build_safe_local_dict(extra_symbols) + + try: + return parse_expr( + stripped, + local_dict=local_dict, + global_dict=_SAFE_GLOBAL_DICT, + transformations=transformations, + ) + except Exception as exc: + raise ValueError(f"Failed to parse expression: {exc}") from exc diff --git a/src/qwed_new/core/validator.py b/src/qwed_new/core/validator.py index 9fe93035..ed68460e 100644 --- a/src/qwed_new/core/validator.py +++ b/src/qwed_new/core/validator.py @@ -10,7 +10,7 @@ 3. Evaluable: Can we calculate a numerical result? """ -from sympy.parsing.sympy_parser import parse_expr +from qwed_new.core.safe_parser import safe_parse_expr from typing import Dict @@ -61,7 +61,7 @@ def validate(self, expression: str) -> Dict[str, any]: # Check 1: Syntax validation try: - expr = parse_expr(expression) + expr = safe_parse_expr(expression) checks_passed.append("syntax") except Exception as e: checks_failed.append("syntax") diff --git a/src/qwed_new/core/verifier.py b/src/qwed_new/core/verifier.py index 6f887b87..d699ecd5 100644 --- a/src/qwed_new/core/verifier.py +++ b/src/qwed_new/core/verifier.py @@ -18,7 +18,7 @@ diff, integrate, limit, oo, simplify, expand ) -from sympy.parsing.sympy_parser import parse_expr, standard_transformations, implicit_multiplication_application +from qwed_new.core.safe_parser import safe_parse_expr, SAFE_TRANSFORMATIONS from typing import Any, Dict, List, Optional from decimal import Decimal, ROUND_HALF_UP from dataclasses import dataclass @@ -52,7 +52,7 @@ class VerificationEngine: """ # Parsing transformations for more natural input - TRANSFORMATIONS = standard_transformations + (implicit_multiplication_application,) + TRANSFORMATIONS = SAFE_TRANSFORMATIONS MAX_VERIFY_MATH_TOLERANCE_RATIO = Decimal("0.01") MIN_VERIFY_MATH_TOLERANCE_CAP = Decimal("0.01") @@ -120,7 +120,7 @@ def verify_math( try: # 1. Parse the expression safely - expr = parse_expr(expression, transformations=self.TRANSFORMATIONS) + expr = safe_parse_expr(expression) # 2. Evaluate deterministically if use_decimal: @@ -197,8 +197,8 @@ def verify_identity(self, lhs: str, rhs: str) -> Dict[str, Any]: verify_identity("sin(x)**2 + cos(x)**2", "1") # True """ try: - left = parse_expr(lhs, transformations=self.TRANSFORMATIONS) - right = parse_expr(rhs, transformations=self.TRANSFORMATIONS) + left = safe_parse_expr(lhs) + right = safe_parse_expr(rhs) # Method 1: Simplify difference diff = simplify(left - right) @@ -281,9 +281,9 @@ def verify_derivative( order: Order of derivative (1 for first, 2 for second, etc.) """ try: - expr = parse_expr(expression, transformations=self.TRANSFORMATIONS) + expr = safe_parse_expr(expression) var = Symbol(variable) - expected_expr = parse_expr(expected, transformations=self.TRANSFORMATIONS) + expected_expr = safe_parse_expr(expected) # Calculate derivative actual_derivative = diff(expr, var, order) @@ -328,14 +328,14 @@ def verify_integral( upper_bound: Upper bound for definite integral """ try: - expr = parse_expr(expression, transformations=self.TRANSFORMATIONS) + expr = safe_parse_expr(expression) var = Symbol(variable) - expected_expr = parse_expr(expected, transformations=self.TRANSFORMATIONS) + expected_expr = safe_parse_expr(expected) if lower_bound is not None and upper_bound is not None: # Definite integral - lower = parse_expr(lower_bound, transformations=self.TRANSFORMATIONS) - upper = parse_expr(upper_bound, transformations=self.TRANSFORMATIONS) + lower = safe_parse_expr(lower_bound) + upper = safe_parse_expr(upper_bound) actual_integral = integrate(expr, (var, lower, upper)) # For definite integrals, compare values @@ -385,9 +385,9 @@ def verify_limit( direction: "+" for right, "-" for left, "+-" for both """ try: - expr = parse_expr(expression, transformations=self.TRANSFORMATIONS) + expr = safe_parse_expr(expression) var = Symbol(variable) - expected_expr = parse_expr(expected, transformations=self.TRANSFORMATIONS) + expected_expr = safe_parse_expr(expected) # Parse the point (handle infinity) if point.lower() in ['oo', 'inf', 'infinity']: @@ -395,7 +395,7 @@ def verify_limit( elif point.lower() in ['-oo', '-inf', '-infinity']: pt = -oo else: - pt = parse_expr(point, transformations=self.TRANSFORMATIONS) + pt = safe_parse_expr(point) # Calculate limit actual_limit = limit(expr, var, pt, dir=direction) diff --git a/tests/security/test_safe_parser.py b/tests/security/test_safe_parser.py new file mode 100644 index 00000000..f76b8ab0 --- /dev/null +++ b/tests/security/test_safe_parser.py @@ -0,0 +1,97 @@ +""" +Security tests for CWE-95: parse_expr code injection prevention. + +Verifies that safe_parse_expr blocks code execution payloads while +allowing legitimate mathematical expressions. +""" + +import pytest +from qwed_new.core.safe_parser import safe_parse_expr + + +class TestSafeParseExprBlocksCodeExecution: + """Verify that crafted expressions cannot execute arbitrary code.""" + + @pytest.mark.parametrize( + "payload", + [ + '__import__("os").system("id")', + '__import__("os").getpid()', + '__import__("subprocess").check_output("id")', + 'exec("import os")', + 'open("/etc/passwd").read()', + 'getattr(int, "__subclasses__")', + '().__class__.__bases__[0].__subclasses__()', + '__builtins__["__import__"]("os")', + 'globals()["__builtins__"]', + 'locals()', + 'compile("1", "<>", "exec")', + 'type("X", (), {})', + 'os.system("id")', + 'sys.exit(1)', + 'breakpoint()', + 'dir()', + 'vars()', + 'setattr(x, "y", 1)', + 'delattr(x, "y")', + 'subprocess.call("id")', + ], + ids=lambda p: p[:40], + ) + def test_dangerous_payload_is_rejected(self, payload: str) -> None: + with pytest.raises(ValueError, match="disallowed constructs"): + safe_parse_expr(payload) + + @pytest.mark.parametrize( + "payload", + [ + 'print("hello")', + 'input("prompt")', + ], + ids=lambda p: p[:40], + ) + def test_io_functions_blocked(self, payload: str) -> None: + with pytest.raises(ValueError, match="disallowed constructs"): + safe_parse_expr(payload) + + +class TestSafeParseExprAllowsLegitMath: + """Verify that normal math expressions still parse correctly.""" + + @pytest.mark.parametrize( + "expression,expected_str", + [ + ("2+2", "4"), + ("x**2 + 2*x + 1", "x**2 + 2*x + 1"), + ("sin(x)", "sin(x)"), + ("cos(x)**2 + sin(x)**2", "sin(x)**2 + cos(x)**2"), + ("pi", "pi"), + ("sqrt(16)", "4"), + ("3/4", "3/4"), + ("log(x)", "log(x)"), + ("exp(1)", "E"), + ("factorial(5)", "120"), + ("atan2(1, 1)", "pi/4"), + ], + ) + def test_valid_expression_parses( + self, expression: str, expected_str: str + ) -> None: + result = safe_parse_expr(expression) + assert str(result) == expected_str + + +class TestSafeParseExprInputValidation: + """Verify input validation guards.""" + + def test_rejects_empty_string(self) -> None: + with pytest.raises(ValueError, match="must not be empty"): + safe_parse_expr("") + + def test_rejects_non_string(self) -> None: + with pytest.raises(ValueError, match="must be a string"): + safe_parse_expr(123) # type: ignore[arg-type] + + def test_rejects_oversized_input(self) -> None: + with pytest.raises(ValueError, match="too long"): + safe_parse_expr("x + " * 2000) diff --git a/tests/test_api_exceptions.py b/tests/test_api_exceptions.py index e02f7067..006ee364 100644 --- a/tests/test_api_exceptions.py +++ b/tests/test_api_exceptions.py @@ -31,7 +31,7 @@ def test_verify_math_exception_handling(): Test that verify_math catches internal errors and returns a sanitized message. """ # Force an exception during parsing/processing - with patch('sympy.parsing.sympy_parser.parse_expr', side_effect=ValueError("CRITICAL SENSITIVE STACK TRACE")): + with patch('qwed_new.core.safe_parser.parse_expr', side_effect=ValueError("CRITICAL SENSITIVE STACK TRACE")): # Providing valid minimal input to pass Pydantic validation response = client.post( "/verify/math", From bbf9ade18facaa8ed47b15d443798d641e6bd4b4 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Sat, 13 Jun 2026 09:49:14 +0100 Subject: [PATCH 2/8] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20add?= =?UTF-8?q?=20Greek=20symbols,=20copy=20global=5Fdict=20per=20call,=20vali?= =?UTF-8?q?date=20variable=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/qwed_new/core/safe_parser.py | 78 ++++++++++++++++++++++++++++-- src/qwed_new/core/verifier.py | 5 +- tests/security/test_safe_parser.py | 77 ++++++++++++++++++++++++++++- 3 files changed, 155 insertions(+), 5 deletions(-) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index fab11a2b..033a4e97 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -65,6 +65,40 @@ 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: """ @@ -72,9 +106,11 @@ def _build_safe_local_dict(extra_symbols: Optional[Dict[str, Any]] = None) -> di 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: diff --git a/src/qwed_new/core/verifier.py b/src/qwed_new/core/verifier.py index d699ecd5..9f74a469 100644 --- a/src/qwed_new/core/verifier.py +++ b/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) diff --git a/tests/security/test_safe_parser.py b/tests/security/test_safe_parser.py index f76b8ab0..e7318ada 100644 --- a/tests/security/test_safe_parser.py +++ b/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] From e1454de442cf5cea64fa440929c226e5a8d9aefa Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 15:05:07 +0530 Subject: [PATCH 3/8] fix: resolve Sentry symbol mismatch and address CodeRabbit feedback - Add get_safe_symbol() to return consistent Symbol objects matching safe_parse_expr namespace (fixes 'n' variable mismatch in calculus) - Add AST depth limit and post-parse sympy.Basic type validation - Add comment documenting Symbol in allow-list boundary - Strengthen global dict isolation test - Update verifier.py calculus methods to use get_safe_symbol --- .coverage | Bin 77824 -> 53248 bytes src/qwed_new/core/safe_parser.py | 373 +++++++++++++---------------- src/qwed_new/core/verifier.py | 14 +- tests/security/test_safe_parser.py | 337 ++++++++++++++------------ 4 files changed, 352 insertions(+), 372 deletions(-) diff --git a/.coverage b/.coverage index 89f54e538b5c45fe5b8a94d37789712fd8d7c876..f626ef2cbe5b6db11cf63e18b43545bbf4169f25 100644 GIT binary patch delta 231 zcmZp8z|ydQd4fD6??#0O_FTLS3=E9?GZ^@1Y!(#g<7Z1_WMOCwoP5wTYI=htBL^23 zSbio0|4g8KKR;_4OJg7>TzXRiivk-X{{#m92|(#K{`x#dR!+{wI3|YwR~s1_4oEV5 zke6^M-~5Sd$@Fuprw7?FvfTJv%*w{F;C{ce@$VlEcE8U53ck9X;f$WWf_k30Z024Y(tpET3 literal 77824 zcmeHw3wRV&n)a#cO5eLGkU$85q`8Jb2$ygTBx&v-w*Vrd8JbFWrBkGD>F$ICfpmkT z_>Vg{=#F=jS@&OcbVb*390!EBI>NhiU1W}cm+ zk8ahI^jDWt^`7rN-}jwM)v4OFVT~M+%%vWmOAMHEQ6gd(WHFl&LOS?Qg8$f}ffX*c z0g{PZ|E5(P%Bi}`B;16I>YI_U%`{S&YIGPc8Xhsu(``1$nrvMT98ea80fhmD0fm7d z3Of=%Ttt!`Zk)nADSCRA*(a4sKFMv9{E=KtlI>agto^lmO-fP{ zbEiaf5`8xKGxXC2cJ!&TsuV6{^Z4vI_a#2jZ7Y-f*=Dg+`s#1w3Y_ z>?Ufu{c=F|xXn_vWUCBF_UkH`jY*3wKwi8yaINiFq3w|~qgZk5^qqX2Go#6)CznFz z+@2V%eJ5gQIGN33>vIJ1xkbBWVa(%;sI#rP#~5Qqh_WTlmE8+H{x z*1_hfl6;~=%84G0goW?gkic|i=+r68GQP=ldQHGOqjyywN>yu8($biHI$}oA(uxXA zKNE_Tl=e;PMAKPkL|3#S4W~&N zHjD{HJOalh{GZTI3B0Gz1qM_Ee|z@5VxY`CIPZ8jWIQuLow8uqH)R}W!8!x~s&7wZ zNtO?&2>UJw`uJbfFUxpOUzS%(HB+lZCqxskDEq)=AqGck05eW#7a_4&;;9S}`^B5Y zJaOIAm*2<*`nF@-L#iy29NfFeS9SpBLL}&MXuCI12eQ%_Sa>Qxe^gx&h8u zc5BzcNeE;!8J2u>ICFIaK8M7KR!U*T#va#e`^*f}vy zB#6L?=1g!(Ke*_CYsM}bJ&Ge%?D{!g-~?aQCE6qIjteF3iaX1sL6?ivMoYzj6p&q# z87Co`A%?Y6^qXxy2@XcaW=jE@JBp{EK_ZTHKTi zWiXnQDN~qGWz>Ts!O7}}mVir)=^EW+`&rh2gMCJ}9-=ffk>ZUD6CTGQ-i#pNh!`pM zk_dINRM7^KKFb8TWP~mRf6AgTpfI2?pfI2?pfI2?pfI2?pfI2?pfI2?pfK>mV?f2Q zj0X4rS>X&4F2IknC=4hJC=4hJC=4hJC=4hJC=4hJC=4hJC=4hJd@mU=vZ~QS?6HvP ziHs^OHTn#|+?*M?IWzR26)T)W!a3pc_fkP6(+UF$0}2BQ0}2BQ0}2BQ0}2BQ0}2BQ z0}2BQ1HB9wRil~MFaWDprRk!h011$S6|UfPLxnEkis^Hc-=s4>Zk%U$(_qzi=@02A z=#J=CYfo#-HJzF}HN({P>a2ucCFFB&a$8k@QSDKg*oW9G<`}aXeThQ7fPd?XV?2jK zo0jAiZ}!6+kz$`%R_QFxaY!!NEf6HF(S zid`PN=a*kZo27oN}p5bgWDTP!APQug_B@+i^tnIGv(Pw8nM- zwf+&*bd8|^tQQNdQSFp!Y#SNH6nwys`}deM13Irp@XunBxp8 zV&etO9YX+fD`m`R4e3q(rjn98gbAC`fow{R@1dzi%nyLYo*m z-rcYNKYj-n+CmX1uIR?m(}}oSZ~uQRJ)HpQJmkZhG&_|#K_5aO2F65vHc%A-# zhKCD@G$9A{|I^)E$Vw4!K>vTVlMCHS8LqegKk8f1EzafA0$iw^u5N?-|0AookcSRv zUyFZm|38(Uu5qvf`u`)o4P6KK|5Lt2U3>fg!z;LuM7v!3_5X*_3wC`)+Q0vwOfR7K zLB{w0ll)vrrp{{r{{PT&F0_?4X0-o5gkJ5@M`&EgOO(0LW;(i&8o96kFHo*H`t|>L z%8jSq{=dn_g;vqxFWUb%QdXw7|8JmYe8d-7KC^kO}M9Mu0$pllv-{eO;HaDslUK(7_+s1$v6Kir>#A$hV6!wUrf z$y3t@u;Au#zwr!}^8UZSZ?r;5tuUZ4pfI2?pfI2?pfI2?pfI2?pfI2?pfK<~V?f2I zkO|NKXM}D9f6AgTpfI2?pfI2?pfI2?pfI2?pfI2?pfI2?pfKG1vkjjRm` z9l}K+OE3#M{wjZg|Cm3*Kfxd3_whIIxxB{okg3v?ZxW34#@$Ap;S<9LhPMn48%hns z^=I@y)i2hy=>Djy*4cGSbV=I3XkXL*Qu~0`qRrHf&@!5fnxAMqnnF#I`fYWUnol^M zaDT$x39f{-3AqU)xqsmv;(p9+=Zd%jZZu?2dPjASYQO46)hyL;wu?Q*zQ#Vm z)vf+Z-y5o(<@7#0xc>bP|6F0!Ib;H@OXP&y){@%`~ z4;LF-PKIw=HtI_4mJ?!okM++V?|!VOK%Z6w>E?}K&-_STf8_D^w}h-8+q#FJd2ZC} zbC(Fi*Tcb_RJh2mI-WV4mvr`}?w|jz?2h!&>tO%FC2aA!%;$DIb@k*E$F9D-H`F-$ zbXR*j6PTO277ou!`Sg{~Ua71N_jDdU-l5*zwYTfS*LkDXz@fYa?3w!uOh?y-pTe{^ ztG3_b*8O^#OGsZ0M+@@UaQT*7kDMwpAk^?*mu_1)>F7t@U!Q1o2vcu>m!V>FIWj1)k8m>3*Qj!{fuZdB}$B~kVBD; z$VJH!OX3PRdF(K@(8S{JkD6BvoxdFR1d={Yi)1C(djVJ>UnoVWcVA@N5>3LXm%*ug zA~R$u$nk5Bz5e{>%NM5{$*J>bG)rJtVxfCz;$m1CyXaI?Cc5p5LZ+oApI-!f(u?M_ ztsYvnsOcIr&H~#jEiBrreth;h0WLmj@A1%w)81V#R9AWnt?kRZPOFj%A+(Ds-CfjMw% z=U{S1u47Tl?w;xW3=*5E0O{-g<_)t|k8((hCGPb+BwT?l-;CZbjSljl-x@%88 zT9>>Mpxg1ZF<3a%b@o_>YIPKR1WH3N!Wd8^)BXcYoQ%Sg=LmC2?Ni@?3mT;~KW5yQ3}_ z1#7#t?e8;ZAcHf}s+lkJoIf49l4*XjcAu|MtxYs1-Zy^!{LNzWedDv948mfuG@Yzv zx|7-_&jjvo((Rj8caNipLuLk98cpysouyIWr*bw&iTg3o}t;jlSV- z>EZhsbn3{IYwJ4=FTOXq`p0aL#m)&PRQBFpe&lpWKSy96U{LbLf91A$DfZd;;l0 zGVZmcTx`9&_rIEP(`20K?4@ild@0=RfL!0K3ATnXp^OSLA6uf9)@&qa%kn9(Jy$Hhy9Mr@a9qZCNN-usp0KD<5h}@N#w6aJxOnLzlxUvb&LFR9=r~9) zajd)h^}^qUb8FAm&FxWb8;f~cn2?C7dpc5)C9&Zvd3`=Y58ZV!Y+{dy$5`qZTSE=fsc|^Ypp(daRFXtk%ZTJ|#%$doyzcL3L0BeSRag0SpMP`g^ zzJU!k*Y>0$vzgnT57yirJspyflENlS>J)rk>|}x4cg6zRE@Vcy1fkjs0`3hN_=?Gk zp+Kwm_FO(u7d9eGuH&bf+g0bFWLI_WJieE+OwRY7exvK+-c#%0`rtp?4(mSe?(Qy- zKHvTONB;26iKQPOU${XiN+-En#ReM-!;m{`<|9?y?{b zeW-y;p=VU60e(R%*xo)G-JF4f@bc^Dz}4~)x{`!~>qg;xj2g*W-wqptl_*)u?H+z+ z4Ff@&LnBfBSWq6qfflw9+Vw%@<<+%MAK$&D=l$cGt}XacS?)E(Ne-S#9^#|1_!zzr#c-^0$uzd5uC=4PG2C6pSTtS(*3gV?A&8I>wO@Q6Jy5GY{OT8TM_3~E(1sfR za=#T_bA&B21Ym45U`A1=y{9R^ujKAmkb z-1i=_zi7OGLWnK1T*)|Fx6EZ5YkM3Bb=Q6N`-`;&OiycFv(Vkqttkl4Z^_WIX$ zf<W4;C*w3LTdLMHr(CPS{Lr_dH&(A{Ob+g`xWe51r@=3@B* zH!M%Yn%}gL4Zq#o+}PMySQoDC>1b{a*M-9E?=&9&_uHGo+tvHSPpofi+rOygXW@Ht zFLhYMKRxzrdUe||bNkhu$C__D_|^L39b@alSDyOlGxzK3|8(;3=*v$}M&U`DHcnIN z#utsoim~j<$pYl)rfjxayg3XGibf+#EkYz5Tfzs!_r8<|#y%R7<|Fek9@1t{Wvfrm z{ru0T9@u*J#3TQd+sMu_VKUS#Y5?Qus0*Jwc{0^}#*uG%>*dV3;R&Iy!xwrEHh9>1 zU8f&suZ8RC&>zA-3nrO)Bb=AcvP=-639AlqNe0-SWZd`B+;g=PR1Y@K&&_Qx z42=eMBu>82mZ*l6v6-Jfl3+oWr6_p7GAsf1WM^I4c@39G0cs!?nXcjR8RL9RXqgJi zW4~V8^Vyzb=RfJb8I|Q-uKDf7|F3FDL!X_Ti;J;AQ!fB7e7W}v3Jt5?6=)VTl3SIsZ=)HLqD zaJco>aPT?C`x`%M!Pf?Wqo+?^Mz2y4U@BeQ!y@uf3f6oeY`7J`S z@PyDNe8vB(AoA}DFY*_8iT@8igIDo-euSx;A7Xmj)NJ~HrauVL|cBmbpvT6mBDKzLB7=bb{C;Nks3n(!0;tguJ8U8v&U;J2ABn=S~4 z_#%E4Kb2q1&l0jsXZTb6LH=p}=cd<$>B3n4W&TlqB7ZdE>h((KuC|X2>#X>|uArbinL~!{;%wI?ZzkrCG zJR)YyC&E0Bh>dfJST~1=WwVJ`F^h;bxkTt@60v*+5yEsLR!<{hLki$|NFVA`$5mh*&?Kh_T~{NEl1R%rQjVU?yU51`(^$iBOFuV&o_y zMx_xkdL$8PsYHw!L4-bqh-t%#m_Ce%xyeM#P9kE?P$F`N5V4R9P%zm7*_z4|p*9i0 z8i_C(h?uA+V!V!s30fj1Ylz5H6Oolb#5j%!t%?W@%fSEux&I#>-1=YaL5k`M0}2BQ z0}2BQ0}2BQ0}2BQ0}2BQ0}2BQ13zpA@cnHjNspfI2?pfI2?pfI2?pfI2?pfI2?pfI2?pfK>mW&q#+AIsd1 zgolM0s;Bw4`R%Ga_HENe(;cQ%<8#IphEs-WgGv98ev0n5x*N2owH0i!R>!n4w`(5M zj8i|WE=YJk!OmUg4sb~fhaUb1vPJpRseRvprTjh4>wRdxk_)+dKWR$+)2;A@NvRq> zs2cG2;3JbaJGjt>Uhra{SXSvQ&WU__yBI#$EIWK+l^m!k_BzEH2Yg}HZ7;5pY;f{a zmlTjZK96iKuGlHrt!`;&vEOIw+lYKBRrz9I+F57PXV;pEBgGzQby6Ul$eI_sa2O(b`rSJ-vV6RXTj4be|k60B*##XT<(C+ z z#ujO$rw(bM_@PDVgLWc7O{CXWCs5D`1-AyE@sxpj&2Iv|1_{x~CEM*zX{YGJzC4+7 zNySy!ne@CkuG!jKURjhii*u=Q5*J!a$E9%l*d@xiM$yV#Xfr*OIDIsB&_}l}B@{mE2e z>@*%!?J%Yre#v$)FB|6T-_UQ-eW}|EZ~gx_hDC?|5xswU^{Ch9sgmu4%)V-ECA~If zAD+i6xkcG(mwnKW1}hr=KL;)6LR;zBA*ucYl2d}-KB@UPQpWO&ZaE$ zk6Xeu)_Tg#rvQYDfK_yh&YFl~>*xhuQegfHXMB5pO$8T{DB6b~#&}#}JmPA~jLI%( zA;tsVK$%gW7?7+^*(FDMWRvJMJxBq5r{j_If-cd`Q-V7#Vu|dGv=`GUbJ^RB8%xZIDI%R14lTrWq^f)AH1$?60?~H=+lno@{fl)!xiyK&?6KeAu%C%jf z%qNKfs~v8C;QJteID>Lv2|$Ap8W1od8IY=RlhN$wLNd(?y~b7IarPgTUq)F;JW$tf ztaSxt--{cv&{c6E4^2Mt=!gUb*#G9+a(ewo;)aoS86GMTXcwMX3M1Cwf0a?EpGUb; zCxZ&u_Y9QlSf6B<{n+&)<7iX<=bg2`dcPDxi{ImhQRMh4c4;XWT2IkDeI=p|sGNnL z{~wP+NO(w?#{Vb3#q_z!Z_*hbH_kJ>X|U?M^oR5lbVqcn;m&`#rc-mLW|+EOot5yb zgnaHzZma4qsy!+b`w*MO9Ah@4FX2Le@8THGq0lBO!yNsG4&`wGw1pl}Tob}EmYPn) z$$Oi(@)&A55umt%LysAtY*grrv>ygmcoi7{=%5F9oyRiV=>REGgdETfa*YN&D?L1X z*E{Y;bonTNyOl0n?@e}B8hzd3T+Tld;L54$Hu$N9%2WXLPyvk_J9Iil7-@;^fS~Ok zK~2{fioZBIWQom3#O=?@ly5=T$U{i|bcHVqQYzO$y7mswdWQk1M7dm{cJf&LWpXKI zw;~x3=>)qzHEm9KhDow|yp_0p=t%-dIsv^868bP@K95(%Z4KX00F-I7x(m0uBgPAu zJB9$}R?3*s2ejOYfJLYF=p!^P6>K$h- zF#^ykO8iCp4h{odWqSKWZao0eF+Rc?9*Bs46ie0t9-Y>7#Q6FiF0g9>YAuy=io3B} zrU4W>-h@%&{I)_3NOWR7q3$|Qws;Z%l1{8Akb{Q7Wexx-nn&CqYPkxaC|LVt_y zye_Cq)jqFXqIq9aqQ0UIse3MZ7Xd1rE=BYf7HDjf{C+Fh#LaCBAkwj-D5BltaY3ipj~PkHMAa100=-CJGl9m)#2ijADCk;N!$q3jnWzO3C%Pq76pY z;VEWWbXxsz8x24D7{~)$I^GzEONR2TE_fU#GFNEFd_bksEhI4@KHhld0T#_#Yv6qt z_gny^Xgb6x;u>JiIedxqf_G%xWV`21JePKPAnmacr0}AOdbC;K&9Any6&VTcMgD3 z^znfYSh%JF9!1+3F!{ri4R91=W&|#7a*JyUUHt~!QZAniaCDjygJ&1GvZ(2Hy+{6C zlc>QFFC6e#rkn|IbOI>RF76hnJdrL`+|wzJ2>?ZB5Hw;Sv1!b>|35_g91`vk%=}?K z-}J6&oAE2-kBupYrwxVrkMyOwtGWX^GbB+Kg#m>Dg#m>Dg#m>Dg@Nyc0r$<&FrwJ; z8Th#3C%=`zM9eR|db4jnU$ z3`q@~^yyv;fOMi<z5dQtBnrwFdAgMv(!NOuefCj^YsC;7O*Q8|aqcph>3gRn$t5 zn9RVbZtj)T@cQnD#0`Am`Tt3pJxI7)vqu=oKchL%FEqVv+G_mFSYuQh?lFwe|3aUw zdr@c6zOB9WJE`&&P=x`70fhmD0fm7d76bI&g?F7sb`iZf+1G80uBC@}-ImPv^+i74 zG&i(Q$$?XGM$ntO90xjR+EFUKd9Z!31E1Dg@@?okc-m3QPA*izP#t^fc4 diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index 033a4e97..15555fd0 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -1,28 +1,27 @@ """ -Safe wrapper around sympy's parse_expr to prevent code execution. +Safe SymPy expression parser. -sympy.parsing.sympy_parser.parse_expr() uses Python eval() internally. -Without restrictions on local_dict/global_dict, an attacker can execute -arbitrary code via crafted math expressions (CWE-95). +Wraps sympy.parsing.sympy_parser.parse_expr with input validation, +a denylist for dangerous constructs, and a restricted evaluation +namespace. This module is the ONLY approved entry point for parsing +user-supplied math expressions in production code. -This module provides safe_parse_expr() which: -1. Validates input against a denylist of dangerous patterns -2. Restricts the eval namespace to only known-safe sympy objects -3. Strips Python builtins from the global namespace +Security boundary: + 1. Reject known-dangerous Python/OS constructs (denylist). + 2. Remove __builtins__ from the eval global dict. + 3. Allow-list only expected math symbols, constants, and functions. + 4. Enforce basic input validation (type, length, empty check). -Usage: - from qwed_new.core.safe_parser import safe_parse_expr - expr = safe_parse_expr("x**2 + 2*x + 1") +CWE-95 mitigation -- see PR #200 for full security analysis. """ +import ast import re -import logging -from typing import Optional, Dict, Any +from typing import Any, Dict, Optional, Tuple import sympy from sympy import ( - Symbol, Integer, Float, Rational, - pi, E, oo, I, + E, I, Integer, Float, Rational, Symbol, oo, pi, ) from sympy.parsing.sympy_parser import ( parse_expr, @@ -30,242 +29,190 @@ implicit_multiplication_application, ) -logger = logging.getLogger(__name__) - -# Transformations that enable natural math input (e.g. "2x" → "2*x") -SAFE_TRANSFORMATIONS = standard_transformations + (implicit_multiplication_application,) - -# Patterns that must never appear in math expressions. -# Checked before the string reaches parse_expr / eval. -_DANGEROUS_PATTERNS = re.compile( - r"__\w+__|" # dunder attributes (__import__, __class__, …) - r"\bimport\b|" # import keyword - r"\bexec\b|" # exec() - r"\beval\b|" # eval() - r"\bgetattr\b|" # getattr() - r"\bsetattr\b|" # setattr() - r"\bdelattr\b|" # delattr() - r"\bglobals\b|" # globals() - r"\blocals\b|" # locals() - r"\bcompile\b|" # compile() - r"\bopen\b|" # open() - r"\bbreakpoint\b|" # breakpoint() - r"\bprint\b|" # print() - r"\binput\b|" # input() - r"\bvars\b|" # vars() - r"\bdir\b|" # dir() - r"\btype\b|" # type() - r"\bsuper\b|" # super() - r"\bsubclasses\b|" # __subclasses__() - r"\bmro\b|" # mro() - r"\bbases\b|" # __bases__ - r"\bos\b|" # os module - r"\bsys\b|" # sys module - r"\bsubprocess\b", # subprocess module +__all__ = ["safe_parse_expr", "validate_variable_name", "get_safe_symbol", "SafeParserError"] + +MAX_EXPRESSION_LENGTH = 5_000 +_AST_MAX_DEPTH = 30 + +_DENYLIST_PATTERN = re.compile( + r"(?:" + r"__import__|__builtins__|__subclasses__|__globals__|__locals__" + r"|__getattr__|__setattr__|__delattr__|__class__|__bases__|__mro__" + r"|\beval\b|\bexec\b|\bcompile\b|\bgetattr\b|\bsetattr\b|\bdelattr\b" + r"|\bimport\b|\bimportlib\b" + r"|\bos\b|\bsys\b|\bsubprocess\b|\bshutil\b|\bsocket\b" + r"|\bpopen\b|\bsystem\b|\bspawn\b" + r"|\bopen\b|\bfile\b|\bpath\b|\bglob\b" + r"|\bchr\b|\bord\b|\bhex\b|\btype\b|\bvars\b|\bdir\b|\brepr\b" + r"|\binput\b|\bprint\b|\bbreakpoint\b|\bexit\b|\bquit\b" + r"|\bcodecs\b|\bcode\b|\bctypes\b" + r")", 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}$") +_SAFE_GLOBAL_DICT_TEMPLATE: Dict[str, Any] = {"__builtins__": {}} -def validate_variable_name(name: str) -> None: - """ - Validate a user-supplied variable name before it reaches Symbol(). +def _check_ast_depth(expression: str) -> None: + """Reject expressions whose AST exceeds max depth (DoS defence).""" + try: + tree = ast.parse(expression, mode="eval") + except SyntaxError: + return + depth = _ast_node_depth(tree) + if depth > _AST_MAX_DEPTH: + raise SafeParserError( + f"Expression AST depth {depth} exceeds limit of {_AST_MAX_DEPTH}" + ) - 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") +def _ast_node_depth(node: ast.AST, current: int = 0) -> int: + max_depth = current + for child in ast.iter_child_nodes(node): + child_depth = _ast_node_depth(child, current + 1) + if child_depth > max_depth: + max_depth = child_depth + return max_depth - 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" +def _validate_sympy_result(result: Any) -> None: + """Ensure parse_expr returned an expected SymPy type.""" + import sympy + if not isinstance(result, sympy.Basic): + raise SafeParserError( + f"Parsed result is not a valid SymPy expression, got {type(result).__name__}" ) - 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 single-letter symbolic variables - "x": Symbol("x"), - "y": Symbol("y"), - "z": Symbol("z"), - "a": Symbol("a"), - "b": Symbol("b"), - "c": Symbol("c"), +def _build_safe_local_dict( + extra_symbols: Optional[Dict[str, Any]] = None, +) -> Dict[str, Any]: + safe: Dict[str, Any] = { + "x": Symbol("x"), "y": Symbol("y"), "z": Symbol("z"), + "a": Symbol("a"), "b": Symbol("b"), "c": Symbol("c"), + "d": Symbol("d"), "f": Symbol("f"), "g": Symbol("g"), + "h": Symbol("h"), "k": Symbol("k"), "m": Symbol("m"), "n": Symbol("n", integer=True, positive=True), - "t": Symbol("t"), - "r": Symbol("r"), - "k": Symbol("k"), - "m": Symbol("m"), - "p": Symbol("p"), - "q": Symbol("q"), - "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"), + "p": Symbol("p"), "q": Symbol("q"), "r": Symbol("r"), + "s": Symbol("s"), "t": Symbol("t"), "u": Symbol("u"), + "v": Symbol("v"), "w": Symbol("w"), + "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"), "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, - "E": E, - "I": I, - "oo": oo, - # Trigonometric functions - "sin": sympy.sin, - "cos": sympy.cos, - "tan": sympy.tan, - "cot": sympy.cot, - "sec": sympy.sec, - "csc": sympy.csc, - # Inverse trigonometric - "asin": sympy.asin, - "acos": sympy.acos, - "atan": sympy.atan, + "pi": pi, "e": E, "E": E, "I": I, "oo": oo, + "sin": sympy.sin, "cos": sympy.cos, "tan": sympy.tan, + "cot": sympy.cot, "sec": sympy.sec, "csc": sympy.csc, + "asin": sympy.asin, "acos": sympy.acos, "atan": sympy.atan, "atan2": sympy.atan2, - # Hyperbolic - "sinh": sympy.sinh, - "cosh": sympy.cosh, - "tanh": sympy.tanh, - # Logarithmic / exponential - "log": sympy.log, - "ln": sympy.log, - "exp": sympy.exp, - # Roots and absolute value - "sqrt": sympy.sqrt, - "cbrt": sympy.cbrt, - "abs": sympy.Abs, - "Abs": sympy.Abs, - # Combinatorial - "factorial": sympy.factorial, - "binomial": sympy.binomial, - # Sympy internal types emitted by standard_transformations - "Integer": Integer, - "Float": Float, - "Rational": Rational, + "sinh": sympy.sinh, "cosh": sympy.cosh, "tanh": sympy.tanh, + "log": sympy.log, "ln": sympy.log, "exp": sympy.exp, + "sqrt": sympy.sqrt, "cbrt": sympy.cbrt, + "abs": sympy.Abs, "Abs": sympy.Abs, + "factorial": sympy.factorial, "binomial": sympy.binomial, + "Integer": Integer, "Float": Float, "Rational": Rational, + # Symbol is required because SymPy standard_transformations may emit + # Symbol('name') during evaluation. This allows users to create symbols + # with arbitrary names — the denylist and stripped builtins mitigate + # downstream attribute-access risks on resulting objects. "Symbol": Symbol, } - if extra_symbols: - # Only allow Symbol instances or sympy types as overrides for key, value in extra_symbols.items(): if isinstance(value, (Symbol, sympy.Basic)): safe[key] = value - return safe -# 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__": {}} +class SafeParserError(ValueError): + pass def safe_parse_expr( expression: str, *, - transformations=SAFE_TRANSFORMATIONS, extra_symbols: Optional[Dict[str, Any]] = None, -) -> sympy.Basic: - """ - Safely parse a mathematical expression string into a sympy expression. - - Raises ValueError if the expression contains dangerous patterns or - cannot be parsed. - - Args: - expression: The math expression string to parse. - transformations: sympy transformations to apply (default includes - implicit multiplication). - extra_symbols: Additional Symbol mappings to include in the - local namespace. - - Returns: - A sympy expression object. - - Raises: - ValueError: If the expression is rejected by safety checks or - cannot be parsed. - """ + transformations: Optional[Tuple] = None, +) -> Any: if not isinstance(expression, str): - raise ValueError("Expression must be a string") - + raise SafeParserError( + f"Expression must be a string, got {type(expression).__name__}" + ) stripped = expression.strip() if not stripped: - raise ValueError("Expression must not be empty") - - # Length limit to prevent resource exhaustion - if len(stripped) > 5000: - raise ValueError("Expression too long (max 5000 characters)") - - # Deny-list check: reject expressions with dangerous patterns - if _DANGEROUS_PATTERNS.search(stripped): - raise ValueError("Expression contains disallowed constructs") - + raise SafeParserError("Expression is empty") + if len(stripped) > MAX_EXPRESSION_LENGTH: + raise SafeParserError( + f"Expression exceeds maximum length of {MAX_EXPRESSION_LENGTH} characters" + ) + match = _DENYLIST_PATTERN.search(stripped) + if match: + raise SafeParserError( + f"Expression contains disallowed construct: {match.group()!r}" + ) + _check_ast_depth(stripped) local_dict = _build_safe_local_dict(extra_symbols) - + if transformations is None: + transformations = standard_transformations + ( + implicit_multiplication_application, + ) + global_dict = dict(_SAFE_GLOBAL_DICT_TEMPLATE) try: - return parse_expr( + result = parse_expr( stripped, local_dict=local_dict, - global_dict=dict(_SAFE_GLOBAL_DICT), + global_dict=global_dict, transformations=transformations, ) + _validate_sympy_result(result) + return result + except SafeParserError: + raise except Exception as exc: raise ValueError(f"Failed to parse expression: {exc}") from exc + + +def validate_variable_name(variable: str) -> str: + if not isinstance(variable, str): + raise SafeParserError( + f"Variable name must be a string, got {type(variable).__name__}" + ) + stripped = variable.strip() + if not stripped: + raise SafeParserError("Variable name is empty") + if len(stripped) > 50: + raise SafeParserError("Variable name is too long") + if not re.match(r"^[A-Za-z][A-Za-z0-9_]*$", stripped): + raise SafeParserError( + f"Invalid variable name: {stripped!r}. " + "Must start with a letter and contain only alphanumeric characters." + ) + match = _DENYLIST_PATTERN.search(stripped) + if match: + raise SafeParserError( + f"Variable name contains disallowed construct: {match.group()!r}" + ) + return stripped + + +def get_safe_symbol(name: str) -> Symbol: + """Return a Symbol consistent with safe_parse_expr's namespace. + + Ensures calculus operation variables match any special assumptions + (e.g. Symbol(\"n\", integer=True, positive=True)) applied during parsing, + preventing symbol mismatch in diff/integrate/limit. + """ + name = validate_variable_name(name) + safe = _build_safe_local_dict() + if name in safe: + sym = safe[name] + if isinstance(sym, Symbol): + return sym + return Symbol(name) diff --git a/src/qwed_new/core/verifier.py b/src/qwed_new/core/verifier.py index 9f74a469..ee4ec757 100644 --- a/src/qwed_new/core/verifier.py +++ b/src/qwed_new/core/verifier.py @@ -18,7 +18,8 @@ diff, integrate, limit, oo, simplify, expand ) -from qwed_new.core.safe_parser import safe_parse_expr, validate_variable_name, SAFE_TRANSFORMATIONS +from qwed_new.core.safe_parser import safe_parse_expr, validate_variable_name, get_safe_symbol +from sympy.parsing.sympy_parser import standard_transformations, implicit_multiplication_application from typing import Any, Dict, List, Optional from decimal import Decimal, ROUND_HALF_UP from dataclasses import dataclass @@ -52,7 +53,7 @@ class VerificationEngine: """ # Parsing transformations for more natural input - TRANSFORMATIONS = SAFE_TRANSFORMATIONS + TRANSFORMATIONS = standard_transformations + (implicit_multiplication_application,) MAX_VERIFY_MATH_TOLERANCE_RATIO = Decimal("0.01") MIN_VERIFY_MATH_TOLERANCE_CAP = Decimal("0.01") @@ -282,8 +283,7 @@ def verify_derivative( """ try: expr = safe_parse_expr(expression) - validate_variable_name(variable) - var = Symbol(variable) + var = get_safe_symbol(variable) expected_expr = safe_parse_expr(expected) # Calculate derivative @@ -330,8 +330,7 @@ def verify_integral( """ try: expr = safe_parse_expr(expression) - validate_variable_name(variable) - var = Symbol(variable) + var = get_safe_symbol(variable) expected_expr = safe_parse_expr(expected) if lower_bound is not None and upper_bound is not None: @@ -388,8 +387,7 @@ def verify_limit( """ try: expr = safe_parse_expr(expression) - validate_variable_name(variable) - var = Symbol(variable) + var = get_safe_symbol(variable) expected_expr = safe_parse_expr(expected) # Parse the point (handle infinity) diff --git a/tests/security/test_safe_parser.py b/tests/security/test_safe_parser.py index e7318ada..75d97950 100644 --- a/tests/security/test_safe_parser.py +++ b/tests/security/test_safe_parser.py @@ -1,172 +1,207 @@ """ -Security tests for CWE-95: parse_expr code injection prevention. - -Verifies that safe_parse_expr blocks code execution payloads while -allowing legitimate mathematical expressions. +Security tests for safe_parse_expr - CWE-95 mitigation. """ import pytest -from qwed_new.core.safe_parser import safe_parse_expr, validate_variable_name - - -class TestSafeParseExprBlocksCodeExecution: - """Verify that crafted expressions cannot execute arbitrary code.""" - - @pytest.mark.parametrize( - "payload", - [ - '__import__("os").system("id")', - '__import__("os").getpid()', - '__import__("subprocess").check_output("id")', - 'exec("import os")', - 'open("/etc/passwd").read()', - 'getattr(int, "__subclasses__")', - '().__class__.__bases__[0].__subclasses__()', - '__builtins__["__import__"]("os")', - 'globals()["__builtins__"]', - 'locals()', - 'compile("1", "<>", "exec")', - 'type("X", (), {})', - 'os.system("id")', - 'sys.exit(1)', - 'breakpoint()', - 'dir()', - 'vars()', - 'setattr(x, "y", 1)', - 'delattr(x, "y")', - 'subprocess.call("id")', - ], - ids=lambda p: p[:40], - ) - def test_dangerous_payload_is_rejected(self, payload: str) -> None: - with pytest.raises(ValueError, match="disallowed constructs"): - safe_parse_expr(payload) - - @pytest.mark.parametrize( - "payload", - [ - 'print("hello")', - 'input("prompt")', - ], - ids=lambda p: p[:40], - ) - def test_io_functions_blocked(self, payload: str) -> None: - with pytest.raises(ValueError, match="disallowed constructs"): +from qwed_new.core.safe_parser import ( + safe_parse_expr, + validate_variable_name, + SafeParserError, + MAX_EXPRESSION_LENGTH, +) + + +class TestDenylist: + + @pytest.mark.parametrize("payload", [ + '__import__("os").system("id")', + '__import__(chr(111)+chr(115)).system(chr(105)+chr(100))', + 'eval("1+1")', + 'exec("print(1)")', + 'os.system("id")', + 'os.popen("id").read()', + 'subprocess.call(["id"])', + '__builtins__["__import__"]("os")', + 'getattr(__import__("os"), "system")("id")', + 'open("/etc/passwd")', + '"".__class__.__bases__[0].__subclasses__()', + 'sys.modules["os"]', + 'compile("import os", "", "exec")', + 'chr(65)', + 'ord("A")', + 'print("hello")', + 'input("prompt")', + 'breakpoint()', + 'exit(0)', + 'type("X", (), {})', + 'dir()', + 'vars()', + ]) + def test_injection_blocked(self, payload): + with pytest.raises(SafeParserError, match="disallowed construct"): safe_parse_expr(payload) -class TestSafeParseExprAllowsLegitMath: - """Verify that normal math expressions still parse correctly.""" - - @pytest.mark.parametrize( - "expression,expected_str", - [ - ("2+2", "4"), - ("x**2 + 2*x + 1", "x**2 + 2*x + 1"), - ("sin(x)", "sin(x)"), - ("cos(x)**2 + sin(x)**2", "sin(x)**2 + cos(x)**2"), - ("pi", "pi"), - ("sqrt(16)", "4"), - ("3/4", "3/4"), - ("log(x)", "log(x)"), - ("exp(1)", "E"), - ("factorial(5)", "120"), - ("atan2(1, 1)", "pi/4"), - ], - ) - def test_valid_expression_parses( - self, expression: str, expected_str: str - ) -> None: - result = safe_parse_expr(expression) +class TestLegitimateExpressions: + + @pytest.mark.parametrize("expr,expected_str", [ + ("2 + 2", "4"), + ("3 * 4 + 1", "13"), + ("10 / 2", "5"), + ("x**2 + 2*x + 1", "x**2 + 2*x + 1"), + ("(x + 1)**2", "(x + 1)**2"), + ("sin(pi/2)", "1"), + ("cos(0)", "1"), + ("log(1)", "0"), + ("exp(0)", "1"), + ("sqrt(16)", "4"), + ("factorial(5)", "120"), + ("pi", "pi"), + ("E", "E"), + ("Abs(-5)", "5"), + ]) + def test_valid_expression(self, expr, expected_str): + result = safe_parse_expr(expr) 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 +class TestMultiLetterVariables: - def test_mixed_single_and_greek_variables(self) -> None: - result = safe_parse_expr("x + alpha") - assert str(result) == "alpha + x" + @pytest.mark.parametrize("var_name", [ + "alpha", "beta", "gamma", "delta", "epsilon", + "theta", "phi", "psi", "omega", "sigma", + "tau", "mu", "nu", "xi", "eta", "zeta", + "kappa", "rho", "chi", + ]) + def test_greek_variable(self, var_name): + result = safe_parse_expr(f"{var_name}**2 + 1") + assert var_name in str(result) + def test_multi_variable_expression(self): + result = safe_parse_expr("alpha * beta + gamma") + result_str = str(result) + assert "alpha" in result_str + assert "beta" in result_str + assert "gamma" in result_str -class TestSafeParseExprInputValidation: - """Verify input validation guards.""" - def test_rejects_empty_string(self) -> None: - with pytest.raises(ValueError, match="must not be empty"): +class TestInputValidation: + + def test_non_string_input(self): + with pytest.raises(SafeParserError, match="must be a string"): + safe_parse_expr(42) + + def test_none_input(self): + with pytest.raises(SafeParserError, match="must be a string"): + safe_parse_expr(None) + + def test_empty_string(self): + with pytest.raises(SafeParserError, match="empty"): safe_parse_expr("") - def test_rejects_non_string(self) -> None: - with pytest.raises(ValueError, match="must be a string"): - safe_parse_expr(123) # type: ignore[arg-type] + def test_whitespace_only(self): + with pytest.raises(SafeParserError, match="empty"): + safe_parse_expr(" ") + + def test_too_long(self): + expr = "x + " * (MAX_EXPRESSION_LENGTH // 4 + 1) + with pytest.raises(SafeParserError, match="maximum length"): + safe_parse_expr(expr) + + +class TestVariableNameValidation: + + @pytest.mark.parametrize("name", ["x", "y", "theta", "alpha", "x1"]) + def test_valid_variable(self, name): + assert validate_variable_name(name) == name + + def test_non_string(self): + with pytest.raises(SafeParserError, match="must be a string"): + validate_variable_name(42) + + def test_empty(self): + with pytest.raises(SafeParserError, match="empty"): + validate_variable_name("") + + def test_too_long(self): + with pytest.raises(SafeParserError, match="too long"): + validate_variable_name("a" * 51) + + def test_starts_with_number(self): + with pytest.raises(SafeParserError, match="Invalid variable name"): + validate_variable_name("1x") + + def test_special_characters(self): + with pytest.raises(SafeParserError, match="Invalid variable name"): + validate_variable_name("x;drop") - def test_rejects_oversized_input(self) -> None: - with pytest.raises(ValueError, match="too long"): - safe_parse_expr("x + " * 2000) + def test_denylist_blocked(self): + with pytest.raises(SafeParserError): + validate_variable_name("__import__") + def test_os_blocked(self): + with pytest.raises(SafeParserError, match="disallowed"): + validate_variable_name("os") -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.""" +class TestNamespaceIsolation: + + def test_global_dict_not_polluted(self): + safe_parse_expr("x + 1") + safe_parse_expr("y + 2") + result = safe_parse_expr("x + y") + assert "x" in str(result) + assert "y" in str(result) + + def test_global_dict_not_shared_between_calls(self): + from qwed_new.core.safe_parser import _SAFE_GLOBAL_DICT_TEMPLATE + before = dict(_SAFE_GLOBAL_DICT_TEMPLATE) 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] + safe_parse_expr("y + 2") + assert _SAFE_GLOBAL_DICT_TEMPLATE == before + + +class TestGetSafeSymbol: + + def test_consistency_with_n(self): + from qwed_new.core.safe_parser import get_safe_symbol + n1 = get_safe_symbol("n") + n2 = get_safe_symbol("n") + assert n1 == n2 + + def test_plain_symbol(self): + from qwed_new.core.safe_parser import get_safe_symbol + s = get_safe_symbol("myvar") + assert str(s) == "myvar" + + def test_invalid_variable(self): + from qwed_new.core.safe_parser import get_safe_symbol + with pytest.raises(SafeParserError): + get_safe_symbol("os.system") + + +class TestASTDepthLimit: + + def test_deeply_nested_rejected(self): + deep_expr = "x" + for _ in range(40): + deep_expr = f"sin({deep_expr})" + with pytest.raises((SafeParserError, ValueError)): + safe_parse_expr(deep_expr) + + def test_normal_expression_accepted(self): + result = safe_parse_expr("sin(cos(x + 1) * 2)") + assert result is not None + + +class TestExtraSymbols: + + def test_extra_symbol(self): + from sympy import Symbol + custom = {"myvar": Symbol("myvar")} + result = safe_parse_expr("myvar + 1", extra_symbols=custom) + assert "myvar" in str(result) + + def test_extra_symbol_rejects_non_sympy(self): + result = safe_parse_expr("x + 1", extra_symbols={"bad": lambda: None}) + assert str(result) == "x + 1" From 356577dc62925fb56ed38e11b9d294810b4ea663 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 15:19:28 +0530 Subject: [PATCH 4/8] fix: resolve CodeQL, Sentry, and CodeRabbit review feedback - Remove unused validate_variable_name import from verifier.py (CodeQL) - Fail-closed on SyntaxError in _check_ast_depth (Sentry HIGH) - Raise SafeParserError for invalid extra_symbols (CodeRabbit) - Use SafeParserError with sanitized message (CodeRabbit) - Tighten tests to only expect SafeParserError --- src/qwed_new/core/safe_parser.py | 16 +++++++++++----- src/qwed_new/core/verifier.py | 2 +- tests/security/test_safe_parser.py | 6 +++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index 15555fd0..693ef65d 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -58,7 +58,9 @@ def _check_ast_depth(expression: str) -> None: try: tree = ast.parse(expression, mode="eval") except SyntaxError: - return + raise SafeParserError( + "Expression uses syntax that cannot be validated for safety" + ) depth = _ast_node_depth(tree) if depth > _AST_MAX_DEPTH: raise SafeParserError( @@ -126,8 +128,12 @@ def _build_safe_local_dict( } if extra_symbols: for key, value in extra_symbols.items(): - if isinstance(value, (Symbol, sympy.Basic)): - safe[key] = value + if not isinstance(value, (Symbol, sympy.Basic)): + raise SafeParserError( + f"extra_symbols[{key!r}] must be a SymPy Symbol or Basic, " + f"got {type(value).__name__}" + ) + safe[key] = value return safe @@ -175,8 +181,8 @@ def safe_parse_expr( return result except SafeParserError: raise - except Exception as exc: - raise ValueError(f"Failed to parse expression: {exc}") from exc + except Exception: + raise SafeParserError("Failed to parse expression") from None def validate_variable_name(variable: str) -> str: diff --git a/src/qwed_new/core/verifier.py b/src/qwed_new/core/verifier.py index ee4ec757..8d5a0bf0 100644 --- a/src/qwed_new/core/verifier.py +++ b/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, validate_variable_name, get_safe_symbol +from qwed_new.core.safe_parser import safe_parse_expr, get_safe_symbol from sympy.parsing.sympy_parser import standard_transformations, implicit_multiplication_application from typing import Any, Dict, List, Optional from decimal import Decimal, ROUND_HALF_UP diff --git a/tests/security/test_safe_parser.py b/tests/security/test_safe_parser.py index 75d97950..d6a2d344 100644 --- a/tests/security/test_safe_parser.py +++ b/tests/security/test_safe_parser.py @@ -186,7 +186,7 @@ def test_deeply_nested_rejected(self): deep_expr = "x" for _ in range(40): deep_expr = f"sin({deep_expr})" - with pytest.raises((SafeParserError, ValueError)): + with pytest.raises(SafeParserError): safe_parse_expr(deep_expr) def test_normal_expression_accepted(self): @@ -203,5 +203,5 @@ def test_extra_symbol(self): assert "myvar" in str(result) def test_extra_symbol_rejects_non_sympy(self): - result = safe_parse_expr("x + 1", extra_symbols={"bad": lambda: None}) - assert str(result) == "x + 1" + with pytest.raises(SafeParserError, match="must be a SymPy"): + safe_parse_expr("x + 1", extra_symbols={"bad": lambda: None}) From d72bc45d83e39cfa8df55cdc6215a73de341ebf3 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 15:32:39 +0530 Subject: [PATCH 5/8] fix: post-parse sympy depth check, revert implicit-mult regression - Skip pre-parse AST depth for implicit-mult syntax (fixes 2x regression) - Add post-parse SymPy expression tree depth check (catches all syntax) --- src/qwed_new/core/safe_parser.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index 693ef65d..772b2212 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -54,13 +54,15 @@ def _check_ast_depth(expression: str) -> None: - """Reject expressions whose AST exceeds max depth (DoS defence).""" + """Reject Python-parseable expressions exceeding max AST depth (DoS defence). + + Expressions using implicit multiplication (e.g. 2x, sin x) fail ast.parse + and skip this check — they are caught by the post-parse sympy depth check. + """ try: tree = ast.parse(expression, mode="eval") except SyntaxError: - raise SafeParserError( - "Expression uses syntax that cannot be validated for safety" - ) + return depth = _ast_node_depth(tree) if depth > _AST_MAX_DEPTH: raise SafeParserError( @@ -77,13 +79,31 @@ def _ast_node_depth(node: ast.AST, current: int = 0) -> int: return max_depth +_SYMPY_MAX_DEPTH = 40 + + +def _sympy_tree_depth(expr: Any, current: int = 0) -> int: + """Compute nesting depth of a SymPy expression tree.""" + max_depth = current + for arg in getattr(expr, "args", ()): + child_depth = _sympy_tree_depth(arg, current + 1) + if child_depth > max_depth: + max_depth = child_depth + return max_depth + + def _validate_sympy_result(result: Any) -> None: - """Ensure parse_expr returned an expected SymPy type.""" + """Ensure parse_expr returned a valid SymPy expression within depth limits.""" import sympy if not isinstance(result, sympy.Basic): raise SafeParserError( f"Parsed result is not a valid SymPy expression, got {type(result).__name__}" ) + depth = _sympy_tree_depth(result) + if depth > _SYMPY_MAX_DEPTH: + raise SafeParserError( + f"Expression tree depth {depth} exceeds limit of {_SYMPY_MAX_DEPTH}" + ) def _build_safe_local_dict( From 430686b58586e2652e26ccbfc6bea365360902f1 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 15:41:11 +0530 Subject: [PATCH 6/8] fix: treat lowercase e as variable, not Euler constant - e is commonly used as a free symbol; only uppercase E maps to Euler's number - Removes silent semantic change from original parse_expr behavior --- src/qwed_new/core/safe_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index 772b2212..646f18b3 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -129,7 +129,7 @@ def _build_safe_local_dict( "tau": Symbol("tau"), "phi": Symbol("phi"), "chi": Symbol("chi"), "psi": Symbol("psi"), "omega": Symbol("omega"), - "pi": pi, "e": E, "E": E, "I": I, "oo": oo, + "pi": pi, "E": E, "I": I, "oo": oo, "sin": sympy.sin, "cos": sympy.cos, "tan": sympy.tan, "cot": sympy.cot, "sec": sympy.sec, "csc": sympy.csc, "asin": sympy.asin, "acos": sympy.acos, "atan": sympy.atan, From 2b0f24d08fd7ffc7266224f167eb59a1dfac141d Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 16:31:11 +0530 Subject: [PATCH 7/8] fix: preserve error messages, reject relational expressions - Preserve original parse error messages for domain error detection - Check sympy.Expr instead of sympy.Basic to reject relationals (x < y) --- src/qwed_new/core/safe_parser.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index 646f18b3..c78e2d5f 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -95,9 +95,9 @@ def _sympy_tree_depth(expr: Any, current: int = 0) -> int: def _validate_sympy_result(result: Any) -> None: """Ensure parse_expr returned a valid SymPy expression within depth limits.""" import sympy - if not isinstance(result, sympy.Basic): + if not isinstance(result, sympy.Expr): raise SafeParserError( - f"Parsed result is not a valid SymPy expression, got {type(result).__name__}" + f"Parsed result is not a supported arithmetic expression, got {type(result).__name__}" ) depth = _sympy_tree_depth(result) if depth > _SYMPY_MAX_DEPTH: @@ -201,8 +201,8 @@ def safe_parse_expr( return result except SafeParserError: raise - except Exception: - raise SafeParserError("Failed to parse expression") from None + except Exception as exc: + raise SafeParserError(f"Failed to parse expression: {exc}") from exc def validate_variable_name(variable: str) -> str: From 70f0978502be50819aa2e876f529b2a3a0d4cbd8 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 14 Jun 2026 16:42:13 +0530 Subject: [PATCH 8/8] fix: validate extra_symbols keys against denylist - Keys must be strings - Keys checked against _DENYLIST_PATTERN before being used --- src/qwed_new/core/safe_parser.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qwed_new/core/safe_parser.py b/src/qwed_new/core/safe_parser.py index c78e2d5f..2bd1e7fe 100644 --- a/src/qwed_new/core/safe_parser.py +++ b/src/qwed_new/core/safe_parser.py @@ -148,6 +148,14 @@ def _build_safe_local_dict( } if extra_symbols: for key, value in extra_symbols.items(): + if not isinstance(key, str): + raise SafeParserError( + f"extra_symbols keys must be strings, got {type(key).__name__}" + ) + if _DENYLIST_PATTERN.search(key): + raise SafeParserError( + f"extra_symbols key {key!r} contains disallowed construct" + ) if not isinstance(value, (Symbol, sympy.Basic)): raise SafeParserError( f"extra_symbols[{key!r}] must be a SymPy Symbol or Basic, "