From 3e709118d23b37ce353b639d7c3bec0fd9d88816 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 1 Aug 2023 17:03:48 -0400 Subject: [PATCH 1/5] feat(NODE-5233)!: prevent session from one client from being used on another --- src/mongo_client.ts | 9 ++++- src/operations/execute_operation.ts | 2 + .../sessions/sessions.prose.test.ts | 37 ++++++++++++++++++- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 7d1e02cf8ee..115808638a9 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -598,7 +598,14 @@ export class MongoClient extends TypedEventEmitter { return client.connect(); } - /** Starts a new session on the server */ + /** + * Creates a new ClientSession. When using the returned session in an operation + * a corresponding ServerSession will be created. + * + * @remarks + * A ClientSession instance may only be passed to operations being performed on the same + * MongoClient it was started from. + */ startSession(options?: ClientSessionOptions): ClientSession { const session = new ClientSession( this, diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 50f77648f97..5424858e0aa 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -118,6 +118,8 @@ async function executeOperationAsync< throw new MongoExpiredSessionError('Use of expired sessions is not permitted'); } else if (session.snapshotEnabled && !topology.capabilities.supportsSnapshotReads) { throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later'); + } else if (session.client !== client) { + throw new MongoRuntimeError('ClientSession must be from the same MongoClient'); } const readPreference = operation.readPreference ?? ReadPreference.primary; diff --git a/test/integration/sessions/sessions.prose.test.ts b/test/integration/sessions/sessions.prose.test.ts index ee851e5f318..4015932575e 100644 --- a/test/integration/sessions/sessions.prose.test.ts +++ b/test/integration/sessions/sessions.prose.test.ts @@ -6,11 +6,46 @@ import { type Collection, type CommandStartedEvent, MongoClient, - MongoDriverError + MongoDriverError, + MongoRuntimeError } from '../../mongodb'; import { sleep } from '../../tools/utils'; describe('Sessions Prose Tests', () => { + describe('5. Session argument is for the right client', () => { + let client1: MongoClient; + let client2: MongoClient; + beforeEach(async function () { + client1 = this.configuration.newClient(); + client2 = this.configuration.newClient(); + }); + + afterEach(async function () { + await client1?.close(); + await client2?.close(); + }); + + /** + * Steps: + * - Create client1 and client2 + * - Get database from client1 + * - Get collection from database + * - Start session from client2 + * - Call collection.insertOne(session,...) + * - Assert that an error was reported because session was not started from client1 + */ + context('when session is started from a different client than operation is being run on', () => + it('the operation throws a MongoRuntimeError', async () => { + const db = client1.db(); + const collection = db.collection('test'); + const session = client2.startSession(); + const error = await collection.insertOne({}, { session }).catch(error => error); + expect(error).to.be.instanceOf(MongoRuntimeError); + expect(error).to.match(/ClientSession must be from the same MongoClient/i); + }) + ); + }); + describe('14. Implicit sessions only allocate their server session after a successful connection checkout', () => { let client: MongoClient; let testCollection: Collection<{ _id: number; a?: number }>; From b7f9623f4f2c02ede5b5380ceff62a0d93b9b150 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 3 Aug 2023 14:14:23 -0400 Subject: [PATCH 2/5] feat: use MongoInvalidArgumentError --- src/operations/execute_operation.ts | 3 +- .../sessions/sessions.prose.test.ts | 38 ++++++++++++++----- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 5424858e0aa..e815f091a8b 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -7,6 +7,7 @@ import { MongoError, MongoErrorLabel, MongoExpiredSessionError, + MongoInvalidArgumentError, MongoNetworkError, MongoNotConnectedError, MongoRuntimeError, @@ -119,7 +120,7 @@ async function executeOperationAsync< } else if (session.snapshotEnabled && !topology.capabilities.supportsSnapshotReads) { throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later'); } else if (session.client !== client) { - throw new MongoRuntimeError('ClientSession must be from the same MongoClient'); + throw new MongoInvalidArgumentError('ClientSession must be from the same MongoClient'); } const readPreference = operation.readPreference ?? ReadPreference.primary; diff --git a/test/integration/sessions/sessions.prose.test.ts b/test/integration/sessions/sessions.prose.test.ts index 4015932575e..3fc41a5de43 100644 --- a/test/integration/sessions/sessions.prose.test.ts +++ b/test/integration/sessions/sessions.prose.test.ts @@ -7,7 +7,7 @@ import { type CommandStartedEvent, MongoClient, MongoDriverError, - MongoRuntimeError + MongoInvalidArgumentError } from '../../mongodb'; import { sleep } from '../../tools/utils'; @@ -33,16 +33,34 @@ describe('Sessions Prose Tests', () => { * - Start session from client2 * - Call collection.insertOne(session,...) * - Assert that an error was reported because session was not started from client1 + * + * This validation lives in our executeOperation layer so it applies universally. + * A find and an insert provide enough coverage, we determined we do not need to enumerate every possible operation. */ - context('when session is started from a different client than operation is being run on', () => - it('the operation throws a MongoRuntimeError', async () => { - const db = client1.db(); - const collection = db.collection('test'); - const session = client2.startSession(); - const error = await collection.insertOne({}, { session }).catch(error => error); - expect(error).to.be.instanceOf(MongoRuntimeError); - expect(error).to.match(/ClientSession must be from the same MongoClient/i); - }) + context( + 'when session is started from a different client than operation is being run on', + () => { + it('insertOne operation throws a MongoInvalidArgumentError', async () => { + const db = client1.db(); + const collection = db.collection('test'); + const session = client2.startSession(); + const error = await collection.insertOne({}, { session }).catch(error => error); + expect(error).to.be.instanceOf(MongoInvalidArgumentError); + expect(error).to.match(/ClientSession must be from the same MongoClient/i); + }); + + it('find operation throws a MongoInvalidArgumentError', async () => { + const db = client1.db(); + const collection = db.collection('test'); + const session = client2.startSession(); + const error = await collection + .find({}, { session }) + .toArray() + .catch(error => error); + expect(error).to.be.instanceOf(MongoInvalidArgumentError); + expect(error).to.match(/ClientSession must be from the same MongoClient/i); + }); + } ); }); From fe0381d881715b3703c1227943f089cddb35ffc1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 3 Aug 2023 14:20:16 -0400 Subject: [PATCH 3/5] test: legacy client --- test/integration/sessions/sessions.test.ts | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/integration/sessions/sessions.test.ts b/test/integration/sessions/sessions.test.ts index a9629a47e5a..79637af7cc0 100644 --- a/test/integration/sessions/sessions.test.ts +++ b/test/integration/sessions/sessions.test.ts @@ -1,4 +1,5 @@ import { expect } from 'chai'; +import { MongoClient as LegacyMongoClient } from 'mongodb-legacy'; import type { CommandStartedEvent, CommandSucceededEvent, MongoClient } from '../../mongodb'; import { LEGACY_HELLO_COMMAND, MongoServerError } from '../../mongodb'; @@ -422,4 +423,28 @@ describe('Sessions Spec', function () { expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(2); }); }); + + context.only('when using a LegacyMongoClient', () => { + let legacyClient; + beforeEach(async function () { + legacyClient = new LegacyMongoClient(this.configuration.url()); + }); + + afterEach(async function () { + await legacyClient?.close(); + }); + + it('insertOne accepts session started by legacy client', async () => { + const db = legacyClient.db(); + const collection = db.collection('test'); + const session = legacyClient.startSession(); + const error = await collection.insertOne({}, { session }).catch(error => error); + expect(error).to.not.be.instanceOf(Error); + }); + + it('session returned by legacy startSession has reference to legacyClient', async () => { + const session = legacyClient.startSession(); + expect(session).to.have.property('client', legacyClient); + }); + }); }); From 7958c38ce21b1d3591f58bf4049d3c4ffc779565 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 3 Aug 2023 14:26:56 -0400 Subject: [PATCH 4/5] lint --- test/integration/sessions/sessions.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/sessions/sessions.test.ts b/test/integration/sessions/sessions.test.ts index 79637af7cc0..7c2c90f6fd1 100644 --- a/test/integration/sessions/sessions.test.ts +++ b/test/integration/sessions/sessions.test.ts @@ -424,7 +424,7 @@ describe('Sessions Spec', function () { }); }); - context.only('when using a LegacyMongoClient', () => { + context('when using a LegacyMongoClient', () => { let legacyClient; beforeEach(async function () { legacyClient = new LegacyMongoClient(this.configuration.url()); From e0819431bd1d0c19c81b966e989f3eee582c5b13 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 3 Aug 2023 15:14:47 -0400 Subject: [PATCH 5/5] test: fix serverApi --- test/integration/sessions/sessions.test.ts | 5 ++++- test/tools/runner/config.ts | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/sessions/sessions.test.ts b/test/integration/sessions/sessions.test.ts index 7c2c90f6fd1..3bb00e181cc 100644 --- a/test/integration/sessions/sessions.test.ts +++ b/test/integration/sessions/sessions.test.ts @@ -427,7 +427,10 @@ describe('Sessions Spec', function () { context('when using a LegacyMongoClient', () => { let legacyClient; beforeEach(async function () { - legacyClient = new LegacyMongoClient(this.configuration.url()); + const options = this.configuration.serverApi + ? { serverApi: this.configuration.serverApi } + : {}; + legacyClient = new LegacyMongoClient(this.configuration.url(), options); }); afterEach(async function () { diff --git a/test/tools/runner/config.ts b/test/tools/runner/config.ts index 2f190fe3b1d..375b909a744 100644 --- a/test/tools/runner/config.ts +++ b/test/tools/runner/config.ts @@ -7,6 +7,7 @@ import { type AuthMechanism, HostAddress, MongoClient, + type ServerApi, TopologyType, type WriteConcernSettings } from '../../mongodb'; @@ -71,7 +72,7 @@ export class TestConfiguration { auth?: { username: string; password: string; authSource?: string }; proxyURIParams?: ProxyParams; }; - serverApi: string; + serverApi: ServerApi; constructor(uri: string, context: Record) { const url = new ConnectionString(uri);