merge: Remove assertActivityMatchesUrls in favor of three-way same-authority checks (resolves #956 and #914) (!914)

View MR for information: https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/914

Closes #956 and #914

Approved-by: dakkar <dakkar@thenautilus.net>
Approved-by: Marie <github@yuugi.dev>

packages/backend/src/core/CoreModule.ts

@@ -17,6 +17,8 @@ import { WebhookTestService } from '@/core/WebhookTestService.js';
 import { FlashService } from '@/core/FlashService.js';
 import { TimeService } from '@/core/TimeService.js';
 import { EnvService } from '@/core/EnvService.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
+import { ApLogService } from '@/core/ApLogService.js';
 import { AccountMoveService } from './AccountMoveService.js';
 import { AccountUpdateService } from './AccountUpdateService.js';
 import { AnnouncementService } from './AnnouncementService.js';
@@ -157,7 +159,6 @@ import { QueueService } from './QueueService.js';
 import { LoggerService } from './LoggerService.js';
 import { SponsorsService } from './SponsorsService.js';
 import type { Provider } from '@nestjs/common';
-import { ApLogService } from '@/core/ApLogService.js';
 
 //#region 文字列ベースでのinjection用(循環参照対応のため)
 const $LoggerService: Provider = { provide: 'LoggerService', useExisting: LoggerService };
@@ -308,6 +309,7 @@ const $ApMentionService: Provider = { provide: 'ApMentionService', useExisting:
 const $ApNoteService: Provider = { provide: 'ApNoteService', useExisting: ApNoteService };
 const $ApPersonService: Provider = { provide: 'ApPersonService', useExisting: ApPersonService };
 const $ApQuestionService: Provider = { provide: 'ApQuestionService', useExisting: ApQuestionService };
+const $ApUtilityService: Provider = { provide: 'ApUtilityService', useExisting: ApUtilityService };
 //#endregion
 
 const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: SponsorsService };
@@ -465,6 +467,7 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp
 		ApNoteService,
 		ApPersonService,
 		ApQuestionService,
+		ApUtilityService,
 		QueueService,
 
 		SponsorsService,
@@ -618,6 +621,7 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp
 		$ApNoteService,
 		$ApPersonService,
 		$ApQuestionService,
+		$ApUtilityService,
 		//#endregion
 
 		$SponsorsService,
@@ -771,6 +775,7 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp
 		ApNoteService,
 		ApPersonService,
 		ApQuestionService,
+		ApUtilityService,
 		QueueService,
 
 		SponsorsService,
@@ -922,6 +927,7 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp
 		$ApNoteService,
 		$ApPersonService,
 		$ApQuestionService,
+		$ApUtilityService,
 		//#endregion
 
 		$SponsorsService,

packages/backend/src/core/HttpRequestService.ts

@@ -16,8 +16,8 @@ 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 { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js';
+import { IObject } from '@/core/activitypub/type.js';
-import type { IObject } from '@/core/activitypub/type.js';
+import { ApUtilityService } from './activitypub/ApUtilityService.js';
 import type { Response } from 'node-fetch';
 import type { URL } from 'node:url';
 
@@ -145,6 +145,7 @@ export class HttpRequestService {
 	constructor(
 		@Inject(DI.config)
 		private config: Config,
+		private readonly apUtilityService: ApUtilityService,
 	) {
 		const cache = new CacheableLookup({
 			maxTtl: 3600,	// 1hours
@@ -198,6 +199,7 @@ export class HttpRequestService {
 	 * Get agent by URL
 	 * @param url URL
 	 * @param bypassProxy Allways bypass proxy
+	 * @param isLocalAddressAllowed
 	 */
 	@bindThis
 	public getAgentByUrl(url: URL, bypassProxy = false, isLocalAddressAllowed = false): http.Agent | https.Agent {
@@ -229,10 +231,11 @@ export class HttpRequestService {
 			validators: [validateContentTypeSetAsActivityPub],
 		});
 
-		const finalUrl = res.url; // redirects may have been involved
 		const activity = await res.json() as IObject;
 
-		assertActivityMatchesUrls(activity, [finalUrl]);
+		// 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);
 
 		return activity;
 	}

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

@@ -11,13 +11,12 @@ import { DI } from '@/di-symbols.js';
 import type { Config } from '@/config.js';
 import type { MiUser } from '@/models/User.js';
 import { UserKeypairService } from '@/core/UserKeypairService.js';
-import { UtilityService } from '@/core/UtilityService.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
 import { HttpRequestService } from '@/core/HttpRequestService.js';
 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 { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js';
 import type { IObject } from './type.js';
 
 type Request = {
@@ -148,7 +147,7 @@ export class ApRequestService {
 		private userKeypairService: UserKeypairService,
 		private httpRequestService: HttpRequestService,
 		private loggerService: LoggerService,
-		private utilityService: UtilityService,
+		private readonly apUtilityService: ApUtilityService,
 	) {
 		// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
 		this.logger = this.loggerService?.getLogger('ap-request'); // なぜか TypeError: Cannot read properties of undefined (reading 'getLogger') と言われる
@@ -183,9 +182,10 @@ export class ApRequestService {
 	 * Get AP object with http-signature
 	 * @param user http-signature user
 	 * @param url URL to fetch
+	 * @param followAlternate
 	 */
 	@bindThis
-	public async signedGet(url: string, user: { id: MiUser['id'] }, followAlternate?: boolean): Promise<object> {
+	public async signedGet(url: string, user: { id: MiUser['id'] }, followAlternate?: boolean): Promise<IObject> {
 		const _followAlternate = followAlternate ?? true;
 		const keypair = await this.userKeypairService.getUserKeypair(user.id);
 
@@ -253,7 +253,7 @@ export class ApRequestService {
 
 				if (alternate) {
 					const href = alternate.getAttribute('href');
-					if (href && this.utilityService.punyHostPSLDomain(url) === this.utilityService.punyHostPSLDomain(href)) {
+					if (href && this.apUtilityService.haveSameAuthority(url, href)) {
 						return await this.signedGet(href, user, false);
 					}
 				}
@@ -266,10 +266,12 @@ export class ApRequestService {
 		//#endregion
 
 		validateContentTypeSetAsActivityPub(res);
-		const finalUrl = res.url; // redirects may have been involved
+
 		const activity = await res.json() as IObject;
 
-		assertActivityMatchesUrls(activity, [finalUrl]);
+		// 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);
 
 		return activity;
 	}

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

@@ -18,7 +18,8 @@ import type Logger from '@/logger.js';
 import { fromTuple } from '@/misc/from-tuple.js';
 import { IdentifiableError } from '@/misc/identifiable-error.js';
 import { ApLogService, calculateDurationSince, extractObjectContext } from '@/core/ApLogService.js';
-import { getNullableApId, isCollectionOrOrderedCollection } from './type.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
+import { getApId, getNullableApId, isCollectionOrOrderedCollection } from './type.js';
 import { ApDbResolverService } from './ApDbResolverService.js';
 import { ApRendererService } from './ApRendererService.js';
 import { ApRequestService } from './ApRequestService.js';
@@ -45,6 +46,7 @@ export class Resolver {
 		private apDbResolverService: ApDbResolverService,
 		private loggerService: LoggerService,
 		private readonly apLogService: ApLogService,
+		private readonly apUtilityService: ApUtilityService,
 		private recursionLimit = 256,
 	) {
 		this.history = new Set();
@@ -176,20 +178,16 @@ export class Resolver {
 			throw new IdentifiableError('72180409-793c-4973-868e-5a118eb5519b', `invalid AP object ${value}: does not have ActivityStreams context`);
 		}
 
-		// Since redirects are allowed, we cannot safely validate an anonymous object.
+		// The object ID is already validated to match the final URL's authority by signedGet / getActivityJson.
-		// Reject any responses without an ID, as all other checks depend on that value.
+		// We only need to validate that it also matches the original URL's authority, in case of redirects.
-		if (object.id == null) {
+		const objectId = getApId(object);
-			throw new IdentifiableError('ad2dc287-75c1-44c4-839d-3d2e64576675', `invalid AP object ${value}: missing id`);
-		}
 
 		// We allow some limited cross-domain redirects, which means the host may have changed during fetch.
 		// Additional checks are needed to validate the scope of cross-domain redirects.
-		const finalHost = this.utilityService.extractDbHost(object.id);
+		const finalHost = this.utilityService.extractDbHost(objectId);
 		if (finalHost !== host) {
 			// Make sure the redirect stayed within the same authority.
-			if (this.utilityService.punyHostPSLDomain(object.id) !== this.utilityService.punyHostPSLDomain(value)) {
+			this.apUtilityService.assertIdMatchesUrlAuthority(object, value);
-				throw new IdentifiableError('fd93c2fa-69a8-440f-880b-bf178e0ec877', `invalid AP object ${value}: id ${object.id} has different host`);
-			}
 
 			// Check if the redirect bounce from [allowed domain] to [blocked domain].
 			if (!this.utilityService.isFederationAllowedHost(finalHost)) {
@@ -287,6 +285,7 @@ export class ApResolverService {
 		private apDbResolverService: ApDbResolverService,
 		private loggerService: LoggerService,
 		private readonly apLogService: ApLogService,
+		private readonly apUtilityService: ApUtilityService,
 	) {
 	}
 
@@ -308,6 +307,7 @@ export class ApResolverService {
 			this.apDbResolverService,
 			this.loggerService,
 			this.apLogService,
+			this.apUtilityService,
 		);
 	}
 }

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

@@ -0,0 +1,108 @@
+/*
+ * SPDX-FileCopyrightText: hazelnoot and other Sharkey contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+import { Injectable } from '@nestjs/common';
+import { UtilityService } from '@/core/UtilityService.js';
+import { IdentifiableError } from '@/misc/identifiable-error.js';
+import { toArray } from '@/misc/prelude/array.js';
+import { EnvService } from '@/core/EnvService.js';
+import { getApId, getOneApHrefNullable, IObject } from './type.js';
+
+@Injectable()
+export class ApUtilityService {
+	constructor(
+		private readonly utilityService: UtilityService,
+		private readonly envService: EnvService,
+	) {}
+
+	/**
+	 * Verifies that the object's ID has the same authority as the provided URL.
+	 * Returns on success, throws on any validation error.
+	 */
+	public assertIdMatchesUrlAuthority(object: IObject, url: string): void {
+		// This throws if the ID is missing or invalid, but that's ok.
+		// Anonymous objects are impossible to verify, so we don't allow fetching them.
+		const id = getApId(object);
+
+		// 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.
+		if (!this.haveSameAuthority(url, id)) {
+			throw new IdentifiableError('fd93c2fa-69a8-440f-880b-bf178e0ec877', `invalid AP object ${url}: id ${id} has different host authority`);
+		}
+	}
+
+	/**
+	 * Checks if two URLs have the same host authority
+	 */
+	public haveSameAuthority(url1: string, url2: string): boolean {
+		if (url1 === url2) return true;
+
+		const authority1 = this.utilityService.punyHostPSLDomain(url1);
+		const authority2 = this.utilityService.punyHostPSLDomain(url2);
+		return authority1 === authority2;
+	}
+
+	/**
+	 * Finds the "best" URL for a given AP object.
+	 * The list of URLs is first filtered via findSameAuthorityUrl, then further filtered based on mediaType, and finally sorted to select the best one.
+	 * @throws {IdentifiableError} if object does not have an ID
+	 * @returns the best URL, or null if none were found
+	 */
+	public findBestObjectUrl(object: IObject): string | null {
+		const targetUrl = getApId(object);
+		const targetAuthority = this.utilityService.punyHostPSLDomain(targetUrl);
+
+		const rawUrls = toArray(object.url);
+		const acceptableUrls = rawUrls
+			.map(raw => ({
+				url: getOneApHrefNullable(raw),
+				type: typeof(raw) === 'object'
+					? raw.mediaType?.toLowerCase()
+					: undefined,
+			}))
+			.filter(({ url, type }) => {
+				if (!url) return false;
+				if (!this.checkHttps(url)) return false;
+				if (!isAcceptableUrlType(type)) return false;
+
+				const urlAuthority = this.utilityService.punyHostPSLDomain(url);
+				return urlAuthority === targetAuthority;
+			})
+			.sort((a, b) => {
+				return rankUrlType(a.type) - rankUrlType(b.type);
+			});
+
+		return acceptableUrls[0]?.url ?? null;
+	}
+
+	/**
+	 * Checks if the URL contains HTTPS.
+	 * Additionally, allows HTTP in non-production environments.
+	 * Based on check-https.ts.
+	 */
+	private checkHttps(url: string): boolean {
+		const isNonProd = this.envService.env.NODE_ENV !== 'production';
+
+		// noinspection HttpUrlsUsage
+		return url.startsWith('https://') || (url.startsWith('http://') && isNonProd);
+	}
+}
+
+function isAcceptableUrlType(type: string | undefined): boolean {
+	if (!type) return true;
+	if (type.startsWith('text/')) return true;
+	if (type.startsWith('application/ld+json')) return true;
+	if (type.startsWith('application/activity+json')) return true;
+	return false;
+}
+
+function rankUrlType(type: string | undefined): number {
+	if (!type) return 2;
+	if (type === 'text/html') return 0;
+	if (type.startsWith('text/')) return 1;
+	if (type.startsWith('application/ld+json')) return 3;
+	if (type.startsWith('application/activity+json')) return 4;
+	return 5;
+}

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

@@ -1,31 +0,0 @@
-/*
- * SPDX-FileCopyrightText: dakkar and sharkey-project
- * SPDX-License-Identifier: AGPL-3.0-only
- */
-
-import { UnrecoverableError } from 'bullmq';
-import type { IObject } from '../type.js';
-
-function getHrefsFrom(one: IObject | string | undefined | (IObject | string | undefined)[]): (string | undefined)[] {
-	if (Array.isArray(one)) {
-		return one.flatMap(h => getHrefsFrom(h));
-	}
-	return [
-		typeof(one) === 'object' ? one.href : one,
-	];
-}
-
-export function assertActivityMatchesUrls(activity: IObject, urls: string[]) {
-	const expectedUrls = new Set(urls
-		.filter(u => URL.canParse(u))
-		.map(u => new URL(u).href),
-	);
-
-	const actualUrls = [activity.id, ...getHrefsFrom(activity.url)]
-		.filter(u => u && URL.canParse(u))
-		.map(u => new URL(u as string).href);
-
-	if (!actualUrls.some(u => expectedUrls.has(u))) {
-		throw new UnrecoverableError(`bad Activity: neither id nor url (${actualUrls.join(', ')}) match location (${urls.join(', ')})`);
-	}
-}

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

@@ -26,12 +26,13 @@ import { bindThis } from '@/decorators.js';
 import { checkHttps } from '@/misc/check-https.js';
 import { IdentifiableError } from '@/misc/identifiable-error.js';
 import { isRetryableError } from '@/misc/is-retryable-error.js';
-import { getOneApId, getApId, getOneApHrefNullable, validPost, isEmoji, getApType, isApObject, isDocument, IApDocument } from '../type.js';
+import { getOneApId, getApId, validPost, isEmoji, getApType, isApObject, isDocument, IApDocument } from '../type.js';
 import { ApLoggerService } from '../ApLoggerService.js';
 import { ApMfmService } from '../ApMfmService.js';
 import { ApDbResolverService } from '../ApDbResolverService.js';
 import { ApResolverService } from '../ApResolverService.js';
 import { ApAudienceService } from '../ApAudienceService.js';
+import { ApUtilityService } from '../ApUtilityService.js';
 import { ApPersonService } from './ApPersonService.js';
 import { extractApHashtags } from './tag.js';
 import { ApMentionService } from './ApMentionService.js';
@@ -82,6 +83,7 @@ export class ApNoteService {
 		private noteEditService: NoteEditService,
 		private apDbResolverService: ApDbResolverService,
 		private apLoggerService: ApLoggerService,
+		private readonly apUtilityService: ApUtilityService,
 	) {
 		this.logger = this.apLoggerService.logger;
 	}
@@ -92,7 +94,6 @@ export class ApNoteService {
 		uri: string,
 		actor?: MiRemoteUser,
 		user?: MiRemoteUser,
-		note?: MiNote,
 	): Error | null {
 		const expectHost = this.utilityService.extractDbHost(uri);
 		const apType = getApType(object);
@@ -124,13 +125,6 @@ export class ApNoteService {
 			}
 		}
 
-		if (note) {
-			const url = (object.url) ? getOneApId(object.url) : note.url;
-			if (url && url !== note.url) {
-				return new IdentifiableError('d450b8a9-48e4-4dab-ae36-f4db763fda7c', `invalid Note: updated url does not match original url. updated url: ${url}, original url: ${note.url}`);
-			}
-		}
-
 		return null;
 	}
 
@@ -186,17 +180,7 @@ export class ApNoteService {
 			throw new UnrecoverableError(`unexpected schema of note.id ${note.id} in ${entryUri}`);
 		}
 
-		const url = getOneApHrefNullable(note.url);
+		const url = this.apUtilityService.findBestObjectUrl(note);
-
-		if (url != null) {
-			if (!checkHttps(url)) {
-				throw new UnrecoverableError(`unexpected schema of note.url ${url} in ${entryUri}`);
-			}
-
-			if (this.utilityService.punyHostPSLDomain(url) !== this.utilityService.punyHostPSLDomain(note.id)) {
-				throw new UnrecoverableError(`note url <> uri host mismatch: ${url} <> ${note.id} in ${entryUri}`);
-			}
-		}
 
 		this.logger.info(`Creating the Note: ${note.id}`);
 
@@ -385,7 +369,7 @@ export class ApNoteService {
 		const object = await resolver.resolve(value);
 
 		const entryUri = getApId(value);
-		const err = this.validateNote(object, entryUri, actor, user, updatedNote);
+		const err = this.validateNote(object, entryUri, actor, user);
 		if (err) {
 			this.logger.error(err.message, {
 				resolver: { history: resolver.getHistory() },
@@ -411,17 +395,7 @@ export class ApNoteService {
 			throw new UnrecoverableError(`unexpected schema of note.id ${note.id} in ${noteUri}`);
 		}
 
-		const url = getOneApHrefNullable(note.url);
+		const url = this.apUtilityService.findBestObjectUrl(note);
-
-		if (url != null) {
-			if (!checkHttps(url)) {
-				throw new UnrecoverableError(`unexpected schema of note.url ${url} in ${noteUri}`);
-			}
-
-			if (this.utilityService.punyHostPSLDomain(url) !== this.utilityService.punyHostPSLDomain(note.id)) {
-				throw new UnrecoverableError(`note url <> id host mismatch: ${url} <> ${note.id} in ${noteUri}`);
-			}
-		}
 
 		this.logger.info(`Creating the Note: ${note.id}`);
 

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

@@ -39,8 +39,8 @@ import { bindThis } from '@/decorators.js';
 import { RoleService } from '@/core/RoleService.js';
 import { DriveFileEntityService } from '@/core/entities/DriveFileEntityService.js';
 import type { AccountMoveService } from '@/core/AccountMoveService.js';
-import { checkHttps } from '@/misc/check-https.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
-import { getApId, getApType, getOneApHrefNullable, isActor, isCollection, isCollectionOrOrderedCollection, isPropertyValue } from '../type.js';
+import { getApId, getApType, isActor, isCollection, isCollectionOrOrderedCollection, isPropertyValue } from '../type.js';
 import { extractApHashtags } from './tag.js';
 import type { OnModuleInit } from '@nestjs/common';
 import type { ApNoteService } from './ApNoteService.js';
@@ -106,6 +106,7 @@ export class ApPersonService implements OnModuleInit {
 		private followingsRepository: FollowingsRepository,
 
 		private roleService: RoleService,
+		private readonly apUtilityService: ApUtilityService,
 	) {
 	}
 
@@ -346,21 +347,11 @@ export class ApPersonService implements OnModuleInit {
 
 		const bday = person['vcard:bday']?.match(/^\d{4}-\d{2}-\d{2}/);
 
-		const url = getOneApHrefNullable(person.url);
-
 		if (person.id == null) {
 			throw new UnrecoverableError(`Refusing to create person without id: ${uri}`);
 		}
 
-		if (url != null) {
+		const url = this.apUtilityService.findBestObjectUrl(person);
-			if (!checkHttps(url)) {
-				throw new UnrecoverableError(`unexpected schema of person url ${url} in ${uri}`);
-			}
-
-			if (this.utilityService.punyHostPSLDomain(url) !== this.utilityService.punyHostPSLDomain(person.id)) {
-				throw new UnrecoverableError(`person url <> uri host mismatch: ${url} <> ${person.id} in ${uri}`);
-			}
-		}
 
 		// Create user
 		let user: MiRemoteUser | null = null;
@@ -566,21 +557,11 @@ export class ApPersonService implements OnModuleInit {
 
 		const bday = person['vcard:bday']?.match(/^\d{4}-\d{2}-\d{2}/);
 
-		const url = getOneApHrefNullable(person.url);
-
 		if (person.id == null) {
 			throw new UnrecoverableError(`Refusing to update person without id: ${uri}`);
 		}
 
-		if (url != null) {
+		const url = this.apUtilityService.findBestObjectUrl(person);
-			if (!checkHttps(url)) {
-				throw new UnrecoverableError(`unexpected schema of person url ${url} in ${uri}`);
-			}
-
-			if (this.utilityService.punyHostPSLDomain(url) !== this.utilityService.punyHostPSLDomain(person.id)) {
-				throw new UnrecoverableError(`person url <> uri host mismatch: ${url} <> ${person.id} in ${uri}`);
-			}
-		}
 
 		const updates = {
 			lastFetchedAt: new Date(),

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

@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: AGPL-3.0-only
  */
 
-import { UnrecoverableError } from 'bullmq';
+import { IdentifiableError } from '@/misc/identifiable-error.js';
 import { fromTuple } from '@/misc/from-tuple.js';
 
 export type Obj = { [x: string]: any };
@@ -56,29 +56,22 @@ export function getOneApId(value: ApObject): string {
 	return getApId(firstOne);
 }
 
-/**
- * Minimal AP payload - just an object with optional ID.
- */
-export interface ObjectWithId {
-	id?: string;
-}
-
 /**
  * Get ActivityStreams Object id
  */
-export function getApId(value: string | ObjectWithId | [string | ObjectWithId]): string {
+export function getApId(value: string | IObject | [string | IObject]): string {
 	// eslint-disable-next-line no-param-reassign
 	value = fromTuple(value);
 
 	if (typeof value === 'string') return value;
 	if (typeof value.id === 'string') return value.id;
-	throw new UnrecoverableError('cannot determine id');
+	throw new IdentifiableError('ad2dc287-75c1-44c4-839d-3d2e64576675', `invalid AP object ${value}: missing id`);
 }
 
 /**
  * Get ActivityStreams Object id, or null if not present
  */
-export function getNullableApId(value: string | ObjectWithId | [string | ObjectWithId]): string | null {
+export function getNullableApId(value: string | IObject | [string | IObject]): string | null {
 	// eslint-disable-next-line no-param-reassign
 	value = fromTuple(value);
 

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, ObjectWithId } from '@/core/activitypub/type.js';
+import { isActor, isPost, getApId, getNullableApId } 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';
@@ -154,7 +154,9 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 		// 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
-		if (!this.utilityService.isFederationAllowedUri(uri)) return null;
+		if (!this.utilityService.isFederationAllowedUri(uri)) {
+			throw new ApiError(meta.errors.federationNotAllowed);
+		}
 
 		const host = this.utilityService.extractDbHost(uri);
 
@@ -244,7 +246,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
 	 */
 	private async resolveCanonicalUri(uri: string): Promise<string> {
 		const user = await this.instanceActorService.getInstanceActor();
-		const res = await this.apRequestService.signedGet(uri, user, true) as ObjectWithId;
+		const res = await this.apRequestService.signedGet(uri, user, true);
 		return getNullableApId(res) ?? uri;
 	}
 }

packages/backend/test/misc/mock-resolver.ts

@@ -24,6 +24,7 @@ import type {
 	UsersRepository,
 } from '@/models/_.js';
 import { ApLogService } from '@/core/ApLogService.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
 
 type MockResponse = {
 	type: string;
@@ -51,6 +52,7 @@ export class MockResolver extends Resolver {
 			{} as ApDbResolverService,
 			loggerService,
 			{} as ApLogService,
+			{} as ApUtilityService,
 		);
 	}
 

packages/backend/test/unit/core/activitypub/ApUtilityService.ts

@@ -0,0 +1,354 @@
+/*
+ * SPDX-FileCopyrightText: hazelnoot and other Sharkey contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+import type { UtilityService } from '@/core/UtilityService.js';
+import type { IObject } from '@/core/activitypub/type.js';
+import type { EnvService } from '@/core/EnvService.js';
+import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
+
+describe(ApUtilityService, () => {
+	let serviceUnderTest: ApUtilityService;
+	let env: Record<string, string>;
+
+	beforeEach(() => {
+		const utilityService = {
+			punyHostPSLDomain(input: string) {
+				const host = new URL(input).host;
+				const parts = host.split('.');
+				return `${parts[parts.length - 2]}.${parts[parts.length - 1]}`;
+			},
+		} as unknown as UtilityService;
+
+		env = {};
+		const envService = {
+			env,
+		} as unknown as EnvService;
+
+		serviceUnderTest = new ApUtilityService(utilityService, envService);
+	});
+
+	describe('assertIdMatchesUrlAuthority', () => {
+		it('should return when input matches', () => {
+			const object = { id: 'https://first.example.com' } as IObject;
+			const url = 'https://second.example.com';
+
+			expect(() => {
+				serviceUnderTest.assertIdMatchesUrlAuthority(object, url);
+			}).not.toThrow();
+		});
+
+		it('should throw when id is missing', () => {
+			const object = { id: undefined } as IObject;
+			const url = 'https://second.example.com';
+
+			expect(() => {
+				serviceUnderTest.assertIdMatchesUrlAuthority(object, url);
+			}).toThrow();
+		});
+
+		it('should throw when id does not match', () => {
+			const object = { id: 'https://other-domain.com' } as IObject;
+			const url = 'https://second.example.com';
+
+			expect(() => {
+				serviceUnderTest.assertIdMatchesUrlAuthority(object, url);
+			}).toThrow();
+		});
+	});
+
+	describe('haveSameAuthority', () => {
+		it('should return true when URLs match', () => {
+			const url = 'https://example.com';
+
+			const result = serviceUnderTest.haveSameAuthority(url, url);
+
+			expect(result).toBeTruthy();
+		});
+
+		it('should return true when URLs have same host', () => {
+			const first = 'https://example.com/first';
+			const second = 'https://example.com/second';
+
+			const result = serviceUnderTest.haveSameAuthority(first, second);
+
+			expect(result).toBeTruthy();
+		});
+
+		it('should return true when URLs have same authority', () => {
+			const first = 'https://first.example.com/first';
+			const second = 'https://second.example.com/second';
+
+			const result = serviceUnderTest.haveSameAuthority(first, second);
+
+			expect(result).toBeTruthy();
+		});
+
+		it('should return false when URLs have different authority', () => {
+			const first = 'https://first.com';
+			const second = 'https://second.com';
+
+			const result = serviceUnderTest.haveSameAuthority(first, second);
+
+			expect(result).toBeFalsy();
+		});
+	});
+
+	describe('findBestObjectUrl', () => {
+		it('should return null when input is undefined', () => {
+			const object = {
+				id: 'https://example.com',
+				url: undefined,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBeNull();
+		});
+
+		it('should return null when input is empty array', () => {
+			const object = {
+				id: 'https://example.com',
+				url: [] as string[],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBeNull();
+		});
+
+		it('should return return url if string input matches', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: 'https://example.com/2',
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should return return url if object input matches', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should return return url if string[] input matches', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: ['https://example.com/2'],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should return return url if object[] input matches', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: [{
+					href: 'https://example.com/2',
+				} as IObject],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should skip invalid entries', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: [{} as IObject, 'https://example.com/2'],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow empty mediaType', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow text/html mediaType', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+					mediaType: 'text/html',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow other text/ mediaTypes', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+					mediaType: 'text/imaginary',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow application/ld+json mediaType', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+					mediaType: 'application/ld+json;profile=https://www.w3.org/ns/activitystreams',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow application/activity+json mediaType', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: {
+					href: 'https://example.com/2',
+					mediaType: 'application/activity+json',
+				} as IObject,
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should reject other mediaTypes', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: [
+					{
+						href: 'https://example.com/2',
+						mediaType: 'application/json',
+					} as IObject,
+					{
+						href: 'https://example.com/3',
+						mediaType: 'image/jpeg',
+					} as IObject,
+				],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBeNull();
+		});
+
+		it('should return best match', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: [
+					'https://example.com/2',
+					{
+						href: 'https://example.com/3',
+					} as IObject,
+					{
+						href: 'https://example.com/4',
+						mediaType: 'text/html',
+					} as IObject,
+					{
+						href: 'https://example.com/5',
+						mediaType: 'text/plain',
+					} as IObject,
+					{
+						href: 'https://example.com/6',
+						mediaType: 'application/ld+json',
+					} as IObject,
+					{
+						href: 'https://example.com/7',
+						mediaType: 'application/activity+json',
+					} as IObject,
+					{
+						href: 'https://example.com/8',
+						mediaType: 'image/jpeg',
+					} as IObject,
+				],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/4');
+		});
+
+		it('should return first match in case of ties', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: ['https://example.com/2', 'https://example.com/3'],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should skip invalid scheme', () => {
+			const object = {
+				id: 'https://example.com/1',
+				url: ['file://example.com/1', 'https://example.com/2'],
+			} as IObject;
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should skip HTTP in production', () => {
+			// noinspection HttpUrlsUsage
+			const object = {
+				id: 'https://example.com/1',
+				url: ['http://example.com/1', 'https://example.com/2'],
+			} as IObject;
+			env.NODE_ENV = 'production';
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			expect(result).toBe('https://example.com/2');
+		});
+
+		it('should allow HTTP in non-prod', () => {
+			// noinspection HttpUrlsUsage
+			const object = {
+				id: 'https://example.com/1',
+				url: ['http://example.com/1', 'https://example.com/2'],
+			} as IObject;
+			env.NODE_ENV = 'test';
+
+			const result = serviceUnderTest.findBestObjectUrl(object);
+
+			// noinspection HttpUrlsUsage
+			expect(result).toBe('http://example.com/1');
+		});
+	});
+});

packages/backend/test/unit/misc/check-against-url.ts

@@ -1,55 +0,0 @@
-/*
- * SPDX-FileCopyrightText: dakkar and sharkey-project
- * SPDX-License-Identifier: AGPL-3.0-only
- */
-
-import { describe, expect, test } from '@jest/globals';
-import type { IObject } from '@/core/activitypub/type.js';
-import { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js';
-
-function assertOne(activity: IObject, good = 'http://good') {
-	// return a function so we can use `.toThrow`
-	return () => assertActivityMatchesUrls(activity, [good]);
-}
-
-describe('assertActivityMatchesUrls', () => {
-	it('should throw when no ids are URLs', () => {
-		expect(assertOne({ type: 'Test', id: 'bad' }, 'bad')).toThrow(/bad Activity/);
-	});
-
-	test('id', () => {
-		expect(assertOne({ type: 'Test', id: 'http://bad' })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', id: 'http://good' })).not.toThrow();
-	});
-
-	test('simple url', () => {
-		expect(assertOne({ type: 'Test', url: 'http://bad' })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: 'http://good' })).not.toThrow();
-	});
-
-	test('array of urls', () => {
-		expect(assertOne({ type: 'Test', url: ['http://bad'] })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: ['http://bad', 'http://other'] })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: ['http://good'] })).not.toThrow();
-		expect(assertOne({ type: 'Test', url: ['http://bad', 'http://good'] })).not.toThrow();
-	});
-
-	test('array of objects', () => {
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://bad' }] })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://bad' }, { type: 'Test', href: 'http://other' }] })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://good' }] })).not.toThrow();
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://bad' }, { type: 'Test', href: 'http://good' }] })).not.toThrow();
-	});
-
-	test('mixed array', () => {
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://bad' }, 'http://other'] })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', url: [{ type: 'Test', href: 'http://bad' }, 'http://good'] })).not.toThrow();
-		expect(assertOne({ type: 'Test', url: ['http://bad', { type: 'Test', href: 'http://good' }] })).not.toThrow();
-	});
-
-	test('id and url', () => {
-		expect(assertOne({ type: 'Test', id: 'http://other', url: 'http://bad' })).toThrow(/bad Activity/);
-		expect(assertOne({ type: 'Test', id: 'http://bad', url: 'http://good' })).not.toThrow();
-		expect(assertOne({ type: 'Test', id: 'http://good', url: 'http://bad' })).not.toThrow();
-	});
-});