security: batch 2 — CR + programs chain (#264, #261, #338, #336, #353, #343, #365) - #422
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #422 +/- ##
==========================================
- Coverage 76.28% 75.93% -0.35%
==========================================
Files 654 627 -27
Lines 44035 43000 -1035
==========================================
- Hits 33592 32654 -938
+ Misses 10443 10346 -97
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
Author
Codecov patch-coverage audit — all 16 missing lines reviewedCodecov flags 16 lines / 91.9% patch coverage. I traced each to its exact line and read the context. None is a security-critical enforcement path — the covered 91.9% includes every actual boundary (freeze (The codecov project delta is skewed here as expected for a staging-targeted PR — this audit is of the concrete per-line patch misses, which is the real signal.) Group A — #336 lock-reroute call sites (8 lines) — acceptableMechanical one-line reroutes of pre-existing lock writes onto the new
The boundary itself (the Group B — CR fixes (8 lines) — defensive/edge, not enforcement
DispositionNo security-critical line is untested, so this is not a merge blocker. A small follow-up will add edge-case tests worth pinning ( |
Drift back-merge per the staging plan (19.0 advanced 11 commits since the batch-2 cut: #303, #322, #323, #390, #391, #393, #411, #412, #414, #416, #433). Conflicts were confined to spp_farmer_registry_demo metadata: batch 2's #336 demo fix claimed version 19.0.2.1.2, which 19.0 has since consumed (#412=.2, #322=.3, #323=.4). Resolution re-bumps the batch-2 farmer-demo change to 19.0.2.1.5 with its HISTORY entry moved on top; README.rst/index.html regenerated via the pinned oca-gen hook. The change ships no migration directory, so the collision was metadata-only. The generator code change itself auto-merged cleanly.
Second drift back-merge (#392, #436, #438 landed on 19.0). Conflicts confined to spp_programs metadata: 19.0's #436 took 19.0.2.2.1, which the batch chain had assigned to #336 (with #353 at .2 carrying a migration). Resolution renumbers the batch chain one notch up: manifest -> 19.0.2.2.3, HISTORY ladder 2.2.3 = #353, 2.2.2 = #336, 2.2.1 = #436, and migrations/19.0.2.2.2/ renamed to 19.0.2.2.3/ so the dir matches the version claiming #353's change in the merged changelog (either name runs for real upgrade paths; only test DBs ever saw it as 2.2.2). README.rst/index.html regenerated via the pinned oca-gen hook. program_manager.py auto-merged; batch 2 (lock helpers) and #436 (protected-state filtering) touch disjoint functions.
The is_locked/locked_reason write guard shipped as spp_programs 19.0.2.2.2, not 19.0.2.2.1 — 2.2.1 is the unrelated Enroll Eligible pause fix already on 19.0. The stale reference dates from the version bump spp_programs took when 19.0 claimed 2.2.1.
… request The member wizard and its phone/bank children are transient models whose ACLs grant change-request users read, write, create and delete, and no record rule covered them. Odoo grants transient models no implicit creator-only scoping — ir.rule applies to them as it does to persistent models, and with no rule the domain resolves to true — so any change-request user could enumerate, read, alter or delete another user's proposed-member names, birthdates, phone numbers and bank account numbers. Each wizard model now carries the same parent-change-request ownership rules as the persistent Create-Group detail rows, scoped on every operation its ACL grants, plus the global area filter. The completeness test skipped transient models on the strength of the same false premise; removing that skip is what surfaced the missing area-filter rules.
Comparison required the two derived change sets to be identical. A dynamic-approval type applies only the routed field, so a requester could pad their request with a throwaway edit to another mapped field, make the sets unequal, drop similarity to zero and still have their real change applied unaltered — the evasion cost nothing. Confirmed against a live database: an identical pair scored 100 and was flagged, the same pair plus one decoy scored 0 and was not, and apply wrote only the routed field. Similarity is now scored over the fields both requests propose to change, proportionally, on the 1.0 exact / 0.8 fuzzy scale the static path already uses. Padding falls outside the shared set, so it cannot dilute the score, and a mostly identical request no longer collapses to zero the moment one shared field differs. The change set itself is untouched: still derived from the detail-versus-registrant diff and never from the requester-writable selected_field_name or field_to_modify, which is what keeps a mislabelled request detectable. A test now asserts that independence directly rather than relying on the #343 suite to catch a regression.
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), which lands above the 2.2.2 / 2.2.3 this branch claimed. Left alone the manifest would regress to 2.2.3 and — worse — the #353 role-repointing migration in migrations/19.0.2.2.3 would never run on a database already upgraded to 2.3.0, since Odoo only runs scripts where installed < script <= target. The batch's two spp_programs fixes move above 2.3.0: #336 to 19.0.2.3.1 and #353 to 19.0.2.3.2, with the migration directory renamed to match. 19.0's 2.3.0 entry keeps its number and its code is untouched.
kneckinator
added a commit
that referenced
this pull request
Aug 25, 2026
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), above the 19.0.2.2.4 this branch claimed. Left alone the manifest would regress and the new Tier-3 access-control entries would never be applied to a database already upgraded to 2.3.0. This branch's spp_programs entry moves to 19.0.2.3.3, above the 19.0.2.3.1 / 19.0.2.3.2 that the security batch-2 branch (#422) takes, so the two stack cleanly with #422 merging first. The access-control file conflict is a both-sides append: 19.0's deduplication wizard entries and this branch's Tier-3 registry-read entries are both kept. The two new entries are also renamed from access_spp_cycle_registry_registry_read / access_spp_cycle_membership_registry_registry_read, which doubled a word through a generation slip; nothing references the old identifiers and neither has shipped.
Local README regeneration renders two RST table columns one percent different from CI, and the routine for discarding that difference only reverted the first hunk of a file. In 44a3cd7 the whole diff was that rendering difference spread over several hunks, so one was reverted and the rest committed, leaving spp_change_request_v2's index.html out of sync with what oca-gen-addon-readme produces. Only the column widths are restored; the changelog anchors regenerated alongside them are correct and kept.
The is_locked / locked_reason guard covered write only, so a program officer could create a cycle or program already locked, bypassing it entirely. Clearing the lock afterwards goes through the guarded write, so the creator could not undo it without a system administrator — a self-inflicted lockout, confirmed live on both models. The check moves into _assert_operation_lock_writable and is called from create and write, rather than pasting the condition and its message a third and fourth time. sudo() and system administrators are unaffected, so the async pipeline that creates through sudo keeps working.
field_mapping.apply() returned True even when it had no mapping to write, so the request was stamped applied — with applied_date, an audit event and a log line — having changed nothing. Operators saw a green, applied request whose change had been silently dropped. This happens when the field a dynamic-approval request was routed on loses its mapping, or when the type has none configured, because _effective_mappings fails closed. Applying now raises in that case, so _apply_change_request records apply_error and leaves the request unapplied. A genuine no-op is unaffected and still succeeds: the mappings exist, the registrant simply already holds the proposed values. The fail-closed test from #343 keeps its guarantee — nothing is written for an unmapped selection — and now also expects the rejection.
Odoo stores an unset Char as False, but a JSON-RPC client or integration re-saving a record sends "". The freeze guards normalised only None, so "" did not match the stored False and an idempotent re-save was rejected with the lockout error as though it had altered the approved content. The normalisation also existed twice, verbatim, on spp.change.request and on spp.cr.detail.base — so the gap had to be closed in both places or the two guards would disagree about what counts as a change. It now lives once in models/frozen_value.py, deliberately model-free: change_request is imported before change_request_detail_base, so having either import the other would tie the freeze to model registration order. Clearing a populated frozen field with "" is still rejected; "" reads as unset, not as a licence to blank an approved value.
detail_res_id is frozen once a request leaves draft so a substituted detail cannot be attached after approval, but the guard compared old against new without telling binding apart from re-pointing. The legitimate False -> id transition was refused too, and the guard has no sudo exemption, so a submitted request that never got a detail row could not be opened or repaired from any context — _ensure_detail() performs exactly that transition. Exempting False -> id outright would let an arbitrary detail be attached to an approved request that happens to have none, which is what the freeze exists to prevent. Binding is therefore accepted only for a row that already points back at this request; _ensure_detail() creates the detail with that link before setting the pointer, so the repair path qualifies and a foreign detail id does not.
Conflict and duplicate detection decided whether a mapped field changed using _normalize_field_value, which lowercases and strips strings, while apply compares raw. A case- or whitespace-only edit was therefore invisible to detection yet still written to the registrant, so a field-scoped conflict rule could be sidestepped by a cosmetic edit. Detection also ignored transform expressions, which apply evaluates before comparing. The comparison moves onto the strategy as current_target_value / proposed_target_value / mapping_changes_value, and detection now asks it, so the two cannot drift apart again. Every configured mapping is still considered, not just the routed one: narrowing to selected_field_name would put the change set back under the requester's control. Similarity scoring keeps using _normalize_field_value — being case-insensitive is the point of a fuzzy match. Only the "did this change" derivation had to match apply. Also derives the caller's change set once per duplicate run instead of recomputing it for every candidate; it does not vary by candidate and each derivation re-browses the detail and re-reads every mapping.
spp_change_request_v2 -> 19.0.3.1.10 (detection/apply comparison, apply no-op rejection, detail re-binding, empty-string freeze normalisation, duplicate-run memoisation) and spp_programs -> 19.0.2.3.3 (operation lock on create). One bump per module rather than per fix: the per-fix versions in this batch came from each fix being its own PR, and these land together here. spp_programs takes 2.3.3, so the Tier-3 access-control follow-up moves to 2.3.4.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
semgrep's odoo-sudo-without-context flagged the ownership lookup added for re-binding a detail row. The sudo is deliberate: the caller may hold no access to the detail model, and the result is only ever used to accept or reject the write — the record is never returned or exposed. Annotated in the same way as the other intentional sudo calls in this module, with the reasoning inline.
Contributor
Follow-up PRsRecording the follow-up work so it isn't lost once this merges. Open now
Merge order — Still to openTwo upstream items came out of reviewing this branch. Both are backwards-compatible, so neither blocks this merge, but both should land before the next packaged release. 1. Auto-apply no longer runs the # _on_approve
- self._apply_change_request()
+ self.sudo().action_apply()Worth treating as a priority: it fails silently, and it affects every consumer that extends 2. Dynamic routing keys are assumed 1:1 with mapping source fields. # spp.change.request.type.mapping
routing_field = fields.Char(help="Routing key this mapping serves. Defaults to source_field.")
# _effective_mappings
mappings.filtered(lambda m: (m.routing_field or m.source_field) == selected)Worth pairing with a config-time check that, for Note the fix in this PR that rejects an apply which can write nothing turns case 2 from silent data loss into a visible error. That's the safer failure mode, but affected configurations still need the mapping fix. Already tracked#403 (custom-strategy content freeze — this batch adds another affected type, commented there), #337, #344, #346, #347. |
kneckinator
approved these changes
Aug 26, 2026
kneckinator
added a commit
that referenced
this pull request
Aug 26, 2026
Absorbs the security batch-2 merge (#422), which took spp_programs to 19.0.2.3.3. This branch keeps 19.0.2.3.4, 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.
kneckinator
added a commit
that referenced
this pull request
Aug 26, 2026
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
pushed a commit
that referenced
this pull request
Aug 27, 2026
…odels (#454) * fix(security): let Tier-3 registry read reach the registrant form's models spp_registry.group_registry_viewer (Tier-2) implies group_registry_read (Tier-3), so any model granted only to the viewer tier becomes unreadable for a role scoped to the read tier. Several modules that extend the registrant form grant their models to the viewer tier alone, so such a role gets an AccessError merely opening a registrant. Verified per model against a live database rather than by reading ACL files: spp_consent's nine models, spp_programs' spp.cycle and spp.cycle.membership (the entitlement lists render cycle_id) and spp_irrigation's spp.irrigation.asset are genuinely unreachable. res.partner.bank is not, despite being viewer-only here — Odoo core already grants it to base.group_user — so spp_banking needs no change. spp_programs skips 19.0.2.2.2 and .2.2.3, which the security batch-2 branch claims; this assumes that branch merges first. Also mounts docker/postgresql.conf with :ro,z. Without the SELinux relabel flag the database container cannot read it from a fresh git worktree, which breaks scripts/test_single_module.sh there. * chore: move spp_programs to 19.0.2.3.4 The security batch-2 branch (#422) now also takes 19.0.2.3.3, for guarding the operation lock on create as well as write, so this branch moves up one to stay above it. Still assumes #422 merges first. * chore: restore spp_change_request_v2 README to the generator's output Regenerating the README fragments after merging 19.0 rewrote every module, not just the one this branch touches, and the local renderer lays RST table columns out one character wider than CI does. Only spp_programs was corrected, so an unrelated spp_change_request_v2 diff rode along. Reverted to 19.0's version — this branch does not touch that module. * fix(security): address review — drop the wizard grants, fix the ACL xmlids Two corrections from review. The two consent wizard grants are dropped. Neither wizard is a registrant-form dependency by this branch's own criterion — both Record Consent entry points are gated to the officer and manager tiers, and read without create cannot open a wizard anyway — so the grant buys a Tier-3 user nothing. It is not inert elsewhere: the bulk wizard's action binds to res.partner with no group_ids, and get_bindings filters only on read access to the action's model, so the row would surface "Bulk Record Consent" in the registrant list for read-only users who then fail on click. Odoo also enforces no per-user isolation on transient models, so a model-level read grant exposes other users' in-flight wizard rows. Restoring pre-#353 parity here would have restored a bug. The irrigation ACL identifiers now follow the documented access_{model}_{group} convention: the misnamed pre-existing row becomes ..._registry_viewer, and the new Tier-3 row takes ..._registry_read. The claim that renaming would orphan the old xmlid was wrong — CSV rows load with noupdate false, and _process_end removes exactly those identifiers when a reloaded module no longer declares them — so the rationale is dropped from the changelog rather than restated. Also from review: the spp.cycle.membership reason now names the real dependency (latest_cycle_state, computed by searching that model as the acting user) rather than cycle_id; the consent changelog records why the wizards are excluded; Command.link() replaces (4, id) tuples per the Odoo 19 compatibility guidance; spp_consent/tests/__init__.py is alphabetical again; and ./e2e/reports takes ,z, the last bind mount without it.
This was referenced Aug 27, 2026
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
3 participants
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.
Lands security batch 2 — the change-request + programs chain, seven interdependent fixes validated together on a staging branch so
19.0is never exposed to an unverified intermediate state.⚠ Merge with a MERGE COMMIT — not squash
Squashing collapses seven distinct security fixes into one opaque commit. The merge commit preserves one commit per fix (each carrying its
(#NNN)reference) for bisect and attribution. The repo default is squash — change it at merge time. (Batch 1 / #399 was accidentally squashed; do not repeat.)What's in it
Seven PRs, hard-ordered because the CR module version chain and a migration dir depend on it: #264 → #261 → #338 → #336 → #353 → #343 → #365.
spp_change_request_v2→ 3.1.3spp_change_request_v23.1.4,spp_cr_type_assign_program1.0.2spp_cr_type_assign_program→ 1.0.3spp_programs2.2.2,spp_program_geofence1.0.1,spp_farmer_registry_demo2.1.5spp_change_request_v23.1.5 (+migration),spp_programs2.2.3 (+migration)selected_fieldno longer bypasses CR conflict/duplicate checks (fail-closed)spp_change_request_v2→ 3.1.6spp_change_request_v2→ 3.1.7🔁 Merged
19.0— version chain renumbered19.0shipped #437 asspp_change_request_v219.0.3.1.2 after this branch had already claimed that number for #264. Resolved by merging19.0in — no rebase, the seven commits are preserved — and shifting the batch-2 chain up one, since19.0's number is already on mainline and cannot be reused:#437 keeps
19.0.3.1.2, and its code (strategies/update_id.py,tests/test_update_id_strategy.py) is byte-identical to19.0after the merge. The #353 migration directory moved19.0.3.1.4→19.0.3.1.5(100% rename) to match its manifest; a database at3.1.2upgrading to3.1.7still runs it (installed < script ≤ target).git show --remerge-diffon the merge commit touches exactly__manifest__.py,readme/HISTORY.md, the two generated README files and the migration rename — no security-fix code was modified by the merge, and the delta vs19.0is still confined to the five batch-2 modules.➕ Post-review addition — wizard record rules (
19.0.3.1.8)A
/code-reviewpass over the merged branch found that the three Create-Group member wizards (spp.cr.detail.create_group.member.wizardand its.phone/.bankchildren) areTransientModels whose ACLs grantgroup_cr_userread/write/create and unlink, with noir.rulecovering them. Odoo grants transient models no implicit creator-only scoping —ir_rule._compute_domainhas no transient branch, so a transient model with no rule resolves to a TRUE domain — so any change-request user could enumerate, read, alter or delete another user's proposed-member names, birthdates, phone numbers and bank account numbers.This is the same missing-record-rule class as #261, on models #261's completeness test explicitly skipped: it exempted transient models on the strength of the (stale)
TransientModeldocstring. That skip is removed, which is what then surfaced three additional missing global area-filter rules on the same models.Added: 12 ownership rules (3 models × 4 roles, scoped through
detail_id.change_request_id,perm_unlinkincluded since these ACLs grant it) + 3 global area-filter rules + 7 regression tests.spp_change_request_v2→ 19.0.3.1.8. Full module suite: 366 tests, 0 failures.➕ Post-review addition — duplicate-detection padding (
19.0.3.1.9)_calculate_similarityrequired the two derived change sets to be identical. Since a dynamic-approval type applies only the routed field, a requester could pad their request with a throwaway edit to another mapped field, make the sets unequal, drop similarity to0.0and still have their real change applied unaltered — the evasion was free. Reproduced against a live database: an identical pair scored100.0and was flagged; the same pair plus one decoy scored0.0and was not; apply wrote onlygiven_name.Similarity is now scored over the fields both requests propose to change, proportionally, on the
1.0exact /0.8fuzzy scale the static path already uses. Padding falls outside the shared set so it cannot dilute the score, and a mostly identical request no longer collapses to zero the moment one shared field differs.The derived change set is unchanged — still the detail-versus-registrant diff, never the requester-writable
selected_field_name/field_to_modify, which is what keeps a mislabelled request detectable (#343). A test asserts that independence directly instead of relying on #343's suite to catch a regression.Reachability: gated behind
use_dynamic_approval, which no shipped CR type enables (verified: 0 of 2 types in a default database). It is an admin-editable Boolean on the CR-type form, so any deployment that turned it on is affected.spp_change_request_v2→ 19.0.3.1.9. Full module suite: 372 tests, 0 failures.➕ Post-review additions — findings #6-#13 (
19.0.3.1.10, programs19.0.2.3.3)A
/code-reviewpass over the merged branch raised 14 findings; #3, #4 and #14 were handled above and in follow-ups. The remaining eight were each verified against a live database before being fixed — two turned out to be misdescribed by the review, and one was refuted outright.writeonlycreatetoo, onspp.cycleandspp.program. A program officer could create an already-locked record and then be unable to clear it — confirmed live on both models.apply()reported success having written nothing""vsFalsein the post-submit freeze""for an empty Char, which did not match the storedFalse, so an idempotent re-save was rejected as a content change. The normalisation existed verbatim in two places and now lives once.detail_res_idfreeze blocked_ensure_detail()Two corrections to the review worth recording: the
""/Falsetrigger it described does not exist (_format_value_for_displayreturns"No", not"", for an unset field — the reachable path is an external write payload), and its claim that the apply-time program-access check is weaker than the write-time one has no trigger in the shipped system — a live enumeration found exactly oneir.ruleonspp.programacross 93 installed modules, and it is the company rule.Suites on the merged tree:
spp_change_request_v2395,spp_programs739, zero failures.Review
Every fix was independently adversarially reviewed against its exact vulnerability, Odoo 19 core semantics, and test coverage before merging into staging (
internal/plans/batch2-fix-review.md): six ACCURATE-AND-COMPLETE with follow-ups only. The one CONCERNS finding — #338 rested on a false "module unreleased" premise, leaving a residual on already-deployed DBs — was fixed in-branch (apply-time sink re-check bound to the requester's company scope + preview redaction + corrected rationale) and that delta separately reviewed.Verification (on the merged staging tree)
19.0(bases re-measured against current19.0): CR 3.1.2→3.1.10, programs 2.3.0→2.3.3, assign_program 1.0.1→1.0.3, geofence 1.0.0→1.0.1, farmer_demo 2.1.4→2.1.5. Migration dirs match manifests (CR19.0.3.1.5, programs19.0.2.3.2).19.0released programs2.3.0(deduplication, feat(spp_programs): deduplication — configure by method, recompute on each run, bulk return to draft #445) mid-review, so the batch's programs entries were renumbered above it — left at2.2.xthe manifest would have regressed and the security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu) #353 role migration would never have run on a database already at2.3.0.19.0is confined to exactly the 5 batch-2 modules, no riders (18 CR + 19 programs + 10 assign_program + 4 geofence + 5 farmer_demo files).--match-head-commit <CI'd head>(manifest:internal/plans/batch2-manifest.tsv), green on all module test jobs at that head. Each rebase was verified footprint-clean with its code files byte-identical to the reviewed head — only manifest/HISTORY/generated-README metadata was merged.workflow_dispatchon the staging head is running (linked in checks).check_version_chain.py: run post-merge it reports false "collision" errors, because the seven branches are now stacked (each rebased onto its predecessors) rather than independent off19.0— the tool's pre-merge model. The merged-tree checks above are the authoritative post-merge verification.🔴 Release notes — behavior changes
base.group_systemserver-side; program officers/managers/cycle-approvers can no longer clear an active operation lock via RPC.$applyendpoint now requires the endpoint user to holdgroup_cr_manager. Deployments applying CRs via API must grant that role.After merging
git show --remerge-diff <merge-sha>empty, andgit diff <merge-sha> origin/19.0-staging-sec-batch2empty.ir.rule(spp_farmer_registry_cr/spp_studio_change_requests), security(spp_change_request_v2): enforce manager authorization on CR apply (server-side) #365 API-level authz test + HTTP 403, security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu) #353 load_menus assertion.Plan & manifest:
internal/plans/security-staging-merge-plan.md,internal/plans/batch2-manifest.tsv,internal/plans/batch2-fix-review.md.