Skip to content

Commit 6372667

Browse files
committed
fix(review): address Copilot review feedback
- Make SAFE_FUNCTIONS immutable with MappingProxyType - Document InvalidExpression as the raised base exception - Use stable assertions in security tests (result != "" and parameter name presence) instead of translated error substrings Signed-off-by: Yash Goel <yashhzd@gmail.com> Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
1 parent 6044d36 commit 6372667

File tree

2 files changed

+18
-14
lines changed

2 files changed

+18
-14
lines changed

ardupilot_methodic_configurator/safe_expression_evaluator.py

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,20 @@
1212
"""
1313

1414
from math import log
15+
from types import MappingProxyType
1516

16-
from simpleeval import simple_eval
17+
from simpleeval import InvalidExpression, simple_eval
1718

18-
SAFE_FUNCTIONS = {
19-
"max": max,
20-
"min": min,
21-
"round": round,
22-
"abs": abs,
23-
"len": len,
24-
"log": log,
25-
}
19+
SAFE_FUNCTIONS = MappingProxyType(
20+
{
21+
"max": max,
22+
"min": min,
23+
"round": round,
24+
"abs": abs,
25+
"len": len,
26+
"log": log,
27+
}
28+
)
2629

2730

2831
def safe_evaluate(expression: str, variables: dict) -> object:
@@ -42,9 +45,8 @@ def safe_evaluate(expression: str, variables: dict) -> object:
4245
The result of evaluating the expression.
4346
4447
Raises:
45-
simpleeval.FeatureNotAvailable: If the expression uses a disallowed feature.
46-
simpleeval.FunctionNotDefined: If the expression calls an unknown function.
47-
simpleeval.NameNotDefined: If the expression references an undefined variable.
48+
InvalidExpression: If the expression uses a disallowed feature,
49+
calls an unknown function, or references an undefined variable.
4850
4951
"""
5052
return simple_eval(expression, names=variables, functions=SAFE_FUNCTIONS)

tests/test_backend_filesystem_configuration_steps.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,8 @@ def test_malicious_expression_import_blocked() -> None:
475475

476476
result = config_steps.compute_parameters("test_file", file_info, "forced", variables)
477477

478-
assert "could not be computed" in result
478+
assert result != ""
479+
assert "PARAM1" in result
479480
assert "test_file" not in config_steps.forced_parameters
480481

481482

@@ -491,7 +492,8 @@ def test_malicious_expression_builtins_blocked() -> None:
491492

492493
result = config_steps.compute_parameters("test_file", file_info, "forced", variables)
493494

494-
assert "could not be computed" in result
495+
assert result != ""
496+
assert "PARAM1" in result
495497
assert "test_file" not in config_steps.forced_parameters
496498

497499

0 commit comments

Comments
 (0)