Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
robertbullen opened this issue Jun 5, 2019 · 3 comments · May be fixed by #21
Closed

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

robertbullen opened this issue Jun 5, 2019 · 3 comments · May be fixed by #21

Comments

@robertbullen
Copy link

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
Copy link
Author

robertbullen commented Jun 6, 2019 •
edited
Loading

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 robertbullen mentioned this issue Jun 6, 2019
Open
@robertbullen
Copy link
Author

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

@buccfer
Copy link

buccfer commented Aug 5, 2019

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

@ghdna ghdna closed this as completed Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ghdna @robertbullen @buccfer