Skip to content

Fix JWT::JWK::Set sharing its key collection when copied - #751

Open
anakinj wants to merge 2 commits into
jwt:mainfrom
anakinj:fix/jwk-set-key-array-ownership
Open

Fix JWT::JWK::Set sharing its key collection when copied#751
anakinj wants to merge 2 commits into
jwt:mainfrom
anakinj:fix/jwk-set-key-array-ownership

Conversation

@anakinj

@anakinj anakinj commented Sep 2, 2026

Copy link
Copy Markdown
Member

Description

Fixes #750.

JWT::JWK::Set.new(other_set) returned the source set's keys array by reference, despite the # Simple duplication comment on that branch. The class also defined no initialize_copy, so dup and clone shared the array as well — a case not covered in the issue.

Mutating a copy through add, <<, delete, select!, reject! or uniq! therefore mutated the original too:

original = JWT::JWK::Set.new([jwk])
JWT::JWK::Set.new(original).reject! { true }
original.size # => 0

This matters because key sets are commonly cached and shared between consumers, so one caller filtering its own copy can silently empty another's.

union was unaffected, since merge rebinds @keys with += rather than mutating in place. There is a regression test for that too.

Both copy paths now dup the array. The JWT::JWK objects themselves are still shared — only the collection is copied.

Checklist

Before the PR can be merged be sure the following are checked:

  • There are tests for the fix or feature added/changed
  • A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

@anakinj
anakinj force-pushed the fix/jwk-set-key-array-ownership branch from 4ddf558 to d4b2413 Compare September 2, 2026 13:49
JWT::JWK::Set.new(other_set) returned the source set's `keys` array by
reference, despite the "Simple duplication" comment on that branch. The
class also defined no `initialize_copy`, so `dup` and `clone` shared the
array as well.

Mutating a copy through `add`, `<<`, `delete`, `select!`, `reject!` or
`uniq!` therefore mutated the original too, which matters because key
sets are commonly cached and shared between consumers. `union` was
unaffected, as `merge` rebinds `@keys` with `+=` instead of mutating.

Both paths now copy the array. The JWK objects themselves stay shared.

Fixes jwt#750
@anakinj
anakinj force-pushed the fix/jwk-set-key-array-ownership branch from d4b2413 to ce143f6 Compare September 2, 2026 14:01
@anakinj
anakinj requested a balanced review from Copilot September 3, 2026 11:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

initialize_copy should duplicate @keys directly to avoid corrupting copied subclass state through an overridden accessor.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes shared key-array ownership when copying JWT::JWK::Set instances.

Changes:

  • Duplicates key collections during construction and object copying.
  • Adds regression tests.
  • Updates the changelog.
File summaries
File Description
lib/jwt/jwk/set.rb Adds independent collection copying.
spec/jwt/jwk/set_spec.rb Tests copy isolation.
CHANGELOG.md Documents the fix.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread lib/jwt/jwk/set.rb Outdated
After `super`, `initialize_copy` already holds a reference to the
source set's array in `@keys`, so duplicate that directly instead of
going through the `keys` reader. A subclass overriding `keys` with a
filtered or computed view then keeps its own state on copy.

Also move the union regression test out of the duplication context,
since it does not duplicate anything.
@anakinj
anakinj force-pushed the fix/jwk-set-key-array-ownership branch from f0a6c9b to 650851a Compare September 3, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Constructing a JWK::Set from another set aliases its key array

2 participants