Skip to content

Commit e46a70e

Browse files
committed
fix: always check for network errors during SCRAM conversation
A recent change to verify proper server signatures during SCRAM conversations introduced a subtle bug where not checking for network errors could lead to a type error while trying to access the `payload` of the result. NODE-2390
1 parent e44f553 commit e46a70e

File tree

2 files changed

+44
-1
lines changed

2 files changed

+44
-1
lines changed

lib/core/auth/scram.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ class ScramSHA extends AuthProvider {
236236
};
237237

238238
sendAuthCommand(connection, `${db}.$cmd`, saslContinueCmd, (err, r) => {
239-
if (r && typeof r.ok === 'number' && r.ok === 0) {
239+
if (err || (r && typeof r.ok === 'number' && r.ok === 0)) {
240240
callback(err, r);
241241
return;
242242
}

test/unit/core/scram_iterations.test.js

+43
Original file line numberDiff line numberDiff line change
@@ -112,4 +112,47 @@ describe('SCRAM Iterations Tests', function() {
112112

113113
client.connect();
114114
});
115+
116+
it('should properly handle network errors on `saslContinue`', function(_done) {
117+
const credentials = new MongoCredentials({
118+
mechanism: 'default',
119+
source: 'db',
120+
username: 'user',
121+
password: 'pencil'
122+
});
123+
124+
let done = e => {
125+
done = () => {};
126+
return _done(e);
127+
};
128+
129+
test.server.setMessageHandler(request => {
130+
const doc = request.document;
131+
if (doc.ismaster) {
132+
return request.reply(Object.assign({}, mock.DEFAULT_ISMASTER));
133+
} else if (doc.saslStart) {
134+
return request.reply({
135+
ok: 1,
136+
done: false,
137+
payload: Buffer.from(
138+
'r=VNnXkRqKflB5+rmfnFiisCWzgDLzez02iRpbvE5mQjMvizb+VkSPRZZ/pDmFzLxq,s=dZTyOb+KZqoeTFdsULiqow==,i=10000'
139+
)
140+
});
141+
} else if (doc.saslContinue) {
142+
request.connection.destroy();
143+
}
144+
});
145+
146+
const client = new Server(Object.assign({}, test.server.address(), { credentials }));
147+
client.on('error', err => {
148+
expect(err).to.not.be.null;
149+
expect(err)
150+
.to.have.property('message')
151+
.that.matches(/failed to connect to server/);
152+
153+
client.destroy(done);
154+
});
155+
156+
client.connect();
157+
});
115158
});

0 commit comments

Comments
 (0)