add more comments

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?
+	// But then again:
-	if (!['http:', 'https:'].includes(url.protocol)) {
+	// https://datatracker.ietf.org/doc/html/rfc6749.html#section-3.1.2.1
-		throw new AuthorizationError('client_id must be either https or http URL', 'invalid_request');
+	// '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') {
+					if (ipaddr.parse(lookup.address).range() !== 'unicast') {
-						throw new AuthorizationError('client_id unexpectedly resolves to loopback IP.', 'invalid_request');
+						throw new AuthorizationError('client_id resolves to disallowed IP range.', 'invalid_request');
 					}
 				}
 

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<string, (reply: FastifyReply) => 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({