diff --git a/packages/flow/docs/API_TIER.md b/packages/flow/docs/API_TIER.md index 22199d968..152d04921 100644 --- a/packages/flow/docs/API_TIER.md +++ b/packages/flow/docs/API_TIER.md @@ -112,9 +112,16 @@ Before it sends the save, the API step: 1. reads the form page again in this session; 2. refuses if the form's fields or its client-side validation rules (for example, OpenEMR's `constraints` object) differ from the recording; -3. builds the body, and refuses if a field the form requires would be - empty; -4. runs the pre-checks. +3. refuses if the form runs script it can't run: an `on*` handler on the + form (`onsubmit`) or on a field it sends (`onchange`, `oninput`, + `onblur`, script that can validate or rewrite a value), a page script + listening for the submit, or a validate.js rule kind other than + `presence`, `length`, `format`, `numericality` and `email`; +4. builds the body and checks it the way the browser would before it + submits: native HTML constraints (`required`, `minlength`, `maxlength`, + `pattern`, `min`, `max`, and the `email`, `url`, `number` and `date` + input types) and the page's validate.js rules; +5. runs the pre-checks. Each of these refusals means the save wasn't sent, so the GUI steps run. After the save request starts, the API tier's no-double-write rule @@ -295,9 +302,16 @@ recording. over a websocket aren't proposed. - The server may enforce less than the page. On OpenEMR, the duplicate check and the required fields run only in the browser. The API step - carries over the checks it can see (pre-checks and `constraints` - presence rules). Logic in the page's script that it can't see is the main - risk of this tier, and it's why a person admits each API step. + carries over the checks it can evaluate (pre-checks, native HTML + constraints and validate.js rules of the kinds above), and leaves a form + whose script it can't run to the GUI. A `pattern` is evaluated with + Python's regular expressions, which differ from the browser's in rare + syntax; one that doesn't compile keeps the save on the GUI. Letting + qualification show that the server enforces a script-only rule, so the + API step can run anyway, isn't built yet. +- OpenEMR's new-patient form runs script on submit (`submitme(...)`) and on + each name field (`capitalizeMe`, which rewrites the typed value), and its + `constraints` use a custom `pastDate` rule. So its save stays on the GUI. - Multi-step server state (wizards, uploads, locks held between pages) needs the whole request sequence, and isn't proposed. diff --git a/packages/flow/openadapt_flow/compiler/api_proposal.py b/packages/flow/openadapt_flow/compiler/api_proposal.py index d1da259b8..622802c85 100644 --- a/packages/flow/openadapt_flow/compiler/api_proposal.py +++ b/packages/flow/openadapt_flow/compiler/api_proposal.py @@ -66,6 +66,8 @@ client_rules, folded_checks_digest, form_controls, + form_scripts, + unsupported_client_rules, ) @@ -348,6 +350,30 @@ def carried(line: dict[str, Any]) -> int: form_path, url_problems = _url_template(form_line["url"], by_value) if url_problems: return workflow, None, url_problems + # A browser runs the page's validation and script before it submits; the + # API step can only stand in for what it can evaluate itself. + unsupported = unsupported_client_rules(client_rules(form_line["response_text"])) + if unsupported: + return ( + workflow, + None, + [ + "the form validates with rules the API step can't evaluate " + f"({', '.join(unsupported)}); it stays on the GUI" + ], + ) + scripts = form_scripts( + form_line["response_text"], _path(post["url"]), [n for n, _ in post["form"]] + ) + if scripts: + return ( + workflow, + None, + [ + "the form runs script the API step can't run " + f"({', '.join(scripts[:5])}); it stays on the GUI" + ], + ) # Steps the API step stands in for: those recorded on the form page. folded: list[Step] = [] diff --git a/packages/flow/openadapt_flow/runtime/actuators/web_form.py b/packages/flow/openadapt_flow/runtime/actuators/web_form.py index 14bc2fe75..60547a231 100644 --- a/packages/flow/openadapt_flow/runtime/actuators/web_form.py +++ b/packages/flow/openadapt_flow/runtime/actuators/web_form.py @@ -42,7 +42,7 @@ import urllib.parse as up from dataclasses import dataclass from html.parser import HTMLParser -from typing import Any, Optional, Protocol +from typing import Any, Iterable, Optional, Protocol from openadapt_flow.runtime.actuators.api import ( ActuationStatus, @@ -73,7 +73,12 @@ def handle_starttag(self, tag: str, attrs: list[tuple[str, Optional[str]]]) -> N # controls belong to, and are posted with, the outer form. self._nested += 1 return - self._form = {"action": a.get("action", ""), "controls": {}} + self._form = { + "action": a.get("action", ""), + "controls": {}, + "handlers": sorted(k for k in a if k.startswith("on")), + "novalidate": "novalidate" in a, + } self.forms.append(self._form) return if self._form is None: @@ -89,7 +94,8 @@ def handle_starttag(self, tag: str, attrs: list[tuple[str, Optional[str]]]) -> N value = a.get("value", "on" if kind in ("checkbox", "radio") else "") self._form["controls"].setdefault( name, - {"kind": kind, "default": value, "options": None, "id": a.get("id")}, + {"kind": kind, "default": value, "options": None, "id": a.get("id")} + | _native_rules(a), ) elif tag == "select" and name: self._select = { @@ -98,7 +104,7 @@ def handle_starttag(self, tag: str, attrs: list[tuple[str, Optional[str]]]) -> N "default": None, "options": [], "multiple": "multiple" in a, - } + } | _native_rules(a) self._form["controls"].setdefault(name, self._select) elif tag == "option" and self._select is not None: self._option = { @@ -113,7 +119,7 @@ def handle_starttag(self, tag: str, attrs: list[tuple[str, Optional[str]]]) -> N "default": "", "options": None, "id": a.get("id"), - } + } | _native_rules(a) self._form["controls"].setdefault(name, self._textarea) def handle_data(self, data: str) -> None: @@ -144,16 +150,174 @@ def handle_endtag(self, tag: str) -> None: self._form = None -def form_controls(html: str, action_path: str) -> Optional[dict[str, Any]]: - """The named controls of the form in ``html`` that posts to ``action_path``.""" +def _native_rules(a: dict[str, str]) -> dict[str, Any]: + """A control's native HTML constraints and its script handlers.""" + + return { + "required": "required" in a, + "pattern": a.get("pattern"), + "min": a.get("min"), + "max": a.get("max"), + "minlength": a.get("minlength"), + "maxlength": a.get("maxlength"), + "disabled": "disabled" in a, + "handlers": sorted(k for k in a if k.startswith("on")), + } + +def _form(html: str, action_path: str) -> Optional[dict[str, Any]]: parser = _FormParser() parser.feed(html) leaf = up.urlsplit(action_path).path.rsplit("/", 1)[-1] for form in parser.forms: action = up.urlsplit(form["action"]).path if action == action_path or (leaf and action.rsplit("/", 1)[-1] == leaf): - return form["controls"] + return form + return None + + +def form_controls(html: str, action_path: str) -> Optional[dict[str, Any]]: + """The named controls of the form in ``html`` that posts to ``action_path``.""" + + form = _form(html, action_path) + return form["controls"] if form is not None else None + + +_SUBMIT_LISTENER = re.compile( + r"""addEventListener\(\s*['"]submit['"]|\.on\(\s*['"]submit['"]|\.submit\(\s*function""", +) + + +def form_scripts(html: str, action_path: str, sent: Iterable[str]) -> list[str]: + """Script the browser would run on this save that the API step can't. + + The form's own ``on*`` handlers (``onsubmit``), the ``on*`` handlers of + the fields the save sends (``onchange``, ``oninput``, ``onblur``: script + that can validate or rewrite a value, such as capitalizing a name), and + page script that listens for the submit. Each is named; empty when none. + """ + + form = _form(html, action_path) + if form is None: + return [] + found = [f"form {h}" for h in form["handlers"]] + sent_names = set(sent) + for name, control in form["controls"].items(): + if name in sent_names: + found += [f"{name} {h}" for h in control.get("handlers") or []] + if _SUBMIT_LISTENER.search(html): + found.append("a page script listening for submit") + return found + + +#: validate.js rule kinds the API step evaluates. Any other kind is script it +#: can't run (for example OpenEMR's ``pastDate``). +CLIENT_RULE_KINDS = frozenset({"presence", "length", "format", "numericality", "email"}) + + +def unsupported_client_rules(rules: Optional[dict[str, Any]]) -> list[str]: + """``field kind`` for every validate.js rule the API step can't evaluate.""" + + out = [] + for name, rule in (rules or {}).items(): + if isinstance(rule, dict): + out += [f"{name} {kind}" for kind in rule if kind not in CLIENT_RULE_KINDS] + return sorted(out) + + +_EMAIL = re.compile(r"^[^\s@]+@[^\s@]+\.[^\s@]+$") + + +def _number(value: str) -> Optional[float]: + try: + return float(value) + except ValueError: + return None + + +def native_problem(name: str, control: dict[str, Any], value: str) -> Optional[str]: + """Why the browser's own validation would refuse ``value``, or None. + + The constraints a browser checks before it submits: ``required``, + ``minlength``/``maxlength``, ``pattern``, ``min``/``max`` and the input + ``type`` (email, url, number, date). A pattern that can't be compiled is + refused (it can't be evaluated). + """ + + if control.get("disabled") or control.get("kind") == "hidden": + return None + if control.get("required") and value == "": + return f"{name} is required by the form and would be empty" + if value == "": + return None + for attr, test in ( + ("minlength", lambda n: len(value) < n), + ("maxlength", lambda n: len(value) > n), + ): + limit = control.get(attr) + if limit not in (None, "") and str(limit).isdigit() and test(int(limit)): + return f"{name} is {len(value)} characters; the form's {attr} is {limit}" + pattern = control.get("pattern") + if pattern: + try: + if re.fullmatch(f"(?:{pattern})", value) is None: + return f"{name} doesn't match the form's pattern" + except re.error: + return f"{name}'s pattern can't be evaluated outside the browser" + kind = control.get("kind") + if kind == "email" and not all(_EMAIL.match(v.strip()) for v in value.split(",")): + return f"{name} is not an email address" + if kind == "url" and not re.match(r"^[A-Za-z][A-Za-z0-9+.-]*:", value): + return f"{name} is not a URL" + if kind in ("number", "range"): + number = _number(value) + if number is None: + return f"{name} is not a number" + low = _number(str(control.get("min") or "")) + high = _number(str(control.get("max") or "")) + if low is not None and number < low: + return f"{name} is outside the form's min {control.get('min')}" + if high is not None and number > high: + return f"{name} is outside the form's max {control.get('max')}" + if kind == "date": + if not re.fullmatch(r"\d{4}-\d{2}-\d{2}", value): + return f"{name} is not a date (YYYY-MM-DD)" + low_date, high_date = control.get("min"), control.get("max") + if low_date and value < str(low_date): + return f"{name} is outside the form's min {low_date}" + if high_date and value > str(high_date): + return f"{name} is outside the form's max {high_date}" + return None + + +def client_rule_problem(name: str, rule: dict[str, Any], value: str) -> Optional[str]: + """Why a validate.js rule would refuse ``value``, or None.""" + + if rule.get("presence") and value == "": + return f"{name} is required by the form and would be empty" + if value == "": + return None + length = rule.get("length") + if isinstance(length, dict): + for key, bad in ( + ("minimum", lambda n: len(value) < n), + ("maximum", lambda n: len(value) > n), + ("is", lambda n: len(value) != n), + ): + if isinstance(length.get(key), int) and bad(length[key]): + return f"{name} has the wrong length for the form" + fmt = rule.get("format") + pattern = fmt.get("pattern") if isinstance(fmt, dict) else fmt + if isinstance(pattern, str): + try: + if re.fullmatch(f"(?:{pattern})", value) is None: + return f"{name} doesn't match the form's format" + except re.error: + return f"{name}'s format can't be evaluated outside the browser" + if rule.get("numericality") and _number(value) is None: + return f"{name} is not a number" + if rule.get("email") and not _EMAIL.match(value): + return f"{name} is not an email address" return None @@ -477,6 +641,22 @@ def unavailable(reason: str) -> ApiActuationResult: return unavailable( "the form's validation rules changed since the recording" ) + unsupported = unsupported_client_rules(spec.client_rules) + if unsupported: + return unavailable( + "the form validates with rules the API step can't evaluate " + f"({', '.join(unsupported)})" + ) + scripts = form_scripts( + page.text, + up.urlsplit(save_path).path, + [f.name for f in spec.fields], + ) + if scripts: + return unavailable( + "the form runs script the API step can't run " + f"({', '.join(scripts[:5])})" + ) body, problem = self.build_body(spec, params, controls) if problem: return unavailable(problem) @@ -603,13 +783,17 @@ def build_body( body.append((field.name, field.value or "")) if enforce_rules: sent = dict(body) + # The browser's own checks before it submits, then the page's + # validate.js rules: the API step skips neither. + for name, value in body: + problem = native_problem(name, controls.get(name) or {}, value) + if problem: + return [], problem for name, rule in (spec.client_rules or {}).items(): - if ( - isinstance(rule, dict) - and rule.get("presence") - and not sent.get(name) - ): - return [], f"{name} is required by the form and would be empty" + if isinstance(rule, dict): + problem = client_rule_problem(name, rule, sent.get(name, "")) + if problem: + return [], problem return body, None diff --git a/packages/flow/tests/test_api_step_native_validation.py b/packages/flow/tests/test_api_step_native_validation.py new file mode 100644 index 000000000..6ca5eea5a --- /dev/null +++ b/packages/flow/tests/test_api_step_native_validation.py @@ -0,0 +1,155 @@ +"""The API step enforces what the browser would check before it submits. + +The reviewer's case for #1269: only validate.js ``presence`` rules were +enforced. A browser also checks native HTML constraints (required, pattern, +min, max, maxlength, type), and a form's own script can validate or rewrite +values; the API step either evaluates them or leaves the save to the GUI. +Synthetic values only. +""" + +from __future__ import annotations + +import json + +import pytest + +from openadapt_flow import network_capture as nc +from openadapt_flow.compiler.api_proposal import propose_api_step +from openadapt_flow.ir import WebFormField, WebFormSpec +from openadapt_flow.runtime.actuators import ActuationStatus +from openadapt_flow.runtime.actuators.web_form import ( + WebFormActuator, + client_rule_problem, + form_controls, + native_problem, +) +from tests.test_api_tier_web_form import ( + FORM_HTML, + FakeTransport, + _admitted_binding, + _recording, + _workflow, +) + +NATIVE = """
""" + + +def _controls(): + return form_controls(NATIVE, "/save.php") + + +@pytest.mark.parametrize( + "name, value, problem", + [ + ("code", "", "required"), + ("code", "abc-12", "pattern"), + ("code", "ABC-123", "maxlength"), + ("email", "not-an-address", "email"), + ("dose", "11", "max"), + ("dose", "0", "min"), + ("dose", "ten", "number"), + ("seen", "2026-10-20", "max"), + ("seen", "10/19/2026", "date"), + ], +) +def test_native_constraints_refuse_what_the_browser_would(name, value, problem): + found = native_problem(name, _controls()[name], value) + assert found is not None and problem in found + + +@pytest.mark.parametrize( + "name, value", + [ + ("code", "ABC-12"), + ("email", "a@b.example"), + ("dose", "5"), + ("seen", "2026-10-01"), + ("h", ""), + ], +) +def test_native_constraints_accept_valid_values(name, value): + assert native_problem(name, _controls()[name], value) is None + + +def test_validate_js_length_format_and_numericality_are_enforced(): + rule = {"length": {"maximum": 3}, "format": {"pattern": "[a-z]+"}} + assert "length" in client_rule_problem("f", rule, "abcd") + assert "format" in client_rule_problem("f", rule, "AB") + assert client_rule_problem("f", {"numericality": True}, "x") is not None + assert client_rule_problem("f", rule, "ab") is None + + +def test_the_actuator_refuses_a_value_the_browser_would_refuse(tmp_path): + spec = WebFormSpec( + form_path="/new.php", + fields=[WebFormField(name="code", source="param", param="code")], + controls=["code", "dose", "email", "h", "seen"], + replaces_steps=["s0"], + folded_checks_digest="sha256:" + "0" * 64, + ) + body, problem = WebFormActuator.build_body(spec, {"code": "abc"}, _controls()) + assert body == [] and "pattern" in problem + + +def _rec_with_form(tmp_path, html): + rec = _recording(tmp_path) + rows = [json.loads(x) for x in (rec / nc.NETWORK_FILE).read_text().splitlines()] + for row in rows[1:]: + if "