OpenSPP Attachment Scan: Protecting Admin-Tuned Crons From Upgrade Resets

Context

The spp_attachment_av_scan module ships some scheduled jobs and settings as data records. In data/quarantine_cron.xml there are two crons, one that purges old quarantined files daily and one that cleans up forensic download copies hourly, plus two configuration parameters: a quarantine retention window of 90 days and a forensic-download retention window of 24 hours.

These are exactly the kind of values a deployment is expected to tune. An administrator might lengthen the retention window, change how often a cron runs, or deliberately switch one off.

Problem

The file declared all four records in a plain <odoo> block, with no noupdate flag:

<odoo>
    <record id="ir_cron_purge_quarantined_files" model="ir.cron"> ... </record>
    <record id="ir_cron_cleanup_forensic_downloads" model="ir.cron"> ... </record>
    <record id="config_param_quarantine_retention_days" ...>90</...>
    <record id="config_param_forensic_download_retention_hours" ...>24</...>
</odoo>

Without noupdate, Odoo rewrites those records on every module upgrade. So an admin who tuned a retention window, changed a cron interval, or disabled a cron would find their change silently reverted to the shipped default the next time the module was upgraded. The tuning just vanished, with nothing to point at.

Background

This defect was already known in a neighboring file. The review of pull request #470 fixed the same class of problem in that module's newer scan_sweep_cron.xml, and added a regression test for it. The pre-existing quarantine_cron.xml had the same flaw, and was split out into its own issue so it could be fixed the same way. That gave me a proven template to follow rather than a blank page.

My Approach

The fix has three parts, because marking the file noupdate is necessary but not sufficient on databases that already exist.

First, wrap the data file so new installs create the records as protected:

<odoo noupdate="1">

Second, a migration. Odoo only honors noupdate at the moment a record is first created. On any deployment that already installed the module, the four ir.model.data rows already exist with noupdate = False, and changing the XML alone flips nothing. So I added a post-migration that updates the existing rows in place:

cr.execute(
    """
    UPDATE ir_model_data
    SET noupdate = TRUE
    WHERE module = 'spp_attachment_av_scan'
      AND name IN %s
    """,
    (_RECORDS,),
)

It sets the flag and leaves the stored values untouched, which is the safe default. Where an admin never tuned a value it is still the shipped default, and where they did tune it, their value is preserved.

Third, a manifest version bump to 19.0.2.2.0, with the migration placed under migrations/19.0.2.2.0/post-migrate.py. Odoo runs migration scripts from the folder named after the version it is upgrading to, so the folder name has to match the new manifest version exactly.

Tests

Rather than write a new test, I extended the one #470 already created: test_the_cron_and_config_defaults_are_not_reset_by_a_module_upgrade. It loops over each shipped record and asserts its ir.model.data row has noupdate = True. I added the four quarantine record names to that loop, so the single test now covers both cron files in the module.

Considerations

• Values are left as-is: the migration flips only the flag, never the stored value. Rewriting values on upgrade is the very behavior being fixed, so the migration is careful not to reintroduce it.
• The folder name is load-bearing: a migration folder whose name does not exactly match the manifest version is silently skipped. Getting the version bump and the folder name to agree is part of the fix, not an afterthought.
• It sits on top of #470: because this change extends a test and a pattern introduced by #470, it belongs on a branch that already contains #470's work, not on an older base.

Fix Process

• compared quarantine_cron.xml against the already fixed scan_sweep_cron.xml to copy the exact pattern
• added the noupdate wrapper, the post-migration, and the manifest bump as one coherent change
• extended the existing upgrade-reset regression test to cover the four quarantine records
• rebased the work onto a branch containing #470 so the shared test and helper code were present, and kept the migration path free of any stray characters so Odoo will actually run it

Current Status

Pull request #479 is open against the 19.0 branch and merges cleanly, awaiting CI and maintainer review. As with my other contributions, I could not run the Odoo suite locally, so I am relying on the project's CI to exercise the spp_attachment_av_scan tests and the migration on the pull request.

Reflection

The one-line XML change is the visible part, but the interesting part is what it does not do. Because noupdate is only read at creation time, the honest fix has to reach back to the records that already exist and correct them, without disturbing the very values it is trying to protect. That distinction between a fresh install and an upgraded one is easy to miss, and missing it would have shipped a change that quietly did nothing on every real deployment.

For an antivirus quarantine, the settings being protected decide how long potentially harmful files are retained. Letting an operator's deliberate choices survive an upgrade is a small piece of trustworthiness that matters more than its size suggests.