diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 4ee4aba4..cec371c0 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,44 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.1.11 +~~~~~~~~~~~ + +- fix(change_request): field-mapping transform expressions are evaluated + again. ``_eval_expression`` passed ``nocopy=True`` to ``safe_eval``, + which takes no such argument in Odoo 19, so every expression raised + ``TypeError``; the blanket fallback swallowed it and the + **untransformed** value was written to the registrant. A configured + transform was therefore ignored, reported only as a warning in the + log. **Behaviour change:** request types that already have an + Expression transform configured will start transforming values on + upgrade, having silently passed the raw value through until now. +- fix(security): a transform expression can no longer reach the ORM. + ``safe_eval`` places no allowlist on attribute access, so a live + ``detail``/``registrant`` recordset in the evaluation context exposed + ``env``, ``sudo()`` and the database cursor — a change-request + manager, who is not a system administrator, could obtain superuser ORM + access and raw SQL. The context now carries attribute-readable + snapshots of the two records (stored scalar fields only; no methods, + no relation traversal, no database handle) instead of the recordsets + themselves. Group-gated, binary and reference fields are excluded from + the snapshot: a gated field cannot be read by the requester on the + detection path — and apply must build the identical snapshot or the + two disagree again — a binary would haul image payloads into every + evaluation, and a stored Reference value is itself a live recordset. +- fix(security): the transform expression is now restricted to system + administrators (``groups="base.group_system"``) rather than only + warned against in the help text, and is enforced by the ORM on read + and write. The detection path reads the expression as superuser so it + keeps working for non-administrator requesters. +- fix(security): an unevaluable transform expression now fails closed — + the change is not applied — instead of falling back to writing the raw + value. Because the source value is requester-controlled, the fallback + let a requester force the untransformed value onto the registrant by + feeding input the transform could not handle. Failures are logged with + the expression and error type (never the field value, which is PII); + the full traceback is logged only at DEBUG. + 19.0.3.1.10 ~~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 1f10e1b9..f34e3557 100644 --- a/spp_change_request_v2/__manifest__.py +++ b/spp_change_request_v2/__manifest__.py @@ -1,6 +1,6 @@ { "name": "OpenSPP Change Request V2", - "version": "19.0.3.1.10", + "version": "19.0.3.1.11", "sequence": 50, "category": "OpenSPP", "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention", diff --git a/spp_change_request_v2/models/change_request_type_mapping.py b/spp_change_request_v2/models/change_request_type_mapping.py index d4275fe0..53879ffb 100644 --- a/spp_change_request_v2/models/change_request_type_mapping.py +++ b/spp_change_request_v2/models/change_request_type_mapping.py @@ -32,10 +32,15 @@ class SPPChangeRequestTypeMapping(models.Model): default="direct", ) transform_expression = fields.Char( + groups="base.group_system", help=( "Python expression for value transformation. " - "Available variables: value, detail, registrant, datetime, date. " - "WARNING: Only administrators should configure expressions - " - "arbitrary code execution risk." + "Available variables: value (the source value), and read-only snapshots " + "of detail and registrant exposing their stored scalar fields only - " + "no method calls, no relation traversal, no database access, and no " + "group-restricted, binary or reference fields - plus " + "datetime and date. Restricted to system administrators: it is evaluated " + "server-side and an unevaluable expression blocks the change rather than " + "writing the raw value." ), ) diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 3ffd3b06..54cd843e 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,10 @@ +### 19.0.3.1.11 + +- fix(change_request): field-mapping transform expressions are evaluated again. `_eval_expression` passed `nocopy=True` to `safe_eval`, which takes no such argument in Odoo 19, so every expression raised `TypeError`; the blanket fallback swallowed it and the **untransformed** value was written to the registrant. A configured transform was therefore ignored, reported only as a warning in the log. **Behaviour change:** request types that already have an Expression transform configured will start transforming values on upgrade, having silently passed the raw value through until now. +- fix(security): a transform expression can no longer reach the ORM. `safe_eval` places no allowlist on attribute access, so a live `detail`/`registrant` recordset in the evaluation context exposed `env`, `sudo()` and the database cursor — a change-request manager, who is not a system administrator, could obtain superuser ORM access and raw SQL. The context now carries attribute-readable snapshots of the two records (stored scalar fields only; no methods, no relation traversal, no database handle) instead of the recordsets themselves. Group-gated, binary and reference fields are excluded from the snapshot: a gated field cannot be read by the requester on the detection path — and apply must build the identical snapshot or the two disagree again — a binary would haul image payloads into every evaluation, and a stored Reference value is itself a live recordset. +- fix(security): the transform expression is now restricted to system administrators (`groups="base.group_system"`) rather than only warned against in the help text, and is enforced by the ORM on read and write. The detection path reads the expression as superuser so it keeps working for non-administrator requesters. +- fix(security): an unevaluable transform expression now fails closed — the change is not applied — instead of falling back to writing the raw value. Because the source value is requester-controlled, the fallback let a requester force the untransformed value onto the registrant by feeding input the transform could not handle. Failures are logged with the expression and error type (never the field value, which is PII); the full traceback is logged only at DEBUG. + ### 19.0.3.1.10 - fix(security): conflict and duplicate detection now decide whether a mapped field changed using the same comparison the apply strategy uses. Detection compared through a helper that lowercases and strips strings while apply compares raw, so a case- or whitespace-only edit was invisible to detection yet still written to the registrant — enough to sidestep a field-scoped conflict rule with a cosmetic edit. Detection also ignored transform expressions, which apply evaluates before comparing. Similarity scoring is unchanged and stays case-insensitive, since that is the point of a fuzzy match. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 22feeaea..705cc88b 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,45 @@

