mode: indirect

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

@@ -111,6 +111,22 @@ interface OAuthRequest extends OAuth2Req {
 	codeChallengeMethod: string;
 }
 
+function getQueryMode(issuerUrl: string): oauth2orize.grant.Options['modes'] {
+	return {
+		query: (txn, res, params): void => {
+			// RFC 9207
+			params.iss = issuerUrl;
+
+			const parsed = new URL(txn.redirectURI);
+			for (const [key, value] of Object.entries(params)) {
+				parsed.searchParams.append(key, value as string);
+			}
+
+			return (res as any).redirect(parsed.toString());
+		},
+	};
+}
+
 class OAuth2Store {
 	#cache = new MemoryKVCache<OAuth2>(1000 * 60 * 5); // 5min
 
@@ -128,13 +144,13 @@ class OAuth2Store {
 		cb(null, loaded);
 	}
 
-	store(req: any, oauth2: OAuth2, cb: (err: Error | null, transactionID?: string) => void): void {
+	store(req: unknown, oauth2: OAuth2, cb: (err: Error | null, transactionID?: string) => void): void {
 		const transactionId = secureRndstr(128, true);
 		this.#cache.set(transactionId, oauth2);
 		cb(null, transactionId);
 	}
 
-	remove(req: any, tid: string, cb: () => void): void {
+	remove(req: unknown, tid: string, cb: () => void): void {
 		this.#cache.delete(tid);
 		cb();
 	}
@@ -168,19 +184,7 @@ export class OAuth2ProviderService {
 
 		this.#server.grant(oauth2Pkce.extensions());
 		this.#server.grant(oauth2orize.grant.code({
-			modes: {
+			modes: getQueryMode(config.url),
-				query: (txn, res, params) => {
-					// RFC 9207
-					params.iss = config.url;
-
-					const parsed = new URL(txn.redirectURI);
-					for (const [key, value] of Object.entries(params)) {
-						parsed.searchParams.append(key, value as string);
-					}
-
-					return (res as any).redirect(parsed.toString());
-				},
-			},
 		}, (client, redirectUri, token, ares, areq, locals, done) => {
 			(async (): Promise<OmitFirstElement<Parameters<typeof done>>> => {
 				console.log('HIT grant code:', client, redirectUri, token, ares, areq);
@@ -303,27 +307,14 @@ export class OAuth2ProviderService {
 
 		await fastify.register(fastifyExpress);
 		fastify.use('/oauth/authorize', this.#server.authorize(((areq, done) => {
-			(async (): Promise<OmitFirstElement<Parameters<typeof done>>> => {
+			(async (): Promise<Parameters<typeof done>> => {
 				console.log('HIT /oauth/authorize validation middleware', areq);
 
+				// This should return client/redirectURI AND the error, or
+				// the handler can't send error to the redirection URI
+
 				const { codeChallenge, codeChallengeMethod, clientID, redirectURI, scope, type } = areq as OAuthRequest;
 
-				const scopes = [...new Set(scope)].filter(s => kinds.includes(s));
-				if (!scopes.length) {
-					throw new AuthorizationError('`scope` parameter has no known scope', 'invalid_scope');
-				}
-				areq.scope = scopes;
-
-				if (type !== 'code') {
-					throw new AuthorizationError('`response_type` parameter must be set as "code"', 'invalid_request');
-				}
-				if (typeof codeChallenge !== 'string') {
-					throw new AuthorizationError('`code_challenge` parameter is required', 'invalid_request');
-				}
-				if (codeChallengeMethod !== 'S256') {
-					throw new AuthorizationError('`code_challenge_method` parameter must be set as S256', 'invalid_request');
-				}
-
 				const clientUrl = validateClientId(clientID);
 
 				if (process.env.NODE_ENV !== 'test' || process.env.MISSKEY_TEST_DISALLOW_LOOPBACK === '1') {
@@ -339,13 +330,33 @@ export class OAuth2ProviderService {
 					throw new AuthorizationError('Invalid redirect_uri', 'invalid_request');
 				}
 
-				return [clientInfo, redirectURI];
+				try {
-			})().then(args => done(null, ...args), err => done(err));
+					const scopes = [...new Set(scope)].filter(s => kinds.includes(s));
+					if (!scopes.length) {
+						throw new AuthorizationError('`scope` parameter has no known scope', 'invalid_scope');
+					}
+					areq.scope = scopes;
+
+					if (type !== 'code') {
+						throw new AuthorizationError('`response_type` parameter must be set as "code"', 'invalid_request');
+					}
+					if (typeof codeChallenge !== 'string') {
+						throw new AuthorizationError('`code_challenge` parameter is required', 'invalid_request');
+					}
+					if (codeChallengeMethod !== 'S256') {
+						throw new AuthorizationError('`code_challenge_method` parameter must be set as S256', 'invalid_request');
+					}
+				} catch (err) {
+					return [err as Error, clientInfo, redirectURI];
+				}
+
+				return [null, clientInfo, redirectURI];
+			})().then(args => done(...args), err => done(err));
 		}) as ValidateFunctionArity2));
-		// TODO: use mode: indirect
+		fastify.use('/oauth/authorize', this.#server.errorHandler({
-		// https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.2.1
+			mode: 'indirect',
-		// But make sure not to redirect to an invalid redirect_uri
+			modes: getQueryMode(this.config.url),
-		fastify.use('/oauth/authorize', this.#server.errorHandler());
+		}));
 
 		fastify.use('/oauth/decision', bodyParser.urlencoded({ extended: false }));
 		fastify.use('/oauth/decision', this.#server.decision((req, done) => {

packages/backend/test/e2e/oauth.ts

@@ -32,6 +32,10 @@ interface OAuthErrorResponse {
 	error_description: string;
 }
 
+interface OAuthErrorDirectResponse {
+	code: string;
+}
+
 interface AuthorizationParamsExtended {
 	redirect_uri: string;
 	scope: string | string[];
@@ -294,9 +298,12 @@ describe('OAuth', () => {
 				redirect_uri,
 				scope: 'write:notes',
 				state: 'state',
-			}));
+			}), { redirect: 'manual' });
-			assert.strictEqual(response.status, 400);
+			assert.strictEqual(response.status, 302);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+
+			let location = response.headers.get('location');
+			assert.ok(location);
+			assert.strictEqual(new URL(location).searchParams.get('error'), 'invalid_request');
 
 			// Pattern 2: Only code_challenge
 			response = await fetch(client.authorizeURL({
@@ -304,9 +311,12 @@ describe('OAuth', () => {
 				scope: 'write:notes',
 				state: 'state',
 				code_challenge: 'code',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
-			assert.strictEqual(response.status, 400);
+			assert.strictEqual(response.status, 302);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+
+			location = response.headers.get('location');
+			assert.ok(location);
+			assert.strictEqual(new URL(location).searchParams.get('error'), 'invalid_request');
 
 			// Pattern 2: Only code_challenge_method
 			response = await fetch(client.authorizeURL({
@@ -314,9 +324,12 @@ describe('OAuth', () => {
 				scope: 'write:notes',
 				state: 'state',
 				code_challenge_method: 'S256',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
-			assert.strictEqual(response.status, 400);
+			assert.strictEqual(response.status, 302);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+
+			location = response.headers.get('location');
+			assert.ok(location);
+			assert.strictEqual(new URL(location).searchParams.get('error'), 'invalid_request');
 
 			// Pattern 3: Unsupported code_challenge_method
 			response = await fetch(client.authorizeURL({
@@ -325,9 +338,12 @@ describe('OAuth', () => {
 				state: 'state',
 				code_challenge: 'code',
 				code_challenge_method: 'SSSS',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
-			assert.strictEqual(response.status, 400);
+			assert.strictEqual(response.status, 302);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+
+			location = response.headers.get('location');
+			assert.ok(location);
+			assert.strictEqual(new URL(location).searchParams.get('error'), 'invalid_request');
 		});
 
 		// Use precomputed challenge/verifier set here for deterministic test
@@ -407,7 +423,10 @@ describe('OAuth', () => {
 		const decisionResponse = await fetchDecisionFromResponse(response, alice, { cancel: true });
 		assert.strictEqual(decisionResponse.status, 302);
 
-		const location = new URL(decisionResponse.headers.get('location')!);
+		const locationHeader = decisionResponse.headers.get('location');
+		assert.ok(locationHeader);
+
+		const location = new URL(locationHeader);
 		assert.ok(!location.searchParams.has('code'));
 		assert.ok(location.searchParams.has('error'));
 	});
@@ -421,10 +440,13 @@ describe('OAuth', () => {
 				state: 'state',
 				code_challenge: 'code',
 				code_challenge_method: 'S256',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
+			assert.strictEqual(response.status, 302);
 
-			assert.strictEqual(response.status, 400);
+			const locationHeader = response.headers.get('location');
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope');
+			assert.ok(locationHeader);
+
+			assert.strictEqual(new URL(locationHeader).searchParams.get('error'), 'invalid_scope');
 		});
 
 		test('Empty scope', async () => {
@@ -436,10 +458,13 @@ describe('OAuth', () => {
 				state: 'state',
 				code_challenge: 'code',
 				code_challenge_method: 'S256',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
+			assert.strictEqual(response.status, 302);
 
-			assert.strictEqual(response.status, 400);
+			const locationHeader = response.headers.get('location');
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope');
+			assert.ok(locationHeader);
+
+			assert.strictEqual(new URL(locationHeader).searchParams.get('error'), 'invalid_scope');
 		});
 
 		test('Unknown scopes', async () => {
@@ -451,10 +476,13 @@ describe('OAuth', () => {
 				state: 'state',
 				code_challenge: 'code',
 				code_challenge_method: 'S256',
-			} as AuthorizationParamsExtended));
+			} as AuthorizationParamsExtended), { redirect: 'manual' });
+			assert.strictEqual(response.status, 302);
 
-			assert.strictEqual(response.status, 400);
+			const locationHeader = response.headers.get('location');
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope');
+			assert.ok(locationHeader);
+
+			assert.strictEqual(new URL(locationHeader).searchParams.get('error'), 'invalid_scope');
 		});
 
 		test('Partially known scopes', async () => {
@@ -584,7 +612,7 @@ describe('OAuth', () => {
 			} as AuthorizationParamsExtended));
 
 			assert.strictEqual(response.status, 400);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+			assert.strictEqual((await response.json() as OAuthErrorDirectResponse).code, 'invalid_request');
 		});
 
 		test('Invalid redirect_uri including the valid one at authorization endpoint', async () => {
@@ -599,7 +627,7 @@ describe('OAuth', () => {
 			} as AuthorizationParamsExtended));
 
 			assert.strictEqual(response.status, 400);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+			assert.strictEqual((await response.json() as OAuthErrorDirectResponse).code, 'invalid_request');
 		});
 
 		test('No redirect_uri at authorization endpoint', async () => {
@@ -613,7 +641,7 @@ describe('OAuth', () => {
 			} as AuthorizationParamsExtended));
 
 			assert.strictEqual(response.status, 400);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+			assert.strictEqual((await response.json() as OAuthErrorDirectResponse).code, 'invalid_request');
 		});
 
 		test('Invalid redirect_uri at token endpoint', async () => {
@@ -799,7 +827,7 @@ describe('OAuth', () => {
 				} as AuthorizationParamsExtended));
 
 				assert.strictEqual(response.status, 400);
-				assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+				assert.strictEqual((await response.json() as OAuthErrorDirectResponse).code, 'invalid_request');
 			});
 		});
 
@@ -816,7 +844,7 @@ describe('OAuth', () => {
 			} as AuthorizationParamsExtended));
 
 			assert.strictEqual(response.status, 400);
-			assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request');
+			assert.strictEqual((await response.json() as OAuthErrorDirectResponse).code, 'invalid_request');
 		});
 
 		test('Missing name', async () => {