diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index e8b0326e66..12047346fb 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/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 { + public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise { 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; } diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index ac4a408fa6..37b01a6a40 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/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ならスキップ diff --git a/packages/backend/src/core/activitypub/ApRequestService.ts b/packages/backend/src/core/activitypub/ApRequestService.ts index 5caee21610..7118ce1e02 100644 --- a/packages/backend/src/core/activitypub/ApRequestService.ts +++ b/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 { + public async signedGet(url: string, user: { id: MiUser['id'] }, followAlternate?: boolean): Promise { 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; } diff --git a/packages/backend/src/core/activitypub/ApResolverService.ts b/packages/backend/src/core/activitypub/ApResolverService.ts index 27dcf2372b..5e58f848c0 100644 --- a/packages/backend/src/core/activitypub/ApResolverService.ts +++ b/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; - public async resolve(value: string | IObject | [string | IObject], allowSoftfail?: FetchAllowSoftFailMask): Promise; + public async resolve(value: string | [string]): Promise; + public async resolve(value: string | IObject | [string | IObject]): Promise; @bindThis - public async resolve(value: string | IObject | [string | IObject], allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise { + public async resolve(value: string | IObject | [string | IObject]): Promise { 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 { + private async _resolveLogged(requestUri: string, host: string): Promise { 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 { + private async _resolve(value: string, host: string, log?: SkApFetchLog): Promise { 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.httpRequestService.getActivityJson(value, allowSoftfail)) as IObject; + ? await this.apRequestService.signedGet(value, this.user) + : await this.httpRequestService.getActivityJson(value)); if (log) { const { object: objectOnly, context, contextHash } = extractObjectContext(object); diff --git a/packages/backend/src/core/activitypub/misc/check-against-url.ts b/packages/backend/src/core/activitypub/misc/check-against-url.ts deleted file mode 100644 index 282859907d..0000000000 --- a/packages/backend/src/core/activitypub/misc/check-against-url.ts +++ /dev/null @@ -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, -} diff --git a/packages/backend/src/core/activitypub/models/ApNoteService.ts b/packages/backend/src/core/activitypub/models/ApNoteService.ts index 63f9887a8d..5d168e623b 100644 --- a/packages/backend/src/core/activitypub/models/ApNoteService.ts +++ b/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 { + public async resolveNote(value: string | IObject, options: { sentFrom?: string, resolver?: Resolver } = {}): Promise { 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 このサーバーに既に登録されていたらそれを返す - const exist = await this.fetchNote(uri); - if (exist) return exist; - //#endregion - - 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; + // 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 = options.sentFrom && this.apUtilityService.haveSameAuthority(options.sentFrom, uri); + const createFrom = haveSameAuthority ? value : uri; return await this.createNote(createFrom, undefined, options.resolver, true); } finally { unlock(); diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 59f1d73fb0..1c394a9276 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/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 { + 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); - if (object.id == null) throw new UnrecoverableError(`null object.id in ${uri}`); - + const object = await resolver.resolve(value); 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 { + public async resolvePerson(value: string | IObject, resolver?: Resolver, sentFrom?: string): Promise { + 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 - // リモートサーバーからフェッチしてきて登録 - // eslint-disable-next-line no-param-reassign - if (resolver == null) resolver = this.apResolverService.createResolver(); - return await this.createPerson(uri, resolver); + // Bail if local URI doesn't exist + if (this.utilityService.isUriLocal(uri)) { + // TODO convert to identifiable error + 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 => { diff --git a/packages/backend/src/server/api/endpoints/ap/show.ts b/packages/backend/src/server/api/endpoints/ap/show.ts index 8cf7747d5f..8ba18a3b8d 100644 --- a/packages/backend/src/server/api/endpoints/ap/show.ts +++ b/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 { // 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 { // 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 { // 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 { // 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, FetchAllowSoftFailMask.CrossOrigin | FetchAllowSoftFailMask.NonCanonicalId).catch((err) => { + const object = await resolver.resolve(uri).catch((err) => { if (err instanceof IdentifiableError) { switch (err.id) { // resolve @@ -190,25 +188,13 @@ export default class extends Endpoint { // eslint- throw new ApiError(meta.errors.requestFailed); }); - if (object.id == null) { - throw new ApiError(meta.errors.responseInvalid); - } - - // /@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を再利用してはいけない + // Object is already validated to have a valid id (URI). + // 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. return await this.mergePack( me, - isActor(object) ? await this.apPersonService.createPerson(getApId(object)) : null, - isPost(object) ? await this.apNoteService.createNote(getApId(object), undefined, undefined, true) : null, + isActor(object) ? await this.apPersonService.resolvePerson(object, resolver, uri) : null, + isPost(object) ? await this.apNoteService.resolveNote(object, { resolver, sentFrom: uri }) : null, ); } @@ -239,8 +225,8 @@ export default class extends Endpoint { // eslint- * Resolves an arbitrary URI to its canonical, post-redirect form. */ private async resolveCanonicalUri(uri: string): Promise { - 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); } } diff --git a/packages/backend/test/unit/activitypub.ts b/packages/backend/test/unit/activitypub.ts index 8f1e792829..6f6d4c4121 100644 --- a/packages/backend/test/unit/activitypub.ts +++ b/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'; diff --git a/packages/backend/test/unit/ap-request.ts b/packages/backend/test/unit/ap-request.ts index f8b2a697f2..11364e1735 100644 --- a/packages/backend/test/unit/ap-request.ts +++ b/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', @@ -78,7 +78,7 @@ describe('ap-request', () => { 'https://alice.example.com/abc', FetchAllowSoftFailMask.Any, ), 'validation should fail no matter what if the response URL is inconsistent with the object ID'); - + assert.doesNotThrow(() => assertActivityMatchesUrl( 'https://alice.example.com/abc#test', { id: 'https://alice.example.com/abc' } as IObject, @@ -168,4 +168,5 @@ describe('ap-request', () => { FetchAllowSoftFailMask.Strict, ), 'throws if HTTP downgrade is detected'); }); + */ });