From 49ed86abad21a7ce1f6101112a6f9a355edbd657 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 13:09:36 +0500 Subject: [PATCH] fix(workflows): reject a multi-argument filter call in expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_apply_filter` parses a call with `re.fullmatch(r"(\w+)\((.+)\)")` and hands the ENTIRE captured argument text to `_evaluate_simple_expression` as one expression. Every filter in this subset takes exactly one argument, so a two-argument call is not a valid expression and evaluates to None: {{ inputs.missing | default(1) }} -> 1 {{ inputs.missing | default(1, 2) }} -> None <-- silently wrong {{ inputs.name | join(",", "extra") }} -> ValueError: join: expected a string separator, got NoneType So `default` silently returns None instead of its default, and `join` raises a message blaming the separator rather than the extra argument. Fall through to the existing "unsupported form" error, which names the filter and lists the accepted forms: filter 'default' used in an unsupported form (got '| default(1, 2)'): ... The check is quote-aware, because a single argument may legitimately contain a comma — `join(", ")` and `default("a, b")` must keep working, so a plain split would reject valid expressions. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/expressions.py | 29 +++++++++++++++++ tests/test_workflows.py | 40 ++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index a38cd6cb68..2fb637bab3 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -366,6 +366,25 @@ def _find_top_level(text: str, token: str) -> int: return -1 +def _has_top_level_comma(text: str) -> bool: + """True when *text* has a comma outside any quoted span. + + Every filter in this subset takes exactly one argument, but that argument may + itself contain a comma -- ``join(", ")`` and ``default("a, b")`` are both + valid -- so the check has to be quote-aware rather than a plain ``split``. + """ + quote = "" + for ch in text: + if quote: + if ch == quote: + quote = "" + elif ch in ("'", '"'): + quote = ch + elif ch == ",": + return True + return False + + def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> Any: """Apply a single pipe filter segment to *value*. @@ -400,6 +419,16 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An # branch above. The greedy ``.+`` still handles literal ``)`` and ``|`` # inside quoted args. filter_match = re.fullmatch(r"(\w+)\((.+)\)", filter_expr) + # A multi-argument call is not a supported form: every filter here takes + # exactly one argument, and the whole captured argument text was handed to + # ``_evaluate_simple_expression`` as ONE expression. "1, 2" is not a valid + # expression, so it evaluated to None -- making ``default(1, 2)`` return + # None (silently wrong) and ``join(",", "extra")`` raise a message blaming + # the separator rather than the extra argument. Fall through to the + # unsupported-form error below instead, which names the filter and lists + # the accepted forms. The check is quote-aware so ``join(", ")`` still works. + if filter_match and _has_top_level_comma(filter_match.group(2)): + filter_match = None if filter_match: fname = filter_match.group(1) farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 57536081ea..5e59107bd1 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -708,6 +708,46 @@ def test_filter_call_with_trailing_tokens_fails_loudly(self): StepContext(inputs={"tags": ["a", "b"]}), ) + def test_multi_argument_filter_call_fails_loudly(self): + """A second argument must be reported, not silently mis-evaluated. + + The whole captured argument text was handed to + `_evaluate_simple_expression` as ONE expression. `"1, 2"` is not a valid + expression, so it evaluated to None — making `default(1, 2)` return None + (silently wrong) and `join(",", "extra")` raise a message blaming the + separator rather than the extra argument. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + with pytest.raises(ValueError, match="unsupported form"): + evaluate_expression( + "{{ inputs.missing | default(1, 2) }}", StepContext(inputs={}) + ) + with pytest.raises(ValueError, match="unsupported form"): + evaluate_expression( + '{{ inputs.tags | join(",", "extra") }}', + StepContext(inputs={"tags": ["a", "b"]}), + ) + + def test_single_argument_containing_a_comma_still_works(self): + """The multi-argument check must be quote-aware. + + `join(", ")` and `default("a, b")` are single arguments that happen to + contain a comma, so a plain split would reject valid expressions. + """ + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"tags": ["a", "b"]}) + assert evaluate_expression('{{ inputs.tags | join(", ") }}', ctx) == "a, b" + assert evaluate_expression('{{ inputs.tags | join(",") }}', ctx) == "a,b" + assert ( + evaluate_expression('{{ inputs.missing | default("a, b") }}', ctx) + == "a, b" + ) + def test_chained_filters_apply_left_to_right(self): # Filters chain: each filter's result feeds the next. `map` yields a # list and `join` is the only filter that renders a list to a string,