From 029870ef0176ed94740872fa0af462e91ab6d3d6 Mon Sep 17 00:00:00 2001 From: Johann150 Date: Mon, 7 Nov 2022 00:10:48 +0100 Subject: [PATCH] check redirect URIs --- .../src/server/api/common/compare-url.ts | 34 +++++++++++++++++++ .../backend/src/server/api/common/oauth.ts | 15 ++++++-- .../api/endpoints/auth/session/generate.ts | 16 +++++++-- packages/client/src/pages/auth.vue | 2 ++ 4 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 packages/backend/src/server/api/common/compare-url.ts diff --git a/packages/backend/src/server/api/common/compare-url.ts b/packages/backend/src/server/api/common/compare-url.ts new file mode 100644 index 000000000..083f9fffb --- /dev/null +++ b/packages/backend/src/server/api/common/compare-url.ts @@ -0,0 +1,34 @@ +import { URL } from 'node:url'; + +/** + * Compares two URLs for OAuth. The first parameter is the trusted URL + * which decides how the comparison is conducted. + * + * Implements the current draft-ietf-oauth-security-topics-21 ยง 4.1.3 + * (published 2022-09-27) + */ +export function compareUrl(trusted: string, untrusted: string): boolean { + let trustedUrl = new URL(trusted); + let untrustedUrl = new URL(untrusted); + + // Excerpt from RFC 8252: + //> Loopback redirect URIs use the "http" scheme and are constructed with + //> the loopback IP literal and whatever port the client is listening on. + //> That is, "http://127.0.0.1:{port}/{path}" for IPv4, and + //> "http://[::1]:{port}/{path}" for IPv6. + // + // To be nice we also include the "localhost" name, since it is required + // to resolve to one of the other two. + if (trustedUrl.protocol === 'http:' && ['localhost', '127.0.0.1', '[::1]'].includes(trustedUrl.host)) { + // localhost comparisons should ignore port number + trustedUrl.port = ''; + untrustedUrl.port = ''; + } + + // security recommendation is to just compare the (normalized) string + //> This document therefore advises to simplify the required logic and configuration + //> by using exact redirect URI matching. This means the authorization server MUST + //> compare the two URIs using simple string comparison as defined in [RFC3986], + //> Section 6.2.1. + return trustedUrl.href === untrustedUrl.href; +} diff --git a/packages/backend/src/server/api/common/oauth.ts b/packages/backend/src/server/api/common/oauth.ts index fcf235d08..bef5ee919 100644 --- a/packages/backend/src/server/api/common/oauth.ts +++ b/packages/backend/src/server/api/common/oauth.ts @@ -1,15 +1,14 @@ +import * as crypto from 'node:crypto'; import Koa from 'koa'; import { IsNull, Not } from 'typeorm'; import { Apps, AuthSessions, AccessTokens } from '@/models/index.js'; import config from '@/config/index.js'; +import { compareUrl } from './compare-url.js'; export async function oauth(ctx: Koa.Context): void { const { grant_type, code, - // TODO: check redirect_uri - // since this is also not checked in the legacy app authentication - // it seems pointless to check it here, and it is also not stored. redirect_uri, } = ctx.request.body; @@ -80,6 +79,16 @@ export async function oauth(ctx: Koa.Context): void { return; } + // check redirect URI + if (!compareUrl(app.callbackUrl, redirect_uri)) { + ctx.response.status = 400; + ctx.response.body = { + error: 'invalid_grant', + error_description: 'Mismatched redirect_uri', + }; + return; + } + // session is single use await AuthSessions.delete(session.id), diff --git a/packages/backend/src/server/api/endpoints/auth/session/generate.ts b/packages/backend/src/server/api/endpoints/auth/session/generate.ts index e1b498a49..0c06c5895 100644 --- a/packages/backend/src/server/api/endpoints/auth/session/generate.ts +++ b/packages/backend/src/server/api/endpoints/auth/session/generate.ts @@ -2,6 +2,7 @@ import { v4 as uuid } from 'uuid'; import config from '@/config/index.js'; import { Apps, AuthSessions } from '@/models/index.js'; import { genId } from '@/misc/gen-id.js'; +import { compareUrl } from '@/server/api/common/compare-url.js'; import define from '../../../define.js'; import { ApiError } from '../../../error.js'; @@ -43,14 +44,17 @@ export const meta = { } as const; export const paramDef = { + type: 'object', oneOf: [{ - type: 'object', properties: { clientId: { type: 'string' }, + callbackUrl: { + type: 'string', + minLength: 1, + }, }, required: ['clientId'] }, { - type: 'object', properties: { appSecret: { type: 'string' }, }, @@ -71,6 +75,14 @@ export default define(meta, paramDef, async (ps) => { throw new ApiError('NO_SUCH_APP'); } + // check URL if provided + // technically the OAuth specification says that the redirect URI has to be + // bound with the token request, but since an app may only register one + // redirect URI, we don't actually have to store that. + if (ps.callbackUrl && !compareUrl(app.callbackUrl, ps.callbackUrl)) { + throw new ApiError('NO_SUCH_APP', 'redirect URI mismatch'); + } + // Generate token const token = uuid(); const id = genId(); diff --git a/packages/client/src/pages/auth.vue b/packages/client/src/pages/auth.vue index a931a3f9e..a74f22a79 100644 --- a/packages/client/src/pages/auth.vue +++ b/packages/client/src/pages/auth.vue @@ -73,6 +73,8 @@ onMounted(async () => { session = await os.api('auth/session/generate', { clientId, + // make the server check the redirect, if provided + callbackUrl: params.get('redirect_uri') ?? undefined, }).catch(e => { const response = { error: 'server_error',