Repository navigation
Conversation
getInternalClient copied the application's client listeners onto the internal key-vault client and forwarded every later newListener. It did not forward removeListener, and close() left the forwarder in place. A listener removed from the application client kept running for key-vault traffic and stayed reachable from the internal client. Forward removeListener for the same events newListener already mirrors, and detach both hooks when the encrypter closes. Registering the removeListener hook emits newListener, which copies that function onto the internal client; close() removes that copy explicitly.
Contributor
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of Changes
The internal key-vault client used by automatic encryption now drops a listener when the application client drops it, and
Encrypter.close()detaches the forwarding hooks.Notes for Reviewers
newListenerstill forwards every event name, which is the previous behavior, not onlyMONGO_CLIENT_EVENTS.removeListenermatches that set. Listeners that were already attached whengetInternalClientran are copied, andremoveListenerremoves those too, because they are the same function references.Registering the
removeListenerhook emitsnewListener, so that function is also attached to the internal client.close()removes it there directly. Once it has been removed from the application client it cannot observe its own removal.new Encrypter()constructsAutoEncrypterand can spawn mongocryptd. The unit test callsgetInternalClienton an uninitialized instance so the forwarding can be tested without that.close()is covered with a stubautoEncrypter.Jira: NODE-7986
What is the motivation for this change?
Applications that subscribe to command monitoring for the life of one operation, then remove the listener, kept receiving key-vault traffic and retained the listener for the life of the encrypted client.
For bug fixes
Current (incorrect) behavior:
Removing a listener from the application
MongoClientdoes not remove it from the internal client created for the key vault. Listeners added after the internal client is created are copied onnewListenerand then left in place.Encrypter.close()does not remove the forwarder.Expected behavior:
client.removeListener(event, listener)removes that function from the internal client. Afterencrypter.close(client), new subscriptions on the application client are not copied, and the internal client no longer holds the forwarding function.How to reproduce:
Unit coverage:
test/unit/client-side-encryption/encrypter.test.ts.Affected versions:
Present on main for any client that creates the internal key-vault client (auto-encryption without an explicit
keyVaultClient, and withmaxPoolSize !== 0). No specific server version.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript