security(cr): route and apply the same single field for dynamic approvals#264
Open
gonzalesedwin1123 wants to merge 5 commits into
Open
security(cr): route and apply the same single field for dynamic approvals#264gonzalesedwin1123 wants to merge 5 commits into
gonzalesedwin1123 wants to merge 5 commits into
Conversation
Dynamic-approval CR types route and approve based on a single selected field (selected_field_name), but the field_mapping apply strategy iterated over ALL apply_mapping_ids and wrote every changed detail field to the registrant under sudo(). A CR user could select a low-risk field (routing to a weak/local approval), also change high-risk mapped fields on the detail, obtain the weak approval, and have every changed field written — a privilege-escalation bypass of the tiered approval routing. (field_mapping also treats empty detail values as intentional clears, so non-selected fields could even be wiped.) Restrict field_mapping to the selected field's mapping for dynamic-approval CR types, in both apply() and preview() (so the approver preview matches what is written). Fail closed: a missing or unmapped selection applies nothing. Non-dynamic CR types are unchanged. The custom apply strategy is out of scope (author-controlled writes). Tests: a dynamic CR applies/previews only the selected field even when another mapped field changed; an unmapped selection writes nothing; non-dynamic CRs still apply all changed mappings. spp_change_request_v2 302/302.
Follow-up to the apply-time restriction: close the time-of-check/time-of-use desync where the routed change stayed mutable after submission. A user could route on a low-risk field (weak approval), then before apply either swap the selected field, change the routed field's value, or substitute the detail record — applying an unapproved field/value under the weaker approval. The views already made these readonly once approval_state left draft/revision; this enforces the same on the server so it cannot be bypassed via RPC. - spp.cr.detail.base.write(): reject changes to proposed-change fields (field_to_modify + field_mapping source fields) when the CR is not in draft/revision. Apply-output fields (created_*_id) are not protected, so apply strategies still record results post-approval. - spp.change.request.write(): once submitted, reject changes to selected_field_name/old/new value AND detail_res_id/detail_res_model. get_detail() resolves the detail strictly by that pointer for both routing and apply, so freezing it prevents substituting a second detail record. Editing requires resetting to draft, which re-routes the approval. Tests: field swap, direct selected_field_name write, value swap, and detail substitution are all rejected post-submit; draft edits still allowed. spp_change_request_v2 307/307; spp_cr_type_assign_program 19/19, spp_farmer_registry_cr 95/95 (custom detail types' created_* writes unaffected).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security and integrity constraints to freeze change requests and their proposed details once they leave the draft or revision state, preventing users from modifying approved changes before they are applied. It also updates the field mapping strategy to apply and preview only the selected field for dynamic-approval change requests, backed by comprehensive unit tests. The review feedback correctly identifies potential false-positive lockouts in both change_request.py and change_request_detail_base.py due to direct comparisons of unset fields (where None is compared to False) and Odoo recordsets, which should be normalized to prevent unexpected UserError exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #264 +/- ##
==========================================
+ Coverage 75.30% 75.34% +0.04%
==========================================
Files 1095 1097 +2
Lines 64991 65474 +483
==========================================
+ Hits 48939 49332 +393
- Misses 16052 16142 +90
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ckout Address Gemini review on PR #264: the post-submit freeze guards compared the write payload to the stored value directly. Odoo stores unset fields as False, but a JSON-RPC/integration payload may send None for the same field, and a Many2one may be written as a recordset — so an idempotent re-save (None vs False, or recordset vs the id-normalized current) was wrongly treated as a change and locked out. Normalize both sides (recordset -> id, None -> False) in both the CR and detail guards via a shared _normalize_frozen_value helper. Real changes still differ and are still blocked. Test: writing None to an already-unset protected field post-submit no longer raises. spp_change_request_v2 308/308.
Record the dynamic-approval single-field apply + post-submit freeze hardening in the version and HISTORY changelog.
gonzalesedwin1123
force-pushed
the
security-cr-dynamic-approval-field
branch
from
1be95e6 to
45c8063
Compare
…I generator The earlier README regeneration was run in a local env whose RST renderer produced wider (28-char) table columns than CI's pinned oca-gen environment (26-char), causing the 'Generate addons README files from fragments' pre-commit hook to fail on CI. Restore the column widths CI's generator produces so the generated docs match the fragments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dynamic-approval CR types route/approve on a single selected field (
selected_field_name, synced from the detail'sfield_to_modify), but thefield_mappingapply strategy iterated over allapply_mapping_idsand wrote every changed detail field to the registrant undersudo(). A CR user could select a low-risk field (routing to a weak/local approval), also change high-risk mapped fields (birthdate, identifiers, …), obtain the weak approval, and have all changed fields written — a privilege-escalation bypass of the tiered approval routing. (field_mapping also treats empty detail values as intentional clears, so non-selected fields could be wiped.)Fix (two layers)
1. Apply-restriction — for dynamic-approval CR types,
field_mappingapply()andpreview()operate only on the mapping whosesource_field == selected_field_name(the routed + approved field). Fail-closed: a missing/unmapped selection applies nothing. Non-dynamic types are unchanged; thecustomstrategy is out of scope (author-controlled writes).2. Post-submit freeze — the apply-restriction alone left a time-of-check/time-of-use gap: the selection and detail stayed mutable after routing (the views already made them readonly once submitted, but that was UI-only). Enforced server-side:
spp.cr.detail.base.write()rejects changes to proposed-change fields (field_to_modify+ field_mapping source fields) onceapproval_state not in ('draft','revision'). Apply-output fields (created_*_id) are not protected, so apply strategies still record results post-approval.spp.change.request.write()rejects changes toselected_field_name/old/new value anddetail_res_id/detail_res_modelonce submitted —get_detail()resolves the detail strictly by that pointer for both routing and apply, so freezing it prevents substituting a second detail record.Editing requires resetting to draft, which re-routes the approval.
Tests
selected_field_namewrite, value-swap, and detail-substitution (repointdetail_res_id) are all rejected post-submit; draft edits still allowed.spp_change_request_v2307/307; downstream custom-detail modulesspp_cr_type_assign_program19/19,spp_farmer_registry_cr95/95 (apply's post-approvalcreated_*writes unaffected). Lint/semgrep clean.Reviewed
Three adversarial staff-review passes. The first two each found a real residual bypass — (a) the initial apply-restriction left a TOCTOU field/value swap; (b) then a detail-substitution via
detail_res_idrepoint — both fixed. Final pass: APPROVE, substitution closed on every detail-resolution path, no new breakage.Follow-up (out of scope)
Freeze the CR's identity fields post-submit too —
registrant_id(swap → apply to a different person) andrequest_type_id— same "frozen once approved" class, but needs anenv.sudiscriminator (not a client-forgeable context flag) because thecreate_groupapply strategy legitimately writesregistrant_idpost-approval. Tracked for a deliberate follow-up.