remerge: remove FetchAllowSoftFailMask in favor of our same-authority checks

packages/backend/src/core/HttpRequestService.ts

@@ -16,7 +16,6 @@ import type { Config } from '@/config.js';
 import { StatusError } from '@/misc/status-error.js';
 import { bindThis } from '@/decorators.js';
 import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
-import { FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
 import type { IObject, IObjectWithId } from '@/core/activitypub/type.js';
 import { ApUtilityService } from './activitypub/ApUtilityService.js';
 import type { Response } from 'node-fetch';
@@ -250,7 +249,7 @@ export class HttpRequestService {
 	}
 
 	@bindThis
-	public async getActivityJson(url: string, isLocalAddressAllowed = false, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObjectWithId> {
+	public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise<IObjectWithId> {
 		const res = await this.send(url, {
 			method: 'GET',
 			headers: {
@@ -268,7 +267,7 @@ export class HttpRequestService {
 
 		// Make sure the object ID matches the final URL (which is where it actually exists).
 		// The caller (ApResolverService) will verify the ID against the original / entry URL, which ensures that all three match.
-		this.apUtilityService.assertIdMatchesUrlAuthority(activity, res.url, allowSoftfail);
+		this.apUtilityService.assertIdMatchesUrlAuthority(activity, res.url);
 
 		return activity as IObjectWithId;
 	}

packages/backend/src/core/activitypub/ApInboxService.ts

@@ -354,7 +354,7 @@ export class ApInboxService {
 			try {
 				// The target ID is verified by secureResolve, so we know it shares host authority with the actor who sent it.
 				// This means we can pass that ID to resolveNote and avoid an extra fetch, which will fail if the note is private.
-				renote = await this.apNoteService.resolveNote(target, { resolver, sentFrom: new URL(getApId(target)) });
+				renote = await this.apNoteService.resolveNote(target, { resolver, sentFrom: getApId(target) });
 				if (renote == null) return 'announce target is null';
 			} catch (err) {
 				// 対象が4xxならスキップ

packages/backend/src/core/activitypub/ApRequestService.ts

@@ -17,7 +17,6 @@ import { LoggerService } from '@/core/LoggerService.js';
 import { bindThis } from '@/decorators.js';
 import type Logger from '@/logger.js';
 import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
-import { FetchAllowSoftFailMask as FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
 import type { IObject, IObjectWithId } from './type.js';
 
 type Request = {
@@ -186,7 +185,7 @@ export class ApRequestService {
 	 * @param followAlternate
 	 */
 	@bindThis
-	public async signedGet(url: string, user: { id: MiUser['id'] }, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict, followAlternate?: boolean): Promise<IObjectWithId> {
+	public async signedGet(url: string, user: { id: MiUser['id'] }, followAlternate?: boolean): Promise<IObjectWithId> {
 		const _followAlternate = followAlternate ?? true;
 		const keypair = await this.userKeypairService.getUserKeypair(user.id);
 
@@ -255,10 +254,10 @@ export class ApRequestService {
 				if (alternate) {
 					const href = alternate.getAttribute('href');
 					if (href && this.apUtilityService.haveSameAuthority(url, href)) {
-						return await this.signedGet(href, user, allowSoftfail, false);
+						return await this.signedGet(href, user, false);
 					}
 				}
-			} catch (e) {
+			} catch {
 				// something went wrong parsing the HTML, ignore the whole thing
 			} finally {
 				happyDOM.close().catch(err => {});
@@ -272,7 +271,7 @@ export class ApRequestService {
 
 		// Make sure the object ID matches the final URL (which is where it actually exists).
 		// The caller (ApResolverService) will verify the ID against the original / entry URL, which ensures that all three match.
-		this.apUtilityService.assertIdMatchesUrlAuthority(activity, res.url, allowSoftfail);
+		this.apUtilityService.assertIdMatchesUrlAuthority(activity, res.url);
 
 		return activity as IObjectWithId;
 	}

packages/backend/src/core/activitypub/ApResolverService.ts

@@ -23,7 +23,6 @@ import { getApId, getNullableApId, IObjectWithId, isCollectionOrOrderedCollectio
 import { ApDbResolverService } from './ApDbResolverService.js';
 import { ApRendererService } from './ApRendererService.js';
 import { ApRequestService } from './ApRequestService.js';
-import { FetchAllowSoftFailMask } from './misc/check-against-url.js';
 import type { IObject, ICollection, IOrderedCollection, ApObject } from './type.js';
 
 export class Resolver {
@@ -104,10 +103,10 @@ export class Resolver {
 		return await this.resolve(id);
 	}
 
-	public async resolve(value: string | [string], allowSoftfail?: FetchAllowSoftFailMask): Promise<IObjectWithId>;
+	public async resolve(value: string | [string]): Promise<IObjectWithId>;
-	public async resolve(value: string | IObject | [string | IObject], allowSoftfail?: FetchAllowSoftFailMask): Promise<IObject>;
+	public async resolve(value: string | IObject | [string | IObject]): Promise<IObject>;
 	@bindThis
-	public async resolve(value: string | IObject | [string | IObject], allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObject> {
+	public async resolve(value: string | IObject | [string | IObject]): Promise<IObject> {
 		value = fromTuple(value);
 
 		if (typeof value !== 'string') {
@@ -116,13 +115,13 @@ export class Resolver {
 
 		const host = this.utilityService.extractDbHost(value);
 		if (this.config.activityLogging.enabled && !this.utilityService.isSelfHost(host)) {
-			return await this._resolveLogged(value, host, allowSoftfail);
+			return await this._resolveLogged(value, host);
 		} else {
-			return await this._resolve(value, host, allowSoftfail);
+			return await this._resolve(value, host);
 		}
 	}
 
-	private async _resolveLogged(requestUri: string, host: string, allowSoftfail: FetchAllowSoftFailMask): Promise<IObjectWithId> {
+	private async _resolveLogged(requestUri: string, host: string): Promise<IObjectWithId> {
 		const startTime = process.hrtime.bigint();
 
 		const log = await this.apLogService.createFetchLog({
@@ -131,7 +130,7 @@ export class Resolver {
 		});
 
 		try {
-			const result = await this._resolve(requestUri, host, allowSoftfail, log);
+			const result = await this._resolve(requestUri, host, log);
 
 			log.accepted = true;
 			log.result = 'ok';
@@ -151,7 +150,7 @@ export class Resolver {
 		}
 	}
 
-	private async _resolve(value: string, host: string, allowSoftfail: FetchAllowSoftFailMask, log?: SkApFetchLog): Promise<IObjectWithId> {
+	private async _resolve(value: string, host: string, log?: SkApFetchLog): Promise<IObjectWithId> {
 		if (value.includes('#')) {
 			// URLs with fragment parts cannot be resolved correctly because
 			// the fragment part does not get transmitted over HTTP(S).
@@ -182,8 +181,8 @@ export class Resolver {
 		}
 
 		const object = (this.user
-			? await this.apRequestService.signedGet(value, this.user, allowSoftfail) as IObject
+			? await this.apRequestService.signedGet(value, this.user)
-			: await this.httpRequestService.getActivityJson(value, allowSoftfail)) as IObject;
+			: await this.httpRequestService.getActivityJson(value));
 
 		if (log) {
 			const { object: objectOnly, context, contextHash } = extractObjectContext(object);

packages/backend/src/core/activitypub/misc/check-against-url.ts

@@ -1,38 +0,0 @@
-/*
- * SPDX-FileCopyrightText: dakkar and sharkey-project
- * SPDX-License-Identifier: AGPL-3.0-only
- */
-import type { IObject } from '../type.js';
-
-export enum FetchAllowSoftFailMask {
-	// Allow no softfail flags
-	Strict = 0,
-	// The values in tuple (requestUrl, finalUrl, objectId) are not all identical
-	//
-	// This condition is common for user-initiated lookups but should not be allowed in federation loop
-	//
-	// Allow variations:
-	//   good example: https://alice.example.com/@user -> https://alice.example.com/user/:userId
-	//   problematic example: https://alice.example.com/redirect?url=https://bad.example.com/ -> https://bad.example.com/ -> https://alice.example.com/somethingElse
-	NonCanonicalId = 1 << 0,
-	// Allow the final object to be at most one subdomain deeper than the request URL, similar to SPF relaxed alignment
-	//
-	// Currently no code path allows this flag to be set, but is kept in case of future use as some niche deployments do this, and we provide a pre-reviewed mechanism to opt-in.
-	//
-	// Allow variations:
-	//   good example: https://example.com/@user -> https://activitypub.example.com/@user { id: 'https://activitypub.example.com/@user' }
-	//   problematic example: https://example.com/@user -> https://untrusted.example.com/@user { id: 'https://untrusted.example.com/@user' }
-	MisalignedOrigin = 1 << 1,
-	// The requested URL has a different host than the returned object ID, although the final URL is still consistent with the object ID
-	//
-	// This condition is common for user-initiated lookups using an intermediate host but should not be allowed in federation loops
-	//
-	// Allow variations:
-	//   good example: https://alice.example.com/@user@bob.example.com -> https://bob.example.com/@user { id: 'https://bob.example.com/@user' }
-	//   problematic example: https://alice.example.com/definitelyAlice -> https://bob.example.com/@somebodyElse { id: 'https://bob.example.com/@somebodyElse' }
-	CrossOrigin = 1 << 2 | MisalignedOrigin,
-	// Allow all softfail flags
-	//
-	// do not use this flag on released code
-	Any = ~0,
-}

packages/backend/src/core/activitypub/models/ApNoteService.ts

@@ -550,29 +550,32 @@ export class ApNoteService {
 	 * リモートサーバーからフェッチしてMisskeyに登録しそれを返します。
 	 */
 	@bindThis
-	public async resolveNote(value: string | IObject, options: { sentFrom?: URL, resolver?: Resolver } = {}): Promise<MiNote | null> {
+	public async resolveNote(value: string | IObject, options: { sentFrom?: string, resolver?: Resolver } = {}): Promise<MiNote | null> {
 		const uri = getApId(value);
 
 		if (!this.utilityService.isFederationAllowedUri(uri)) {
+			// TODO convert to identifiable error
 			throw new StatusError(`blocked host: ${uri}`, 451, 'blocked host');
 		}
 
+		//#region このサーバーに既に登録されていたらそれを返す
+		const exist = await this.fetchNote(uri);
+		if (exist) return exist;
+		//#endregion
+
+		// Bail if local URI doesn't exist
+		if (this.utilityService.isUriLocal(uri)) {
+			// TODO convert to identifiable error
+			throw new StatusError(`cannot resolve local note: ${uri}`, 400, 'cannot resolve local note');
+		}
+
 		const unlock = await this.appLockService.getApLock(uri);
 
 		try {
-			//#region このサーバーに既に登録されていたらそれを返す
+			// Optimization: we can avoid re-fetching the value *if and only if* it matches the host authority that it was sent from.
-			const exist = await this.fetchNote(uri);
+			// Instances can create any object within their host authority, but anything outside of that MUST be untrusted.
-			if (exist) return exist;
+			const haveSameAuthority = options.sentFrom && this.apUtilityService.haveSameAuthority(options.sentFrom, uri);
-			//#endregion
+			const createFrom = haveSameAuthority ? value : uri;
-
-			if (this.utilityService.isUriLocal(uri)) {
-				throw new StatusError(`cannot resolve local note: ${uri}`, 400, 'cannot resolve local note');
-			}
-
-			// リモートサーバーからフェッチしてきて登録
-			// ここでuriの代わりに添付されてきたNote Objectが指定されていると、サーバーフェッチを経ずにノートが生成されるが
-			// 添付されてきたNote Objectは偽装されている可能性があるため、常にuriを指定してサーバーフェッチを行う。
-			const createFrom = options.sentFrom?.origin === new URL(uri).origin ? value : uri;
 			return await this.createNote(createFrom, undefined, options.resolver, true);
 		} finally {
 			unlock();

packages/backend/src/core/activitypub/models/ApPersonService.ts

@@ -40,6 +40,7 @@ import { RoleService } from '@/core/RoleService.js';
 import { DriveFileEntityService } from '@/core/entities/DriveFileEntityService.js';
 import type { AccountMoveService } from '@/core/AccountMoveService.js';
 import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
+import { AppLockService } from '@/core/AppLockService.js';
 import { getApId, getApType, isActor, isCollection, isCollectionOrOrderedCollection, isPropertyValue } from '../type.js';
 import { extractApHashtags } from './tag.js';
 import type { OnModuleInit } from '@nestjs/common';
@@ -107,6 +108,7 @@ export class ApPersonService implements OnModuleInit {
 
 		private roleService: RoleService,
 		private readonly apUtilityService: ApUtilityService,
+		private readonly appLockService: AppLockService,
 	) {
 	}
 
@@ -314,12 +316,17 @@ export class ApPersonService implements OnModuleInit {
 			throw new StatusError(`cannot resolve local user: ${uri}`, 400, 'cannot resolve local user');
 		}
 
+		return await this._createPerson(uri, resolver);
+	}
+
+	private async _createPerson(value: string | IObject, resolver?: Resolver): Promise<MiRemoteUser> {
+		const uri = getApId(value);
+		const host = this.utilityService.punyHost(uri);
+
 		// eslint-disable-next-line no-param-reassign
 		if (resolver == null) resolver = this.apResolverService.createResolver();
 
-		const object = await resolver.resolve(uri);
+		const object = await resolver.resolve(value);
-		if (object.id == null) throw new UnrecoverableError(`null object.id in ${uri}`);
-
 		const person = this.validateActor(object, uri);
 
 		this.logger.info(`Creating the Person: ${person.id}`);
@@ -685,16 +692,36 @@ export class ApPersonService implements OnModuleInit {
 	 * リモートサーバーからフェッチしてMisskeyに登録しそれを返します。
 	 */
 	@bindThis
-	public async resolvePerson(uri: string, resolver?: Resolver): Promise<MiLocalUser | MiRemoteUser> {
+	public async resolvePerson(value: string | IObject, resolver?: Resolver, sentFrom?: string): Promise<MiLocalUser | MiRemoteUser> {
+		const uri = getApId(value);
+
+		if (!this.utilityService.isFederationAllowedUri(uri)) {
+			// TODO convert to identifiable error
+			throw new StatusError(`blocked host: ${uri}`, 451, 'blocked host');
+		}
+
 		//#region このサーバーに既に登録されていたらそれを返す
 		const exist = await this.fetchPerson(uri);
 		if (exist) return exist;
 		//#endregion
 
-		// リモートサーバーからフェッチしてきて登録
+		// Bail if local URI doesn't exist
-		// eslint-disable-next-line no-param-reassign
+		if (this.utilityService.isUriLocal(uri)) {
-		if (resolver == null) resolver = this.apResolverService.createResolver();
+			// TODO convert to identifiable error
-		return await this.createPerson(uri, resolver);
+			throw new StatusError(`cannot resolve local person: ${uri}`, 400, 'cannot resolve local person');
+		}
+
+		const unlock = await this.appLockService.getApLock(uri);
+
+		try {
+			// Optimization: we can avoid re-fetching the value *if and only if* it matches the host authority that it was sent from.
+			// Instances can create any object within their host authority, but anything outside of that MUST be untrusted.
+			const haveSameAuthority = sentFrom && this.apUtilityService.haveSameAuthority(sentFrom, uri);
+			const createFrom = haveSameAuthority ? value : uri;
+			return await this._createPerson(createFrom, resolver);
+		} finally {
+			unlock();
+		}
 	}
 
 	@bindThis
@@ -748,7 +775,7 @@ export class ApPersonService implements OnModuleInit {
 			.slice(0, maxPinned)
 			.map(item => limit(() => this.apNoteService.resolveNote(item, {
 				resolver: _resolver,
-				sentFrom: new URL(user.uri),
+				sentFrom: user.uri,
 			}))));
 
 		await this.db.transaction(async transactionalEntityManager => {

packages/backend/src/server/api/endpoints/ap/show.ts

@@ -7,7 +7,7 @@ import { Inject, Injectable } from '@nestjs/common';
 import { Endpoint } from '@/server/api/endpoint-base.js';
 import type { MiNote } from '@/models/Note.js';
 import type { MiLocalUser, MiUser } from '@/models/User.js';
-import { isActor, isPost, getApId, getNullableApId } from '@/core/activitypub/type.js';
+import { isActor, isPost, getApId } from '@/core/activitypub/type.js';
 import type { SchemaType } from '@/misc/json-schema.js';
 import { ApResolverService } from '@/core/activitypub/ApResolverService.js';
 import { ApDbResolverService } from '@/core/activitypub/ApDbResolverService.js';
@@ -18,10 +18,9 @@ import { NoteEntityService } from '@/core/entities/NoteEntityService.js';
 import { UtilityService } from '@/core/UtilityService.js';
 import { bindThis } from '@/decorators.js';
 import { ApRequestService } from '@/core/activitypub/ApRequestService.js';
-import { InstanceActorService } from '@/core/InstanceActorService.js';
+import { SystemAccountService } from '@/core/SystemAccountService.js';
 import { ApiError } from '../../error.js';
 import { IdentifiableError } from '@/misc/identifiable-error.js';
-import { FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
 
 export const meta = {
 	tags: ['federation'],
@@ -119,7 +118,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 		private apPersonService: ApPersonService,
 		private apNoteService: ApNoteService,
 		private readonly apRequestService: ApRequestService,
-		private readonly instanceActorService: InstanceActorService,
+		private readonly systemAccountService: SystemAccountService,
 	) {
 		super(meta, paramDef, async (ps, me) => {
 			const object = await this.fetchAny(ps.uri, me);
@@ -140,7 +139,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 			throw new ApiError(meta.errors.federationNotAllowed);
 		}
 
-		let local = await this.mergePack(me, ...await Promise.all([
+		const local = await this.mergePack(me, ...await Promise.all([
 			this.apDbResolverService.getUserFromApId(uri),
 			this.apDbResolverService.getNoteFromApId(uri),
 		]));
@@ -149,7 +148,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 		// No local object found with that uri.
 		// Before we fetch, resolve the URI in case it has a cross-origin redirect or anything like that.
 		// Resolver.resolve() uses strict verification, which is overly paranoid for a user-provided lookup.
-		uri = await this.resolveCanonicalUri(uri); // eslint-disable-line no-param-reassign
+		uri = await this.resolveCanonicalUri(uri);
 		if (!this.utilityService.isFederationAllowedUri(uri)) {
 			throw new ApiError(meta.errors.federationNotAllowed);
 		}
@@ -161,8 +160,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 
 		// リモートから一旦オブジェクトフェッチ
 		const resolver = this.apResolverService.createResolver();
-		// allow ap/show exclusively to lookup URLs that are cross-origin or non-canonical (like https://alice.example.com/@bob@bob.example.com -> https://bob.example.com/@bob)
+		const object = await resolver.resolve(uri).catch((err) => {
-		const object = await resolver.resolve(uri, FetchAllowSoftFailMask.CrossOrigin | FetchAllowSoftFailMask.NonCanonicalId).catch((err) => {
 			if (err instanceof IdentifiableError) {
 				switch (err.id) {
 					// resolve
@@ -190,25 +188,13 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 			throw new ApiError(meta.errors.requestFailed);
 		});
 
-		if (object.id == null) {
+		// Object is already validated to have a valid id (URI).
-			throw new ApiError(meta.errors.responseInvalid);
+		// We can pass it through with the same resolver and sentFrom to avoid a duplicate fetch.
-		}
+		// The resolve* methods automatically check for locally cached copies.
-
-		// /@user のような正規id以外で取得できるURIが指定されていた場合、ここで初めて正規URIが確定する
-		// これはDBに存在する可能性があるため再度DB検索
-		if (uri !== object.id) {
-			local = await this.mergePack(me, ...await Promise.all([
-				this.apDbResolverService.getUserFromApId(object.id),
-				this.apDbResolverService.getNoteFromApId(object.id),
-			]));
-			if (local != null) return local;
-		}
-
-		// 同一ユーザーの情報を再度処理するので、使用済みのresolverを再利用してはいけない
 		return await this.mergePack(
 			me,
-			isActor(object) ? await this.apPersonService.createPerson(getApId(object)) : null,
+			isActor(object) ? await this.apPersonService.resolvePerson(object, resolver, uri) : null,
-			isPost(object) ? await this.apNoteService.createNote(getApId(object), undefined, undefined, true) : null,
+			isPost(object) ? await this.apNoteService.resolveNote(object, { resolver, sentFrom: uri }) : null,
 		);
 	}
 
@@ -239,8 +225,8 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 	 * Resolves an arbitrary URI to its canonical, post-redirect form.
 	 */
 	private async resolveCanonicalUri(uri: string): Promise<string> {
-		const user = await this.instanceActorService.getInstanceActor();
+		const user = await this.systemAccountService.getInstanceActor();
 		const res = await this.apRequestService.signedGet(uri, user, true);
-		return getNullableApId(res) ?? uri;
+		return getApId(res);
 	}
 }

packages/backend/test/unit/activitypub.ts

@@ -2,7 +2,6 @@
  * SPDX-FileCopyrightText: syuilo and misskey-project
  * SPDX-License-Identifier: AGPL-3.0-only
  */
-
 process.env.NODE_ENV = 'test';
 
 import * as assert from 'assert';

packages/backend/test/unit/ap-request.ts

@@ -8,7 +8,6 @@ import httpSignature from '@peertube/http-signature';
 
 import { genRsaKeyPair } from '@/misc/gen-key-pair.js';
 import { ApRequestCreator } from '@/core/activitypub/ApRequestService.js';
-import { assertActivityMatchesUrl, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
 import { IObject } from '@/core/activitypub/type.js';
 
 export const buildParsedSignature = (signingString: string, signature: string, algorithm: string) => {
@@ -65,6 +64,7 @@ describe('ap-request', () => {
 		assert.deepStrictEqual(result, true);
 	});
 
+	/*
 	test('rejects non matching domain', () => {
 		assert.doesNotThrow(() => assertActivityMatchesUrl(
 			'https://alice.example.com/abc',
@@ -168,4 +168,5 @@ describe('ap-request', () => {
 			FetchAllowSoftFailMask.Strict,
 		), 'throws if HTTP downgrade is detected');
 	});
+	*/
 });