refactor actor validation to reduce code duplication

packages/backend/src/core/UtilityService.ts

@@ -10,8 +10,8 @@ import psl from 'psl';
 import { DI } from '@/di-symbols.js';
 import type { Config } from '@/config.js';
 import { bindThis } from '@/decorators.js';
-import { MiMeta, SoftwareSuspension } from '@/models/Meta.js';
-import { MiInstance } from '@/models/Instance.js';
+import type { MiMeta } from '@/models/Meta.js';
+import type { MiInstance } from '@/models/Instance.js';
 import { IdentifiableError } from '@/misc/identifiable-error.js';
 import { EnvService } from '@/core/EnvService.js';
 

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

@@ -7,18 +7,29 @@ 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 { getApId, getOneApHrefNullable, IObject } from '@/core/activitypub/type.js';
+import { getApId, getNullableApId, getOneApHrefNullable } from '@/core/activitypub/type.js';
+import type { IObject, IObjectWithId } from '@/core/activitypub/type.js';
+import { bindThis } from '@/decorators.js';
+import { renderInlineError } from '@/misc/render-inline-error.js';
+import { LoggerService } from '@/core/LoggerService.js';
+import type Logger from '@/logger.js';
 
 @Injectable()
 export class ApUtilityService {
+	private readonly logger: Logger;
+
 	constructor(
 		private readonly utilityService: UtilityService,
-	) {}
+		loggerService: LoggerService,
+	) {
+		this.logger = loggerService.getLogger('ap-utility');
+	}
 
 	/**
 	 * Verifies that the object's ID has the same authority as the provided URL.
 	 * Returns on success, throws on any validation error.
 	 */
+	@bindThis
 	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.
