Skip to content

Commit

Permalink
postLogoutRedirectUri isn't always a URI and login needs a check fo…
Browse files Browse the repository at this point in the history
…r `response_type` (#123)
  • Loading branch information
adamjmcgrath authored Aug 10, 2020
1 parent bb2e36a commit 6197b66
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 13 deletions.
6 changes: 2 additions & 4 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ interface LoginOptions {
*/
interface LogoutOptions {
/**
* URL to returnTo after logout, overrides the Default in {@link ConfigParams.routes.postLogoutRedirectUri routes.postLogoutRedirectUri}
* URL to returnTo after logout, overrides the Default in {@link ConfigParams.routes.postLogoutRedirect routes.postLogoutRedirect}
*/
returnTo?: string;
}
Expand Down Expand Up @@ -376,7 +376,7 @@ interface ConfigParams {
* This value must be registered on the authorization server.
* The user will be redirected to this after a logout has been performed.
*/
postLogoutRedirectUri?: string;
postLogoutRedirect?: string;

/**
* Relative path to the application callback to process the response from the authorization server.
Expand Down Expand Up @@ -452,8 +452,6 @@ interface SessionConfigParams {
* Defaults to "Lax" but will be adjusted based on {@link AuthorizationParameters.response_type}.
*/
sameSite?: string;

// TODO do we need a path option?
}

/**
Expand Down
4 changes: 1 addition & 3 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,7 @@ const paramsSchema = Joi.object({
Joi.boolean().valid(false),
]).default('/logout'),
callback: Joi.string().uri({ relativeOnly: true }).default('/callback'),
postLogoutRedirectUri: Joi.string()
.uri({ allowRelative: true })
.default(''), //_ TODO: find a better name
postLogoutRedirect: Joi.string().uri({ allowRelative: true }).default(''),
})
.default()
.unknown(false),
Expand Down
8 changes: 6 additions & 2 deletions lib/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,11 @@ class ResponseContext {
: undefined),
};

// TODO: validate response_type / response_mode?
const validResponseTypes = ['id_token', 'code id_token', 'code'];
assert(
validResponseTypes.includes(authParams.response_type),
`response_type should be one of ${validResponseTypes.join(', ')}`
);
assert(
/\bopenid\b/.test(authParams.scope),
'scope should contain "openid"'
Expand Down Expand Up @@ -230,7 +234,7 @@ class ResponseContext {
next = cb(next).once();
const client = await getClient(config);

let returnURL = params.returnTo || config.routes.postLogoutRedirectUri;
let returnURL = params.returnTo || config.routes.postLogoutRedirect;
debug('req.oidc.logout() with return url: %s', returnURL);

if (url.parse(returnURL).host === null) {
Expand Down
25 changes: 25 additions & 0 deletions test/login.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,31 @@ describe('auth', () => {
assert.equal(res.body.err.message, 'scope should contain "openid"');
});

it('should not allow an invalid response_type', async function () {
const router = auth({ ...defaultConfig, routes: { login: false } });
router.get('/login', (req, res) => {
res.oidc.login({
authorizationParams: {
response_type: 'invalid',
},
});
});
server = await createServer(router);

const cookieJar = request.jar();
const res = await request.get('/login', {
cookieJar,
baseUrl,
json: true,
followRedirect: false,
});
assert.equal(res.statusCode, 500);
assert.equal(
res.body.err.message,
'response_type should be one of id_token, code id_token, code'
);
});

it('should use a custom state builder', async () => {
server = await createServer(
auth({
Expand Down
8 changes: 4 additions & 4 deletions test/logout.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,12 @@ describe('logout route', async () => {
);
});

it('should redirect to postLogoutRedirectUri', async () => {
it('should redirect to postLogoutRedirect', async () => {
server = await createServer(
auth({
...defaultConfig,
routes: {
postLogoutRedirectUri: '/after-logout-in-auth-config',
postLogoutRedirect: '/after-logout-in-auth-config',
},
})
);
Expand All @@ -143,7 +143,7 @@ describe('logout route', async () => {
{
location: 'https://example.org/after-logout-in-auth-config',
},
'should redirect to postLogoutRedirectUri'
'should redirect to postLogoutRedirect'
);
});

Expand All @@ -152,7 +152,7 @@ describe('logout route', async () => {
...defaultConfig,
routes: {
logout: false,
postLogoutRedirectUri: '/after-logout-in-auth-config',
postLogoutRedirect: '/after-logout-in-auth-config',
},
});
server = await createServer(router);
Expand Down

0 comments on commit 6197b66

Please sign in to comment.