CognitoExpress.validate()'s callback parameter is optional, but is blindly invoked for error conditions

[robertbullen]

This means that the promisified code path in CognitoExpress.validate() is fundamentally broken for failure scenarios.

Here are the offending lines:

cognito-express/lib/strategy.js

Lines 52 to 63 in 86c7386

	if (!decodedJwt) return callback(`Not a valid JWT token`, null);
	if (decodedJwt.payload.iss !== this.iss)
	return callback(`token is not from your User Pool`, null);
	if (decodedJwt.payload.token_use !== this.tokenUse)
	return callback(`Not an ${this.tokenUse} token`, null);
	let kid = decodedJwt.header.kid;
	let pem = this.pems[kid];
	if (!pem) return callback(`Invalid ${this.tokenUse} token`, null);

Meanwhile, just a little further down callback is properly checked in two locations:

cognito-express/lib/strategy.js

Lines 72 to 89 in 86c7386

	if (callback) {
	jwtVerify(params, callback);
	} else {
	return new Promise((resolve, reject) => {
	jwtVerify(params, (err, result) => {
	if (err) {
	reject(err);
	} else {
	resolve(result);
	}
	});
	});
	}
	});
	if (!callback) {
	return p;
	}

[robertbullen]

The unit tests mask this bug because of how they are passing an async callback:

cognito-express/test/strategy.js

Lines 121 to 130 in 86c7386

	it("should check if Validate function can fail successfully when invalid token is passed (Promise)", async () => {
	await strategy.init(async callback => {
	try {
	await strategy.validate("token");
	expect(true).to.eql(false);
	} catch (err) {
	expect(err).to.eql("Not a valid JWT token");
	};
	});
	});

Because the callback is async, it will be executed after the strategy.init() promise, and hence the test itself, resolves. This means that the expect assertions occur outside of the detection of chai.

Restructuring the test as follows:

	it("should check if Validate function can fail successfully when invalid token is passed (Promise)", async () => {
		await strategy.init(result => expect(result).to.eql(true));
		try {
			await strategy.validate("token");
		} catch (err) {
			expect(err).to.eql("Not a valid JWT token");
		};
	});

Surfaces the failure:

  Strategy Positive Scenarios
    ✓ should check if GeneratePem function exists
    ✓ should check if jwtVerify function exists
    ✓ should check if configurationIsCorrect function exists
    ✓ should check if Validate function exists
    ✓ should check if Strategy can initialized successfully (208ms)
    ✓ should check if Validate function can fail successfully when invalid token is passed (129ms)
    1) should check if Validate function can fail successfully when invalid token is passed (Promise)
    ✓ should check if Validate function can execute successfully by detecting an expired token (134ms)


  14 passing (632ms)
  1 failing

  1) Strategy Positive Scenarios
       should check if Validate function can fail successfully when invalid token is passed (Promise):
     AssertionError: expected [TypeError: callback is not a function] to deeply equal 'Not a valid JWT token'
      at Context.it (test/strategy.js:126:19)
      at <anonymous>

[robertbullen]

@ghdna, do you have any thoughts on or plans for this PR?

[buccfer]

@robertbullen Since issues like this one have been open for so long I created a different library aws-cognito-express.