MONGOID-5991 Filter nil keys out of association membership queries

[jamis]

Description

HasManyThrough#criteria built its membership query by plucking key values from the intermediate criteria and dropping the result straight into an $in, with no filtering. An intermediate document missing the plucked key contributes a literal nil, and a nil in an $in matches every target document whose queried field is null or absent. The association read then returned documents that are not in the relationship, including documents belonging to another owner.

Both branches of #criteria were affected. The eager loader already did the right thing (filter_map in has_many_through/eager.rb), so the lazy and eager paths returned different results for identical data — which is what makes this an omission rather than a design choice.

The precondition is a custom primary key. With the default _id, nil still enters the $in, but no document has a null or absent _id, so nothing matches and nothing leaks.

The fix

Both branches now route plucked values through a key_values helper that applies compact and uniq, mirroring the eager loader. compact is the fix; uniq shrinks the query and makes the two paths structurally identical. If filtering leaves the array empty, an empty $in correctly matches nothing — the right outcome for an association whose intermediates carry no usable key.

has_and_belongs_to_many

has_and_belongs_to_many.rb builds the same primary_key => $in id_list shape from the stored foreign-key array. Associating a target whose custom primary key is unset stores a literal nil in that array and reproduces the same leak. That array is now compacted too.

One note on this half: id_list is not guaranteed to be an Array — an existing spec passes a class — so the compact is guarded with an is_a?(Array) check rather than changing that spec's contract. The nil id_list → crit.none branch is untouched, so "no list" and "list of only nils" remain distinguishable.

Tests

11 new examples, each watched fail before the fix was written:

• has_many :through, both branches (FK on intermediate, FK on source). Each asserts no leaked documents, no nil in the emitted selector, and that the lazy and eager paths agree on identical data. Plus an all-keys-nil case (returns nothing, not everything) and the cross-owner leak with an orphaned target.
• HABTM: associating a target whose custom primary key is unset, asserting the same three properties.

The lazy/eager agreement specs are redundant with the leak specs today. They are included because the divergence between the two paths is what made this defect possible, and they are the cheapest guard against reintroducing it.

Verification

• spec/mongoid/association/referenced/ — 3466 examples, 0 failures, 7 pending (pre-existing).
• Confirmed the new specs fail with lib/ reverted and pass with the fix.
• RuboCop clean.

[Copilot]

Pull request overview

Fixes an association query correctness/security issue where nil values could enter $in membership queries for custom primary keys, causing unrelated documents (including cross-owner) to be returned. This brings lazy association loading behavior in line with the existing eager loader behavior.

Changes:

• HasManyThrough#criteria now compacts and de-duplicates plucked key values via a helper before building $in selectors.
• HABTM membership querying now compacts stored foreign key arrays (when the list is an Array) before building the $in selector.
• Adds integration specs covering both HasManyThrough branches and HABTM, including “all keys nil” and cross-owner leak scenarios.

Reviewed changes

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

File	Description
lib/mongoid/association/referenced/has_many_through.rb	Filters nil (and de-dupes) plucked intermediate key values before $in membership queries.
lib/mongoid/association/referenced/has_and_belongs_to_many.rb	Compacts stored foreign key arrays (when Array) to prevent nil from entering $in queries.
spec/mongoid/association/referenced/has_many_through_spec.rb	Adds integration coverage for missing plucked keys in both HasManyThrough criteria branches, ensuring no leakage and eager/lazy agreement.
spec/mongoid/association/referenced/has_and_belongs_to_many_spec.rb	Adds integration coverage for nil stored keys in HABTM foreign key arrays, ensuring no leakage and correct empty results.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.