diff --git a/packages/backend/src/server/oauth/OAuth2ProviderService.ts b/packages/backend/src/server/oauth/OAuth2ProviderService.ts index 6461e02a3..591c55b33 100644 --- a/packages/backend/src/server/oauth/OAuth2ProviderService.ts +++ b/packages/backend/src/server/oauth/OAuth2ProviderService.ts @@ -115,15 +115,14 @@ class OAuth2Store { #cache = new MemoryKVCache(1000 * 60 * 5); // 5min load(req: any, cb: (err: Error | null, txn?: OAuth2) => void): void { - console.log(req); const { transaction_id } = req.body; if (!transaction_id) { - cb(new Error('Missing transaction ID')); + cb(new AuthorizationError('Missing transaction ID', 'invalid_request')); return; } const loaded = this.#cache.get(transaction_id); if (!loaded) { - cb(new Error('Failed to load transaction')); + cb(new AuthorizationError('Failed to load transaction', 'access_denied')); return; } cb(null, loaded); @@ -187,6 +186,9 @@ export class OAuth2ProviderService { console.log('HIT grant code:', client, redirectUri, token, ares, areq); const code = secureRndstr(32, true); + if (!token) { + throw new AuthorizationError('No user', 'invalid_request'); + } const user = await this.cacheService.localUserByNativeTokenCache.fetch(token, () => this.usersRepository.findOneBy({ token }) as Promise); if (!user) { diff --git a/packages/backend/test/e2e/oauth.ts b/packages/backend/test/e2e/oauth.ts index 8d6c9b42d..87e79c907 100644 --- a/packages/backend/test/e2e/oauth.ts +++ b/packages/backend/test/e2e/oauth.ts @@ -19,6 +19,14 @@ const host = `http://127.0.0.1:${port}`; const clientPort = port + 1; const redirect_uri = `http://127.0.0.1:${clientPort}/redirect`; +const basicAuthParams: AuthorizationParamsExtended = { + redirect_uri, + scope: 'write:notes', + state: 'state', + code_challenge: 'code', + code_challenge_method: 'S256', +}; + interface OAuthErrorResponse { error: string; error_description: string; @@ -95,7 +103,6 @@ async function fetchAuthorizationCode(user: misskey.entities.MeSignup, scope: st } as AuthorizationParamsExtended)); assert.strictEqual(response.status, 200); - // TODO: this fetch-decision-code checks are everywhere, maybe get a helper for this. const decisionResponse = await fetchDecisionFromResponse(response, user); assert.strictEqual(decisionResponse.status, 302); @@ -532,7 +539,7 @@ describe('OAuth', () => { // RFC 6750 section 3.1 says 401 but it's SHOULD not MUST. 403 should be okay for now. assert.strictEqual(createResponse.status, 403); - // TODO: error code (invalid_token) + // TODO: error code (wrong Authorization header should emit OAuth error instead of Misskey API error) }); describe('Redirection', () => { @@ -625,6 +632,65 @@ describe('OAuth', () => { assert.ok(body.scopes_supported.includes('write:notes')); }); + describe('Decision endpoint', () => { + test('No login token', async () => { + const client = getClient(); + + const response = await fetch(client.authorizeURL(basicAuthParams)); + assert.strictEqual(response.status, 200); + + const { transactionId } = getMeta(await response.text()); + assert.ok(transactionId); + + const decisionResponse = await fetch(new URL('/oauth/decision', host), { + method: 'post', + body: new URLSearchParams({ + transaction_id: transactionId, + }), + redirect: 'manual', + headers: { + 'content-type': 'application/x-www-form-urlencoded', + }, + }); + + assert.strictEqual(decisionResponse.status, 400); + assert.strictEqual((await decisionResponse.json() as OAuthErrorResponse).error, 'invalid_request'); + }); + + test('No transaction ID', async () => { + const decisionResponse = await fetch(new URL('/oauth/decision', host), { + method: 'post', + body: new URLSearchParams({ + login_token: alice.token, + }), + redirect: 'manual', + headers: { + 'content-type': 'application/x-www-form-urlencoded', + }, + }); + + assert.strictEqual(decisionResponse.status, 400); + assert.strictEqual((await decisionResponse.json() as OAuthErrorResponse).error, 'invalid_request'); + }); + + test('Invalid transaction ID', async () => { + const decisionResponse = await fetch(new URL('/oauth/decision', host), { + method: 'post', + body: new URLSearchParams({ + login_token: alice.token, + transaction_id: 'invalid_id', + }), + redirect: 'manual', + headers: { + 'content-type': 'application/x-www-form-urlencoded', + }, + }); + + assert.strictEqual(decisionResponse.status, 403); + assert.strictEqual((await decisionResponse.json() as OAuthErrorResponse).error, 'access_denied'); + }); + }); + describe('Client Information Discovery', () => { describe('Redirection', () => { const tests: Record void> = { @@ -748,8 +814,6 @@ describe('OAuth', () => { }); }); - // TODO: Invalid decision endpoint parameters - // TODO: Unknown OAuth endpoint // TODO: successful token exchange should invalidate the grant token (spec?)