security: batch 1 — DCI/Key admin privilege escalations, OAuth signing keys, GRM rule ACL (#327, #329, #265, #266) - #399
Conversation
Bring the staging branch up to date with 19.0 before the batch lands, so the state that is tested is the state that ships. The 12 upstream commits touch no module in this batch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #399 +/- ##
==========================================
+ Coverage 72.66% 75.56% +2.90%
==========================================
Files 329 487 +158
Lines 24298 34195 +9897
==========================================
+ Hits 17655 25840 +8185
- Misses 6643 8355 +1712 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
LGTM — four correct, independently-verified security fixes. CI fully green (every module test job) with 100% patch coverage; migrations idempotent; tests genuinely adversarial (transitive-closure guards, migration replay + idempotency, positive/negative ACL paths). Exemplary security work.
Verified against the code:
- #327 DCI —
implied_ids → base.group_systemremoved (that grant escalated every PII-visibility holder to system admin); inverted sospp_security.group_spp_adminimpliesgroup_dci_admin. Group staysnoupdate; the implication sits outside thenoupdateblock so it re-applies on upgrade; migration strips the stale link + comment on existing DBs. - #329 Key mgmt — same escalation removed, admin now implies Key Operator. Confirmed the crux: every key model (
spp.encryption.key,spp.key.purpose,spp.key.provider.registry,spp.asymmetric.key) has its own explicitgroup_key_adminANDbase.group_systemACL rows, so removing the system implication strips nothing from key admins or real system admins. Menu correctly moved out of Settings. - #265 OAuth — removes the row widening
res.config.settingstobase.group_user, and guardsdefault_get()(readsconfig_parameterviasudo()with no ACL check) to strip the signing keys for non-system, non-su callers; honorsenv.su. Textbook fix. - #266 GRM — portal users dropped to read-only on the two rule models (no record rules → ACL was the only boundary; portal submission runs sudo). The
self.sudo().write()onescalation_countis a narrow, correct consequence, documented with nosemgrep.
Surfacing (not blockers):
- #265 operator action (must hit release notes): the deployed OAuth RSA keypair was readable by every internal user — rotate it and invalidate outstanding tokens; no code change fixes that retroactively.
- #265 → #114 ordering hazard:
OAUTH_KEY_FIELDSnames the current field names; when #114 (rename) rebases it must update those entries AND port the ACL test, or the guard silently points at dead names and reopens the leak with green tests. - Merge mechanics: merge commit (not squash); branch auto-deletes, so run
git show --remerge-diff <merge-sha>(andgit diffvs the branch) before it disappears; tag the release by the merge-commit SHA. - Minor —
spp_dcideps:dci_groups.xmlreferencesspp_security.group_spp_adminbut the manifestdependsis only["base","spp_registry"]; it resolves transitively (CI proves install) — recommend declaringspp_securityexplicitly so it can't break ifspp_registryever drops it. - Note:
encrypted_keyfield access widened togroup_key_admin— fine as KMS-wrapped ciphertext with operators still blocked (tested); rests on the assumption no provider stores plaintext there.
Leaving the merge to you given the merge-commit / tag-by-SHA requirements.
kneckinator
left a comment
kneckinator
left a comment
There was a problem hiding this comment.
Code review of this batch — 13 findings, all inline below.
Two I'd resolve before merge:
spp_oauth/models/res_config_settings.py— thedefault_getoverride is missing@api.model, which breaksres.config.settings.default_getover RPC for every module oncespp_oauthis installed. The new test exercises it in-process, so it can't catch this.spp_dci/security/dci_groups.xml— the replacement group link runs the opposite direction from the one removed, so this grants DCI PII visibility (including disability data) to every system administrator rather than preserving the previous boundary. It also contradicts the key-custody reasoning inspp_key_management/security/security_groups.xml:37in this same PR.
The rest: latent security-posture issues (portal read on the GRM rule models, the encrypted_key field-group widening), a lost-update race on the escalation counter, two test-fixture/coverage gaps, and small cleanups.
| config_parameter="spp_oauth.oauth_pub_key", | ||
| ) | ||
|
|
||
| def default_get(self, fields_list): |
There was a problem hiding this comment.
Blocker — default_get override is missing @api.model.
BaseModel.default_get carries _api = 'model', but _api is not inherited by overrides — call_kw reads it off the most-derived method via getattr(type(model), name). Without the decorator, any RPC call to res.config.settings.default_get falls into _call_kw_multi:
ids, args = args[0], args[1:] -> self.browse(['oauth_priv_key'])
method(recs) -> TypeError: default_get() missing 1 required
positional argument: 'fields_list'
This breaks res.config.settings for every module once spp_oauth is installed, not just this one. Every other default_get override in the repo (spp_change_request_v2, spp_consent, spp_programs, spp_registry, spp_studio, …) carries @api.model; api isn't even imported in this file. The new test calls default_get in-process, so it can't catch this.
| def default_get(self, fields_list): | |
| @api.model | |
| def default_get(self, fields_list): |
(plus from odoo import api, fields, models)
There was a problem hiding this comment.
Fixed in d290e3dd, with a regression test that goes through the real dispatcher
(call_kw) — you're right that the in-process test could never catch this. Confirmed
against core: call_kw reads _api_model off the most-derived method
(odoo/service/model.py:79-87) and the decorator marks only the function it decorates
(odoo/orm/decorators.py:320); get_public_method walks the MRO only for _api_private.
One correction to the blast radius: the Settings UI does not break — since 16.0 the web
client resolves form defaults via onchange, which calls default_get in-process, and
Form tests likewise. What crashed was the external XML-RPC/JSON-RPC surface — bad enough
(it's a documented integration path, and it broke for every module's settings), but "breaks
res.config.settings for every module" overstates it for the UI. The pre-fix test run
reproduced your exact TypeError.
| # who is not a system administrator. Superuser mode (self.env.su) is a | ||
| # trusted server-side context that already bypasses ACLs, so honour it here | ||
| # too — env.user stays the original (possibly non-admin) user under sudo(). | ||
| if not self.env.su and not self.env.user.has_group("base.group_system"): |
There was a problem hiding this comment.
Popping the values out of default_get also silently blanks them on the write path.
create() backfills missing fields from default_get via _add_missing_default_values; execute() then calls set_values(), which does set_param(icp, value or False) for every config_parameter field — and set_param(key, False) deletes the parameter. So a principal that can create res.config.settings but fails has_group('base.group_system') wipes spp_oauth.oauth_priv_key / oauth_pub_key on Save, killing all RS256 issuance and validation, with no error raised.
Today core's ACL limits creation to base.group_system, so this is latent — but the boundary is now exactly one core ACL row that any installed module can re-widen, which is precisely what this module was doing until this PR.
Adding groups="base.group_system" to both key fields would make the write path raise AccessError instead of blanking, and would gate read()/search_read() independently of the model ACL. The pop here is still needed (core's default_get performs no field-group check), so it's both, not either/or.
There was a problem hiding this comment.
Fixed in d290e3dd: groups="base.group_system" on both fields, pop retained — your
"both, not either/or" is exactly right. We verified the whole chain against core
(set_param(False) unlinks the parameter, create() backfills from default_get), plus
one detail that makes it slightly worse than you stated: execute()'s admin gate is
env.is_admin() = base.group_erp_manager (res_config.py:366), not group_system, so an
erp-manager-but-not-system principal fits the blanking profile — and set_values itself is
public with no gate of its own. In Odoo 19 field groups are enforced even on internal
attribute access (Field.__get__, odoo/orm/fields.py:1644-1653), so the save path now
fails closed with AccessError before set_param is reached. New tests: fields_get
filtering (isolates the field gate from the model ACL), fail-closed save with the stored
keys surviving, and an admin round-trip.
| DCI payload visibility automatically, following the extension | ||
| pattern documented in spp_security/security/groups_admin.xml. | ||
| Kept outside the noupdate block so upgrades apply it. --> | ||
| <record id="spp_security.group_spp_admin" model="res.groups"> |
There was a problem hiding this comment.
This is a new privilege grant, not a preservation of the old one — the link runs the other way.
The record being replaced was group_dci_admin.implied_ids = [base.group_system], i.e. DCI admin → system admin. The reverse (system admin → DCI admin) never existed, so a system administrator who was never explicitly granted group_dci_admin could not see the gated fields.
After this change, every base.group_system user — via base.group_system → group_spp_admin in spp_security/security/groups_admin.xml:39 — automatically sees raw DCI payloads, full identifiers and disability data (spp_dci_client_dr/views/disability_status_views.xml, spp_dci_client_crvs, spp_dci_client_ibr, spp_dci_client_sr).
Worth reconciling with spp_key_management/security/security_groups.xml:37 in this same PR, which argues the opposite for key custody: "Deliberately NOT linked into spp_security.group_spp_admin … key custody is intentionally separated from platform administration." The two fixes take contradictory positions on the same question, and the more privacy-sensitive of the two got widened.
There was a problem hiding this comment.
Adopted in 6ba605d2: the implication is removed, PII display is opt-in per administrator,
and the pinning test is inverted to guard against a consistency sweep reintroducing the
link. But the record needs correcting, because the finding's security framing is what made
it a "blocker" and it doesn't hold: group_dci_admin is view-XML-only — a full-repo
enumeration finds zero ir.model.access rows, zero ir.rules, zero Python field-level
groups= using it, and Odoo 19 view groups= is arch-stripping only
(ir_ui_view.py:1317-1336). The models it gates carry their own ACLs: the disability and
CRVS models are read/write/create for every base.group_user internal user, the IBR/SR
models readable by registry viewers, and system admins hold export rights. So the link
granted system admins zero data access they didn't already have over RPC — it changed
what renders on screen. We removed it anyway, on grounds that do hold: consistency with the
deliberate key-custody separation in this same PR, an explicit reviewable grant instead of
an automatic side effect of adminship, and timing (2.0.2 is unreleased — after release this
becomes a behavior change requiring a migration). The broader issue your finding points at
— those models' own ACLs being wide — predates this PR and is real; that's follow-up
territory, not a property of this change.
| "summary": "Core DCI (Digital Convergence Initiative) API components", | ||
| "category": "OpenSPP/Integration", | ||
| "version": "19.0.2.0.1", | ||
| "version": "19.0.2.0.2", |
There was a problem hiding this comment.
spp_security should be added to depends (the actual line is the depends list just below — GitHub won't let me anchor outside the hunk).
spp_dci/security/dci_groups.xml now references spp_security.group_spp_admin, but depends is ["base", "spp_registry"] — the xml_id only resolves transitively. Of the 27 modules in this repo that extend spp_security.group_spp_admin in a security XML, spp_dci is the only one that doesn't declare spp_security directly (spp_alerts, spp_area, spp_grm, spp_audit, spp_gis all do).
Load order and xml_id availability are only guaranteed through the declared dependency graph. If spp_registry ever drops spp_security, a fresh install raises ValueError: External ID not found in the system: spp_security.group_spp_admin.
There was a problem hiding this comment.
Done in 6ba605d2 — with the XML reference gone (F3), the security tests still pin design
decisions against spp_security.group_spp_admin, so the dependency is now declared
explicitly rather than left transitive. Two corrections for the record: 33 modules
extend the group (not 27), and spp_dci was not the only transitive-only one —
spp_disability_registry, spp_gis_report, spp_registry_search, and
spp_farmer_registry_demo share the gap. Those four are follow-up material, not this
batch.
| # and GCP KMS providers read the cached wrapped key in the calling | ||
| # user's context. The value is ciphertext (wrapped by the KMS); | ||
| # plaintext key material is never stored here. | ||
| groups="base.group_system,spp_key_management.group_key_admin", |
There was a problem hiding this comment.
The justifying comment isn't true for the default provider, and the real cause is a missing .sudo() in the KMS providers.
The comment says "The value is ciphertext (wrapped by the KMS)". But the model docstring (line 5) says the keys are wrapped with a KEK from configuration, and key_provider_database.py:_get_master_key falls back to sha256(_DERIVED_KEY_SALT + database.uuid) when neither SPP_MASTER_KEY nor spp_master_key is set — the zero-config default. For that provider the stored value is a DEK wrapped by a locally derivable KEK, not KMS ciphertext.
Meanwhile key_provider_database.py:181 already reads the record via self.env["spp.encryption.key"].sudo(), whereas key_provider_aws_kms.py:152,215,290, key_provider_gcp_kms.py:215,268,363 and key_provider_azure_keyvault.py:201,324 use a plain self.env[...]. That inconsistency, not the field gate, is what's actually failing — .sudo() in the KMS providers is the narrower fix.
Also note groups= gates writes as well as reads, so this hands Key Management Admins (now explicitly non-system users) write access to wrapped DEK material, where they already hold ACL write (security/ir.model.access.csv:2).
There was a problem hiding this comment.
The comment part is right and fixed in 74a92ef9: the database provider's KEK falls back
to a database.uuid-derived value in zero-config setups, so "wrapped by the KMS" was wrong
for that provider; the comment now states both families. The sudo rework we decline for
this PR, because "that inconsistency … is what's actually failing" doesn't hold: a complete
call-graph sweep shows no non-privileged runtime path reaches encrypted_key —
spp.key.manager._check_key_access (key_manager.py:57-90) gates every entry point before
any provider runs, and the one ordinary-user flow (AV-scan quarantine) is fully sudo'd. The
only role the missing provider sudo affects — Key Operator on KMS backends — was equally
broken before this PR (model-ACL write block plus the old system-only field gate) and is
already tracked in #330/#331. The write-access "delta" is nil: key admins already hold
model write ACL (ir.model.access.csv:2), rotation legitimately rewrites the field, and
unlink is blocked for everyone. Narrowing the gate back to system-only would also break
the shipped tests pinning key-admin field access and hide the field from the custody role's
own form view.
| # Update escalation count | ||
| self.write({"escalation_count": self.escalation_count + 1}) | ||
| # nosemgrep: semgrep.odoo-sudo-without-context -- counter update needs sudo | ||
| self.sudo().write({"escalation_count": self.escalation_count + 1}) |
There was a problem hiding this comment.
Non-atomic read-modify-write — escalation counts can be lost.
Two workers escalating different tickets against the same rule (the hourly check_escalations cron at line 435 iterating all open tickets, concurrent with a stage-change write from the UI) both read escalation_count == N under READ COMMITTED; the second UPDATE blocks, then overwrites with N+1. One escalation is never counted.
A single UPDATE ... SET escalation_count = escalation_count + 1 is the atomic form.
There was a problem hiding this comment.
The race is real and your atomic form is the right fix — but it's pre-existing (this PR
only added sudo() around the existing expression), the counter is display-only (views and
admin tests are its only readers), and the identical race exists at
grm_routing_rule.py:263 on match_count, which the finding doesn't cover. Fixing one
counter and not its twin would be asymmetric, so both go to a follow-up together, with
flush_recordset + a literal parameterized UPDATE + invalidate_recordset, and a note
that raw SQL is invisible to spp_audit's ORM hooks (acceptable for a stats counter, worth
stating). Not a merge condition for this batch.
|
|
||
| # Update escalation count | ||
| self.write({"escalation_count": self.escalation_count + 1}) | ||
| # nosemgrep: semgrep.odoo-sudo-without-context -- counter update needs sudo |
There was a problem hiding this comment.
The half-applied state the changelog promises to prevent is still reachable, and there's no test for this fix.
The changelog says a caller "cannot leave an escalation applied half-way (notification sent and case created, counter and chatter missing)". But _send_escalation_notification (line 362) and _create_case_from_ticket (line 385) each wrap their work in a bare except Exception that only logs. A caller lacking create rights on spp.case still gets ticket.write(vals) applied, escalation_rule_ids linked, the counter incremented and the chatter post made — with no case. Same half-applied state, different missing piece.
Coverage: the only test file added to spp_grm_cel is tests/test_rule_acl.py, which exercises ACL rows and never calls apply_escalation. Revert line 349 to self.write(...) and the whole suite still passes. Worth a test that a GRM officer without write on spp.grm.escalation.rule can escalate and have the counter increment.
There was a problem hiding this comment.
You're right on the two load-bearing points: no test called apply_escalation as a
non-superuser (reverting the sudo passed the whole suite), and the changelog sentence
invited a broader reading than the fix delivers. Both fixed in 37020f52. One correction
to the proposed test: "a GRM officer without write on spp.grm.escalation.rule" cannot be
constructed — officers hold 1,1,1,0 on both rule models. The population the sudo actually
serves is portal users (ticket write via spp_grm, rule read-only after this PR), so the
regression test uses a portal actor; with the sudo reverted it now fails loudly. The
notification/case except Exception blocks are pre-existing (verified with git log -L —
they shipped with the module), so tightening them is follow-up material, not a condition on
a security batch that didn't touch them.
| access_spp_grm_routing_rule_manager,GRM Routing Rule Manager Access,model_spp_grm_routing_rule,spp_grm.group_grm_manager,1,1,1,1 | ||
| access_spp_grm_routing_rule_base_user,GRM Routing Rule Base User Access,model_spp_grm_routing_rule,base.group_user,1,0,0,0 | ||
| access_spp_grm_routing_rule_portal_user,GRM Routing Rule Portal User Access,model_spp_grm_routing_rule,base.group_portal,1,1,1,0 | ||
| access_spp_grm_routing_rule_portal_user,GRM Routing Rule Portal User Access,model_spp_grm_routing_rule,base.group_portal,1,0,0,0 |
There was a problem hiding this comment.
Portal read on the rule models looks unnecessary, and the new test pins it as a requirement.
Rows 6 and 11 keep base.group_portal at 1,0,0,0. The PR's own rationale (README.rst:132, test_rule_acl.py docstring) says portal grievance submission runs sudo — and spp_grm_cel/models/grm_ticket.py:_apply_routing_rules is the only portal-reachable entry point — so this grant isn't load-bearing for portal ticket creation.
These models carry no ir.rule records, so the read is global. Any authenticated portal user can search_read('spp.grm.routing.rule') / ('spp.grm.escalation.rule') over RPC and enumerate every CEL condition, assign_user_id, assign_team_id, escalate_to_user_id, escalate_to_team_id, severity/priority override and time threshold — a complete map of how to phrase a grievance to route to a chosen officer or trigger escalation.
test_portal_user_can_read_rules (line 71) asserts this access exists, so tightening it later means deleting a test that reads as a security requirement.
There was a problem hiding this comment.
The premise doesn't hold: the sudo'd controller is not the only portal-reachable entry
point. Portal holds 1,1,1,0 on spp.grm.ticket itself (spp_grm/security/ ir.model.access.csv:6), so direct-RPC ticket creates and stage-writes evaluate rules as
the portal user — drop the read row and those tickets go silently unrouted (the
AccessError is swallowed in _apply_routing_rules/_check_escalation_rules) while
controller tickets still route. That inconsistent, hard-to-debug state is why read was
deliberately retained. That said, your enumeration concern is real and actually worse
than stated: Odoo 19's Many2one.convert_to_read returns display names via sudo()
(orm/fields_relational.py:366-373), so portal users get officer/team names despite the
core res.users portal record rule. And the verification surfaced the underlying hole your
finding circles: there is no portal record rule on spp.grm.ticket at all — any portal
user can read and rewrite every grievance in the system over RPC, which is strictly worse
than reading the rules. All of it is now tracked together in #413 (own-tickets
ir.rule, sudo rule evaluation, dropping both the portal and base.group_user read rows,
and replacing the test you flagged with a read-denial test). In this PR (37020f52) the
test docstring is reworded so the grant reads as a current implementation dependency, not a
security requirement — your concern about it pinning the wrong invariant was fair.
There was a problem hiding this comment.
Correction to the tracking reference above: the consolidated issue for this cluster is #380 (filed earlier by the #266 staff review) — #413 was closed as its duplicate, with the unique evidence ported to #380. Also note #379 identifies elevated rule evaluation as the core vulnerability, so the evaluation rework will follow the owner-identity pattern rather than the blanket sudo() suggested above; the read-row drops land with that cluster either way.
| { | ||
| "name": "GRM Manager", | ||
| "login": "grm_manager_acl_test", | ||
| "group_ids": [Command.link(cls.env.ref("spp_grm.group_grm_manager").id)], |
There was a problem hiding this comment.
This fixture creates a share=True non-internal user, so the "GRM staff retain management" test doesn't prove what it's meant to.
spp_grm/security/groups.xml: group_grm_manager → group_grm_officer → group_grm_viewer + group_grm_write → group_grm_read — none of these link base.group_user. res.users._check_one_user_type only rejects more than one type group, so a user with zero is created fine, with share = True.
test_grm_manager_can_create_rules therefore proves a shared/external principal can create rules, and wouldn't catch a regression where the staff path depends on internal-user status (a global ir.rule gated on base.group_user, or a future perm_create row keyed to internal users).
The sibling tests in this same PR do it right — spp_oauth/tests/test_config_settings_acl.py:38 and spp_key_management/tests/test_key_admin_group.py:561 both link base.group_user alongside the role group.
There was a problem hiding this comment.
Fixed in 37020f52 — base.group_user linked in the fixture, with a comment explaining
why. Substance fully confirmed against core (share compute at res_users.py:460-464; in
19 the constraint is _check_disjoint_groups, which indeed only rejects more than one
user-type group). For the record, two of the supporting citations were broken: the
spp_oauth line is off by one, and test_key_admin_group.py is 162 lines long (there is no
:561) — it links base.group_user at :47/:59/:76/:83/:122, so the pattern claim holds.
| @@ -1,2 +1 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
There was a problem hiding this comment.
This file is now header-only but is still loaded from the manifest.
With access_res_config_settings_spp_oauth_user gone the file holds nothing but the column header, yet spp_oauth/__manifest__.py:18 still lists it in data. spp_oauth declares no models of its own (only an inherit of res.config.settings), so there are no future rows for it to hold. Odoo tolerates the empty load, but the file reads as "this module has ACLs" to anyone auditing it — the opposite of the posture this PR is establishing. Suggest deleting the file and dropping the manifest entry.
There was a problem hiding this comment.
Declined. The facts are right — header-only file, no models of its own, and Odoo loads it
cleanly (convert_csv_import handles a data-less CSV without complaint). But deleting it
is wrong for this repo: the openspp-check-acl pre-commit hook flags every installable
module lacking security/ir.model.access.csv (scripts/lint/check_acl.py:113-131, no
model-presence exemption), docs/principles/access-rights.md mandates the file, and 30
modules already keep header-only csvs — it's the established convention here (every
spp_api_v2_* module among them). Deleting would trade zero functional benefit for a
standing lint warning and a convention break; the empty file also keeps the module's ACL
posture greppable in the standard location.
… keys (#399) Review response on PR #399 (findings 1-2): - default_get() override lacked @api.model. call_kw reads the dispatch marker off the most-derived method, so every external RPC call to res.config.settings.default_get crashed with a TypeError once this module was installed (web Settings UI unaffected - it resolves defaults in-process). Regression-tested through the real dispatcher. - oauth_priv_key/oauth_pub_key now carry groups="base.group_system": a settings save by an unauthorized principal fails closed with AccessError instead of silently deleting the stored parameters via set_param(False). The default_get pop stays - field groups are not enforced in default_get. Defence-in-depth: core's ACL already limits the model to system admins; the gate holds if that is ever re-widened.
…ion (#399) Review response on PR #399 (findings 3, 7, 8): - Drop the spp_security.group_spp_admin -> group_dci_admin implication: PII rendering is now opt-in per administrator via an explicit, reviewable Access Rights grant instead of an automatic side effect of adminship, mirroring the deliberate key-custody separation in spp_key_management. The pinning test is inverted to guard against a consistency sweep reintroducing the link. The link never shipped in any release and no production DB is built from this staging branch, so no unlink migration is needed; the upgrade-test seed DB is rebuilt for the refreshed evidence run. - Migration comment refresh now strips only the stale sentence ("Members must already be system administrators") so operator rewrites and extensions survive, honouring noupdate. - Migration warning counts all_user_ids (transitive membership), matching the spp_key_management migration's counting basis - safe now that nothing implies the group. - Declare the spp_security dependency explicitly: the security tests pin design decisions against spp_security.group_spp_admin, and the dependency was previously only transitive via spp_registry.
…icon (#399) Review response on PR #399 (findings 5, 6): - The encrypted_key field comment claimed the value is always KMS ciphertext; for the database provider it is wrapped by the master KEK, which in zero-config setups is derived from database.uuid. State both provider families accurately. The field gate itself is unchanged (the KMS-provider sudo rework is tracked in #330/#331). - The top-level Key Management menu now carries the module icon - every other top-level app menu sets web_icon; without it the app switcher shows a generic placeholder tile. Pinned in the existing load_menus test via web_icon_data.
…ssion (#399) Review response on PR #399 (findings 10, 11, 12): - The GRM manager fixture now links base.group_user: the spp_grm group chain carries no user-type group, so the manager was created as a share=True external principal and the staff-retention tests proved less than intended. - New regression test: a caller with read-only rule access gets a fully applied escalation with the counter incremented. The actor is a portal user - the population the sudo'd counter write serves in practice (officers/managers hold rule write; internal base users and GRM viewers are read-only too but do not drive ticket flow). Reverting the 19.0.2.0.1 sudo fix now fails loudly instead of passing the suite. - Portal-read docstrings reworded: the read row is a current implementation dependency (rule evaluation runs as the acting user, reachable by portal via direct-RPC ticket create/stage-write), not a security requirement. Tightening it - sudo evaluation, dropping the read rows, and the missing portal record rule on spp.grm.ticket - is tracked in #413. - HISTORY: scope the half-way claim to the counter/chatter state this fix addresses; notification/case partial failures are pre-existing.
Applied verbatim from the pre-commit CI run's printed diff (run 31679116024) - local regeneration is not byte-reproducible against CI's hook env, so CI output is the sole authority for generated files.
#399) With the group_spp_admin record removed, the noupdate <data> wrapper became the file's sole element, which oca-checks-odoo-module flags as xml-deprecated-data-node (the failing 'Checks for Odoo modules' CI hook). Same noupdate semantics, modern form.
|
Thanks for the thorough review — 13 findings, every one worth verifying. We verified each |
3 participants
Lands security batch 1 — four independent security fixes, validated together on a staging branch so
19.0was never exposed to an unverified intermediate state.What's in it
DCI Administratorno longer grants system administration (privilege escalation)spp_dci19.0.2.0.1 → 2.0.2 + migrationKey Management Adminno longer grants system administration (privilege escalation)spp_key_management19.0.2.0.0 → 2.0.1 + migrationspp_oauth19.0.2.0.0 → 2.0.1spp_grm_cel19.0.2.0.0 → 2.0.133 files as of the batch merge (
07a0ad3a), plus the four review-response commits below. Every fix was individually adversarially reviewed, and #265/#266 were re-reviewed after rebasing because upstream had moved substantially under them.Review response (2026-08-13)
The 13-finding inline review was independently verified claim-by-claim against Odoo 19 core source; the full analysis (verdicts, corrected severities, declines with evidence) lives in
internal/plans/pr399-kneckinator-review-analysis.md. Four addendum commits land the accepted findings — one per module, each conventions-reviewed AND adversarially reviewed before push, all four module suites green at the final state:d290e3ddspp_oauth@api.modelondefault_get(external RPCdefault_getcrashed for every module's settings — genuine blocker, confirmed againstcall_kw), field-gate the signing keys (groups="base.group_system"), 4 new tests incl. a real-dispatcher regression test6ba605d2spp_dcigroup_spp_admin → group_dci_adminimplication is removed — PII display is now opt-in per administrator via an explicit, reviewable grant (design decision revisited and adopted; note the group is view-gating only, so this changes UI rendering, not data access). Migration comment-refresh narrowed to strip only the stale sentence; warning countsall_user_ids;spp_securitydependency declared74a92ef9spp_key_managementencrypted_keywrap comment for both provider families;web_iconon the top-level menu + test assertion37020f52spp_grm_cel8fe91af66528c305spp_dci<odoo noupdate="1">instead of a lone deprecated<data>wrapper (theoca-checks-odoo-moduleCI failure; identical noupdate semantics, record unchanged — the-uevidence at8fe91af6is unaffected)Declined, with rationale in the analysis doc: deleting the header-only
spp_oauthACL csv (repo convention — 30 modules keep one; our own ACL lint requires it); the KMS-providersudo()rework (no non-privileged path reaches the field; pre-existing operator gap already tracked in #330/#331); dropping the portal read row on the rule models now (load-bearing for direct-RPC rule evaluation — tightening is bundled into #413); the counter-race fix in this PR (pre-existing, and the identical race exists onmatch_count— follow-up).Rediscovered by the verification:
spp_grmgrants portal users read/write on ALL grievance tickets with no portal record rule (HIGH; pre-existing on19.0, out of this batch's scope, and the root cause that makes the rule-read row load-bearing). Tracked in #380 (from the #266 staff review; the verification's #413 was closed as its duplicate with the unique evidence — sudo display-name leak, load-bearing read rows — ported there). Fix direction follows #379's owner-identity pattern, with #381 in the same cluster.Branch-built DBs: the removed implication loaded on any DB built from this branch between
ec4493ddand07a0ad3a. No deployment runs from this staging branch; the only such DB was the upgrade-test DB, which is rebuilt for the refreshed evidence run below. Production upgrade paths (19.0 → this head) never load the link, so no unlink migration is needed.⚠ Merge with a MERGE COMMIT — not squash
Squashing would collapse four distinct security fixes into one opaque commit. The merge commit preserves one commit per fix (each carrying its
(#NNN)reference) for bisect and attribution.Verification
Each PR was squash-merged into staging pinned with
--match-head-commit <reviewed sha>, so what merged is provably what was reviewed. Manifest:internal/plans/batch1-manifest.tsv.07a0ad3athe delta vs19.0was exactly 33 files, precisely the union of the 4 commits; the review-response commits extend that union only within the same four modulesci.ymldispatch on the pre-addendum staging head: green on all four module test jobs (run 31352167158). CI on the addendum head: pending (see checks on this PR); README regen diffs from CI's pinned generator will be applied if flagged19.0advanced 12 commits after the batch merged;19.0was back-merged into staging (not rebased, which would have destroyed the audit trail above).git show --remerge-diffon the merge is empty — purely mechanical, nothing smuggled in. The drift touched none of the four modules. Second back-mergef5f2df73(2026-08-13):19.0advanced 15 more commits (DRIMS review flow, hide-menus fix(spp_hide_menus_base): a duplicate menu_id row must not abort the registry load #409, PyPDF2 docker fix — files confined todocker/,spp_drims,spp_hide_menus_base,spp_base_common; zero overlap with the batch). Verified before push: remerge-diff empty; the four modules byte-identical to the reviewed state; merged tree vs19.0differs in exactly the batch's files. The post-merge audit (git diff <merge-sha>vs the branch empty) is valid again.Upgrade test (
-u) — the gate CI does not provideCI only ever runs
-i(fresh install), so the upgrade path is untested by the pipeline. Re-run 2026-08-13 against the addendum head on a rebuilt seed DB (the old test DB had loaded the since-removed implication): seeded at current19.0(64e6b31c) → staging (8fe91af6); exit 0, zero errors. Evidence:internal/plans/batch1-upgrade-test-evidence.md.Baseline re-confirmed all four vulnerabilities live on current
19.0, then after upgrade:ir_module_module.latest_versionmatches manifestsgroup_dci_admin/group_key_adminimplybase.group_systemspp_oauthwideningir.model.accessrowr,w,cspp_admin → dci_adminimplication (addendum)Both migrations fired with their audit warnings; the Key Management menu icon applied on upgrade.
🔴 Release notes — required
base.group_systemto whoever administers the DCI server (tokens, sender registries, subscriptions, server keys, settings) — every one of those surfaces is system-gated. And the reverse: no group implies "DCI Administrator" — any administrator who needs raw DCI payloads/PII rendered on screen must be granted the group explicitly.base.group_system.Both migrations log warnings, but upgrade-log warnings are easily missed — the notes are the real channel.
Ordering constraint that survives this merge
#265 must land before #114. #114 renames the exact fields #265's
default_getguard pops (oauth_priv_key/oauth_pub_key→oauth_private_key/oauth_public_key). Merging this PR satisfies the ordering, but #114's rebase must rename theOAUTH_KEY_FIELDSentries AND porttests/test_config_settings_acl.py— a naive conflict resolution leaves the guard pointing at dead field names and silently reopens the signing-key leak with green-looking code. The addendum addsgroups=to those same fields — #114's rebase must carry that attribute over to the renamed fields too.After merging
git show --remerge-diff <merge-sha>should be empty, andgit diff <merge-sha> origin/19.0-staging-sec-batch1should be empty — check before the branch auto-deletes (delete_branch_on_mergeis on)19.0with its full fix set recorded on the PRPlan and checklist:
internal/plans/security-staging-merge-plan.md.