security(dci): purge stale compliance bearer token on upgrade by gonzalesedwin1123 · Pull Request #326 · OpenSPP/OpenSPP2

gonzalesedwin1123 · 2026-07-23T03:45:59Z

Summary

Fixes an upgrade-path exposure in spp_dci_client_compliance: the 19.0.1.0.1
hardening was meant to fail closed and retire the old shared compliance bearer
token, but it only deleted the ir.config_parameter copy. Earlier versions
(19.0.1.0.0) also created an spp.dci.data.source record with
auth_type='bearer' and bearer_token='compliance-test-api-key-12345'. That
record survives the upgrade (a runtime-created one has no xml_id, so Odoo's
orphan cleanup never removes it), and _get_test_data_source() returned an
existing is_compliance_test record before ever reaching the fail-closed
_get_compliance_bearer_token(). Because the trigger routes are auth='none',
an unauthenticated network caller could drive DCI client operations
(search/subscribe/unsubscribe/txn_status) through the retained known credential
on upgraded deployments once the compliance gate is enabled.

Changes (final)

Migration (migrations/19.0.1.0.2/post-migration.py + _purge_default_compliance_bearer_token):
deletes any spp.dci.data.source still holding the well-known default token,
including archived (active=False) records (searched with
active_test=False). A new version + folder ensures it runs for both affected
populations — DBs upgrading from 19.0.1.0.0 and DBs already at 19.0.1.0.1.
Records an operator re-keyed with a real token are matched on the token value
alone, so they are left untouched.
Read path (_get_test_data_source): excludes the default token directly
in the search domain (("bearer_token", "!=", DEFAULT)), so a retained
record is never served even if the migration is skipped — and a legitimately
re-keyed record is still found when a stale record would otherwise sort ahead
of it under limit=1.
Create path (_get_compliance_bearer_token): rejects the well-known
default token when it is configured, so the create path cannot mint a fresh
data source carrying the public secret through the front door.
Docs: readme/DESCRIPTION.md no longer instructs operators to use the
default token; it now requires a private token and documents that the
default is rejected.
Version bump 19.0.1.0.1 → 19.0.1.0.2 + HISTORY.md fragment; README.rst
/ index.html regenerated by CI's pinned generator.

Design decisions (confirmed with owner)

Delete vs. neutralize: delete the retained record (scoped strictly to the
known public token) so the intended fail-closed create path governs.
Defense in depth: migration (cleanup) + read-path exclusion + create-path
rejection — guard every path to the sink, per the repo pitfall.

Post-review hardening

Two adversarial staff-engineer reviews ran on this branch. Beyond the original
migration + guard, they surfaced three gaps, all fixed here:

The create path accepted the default token (and the docs told operators to
set it) → create-path rejection + doc fix.
The purge skipped archived records (default active_test=True), leaving
the secret at rest and usable by non-controller consumers → purge with
active_test=False.
The original post-limit=1 filter could mask a valid re-keyed record when a
stale record sorted ahead → moved the exclusion into the search domain
(replacing the earlier _reject_default_token post-filter).

Verified SOLID by the reviews: migration version mechanics, superuser
field-group visibility, unlink() FK safety (only set-null refs),
nosemgrep suppressions, exact-match rationale, tests fail without the fix.

Testing

./spp t spp_dci_client_compliance → 39 passed, 0 failed, 0 errors
(33 original + 6 review-driven), confirmed locally and by CI's own
test (spp_dci_client_compliance) job.
Live upgrade-path test (×2): seeded a 19.0.1.0.0 state (active stale +
archived stale + re-keyed control), rolled the installed version back, ran
odoo -u spp_dci_client_compliance. Log: both migrations execute, then
Removed 2 DCI compliance data source(s) that still held the default bearer token. Post-upgrade assertions: default-token records = 0 (active and
archived), re-keyed record untouched, module at 19.0.1.0.2.

Notes for reviewers

README generated files are applied from CI's pinned oca-gen verbatim (local
regen is not byte-reproducible per repo guidance).
Out of scope, follow-up filed as security(spp_dci_compliance): _post_init_hook presets the well-known token as an accepted server API token [Severity: Medium] #328 (Severity: Medium): the server-side
spp_dci_compliance/__init__.py _post_init_hook presets the same secret as
an accepted inbound dci.api_tokens value and disables signature checks —
a separate module and exposure.

