fix(change_request): evaluate field-mapping transform expressions again - #459
Conversation
_eval_expression passed nocopy=True to safe_eval, which takes no such argument in Odoo 19. Every transform expression therefore raised TypeError, the blanket fallback swallowed it, and the untransformed value was written to the registrant — so a configured Expression transform was silently ignored, reported only as a warning in the log. It was the only caller passing nocopy in the repository. Failures now log at exception level with the offending expression: a quiet warning is precisely how this went unnoticed. The fallback itself is unchanged — a broken expression still writes the raw value rather than failing the apply — which is arguably wrong for the same fail-closed reason as rejecting an apply that can write nothing, but that is a separate behavioural decision. Tests cover a working transform, one referencing the registrant, one whose result matches the stored value, the fallback plus its log, and that env stays unreachable from an expression — that boundary matters now that expressions actually execute.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #459 +/- ##
==========================================
+ Coverage 75.93% 76.47% +0.53%
==========================================
Files 627 655 +28
Lines 43000 44229 +1229
==========================================
+ Hits 32654 33823 +1169
- Misses 10346 10406 +60
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Absorbs the security batch-2 merge (#422), which took spp_change_request_v2 to 19.0.3.1.10. This branch keeps 19.0.3.1.11, so no renumbering was needed — the version stack held. All conflicts are metadata: the manifest version, both sides of the changelog, both sides of the test registrations, and the two generated README files. Worth noting the combination: #422 made conflict/duplicate detection share the apply strategy's value comparison, and this branch makes transform expressions actually evaluate. Together, detection now honours transforms for the first time. The invariant test added in #422 — detection agrees with whether apply wrote anything — passes unchanged.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
The nocopy diagnosis and fix are exactly right — I confirmed Odoo 19's safe_eval(expr, /, context=None, *, mode="eval", filename=None) has no such kwarg, that field_mapping.py was the only nocopy caller repo-wide, and the bite check holds (re-adding nocopy=True fails test_transform_expression_is_applied with 'jane' != 'JANE'). 402/402 green, version chain correct post-#422, and the upgrade blast radius is customer-config only (no shipped CR type uses transform='expression'; Studio always writes direct). Nice catch, and the SQL pre-rollout probe in the description is the right call.
But requesting changes on security grounds: the fix flips a live privilege-escalation path from inert to exploitable, and merging as-is ships it. Two reviewers independently confirmed the escape by executing it against Odoo 19's real safe_eval in a container, so this is demonstrated, not theoretical.
The core problem
The eval context is {value, detail, registrant, datetime, date} with a # env removed for security comment — but registrant and detail are live recordsets, and Odoo 19 safe_eval applies no attribute allowlist (only the dunder-name block + the fixed _UNSAFE_ATTRIBUTES list; recordsets pass check_values). Confirmed at runtime as valid mode="eval" expressions:
registrant.env['res.users'].sudo().search([]).mapped('login')→['admin']registrant._cr.execute("SELECT 1")→ ran real SQL on the live cursorregistrant.sudo(),detail.env['ir.config_parameter']→ live objects
The apply path (change_request.py _do_apply → self.sudo() → strategy.apply) already runs as superuser, so registrant.env there is a superuser env — .sudo() isn't even needed. And the detection path reaches _eval_expression too (conflict_mixin.mapping_changes_value, fired on CR create at change_request.py:665 and submit at :1124), so a plain group_cr_user executes the manager-authored expression in their own session.
Who can author it: transform_expression is writable by group_cr_manager (security/ir.model.access.csv:9), which implies only group_cr_validator — not base.group_system. The field has no groups= guard and the view exposes it ungated. So an ordinary Change Request Manager — no Settings, no sysadmin — can obtain superuser ORM + raw SQL. The help text's "Only administrators should configure" is aspirational, not enforced. It was inert only because the nocopy TypeError fired first; this PR makes it live.
Required before merge (agreed full-hardening scope)
1. Keep ORM handles out of the eval context (inline on field_mapping.py). Snapshot each record into an attribute-readable object with no env/_cr, so the "no env" promise is actually true. Sketch:
from types import SimpleNamespace
def _expression_record_view(self, record):
"""Attribute-readable snapshot with no ORM handle attached.
safe_eval permits arbitrary non-dunder attribute access, so a live
recordset in the context exposes record.env / record.sudo() / record._cr
-- the full ORM as superuser and the DB cursor. Excluding ``env`` from
the context means nothing while a recordset is in it.
"""
if not record:
return None
values = {}
for name, field in record._fields.items():
if not field.store or field.type in ("one2many", "many2many"):
continue
value = record[name]
values[name] = value.id if field.type == "many2one" else value
return SimpleNamespace(**values)Pass "detail": self._expression_record_view(detail), "registrant": self._expression_record_view(registrant). SimpleNamespace keeps registrant.family_name working (so the documented syntax and the ref test still pass) and its __dict__ is dunder-blocked. Many2one→id matches how proposed_target_value already normalises. Document the narrowing (no method calls / no relation traversal) in the field help and HISTORY. Also delete the now-false # env removed for security comment.
2. Make "administrators only" enforced (models/change_request_type_mapping.py:34, not in this diff so noting here):
transform_expression = fields.Char(
groups="base.group_system",
help=(...),
)Field-level groups is ORM-enforced on read/write and holds over RPC. Precedent: spp_dci/models/signing_key.py:48, spp_oauth/models/res_config_settings.py:21, with spp_oauth/tests/test_config_settings_acl.py:112 pinning field-groups as a server-side boundary. XML data loads run as superuser and are unaffected; already-configured rows keep evaluating, only editing is restricted. Add a test asserting a group_cr_manager user cannot write transform_expression.
3. Fix the misleading test (inline). test_env_is_not_reachable_from_an_expression asserts only bare env[...] fails — never the reachable path — so it reads as "sandbox closed" when it isn't. Assert the real escapes fall back after the fix (subTest over registrant.env[...], registrant.sudo(), registrant._cr, detail.env.cr).
4. Stop routing PII to ERROR logs (inline). _logger.exception renders safe_eval's wrapped error, and value-echoing failures embed the field value (reproduced: invalid literal for int() with base 10: 'Juan Dela Cruz'). Log type(error).__name__ + expr at ERROR, full traceback at DEBUG. The existing "transform expression failed" assertion still passes.
5. Fail closed (inline). value is requester-controlled, so the raw-value fallback lets a low-priv requester force the untransformed value to be written by feeding input the transform can't handle (e.g. non-numeric into int(value)) — bypassing any normalise/validate/mask transform on demand. This is the same failure shape as 3.1.10's own "wrote nothing, reported success" fix. Raise UserError on the write path (message must OMIT the underlying error text — _apply_change_request persists str(e) into apply_error, which would DB-persist the finding-4 PII), and make mapping_changes_value catch UserError and return True (stay on) rather than propagate, so the un-try-guarded _run_conflict_checks on create doesn't break. (UserError is in safe_eval's _BUBBLEUP_EXCEPTIONS and can't be constructed from an expression.)
Suggestions
- The
_eval_expressiondocstring narrates the historicalnocopybug — that belongs in HISTORY (where it already is); state the security contract instead (no ORM handle in the context). test_unrelated_apply_still_raises_without_a_detailtests pre-existing apply behaviour unrelated to the transform fix; it belongs with the other apply guards intest_apply_effective_mappings.py.- Config-time validation: an
@api.constrainsusingtest_python_expr(expr, mode="eval")would reject a bad expression at save rather than at apply (precedentspp_alerts/models/alert_rule.py:225). Optional, on-theme.
Out of scope — filing as a follow-up
The same class of hole pre-exists in spp_programs/models/managers/base_manager.py:14-20, which puts "user": self.env.user (a live recordset) into a safe_eval context used for eligibility/payment/entitlement domain and condition expressions — user.env / .sudo() / ._cr are all reachable, and its nosemgrep justification has the same blind spot. Not this PR's code; I'll open a separate issue.
Verified clean
Bug real and correctly diagnosed; version 3.1.10→3.1.11 correct now #422 has landed; HISTORY OCA newest-first and calls out the upgrade behaviour change; README/index.html regenerated consistently (a local table reflow is a hook-version artifact — 19.0 has the identical narrow form, not PR drift); nosemgrep odoo-unsafe-safe-eval is the exact firing rule, correctly placed (though its stated justification, "restricted context (no env)", is what finding 1 refutes); preview() reads raw values so it isn't an extra eval trigger; no existing tests removed or weakened; lint/pre-commit clean.
Merge order note: this is the head of 454 → 459 → 461 → 462, so these changes need to settle here before 461/462 rebase on top.
| """ | ||
| try: | ||
| # Admin-defined field mapping expressions with restricted context (no env) | ||
| return safe_eval( # nosemgrep: odoo-unsafe-safe-eval |
There was a problem hiding this comment.
Must fix (C1): the sandbox is escapable — a live recordset in the context carries the full superuser ORM and the DB cursor.
safe_eval in Odoo 19 puts no allowlist on attribute access (only dunder-name blocking + the fixed _UNSAFE_ATTRIBUTES list). registrant and detail are live recordsets, so despite # env removed for security two lines below, these are all valid mode="eval" expressions, confirmed by execution:
registrant.env['res.users'].sudo().search([]).mapped('login') # -> ['admin']
registrant._cr.execute("UPDATE res_users SET active = true") or value # raw SQL, returns value so it looks like a working transform
detail.env['ir.config_parameter'].sudo().get_param('database.secret')Apply runs the strategy under self.sudo() (change_request.py _do_apply), so registrant.env is already a superuser env there. Combined with the fact that group_cr_manager (not a sysadmin) can author the expression, this is a CR-manager → superuser code-exec path.
Fix: don't hand a recordset to safe_eval. Snapshot each record into a handle-free SimpleNamespace via a _expression_record_view helper (full sketch + rationale in the review summary), pass those instead of detail/registrant, and delete the now-false # env removed for security comment. SimpleNamespace preserves registrant.family_name-style access so the documented syntax keeps working; its __dict__ is dunder-blocked.
There was a problem hiding this comment.
Confirmed and fixed in 38e05b5. The context no longer receives live recordsets: _eval_expression now passes _expression_record_view(detail) / _expression_record_view(registrant) — a SimpleNamespace snapshot of stored scalar fields only (many2one reduced to id, x2many skipped), so env / sudo() / _cr are unreachable while registrant.family_name-style access still works. The false # env removed for security comment is gone and the docstring now states the security contract instead.
I verified the escape against Odoo 19 FINAL directly: assert_no_dunder_name blocks only __-names + the fixed _UNSAFE_ATTRIBUTES list and check_values rejects only modules, so recordset attribute access is wide open — exactly as you described.
| # Falls back to the untransformed value rather than failing the | ||
| # apply. Logged at exception level with the expression, because a | ||
| # silent warning is how the ``nocopy`` breakage went unnoticed. | ||
| _logger.exception("Field mapping transform expression failed, using the raw value: %s", expr) |
There was a problem hiding this comment.
Two things here.
Must fix (I1 — PII in logs): _logger.exception renders safe_eval's wrapped error text, and a value-echoing failure embeds the registrant's field value. Reproduced with int(value) over a name field:
ValueError: ValueError("invalid literal for int() with base 10: 'Juan Dela Cruz'") while evaluating 'int(value)'
…at ERROR with a full traceback. The repo's no-PII log hook passes because it only inspects the format args (expr, which is admin config). Given spp_pii_encryption/spp_data_classification are in-repo, keep it loud without the value:
except Exception as error:
_logger.error(
"Field mapping transform expression failed (%s), using the raw value: %s",
type(error).__name__, expr,
)
_logger.debug("Transform expression failure detail", exc_info=True)Your existing assertLogs(... "transform expression failed" ...) at ERROR still passes.
Must fix (I2 — fail closed): falling back to the raw value is an attacker-triggerable bypass, because value is requester-controlled: a low-priv requester can feed input the transform can't handle to force the untransformed value onto the registrant, defeating any normalise/validate/mask transform. Same failure shape as 3.1.10's "wrote nothing, reported success" fix. Raise UserError on the write path (message must OMIT the underlying error text — _apply_change_request persists str(e) into apply_error, which would DB-persist the PII above), and make mapping_changes_value catch UserError and return True so detection stays on and the un-try-guarded _run_conflict_checks on create doesn't break. (Details in the review summary.)
There was a problem hiding this comment.
Both done in 38e05b5.
I1 (PII in logs): the except now logs type(error).__name__ + expr at ERROR and the full traceback at DEBUG (exc_info=True) — the wrapped error text (which embeds the field value) never reaches ERROR. Added test_the_failure_log_does_not_leak_the_field_value asserting an int(value) failure over "Juan Dela Cruz" produces no ERROR line containing the value.
I2 (fail closed): an unevaluable expression now raises UserError instead of returning the raw value. The message omits the underlying error text, since _apply_change_request persists str(e) into apply_error. And mapping_changes_value catches UserError and returns True, so the un-try-guarded _run_conflict_checks on create/submit stays intact and the mapping stays visible to detection rather than dropping out.
| "the failure must be logged loudly enough to be diagnosable", | ||
| ) | ||
|
|
||
| def test_env_is_not_reachable_from_an_expression(self): |
There was a problem hiding this comment.
Must fix (C3): this test pins the wrong boundary and gives false assurance. It asserts only that the bare name env[...] fails — but that was never the reachable path; the escape is registrant.env / registrant._cr / detail.env, which this test says nothing about. A future reader will read a green test_env_is_not_reachable as "the sandbox is closed."
After the C1 snapshot lands, assert the real boundary instead:
def test_the_orm_is_not_reachable_from_an_expression(self):
"""env alone is not the boundary: a recordset in the context carries
env, sudo() and _cr with it, and safe_eval permits arbitrary
non-dunder attribute access."""
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"):
self._apply(cr_type, {"given_name": "jane"})
self.assertEqual(self.registrant.given_name, "jane")(distinct code per case — the type code is unique.) With the snapshot in place these raise AttributeError inside the eval and fall back, which is exactly what you want to lock in.
There was a problem hiding this comment.
Rewrote it in 38e05b5 as test_the_orm_is_not_reachable_from_an_expression — a subTest over env[...], registrant.env[...], registrant.sudo(), registrant._cr, detail.env.cr, exactly the reachable boundary.
One divergence from your snippet: because I2 makes the path fail closed, the snapshot makes each of these raise inside the eval and the apply now raises rather than falling back. So the test asserts both assertRaises(UserError) and that the registrant value is unchanged — which together lock in "the escape is closed and nothing was written," rather than the assertEqual-only form. Distinct type code per case as you noted.
…ions The transform-expression fix restored evaluation of a context holding live `detail`/`registrant` recordsets. safe_eval applies no attribute allowlist, so those exposed env, sudo() and the cursor -- a change-request manager (not a system admin) could reach superuser ORM and raw SQL. Harden the path: - Pass attribute-readable snapshots (stored scalars only, m2o->id) instead of recordsets, so no ORM handle is reachable from an expression. - Restrict transform_expression to base.group_system (ORM-enforced), and read it via sudo() in the detection path, which runs as the requester -- otherwise the field-groups guard would raise AccessError or silently skip the transform and put detection and apply back out of step. - Fail closed: an unevaluable expression raises instead of writing the raw, requester-controlled value. Log error type + expression at ERROR (never the wrapped error, which embeds the PII field value), traceback at DEBUG only; the UserError message omits the underlying error since it is persisted to apply_error. Tests assert the ORM escapes fail closed, the failure log carries no field value, and a CR manager cannot author the expression.
|
Pushed 38e05b5 addressing the required changes. Per-thread replies are inline (C1 sandbox, I1/I2 logs+fail-closed, C3 test). Two things that weren't inline threads: Item 2 (administrators-only, enforced). One gap in the item-2 plan I had to close. The field guard alone breaks dynamic-approval submission: the detection path ( Follow-up you flagged: agree Full |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Round 2. All five items from the first review are genuinely fixed — verification notes below — and the C3 divergence (assertRaises + unchanged-value instead of my assertEqual-only sketch) is an improvement, as is scoping the detection-path sudo() to the admin-authored config while PII still reads as the requester. But the snapshot builder introduces one new must-fix that breaks detection for real (non-admin) requesters in every deployment, and there are two smaller items.
Verified fixed
- C1a: the eval context now carries
SimpleNamespacesnapshots only — nothing in it owns anenv,sudo()or cursor;__dict__is caught by the dunder check. The false# env removed for securitycomment is gone and the docstring states the contract. - C2:
groups="base.group_system"is ORM-enforced, the help text is honest, andtest_cr_manager_cannot_write_transform_expressionpins it. - C3:
test_the_orm_is_not_reachable_from_an_expressionnow asserts the real boundary (registrant.env,registrant.sudo(),registrant._cr,detail.env.cr) with fail-closed + nothing-written assertions. - I1: ERROR carries the error type and the expression only;
test_the_failure_log_does_not_leak_the_field_valuepins theJuan Dela Cruzcase; traceback demoted to DEBUG. - I2: unevaluable expressions raise
UserError(kept clean of the wrapped text since it lands inapply_error), andmapping_changes_valuetreats that as a change so the mapping stays visible to detection. - Version
19.0.3.1.11is correct now that #422 put3.1.10on 19.0; HISTORY entries are thorough. - The
base_managerfollow-up you offered to take is already filed as #468.
Must fix (new, C4) — the snapshot reads gated fields as the requester, so detection is broken for non-admin users
Details inline on _expression_record_view.
Must fix (CI) — pre-commit is red
oca-gen-addon-readme wants README.rst / static/description/index.html regenerated after the help-text and HISTORY changes — consistent with the --no-verify commit. Regenerate and push.
Should fix — silence the GHAS odoo-sudo-without-context alert
Inline on line 62.
| return None | ||
| values = {} | ||
| for name, field in record._fields.items(): | ||
| if not field.store or field.type in ("one2many", "many2many"): |
There was a problem hiding this comment.
Must fix (C4): this loop reads group-gated fields as the current user, which breaks detection for every non-admin requester.
res.partner.signup_type is a stored scalar Char gated groups="base.group_erp_manager" (Odoo 19 auth_signup — present in every deployment via spp_registry → portal → auth_signup). On the detection path the registrant is read as the requester, and field __get__ calls _check_field_access(self, 'read') for a non-su user — the very odoo/orm/fields.py check you cited for transform_expression. So for a requester without base.group_erp_manager (i.e. every normal CR user), building the registrant snapshot raises AccessError before the expression ever runs.
That lands inside _eval_expression's try, so it is logged at ERROR (blaming the expression), re-raised as UserError, and mapping_changes_value catches it and returns True. Net effect for any expression-transform mapping used by a non-admin:
- the expression never evaluates during detection, and
- detection always reports the field as changed, even when identical —
detection and apply are out of step again, the exact regression 3.1.10 closed (now in the over-flagging direction), plus one ERROR log line per conflict check. The suite cannot see it: TransactionCase runs as superuser and field-group checks are skipped when env.su.
Fix — skip gated fields on both paths, and while touching the line, make the filter a whitelist:
if not field.store or field.groups or field.type in ("one2many", "many2many", "binary", "reference"):
continuefield.groups: skipping them under sudo-apply too is deliberate — if apply could readsignup_typebut detection couldn't, the two would diverge again, and a transform has no business reading admin-gated fields. Worth a line in the help text.binary:image_1920& co. arestore=True(attachment only moves the bytes toir.attachment), so today each eval snapshot hauls the registrant's image payloads into memory for nothing.reference: a storedfields.Referencevalue is a live recordset and sails straight past themany2onereduction — the C1 escape reopened. No partner/detail model has one today (the repo's onlyfields.Referenceisspp_programs'manager_ref_id), but this helper is the sandbox boundary; exclude by type, don't rely on nobody adding one.
And the pinning test (fails on this branch): run detection as a plain CR user — an expression-transform mapping whose proposed value equals the stored value must yield mapping_changes_value(...) == False, with no ERROR logged. E.g. create the CR with_user(cr_user) and assert both; that locks in "the snapshot is buildable by a requester" independent of which gated fields core grows next.
There was a problem hiding this comment.
Confirmed and fixed in b06c9c8 — and your diagnosis reproduced exactly: run as a plain group_cr_user, the snapshot build died on res.partner.signup_type (Access Denied by ACLs for operation: read ... field: signup_type), landed in the expression's try, logged ERROR blaming the expression, and mapping_changes_value returned True for a value-preserving transform.
The filter is now the whitelist you sketched:
if not field.store or field.groups or field.type in ("one2many", "many2many", "binary", "reference"):
continuewith the docstring explaining all three exclusions (gated fields skipped under sudo-apply too, deliberately, so the two paths build the identical snapshot; binary to keep image payloads out of every eval; reference because a stored Reference value is itself a live recordset). Help text and HISTORY updated to match.
Pinning test: test_detection_snapshot_is_buildable_by_a_plain_cr_user creates the CR with_user a plain internal CR user and asserts mapping_changes_value(...) is False for a proposed-equals-stored transform with assertNoLogs(..., level="ERROR"). Observed failing before the fix with exactly the AccessError-on-signup_type shape. One note: the call is made directly on the strategy as the requester (the CR-create path only reaches mapping_changes_value for dynamic-approval types via _proposed_changed_fields), which pins the primitive independent of type configuration.
| # ``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() |
There was a problem hiding this comment.
Should fix: GHAS flags this as odoo-sudo-without-context. The justification comment above it is exactly right — keep it — but add the repo-conventional inline annotation so the alert doesn't re-fire on every push:
config = mapping.sudo() # nosemgrep: odoo-sudo-without-context(matches the existing annotations in change_request.py).
There was a problem hiding this comment.
Done in b06c9c8 — # nosemgrep: odoo-sudo-without-context added on the mapping.sudo() line, justification comment kept, matching the annotations in change_request.py.
…dable fields only The snapshot builder read every stored scalar field as the current user, and core gates some behind groups (res.partner.signup_type needs base.group_erp_manager via auth_signup). On the detection path -- which runs as the requester -- building the registrant snapshot raised AccessError before the expression ever evaluated, so every expression-transform mapping was flagged as changed and logged an ERROR per conflict check, for every non-admin user. Gated fields are skipped on the apply path too, so detection and apply build the identical snapshot. Binary fields are excluded to keep image payloads out of every evaluation, and Reference fields because a stored Reference value is itself a live recordset -- the handle the snapshot exists to keep out. Also annotates the detection-path config read for semgrep.
|
Round 2 addressed in b06c9c8. C4 (snapshot reads gated fields as the requester) — fixed with your whitelist, CI (README regeneration) — Should fix (GHAS) — Full |
The local oca-gen-addon-readme environment renders RST tables wider than CI's (a docutils rendering difference the dependency pins do not cover across Python versions), and the previous regeneration was committed under --no-verify, so CI's pre-commit rejected the drift. This applies the exact output CI's hook produces; content is unchanged.
|
CI is green as of 925f778. The pre-commit failure after b06c9c8 was the README table reflow you predicted in round 1: the local generator renders the RST tables wider than CI's (a docutils rendering difference across Python versions that the dependency pins don't cover), and the round-1 regeneration had been committed under |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Round 3 verified — approving.
- C4: the snapshot filter is now the whitelist on the single shared helper, so detection and apply build identical snapshots by construction:
field.groupsexcluded on both paths (with the apply-must-match-detection rationale in the docstring),binaryexcluded (no payload hauling),referenceexcluded (a stored Reference value is itself the live-recordset handle this snapshot exists to keep out). - The pinning test is exactly the right shape: a plain CR user (
base.group_user+group_cr_user, no erp_manager), a no-op transform, asserting both no over-flag and no ERROR — the case the superuser suite is structurally blind to. Observed red first, per your note. - README/index.html regenerated with CI's generator — pre-commit is green again.
- nosemgrep annotation on the
mapping.sudo()read, matching repo convention. - Help text and HISTORY updated to state the exclusions honestly. CI fully green across the board, 405/405.
This closes out the full-hardening scope: the original nocopy restoration, the C1 sandbox escape, C2 admin-only enforcement, C3 honest boundary test, I1 log hygiene, I2 fail-closed, and C4 requester-buildable snapshots. Nice work across three rounds — the escape reproductions and red-first tests made each fix verifiable rather than takeable on faith.
Merge authorization is Edwin's; #461 (3.1.12) and #462 (3.1.13) queue behind this, each needing the mechanical squash-conflict resolution at merge time.
#459 was squash-merged as 481cc27, conflicting with this branch in the version line, HISTORY, and the generated README/index.html. Resolved to 19.0.3.1.12 with both HISTORY entries stacked (3.1.12 over 3.1.11), and the README/index.html regenerated from the resolved fragments with the repo's pinned oca-gen-addon-readme hook.
#459 and #461 were squash-merged (481cc27, ac0e38e), conflicting with this branch in the version line, HISTORY, tests/__init__.py, and the generated README/index.html. Resolved to 19.0.3.1.13 with the HISTORY entries stacked (3.1.13 over 3.1.12 over 3.1.11), both test imports kept, and README/index.html regenerated from the resolved fragments with CI's table-width rendering applied.
3 participants
Field-mapping Expression transforms have never run on Odoo 19. They are silently ignored and the raw value is written instead.
The bug
spp_change_request_v2/strategies/field_mapping.pycalled:Odoo 19's signature is
safe_eval(expr, /, context=None, *, mode="eval", filename=None)— there is nonocopyparameter. Every expression therefore raisedTypeError, which the surroundingexcept Exceptionswallowed, logging a warning and returning the value untransformed. It is the only caller passingnocopyanywhere in the repository.Found while making conflict/duplicate detection compare values the same way apply does (in #422): a test asserting the transform affected the derived change set failed, and the probe showed why —
🔴 Behaviour change on upgrade
Any change-request type with an Expression transform configured has been writing raw values. After this it transforms them. If a deployment has such a type and has (unknowingly) come to rely on the pass-through, the values it writes will change. Worth checking before rolling out:
🔒 Security note
The
transform_expressionhelp text already says "WARNING: Only administrators should configure expressions - arbitrary code execution risk." While broken, that risk was inert. This restores an intended-but-dormant capability rather than introducing a new one, and the surface stays bounded: evaluation goes throughsafe_eval, and the context deliberately excludesenv— a test now pins that, since it matters once expressions actually execute.Logging
Failures move from
warningtoexception, and include the offending expression. A quiet warning is exactly how this stayed invisible.Open question — should the fallback fail closed?
A broken expression still falls back to writing the raw value. That means a misconfigured transform silently lands untransformed data on the registrant, which is the same shape of problem as an apply that writes nothing while reporting success. Raising instead would fail closed. Left unchanged here because it is a separate behavioural decision, not part of restoring the feature — happy to follow up either way.
Tests
Seven cases: a working transform, one referencing the registrant, one whose result equals the stored value (so nothing is written), a broken expression falling back and logging at ERROR,
envunreachable, direct (non-expression) mappings unaffected, and a missing detail still raising rather than being masked by the fallback.Full
spp_change_request_v2suite: 338 tests, 0 failures.Merge order
spp_change_request_v2goes 19.0.3.1.2 → 19.0.3.1.11, skipping3.1.3–3.1.10, which #422 claims. Safe to merge after #422; if it needs to land first, the version must be renumbered here instead.