Changelog

+

19.0.3.1.11

+ +
+

19.0.3.1.10

-
+

19.0.3.1.9

-
+

19.0.3.1.8

  • fix(security): scope the Create-Group member wizards to the parent @@ -1413,7 +1452,7 @@

    19.0.3.1.8

    access-control entry grants.
-
+

19.0.3.1.7

  • fix(security): require change-request manager rights to apply a change @@ -1428,7 +1467,7 @@

    19.0.3.1.7

    endpoint.
-
+

19.0.3.1.6

  • fix(security): derive conflict and duplicate detection from the change @@ -1442,7 +1481,7 @@

    19.0.3.1.6

    an empty one, so detection cannot silently disable itself.
-
+

19.0.3.1.5

  • fix(security): scope the CR Requestor, Local Validator and HQ @@ -1454,7 +1493,7 @@

    19.0.3.1.5

    are noupdate.
-
+

19.0.3.1.4

  • fix(security): add ownership and area record rules to every concrete @@ -1471,7 +1510,7 @@

    19.0.3.1.4

    unrestricted delete their access-control entries grant.
-
+

19.0.3.1.3

  • fix(security): route and apply the same single field for @@ -1484,7 +1523,7 @@

    19.0.3.1.3

    the routing selector.
-
+

19.0.3.1.2

  • fix(change_request_v2): adding an ID now looks for a live one of that @@ -1493,7 +1532,7 @@

    19.0.3.1.2

    (#1136)
-
+

19.0.3.1.1

  • fix(change_request): enforce the (cr_type_id, reason) uniqueness @@ -1507,7 +1546,7 @@

    19.0.3.1.1

    applied) so the constraint applies cleanly on upgrade.
-
+

19.0.3.1.0

  • revert(change_request): restore the create-a-new-individual Add @@ -1525,7 +1564,7 @@

    19.0.3.1.0

    not restored here; reinstate separately if needed.
-
+