Draft: awaiting human security review before undraft/merge.

Earlier versions of spp_dci_client_compliance created an spp.dci.data.source record carrying the well-known token 'compliance-test-api-key-12345'. The 19.0.1.0.1 post-migration removed only the ir.config_parameter copy, so on upgraded databases the retained data source kept working with the known token once the compliance gate was enabled - and since the trigger routes are auth='none', an unauthenticated caller could drive DCI client operations through it. - Add a 19.0.1.0.2 post-migration that deletes any compliance data source still holding the default token (records re-keyed with a real token are left untouched), via a new _purge_default_compliance_bearer_token() helper. - Harden _get_test_data_source() to refuse serving a data source that still holds the default token, so a skipped migration cannot re-expose it. - Add regression tests for the purge helper and the controller guard. - Bump version to 19.0.1.0.2 and add HISTORY fragment.

gemini-code-assist · 2026-07-23T03:46:02Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

codecov · 2026-07-23T03:50:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.36%. Comparing base (1caf794) to head (31b6252).
⚠️ Report is 3 commits behind head on 19.0.

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #326      +/-   ##
==========================================
- Coverage   74.28%   68.36%   -5.92%     
==========================================
  Files         372       99     -273     
  Lines       25385     8462   -16923     
==========================================
- Hits        18857     5785   -13072     
+ Misses       6528     2677    -3851

Flag	Coverage Δ
spp_analytics	`?`
spp_api_v2_cycles	`?`
spp_api_v2_entitlements	`?`
spp_api_v2_gis	`?`
spp_api_v2_programs	`?`
spp_api_v2_simulation	`?`
spp_audit_programs	`?`
spp_base_common	`91.07% <ø> (+0.80%)`	⬆️
spp_case_demo	`?`
spp_case_entitlements	`?`
spp_case_programs	`?`
spp_cr_type_assign_program	`?`
spp_dci_client_compliance	`100.00% <100.00%> (?)`
spp_dci_compliance	`?`
spp_dci_demo	`?`
spp_dci_server_social	`?`
spp_demo	`?`
spp_demo_phl_luzon	`?`
spp_drims	`?`
spp_drims_sl	`?`
spp_drims_sl_demo	`?`
spp_farmer_registry_demo	`?`
spp_gis_report	`?`
spp_grm_demo	`?`
spp_indicator	`?`
spp_indicator_studio	`?`
spp_metric_service	`?`
spp_mis_demo_v2	`?`
spp_programs	`65.27% <ø> (+<0.01%)`	⬆️
spp_registry	`86.94% <ø> (+0.10%)`	⬆️
spp_security	`69.56% <ø> (+2.89%)`	⬆️
spp_simulation	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_dci_client_compliance/controllers/trigger.py	`100.00% <100.00%> (ø)`
spp_dci_client_compliance/models/data_source.py	`100.00% <100.00%> (ø)`

... and 281 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ngelog Applies CI's pinned oca-gen output verbatim for the new HISTORY fragment.

…uard An empty recordset already makes the warning loop, len(), and unlink() no-ops, so the explicit 'if not stale: return 0' early return is dead weight.

gonzalesedwin1123 · 2026-07-23T04:31:02Z

Filed the server-side follow-up as #328: spp_dci_compliance/__init__.py _post_init_hook hardcodes the same well-known secret as an accepted inbound dci.api_tokens value (and relaxes signature/callback checks + grants the public user registry-viewer). Separate module and a distinct exposure from this client-side fix, so kept out of this PR's scope.

gonzalesedwin1123 · 2026-07-23T06:59:20Z

Live upgrade-path test: PASS (beyond the unit suite, this verifies the migration wiring — folder discovery, migrate() execution, version gating — on a real database).

Procedure: fresh-installed the module at 19.0.1.0.2 into a dedicated DB, seeded the pre-fix state (data source with bearer_token='compliance-test-api-key-12345' + a control record re-keyed to a real token), rolled ir_module_module.latest_version back to 19.0.1.0.0, then ran odoo -u spp_dci_client_compliance.

