check redirect URIs

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;
+}

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),
 

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 = {
-	oneOf: [{
 	type: 'object',
+	oneOf: [{
 		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();

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',