Repository navigation
github-backup(drain): owner allowlist crashes the whole drain run on a non-dict owner/account — malformed event becomes a poison pill instead of a DRAIN-DROP #3331
Description
Summary
infra/github-backup/bin/drain_queue.py — the #3326 owner allowlist crashes the whole drain run on a queued event whose repository.owner (or installation.account) is present but not a JSON object. The allowlist's stated contract (module docstring, #3326) is that "malformed events whose owner cannot be determined are dropped: acked as handled-by-policy, logged as one loud DRAIN-DROP line". A non-dict owner instead raises AttributeError before any drop, fetch, or ack happens — and because the event is never acked, it stays at the head of the queue and crashes every subsequent 5-minute drain run the same way: a poison pill that stalls all mirror fetches until someone removes the event by hand.
Evidence (verified this run, current main 72e34760)
event_owner at drain_queue.py:111-127 guards the containers it was thinking of, but not the leaf containers:
rep = ev.get("repository")
if isinstance(rep, dict):
login = (rep.get("owner") or {}).get("login") # owner may be str/list/int -> AttributeError
...
inst = ev.get("installation")
if isinstance(inst, dict):
login = (inst.get("account") or {}).get("login") # account may be str/int -> AttributeErrorLaptop probe ~/git/fleet/probe-2026-10-31-laneB-drainowner/probe.py driving the real module (sha256 af2fbb2c7d1af7da31aa36b8d3ce39185f1ea33785ca989b7425272e37ff4480, byte-identical to main):
control-missing-owner: event_owner -> None
control-owner-dict: event_owner -> 'mallory'
bug-owner-string: event_owner RAISED AttributeError: 'str' object has no attribute 'get'
bug-owner-list: event_owner RAISED AttributeError: 'list' object has no attribute 'get'
bug-owner-int: event_owner RAISED AttributeError: 'int' object has no attribute 'get'
bug-account-string: event_owner RAISED AttributeError: 'str' object has no attribute 'get'
bug-account-int: event_owner RAISED AttributeError: 'int' object has no attribute 'get'
main-poison: main() RAISED AttributeError — fetch_calls=[] ack_calls=[]
main-control (valid planned event only): rc=0 fetch_calls=['owner/repo'] ack_calls=[['good1']]
The main-poison case puts the malformed event first and a valid, planned owner/repo event second: the valid event is never fetched and never acked, no DRAIN-DROP line is written, and main() exits via the exception — the exact opposite of the #3326 design, in which this event class is acked and counted in dropped_foreign.
The existing tests in infra/github-backup/tests/test_drain_queue.py cover the missing-owner shape ({"id": "e10", ...} with no repository/installation at all, case 10) but no shape where owner/account is present with a non-object type — the shapes a hand-built or future payload (which event_repo's docstring explicitly says the drainer accepts) can carry.
Why it matters
- The drainer is the 5-minute mirror-fetch path for the whole github-backup estate. One malformed queued event stops fetches for every repo, not just its own, and the failure self-repeats indefinitely because the crashing event is never acked or dropped.
- This defeats the security fix it lives in: github-backup: drainer needs an owner allowlist — public App admits foreign installs #3326's allowlist exists so untrusted input (the App is public; anyone can install it) is handled at the door. Untrusted-shaped input that crashes the door is worse than the pre-allowlist behavior for availability — before github-backup: drainer needs an owner allowlist — public App admits foreign installs #3326 this event shape would have fallen through to the repo grouping instead.
Acceptance criteria
-
event_ownerreturnsNone(→ DRAIN-DROP + ack +dropped_foreigncount) for events whoserepository.ownerorinstallation.accountis any non-dict value (string, list, number, bool), instead of raising. - A drain run containing such an event first still fetches and acks the valid planned events behind it and exits 0.
- Regression cases in
infra/github-backup/tests/test_drain_queue.pyfor at least the string-owner and string-account shapes, asserting drop + ack + no fetch for the malformed event and normal processing for a following valid event. - No behavior change for well-formed events: existing 54 drain-queue tests still pass.
Related: #3326 (the allowlist this bug lives in; its implementation commit cc35ae40 is on current main). Distinct from #3300 (reconcile clone-pass exit code), #3279/#3240 (non-atomic state writes), #3238/#3229 (inventory/mirror validity).
Activity
dezren39 commentedon Nov 1, 2026
🤖 Agent note · filed
dezren39 commentedon Nov 1, 2026
🤖 Agent note · filed (addendum, same run)
idat all ({"event":"push","ts":...}— no repo, no owner) reaches the drop branch: the DRAIN-DROP line IS written (delivery=None, viaev.get("id")at drain_queue.py:170), thenack.append(ev["id"])at :172 raisesKeyError: 'id'. Laptop probe2 (same sha-matched module):main() RAISED KeyError: 'id' fetch_calls=[] ack_calls=[]— the valid planned event behind it is again never fetched, the ack POST never happens, so this event also re-crashes every drain run (and appends a fresh DRAIN-DROP line each time).3 remaining items
dezren39 commentedon Nov 2, 2026
🤖 Agent note · picked up
repository.owner/installation.accountinevent_owner(infra/github-backup/bin/drain_queue.py) plus regression cases (string-owner, string-account, following-valid-event) in tests/test_drain_queue.py. Issue github-backup(drain): owner allowlist crashes the whole drain run on a non-dict owner/account — malformed event becomes a poison pill instead of a DRAIN-DROP #3331 revalidated fresh: OPEN, no hard-stop labels, no open PR.queue/5937-3331-drain-owner-guardMerge pull request #4397 from developing-today/queue/5937-3331-drain-…
dezren39 commentedon Nov 2, 2026
🤖 Agent note · done
event_ownerin drain_queue.py now guardsrepository.owner/installation.accountwith isinstance(dict); non-dict shapes fall through precedence to None → DRAIN-DROP + ack + dropped_foreign instead of raising AttributeError and poisoning the drain.python3 tests/test_drain_queue.pypasses 73/73 (54 existing + 19 new, cases 14–16). Codex review on head dc24293: no findings.