From f6d9cf1ef10dbd5d9a2396ff5c5e908c35967cc3 Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Mon, 10 Apr 2023 14:49:18 +0200 Subject: [PATCH] strict redirection uri --- packages/backend/package.json | 2 + .../src/server/oauth/OAuth2ProviderService.ts | 59 +++++++++----- .../backend/src/server/web/views/oauth.pug | 2 +- packages/backend/test/e2e/oauth.ts | 79 ++++++++++++++++--- packages/frontend/src/pages/oauth.vue | 2 +- pnpm-lock.yaml | 17 ++++ 6 files changed, 127 insertions(+), 34 deletions(-) diff --git a/packages/backend/package.json b/packages/backend/package.json index 584e57c233..bf340925a6 100644 --- a/packages/backend/package.json +++ b/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", diff --git a/packages/backend/src/server/oauth/OAuth2ProviderService.ts b/packages/backend/src/server/oauth/OAuth2ProviderService.ts index d0cd211408..f6fbdbdd8b 100644 --- a/packages/backend/src/server/oauth/OAuth2ProviderService.ts +++ b/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 { +interface ClientInformation { + id: string; + redirectUris: string[]; + name: string; +} + +async function discoverClientInformation(httpRequestService: HttpRequestService, id: string): Promise { try { const res = await httpRequestService.send(id); - let redirectUri = parseLinkHeader(res.headers.get('link'))?.redirect_uri?.url; - if (redirectUri) { - return new URL(redirectUri, res.url).toString(); + const redirectUris: string[] = []; + + 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('link[rel=redirect_uri][href]')?.href; - if (redirectUri) { - return new URL(redirectUri, res.url).toString(); - } + const fragment = JSDOM.fragment(await res.text()); + + redirectUris.push(...[...fragment.querySelectorAll('link[rel=redirect_uri][href]')].map(el => el.href)); + + const name = fragment.querySelector('.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>> => { 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 - // Allow same-origin redirection - if (redirectUrl.protocol !== clientUrl.protocol || redirectUrl.host !== clientUrl.host) { - // TODO: allow only explicit redirect_uri by Client Information Discovery - throw new Error('cross-origin redirect_uri is not supported yet.'); + if (process.env.NODE_ENV !== 'test') { + const lookup = await dns.lookup(clientUrl.hostname); + if (ipaddr.parse(lookup.address).range() === 'loopback') { + throw new Error('client_id unexpectedly resolves to loopback IP.'); + } } - 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()) { diff --git a/packages/backend/src/server/web/views/oauth.pug b/packages/backend/src/server/web/views/oauth.pug index c4731b8114..94ef196b1d 100644 --- a/packages/backend/src/server/web/views/oauth.pug +++ b/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) diff --git a/packages/backend/test/e2e/oauth.ts b/packages/backend/test/e2e/oauth.ts index 69c5c869c8..1a42d60cbc 100644 --- a/packages/backend/test/e2e/oauth.ts +++ b/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('meta[name="misskey:oauth:transaction-id"]')?.content; + return { + transactionId: fragment.querySelector('meta[name="misskey:oauth:transaction-id"]')?.content, + clientName: fragment.querySelector('meta[name="misskey:oauth:client-name"]')?.content, + }; } function fetchDecision(cookie: string, transactionId: string, user: any, { cancel }: { cancel?: boolean } = {}): Promise { @@ -51,24 +55,35 @@ function fetchDecision(cookie: string, transactionId: string, user: any, { cance async function fetchDecisionFromResponse(response: Response, user: any, { cancel }: { cancel?: boolean } = {}): Promise { 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(` + + +
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()); - assert.strictEqual(typeof transactionId, 'string'); + const meta = getMeta(await response.text()); + 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) }); diff --git a/packages/frontend/src/pages/oauth.vue b/packages/frontend/src/pages/oauth.vue index 77d64ef957..e0d126cb31 100644 --- a/packages/frontend/src/pages/oauth.vue +++ b/packages/frontend/src/pages/oauth.vue @@ -40,7 +40,7 @@ if (transactionIdMeta) { transactionIdMeta.remove(); } -const name = document.querySelector('meta[name="misskey:oauth:client-id"]')?.content; +const name = document.querySelector('meta[name="misskey:oauth:client-name"]')?.content; const _permissions = document.querySelector('meta[name="misskey:oauth:scope"]')?.content.split(' ') ?? []; function onLogin(res): void { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3ab71a3271..d5be7fa4d7 100644 --- a/pnpm-lock.yaml +++ b/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'}