From af967fe6bee8e06470f10bc1a0e274fc0db9d66c Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Tue, 8 Jul 2025 11:01:56 -0400 Subject: [PATCH] refactor actor validation to reduce code duplication --- packages/backend/src/core/UtilityService.ts | 4 +- .../src/core/activitypub/ApUtilityService.ts | 86 ++++++++++- .../activitypub/models/ApPersonService.ts | 143 +++++------------- packages/backend/src/core/activitypub/type.ts | 6 +- .../test/unit/FetchInstanceMetadataService.ts | 2 + .../unit/core/activitypub/ApUtilityService.ts | 112 +++++++++++++- 6 files changed, 243 insertions(+), 110 deletions(-) diff --git a/packages/backend/src/core/UtilityService.ts b/packages/backend/src/core/UtilityService.ts index 281edccca3..27d3cb7776 100644 --- a/packages/backend/src/core/UtilityService.ts +++ b/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'; diff --git a/packages/backend/src/core/activitypub/ApUtilityService.ts b/packages/backend/src/core/activitypub/ApUtilityService.ts index eede48f0c6..26ea0cd632 100644 --- a/packages/backend/src/core/activitypub/ApUtilityService.ts +++ b/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(obj: Partial>, key: Key, parentUri: string | URL, parentHost: string, keyPath = ''): obj is Partial> { + 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 { diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 05bd97a3e3..b3fd17e7a0 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/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; } diff --git a/packages/backend/src/core/activitypub/type.ts b/packages/backend/src/core/activitypub/type.ts index 554420d670..ebe29a4fe6 100644 --- a/packages/backend/src/core/activitypub/type.ts +++ b/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; }; diff --git a/packages/backend/test/unit/FetchInstanceMetadataService.ts b/packages/backend/test/unit/FetchInstanceMetadataService.ts index 1e3605aafc..812ee38703 100644 --- a/packages/backend/test/unit/FetchInstanceMetadataService.ts +++ b/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) => { diff --git a/packages/backend/test/unit/core/activitypub/ApUtilityService.ts b/packages/backend/test/unit/core/activitypub/ApUtilityService.ts index a49ba35112..7b564b1fdd 100644 --- a/packages/backend/test/unit/core/activitypub/ApUtilityService.ts +++ b/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); + }); + }); });