Bug 2054320: Post bug.modify events on dependency changes

[justdave]

This updates the Push extension to generate a bug.modify event for any bug touched by a relationship change (blocked, dependson, regresses, regressed_by), and adds an indirect_change hash to the top-level of the object which contains metadata about the source of the change (which bug was edited that caused it)

[justdave]

force-push was backing out the last commit, that was meant for a different branch.

[Copilot]

Pull request overview

Adds indirect Push events for bugs affected by dependency or regression relationship edits.

Changes:

• Generates counterpart bug.modify events.
• Adds indirect_change metadata to event payloads.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dklawren]

High-effort review of extensions/Push/Extension.pm (new counterpart bug-modify event logic). 4 finders → 9 candidates → 7 verifiers → 6 findings survived (0 refuted). Ranked most-severe first.

---

🔴 Correctness

1. indirect_change.source_bug_id bypasses Webhook redaction — private-bug-id leak

extensions/Push/Extension.pm:382 · CONFIRMED

indirect_change (carrying source_bug_id) is attached to the counterpart event but is not stripped by the Webhook connector's private-modify redaction, unlike event.changes which are deleted for private targets.

▎ Leak: Private source bug A gains/loses a dependency on public bug B. The counterpart modify event for B carries indirect_change.source_bug_id = A. Webhook.pm:166 only deletes event.changes when the target is private — but B is public, so no redaction runs. _owner_can_see only checks visibility of B (the target), never the referenced source_bug_id A. A webhook owner who can see public B but not private A still receives A's bug id.

2. Related bugs serialized in full with no visibility check

extensions/Push/Extension.pm:185 (also :188) · PLAUSIBLE

The no-leak invariant deliberately enforced in _object_modified (lines 108–143, which sends $old_bug "so we don't leak any security sensitive information") is not re-established on the new counterpart path. Related bugs are loaded via Bugzilla::Bug->new($related_bug_id) and pushed with full serialized state, with no check that the actor or consumer may see them.