19.0.3.0.0

  • feat(change_request): redesign the group/membership CR flows (#242) — @@ -1547,7 +1586,7 @@

    19.0.3.0.0

    must adapt (see #1133).
-
+

19.0.2.0.8

  • fix(views): disable inline creation of CR document types on the Change @@ -1558,7 +1597,7 @@

    19.0.2.0.8

    Documents” modal (missing Name field) that blocked saving (#1125)
-
+

19.0.2.0.7

  • fix(security): align CR Requestor / CR Local Validator / CR HQ @@ -1570,7 +1609,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1585,7 +1624,7 @@

    19.0.2.0.6

    list so row-click goes through the stage router.
-
+

19.0.2.0.5

  • fix(security): add a global ir.rule on spp.change.request that @@ -1598,27 +1637,27 @@

    19.0.2.0.5

    roles).
-
+

19.0.2.0.3

  • fix: add HTML escaping to all computed Html fields with sanitize=False to prevent stored XSS (#50)
-
+

19.0.2.0.2

  • fix: fix batch approval wizard line deletion (#130)
-
+

19.0.2.0.1

  • fix: skip field types before getattr and isolate detail prefetch (#129)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_change_request_v2/strategies/field_mapping.py b/spp_change_request_v2/strategies/field_mapping.py index 88c09f99..1e692f5d 100644 --- a/spp_change_request_v2/strategies/field_mapping.py +++ b/spp_change_request_v2/strategies/field_mapping.py @@ -1,5 +1,6 @@ import logging from datetime import date, datetime +from types import SimpleNamespace from odoo import _, models from odoo.exceptions import UserError @@ -53,13 +54,31 @@ def proposed_target_value(self, mapping, detail, registrant): value = getattr(detail, mapping.source_field, None) if hasattr(value, "id"): value = value.id - if mapping.transform == "expression" and mapping.transform_expression: - value = self._eval_expression(mapping.transform_expression, value, detail, registrant) + # The transform is admin-authored configuration. Read it as superuser so + # detection -- which runs as the requester, not under sudo -- can see it: + # ``transform_expression`` is gated by ``groups="base.group_system"``, so a + # plain read by a change-request user would raise AccessError and, worse, + # a silent skip would put detection and apply back out of step. + config = mapping.sudo() # nosemgrep: odoo-sudo-without-context + if config.transform == "expression" and config.transform_expression: + value = self._eval_expression(config.transform_expression, value, detail, registrant) return value def mapping_changes_value(self, mapping, detail, registrant): - """Whether ``mapping`` would write a different value than is stored.""" - return self.proposed_target_value(mapping, detail, registrant) != self.current_target_value(mapping, registrant) + """Whether ``mapping`` would write a different value than is stored. + + A transform that cannot be evaluated fails closed on the apply path + (``_eval_expression`` raises ``UserError``). Detection must not crash on + that and must not silently drop the mapping: treat an unevaluable + transform as a change so the field stays visible to conflict and + duplicate detection. ``_run_conflict_checks`` on create is not + try-guarded, so a propagating error here would break creation. + """ + try: + proposed = self.proposed_target_value(mapping, detail, registrant) + except UserError: + return True + return proposed != self.current_target_value(mapping, registrant) def apply(self, change_request): """Apply field mappings from detail to registrant.""" @@ -131,26 +150,83 @@ def apply(self, change_request): return True + def _expression_record_view(self, record): + """Attribute-readable snapshot of ``record`` with no ORM handle attached. + + ``safe_eval`` permits arbitrary non-dunder attribute access, so a live + recordset in the evaluation context exposes ``record.env`` / + ``record.sudo()`` / ``record._cr`` -- the full ORM (as superuser on the + apply path, which runs under sudo) and the database cursor. Keeping + ``env`` out of the context means nothing while a recordset is in it. + + The snapshot carries stored scalar fields only, so ``registrant.family_name`` + keeps working while method calls and relation traversal do not, and its + ``__dict__`` is blocked by the dunder-name check. Many2one values are + reduced to their id, matching how ``proposed_target_value`` normalises. + + Group-gated fields are excluded on both paths: detection builds the + snapshot as the requester, where reading a gated field (e.g. + ``res.partner.signup_type``) raises AccessError before the expression + runs, and apply -- which runs under sudo, where the read would succeed + -- must build the identical snapshot or the two disagree about what a + mapping writes. Binary fields are excluded so image payloads are not + hauled into every evaluation, and Reference fields because their value + is itself a live recordset -- the handle this snapshot exists to keep + out. + """ + if not record: + return None + values = {} + for name, field in record._fields.items(): + if not field.store or field.groups or field.type in ("one2many", "many2many", "binary", "reference"): + continue + value = record[name] + values[name] = value.id if field.type == "many2one" else value + return SimpleNamespace(**values) + def _eval_expression(self, expr, value, detail, registrant): - """Safely evaluate transform expression.""" + """Safely evaluate a field-mapping transform expression. + + Security contract: the context exposes ``value`` and attribute-readable + snapshots of ``detail`` and ``registrant`` -- never live recordsets, so + no ``env``, ``sudo()`` or cursor is reachable from an expression. It + fails closed: an expression that cannot be evaluated raises rather than + writing the untransformed, requester-controlled ``value`` through. + """ try: - # Admin-defined field mapping expressions with restricted context (no env) return safe_eval( # nosemgrep: odoo-unsafe-safe-eval expr, { "value": value, - "detail": detail, - "registrant": registrant, - # env removed for security + "detail": self._expression_record_view(detail), + "registrant": self._expression_record_view(registrant), "datetime": datetime, "date": date, }, mode="eval", - nocopy=True, ) - except Exception as e: - _logger.warning("Expression eval failed: %s", e) - return value + except Exception as error: + # Fail closed: refuse to write the raw value. ``value`` is + # requester-controlled, so falling back would let a requester force + # the untransformed value onto the registrant by feeding input the + # transform cannot handle. Log the expression and error *type* at + # ERROR -- never the wrapped error text, which embeds the field + # value (PII) -- and the full traceback only at DEBUG. The UserError + # message omits the underlying error for the same reason: it is + # persisted to ``apply_error`` on the change request. + _logger.error( + "Field mapping transform expression failed (%s), refusing to write the raw value: %s", + type(error).__name__, + expr, + ) + _logger.debug("Transform expression failure detail", exc_info=True) + raise UserError( + _( + "A configured field-mapping transform expression could not be evaluated, " + "so the change was not applied. Ask an administrator to review the " + "request type's transform expression; the failure detail is in the server log." + ) + ) from None def _is_value_empty(self, value, record=None, field_name=None): """Check if a value should be considered empty and skipped. diff --git a/spp_change_request_v2/tests/__init__.py b/spp_change_request_v2/tests/__init__.py index 8cf042cf..38ec50cf 100644 --- a/spp_change_request_v2/tests/__init__.py +++ b/spp_change_request_v2/tests/__init__.py @@ -35,3 +35,4 @@ from . import test_frozen_value_normalisation from . import test_frozen_detail_binding from . import test_detection_matches_apply +from . import test_field_mapping_transform diff --git a/spp_change_request_v2/tests/test_field_mapping_transform.py b/spp_change_request_v2/tests/test_field_mapping_transform.py new file mode 100644 index 00000000..644bac93 --- /dev/null +++ b/spp_change_request_v2/tests/test_field_mapping_transform.py @@ -0,0 +1,205 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Field-mapping transform expressions: evaluated, sandboxed, and fail-closed. + +Covers that a configured transform is actually applied (it was once passed a +``nocopy`` kwarg ``safe_eval`` does not accept, so every expression raised and +the raw value was written), that no ORM handle -- ``env``, ``sudo()`` or the +cursor -- is reachable from an expression, that an unevaluable expression fails +closed instead of writing the requester-controlled raw value, that the failure +log does not leak the field value, and that the expression is admin-only. +""" + +from odoo.exceptions import AccessError, UserError +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestFieldMappingTransform(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.registrant = cls.env["res.partner"].create( + { + "name": "Transform Registrant", + "given_name": "john", + "family_name": "Fam", + "is_registrant": True, + "is_group": False, + } + ) + + def _type_with_transform(self, code, expression): + return self.env["spp.change.request.type"].create( + { + "code": code, + "name": code, + "target_type": "individual", + "detail_model": "spp.cr.detail.edit_individual", + "apply_strategy": "field_mapping", + "apply_mapping_ids": [ + ( + 0, + 0, + { + "source_field": "given_name", + "target_field": "given_name", + "transform": "expression", + "transform_expression": expression, + }, + ) + ], + } + ) + + def _apply(self, cr_type, detail_vals): + cr = self.env["spp.change.request"].create({"request_type_id": cr_type.id, "registrant_id": self.registrant.id}) + cr.get_detail().write(detail_vals) + self.env["spp.cr.strategy.field_mapping"].apply(cr) + return cr + + def test_transform_expression_is_applied(self): + cr_type = self._type_with_transform("tf_upper", "value.upper()") + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual( + self.registrant.given_name, + "JANE", + "the configured transform must be applied, not silently ignored", + ) + + def test_transform_can_reference_the_registrant(self): + cr_type = self._type_with_transform("tf_ref", "value + '-' + registrant.family_name") + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual(self.registrant.given_name, "jane-Fam") + + def test_transform_result_is_what_gets_compared(self): + """A transform landing on the stored value means there is nothing to write.""" + cr_type = self._type_with_transform("tf_noop", "'john'") + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual(self.registrant.given_name, "john") + + def test_a_broken_expression_fails_closed(self): + """A transform that cannot be evaluated blocks the apply -- it must not + fall back to writing the raw value. ``value`` is requester-controlled, so + a fallback would let a requester force the untransformed value onto the + registrant by feeding input the transform cannot handle.""" + cr_type = self._type_with_transform("tf_broken", "value.no_such_method()") + with self.assertLogs("odoo.addons.spp_change_request_v2.strategies.field_mapping", level="ERROR") as logs: + with self.assertRaises(UserError): + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual( + self.registrant.given_name, + "john", + "a failing expression must not write anything to the registrant", + ) + self.assertTrue( + any("transform expression failed" in line for line in logs.output), + "the failure must be logged loudly enough to be diagnosable", + ) + + def test_the_failure_log_does_not_leak_the_field_value(self): + """The ERROR log carries the error *type* and the expression, never the + wrapped error text -- which embeds the (PII) field value.""" + cr_type = self._type_with_transform("tf_pii", "int(value)") + with self.assertLogs("odoo.addons.spp_change_request_v2.strategies.field_mapping", level="ERROR") as logs: + with self.assertRaises(UserError): + self._apply(cr_type, {"given_name": "Juan Dela Cruz"}) + error_lines = [line for line in logs.output if line.startswith("ERROR:")] + self.assertTrue(error_lines) + for line in error_lines: + self.assertNotIn("Juan Dela Cruz", line) + + def test_the_orm_is_not_reachable_from_an_expression(self): + """``env`` alone is not the boundary: a live recordset in the context + carries ``env``, ``sudo()`` and ``_cr`` with it, and ``safe_eval`` + permits arbitrary non-dunder attribute access. The record snapshots + close every one of these; each fails closed rather than escaping.""" + for index, expression in enumerate( + ( + "env['res.users'].search([])", + "registrant.env['res.users'].search([])", + "registrant.sudo().family_name", + "registrant._cr", + "detail.env.cr", + ) + ): + with self.subTest(expression=expression): + cr_type = self._type_with_transform(f"tf_escape_{index}", expression) + with self.assertLogs("odoo.addons.spp_change_request_v2.strategies.field_mapping", level="ERROR"): + with self.assertRaises(UserError): + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual(self.registrant.given_name, "john") + + def test_direct_mappings_are_unaffected(self): + cr_type = self.env["spp.change.request.type"].create( + { + "code": "tf_direct", + "name": "tf_direct", + "target_type": "individual", + "detail_model": "spp.cr.detail.edit_individual", + "apply_strategy": "field_mapping", + "apply_mapping_ids": [(0, 0, {"source_field": "given_name", "target_field": "given_name"})], + } + ) + self._apply(cr_type, {"given_name": "jane"}) + self.assertEqual(self.registrant.given_name, "jane") + + def test_unrelated_apply_still_raises_without_a_detail(self): + """Guard against the fallback masking a genuinely missing detail.""" + cr_type = self._type_with_transform("tf_nodetail", "value.upper()") + cr = self.env["spp.change.request"].create({"request_type_id": cr_type.id, "registrant_id": self.registrant.id}) + cr.write({"detail_res_id": False}) + with self.assertRaises(UserError): + self.env["spp.cr.strategy.field_mapping"].apply(cr) + + def test_detection_snapshot_is_buildable_by_a_plain_cr_user(self): + """Detection runs as the requester, and the record snapshot must be + buildable without admin groups: core gates stored scalar fields behind + ``groups=`` (e.g. ``res.partner.signup_type`` needs + ``base.group_erp_manager`` via auth_signup), and reading one as a plain + user raises AccessError before the expression ever runs. Detection + would then over-flag every expression mapping as changed -- putting + detection and apply back out of step -- and log an ERROR on every + conflict check. The superuser suite cannot see this, because field + group checks are skipped when ``env.su``; hence ``with_user``.""" + cr_user = self.env["res.users"].create( + { + "name": "CR User", + "login": "cr_user_tf", + "group_ids": [ + (4, self.env.ref("base.group_user").id), + (4, self.env.ref("spp_change_request_v2.group_cr_user").id), + ], + } + ) + cr_type = self._type_with_transform("tf_plain_user", "'john'") + cr = ( + self.env["spp.change.request"] + .with_user(cr_user) + .create({"request_type_id": cr_type.id, "registrant_id": self.registrant.id}) + ) + strategy = self.env["spp.cr.strategy.field_mapping"].with_user(cr_user) + mapping = cr_type.apply_mapping_ids.with_user(cr_user) + with self.assertNoLogs("odoo.addons.spp_change_request_v2.strategies.field_mapping", level="ERROR"): + changes = strategy.mapping_changes_value(mapping, cr.get_detail(), cr.registrant_id) + self.assertFalse( + changes, + "a transform landing on the stored value must not be flagged as a change", + ) + + def test_cr_manager_cannot_write_transform_expression(self): + """``transform_expression`` is admin-only (``base.group_system``). A + Change Request Manager -- who is not a system administrator -- must not + be able to author the server-side expression, so the "administrators + only" warning is ORM-enforced rather than merely advisory.""" + manager = self.env["res.users"].create( + { + "name": "CR Manager", + "login": "cr_manager_tf", + "group_ids": [(4, self.env.ref("spp_change_request_v2.group_cr_manager").id)], + } + ) + self.assertFalse(manager._has_group("base.group_system")) + cr_type = self._type_with_transform("tf_acl", "value.upper()") + mapping = cr_type.apply_mapping_ids + with self.assertRaises(AccessError): + mapping.with_user(manager).write({"transform_expression": "value.lower()"})