diff --git a/CHANGELOG.md b/CHANGELOG.md index 82ee3a0..524c9bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -239,6 +239,36 @@ Shipped `recipe_library` batch note: 16 recipes, verified against flexicon `project_not_found` now always carries `available_projects` and `total_count`, even when there are no fuzzy suggestions, and its hint says the name must be a plain project name. +- **Safety:** an unguarded write through a loop over a list of LCM + collections no longer passes the `unprotected_writes` gate + ([#350](https://github.com/MattGyverLee/FlexToolsMCP/issues/350)). + `for coll in [e.SensesOS, f.SensesOS]: coll.Add(s)` was certified + read-only because the loop variable was taken for a local Python list. A + name now counts as a local container only when every binding of it in the + script builds one; a loop target, unpacking or any other rebinding keeps + its `.Add` gated. The skip is also tracked per call instead of per line, so + `tmp.Add(x); entry.SensesOS.Add(s)` on one line no longer hides the real + write. Function and lambda parameters, import aliases, `except ... as` + and `match` captures now count as bindings too, so + `def helper(senses, s): senses.Add(s)` stays gated even when `Main` has + its own `senses = []`. Plain local lists and sets (`results = []`, + `seen = set()`) are still not flagged. +- `if modifyAllowed and :` now counts as a write guard + ([#352](https://github.com/MattGyverLee/FlexToolsMCP/issues/352)), as + does the early-return form `if not modifyAllowed or : return`. An + `or` with a non-guard operand still does not. A comparison now counts + only against a literal True/False in the enabling direction: before, + `if modifyAllowed == False:` was wrongly accepted as protecting the write + in its body. When the guard's test itself makes a call + (`if x.SensesOS.Add(s) and modifyAllowed:`), only the lines after the + test are protected, so that call is still reported. +- The write-gate patterns no longer report `nonsense.Form = ...`, + `compose.Comment = ...` or `position.Note = ...` as writes, and a + resolved facade such as `fx` no longer matches inside `prefx` + ([#351](https://github.com/MattGyverLee/FlexToolsMCP/issues/351)). + The gate still fails closed elsewhere on purpose: compound receivers + (`subsense`, `subentry`, `lexentry`, `newEntry`) and any name ending in + `project` (`self._project`, `srcProject`, `myproject`) stay gated. ### Other diff --git a/src/flextoolsmcp/server/validators.py b/src/flextoolsmcp/server/validators.py index 29495c0..d66f743 100644 --- a/src/flextoolsmcp/server/validators.py +++ b/src/flextoolsmcp/server/validators.py @@ -53,6 +53,23 @@ "PronunciationsOS", "LexEntryRefsOS", "ComponentLexemesRS" ) +# Issue #351: receiver-name prefix guard for patterns such as `entry` or +# `sense`. No structural boundary separates a compound receiver from an +# English word -- `subsense`, `subentry` and `lexentry` are real FLEx +# receivers, `nonsense` and `compose` are not -- so the keyword may still sit +# anywhere inside the identifier (the fail-closed direction). Only these +# exact whole words, which cannot be an LCM object's name in practice, are +# skipped. The identifier must start at `\b` so the excluded word is matched +# whole, then `\w*?` re-admits any prefix (`new_`, `sub`, `lex`, `self._`). +_RECEIVER_NOT_ENGLISH_WORDS = ( + "nonsense", "compose", "composes", "position", "positions", "purpose", + "purposes", "suppose", "propose", "expose", "dispose", "impose", + "oppose", "transpose", "deposit", +) +_RECEIVER_PREFIX_BOUNDARY = ( + r'\b(?!(?:' + '|'.join(_RECEIVER_NOT_ENGLISH_WORDS) + r')\b)\w*?' +) + # Compiled regex patterns for efficiency _PATTERN_COMMENT = re.compile(r'#.*$', re.MULTILINE) _PATTERN_CREATE = re.compile(r'\.Create\s*\(', re.IGNORECASE) @@ -60,7 +77,8 @@ r'\.(' + '|'.join(LCM_COLLECTION_NAMES) + r')\s*\.\s*Add\s*\(', re.IGNORECASE ) _PATTERN_CREATE_GENERIC = re.compile( - r'(entry|sense|wordform|analysis|bundle|gloss)\w*\.\w+\.\s*Add\s*\(', re.IGNORECASE + _RECEIVER_PREFIX_BOUNDARY + + r'(entry|sense|wordform|analysis|bundle|gloss)\w*\.\w+\.\s*Add\s*\(', re.IGNORECASE ) _PATTERN_CREATE_PROJECT = re.compile(r'project\.\w+\.Create\w*\s*\(', re.IGNORECASE) _PATTERN_INSERT_COLLECTION = re.compile( @@ -77,7 +95,8 @@ ) _PATTERN_COPY_ALTERNATIVES = re.compile(r'\.CopyAlternatives\s*\(', re.IGNORECASE) _PATTERN_PROPERTY_ASSIGNMENT = re.compile( - r'(entry|sense|wordform|analysis|bundle|morph|gloss|allomorph|pos)\w*\s*\.\s*' + _RECEIVER_PREFIX_BOUNDARY + + r'(entry|sense|wordform|analysis|bundle|morph|gloss|allomorph|pos)\w*\s*\.\s*' r'(LexemeFormOA|MorphoSyntaxAnalysisRA|SenseRA|MsaRA|MorphRA|CategoryRA|' r'InflectionClassRA|EntryRefsOS|ComponentLexemesRS|PrimaryLexemesRS|' r'MorphTypeRA|Gloss|Definition|Form|LiteralMeaning|SummaryDefinition|' @@ -135,7 +154,8 @@ (re.compile(r'\.Set(?:Occurrences|Form|Gloss|Definition|Category|Analysis)\s*\('), 'Set*', 'Update'), # Raw LCM property assignments (entry.LexemeFormOA = ..., sense.Gloss = ..., etc.) (re.compile( - r'(?:entry|sense|wordform|analysis|bundle|morph|gloss|allomorph|pos)\w*\s*\.\s*' + _RECEIVER_PREFIX_BOUNDARY + + r'(?:entry|sense|wordform|analysis|bundle|morph|gloss|allomorph|pos)\w*\s*\.\s*' r'(?:LexemeFormOA|MorphoSyntaxAnalysisRA|SenseRA|MsaRA|MorphRA|CategoryRA|' r'InflectionClassRA|EntryRefsOS|ComponentLexemesRS|PrimaryLexemesRS|' r'MorphTypeRA|Gloss|Definition|Form|LiteralMeaning|SummaryDefinition|' @@ -157,10 +177,10 @@ # index lookup cannot see `fx.LexEntry.SetLexemeForm(...)` at all. These # regexes are then the only thing still holding the write gate. _FACADE_ACCESSOR_MUTABLE_TEMPLATES = [ - (r'{receiver}\s*\.\s*\w+\s*\.\s*Create\w*\s*\(', 'Create', 'Create'), - (r'{receiver}\s*\.\s*\w+\s*\.\s*Delete\w*\s*\(', 'Delete', 'Delete'), + (r'{boundary}{receiver}\s*\.\s*\w+\s*\.\s*Create\w*\s*\(', 'Create', 'Create'), + (r'{boundary}{receiver}\s*\.\s*\w+\s*\.\s*Delete\w*\s*\(', 'Delete', 'Delete'), ( - r'{receiver}\s*\.\s*\w+\s*\.\s*(?:Set|Update|Modify|Change|Edit|Replace)\w*\s*\(', + r'{boundary}{receiver}\s*\.\s*\w+\s*\.\s*(?:Set|Update|Modify|Change|Edit|Replace)\w*\s*\(', 'Set/Update', 'Update', ), @@ -176,9 +196,16 @@ def _facade_accessor_mutable_patterns(receiver_name: str) -> List[tuple]: per call costs nothing measurable. """ escaped = re.escape(receiver_name) + # Issue #351: a resolved facade is one exact identifier, so `fx` must not + # match inside `prefx`. The injected `project` keeps no left boundary on + # purpose: `self._project`, `srcProject` and `old_project` are all + # plausible FLExProject handles, and the gate fails closed. + boundary = '' if receiver_name == 'project' else r'\b' return [ ( - re.compile(template.format(receiver=escaped), re.IGNORECASE), + re.compile( + template.format(boundary=boundary, receiver=escaped), re.IGNORECASE + ), f'{receiver_name}.*.{label}', category, ) @@ -478,7 +505,10 @@ def detect_module_structure(code: str) -> dict: _DOCS_DICT_RE = re.compile(r'^\s*docs\s*=\s*\{', re.MULTILINE) -_MODIFY_GUARD_RE = re.compile(r'\bif\s+(?:not\s+)?modifyAllowed\b') +# Any `if` test that mentions modifyAllowed, so compound guards +# (`if existing is None and modifyAllowed:`, issue #352) also mark the +# scaffold as FTM_ModifiesDB. +_MODIFY_GUARD_RE = re.compile(r'\bif\b[^\n:]*\bmodifyAllowed\b') _SCAFFOLD_IMPORT = "from flextoolslib import *" _SCAFFOLD_BINDING = "FlexToolsModule = FlexToolsModuleClass(Main, docs)" @@ -6310,46 +6340,120 @@ def _is_local_container_constructor(value: ast.AST) -> bool: def _collect_local_container_names(tree: ast.AST) -> Set[str]: - """Names bound to locally constructed containers (issue #126). - - Reassigning a name to a non-local value removes it, so a variable that - later holds an LCM collection is not treated as local. + """Names bound ONLY to locally constructed containers (issues #126, #350). + + A name qualifies when every place it is stored -- anywhere in the tree -- + binds a local container constructor (or another qualifying name). Any + other store disqualifies it, in particular: + + - a ``for`` / comprehension target: the loop variable is an ELEMENT of + the iterable, not the iterable, so ``for coll in [e.SensesOS]:`` + binds ``coll`` to a real LCM collection even though the iterable is a + list literal (issue #350); + - a tuple-unpacking target, ``with ... as``, a non-container right-hand + side, or a reassignment anywhere else in the script; + - a binding that is not an ``ast.Name`` store at all: a function or + lambda parameter (``def helper(senses, s): senses.Add(s)`` called + with ``e.SensesOS``), an import alias, an ``except ... as`` name, a + ``match`` capture, or a ``def`` / ``class`` name. + + This is flow-insensitive on purpose: one non-local binding anywhere keeps + ``.Add`` on that name gated (the fail-closed direction), where "last + binding wins" let ``x = e.SensesOS; x.Add(s); x = []`` certify read-only. """ - assigns, _, bindings = _collect_assign_call_nodes(tree) - binding_nodes = list(assigns) + list(bindings) - binding_nodes.sort(key=lambda n: getattr(n, "lineno", 0)) + safe_values: Dict[int, ast.AST] = {} + store_nodes: Dict[str, List[ast.Name]] = {} + for node in ast.walk(tree): + if isinstance(node, (ast.Assign, ast.AnnAssign, ast.NamedExpr)): + for target, rhs in _iter_assign_pairs(node): + if isinstance(target, ast.Name): + safe_values[id(target)] = rhs + elif isinstance(node, ast.AugAssign) and isinstance(node.target, ast.Name): + safe_values[id(node.target)] = node.value + elif isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): + store_nodes.setdefault(node.id, []).append(node) + other_bindings = _non_name_store_bindings(tree) + local: Set[str] = set() - for node in binding_nodes: - for target, rhs in _iter_assign_pairs(node): - if not isinstance(target, ast.Name): + changed = True + while changed: + changed = False + for name, stores in store_nodes.items(): + if name in local or name in other_bindings: continue - name = target.id - if _is_local_container_constructor(rhs): - local.add(name) - elif isinstance(rhs, ast.Name) and rhs.id in local: + if all( + id(store) in safe_values + and _is_local_container_value(safe_values[id(store)], local) + for store in stores + ): local.add(name) - else: - local.discard(name) + changed = True return local -def _lines_with_local_collection_mutations(tree: ast.AST) -> Set[int]: - """Line numbers where a collection-mutation call targets a local container.""" +def _non_name_store_bindings(tree: ast.AST) -> Set[str]: + """Names bound by anything other than an ``ast.Name`` store (issue #350). + + Parameters, import aliases, ``except ... as``, ``match`` captures and + ``def`` / ``class`` names can each hold a real LCM collection, so any of + them disqualifies a name from `_collect_local_container_names`. + """ + names: Set[str] = set() + for node in ast.walk(tree): + if isinstance(node, ast.arg): + names.add(node.arg) + elif isinstance(node, ast.alias): + names.add(node.asname or node.name.split(".")[0]) + elif isinstance(node, ast.ExceptHandler) and node.name: + names.add(node.name) + elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + names.add(node.name) + elif isinstance(node, (ast.MatchAs, ast.MatchStar)) and node.name: + names.add(node.name) + elif isinstance(node, ast.MatchMapping) and node.rest: + names.add(node.rest) + return names + + +def _is_local_container_value(rhs: ast.AST, local: Set[str]) -> bool: + if _is_local_container_constructor(rhs): + return True + return isinstance(rhs, ast.Name) and rhs.id in local + + +def _local_collection_mutation_sites(code: str, tree: ast.AST) -> Set[Tuple[int, int]]: + """``(line, column)`` of each collection-mutation call on a local container. + + Keyed by the call node rather than by line (issue #350): the column is the + character offset just past the method name, which is where a matching + ``.Add(`` pattern hit's ``start() + 1 + len(method)`` lands, so + ``tmp.Add(x); entry.SensesOS.Add(s)`` suppresses only the first call. + """ local = _collect_local_container_names(tree) if not local: return set() - skip: Set[int] = set() + lines = code.split("\n") + sites: Set[Tuple[int, int]] = set() for node in ast.walk(tree): if not isinstance(node, ast.Call): continue - if not isinstance(node.func, ast.Attribute): + func = node.func + if not isinstance(func, ast.Attribute): + continue + if func.attr not in _COLLECTION_MUTATION_METHODS: continue - if node.func.attr not in _COLLECTION_MUTATION_METHODS: + recv = func.value + if not (isinstance(recv, ast.Name) and recv.id in local): continue - recv = node.func.value - if isinstance(recv, ast.Name) and recv.id in local: - skip.add(node.lineno) - return skip + line_no = func.end_lineno or node.lineno + end_col = func.end_col_offset + if end_col is None or line_no - 1 >= len(lines): + continue + # ast columns are UTF-8 byte offsets; regex matches are str offsets. + line_bytes = lines[line_no - 1].encode("utf-8") + char_col = len(line_bytes[:end_col].decode("utf-8", errors="ignore")) + sites.add((line_no, char_col)) + return sites def find_liblcm_mutations( @@ -6377,9 +6481,9 @@ def find_liblcm_mutations( """ mutations = [] - skip_collection_lines: Set[int] = set() + skip_collection_sites: Set[Tuple[int, int]] = set() try: - skip_collection_lines = _lines_with_local_collection_mutations(ast.parse(code)) + skip_collection_sites = _local_collection_mutation_sites(code, ast.parse(code)) except SyntaxError: pass @@ -6394,12 +6498,16 @@ def find_liblcm_mutations( line_content = line for pattern, method_name, category in patterns: - if ( - method_name in _COLLECTION_MUTATION_METHODS - and line_num in skip_collection_lines - ): - continue - if re.search(pattern, line_content): + if method_name in _COLLECTION_MUTATION_METHODS and skip_collection_sites: + # Node-keyed (issue #350): report the line when ANY hit on it + # is not a local-container call. + hit = any( + (line_num, m.start() + 1 + len(method_name)) not in skip_collection_sites + for m in re.finditer(pattern, line_content) + ) + else: + hit = re.search(pattern, line_content) is not None + if hit: raw_context = ( original_lines[line_num - 1] if line_num - 1 < len(original_lines) @@ -6438,6 +6546,43 @@ def _is_project_receiver(node: ast.AST) -> bool: return False +def _is_write_flag(node: ast.AST) -> bool: + """Bare ``modifyAllowed`` or the project's own ``writeEnabled`` flag.""" + if isinstance(node, ast.Name): + return node.id == 'modifyAllowed' + if isinstance(node, ast.Attribute): + return node.attr == 'writeEnabled' and _is_project_receiver(node.value) + return False + + +def _write_flag_compare_polarity(node: ast.Compare) -> Optional[bool]: + """Polarity of a `` ==/is/!=/is not `` comparison. + + Returns True when the comparison holding means writes are enabled + (``modifyAllowed == True``, ``modifyAllowed != False``), False when it + means they are disabled (``modifyAllowed is False``), and None for + anything else -- chained comparisons, ordering operators, or a non-literal + other side (``modifyAllowed == other``), none of which is a guard. + """ + if len(node.ops) != 1 or len(node.comparators) != 1: + return None + left, right = node.left, node.comparators[0] + if _is_write_flag(left) and isinstance(right, ast.Constant): + const = right.value + elif _is_write_flag(right) and isinstance(left, ast.Constant): + const = left.value + else: + return None + if not isinstance(const, (bool, int)) or const not in (0, 1): + return None + op = node.ops[0] + if isinstance(op, (ast.Eq, ast.Is)): + return bool(const) + if isinstance(op, (ast.NotEq, ast.IsNot)): + return not const + return None + + def find_protected_ranges(code: str, tree: ast.AST | None = None) -> List[tuple]: """Find line ranges protected by modifyAllowed or modifyEnabled/writeEnabled guards. @@ -6511,6 +6656,16 @@ def visit_If(self, node): """Find 'if modifyAllowed:' or 'if project.writeEnabled:' blocks.""" if self._is_write_enabled_check(node.test): start_line = node.lineno + if any(isinstance(n, ast.Call) for n in ast.walk(node.test)): + # Issue #352: a compound test can itself call a mutator + # before (or regardless of) the guard operand + # (`if x.Add(s) and modifyAllowed:`, + # `if not (x.Add(s) or not modifyAllowed):`), so the + # range starts after the test's last line. Ranges are + # line-keyed, so a body on that same line + # (`if x.Add(s) and modifyAllowed: y.Add(t)`) is left + # unprotected -- the fail-closed direction. + start_line = (node.test.end_lineno or node.lineno) + 1 # end_lineno includes the if line, body starts after if node.body: end_line = node.body[-1].end_lineno or start_line + 1000 @@ -6535,7 +6690,7 @@ def _is_modify_enabled(self, node): return False def _is_write_enabled_check(self, node): - """Check if condition checks 'modifyAllowed', 'project.writeEnabled', etc. + """True when ``node`` being truthy implies writes are enabled. Issue #121 sibling: `project.writeEnabled` / `self.project.writeEnabled` require the attribute's receiver to actually be the project (see @@ -6543,68 +6698,41 @@ def _is_write_enabled_check(self, node): attribute (e.g. `cfg.writeEnabled`) must not be accepted as a guard. The bare-name `modifyAllowed` form (FLExTools' standard parameter) is unaffected -- it has no receiver to check. - """ - # Pattern: modifyAllowed (name - FLExTools standard parameter) - if isinstance(node, ast.Name): - return node.id == 'modifyAllowed' - - # Pattern: project.writeEnabled / self.project.writeEnabled (attribute) - if isinstance(node, ast.Attribute): - return node.attr == 'writeEnabled' and _is_project_receiver(node.value) - # Pattern: project.writeEnabled == True (compare) + Issue #352: `modifyAllowed and ` implies the flag, so an + `and` guards when ANY operand does; an `or` only when EVERY + operand does. Comparisons count only against a literal True/False + in the enabling direction -- `modifyAllowed == False` used to be + accepted here because ANY Compare mentioning the flag passed. + """ + if _is_write_flag(node): + return True + if isinstance(node, ast.BoolOp): + if isinstance(node.op, ast.And): + return any(self._is_write_enabled_check(v) for v in node.values) + return all(self._is_write_enabled_check(v) for v in node.values) + if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): + return self._is_write_disabled_check(node.operand) if isinstance(node, ast.Compare): - # Check left side - if isinstance(node.left, ast.Attribute): - if node.left.attr == 'writeEnabled' and _is_project_receiver(node.left.value): - return True - if isinstance(node.left, ast.Name): - if node.left.id == 'modifyAllowed': - return True - # Check comparators - for comp in node.comparators: - if isinstance(comp, ast.Attribute): - if comp.attr == 'writeEnabled' and _is_project_receiver(comp.value): - return True - if isinstance(comp, ast.Name): - if comp.id == 'modifyAllowed': - return True + return _write_flag_compare_polarity(node) is True return False def _is_write_disabled_check(self, node): - """Negated write guard: ``if not modifyAllowed:`` / ``== False`` forms.""" + """True when ``node`` being truthy implies writes are DISABLED. + + The early-return idiom (#139): ``if not modifyAllowed: return``, + ``== False`` forms, and (#352) ``not modifyAllowed or `` -- + an `or` is disabling when ANY operand is, an `and` only when + EVERY operand is. + """ if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): return self._is_write_enabled_check(node.operand) - if isinstance(node, ast.Compare) and len(node.ops) == 1: - if not isinstance(node.ops[0], (ast.Eq, ast.Is)): - return False - # Tuple (not set): False == 0 so {False, 0} is B033-duplicate. - false_val = (False, 0) - if ( - isinstance(node.left, ast.Name) - and node.left.id == 'modifyAllowed' - and node.comparators - and isinstance(node.comparators[0], ast.Constant) - and node.comparators[0].value in false_val - ): - return True - if ( - isinstance(node.left, ast.Constant) - and node.left.value in false_val - and node.comparators - and isinstance(node.comparators[0], ast.Name) - and node.comparators[0].id == 'modifyAllowed' - ): - return True - if isinstance(node.left, ast.Attribute): - if ( - node.left.attr == 'writeEnabled' - and _is_project_receiver(node.left.value) - and node.comparators - and isinstance(node.comparators[0], ast.Constant) - and node.comparators[0].value in false_val - ): - return True + if isinstance(node, ast.BoolOp): + if isinstance(node.op, ast.Or): + return any(self._is_write_disabled_check(v) for v in node.values) + return all(self._is_write_disabled_check(v) for v in node.values) + if isinstance(node, ast.Compare): + return _write_flag_compare_polarity(node) is False return False finder = ProtectionFinder() diff --git a/tests/test_issue350_loop_container_write_gate.py b/tests/test_issue350_loop_container_write_gate.py new file mode 100644 index 0000000..2f12875 --- /dev/null +++ b/tests/test_issue350_loop_container_write_gate.py @@ -0,0 +1,214 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +"""Issue #350: a loop over a list of LCM collections must not hide `.Add`. + +`_collect_local_container_names` used to treat a `for` target as a local +container when its iterable was a list literal or a local list. The loop +variable is an ELEMENT of that list -- possibly a real LCM collection such as +`e.SensesOS` -- so an unguarded `coll.Add(s)` was dropped from +`find_liblcm_mutations` and the script certified read-only. + +The suppression was also keyed by LINE, so a local `tmp.Add(x)` sharing a +line with a real `entry.SensesOS.Add(s)` hid the real one. It is now keyed by +the call node itself. +""" + +from flextoolsmcp.server.validators import ( + certify_script_readonly, + find_liblcm_mutations, +) + + +def _adds(code): + return [m for m in find_liblcm_mutations(code) if m["method"] == "Add"] + + +def _assert_flagged(code): + cert = certify_script_readonly(code, api_index=None) + assert cert["is_certified_readonly"] is False, cert + assert any(m["method"] == "Add" for m in cert["unprotected_liblcm_calls"]), cert + + +class TestLoopElementIsNotALocalContainer: + def test_loop_over_list_literal_of_lcm_collections(self): + code = ( + "e = project.LexEntry.Find('a'); f = project.LexEntry.Find('b')\n" + "for coll in [e.SensesOS, f.SensesOS]:\n" + " coll.Add(s)\n" + ) + _assert_flagged(code) + + def test_loop_over_local_list_comprehension(self): + code = ( + "colls = [e.SensesOS for e in project.LexEntry.GetAll()]\n" + "for coll in colls:\n" + " coll.Add(s)\n" + ) + _assert_flagged(code) + + def test_loop_over_tuple_with_unpacking_target(self): + code = ( + "pairs = [(e.SensesOS, s) for e in project.LexEntry.GetAll()]\n" + "for coll, s in pairs:\n" + " coll.Add(s)\n" + ) + _assert_flagged(code) + + def test_loop_rebinding_a_previously_local_name(self): + # `coll` is a local list first, then rebound per-iteration to an + # element; the earlier local binding must not whitelist it. + code = ( + "coll = []\n" + "for coll in [entry.SensesOS]:\n" + " coll.Add(s)\n" + ) + _assert_flagged(code) + + def test_later_local_rebinding_does_not_whitelist_earlier_lcm_use(self): + # Flow-insensitive "last binding wins" used to leave `x` local. + code = ( + "x = entry.SensesOS\n" + "x.Add(s)\n" + "x = []\n" + ) + _assert_flagged(code) + + def test_guarded_loop_add_is_protected(self): + code = ( + "for coll in [entry.SensesOS]:\n" + " if modifyAllowed:\n" + " coll.Add(s)\n" + ) + cert = certify_script_readonly(code, api_index=None) + assert cert["is_certified_readonly"] is True, cert + assert any(m["method"] == "Add" for m in cert["protected_liblcm_calls"]) + + +class TestSuppressionIsNodeKeyed: + def test_local_add_and_lcm_add_on_one_line(self): + code = ( + "tmp = set()\n" + "tmp.Add(x); entry.SensesOS.Add(s)\n" + ) + adds = _adds(code) + assert len(adds) == 1, adds + assert adds[0]["line"] == 2 + _assert_flagged(code) + + def test_lcm_add_before_local_add_on_one_line(self): + code = ( + "tmp = set()\n" + "entry.SensesOS.Add(s); tmp.Add(x)\n" + ) + assert len(_adds(code)) == 1 + _assert_flagged(code) + + def test_two_local_adds_on_one_line_stay_suppressed(self): + code = ( + "a = set(); b = set()\n" + "a.Add(1); b.Add(2)\n" + ) + assert _adds(code) == [] + + +class TestGenuinelyLocalContainersStillSkipped: + def test_python_list_append_not_flagged(self): + code = ( + "results = []\n" + "for e in project.LexEntry.GetAll():\n" + " results.append(e)\n" + ) + cert = certify_script_readonly(code, api_index=None) + assert cert["is_certified_readonly"] is True, cert + + def test_local_set_add_not_flagged(self): + code = ( + "seen = set()\n" + "for e in project.LexEntry.GetAll():\n" + " seen.Add(e)\n" + ) + assert _adds(code) == [] + assert certify_script_readonly(code, api_index=None)["is_certified_readonly"] + + def test_alias_of_local_container_not_flagged(self): + code = ( + "seen = set()\n" + "alias = seen\n" + "alias.Add(1)\n" + ) + assert _adds(code) == [] + + def test_walrus_and_annotated_local_containers_not_flagged(self): + code = ( + "acc: set = set()\n" + "acc.Add(1)\n" + "if (bag := set()) is not None:\n" + " bag.Add(2)\n" + ) + assert _adds(code) == [] + + def test_augassign_with_local_value_keeps_name_local(self): + code = ( + "seen = set()\n" + "seen |= {1}\n" + "seen.Add(2)\n" + ) + assert _adds(code) == [] + + +class TestNonNameBindingsDisqualify: + """Pattern audit sibling: bindings that are not `ast.Name` stores.""" + + def test_helper_parameter_named_like_a_local_list(self): + code = ( + "def helper(senses, s):\n" + " senses.Add(s)\n" + "def Main(project, report, modifyAllowed):\n" + " senses = []\n" + " for e in project.LexiconAllEntries():\n" + " helper(e.SensesOS, None)\n" + ) + _assert_flagged(code) + + def test_lambda_parameter(self): + code = ( + "tmp = []\n" + "f = lambda tmp, s: tmp.Add(s)\n" + "f(e.SensesOS, None)\n" + ) + _assert_flagged(code) + + def test_import_alias(self): + code = ( + "def a():\n" + " tmp = []\n" + "def b():\n" + " from x import y as tmp\n" + " tmp.Add(s)\n" + ) + _assert_flagged(code) + + def test_except_and_match_names(self): + code = ( + "tmp = []\n" + "try:\n" + " pass\n" + "except Exception as tmp:\n" + " tmp.Add(s)\n" + ) + _assert_flagged(code) + code = ( + "tmp = []\n" + "match e:\n" + " case [*tmp]:\n" + " tmp.Add(s)\n" + ) + _assert_flagged(code) + + def test_plain_local_list_still_not_flagged(self): + code = ( + "def Main(project, report, modifyAllowed):\n" + " results = []\n" + " results.Add(1)\n" + ) + assert _adds(code) == [] diff --git a/tests/test_issue351_regex_word_boundary.py b/tests/test_issue351_regex_word_boundary.py new file mode 100644 index 0000000..067cea8 --- /dev/null +++ b/tests/test_issue351_regex_word_boundary.py @@ -0,0 +1,144 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +"""Issue #351: write-gate regexes over-matched inside longer names. + +The gate fails closed, so only over-matches that cannot be a real receiver +are dropped: + +- a resolved facade name is one exact identifier (`fx` not in `prefx`); +- a handful of exact English words (`nonsense.Form`, `compose.Comment`, + `position.Note`) are not receivers. + +Everything else stays gated on purpose. Compound FLEx receivers +(`subsense`, `subentry`, `lexentry`, `new_entry`, `newEntry`) have no +structural boundary that separates them from those words, and any name +ending in `project` (`self._project`, `srcProject`, `myproject`) is a +plausible FLExProject handle. +""" + +import pytest + +from flextoolsmcp.server.validators import ( + _PATTERN_CREATE_GENERIC, + _PATTERN_CREATE_PROJECT, + _PATTERN_DELETE_PROJECT, + _PATTERN_PROJECT_ACCESSOR_CALL, + _PATTERN_PROPERTY_ASSIGNMENT, + _PATTERN_UPDATE_PROJECT, + certify_script_readonly, + detect_cud_operations, + find_liblcm_mutations, +) + + +def _methods(code, facade_names=None): + return {m["method"] for m in find_liblcm_mutations(code, facade_names)} + + +class TestOverMatchesGone: + @pytest.mark.parametrize( + "code", + [ + "nonsense.Form = 'x'\n", + "compose.Comment = 'x'\n", + "nonsense.Gloss = 'x'\n", + "position.Note = 'x'\n", + "self.purpose.Comment = 'x'\n", + ], + ) + def test_property_receiver_inside_a_longer_word(self, code): + assert "property=" not in _methods(code), code + assert not _PATTERN_PROPERTY_ASSIGNMENT.search(code) + + def test_facade_receiver_inside_a_longer_word(self): + code = "prefx.LexEntry.SetLexemeForm(e, 'x')\n" + assert not any(m.startswith("fx.") for m in _methods(code, {"fx"})) + + def test_generic_add_receiver_inside_a_longer_word(self): + assert not _PATTERN_CREATE_GENERIC.search("nonsense.Things.Add(x)") + + +class TestRealReceiversStillGated: + @pytest.mark.parametrize( + "code", + [ + "project.LexEntry.Delete(e)\n", + "self.project.LexEntry.Delete(e)\n", + "self._project.LexEntry.Delete(e)\n", + "srcProject.LexEntry.Delete(e)\n", + "myproject.LexEntry.Delete(e)\n", + "old_project.LexEntry.Delete(e)\n", + "subproject.Senses.CreateSense(e)\n", + "project.LexEntry.Create('x')\n", + "project.LexEntry.SetLexemeForm(e, 'x')\n", + ], + ) + def test_project_receiver(self, code): + assert any(m.startswith("project.") for m in _methods(code)), code + assert detect_cud_operations(code)["is_cud"] + + def test_facade_receiver(self): + code = "fx.LexEntry.SetLexemeForm(e, 'x')\n" + assert "fx.*.Set/Update" in _methods(code, {"fx"}) + + @pytest.mark.parametrize( + "code", + [ + "entry.LexemeFormOA = form\n", + "sense.Gloss = 'x'\n", + "self.sense.Gloss = 'x'\n", + "new_entry.LexemeFormOA = form\n", + "newEntry.LexemeFormOA = form\n", + "targetSense.Definition = d\n", + "pos2.Comment = c\n", + "entryObj.Comment = c\n", + "subsense.Definition = None\n", + "subentry.MorphTypeRA = None\n", + "lexentry.LexemeFormOA = None\n", + "mainentry.Comment = c\n", + "self._sense.Gloss = 'x'\n", + "nonsenseEntry.Gloss = 'x'\n", + ], + ) + def test_property_receivers(self, code): + assert "property=" in _methods(code), code + assert _PATTERN_PROPERTY_ASSIGNMENT.search(code) + + def test_generic_add_receivers(self): + assert _PATTERN_CREATE_GENERIC.search("entry.Things.Add(x)") + assert _PATTERN_CREATE_GENERIC.search("newEntry.Things.Add(x)") + assert _PATTERN_CREATE_GENERIC.search("new_sense.Things.Add(x)") + + def test_cud_project_patterns(self): + assert _PATTERN_CREATE_PROJECT.search("project.LexEntry.Create('x')") + assert _PATTERN_DELETE_PROJECT.search("self.project.LexEntry.Delete(e)") + assert _PATTERN_UPDATE_PROJECT.search("project.Senses.SetGloss(s, 'x')") + assert _PATTERN_CREATE_PROJECT.search("myproject.LexEntry.Create('x')") + assert _PATTERN_DELETE_PROJECT.search("self._project.LexEntry.Delete(e)") + assert _PATTERN_UPDATE_PROJECT.search("subproject.Senses.SetGloss(s, 'x')") + assert _PATTERN_PROJECT_ACCESSOR_CALL.search("self._project.LexEntry.Delete(e)") + + def test_generic_add_compound_receivers(self): + assert _PATTERN_CREATE_GENERIC.search("subsense.Things.Add(x)") + assert _PATTERN_CREATE_GENERIC.search("lexentry.Things.Add(x)") + + +class TestReviewerProbes: + """Whole-script repros: these must not certify read-only.""" + + @pytest.mark.parametrize( + "code", + [ + "for subsense in e.SensesOS:\n subsense.Definition = None\n", + "for lexentry in project.LexiconAllEntries():\n" + " lexentry.LexemeFormOA = None\n", + "for subentry in project.LexiconAllEntries():\n" + " subentry.MorphTypeRA = None\n", + "class T:\n" + " def run(self, e):\n" + " self._project.LexEntry.Delete(e)\n", + ], + ) + def test_not_certified(self, code): + cert = certify_script_readonly(code, None) + assert cert["is_certified_readonly"] is False, cert diff --git a/tests/test_issue352_compound_guard.py b/tests/test_issue352_compound_guard.py new file mode 100644 index 0000000..d49d3f8 --- /dev/null +++ b/tests/test_issue352_compound_guard.py @@ -0,0 +1,187 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +"""Issue #352: `if modifyAllowed and :` is a write guard; `or` is not. + +Also covers the sibling found by the pattern audit: the Compare branch of +`_is_write_enabled_check` accepted ANY comparison mentioning `modifyAllowed` +or `project.writeEnabled`, so `if modifyAllowed == False:` certified the +write in its body as protected. +""" + +import pytest + +from flextoolsmcp.server.validators import ( + certify_script_readonly, + find_protected_ranges, +) + +_CREATE = " project.LexEntry.Create('x', 'stem')\n" + + +def _certified(code): + return certify_script_readonly(code, api_index=None)["is_certified_readonly"] + + +class TestCompoundAndGuardProtects: + @pytest.mark.parametrize( + "test", + [ + "modifyAllowed and existing is None", + "existing is None and modifyAllowed", + "a and modifyAllowed and b", + "project.writeEnabled and existing is None", + "(modifyAllowed and a) and b", + "modifyAllowed == True and a", + "a and (modifyAllowed or modifyAllowed)", + ], + ) + def test_and_with_write_operand_protects_body(self, test): + code = "existing = project.LexEntry.Find('x')\nif " + test + ":\n" + _CREATE + assert _certified(code), test + + def test_issue_repro(self): + code = ( + "existing = project.LexEntry.Find('x')\n" + "if modifyAllowed and existing is None:\n" + " project.LexEntry.Create('x', 'stem')\n" + ) + cert = certify_script_readonly(code, api_index=None) + assert cert["is_certified_readonly"] is True + assert cert["unprotected_liblcm_calls"] == [] + assert cert["protected_liblcm_calls"] + + def test_nested_form_still_protects(self): + code = ( + "if modifyAllowed:\n" + " if existing is None:\n" + " project.LexEntry.Create('x', 'stem')\n" + ) + assert _certified(code) + + def test_mutation_in_test_before_guard_operand_is_not_protected(self): + # `x.Add(s)` runs before modifyAllowed is evaluated. + code = ( + "if entry.SensesOS.Add(s) and\\\n" + " modifyAllowed:\n" + " pass\n" + ) + assert not _certified(code) + + def test_mutation_under_negated_compound_test_is_not_protected(self): + # `not (x or not flag)` is an enabling guard, but `x` always runs. + code = ( + "if not (entry.SensesOS.Add(None) or not modifyAllowed):\n" + " pass\n" + ) + assert not _certified(code) + + def test_one_line_compound_guard_with_mutating_test(self): + code = "if entry.SensesOS.Add(None) and modifyAllowed: pass\n" + assert not _certified(code) + + def test_one_line_body_after_mutating_test_fails_closed(self): + # Ranges are line-keyed: the body cannot be told apart from the test. + code = "if entry.SensesOS.Add(None) and modifyAllowed: x.SensesOS.Add(s)\n" + assert not _certified(code) + + def test_call_in_test_still_protects_next_line_body(self): + code = ( + "if modifyAllowed and project.LexEntry.Find('x') is None:\n" + + _CREATE + ) + assert _certified(code) + + def test_else_branch_not_protected(self): + code = ( + "if modifyAllowed and a:\n" + " pass\n" + "else:\n" + _CREATE + ) + assert not _certified(code) + + +class TestNonGuardsRejected: + @pytest.mark.parametrize( + "test", + [ + "modifyAllowed or existing is None", + "existing is None or modifyAllowed", + "not (modifyAllowed and a)", + "a and not modifyAllowed", + "modifyAllowed == False", + "modifyAllowed is False", + "modifyAllowed != True", + "modifyAllowed is not True", + "False == modifyAllowed", + "project.writeEnabled == False", + "project.writeEnabled != True", + "modifyAllowed == other", + "modifyAllowed < 1", + ], + ) + def test_body_not_protected(self, test): + code = "if " + test + ":\n" + _CREATE + assert not _certified(code), test + + def test_true_comparisons_still_protect(self): + for test in ( + "modifyAllowed == True", + "modifyAllowed is True", + "True == modifyAllowed", + "modifyAllowed != False", + "project.writeEnabled == True", + ): + assert _certified("if " + test + ":\n" + _CREATE), test + + +class TestEarlyReturnIdiomStillWorks: + def _main(self, guard): + return ( + "def Main(project, report, modifyAllowed):\n" + f" if {guard}:\n" + " return\n" + " project.LexEntry.Create('x')\n" + ) + + @pytest.mark.parametrize( + "guard", + [ + "not modifyAllowed", + "modifyAllowed == False", + "not project.writeEnabled", + "not modifyAllowed or existing is not None", + "existing is not None or not modifyAllowed", + "not (modifyAllowed and existing is None)", + ], + ) + def test_tail_protected(self, guard): + assert _certified(self._main(guard)), guard + + @pytest.mark.parametrize( + "guard", + [ + "not modifyAllowed and existing is not None", + "not (modifyAllowed or existing is None)", + "modifyAllowed", + ], + ) + def test_tail_not_protected(self, guard): + assert not _certified(self._main(guard)), guard + + def test_issue139_range_unchanged(self): + code = ( + "def Main(project, report, modifyAllowed):\n" + " if not modifyAllowed:\n" + " report.Info('dry run')\n" + " return\n" + " project.LexEntry.Create('x')\n" + ) + assert find_protected_ranges(code) == [(5, 5)] + + +def test_scaffold_marks_compound_guard_as_modifying(): + from flextoolsmcp.server.validators import _MODIFY_GUARD_RE + + assert _MODIFY_GUARD_RE.search(" if existing is None and modifyAllowed:\n") + assert _MODIFY_GUARD_RE.search(" if not modifyAllowed:\n") + assert not _MODIFY_GUARD_RE.search(" report.Info(modifyAllowed)\n")