strict redirection uri

packages/backend/package.json

@@ -101,6 +101,7 @@
 		"got": "13.0.0",
 		"happy-dom": "9.20.3",
 		"hpagent": "1.2.0",
+		"http-link-header": "^1.1.0",
 		"ioredis": "5.3.2",
 		"ip-cidr": "3.1.0",
 		"ipaddr.js": "2.1.0",
@@ -180,6 +181,7 @@
 		"@types/escape-regexp": "0.0.1",
 		"@types/express-session": "^1.17.6",
 		"@types/fluent-ffmpeg": "2.1.21",
+		"@types/http-link-header": "^1.0.3",
 		"@types/jest": "29.5.2",
 		"@types/js-yaml": "4.0.5",
 		"@types/jsdom": "21.1.1",

packages/backend/src/server/oauth/OAuth2ProviderService.ts

@@ -2,7 +2,7 @@ import dns from 'node:dns/promises';
 import { Inject, Injectable } from '@nestjs/common';
 import fastifyMiddie, { IncomingMessageExtended } from '@fastify/middie';
 import { JSDOM } from 'jsdom';
-import parseLinkHeader from 'parse-link-header';
+import httpLinkHeader from 'http-link-header';
 import ipaddr from 'ipaddr.js';
 import oauth2orize, { type OAuth2 } from 'oauth2orize';
 import * as oauth2Query from 'oauth2orize/lib/response/query.js';
