From 6e0ac57755a9bfdebb975f16e21ec984f27b2bf5 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Sat, 11 Aug 2018 18:38:40 +0200 Subject: [PATCH] feat: authorization response parameter checking based on response_type passing checks.response_type will now validate that parameters that are normatively REQUIRED are present. --- README.md | 9 +- example/app.js | 31 ++-- lib/client.js | 65 +++++++- lib/helpers/consts.js | 1 + test/client/client_instance.test.js | 223 ++++++++++++++++++++++------ 5 files changed, 260 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 7e5b61b4..b9a0d5bf 100644 --- a/README.md +++ b/README.md @@ -156,19 +156,18 @@ client.authorizationCallback('https://client.example.com/callback', request.quer }); ``` -### Processing callback with state, nonce or max_age check +### Processing callback with required params, state, nonce or max_age checks (recommended) ```js -const state = session.state; -const nonce = session.nonce; +const { state, nonce, max_age, response_type } = session[authorizationRequestState]; -client.authorizationCallback('https://client.example.com/callback', request.query, { state, nonce, max_age }) // => Promise +client.authorizationCallback('https://client.example.com/callback', request.query, { state, nonce, max_age, response_type }) // => Promise .then(function (tokenSet) { console.log('received and validated tokens %j', tokenSet); console.log('validated id_token claims %j', tokenSet.claims); }); ``` -### IdP Errors - OpenIdConnectError +### OP Errors - OpenIdConnectError When the OpenID Provider returns an OIDC formatted error from either authorization callbacks or any of the JSON responses the library will reject a given Promise with `OpenIdConnectError` instance. diff --git a/example/app.js b/example/app.js index 6345e98e..b94d8734 100644 --- a/example/app.js +++ b/example/app.js @@ -1,4 +1,4 @@ -/* eslint-disable import/no-extraneous-dependencies */ +/* eslint-disable import/no-extraneous-dependencies, camelcase */ const crypto = require('crypto'); const url = require('url'); @@ -166,20 +166,23 @@ module.exports = (issuer) => { }); router.get('/login', async (ctx, next) => { - ctx.session.state = crypto.randomBytes(16).toString('hex'); - ctx.session.nonce = crypto.randomBytes(16).toString('hex'); + const state = crypto.randomBytes(16).toString('hex'); + const nonce = crypto.randomBytes(16).toString('hex'); + ctx.session.auth_request = { state, nonce }; const { client } = ctx.session; - const authorizationRequest = Object.assign({ + const authorization_request = Object.assign({ redirect_uri: url.resolve(ctx.href, 'cb'), scope: 'openid profile email address phone', - state: ctx.session.state, - nonce: ctx.session.nonce, + state, + nonce, response_type: client.response_types[0], }, ctx.session.authorization_params); - ctx.redirect(client.authorizationUrl(authorizationRequest)); + ctx.session.auth_request.response_type = authorization_request.response_type; + + ctx.redirect(client.authorizationUrl(authorization_request)); return next(); }); @@ -210,11 +213,10 @@ module.exports = (issuer) => { return ctx.render('repost', { layout: false }); } - const { state, nonce } = ctx.session; - delete ctx.session.state; - delete ctx.session.nonce; + const { state, nonce, response_type } = ctx.session.auth_request; + delete ctx.session.auth_request; - ctx.session.tokenset = await client.authorizationCallback(url.resolve(ctx.href, 'cb'), params, { nonce, state }); + ctx.session.tokenset = await client.authorizationCallback(url.resolve(ctx.href, 'cb'), params, { nonce, state, response_type }); ctx.redirect('/user'); @@ -222,13 +224,12 @@ module.exports = (issuer) => { }); router.post('/cb', body({ patchNode: true }), async (ctx, next) => { - const { state, nonce } = ctx.session; - delete ctx.session.state; - delete ctx.session.nonce; + const { state, nonce, response_type } = ctx.session.auth_request; + delete ctx.session.auth_request; const { client } = ctx.session; const params = client.callbackParams(ctx.request.req); - ctx.session.tokenset = await client.authorizationCallback(url.resolve(ctx.href, 'cb'), params, { nonce, state }); + ctx.session.tokenset = await client.authorizationCallback(url.resolve(ctx.href, 'cb'), params, { nonce, state, response_type }); ctx.redirect('/user'); diff --git a/lib/client.js b/lib/client.js index a80bee4c..074e373b 100644 --- a/lib/client.js +++ b/lib/client.js @@ -320,7 +320,15 @@ class Client { if (this.default_max_age && !checks.max_age) checks.max_age = this.default_max_age; - if (checks.state !== parameters.state) { + if (!params.state && checks.state) { + return Promise.reject(new Error('state missing from response')); + } + + if (params.state && !checks.state) { + return Promise.reject(new Error('checks.state missing')); + } + + if (checks.state !== params.state) { return Promise.reject(new Error('state mismatch')); } @@ -328,6 +336,28 @@ class Client { return Promise.reject(new OpenIdConnectError(params)); } + const RESPONSE_TYPE_REQUIRED_PARAMS = { + code: ['code'], + id_token: ['id_token'], + token: ['access_token', 'token_type'], + }; + + if (checks.response_type) { + for (const type of checks.response_type.split(' ')) { // eslint-disable-line no-restricted-syntax + if (type === 'none') { + if (params.code || params.id_token || params.access_token) { + return Promise.reject(new Error('unexpected params encountered for "none" response')); + } + } else { + for (const param of RESPONSE_TYPE_REQUIRED_PARAMS[type]) { // eslint-disable-line no-restricted-syntax, max-len + if (!params[param]) { + return Promise.reject(new Error(`${param} missing from response`)); + } + } + } + } + } + let promise; if (params.id_token) { @@ -368,7 +398,15 @@ class Client { oauthCallback(redirectUri, parameters, checks = {}) { const params = _.pick(parameters, CALLBACK_PROPERTIES); - if (checks.state !== parameters.state) { + if (!params.state && checks.state) { + return Promise.reject(new Error('state missing from response')); + } + + if (params.state && !checks.state) { + return Promise.reject(new Error('checks.state missing')); + } + + if (checks.state !== params.state) { return Promise.reject(new Error('state mismatch')); } @@ -376,6 +414,29 @@ class Client { return Promise.reject(new OpenIdConnectError(params)); } + const RESPONSE_TYPE_REQUIRED_PARAMS = { + code: ['code'], + token: ['access_token', 'token_type'], + }; + + if (checks.response_type) { + for (const type of checks.response_type.split(' ')) { // eslint-disable-line no-restricted-syntax + if (type === 'none') { + if (params.code || params.id_token || params.access_token) { + return Promise.reject(new Error('unexpected params encountered for "none" response')); + } + } + + if (RESPONSE_TYPE_REQUIRED_PARAMS[type]) { + for (const param of RESPONSE_TYPE_REQUIRED_PARAMS[type]) { // eslint-disable-line no-restricted-syntax, max-len + if (!params[param]) { + return Promise.reject(new Error(`${param} missing from response`)); + } + } + } + } + } + if (params.code) { return this.grant({ grant_type: 'authorization_code', diff --git a/lib/helpers/consts.js b/lib/helpers/consts.js index aac0e7b2..9e4c6d7c 100644 --- a/lib/helpers/consts.js +++ b/lib/helpers/consts.js @@ -30,6 +30,7 @@ const CALLBACK_PROPERTIES = [ 'code', 'error', 'error_description', + 'error_uri', 'expires_in', 'id_token', 'state', diff --git a/test/client/client_instance.test.js b/test/client/client_instance.test.js index 3b4e3b9d..2b9576e0 100644 --- a/test/client/client_instance.test.js +++ b/test/client/client_instance.test.js @@ -262,35 +262,108 @@ const encode = object => base64url.encode(JSON.stringify(object)); }); }); - it('rejects with an Error when states mismatch (returned)', function () { - return this.client.authorizationCallback('https://rp.example.com/cb', { - state: 'should be checked for this', - }).then(fail, (error) => { - expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); + describe('state checks', function () { + it('rejects with an Error when states mismatch (returned)', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + state: 'should be checked for this', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'checks.state missing'); + }); + }); + + it('rejects with an Error when states mismatch (not returned)', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', {}, { + state: 'should be this', + }) + .then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'state missing from response'); + }); + }); + + it('rejects with an Error when states mismatch (general mismatch)', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + state: 'is this', + }, { + state: 'should be this', + }) + .then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'state mismatch'); + }); }); }); - it('rejects with an Error when states mismatch (not returned)', function () { - return this.client.authorizationCallback('https://rp.example.com/cb', {}, { - state: 'should be this', - }) - .then(fail, (error) => { + describe('response type checks', function () { + it('rejects with an Error when code is missing', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + // code: 'foo', + access_token: 'foo', + token_type: 'Bearer', + id_token: 'foo', + }, { + response_type: 'code id_token token', + }).then(fail, (error) => { expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); + expect(error).to.have.property('message', 'code missing from response'); }); - }); + }); - it('rejects with an Error when states mismatch (general mismatch)', function () { - return this.client.authorizationCallback('https://rp.example.com/cb', { - state: 'is this', - }, { - state: 'should be this', - }) - .then(fail, (error) => { + it('rejects with an Error when id_token is missing', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + code: 'foo', + access_token: 'foo', + token_type: 'Bearer', + // id_token: 'foo', + }, { + response_type: 'code id_token token', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'id_token missing from response'); + }); + }); + + it('rejects with an Error when token_type is missing', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + code: 'foo', + access_token: 'foo', + // token_type: 'Bearer', + id_token: 'foo', + }, { + response_type: 'code id_token token', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'token_type missing from response'); + }); + }); + + it('rejects with an Error when access_token is missing', function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + code: 'foo', + // access_token: 'foo', + token_type: 'Bearer', + id_token: 'foo', + }, { + response_type: 'code id_token token', + }).then(fail, (error) => { expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); + expect(error).to.have.property('message', 'access_token missing from response'); + }); + }); + + ['code', 'access_token', 'id_token'].forEach((param) => { + it(`rejects with an Error when ${param} is encoutered during "none" response`, function () { + return this.client.authorizationCallback('https://rp.example.com/cb', { + [param]: 'foo', + }, { + response_type: 'none', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'unexpected params encountered for "none" response'); + }); }); + }); }); }); @@ -337,6 +410,60 @@ const encode = object => base64url.encode(JSON.stringify(object)); }); }); + describe('response type checks', function () { + it('rejects with an Error when code is missing', function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + // code: 'foo', + access_token: 'foo', + token_type: 'Bearer', + }, { + response_type: 'code token', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'code missing from response'); + }); + }); + + it('rejects with an Error when token_type is missing', function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + code: 'foo', + access_token: 'foo', + // token_type: 'Bearer', + }, { + response_type: 'code token', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'token_type missing from response'); + }); + }); + + it('rejects with an Error when access_token is missing', function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + code: 'foo', + // access_token: 'foo', + token_type: 'Bearer', + }, { + response_type: 'code token', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'access_token missing from response'); + }); + }); + + ['code', 'access_token'].forEach((param) => { + it(`rejects with an Error when ${param} is encoutered during "none" response`, function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + [param]: 'foo', + }, { + response_type: 'none', + }).then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'unexpected params encountered for "none" response'); + }); + }); + }); + }); + it('rejects with OpenIdConnectError when part of the response', function () { return this.client.oauthCallback('https://rp.example.com/cb', { error: 'invalid_request', @@ -346,35 +473,37 @@ const encode = object => base64url.encode(JSON.stringify(object)); }); }); - it('rejects with an Error when states mismatch (returned)', function () { - return this.client.oauthCallback('https://rp.example.com/cb', { - state: 'should be checked for this', - }).then(fail, (error) => { - expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); - }); - }); - - it('rejects with an Error when states mismatch (not returned)', function () { - return this.client.oauthCallback('https://rp.example.com/cb', {}, { - state: 'should be this', - }) - .then(fail, (error) => { + describe('state checks', function () { + it('rejects with an Error when states mismatch (returned)', function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + state: 'should be checked for this', + }).then(fail, (error) => { expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); + expect(error).to.have.property('message', 'checks.state missing'); }); - }); + }); - it('rejects with an Error when states mismatch (general mismatch)', function () { - return this.client.oauthCallback('https://rp.example.com/cb', { - state: 'is this', - }, { - state: 'should be this', - }) - .then(fail, (error) => { - expect(error).to.be.instanceof(Error); - expect(error).to.have.property('message', 'state mismatch'); - }); + it('rejects with an Error when states mismatch (not returned)', function () { + return this.client.oauthCallback('https://rp.example.com/cb', {}, { + state: 'should be this', + }) + .then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'state missing from response'); + }); + }); + + it('rejects with an Error when states mismatch (general mismatch)', function () { + return this.client.oauthCallback('https://rp.example.com/cb', { + state: 'is this', + }, { + state: 'should be this', + }) + .then(fail, (error) => { + expect(error).to.be.instanceof(Error); + expect(error).to.have.property('message', 'state mismatch'); + }); + }); }); });