From 20efdc78e2634b82756e4b73baee3f4051c2cc13 Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Wed, 14 Jun 2023 00:22:39 +0200 Subject: [PATCH] add more comments --- .../src/server/oauth/OAuth2ProviderService.ts | 26 ++++++++---- packages/backend/test/e2e/oauth.ts | 42 ++++++++++++++++--- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/packages/backend/src/server/oauth/OAuth2ProviderService.ts b/packages/backend/src/server/oauth/OAuth2ProviderService.ts index 75c5c54c1..4a0775879 100644 --- a/packages/backend/src/server/oauth/OAuth2ProviderService.ts +++ b/packages/backend/src/server/oauth/OAuth2ProviderService.ts @@ -36,9 +36,13 @@ function validateClientId(raw: string): URL { })(); // Client identifier URLs MUST have either an https or http scheme - // XXX: but why allow http in 2023? - if (!['http:', 'https:'].includes(url.protocol)) { - throw new AuthorizationError('client_id must be either https or http URL', 'invalid_request'); + // But then again: + // https://datatracker.ietf.org/doc/html/rfc6749.html#section-3.1.2.1 + // 'The redirection endpoint SHOULD require the use of TLS as described + // in Section 1.6 when the requested response type is "code" or "token"' + const allowedProtocols = process.env.NODE_ENV === 'test' ? ['http:', 'https:'] : ['https:']; + if (!allowedProtocols.includes(url.protocol)) { + throw new AuthorizationError('client_id must be a valid HTTPS URL', 'invalid_request'); } // MUST contain a path component (new URL() implicitly adds one) @@ -116,7 +120,10 @@ interface OAuthRequest extends OAuth2Req { function getQueryMode(issuerUrl: string): oauth2orize.grant.Options['modes'] { return { query: (txn, res, params): void => { - // RFC 9207 + // https://datatracker.ietf.org/doc/html/rfc9207#name-response-parameter-iss + // "In authorization responses to the client, including error responses, + // an authorization server supporting this specification MUST indicate its + // identity by including the iss parameter in the response." params.iss = issuerUrl; const parsed = new URL(txn.redirectURI); @@ -188,6 +195,7 @@ export class OAuth2ProviderService { scopes: string[], }>(1000 * 60 * 5); // 5m + // https://datatracker.ietf.org/doc/html/rfc7636.html this.#server.grant(oauth2Pkce.extensions()); this.#server.grant(oauth2orize.grant.code({ modes: getQueryMode(config.url), @@ -307,10 +315,14 @@ export class OAuth2ProviderService { const clientUrl = validateClientId(clientID); - if (process.env.NODE_ENV !== 'test' || process.env.MISSKEY_TEST_DISALLOW_LOOPBACK === '1') { + // TODO: Consider allowing this for native apps (RFC 8252) + // The current setup requires an explicit list of redirect_uris per + // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.1.3 + // which blocks the support. But we could loose the rule in this case. + if (process.env.NODE_ENV !== 'test' || process.env.MISSKEY_TEST_CHECK_IP_RANGE === '1') { const lookup = await dns.lookup(clientUrl.hostname); - if (ipaddr.parse(lookup.address).range() === 'loopback') { - throw new AuthorizationError('client_id unexpectedly resolves to loopback IP.', 'invalid_request'); + if (ipaddr.parse(lookup.address).range() !== 'unicast') { + throw new AuthorizationError('client_id resolves to disallowed IP range.', 'invalid_request'); } } diff --git a/packages/backend/test/e2e/oauth.ts b/packages/backend/test/e2e/oauth.ts index fe52f6888..a9bdfae77 100644 --- a/packages/backend/test/e2e/oauth.ts +++ b/packages/backend/test/e2e/oauth.ts @@ -121,7 +121,10 @@ function assertIndirectError(response: Response, error: string): void { const location = new URL(locationHeader); assert.strictEqual(location.searchParams.get('error'), error); + + // https://datatracker.ietf.org/doc/html/rfc9207#name-response-parameter-iss assert.strictEqual(location.searchParams.get('iss'), 'http://misskey.local'); + // https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.2.1 assert.ok(location.searchParams.has('state')); } @@ -146,7 +149,7 @@ describe('OAuth', () => { }, 1000 * 60 * 2); beforeEach(async () => { - process.env.MISSKEY_TEST_DISALLOW_LOOPBACK = ''; + process.env.MISSKEY_TEST_CHECK_IP_RANGE = ''; fastify = Fastify(); fastify.get('/', async (request, reply) => { reply.send(` @@ -196,7 +199,8 @@ describe('OAuth', () => { assert.strictEqual(location.origin + location.pathname, redirect_uri); assert.ok(location.searchParams.has('code')); assert.strictEqual(location.searchParams.get('state'), 'state'); - assert.strictEqual(location.searchParams.get('iss'), 'http://misskey.local'); // RFC 9207 + // https://datatracker.ietf.org/doc/html/rfc9207#name-response-parameter-iss + assert.strictEqual(location.searchParams.get('iss'), 'http://misskey.local'); const code = new URL(location).searchParams.get('code'); assert.ok(code); @@ -299,7 +303,11 @@ describe('OAuth', () => { assert.strictEqual(createResponseBodyBob.createdNote.user.username, 'bob'); }); + // https://datatracker.ietf.org/doc/html/rfc7636.html describe('PKCE', () => { + // https://datatracker.ietf.org/doc/html/rfc7636.html#section-4.4.1 + // '... the authorization endpoint MUST return the authorization + // error response with the "error" value set to "invalid_request".' test('Require PKCE', async () => { const client = getClient(); @@ -425,7 +433,13 @@ describe('OAuth', () => { assert.ok(location.searchParams.has('error')); }); + // https://datatracker.ietf.org/doc/html/rfc6749.html#section-3.3 describe('Scope', () => { + // "If the client omits the scope parameter when requesting + // authorization, the authorization server MUST either process the + // request using a pre-defined default value or fail the request + // indicating an invalid scope." + // (And Misskey does the latter) test('Missing scope', async () => { const client = getClient(); @@ -464,6 +478,11 @@ describe('OAuth', () => { assertIndirectError(response, 'invalid_scope'); }); + // "If the issued access token scope + // is different from the one requested by the client, the authorization + // server MUST include the "scope" response parameter to inform the + // client of the actual scope granted." + // (Although Misskey always return scope, which is also fine) test('Partially known scopes', async () => { const { code_challenge, code_verifier } = await pkceChallenge(128); @@ -480,8 +499,6 @@ describe('OAuth', () => { code_verifier, } as AuthorizationTokenConfigExtended); - // OAuth2 requires returning `scope` in the token response if the resulting scope is different than the requested one - // (Although Misskey always return scope, which is also fine) assert.strictEqual(token.token.scope, 'write:notes'); }); @@ -541,6 +558,7 @@ describe('OAuth', () => { }); }); + // https://datatracker.ietf.org/doc/html/rfc6750.html test('Authorization header', async () => { const { code_challenge, code_verifier } = await pkceChallenge(128); @@ -572,12 +590,22 @@ describe('OAuth', () => { }, body: JSON.stringify({ text: 'test' }), }); - // RFC 6750 section 3.1 says 401 but it's SHOULD not MUST. 403 should be okay for now. + + // https://datatracker.ietf.org/doc/html/rfc6750.html#section-3.1 + // "The access token provided is expired, revoked, malformed, or + // invalid for other reasons. The resource SHOULD respond with + // the HTTP 401 (Unauthorized) status code." + // (but it's SHOULD not MUST. 403 should be okay for now.) assert.strictEqual(createResponse.status, 403); // TODO: error code (wrong Authorization header should emit OAuth error instead of Misskey API error) }); + // https://datatracker.ietf.org/doc/html/rfc6749.html#section-3.1.2.4 + // "If an authorization request fails validation due to a missing, + // invalid, or mismatching redirection URI, the authorization server + // SHOULD inform the resource owner of the error and MUST NOT + // automatically redirect the user-agent to the invalid redirection URI." describe('Redirection', () => { test('Invalid redirect_uri at authorization endpoint', async () => { const client = getClient(); @@ -653,6 +681,7 @@ describe('OAuth', () => { }); }); + // https://datatracker.ietf.org/doc/html/rfc8414 test('Server metadata', async () => { const response = await fetch(new URL('.well-known/oauth-authorization-server', host)); assert.strictEqual(response.status, 200); @@ -717,6 +746,7 @@ describe('OAuth', () => { }); }); + // https://indieauth.spec.indieweb.org/#client-information-discovery describe('Client Information Discovery', () => { describe('Redirection', () => { const tests: Record void> = { @@ -801,7 +831,7 @@ describe('OAuth', () => { }); test('Disallow loopback', async () => { - process.env.MISSKEY_TEST_DISALLOW_LOOPBACK = '1'; + process.env.MISSKEY_TEST_CHECK_IP_RANGE = '1'; const client = getClient(); const response = await fetch(client.authorizeURL({