Result:

Running upgrade [19.0.1.0.1>] post-migration
Running upgrade [19.0.1.0.2>] post-migration
WARNING ... Removing DCI compliance data source 'dci_compliance_stale' that still held the default bearer token.
WARNING ... Removed 1 DCI compliance data source(s) ... Set 'dci.client_compliance.bearer_token' before re-enabling the trigger endpoints.

Post-upgrade assertions: default-token records = 0; re-keyed control record untouched (operator-real-token); module at 19.0.1.0.2.

…create path Staff-engineer review found the search-path guard left the create path open: _get_compliance_bearer_token only checked the token was non-empty, so an operator following the module docs (which suggested the public default) would mint a fresh data source carrying it - recreating the exposure the migration purges. Reject the default token there, fix the misleading DESCRIPTION.md instruction, and add create-path + token-only-match regression tests.

…ord masking Second review pass found two more gaps: - The purge searched with default active_test=True, so an ARCHIVED data source still holding the default token survived the migration (secret at rest, still usable by non-controller consumers via get_headers). Purge now runs with active_test=False. - The controller filtered the default token AFTER a limit=1 search, so a stale record sorting ahead of a legitimately re-keyed one (same name/flag) masked the valid record and forced a needless create. Exclude the default token in the search domain instead, replacing the post-filter helper. Adds regression tests for the archived purge and the stale+rekeyed coexistence.

…n guidance Applies CI's pinned oca-gen output verbatim for the updated DESCRIPTION/HISTORY fragments (private-token requirement + create-path rejection).

…xclusion change The record is now dropped by the default-token exclusion in the search domain, not found-then-rejected by a post-filter.

gonzalesedwin1123 added 2 commits July 23, 2026 11:58

docs(spp_dci_client_compliance): regenerate README for 19.0.1.0.2 cha…

e4b8eb9

…ngelog Applies CI's pinned oca-gen output verbatim for the new HISTORY fragment.

refactor(spp_dci_client_compliance): drop redundant empty-recordset g…

c6418b0

…uard An empty recordset already makes the warning loop, len(), and unlink() no-ops, so the explicit 'if not stale: return 0' early return is dead weight.

gonzalesedwin1123 mentioned this pull request Jul 23, 2026

security(spp_dci_compliance): _post_init_hook presets the well-known token as an accepted server API token [Severity: Medium] #328

Open

gonzalesedwin1123 added 4 commits July 23, 2026 15:38

docs(spp_dci_client_compliance): regenerate README for corrected toke…

674263f

…n guidance Applies CI's pinned oca-gen output verbatim for the updated DESCRIPTION/HISTORY fragments (private-token requirement + create-path rejection).

test(spp_dci_client_compliance): correct stale comment after domain-e…

31b6252

…xclusion change The record is now dropped by the default-token exclusion in the search domain, not found-then-rejected by a post-filter.

gonzalesedwin1123 mentioned this pull request Jul 24, 2026

security(dci): stop DCI Administrator group from granting system admin #327

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

security(dci): purge stale compliance bearer token on upgrade#326

security(dci): purge stale compliance bearer token on upgrade#326
gonzalesedwin1123 wants to merge 7 commits into
19.0from
security-dci-compliance-stale-bearer-token

gonzalesedwin1123 commented Jul 23, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot commented Jul 23, 2026

Uh oh!

codecov Bot commented Jul 23, 2026 •

edited

Loading

Uh oh!

gonzalesedwin1123 commented Jul 23, 2026

Uh oh!

gonzalesedwin1123 commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

gonzalesedwin1123 commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes (final)

Design decisions (confirmed with owner)

Post-review hardening

Testing

Notes for reviewers

Uh oh!

gemini-code-assist Bot commented Jul 23, 2026

Uh oh!

codecov Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gonzalesedwin1123 commented Jul 23, 2026

Uh oh!

gonzalesedwin1123 commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

gonzalesedwin1123 commented Jul 23, 2026 •

edited

Loading

codecov Bot commented Jul 23, 2026 •

edited

Loading