@@ -103,18 +103,33 @@ function validateClientId(raw: string): URL {
 // 	return `uid:${uid}`;
 // }
 
-async function fetchFromClientId(httpRequestService: HttpRequestService, id: string): Promise<string | void> {
+interface ClientInformation {
+	id: string;
+	redirectUris: string[];
+	name: string;
+}
+
+async function discoverClientInformation(httpRequestService: HttpRequestService, id: string): Promise<ClientInformation> {
 	try {
 		const res = await httpRequestService.send(id);
-		let redirectUri = parseLinkHeader(res.headers.get('link'))?.redirect_uri?.url;
+		const redirectUris: string[] = [];
-		if (redirectUri) {
+
-			return new URL(redirectUri, res.url).toString();
+		const linkHeader = res.headers.get('link');
+		if (linkHeader) {
+			redirectUris.push(...httpLinkHeader.parse(linkHeader).get('rel', 'redirect_uri').map(r => r.uri));
 		}
 
-		redirectUri = JSDOM.fragment(await res.text()).querySelector<HTMLLinkElement>('link[rel=redirect_uri][href]')?.href;
+		const fragment = JSDOM.fragment(await res.text());
-		if (redirectUri) {
+
-			return new URL(redirectUri, res.url).toString();
+		redirectUris.push(...[...fragment.querySelectorAll<HTMLLinkElement>('link[rel=redirect_uri][href]')].map(el => el.href));
-		}
+
+		const name = fragment.querySelector<HTMLElement>('.h-app .p-name')?.textContent?.trim() ?? id;
+
+		return {
+			id,
+			redirectUris: redirectUris.map(uri => new URL(uri, res.url).toString()),
+			name,
+		};
 	} catch {
 		throw new Error('Failed to fetch client information');
 	}
@@ -267,7 +282,7 @@ async function fetchFromClientId(httpRequestService: HttpRequestService, id: str
 // 	};
 // }
 
-function pkceS256(codeVerifier: string) {
+function pkceS256(codeVerifier: string): string {
 	return crypto.createHash('sha256')
 		.update(codeVerifier, 'ascii')
 		.digest('base64url');
@@ -362,7 +377,7 @@ export class OAuth2ProviderService {
 				}
 
 				TEMP_GRANT_CODES[code] = {
-					clientId: client,
+					clientId: client.id,
 					userId: user.id,
 					redirectUri,
 					codeChallenge: areq.codeChallenge,
@@ -470,7 +485,7 @@ export class OAuth2ProviderService {
 			reply.header('Cache-Control', 'no-store');
 			return await reply.view('oauth', {
 				transactionId: oauth2.transactionID,
-				clientId: oauth2.client,
+				clientName: oauth2.client.name,
 				scope: scopes.join(' '),
 			});
 		});
@@ -494,18 +509,22 @@ export class OAuth2ProviderService {
 			(async (): Promise<OmitFirstElement<Parameters<typeof done>>> => {
 				console.log('HIT /oauth/authorize validation middleware');
 
-				// Find client information from the remote.
 				const clientUrl = validateClientId(clientId);
-				const redirectUrl = new URL(redirectUri);
 
-				// https://indieauth.spec.indieweb.org/#authorization-request
+				if (process.env.NODE_ENV !== 'test') {
-				// Allow same-origin redirection
+					const lookup = await dns.lookup(clientUrl.hostname);
-				if (redirectUrl.protocol !== clientUrl.protocol || redirectUrl.host !== clientUrl.host) {
+					if (ipaddr.parse(lookup.address).range() === 'loopback') {
-					// TODO: allow only explicit redirect_uri by Client Information Discovery
+						throw new Error('client_id unexpectedly resolves to loopback IP.');
-					throw new Error('cross-origin redirect_uri is not supported yet.');
+					}
 				}
 
-				return [clientId, redirectUri];
+				// Find client information from the remote.
+				const clientInfo = await discoverClientInformation(this.httpRequestService, clientUrl.href);
+				if (!clientInfo.redirectUris.includes(redirectUri)) {
+					throw new Error('Invalid redirect_uri');
+				}
+
+				return [clientInfo, redirectUri];
 			})().then(args => done(null, ...args), err => done(err));
 		}));
 		// for (const middleware of this.#server.decision()) {

packages/backend/src/server/web/views/oauth.pug

@@ -5,5 +5,5 @@ block meta
 	//- user navigates away via the navigation bar
 	//- XXX: Remove navigation bar in auth page?
 	meta(name='misskey:oauth:transaction-id' content=transactionId)
-	meta(name='misskey:oauth:client-id' content=clientId)
+	meta(name='misskey:oauth:client-name' content=clientName)
 	meta(name='misskey:oauth:scope' content=scope)

packages/backend/test/e2e/oauth.ts

@@ -1,11 +1,12 @@
 process.env.NODE_ENV = 'test';
 
 import * as assert from 'assert';
-import { port, relativeFetch, signup, startServer } from '../utils.js';
-import type { INestApplicationContext } from '@nestjs/common';
 import { AuthorizationCode } from 'simple-oauth2';
 import pkceChallenge from 'pkce-challenge';
 import { JSDOM } from 'jsdom';
+import { port, relativeFetch, signup, startServer } from '../utils.js';
+import type { INestApplicationContext } from '@nestjs/common';
+import Fastify, { type FastifyInstance } from 'fastify';
 
 const host = `http://127.0.0.1:${port}`;
 
@@ -28,9 +29,12 @@ function getClient(): AuthorizationCode<'client_id'> {
 	});
 }
 
-function getTransactionId(html: string): string | undefined {
+function getMeta(html: string): { transactionId: string | undefined, clientName: string | undefined } | undefined {
 	const fragment = JSDOM.fragment(html);
-	return fragment.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:transaction-id"]')?.content;
+	return {
+		transactionId: fragment.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:transaction-id"]')?.content,
+		clientName: fragment.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:client-name"]')?.content,
+	};
 }
 
 function fetchDecision(cookie: string, transactionId: string, user: any, { cancel }: { cancel?: boolean } = {}): Promise<Response> {
@@ -51,24 +55,35 @@ function fetchDecision(cookie: string, transactionId: string, user: any, { cance
 
 async function fetchDecisionFromResponse(response: Response, user: any, { cancel }: { cancel?: boolean } = {}): Promise<Response> {
 	const cookie = response.headers.get('set-cookie');
-	const transactionId = getTransactionId(await response.text());
+	const { transactionId } = getMeta(await response.text());
 
 	return await fetchDecision(cookie!, transactionId!, user, { cancel });
 }
 
 describe('OAuth', () => {
 	let app: INestApplicationContext;
+	let fastify: FastifyInstance;
 
 	let alice: any;
 
 	beforeAll(async () => {
 		app = await startServer();
+		fastify = Fastify();
+		fastify.get('/', async (request, reply) => {
+			reply.send(`
+				<!DOCTYPE html>
+				<link rel="redirect_uri" href="/redirect" />
+				<div class="h-app"><div class="p-name">Misklient
+			`);
+		});
+		fastify.listen({ port: clientPort, host: '0.0.0.0' });
+
 		alice = await signup({ username: 'alice' });
-		// fastify = Fastify();
 	}, 1000 * 60 * 2);
 
 	afterAll(async () => {
 		await app.close();
+		await fastify.close();
 	});
 
 	test('Full flow', async () => {
@@ -87,10 +102,11 @@ describe('OAuth', () => {
 		const cookie = response.headers.get('set-cookie');
 		assert.ok(cookie?.startsWith('connect.sid='));
 
-		const transactionId = getTransactionId(await response.text());
+		const meta = getMeta(await response.text());
-		assert.strictEqual(typeof transactionId, 'string');
+		assert.strictEqual(typeof meta.transactionId, 'string');
+		assert.strictEqual(meta?.clientName, 'Misklient');
 
-		const decisionResponse = await fetchDecision(cookie!, transactionId!, alice);
+		const decisionResponse = await fetchDecision(cookie!, meta.transactionId!, alice);
 		assert.strictEqual(decisionResponse.status, 302);
 		assert.ok(decisionResponse.headers.has('location'));
 
@@ -468,6 +484,20 @@ describe('OAuth', () => {
 			assert.strictEqual(response.status, 500);
 		});
 
+		test('Invalid redirect_uri including the valid one at authorization endpoint', async () => {
+			const client = getClient();
+
+			const response = await fetch(client.authorizeURL({
+				redirect_uri: 'http://127.0.0.1/redirection',
+				scope: 'write:notes',
+				state: 'state',
+				code_challenge: 'code',
+				code_challenge_method: 'S256',
+			}));
+			// TODO: status code
+			assert.strictEqual(response.status, 500);
+		});
+
 		test('No redirect_uri at authorization endpoint', async () => {
 			const client = getClient();
 
@@ -508,6 +538,33 @@ describe('OAuth', () => {
 			}));
 		});
 
+		test('Invalid redirect_uri including the valid one at token endpoint', async () => {
+			const { code_challenge, code_verifier } = pkceChallenge.default(128);
+
+			const client = getClient();
+
+			const response = await fetch(client.authorizeURL({
+				redirect_uri,
+				scope: 'write:notes',
+				state: 'state',
+				code_challenge,
+				code_challenge_method: 'S256',
+			}));
+			assert.strictEqual(response.status, 200);
+
+			const decisionResponse = await fetchDecisionFromResponse(response, alice);
+			assert.strictEqual(decisionResponse.status, 302);
+
+			const location = new URL(decisionResponse.headers.get('location')!);
+			assert.ok(location.searchParams.has('code'));
+
+			await assert.rejects(client.getToken({
+				code: location.searchParams.get('code')!,
+				redirect_uri: 'http://127.0.0.1/redirection',
+				code_verifier,
+			}));
+		});
+
 		test('No redirect_uri at token endpoint', async () => {
 			const { code_challenge, code_verifier } = pkceChallenge.default(128);
 
@@ -533,8 +590,6 @@ describe('OAuth', () => {
 				code_verifier,
 			}));
 		});
-
-		// TODO: disallow random same-origin URLs with strict redirect_uris with client information discovery
 	});
 
 	test('Server metadata', async () => {
@@ -550,5 +605,5 @@ describe('OAuth', () => {
 
 	// TODO: Error format required by OAuth spec
 
-	// TODO: Client Information Discovery
+	// TODO: Client Information Discovery (use http header, loopback check, missing name or redirection uri)
 });

packages/frontend/src/pages/oauth.vue

@@ -40,7 +40,7 @@ if (transactionIdMeta) {
 	transactionIdMeta.remove();
 }
 
-const name = document.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:client-id"]')?.content;
+const name = document.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:client-name"]')?.content;
 const _permissions = document.querySelector<HTMLMetaElement>('meta[name="misskey:oauth:scope"]')?.content.split(' ') ?? [];
 
 function onLogin(res): void {

pnpm-lock.yaml

@@ -218,6 +218,9 @@ importers:
       hpagent:
         specifier: 1.2.0
         version: 1.2.0
+      http-link-header:
+        specifier: ^1.1.0
+        version: 1.1.0
       ioredis:
         specifier: 5.3.2
         version: 5.3.2
@@ -532,6 +535,9 @@ importers:
       '@types/fluent-ffmpeg':
         specifier: 2.1.21
         version: 2.1.21
+      '@types/http-link-header':
+        specifier: ^1.0.3
+        version: 1.0.3
       '@types/jest':
         specifier: 29.5.2
         version: 29.5.2
@@ -7713,6 +7719,12 @@ packages:
   /@types/http-cache-semantics@4.0.1:
     resolution: {integrity: sha512-SZs7ekbP8CN0txVG2xVRH6EgKmEm31BOxA07vkFaETzZz1xh+cbt8BcI0slpymvwhx5dlFnQG2rTlPVQn+iRPQ==}
 
+  /@types/http-link-header@1.0.3:
+    resolution: {integrity: sha512-y8HkoD/vyid+5MrJ3aas0FvU3/BVBGcyG9kgxL0Zn4JwstA8CglFPnrR0RuzOjRCXwqzL5uxWC2IO7Ub0rMU2A==}
+    dependencies:
+      '@types/node': 20.2.5
+    dev: true
+
   /@types/istanbul-lib-coverage@2.0.4:
     resolution: {integrity: sha512-z/QT1XN4K4KYuslS23k62yDIDLwLFkzxOuMplDtObz0+y7VqJCaO2o+SPwHCvLFZh7xazvvoor2tA/hPz9ee7g==}
     dev: true
@@ -13423,6 +13435,11 @@ packages:
       statuses: 2.0.1
       toidentifier: 1.0.1
 
+  /http-link-header@1.1.0:
+    resolution: {integrity: sha512-pj6N1yxOz/ANO8HHsWGg/OoIL1kmRYvQnXQ7PIRpgp+15AnEsRH8fmIJE6D1OdWG2Bov+BJHVla1fFXxg1JbbA==}
+    engines: {node: '>=6.0.0'}
+    dev: false
+
   /http-proxy-agent@5.0.0:
     resolution: {integrity: sha512-n2hY8YdoRE1i7r6M0w9DIw5GgZN0G25P8zLCRQ8rjXtTU3vsNFBI/vWK/UIeE6g5MUUz6avwAPXmL6Fy9D/90w==}
     engines: {node: '>= 6'}