fix(spp_attachment_av_scan): never queue a malware scan while the registry loads - #464
Conversation
#384 made the create/write scan-queue hooks re-raise database errors instead of swallowing them, so a transient serialization conflict reaches `odoo.service.model.retrying` and is retried. That contract is right inside a request. It is a trap during registry construction, which has no retry wrapper: a re-raised `could not serialize access due to concurrent update` there aborts the whole load and the server never boots. Registry load writes binary attachments routinely. Module data may declare `ir.attachment` records outright, and every `ir.ui.menu` carrying a `web_icon` recomputes `web_icon_data` -- a `Binary(attachment=True)` field -- whenever its XML is loaded, which lands as an `ir.attachment` create or write. Enqueueing there inserts a `queue.job` row, and that insert flushes the deferred `ir_attachment` UPDATE inside the re-raising hook, against a peer instance holding the same rows on a shared database. So gate both hooks on `env.registry.ready`. While it is False, nothing is enqueued. New attachments keep the `scan_status` default ("pending"), so nothing is left looking scanned; and `write` still performs the status reset before bailing out, because skipping it would leave changed bytes wearing the previous scan's `clean` verdict. That reset is a cache-level write on a row the transaction has already dirtied -- no extra SQL, no extra conflict window. Attachments written during load therefore stay `pending` until a rescan; no cron sweeps them today, which is the honest state and strictly safer than the alternative. Runtime uploads are untouched. Tests assert all four sides so the guard cannot regress in either direction: create and write enqueue nothing while the registry loads (and the write path still resets the status), and both still enqueue at runtime.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #464 +/- ##
=======================================
Coverage 75.93% 75.94%
=======================================
Files 627 627
Lines 43000 43004 +4
=======================================
+ Hits 32654 32658 +4
Misses 10346 10346
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approving. The env.registry.ready gate is the right fix for the #384 boot hazard, and I verified the two things that decide it against Odoo source in-container rather than taking the description on trust:
No runtime bypass. registry.ready is False for the entire module-load / _register_hook phase and flips True only at the end of Registry.new() (registry.py:225-226). new() is @locked and __new__ acquires the same lock, so every runtime request, -u upgrade, signaling-driven reload, and queue-job cursor binds a registry that is already ready=True before any user create/write runs. A legitimate runtime upload can never be silently skipped — and this is Odoo's own idiom: loading.py:588-592 gates _register_hook on the same flag for the same reason.
The coverage drop is safe-failing. Grepping every scan_status use: nothing treats pending as clean or safe — the only access-block is is_quarantined, which is set only after a scan confirms infection, and there is no download guard requiring scan_status == 'clean'. So load-time attachments (module data, ir.ui.menu.web_icon_data) stay permanently in the same unscanned state every runtime upload transits transiently; those are trusted build artifacts, not a realistic user-supplied malware vector, and nothing is ever left looking scanned. The write() reset-then-continue is correct: the status reset runs before the gate, so changed bytes can't keep a stale clean verdict, and it adds no SQL/conflict window (plain stored fields on an already-dirtied row; the flush-triggering queue.job insert is exactly what's skipped).
Suite is 50/50 and the bite check is precise — removing only the create() gate fails exactly test_create_does_not_queue_a_scan_while_the_registry_loads while the untouched write() test and the two runtime anti-vacuity tests stay green, so the four tests are per-path guards, not one coarse assertion.
Version 2.0.1→2.0.2 correct, HISTORY OCA newest-first, README/index.html regenerated consistently, no new logging/PII.
One non-blocking follow-up worth a tracking issue: load-time attachments stay pending forever (no cron sweeps pending; the two existing crons only purge quarantine/forensic rows). Since pending isn't treated as safe this is acceptable, but a small runtime cron rescanning scan_status='pending' binary attachments would close the residual gap without reintroducing the boot hazard (a cron runs with ready=True). The UI already exposes a pending filter + action_rescan, so there's a manual path today.
…can (#470) * feat(spp_attachment_av_scan): sweep attachments stranded at pending Queueing a malware scan is best-effort by design (#384): a non-database enqueue failure is logged and the attachment is still written. Nothing came back for the records that left behind — written, at the scan_status default, indistinguishable in the UI from a file still waiting its turn in a deep queue, retained unscanned indefinitely, and reachable only by a human clicking Rescan. #464 added a second, benign source of the same state. An hourly ir.cron re-queues them, active by default and bounded on both axes so it is safe on an existing database: pending_sweep_batch_size caps one run, pending_sweep_max_attempts caps the attempts per record, and pending_sweep_min_age_minutes keeps a fresh upload that is merely waiting in a deep queue from being double-queued. Bumping the attempt counter refreshes write_date, so the age threshold doubles as a flat backoff, and a broken queue is reported once per run at WARNING rather than once per record per tick. Scope is user content: quarantined files (matching action_rescan), forensic download copies, attachments with no res_model, and the system models that store their own source-controlled binaries. A blank res_model is not a proxy for that last set — Binary(attachment=True) storage records the owning model, so menu icons arrive as res_model='ir.ui.menu' — hence the explicit denylist. The sweep also has to search under skip_res_field_check, because ir.attachment._search silently hides every attachment backing a binary field, which would have dropped user-uploaded image_1920 content from the sweep while appearing to cover it. The attempt counter is bumped for every record the batch picks up, before the readability check: an attachment whose filestore file is lost has file_size > 0 but no readable bytes, and skipping it without moving write_date would park it at the head of every run forever. * fix(spp_attachment_av_scan): mark sweep cron and config defaults noupdate Without noupdate every module upgrade silently reset admin-tuned sweep values (batch size, age threshold, a deliberately disabled cron) back to the shipped defaults. * fix(spp_attachment_av_scan): evict swept payloads from the ORM cache The sweep reads each attachment's bytes only to prove they are readable; the queued job re-reads them in its own transaction. Binary fields are never evicted on their own, so a full batch would otherwise hold every payload in one cron transaction's cache simultaneously.
2 participants
Problem
#384 made the create/write scan-queue hooks re-raise database errors instead of swallowing them, so a transient serialization conflict reaches
odoo.service.model.retryingand is retried transparently. That contract is right inside a request, where the retry wrapper exists.It is a trap during registry construction, which has no such wrapper. A re-raised
could not serialize access due to concurrent updatethere aborts the whole registry load and the server never boots.Registry load writes binary attachments routinely:
ir.attachmentrecords outright;ir.ui.menucarrying aweb_iconrecomputesweb_icon_data— aBinary(attachment=True)field (odoo/addons/base/models/ir_ui_menu.py:41,161) — whenever its XML is loaded, which lands as anir.attachmentcreate or write.Enqueueing there inserts a
queue.jobrow, and that insert flushes the deferredir_attachmentUPDATE inside the re-raising hook, against a peer instance holding the same rows on a shared database.registry.readyisFalsefor the whole load and only flips at the end (odoo/orm/registry.py:217);_register_hookruns while it is stillFalse, asodoo/modules/loading.pySTEP 9 notes explicitly.Fix
Gate both hooks on
env.registry.ready. While it isFalse, nothing is enqueued.scan_statusdefault (pending), so nothing is left looking scanned.writestill performs the status reset before bailing out — skipping it would leave changed bytes wearing the previous scan'scleanverdict. The reset is a cache-level write on a row the transaction has already dirtied, so it adds no SQL and no conflict window.Trade-off: this drops scan coverage, and the drop is new
Stated plainly, because it is easy to misread as a pre-existing gap.
job_workerhas no registry gate —with_delayonly writes aqueue.jobrow, which commits with the load transaction and is picked up by the runner after boot (job_worker/delay.py;utils.py:must_run_without_delayis the only bypass). So before this change, attachments written during registry load were eventually scanned. After it, they staypendingindefinitely: no cron sweeps pending attachments today (the module's only crons are quarantine purge and forensic-download cleanup), soaction_rescanis the sole route back.What actually falls into that gap is module data and menu
web_iconimages — files shipped in the addon source tree and trusted exactly as much as the Python beside them. No user-supplied content is written during registry load. So the coverage lost is real but close to nil in practice, andpendingis the honest, safe state: it is findable by a future sweep, and it never claims a verdict the scanner did not give.Follow-up tracked in #465, framed around the older and more consequential hole it also closes: both hooks still end in a broad
except Exceptionthat logs and continues, so a non-DB enqueue failure silently strands a runtime user upload atpendingforever. That predates #384 and is the case that genuinely warrants a sweep.Also worth naming: this removes the hook as a source of boot-aborting errors. If a serialization conflict lands on the base
ir_attachmentUPDATE itself,env.flush_all()after STEP 9 would still surface it — upstream territory, out of scope here.Tests
spp_attachment_av_scan/tests/test_scan_queue_registry_ready.pyasserts all four sides, so the guard cannot regress in either direction:creatependingwriteclean→pendingFull module suite: 50 passed, 0 failed, 0 errors.
pre-commitclean (fresh env) apart from the known localsemgrepimport crash, which CI covers; no newsudo()in this change.