security: batch 2 — CR + programs chain (#264, #261, #338, #336, #353, #343, #365) - #422
Open
gonzalesedwin1123 wants to merge 15 commits into
Open
security: batch 2 — CR + programs chain (#264, #261, #338, #336, #353, #343, #365)#422gonzalesedwin1123 wants to merge 15 commits into
gonzalesedwin1123 wants to merge 15 commits into
Conversation
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.
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
2 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.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.9, programs 2.2.1→2.2.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.2.3).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.