From 072a26cbe85b0247db2c948b5878f6e8bb0a769d Mon Sep 17 00:00:00 2001 From: abrichr Date: Mon, 19 Oct 2026 14:20:15 -0400 Subject: [PATCH 1/2] fix(flow): enforce native form validation and leave script validation to the GUI Review of #1269: only validate.js presence rules were enforced. A browser also checks native HTML constraints, and a form's own script can validate or rewrite values before it submits. - build_body checks required, minlength, maxlength, pattern, min, max and the email, url, number and date input types on what it sends, and the validate.js presence, length, format, numericality and email rules. - A form whose script the API step can't run stays on the GUI: an on* handler on the form or on a sent field (OpenEMR's capitalizeMe rewrites the typed name), a page script listening for submit, or a validate.js rule of another kind (OpenEMR's pastDate). Refused at proposal; unavailable at run time when the live form has it. Co-Authored-By: Claude Opus 5.5 --- packages/flow/docs/API_TIER.md | 26 ++- .../openadapt_flow/compiler/api_proposal.py | 26 +++ .../runtime/actuators/web_form.py | 209 ++++++++++++++++-- .../tests/test_api_step_native_validation.py | 155 +++++++++++++ 4 files changed, 396 insertions(+), 20 deletions(-) create mode 100644 packages/flow/tests/test_api_step_native_validation.py diff --git a/packages/flow/docs/API_TIER.md b/packages/flow/docs/API_TIER.md index a1f2b5e66..38242de35 100644 --- a/packages/flow/docs/API_TIER.md +++ b/packages/flow/docs/API_TIER.md @@ -110,9 +110,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 @@ -201,9 +208,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 1d50a4c5d..97a89ce08 100644 --- a/packages/flow/openadapt_flow/compiler/api_proposal.py +++ b/packages/flow/openadapt_flow/compiler/api_proposal.py @@ -65,6 +65,8 @@ client_rules, folded_checks_digest, form_controls, + form_scripts, + unsupported_client_rules, ) @@ -347,6 +349,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 eda6d6ce0..27915c01a 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,171 @@ 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" + for attr, bad in (("min", number.__lt__), ("max", number.__gt__)): + limit = _number(str(control.get(attr) or "")) + if limit is not None and bad(limit): + return f"{name} is outside the form's {attr} {control.get(attr)}" + 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)" + for attr, bad in (("min", value.__lt__), ("max", value.__gt__)): + limit = control.get(attr) + if limit and bad(limit): + return f"{name} is outside the form's {attr} {limit}" + 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 @@ -447,6 +608,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) @@ -573,13 +750,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 "document.forms[0].addEventListener('submit', v)" + ), + "listening for submit", + ), + ( + lambda h: h.replace( + '{"form_lname":{"presence":true}}', + '{"form_lname":{"presence":true,"pastDate":{}}}', + ), + "form_lname pastDate", + ), + ], +) +def test_script_the_api_step_cant_run_keeps_the_save_on_the_gui(tmp_path, edit, named): + html = edit(FORM_HTML) + _wf, step, reasons = propose_api_step(_workflow(), _rec_with_form(tmp_path, html)) + assert step is None and named in " ".join(reasons) + # The same script on the live form makes an admitted API step unavailable. + live = edit(FORM_HTML.replace("", "live-session-value")) + (tmp_path / "admitted").mkdir() + out = WebFormActuator(FakeTransport(form=live)).actuate( + _admitted_binding(tmp_path / "admitted"), + {"lname": "Marsh", "state_label": "California", "note": "Referred"}, + ) + assert out.status is ActuationStatus.UNAVAILABLE + # A rule kind the recording didn't have is first a changed rule set. + assert named in out.reason or "rules changed" in out.reason From 914539ac5b8c517c854bcbd5b7821c69a1dbd527 Mon Sep 17 00:00:00 2001 From: abrichr Date: Mon, 19 Oct 2026 18:26:18 -0400 Subject: [PATCH 2/2] fix(flow): type the native min/max checks separately (mypy) Co-Authored-By: Claude Opus 5.5 --- .../runtime/actuators/web_form.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/actuators/web_form.py b/packages/flow/openadapt_flow/runtime/actuators/web_form.py index 27915c01a..8473683b2 100644 --- a/packages/flow/openadapt_flow/runtime/actuators/web_form.py +++ b/packages/flow/openadapt_flow/runtime/actuators/web_form.py @@ -273,17 +273,20 @@ def native_problem(name: str, control: dict[str, Any], value: str) -> Optional[s number = _number(value) if number is None: return f"{name} is not a number" - for attr, bad in (("min", number.__lt__), ("max", number.__gt__)): - limit = _number(str(control.get(attr) or "")) - if limit is not None and bad(limit): - return f"{name} is outside the form's {attr} {control.get(attr)}" + 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)" - for attr, bad in (("min", value.__lt__), ("max", value.__gt__)): - limit = control.get(attr) - if limit and bad(limit): - return f"{name} is outside the form's {attr} {limit}" + 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