Skip to content

Commit

Permalink
fix: gracefully handle mixed up response_type(s) order
Browse files Browse the repository at this point in the history
  • Loading branch information
panva committed Nov 21, 2018
1 parent 3d240d9 commit b775591
Show file tree
Hide file tree
Showing 9 changed files with 126 additions and 4 deletions.
2 changes: 2 additions & 0 deletions lib/actions/authorization/check_response_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ module.exports = provider => async function checkResponseType(ctx, next) {
const { params } = ctx.oidc;
const supported = instance(provider).configuration('responseTypes');

params.response_type = Array.from(new Set(params.response_type.split(' '))).sort().join(' ');

if (!supported.includes(params.response_type)) {
throw new UnsupportedResponseType();
}
Expand Down
5 changes: 5 additions & 0 deletions lib/helpers/client_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ module.exports = function getSchema(provider) {
this.whens();
this.arrays();
this.strings();
this.normalizeResponseTypes();
this.enums();
this.webUris();
this.postLogoutRedirectUris();
Expand Down Expand Up @@ -416,6 +417,10 @@ module.exports = function getSchema(provider) {
});
}

normalizeResponseTypes() {
this.response_types = this.response_types.map(type => Array.from(new Set(type.split(' '))).sort().join(' '));
}

normalizeNativeAppUris() {
if (this.application_type === 'web') return;
if (!features.oauthNativeApps) return;
Expand Down
22 changes: 22 additions & 0 deletions lib/helpers/configuration_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ function filterHSandNone(alg) {
return alg.startsWith('HS') || alg === 'none';
}

const supportedResponseTypes = new Set(['none', 'code', 'id_token', 'token']);

module.exports = class ConfigurationSchema {
constructor(config) {
authEndpointDefaults(config);
Expand Down Expand Up @@ -78,6 +80,7 @@ module.exports = class ConfigurationSchema {
}

this.ensureMaps();
this.fixResponseTypes();
this.checkWhitelistedAlgs();
this.collectScopes();
this.unpackArrayClaims();
Expand All @@ -94,6 +97,25 @@ module.exports = class ConfigurationSchema {
}
}

fixResponseTypes() {
const types = new Set();

this.responseTypes.forEach((type) => {
const parsed = new Set(type.split(' '));

if (
(parsed.has('none') && parsed.size !== 1)
|| !Array.from(parsed).every(Set.prototype.has.bind(supportedResponseTypes))
) {
throw new Error(`unsupported response type: ${type}`);
}

types.add(Array.from(parsed).sort().join(' '));
});

this.responseTypes = Array.from(types);
}

collectGrantTypes() {
this.grantTypes = new Set();

Expand Down
13 changes: 9 additions & 4 deletions test/configuration/client_metadata.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,15 @@ describe('Client metadata validation', () => {
});
};

const allows = (prop, value, meta, configuration) => {
const allows = (prop, value, meta, configuration, assertion = (client) => {
expect(client.metadata()[prop]).to.eql(value);
}) => {
let msg = util.format('passes %j', value);
if (meta) msg = util.format(`${msg}, [client %j]`, omit(meta, ['jwks.keys']));
if (configuration) msg = util.format(`${msg}, [provider %j]`, configuration);
it(msg, () => addClient(Object.assign({}, meta, {
[prop]: value,
}), configuration).then((client) => {
expect(client.metadata()[prop]).to.eql(value);
}, (err) => {
}), configuration).then(assertion, (err) => {
if (err instanceof InvalidClientMetadata) {
throw new Error(`InvalidClientMetadata received ${err.message} ${err.error_description}`);
}
Expand Down Expand Up @@ -431,6 +431,11 @@ describe('Client metadata validation', () => {
allows(this.title, responseTypes, {
grant_types: ['implicit', 'authorization_code'],
});
allows(this.title, ['token id_token'], { // mixed up order
grant_types: ['implicit'],
}, undefined, (client) => {
expect(client.metadata().response_types).to.eql(['id_token token']);
});

rejects(this.title, [123], /must only contain strings$/);
rejects(this.title, [], /must contain members$/);
Expand Down
28 changes: 28 additions & 0 deletions test/configuration/response_types.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
const { expect } = require('chai');

const Provider = require('../../lib');

describe('response_types Provider configuration', () => {
it('fixes common issues', () => {
const provider = new Provider('https://op.example.com', { // eslint-disable-line no-new
responseTypes: ['token id_token code', 'token id_token'],
});
expect(i(provider).configuration('responseTypes')).to.eql(['code id_token token', 'id_token token']);
});

it('throws when invalid types are configured', () => {
expect(() => {
new Provider('https://op.example.com', { // eslint-disable-line no-new
responseTypes: ['id_token tokencode'],
});
}).to.throw('unsupported response type: id_token tokencode');
});

it('validates none is always standalone', () => {
expect(() => {
new Provider('https://op.example.com', { // eslint-disable-line no-new
responseTypes: ['none code'],
});
}).to.throw('unsupported response type: none code');
});
});
16 changes: 16 additions & 0 deletions test/core/hybrid/code+id_token+token.authorization.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,22 @@ describe('HYBRID code+id_token+token', () => {
expect(await this.provider.AccessToken.find(auth.res.access_token)).to.have.property('gty', 'implicit');
});

it('handles mixed up response_type order', async function () {
const auth = new this.AuthorizationRequest({
response_type: 'id_token token code',
scope,
});

await this.wrap({ route, verb, auth })
.expect(302)
.expect(auth.validateFragment)
.expect(auth.validatePresence(['code', 'id_token', 'state', 'access_token', 'expires_in', 'token_type']))
.expect(auth.validateState)
.expect(auth.validateClientLocation);

expect(await this.provider.AccessToken.find(auth.res.access_token)).to.have.property('gty', 'implicit');
});

it('populates ctx.oidc.entities', function (done) {
this.provider.use(this.assertOnce((ctx) => {
expect(ctx.oidc.entities).to.have.keys('Client', 'Account', 'AuthorizationCode', 'AccessToken');
Expand Down
14 changes: 14 additions & 0 deletions test/core/hybrid/code+id_token.authorization.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ describe('HYBRID code+id_token', () => {
.expect(auth.validateClientLocation);
});

it('handles mixed up response_type order', function () {
const auth = new this.AuthorizationRequest({
response_type: 'id_token code',
scope,
});

return this.wrap({ route, verb, auth })
.expect(302)
.expect(auth.validateFragment)
.expect(auth.validatePresence(['code', 'id_token', 'state']))
.expect(auth.validateState)
.expect(auth.validateClientLocation);
});

it('populates ctx.oidc.entities', function (done) {
this.provider.use(this.assertOnce((ctx) => {
expect(ctx.oidc.entities).to.have.keys('Client', 'Account', 'AuthorizationCode');
Expand Down
14 changes: 14 additions & 0 deletions test/core/hybrid/code+token.authorization.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ describe('HYBRID code+token', () => {
.expect(auth.validateClientLocation);
});

it('handles mixed up response_type order', function () {
const auth = new this.AuthorizationRequest({
response_type: 'token code',
scope,
});

return this.wrap({ route, verb, auth })
.expect(302)
.expect(auth.validateFragment)
.expect(auth.validatePresence(['code', 'state', 'access_token', 'expires_in', 'token_type']))
.expect(auth.validateState)
.expect(auth.validateClientLocation);
});

it('populates ctx.oidc.entities', function (done) {
this.provider.use(this.assertOnce((ctx) => {
expect(ctx.oidc.entities).to.have.keys('Client', 'Account', 'AuthorizationCode', 'AccessToken');
Expand Down
16 changes: 16 additions & 0 deletions test/core/implicit/id_token+token.authorization.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,22 @@ describe('IMPLICIT id_token+token', () => {
expect(await this.provider.AccessToken.find(auth.res.access_token)).to.have.property('gty', 'implicit');
});

it('handles mixed up response_type order', async function () {
const auth = new this.AuthorizationRequest({
response_type: 'token id_token',
scope,
});

await this.wrap({ route, verb, auth })
.expect(302)
.expect(auth.validateFragment)
.expect(auth.validatePresence(['id_token', 'state', 'access_token', 'expires_in', 'token_type']))
.expect(auth.validateState)
.expect(auth.validateClientLocation);

expect(await this.provider.AccessToken.find(auth.res.access_token)).to.have.property('gty', 'implicit');
});

it('populates ctx.oidc.entities', function (done) {
this.provider.use(this.assertOnce((ctx) => {
expect(ctx.oidc.entities).to.have.keys('Client', 'Account', 'AccessToken');
Expand Down

0 comments on commit b775591

Please sign in to comment.