diff --git a/packages/backend/src/server/oauth/OAuth2ProviderService.ts b/packages/backend/src/server/oauth/OAuth2ProviderService.ts index d6c444893..ded2b2756 100644 --- a/packages/backend/src/server/oauth/OAuth2ProviderService.ts +++ b/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(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: { - 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()); - }, - }, + modes: getQueryMode(config.url), }, (client, redirectUri, token, ares, areq, locals, done) => { (async (): Promise>> => { 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>> => { + (async (): Promise> => { 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]; - })().then(args => done(null, ...args), err => done(err)); + try { + 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 - // https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.2.1 - // But make sure not to redirect to an invalid redirect_uri - fastify.use('/oauth/authorize', this.#server.errorHandler()); + fastify.use('/oauth/authorize', this.#server.errorHandler({ + mode: 'indirect', + modes: getQueryMode(this.config.url), + })); fastify.use('/oauth/decision', bodyParser.urlencoded({ extended: false })); fastify.use('/oauth/decision', this.#server.decision((req, done) => { diff --git a/packages/backend/test/e2e/oauth.ts b/packages/backend/test/e2e/oauth.ts index dae5f9c08..1fc0b95b9 100644 --- a/packages/backend/test/e2e/oauth.ts +++ b/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', - })); - assert.strictEqual(response.status, 400); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request'); + }), { redirect: 'manual' }); + assert.strictEqual(response.status, 302); + + 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)); - assert.strictEqual(response.status, 400); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request'); + } as AuthorizationParamsExtended), { redirect: 'manual' }); + assert.strictEqual(response.status, 302); + + 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)); - assert.strictEqual(response.status, 400); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request'); + } as AuthorizationParamsExtended), { redirect: 'manual' }); + assert.strictEqual(response.status, 302); + + 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)); - assert.strictEqual(response.status, 400); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_request'); + } as AuthorizationParamsExtended), { redirect: 'manual' }); + assert.strictEqual(response.status, 302); + + 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); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope'); + const locationHeader = response.headers.get('location'); + 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); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope'); + const locationHeader = response.headers.get('location'); + 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); - assert.strictEqual((await response.json() as OAuthErrorResponse).error, 'invalid_scope'); + const locationHeader = response.headers.get('location'); + 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 () => {