▎ Leak: A user links a public bug to a security-restricted related bug. _emit_counterpart_bug_modify_events loads the restricted bug and calls _push_object('modify', $related_bug, ...), serializing its contents. A connector without its own owner-visibility filtering (unlike Webhook's _owner_can_see) emits an event exposing the confidential bug's fields.

3. Create-path counterpart events use a fresh change_set_id() — correlation broken

extensions/Push/Extension.pm:474 (also :475) · CONFIRMED

In bug_end_of_create, counterpart modify events get a fresh change_set_id() that differs from the new bug's own create event, so the two halves of one user action can't be correlated. The update path correctly shares one change_set.

▎ Failure: User files bug A that blocks existing bug B. _object_created pushes A's create event with change_set X (line 92); _emit_counterpart_bug_modify_events calls change_set_id() again (line 474) → change_set Y. A consumer grouping by change_set sees A's create and B's modify under two unrelated sets and can't tie them together.

4. Synchronous per-related-bug loads inside the open transaction

extensions/Push/Extension.pm:188 · CONFIRMED

Each related bug is loaded via Bugzilla::Bug->new and pushed synchronously in a loop while the create/update DB transaction is still open, scaling linearly with the number of changed dependencies.

▎ Failure: A tracking/meta bug blocking hundreds of bugs is created or bulk-edited. bug_end_of_create/_object_modified performs hundreds of Bugzilla::Bug->new round-trips and inserts a push message for each — all inside the still-open transaction — causing slow submission, prolonged row/table locking, and potential request timeouts.

---

🟡 Cleanup

5. _counterpart_bug_changes recomputes a diff it was already given

extensions/Push/Extension.pm:218 · CONFIRMED

$changes->{$field} is already [removed_ids, added_ids] (Bugzilla/Bug.pm:1076). Rebuilding added/removed via set-membership diff (%all_ids, $in_old, $in_new, next if $in_old == $in_new) is dead machinery that only ever reproduces the split it was handed — obscuring that the inputs are deltas, not snapshots.

6. Dead ARRAY-ref branch in _parse_related_bug_ids

extensions/Push/Extension.pm:243 · CONFIRMED

Both callers pass comma-joined strings: the modify path gets string values from the changes hash, and the create path explicitly does join(', ', @$related_ids) at line 467. ref $raw_value eq 'ARRAY' is never true — unreachable code, and the create path needlessly stringifies an arrayref only to re-split it.

---

Suggested priority: #1 and #2 are data-leak vectors and should block. #3 breaks consumer event correlation. #4 is a scaling/locking risk worth addressing (consider deferring related-bug pushes outside the transaction, e.g. via the JobQueue). #5/#6 are safe simplifications.

[justdave]

1. indirect_change.source_bug_id leak: fixed in Push emission and Webhook redaction.
2. Related-bug full serialization with no visibility guard: mitigated by skipping private counterpart target events in Push.
3. Create-path fresh change_set_id correlation break: fixed by generating one change_set in bug_end_of_create and reusing it for both create + counterpart modify.
4. Synchronous per-related-bug loads in open transaction: improved with batched load; still synchronous hook-time work (hook runs pre-commit in core Object update flow).
5. _counterpart_bug_changes recompute-diff cleanup: fixed.
6. Dead ARRAY branch cleanup: addressed by making arrayref input real on create path.

[justdave]

The test failures look like something server-side was broken (s3 was down?) when the tests ran, and not the tests themselves actually failing.

s3 Error received unexpected HTTP status: 500 Internal Server Error

[justdave]

3 things in the changeset just pushed to this PR:

1. merged from master to get branch up-to-date
2. renamed indirect_change.dependency to indirect_change.related to avoid ambiguity about whether regressions count (they do).
3. Found a spot where we were still missing redaction of Bug IDs the user couldn't see (we redacted them from the indirect_change object, but not from the event.changes hash in the payload, so added code to deal with that.

[dklawren]

Correctness

1. Autovivification bug (Webhook.pm:170) — the exists $payload->{event}->{indirect_change}->{source_bug_id} check autovivifies an empty indirect_change hash into every non-indirect webhook payload, adding a spurious "indirect_change":{} field and triggering per-send uninitialized-value warnings.
2. Metadata leak (Webhook.pm:248) — after redaction blanks the changes, an empty-changes bug.modify counterpart event is still delivered, signalling to the owner that an invisible private bug altered relationships.

Cleanup

3. Redundant/disagreeing redaction (Extension.pm:194) — source_bug_id is redacted both globally at emit time and per-owner in Webhook.pm; the two layers conflict and over-redact for consumers legitimately entitled to see the private source bug.
4. Unconditional relationship SELECTs (Extension.pm:478) — bug_end_of_create reads all four relationship accessors (4 DB round-trips) on every bug creation before checking connector interest, slowing the create hot path.

[justdave]

Correctness
2. Metadata leak (Webhook.pm:248) — after redaction blanks the changes, an empty-changes bug.modify counterpart event is still delivered, signalling to the owner that an invisible private bug altered relationships.

I knew this one when I posted it... but at the point it gets redacted, the decision has already been made that "there's a webhook getting delivered" and I can't stop it from going out, only make changes to it. Let me think about this one. Might be able to hook it earlier in the process, or get the dispatcher to look for a "please abort" flag or something.

[justdave]

Unconditional relationship SELECTs (Extension.pm:478) — bug_end_of_create reads all four relationship accessors (4 DB round-trips) on every bug creation before checking connector interest, slowing the create hot path.

For this one, I discovered that bug_before_create actually gives us the relationships without an extra DB hit, which bug_end_of_create has no way to get them without hitting the database. So I hooked bug_before_create and stashed them so we could fetch them out again during bug_end_of_create when we actually need them without hitting the DB.

This does raise the question though of what if someone else changes those relationships in an intervening hook before we get that far? Seems unlikely, but surely possible. Do we need to revert this and live with the database hit for data correctness?

There were merge conflicts, so I had to do a merge commit, so for review purposes you can ignore all the stuff that you co-authored. :-) I split the four review comments into four separate commits so you can look at them individually if you want.

[justdave]

My merge commit apparently pulled in some things it shouldn't have ... the Changed Files view is showing a bunch of things I swear I didn't touch ... rebase/force-push coming shortly to clean out unrelated/horked changes.

[justdave]

OK, looks clean now. The last four commits are the ones dealing with the last set of review comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.