@@ -34,6 +45,7 @@ export class ApUtilityService {
 	/**
 	 * Checks if two URLs have the same host authority
 	 */
+	@bindThis
 	public haveSameAuthority(url1: string, url2: string): boolean {
 		if (url1 === url2) return true;
 
@@ -51,6 +63,7 @@ export class ApUtilityService {
 	 * @throws {IdentifiableError} if object does not have an ID
 	 * @returns the best URL, or null if none were found
 	 */
+	@bindThis
 	public findBestObjectUrl(object: IObject): string | null {
 		const targetUrl = getApId(object);
 		const targetAuthority = this.utilityService.punyHostPSLDomain(targetUrl);
@@ -81,6 +94,75 @@ export class ApUtilityService {
 
 		return acceptableUrls[0]?.url ?? null;
 	}
+
+	/**
+	 * Sanitizes an inline / nested Object property within an AP object.
+	 *
+	 * Returns true if the property contains a valid string URL, object w/ valid ID, or an array containing one of those.
+	 * Returns false and erases the property if it doesn't contain a valid value.
+	 *
+	 * Arrays are automatically flattened.
+	 * Falsy values (including null) are collapsed to undefined.
+	 * @param obj Object containing the property to validate
+	 * @param key Key of the property in obj
+	 * @param parentUri URI of the object that contains this inline object.
+	 * @param parentHost PSL host of parentUri
+	 * @param keyPath If obj is *itself* a nested object, set this to the property path from root to obj (including the trailing '.'). This does not affect the logic, but improves the clarity of logs.
+	 */
+	@bindThis
+	public sanitizeInlineObject<Key extends string>(obj: Partial<Record<Key, string | { id?: string } | (string | { id?: string })[]>>, key: Key, parentUri: string | URL, parentHost: string, keyPath = ''): obj is Partial<Record<Key, string | { id: string }>> {
+		let value: unknown = obj[key];
+
+		// Unpack arrays
+		if (Array.isArray(value)) {
+			value = value[0];
+		}
+
+		// Clear the value - we'll add it back once we have a confirmed ID
+		obj[key] = undefined;
+
+		// Collapse falsy values to undefined
+		if (!value) {
+			return false;
+		}
+
+		// Exclude nested arrays
+		if (Array.isArray(value)) {
+			this.logger.warn(`Excluding ${keyPath}${key} from object ${parentUri}: nested arrays are prohibited`);
+			return false;
+		}
+
+		// Exclude incorrect types
+		if (typeof(value) !== 'string' && typeof(value) !== 'object') {
+			this.logger.warn(`Excluding ${keyPath}${key} from object ${parentUri}: incorrect type ${typeof(value)}`);
+			return false;
+		}
+
+		const valueId = getNullableApId(value);
+		if (!valueId) {
+			// Exclude missing ID
+			this.logger.warn(`Excluding ${keyPath}${key} from object ${parentUri}: missing or invalid ID`);
+			return false;
+		}
+
+		try {
+			const parsed = this.utilityService.assertUrl(valueId);
+			const parsedHost = this.utilityService.punyHostPSLDomain(parsed);
+			if (parsedHost !== parentHost) {
+				// Exclude wrong host
+				this.logger.warn(`Excluding ${keyPath}${key} from object ${parentUri}: wrong host in ${valueId} (got ${parsedHost}, expected ${parentHost})`);
+				return false;
+			}
+		} catch (err) {
+			// Exclude invalid URLs
+			this.logger.warn(`Excluding ${keyPath}${key} from object ${parentUri}: invalid URL ${valueId}: ${renderInlineError(err)}`);
+			return false;
+		}
+
+		// Success - store the sanitized value and return
+		obj[key] = value as string | IObjectWithId;
+		return true;
+	}
 }
 
 function isAcceptableUrlType(type: string | undefined): boolean {

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

@@ -46,7 +46,7 @@ import { verifyFieldLinks } from '@/misc/verify-field-link.js';
 import { isRetryableError } from '@/misc/is-retryable-error.js';
 import { renderInlineError } from '@/misc/render-inline-error.js';
 import { IdentifiableError } from '@/misc/identifiable-error.js';
-import { getApId, getApType, getNullableApId, 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';
@@ -159,46 +159,32 @@ export class ApPersonService implements OnModuleInit, OnApplicationShutdown {
 		const parsedUri = this.utilityService.assertUrl(uri);
 		const expectHost = this.utilityService.punyHostPSLDomain(parsedUri);
 
+		// Validate type
 		if (!isActor(x)) {
 			throw new UnrecoverableError(`invalid Actor ${uri}: unknown type '${x.type}'`);
 		}
 
-		if (!(typeof x.id === 'string' && x.id.length > 0)) {
-			throw new UnrecoverableError(`invalid Actor ${uri}: wrong id type`);
+		// Validate id
+		if (!x.id) {
+			throw new UnrecoverableError(`invalid Actor ${uri}: missing id`);
+		}
+		if (typeof(x.id) !== 'string') {
+			throw new UnrecoverableError(`invalid Actor ${uri}: wrong id type ${typeof(x.id)}`);
+		}
+		const parsedId = this.utilityService.assertUrl(x.id);
+		const idHost = this.utilityService.punyHostPSLDomain(parsedId);
+		if (idHost !== expectHost) {
+			throw new UnrecoverableError(`invalid Actor ${uri}: wrong host in id ${x.id} (got ${parsedId}, expected ${expectHost})`);
 		}
 
-		if (!(typeof x.inbox === 'string' && x.inbox.length > 0)) {
-			throw new UnrecoverableError(`invalid Actor ${uri}: wrong inbox type`);
-		}
-
-		this.utilityService.assertUrl(x.inbox);
-		const inboxHost = this.utilityService.punyHostPSLDomain(x.inbox);
-		if (inboxHost !== expectHost) {
-			throw new UnrecoverableError(`invalid Actor ${uri}: wrong inbox host ${inboxHost}`);
+		// Validate inbox
+		this.apUtilityService.sanitizeInlineObject(x, 'inbox', parsedUri, expectHost);
+		if (!x.inbox || typeof(x.inbox) !== 'string') {
+			throw new UnrecoverableError(`invalid Actor ${uri}: missing or invalid inbox ${x.inbox}`);
 		}
 
 		// Sanitize sharedInbox
-		try {
-			if (x.sharedInbox) {
-				const sharedInbox = getNullableApId(x.sharedInbox);
-				if (sharedInbox) {
-					const parsed = this.utilityService.assertUrl(sharedInbox);
-					if (this.utilityService.punyHostPSLDomain(parsed) !== expectHost) {
-						this.logger.warn(`Excluding sharedInbox for actor ${uri}: wrong host in ${sharedInbox}`);
-						x.sharedInbox = undefined;
-					}
-				} else {
-					this.logger.warn(`Excluding sharedInbox for actor ${uri}: missing ID`);
-					x.sharedInbox = undefined;
-				}
-			} else {
-				// Collapse all falsy values to undefined
-				x.sharedInbox = undefined;
-			}
-		} catch {
-			// Shared inbox is unparseable - strip out
-			x.sharedInbox = undefined;
-		}
+		this.apUtilityService.sanitizeInlineObject(x, 'sharedInbox', parsedUri, expectHost);
 
 		// Sanitize endpoints object
 		if (typeof(x.endpoints) === 'object') {
@@ -211,94 +197,47 @@ export class ApPersonService implements OnModuleInit, OnApplicationShutdown {
 
 		// Sanitize endpoints.sharedInbox
 		if (x.endpoints) {
-			try {
-				if (x.endpoints.sharedInbox) {
-					const sharedInbox = getNullableApId(x.endpoints.sharedInbox);
-					if (sharedInbox) {
-						const parsed = this.utilityService.assertUrl(sharedInbox);
-						if (this.utilityService.punyHostPSLDomain(parsed) !== expectHost) {
-							this.logger.warn(`Excluding endpoints.sharedInbox for actor ${uri}: wrong host in ${sharedInbox}`);
-							x.endpoints.sharedInbox = undefined;
-						}
-					} else {
-						this.logger.warn(`Excluding endpoints.sharedInbox for actor ${uri}: missing ID`);
-						x.endpoints.sharedInbox = undefined;
-					}
-				} else {
-					// Collapse all falsy values to undefined
-					x.endpoints.sharedInbox = undefined;
-				}
-			} catch {
-				// Shared inbox is unparseable - strip out
-				x.endpoints.sharedInbox = undefined;
+			this.apUtilityService.sanitizeInlineObject(x.endpoints, 'sharedInbox', parsedUri, expectHost, 'endpoints.');
+
+			if (!x.endpoints.sharedInbox) {
+				x.endpoints = undefined;
 			}
 		}
 
 		// Sanitize collections
-		for (const collection of ['outbox', 'followers', 'following', 'featured'] as (keyof IActor)[]) {
-			try {
-				if (x[collection]) {
-					const collectionUri = getNullableApId(x[collection]);
-					if (collectionUri) {
-						const parsed = this.utilityService.assertUrl(collectionUri);
-						if (this.utilityService.punyHostPSLDomain(parsed) !== expectHost) {
-							this.logger.warn(`Excluding ${collection} for actor ${uri}: wrong host in ${collectionUri}`);
-							x[collection] = undefined;
-						}
-					} else {
-						this.logger.warn(`Excluding ${collection} for actor ${uri}: missing ID`);
-						x[collection] = undefined;
-					}
-				} else {
-					// Collapse all falsy values to undefined
-					x[collection] = undefined;
-				}
-			} catch {
-				// Collection is unparseable - strip out
-				x[collection] = undefined;
-			}
+		for (const collection of ['outbox', 'followers', 'following', 'featured'] as const) {
+			this.apUtilityService.sanitizeInlineObject(x, collection, parsedUri, expectHost);
 		}
 
+		// Validate username
 		if (!(typeof x.preferredUsername === 'string' && x.preferredUsername.length > 0 && x.preferredUsername.length <= 128 && /^\w([\w-.]*\w)?$/.test(x.preferredUsername))) {
 			throw new UnrecoverableError(`invalid Actor ${uri}: wrong username`);
 		}
 
+		// Sanitize name
 		// These fields are only informational, and some AP software allows these
 		// fields to be very long. If they are too long, we cut them off. This way
 		// we can at least see these users and their activities.
-		if (x.name) {
-			if (!(typeof x.name === 'string' && x.name.length > 0)) {
-				throw new UnrecoverableError(`invalid Actor ${uri}: wrong name`);
-			}
-			x.name = truncate(x.name, nameLength);
-		} else if (x.name === '') {
-			// Mastodon emits empty string when the name is not set.
+		if (!x.name) {
 			x.name = undefined;
+		} else if (typeof(x.name) !== 'string') {
+			this.logger.warn(`Excluding name from object ${uri}: incorrect type ${typeof(x)}`);
+			x.name = undefined;
+		} else {
+			x.name = truncate(x.name, nameLength);
 		}
-		if (x.summary) {
-			if (!(typeof x.summary === 'string' && x.summary.length > 0)) {
-				throw new UnrecoverableError(`invalid Actor ${uri}: wrong summary`);
-			}
+
+		// Sanitize summary
+		if (!x.summary) {
+			x.summary = undefined;
+		} else if (typeof(x.summary) !== 'string') {
+			this.logger.warn(`Excluding summary from object ${uri}: incorrect type ${typeof(x)}`);
+		} else {
 			x.summary = truncate(x.summary, summaryLength);
 		}
 
-		const parsedId = this.utilityService.assertUrl(x.id);
-		const idHost = this.utilityService.punyHostPSLDomain(parsedId);
-		if (idHost !== expectHost) {
-			throw new UnrecoverableError(`invalid Actor ${uri}: wrong id ${x.id}`);
-		}
-
-		if (x.publicKey) {
-			if (typeof x.publicKey.id !== 'string') {
-				throw new UnrecoverableError(`invalid Actor ${uri}: wrong publicKey.id type`);
-			}
-
-			const parsed = this.utilityService.assertUrl(x.publicKey.id);
-			const publicKeyIdHost = this.utilityService.punyHostPSLDomain(parsed);
-			if (publicKeyIdHost !== expectHost) {
-				throw new UnrecoverableError(`invalid Actor ${uri}: wrong publicKey.id ${x.publicKey.id}`);
-			}
-		}
+		// Sanitize publicKey
+		this.apUtilityService.sanitizeInlineObject(x, 'publicKey', parsedUri, expectHost);
 
 		return x;
 	}

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

@@ -86,7 +86,7 @@ export function getOneApId(value: ApObject): string {
 /**
  * Get ActivityStreams Object id
  */
-export function getApId(value: string | IObject | [string | IObject], sourceForLogs?: string): string {
+export function getApId(value: unknown | [unknown] | unknown[], sourceForLogs?: string): string {
 	const id = getNullableApId(value);
 
 	if (id == null) {
@@ -102,7 +102,7 @@ export function getApId(value: string | IObject | [string | IObject], sourceForL
 /**
  * Get ActivityStreams Object id, or null if not present
  */
-export function getNullableApId(source: string | IObject | [string | IObject]): string | null {
+export function getNullableApId(source: unknown | [unknown] | unknown[]): string | null {
 	const value: unknown = fromTuple(source);
 
 	if (value != null) {
@@ -276,7 +276,7 @@ export interface IActor extends IObject {
 	followers?: string | ICollection | IOrderedCollection;
 	following?: string | ICollection | IOrderedCollection;
 	featured?: string | IOrderedCollection;
-	outbox: string | IOrderedCollection;
+	outbox?: string | IOrderedCollection;
 	endpoints?: {
 		sharedInbox?: string;
 	};

packages/backend/test/unit/FetchInstanceMetadataService.ts

@@ -16,6 +16,7 @@ import { HttpRequestService } from '@/core/HttpRequestService.js';
 import { LoggerService } from '@/core/LoggerService.js';
 import { UtilityService } from '@/core/UtilityService.js';
 import { IdService } from '@/core/IdService.js';
+import { EnvService } from '@/core/EnvService.js';
 import { DI } from '@/di-symbols.js';
 
 function mockRedis() {
@@ -46,6 +47,7 @@ describe('FetchInstanceMetadataService', () => {
 					LoggerService,
 					UtilityService,
 					IdService,
+					EnvService,
 				],
 			})
 			.useMocker((token) => {

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

@@ -7,6 +7,8 @@ import type { IObject } from '@/core/activitypub/type.js';
 import type { EnvService } from '@/core/EnvService.js';
 import type { MiMeta } from '@/models/Meta.js';
 import type { Config } from '@/config.js';
+import type { LoggerService } from '@/core/LoggerService.js';
+import Logger from '@/logger.js';
 import { ApUtilityService } from '@/core/activitypub/ApUtilityService.js';
 import { UtilityService } from '@/core/UtilityService.js';
 
@@ -36,7 +38,17 @@ describe(ApUtilityService, () => {
 
 		const utilityService = new UtilityService(config, meta, envService);
 
-		serviceUnderTest = new ApUtilityService(utilityService);
+		const loggerService = {
+			getLogger(domain: string) {
+				const logger = new Logger(domain);
+				Object.defineProperty(logger, 'log', {
+					value: () => {},
+				});
+				return logger;
+			},
+		} as unknown as LoggerService;
+
+		serviceUnderTest = new ApUtilityService(utilityService, loggerService);
 	});
 
 	describe('assertIdMatchesUrlAuthority', () => {
@@ -361,4 +373,102 @@ describe(ApUtilityService, () => {
 			expect(result).toBe('http://example.com/1');
 		});
 	});
+
+	describe('sanitizeInlineObject', () => {
+		it('should exclude nested arrays', () => {
+			const input = {
+				test: [[]] as unknown as string[],
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(false);
+		});
+
+		it('should exclude incorrect type', () => {
+			const input = {
+				test: 0 as unknown as string,
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(false);
+		});
+
+		it('should exclude missing ID', () => {
+			const input = {
+				test: {
+					id: undefined,
+				},
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(false);
+		});
+
+		it('should exclude wrong host', () => {
+			const input = {
+				test: 'https://wrong.com/object',
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(false);
+		});
+
+		it('should exclude invalid URLs', () => {
+			const input = {
+				test: 'https://user@example.com/object',
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(false);
+		});
+
+		it('should accept string', () => {
+			const input = {
+				test: 'https://example.com/object',
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(true);
+		});
+
+		it('should accept array of string', () => {
+			const input = {
+				test: ['https://example.com/object'],
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(true);
+		});
+
+		it('should accept object', () => {
+			const input = {
+				test: {
+					id: 'https://example.com/object',
+				},
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(true);
+		});
+
+		it('should accept array of object', () => {
+			const input = {
+				test: [{
+					id: 'https://example.com/object',
+				}],
+			};
+
+			const result = serviceUnderTest.sanitizeInlineObject(input, 'test', 'https://example.com/actor', 'example.com');
+
+			expect(result).toBe(true);
+		});
+	});
 });