feat(spp_attachment_av_scan): sweep attachments stranded at pending scan - #470
Conversation
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #470 +/- ##
==========================================
+ Coverage 76.07% 76.09% +0.02%
==========================================
Files 661 661
Lines 44022 44078 +56
==========================================
+ Hits 33488 33541 +53
- Misses 10534 10537 +3
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.
This is excellent work — the three "worth a reviewer's attention" items are exactly the traps I went looking for, and each is closed with the rule asserted in both directions. Verified in particular:
- the
skip_res_field_checkfinding is real and the handling is right: without it the implicitres_field = Falsefilter would have silently dropped every user-uploaded binary field from a security sweep while appearing to cover it, andtest_a_user_upload_into_a_binary_field_is_still_swept/test_an_attachment_on_an_excluded_system_model_is_skippedpin both halves so the denylist can't silently invert; - the
write_date-vs-create_datereasoning holds, and count-attempt-before-readability-check genuinely closes the batch-slot starvation (with the test to keep it closed); - priority ordering is coherent (manual rescan 10, hooks 20, sweep 30), the error contract matches the hooks (
_MUST_NOT_SWALLOWre-raised, everything else survivable at one WARNING per run),action_rescanand byte-change both re-arm, quarantine/forensic exclusions match existing behaviour; - version 19.0.2.1.0 chains correctly on #464's 2.0.2 (merged), CI fully green.
One small must-fix on the data file and one non-blocking suggestion, both inline. With the noupdate fix this is an approve.
| an existing database drains the backlog gradually rather than enqueueing it all at | ||
| once. Raise the batch size to drain it faster. | ||
| --> | ||
| <record id="ir_cron_sweep_pending_scans" model="ir.cron"> |
There was a problem hiding this comment.
Must fix: wrap this file's records in <data noupdate="1">.
As written, every module upgrade rewrites all four records: an admin who raised pending_sweep_batch_size to drain a backlog, lowered the age threshold, or deliberately disabled the cron gets silently reset to 100/60/3/active on the next upgrade. For a default-active security-control cron whose own comments invite the admin to tune these values, that's an operational trap — and ir.cron + ir.config_parameter defaults are the textbook noupdate="1" case.
The existing quarantine_cron.xml has the same defect, but that's pre-existing and out of scope here — happy to file it as a follow-up so it isn't lost (fixing it retroactively needs a thought about deployments that already re-absorbed the defaults, which is exactly why it shouldn't be bundled in).
There was a problem hiding this comment.
Fixed in 7f87b2b — the whole file is now noupdate (via <odoo noupdate="1">, matching av_scanner_data.xml's existing style in this module), with a test asserting the flag on all four records so it can't regress. Agreed on keeping quarantine_cron.xml out of scope — please do file the follow-up.
| {"scan_queue_attempts": attachment.scan_queue_attempts + 1} | ||
| ) | ||
|
|
||
| if not attachment.datas: |
There was a problem hiding this comment.
Non-blocking suggestion: this read pulls the full base64 payload into the ORM cache, and the cache accumulates across the loop — binary fields aren't prefetched, but they aren't evicted either, so a batch of 100 stranded videos/PDFs holds every payload simultaneously in one hourly cron transaction. The job re-reads the bytes in its own transaction anyway, so nothing here needs them after this check:
readable = bool(attachment.datas)
attachment.invalidate_recordset(["datas", "raw"])
if not readable:
...Fine to take as a follow-up if you'd rather not touch the loop again — the batch size bounds it, it's a spike, not a leak.
There was a problem hiding this comment.
Took it now rather than as a follow-up (3b81eba) — exactly your snippet, plus a test asserting that after the sweep runs, raw is no longer held in the transaction cache (with an anti-vacuity check that a plain read does cache it).
…date Without noupdate every module upgrade silently reset admin-tuned sweep values (batch size, age threshold, a deliberately disabled cron) back to the shipped defaults.
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.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approving. Both items landed better than asked:
noupdate="1"on the whole data file, with a regression test assertingir.model.data.noupdateon all four records — so the guarantee itself is pinned, not just the current XML shape.- The readability check now evicts the payload it read, with an anti-vacuity test proving the read does cache and the sweep does evict (
env.cache.containsis exactly the right probe).
CI fully green. I'll file the quarantine_cron.xml noupdate follow-up so the pre-existing instance of the same defect isn't lost.
Merge authorization is Edwin's, as usual.
2 participants
Closes #465.
The hole
Queueing a malware scan is best-effort by design (#384): the
create/writehooks re-raise database errors but swallow everything else, so a dead broker or a misconfigured queue channel cannot block an attachment write.The consequence was unbounded. When the enqueue failed for any non-DB reason the attachment was still written, left at
scan_status = pending, and nothing ever came back for it — an unscanned file, indistinguishable in the UI from one still waiting its turn in a deep queue, retained indefinitely. The only evidence was one ERROR line in the server log; the only route back was a human clicking Rescan. #464 added a second, benign source of the same state.The sweep
_cron_sweep_pending_scansonir.attachment, hourly, active by default.pending_sweep_batch_size(default 100), orderedwrite_date asc, id ascso successive runs advance through a backlog instead of re-picking its headpending_sweep_min_age_minutes(default 60), againstwrite_date.create_dateis wrong: adataswrite resets an old record topending, so acreate_datecutoff would re-queue it the instant the hooks already didscan_queue_attemptsfield, capped bypending_sweep_max_attempts(default 3). Bumping it refresheswrite_date, so the age threshold doubles as a flat backoff. Reset when the bytes change or onaction_rescan, so a human ask re-arms the sweepis_quarantinedaction_rescanalready refuses_MUST_NOT_SWALLOW, same contract as the hooks — a dead transaction cannot be continued through the loopThree things worth a reviewer's attention
1. The issue's scope premise doesn't hold. #465 proposed skipping records with no
res_modelas "approximately the set of system assets (module data,web_icon_data)". True for module data, not for menu icons:Binary(attachment=True)storage records the owning model (odoo/orm/fields_binary.py:173-175), so they arrive asres_model='ir.ui.menu',res_field='web_icon_data'— confirmed by the lookup domain atodoo/addons/base/models/ir_ui_menu.py:263-265. A bareres_model != Falserule would have swept every source-controlled icon. Hence the explicitSWEEP_EXCLUDED_MODELSdenylist.2. A silent coverage hole, found by the first test run.
ir.attachment._search(odoo/addons/base/models/ir_attachment.py:620-630) quietly ANDsres_field = Falseinto any domain mentioning neitherres_fieldnorid. A plainsearch()therefore sees no field-storage attachment at all. That would have excluded menu icons for free — making the denylist dead code — but it would equally have excluded every user-uploaded binary field (res.partner.image_1920and friends), invisibly, while the sweep appeared to cover them. For a security control that is the unacceptable half, so the sweep searches underskip_res_field_check=Trueand the denylist becomes load-bearing: it now carries the exclusion the implicit filter was providing by accident. Both directions are asserted so the rule cannot silently invert.3. A batch-slot starvation bug, found reviewing the loop. An attachment whose filestore file is lost has
file_size > 0but no readable bytes. Skipping it without counting an attempt leaveswrite_dateuntouched, so underwrite_date ascit sorts to the head of every run and consumes a batch slot forever. The counter is bumped for every record the batch picks up, before the readability check.Tests
spp_attachment_av_scan/tests/test_pending_scan_sweep.py, 17 cases covering every design call in both directions where the rule could invert:res_model, denylisted-model, forensic-download, and URL attachments are all skipped — each asserted directlyaction_rescanboth re-arm the sweep69 tests pass (
./spp test spp_attachment_av_scan), output clean — the tracebacks and ERROR lines remaining in the log are pre-existingav_scanner_backendcases.Follow-up not taken
job_workersupportsidentity_keywith a unique index on active jobs, which would make double-queueing structurally impossible rather than merely improbable. Using it would mean adding the key to thecreate/writehooks too — an already-queued job has a NULL key, so a sweep-only key dedupes against nothing — which changes those hooks and their tests, beyond this issue's scope. Worth revisiting if the age threshold proves too blunt in practice.🤖 Generated with Claude Code