From d883934826122b64439170c2013d65606fd23184 Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 05:13:35 +0200 Subject: [PATCH 01/33] fix: primitive 2: acceptance of cross-origin alternate links --- packages/backend/src/core/activitypub/ApRequestService.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/core/activitypub/ApRequestService.ts b/packages/backend/src/core/activitypub/ApRequestService.ts index 38c78cf900..35eb1a5c53 100644 --- a/packages/backend/src/core/activitypub/ApRequestService.ts +++ b/packages/backend/src/core/activitypub/ApRequestService.ts @@ -18,6 +18,7 @@ import type Logger from '@/logger.js'; import type { IObject } from './type.js'; import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js'; import { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js'; +import { UtilityService } from "@/core/UtilityService.js"; type Request = { url: string; @@ -147,6 +148,7 @@ export class ApRequestService { private userKeypairService: UserKeypairService, private httpRequestService: HttpRequestService, private loggerService: LoggerService, + private utilityService: UtilityService, ) { // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition this.logger = this.loggerService?.getLogger('ap-request'); // なぜか TypeError: Cannot read properties of undefined (reading 'getLogger') と言われる @@ -241,7 +243,9 @@ export class ApRequestService { if (alternate) { const href = alternate.getAttribute('href'); if (href) { - return await this.signedGet(href, user, false); + if (this.utilityService.punyHost(url) === this.utilityService.punyHost(href)) { + return await this.signedGet(href, user, false); + } } } } catch (e) { From 9090b745e69c380847f97f107d39864d7ace0039 Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:04:56 +0200 Subject: [PATCH 02/33] fix: primitive 3: validation of non-final url --- packages/backend/src/core/HttpRequestService.ts | 2 +- packages/backend/src/core/activitypub/ApRequestService.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index bea5dee6ab..08e9f46b2d 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -129,7 +129,7 @@ export class HttpRequestService { const finalUrl = res.url; // redirects may have been involved const activity = await res.json() as IObject; - assertActivityMatchesUrls(activity, [url, finalUrl]); + assertActivityMatchesUrls(activity, [finalUrl]); return activity; } diff --git a/packages/backend/src/core/activitypub/ApRequestService.ts b/packages/backend/src/core/activitypub/ApRequestService.ts index 35eb1a5c53..eeff73385b 100644 --- a/packages/backend/src/core/activitypub/ApRequestService.ts +++ b/packages/backend/src/core/activitypub/ApRequestService.ts @@ -261,7 +261,7 @@ export class ApRequestService { const finalUrl = res.url; // redirects may have been involved const activity = await res.json() as IObject; - assertActivityMatchesUrls(activity, [url, finalUrl]); + assertActivityMatchesUrls(activity, [finalUrl]); return activity; } From 1e14612f0e4349b00f5016f3a9184ff3e40fc37e Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:11:35 +0200 Subject: [PATCH 03/33] fix: primitive 4: missing same-origin identifier validation of collection-wrapped activities --- packages/backend/src/core/activitypub/ApInboxService.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index d54c9544c3..5a6f6f083f 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -100,6 +100,10 @@ export class ApInboxService { const resolver = this.apResolverService.createResolver(); for (const item of toArray(isCollection(activity) ? activity.items : activity.orderedItems)) { const act = await resolver.resolve(item); + if (act.id == null || this.utilityService.extractDbHost(act.id) !== this.utilityService.extractDbHost(actor.uri)) { + this.logger.debug('skipping activity: activity id is null or mismatching'); + continue; + } try { results.push([getApId(item), await this.performOneActivity(actor, act)]); } catch (err) { From ad8e8793c7b0ecc08bb271cd83ba04f6f8be7036 Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:37:47 +0200 Subject: [PATCH 04/33] fix: primitives 5 & 8: reject activities with non-string identifiers --- packages/backend/src/queue/processors/InboxProcessorService.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/backend/src/queue/processors/InboxProcessorService.ts b/packages/backend/src/queue/processors/InboxProcessorService.ts index 11b00bb683..f453d7d1ae 100644 --- a/packages/backend/src/queue/processors/InboxProcessorService.ts +++ b/packages/backend/src/queue/processors/InboxProcessorService.ts @@ -193,6 +193,9 @@ export class InboxProcessorService implements OnApplicationShutdown { throw new Bull.UnrecoverableError(`skip: signerHost(${signerHost}) !== activity.id host(${activityIdHost}`); } } + else { + throw new Bull.UnrecoverableError('skip: activity id is not a string'); + } // Update stats this.federatedInstanceService.fetch(authUser.user.host).then(i => { From 174dfb83d09d13876c65b98c75769d01f5c0ec47 Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:28:43 +0200 Subject: [PATCH 05/33] fix: primitive 6: reject anonymous objects that were fetched by their id --- packages/backend/src/core/activitypub/ApResolverService.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/core/activitypub/ApResolverService.ts b/packages/backend/src/core/activitypub/ApResolverService.ts index 5d5c61ce2c..a2c7ed19d8 100644 --- a/packages/backend/src/core/activitypub/ApResolverService.ts +++ b/packages/backend/src/core/activitypub/ApResolverService.ts @@ -121,7 +121,11 @@ export class Resolver { // `object.id` or `object.url` matches the URL used to fetch the // object after redirects; here we double-check that no redirects // bounced between hosts - if (object.id && (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value))) { + if (object.id == null) { + throw new Error('invalid AP object: missing id'); + } + + if (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value)) { throw new Error(`invalid AP object ${value}: id ${object.id} has different host`); } From 9ab25ede28f4f04ac2ae48c947e7668a9a6012b2 Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:40:33 +0200 Subject: [PATCH 06/33] fix: primitives 9, 10 & 11: http signature validation doesn't enforce required headers or specify auth header name --- packages/backend/src/server/ActivityPubServerService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 52592c47c6..f955329fd1 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -152,7 +152,7 @@ export class ActivityPubServerService { let signature; try { - signature = httpSignature.parseRequest(request.raw, { 'headers': [] }); + signature = httpSignature.parseRequest(request.raw, { 'headers': ['(request-target)', 'host', 'date'], authorizationHeaderName: 'signature' }); } catch (e) { // not signed, or malformed signature: refuse this.authlogger.warn(`${request.id} ${request.url} not signed, or malformed signature: refuse`); @@ -229,7 +229,7 @@ export class ActivityPubServerService { let signature; try { - signature = httpSignature.parseRequest(request.raw, { 'headers': [] }); + signature = httpSignature.parseRequest(request.raw, { 'headers': ['(request-target)', 'digest', 'host', 'date'], authorizationHeaderName: 'signature' }); } catch (e) { reply.code(401); return; From 1c7e05ce9ee6a4a5669ec73c940dcf57ecdc3db5 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 19:57:29 -0500 Subject: [PATCH 07/33] fix: primitive 7 & 12: prevent poll spoofing --- .../src/core/activitypub/ApInboxService.ts | 2 +- .../activitypub/models/ApQuestionService.ts | 29 ++++++++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index 5a6f6f083f..edd1041062 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -786,7 +786,7 @@ export class ApInboxService { await this.apPersonService.updatePerson(actor.uri, resolver, object); return 'ok: Person updated'; } else if (getApType(object) === 'Question') { - await this.apQuestionService.updateQuestion(object, resolver).catch(err => console.error(err)); + await this.apQuestionService.updateQuestion(object, actor, resolver).catch(err => console.error(err)); return 'ok: Question updated'; } else if (getApType(object) === 'Note') { await this.apNoteService.updateNote(object, resolver).catch(err => console.error(err)); diff --git a/packages/backend/src/core/activitypub/models/ApQuestionService.ts b/packages/backend/src/core/activitypub/models/ApQuestionService.ts index 9246398fde..c1aea15ece 100644 --- a/packages/backend/src/core/activitypub/models/ApQuestionService.ts +++ b/packages/backend/src/core/activitypub/models/ApQuestionService.ts @@ -5,16 +5,17 @@ import { Inject, Injectable } from '@nestjs/common'; import { DI } from '@/di-symbols.js'; -import type { NotesRepository, PollsRepository } from '@/models/_.js'; +import type { UsersRepository, NotesRepository, PollsRepository } from '@/models/_.js'; import type { Config } from '@/config.js'; import type { IPoll } from '@/models/Poll.js'; +import type { MiRemoteUser } from '@/models/User.js'; import type Logger from '@/logger.js'; import { bindThis } from '@/decorators.js'; -import { isQuestion } from '../type.js'; +import { getOneApId, isQuestion } from '../type.js'; import { ApLoggerService } from '../ApLoggerService.js'; import { ApResolverService } from '../ApResolverService.js'; import type { Resolver } from '../ApResolverService.js'; -import type { IObject, IQuestion } from '../type.js'; +import type { IObject } from '../type.js'; @Injectable() export class ApQuestionService { @@ -24,6 +25,9 @@ export class ApQuestionService { @Inject(DI.config) private config: Config, + @Inject(DI.usersRepository) + private usersRepository: UsersRepository, + @Inject(DI.notesRepository) private notesRepository: NotesRepository, @@ -65,7 +69,7 @@ export class ApQuestionService { * @returns true if updated */ @bindThis - public async updateQuestion(value: string | IObject, resolver?: Resolver): Promise { + public async updateQuestion(value: string | IObject, actor?: MiRemoteUser, resolver?: Resolver): Promise { const uri = typeof value === 'string' ? value : value.id; if (uri == null) throw new Error('uri is null'); @@ -78,15 +82,26 @@ export class ApQuestionService { const poll = await this.pollsRepository.findOneBy({ noteId: note.id }); if (poll == null) throw new Error('Question is not registered'); + + const user = await this.usersRepository.findOneBy({ id: poll.userId }); + if (user == null) throw new Error('Question is not registered'); //#endregion // resolve new Question object // eslint-disable-next-line no-param-reassign if (resolver == null) resolver = this.apResolverService.createResolver(); - const question = await resolver.resolve(value) as IQuestion; + const question = await resolver.resolve(value); this.logger.debug(`fetched question: ${JSON.stringify(question, null, 2)}`); - if (question.type !== 'Question') throw new Error('object is not a Question'); + if (!isQuestion(question)) throw new Error('object is not a Question'); + + const attribution = (question.attributedTo) ? getOneApId(question.attributedTo) : user.uri; + const attributionMatchesExisting = attribution === user.uri; + const actorMatchesAttribution = (actor) ? attribution === actor.uri : true; + + if (!attributionMatchesExisting || !actorMatchesAttribution) { + throw new Error('Refusing to ingest update for poll by different user'); + } const apChoices = question.oneOf ?? question.anyOf; if (apChoices == null) throw new Error('invalid apChoices: ' + apChoices); @@ -96,7 +111,7 @@ export class ApQuestionService { for (const choice of poll.choices) { const oldCount = poll.votes[poll.choices.indexOf(choice)]; const newCount = apChoices.filter(ap => ap.name === choice).at(0)?.replies?.totalItems; - if (newCount == null) throw new Error('invalid newCount: ' + newCount); + if (newCount == null || !(Number.isInteger(newCount) && newCount >= 0)) throw new Error('invalid newCount: ' + newCount); if (oldCount <= newCount) { changed = true; From 322b3b677ffd8fe893c6a94fbaf60768add095cc Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Sat, 26 Oct 2024 19:51:11 +0200 Subject: [PATCH 08/33] fix: primitive 14: improper validation of outbox, followers, following & shared inbox collections --- .../src/core/activitypub/models/ApPersonService.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 2046dad099..97b4dd27c9 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -154,13 +154,24 @@ export class ApPersonService implements OnModuleInit { throw new Error('invalid Actor: inbox has different host'); } + const sharedInboxObject = x.sharedInbox ?? (x.endpoints ? x.endpoints.sharedInbox : undefined); + if (sharedInboxObject != null) { + const sharedInbox = getApId(sharedInboxObject); + if (!(typeof sharedInbox === "string" && sharedInbox.length > 0 && this.utilityService.punyHost(sharedInbox) === expectHost)) { + throw new Error("invalid Actor: wrong shared inbox"); + } + } + for (const collection of ['outbox', 'followers', 'following'] as (keyof IActor)[]) { - const collectionUri = (x as IActor)[collection]; + const collectionUri = getApId((x as IActor)[collection]); if (typeof collectionUri === 'string' && collectionUri.length > 0) { if (this.utilityService.punyHost(collectionUri) !== expectHost) { throw new Error(`invalid Actor: ${collection} has different host`); } } + else if (collectionUri != null) { + throw new Error(`invalid Actor: wrong ${collection}`); + } } if (!(typeof x.preferredUsername === 'string' && x.preferredUsername.length > 0 && x.preferredUsername.length <= 128 && /^\w([\w-.]*\w)?$/.test(x.preferredUsername))) { From 4c432c07cbea935e06a839d897c3728c8964200d Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 20:21:17 -0500 Subject: [PATCH 09/33] fix: code style for primitive 14 --- .../backend/src/core/activitypub/models/ApPersonService.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 97b4dd27c9..8ddd646f05 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -157,8 +157,8 @@ export class ApPersonService implements OnModuleInit { const sharedInboxObject = x.sharedInbox ?? (x.endpoints ? x.endpoints.sharedInbox : undefined); if (sharedInboxObject != null) { const sharedInbox = getApId(sharedInboxObject); - if (!(typeof sharedInbox === "string" && sharedInbox.length > 0 && this.utilityService.punyHost(sharedInbox) === expectHost)) { - throw new Error("invalid Actor: wrong shared inbox"); + if (!(typeof sharedInbox === 'string' && sharedInbox.length > 0 && this.utilityService.punyHost(sharedInbox) === expectHost)) { + throw new Error('invalid Actor: wrong shared inbox'); } } @@ -168,8 +168,7 @@ export class ApPersonService implements OnModuleInit { if (this.utilityService.punyHost(collectionUri) !== expectHost) { throw new Error(`invalid Actor: ${collection} has different host`); } - } - else if (collectionUri != null) { + } else if (collectionUri != null) { throw new Error(`invalid Actor: wrong ${collection}`); } } From ebea1a296228fb2a7694e9090e4fa8080cbaa1ec Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 05:07:58 +0200 Subject: [PATCH 10/33] fix: primitive 15: improper same-origin validation for note uri and url --- .../core/activitypub/models/ApNoteService.ts | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/core/activitypub/models/ApNoteService.ts b/packages/backend/src/core/activitypub/models/ApNoteService.ts index f404a77fbb..146ccb11a2 100644 --- a/packages/backend/src/core/activitypub/models/ApNoteService.ts +++ b/packages/backend/src/core/activitypub/models/ApNoteService.ts @@ -141,14 +141,24 @@ export class ApNoteService { this.logger.debug(`Note fetched: ${JSON.stringify(note, null, 2)}`); - if (note.id && !checkHttps(note.id)) { + if (note.id == null) { + throw new Error('Refusing to create note without id'); + } + + if (!checkHttps(note.id)) { throw new Error('unexpected schema of note.id: ' + note.id); } const url = getOneApHrefNullable(note.url); - if (url && !checkHttps(url)) { - throw new Error('unexpected schema of note url: ' + url); + if (url != null) { + if (!checkHttps(url)) { + throw new Error('unexpected schema of note url: ' + url); + } + + if (this.utilityService.punyHost(url) !== this.utilityService.punyHost(note.id)) { + throw new Error(`note url <> uri host mismatch: ${url} <> ${note.id}`); + } } this.logger.info(`Creating the Note: ${note.id}`); @@ -366,7 +376,11 @@ export class ApNoteService { this.logger.debug(`Note fetched: ${JSON.stringify(note, null, 2)}`); - if (note.id && !checkHttps(note.id)) { + if (note.id == null) { + throw new Error('Refusing to update note without id'); + } + + if (!checkHttps(note.id)) { throw new Error('unexpected schema of note.id: ' + note.id); } @@ -376,6 +390,16 @@ export class ApNoteService { throw new Error('unexpected schema of note url: ' + url); } + if (url != null) { + if (!checkHttps(url)) { + throw new Error('unexpected schema of note url: ' + url); + } + + if (this.utilityService.punyHost(url) !== this.utilityService.punyHost(note.id)) { + throw new Error(`note url <> id host mismatch: ${url} <> ${note.id}`); + } + } + this.logger.info(`Creating the Note: ${note.id}`); // 投稿者をフェッチ From b74e2e91674ee56ef0b835daa31f5a72d02ab37d Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 05:11:16 +0200 Subject: [PATCH 11/33] fix: primitive 16: improper same-origin validation for user uri and url --- .../activitypub/models/ApPersonService.ts | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 8ddd646f05..7a3bd57d43 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -337,8 +337,18 @@ export class ApPersonService implements OnModuleInit { const url = getOneApHrefNullable(person.url); - if (url && !checkHttps(url)) { - throw new Error('unexpected schema of person url: ' + url); + if (person.id == null) { + throw new Error('Refusing to create person without id'); + } + + if (url != null) { + if (!checkHttps(url)) { + throw new Error('unexpected schema of person url: ' + url); + } + + if (this.utilityService.punyHost(url) !== this.utilityService.punyHost(person.id)) { + throw new Error(`person url <> uri host mismatch: ${url} <> ${person.id}`); + } } // Create user @@ -539,8 +549,18 @@ export class ApPersonService implements OnModuleInit { const url = getOneApHrefNullable(person.url); - if (url && !checkHttps(url)) { - throw new Error('unexpected schema of person url: ' + url); + if (person.id == null) { + throw new Error('Refusing to update person without id'); + } + + if (url != null) { + if (!checkHttps(url)) { + throw new Error('unexpected schema of person url: ' + url); + } + + if (this.utilityService.punyHost(url) !== this.utilityService.punyHost(person.id)) { + throw new Error(`person url <> uri host mismatch: ${url} <> ${person.id}`); + } } const updates = { From 4d925fc08683a9415c9488b5bcc516ca8f43d4af Mon Sep 17 00:00:00 2001 From: Laura Hausmann Date: Thu, 24 Oct 2024 04:18:49 +0200 Subject: [PATCH 12/33] fix: primitive 17: note same-origin identifier validation can be bypassed by wrapping the id in an array --- packages/backend/src/core/activitypub/ApInboxService.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index edd1041062..b5a97d34c4 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -426,6 +426,9 @@ export class ApInboxService { return 'skip: host in actor.uri !== note.id'; } } + else { + return 'skip: note.id is not a string' + } } const unlock = await this.appLockService.getApLock(uri); From b9080da75dea7e9c5d38976814118a594be9e019 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 20:28:50 -0500 Subject: [PATCH 13/33] fix: code style for primitive 17 --- packages/backend/src/core/activitypub/ApInboxService.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index b5a97d34c4..42c2007799 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -425,9 +425,8 @@ export class ApInboxService { if (this.utilityService.extractDbHost(actor.uri) !== this.utilityService.extractDbHost(note.id)) { return 'skip: host in actor.uri !== note.id'; } - } - else { - return 'skip: note.id is not a string' + } else { + return 'skip: note.id is not a string'; } } From c04f34404991d69103703d781f52658fe3214d15 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 21:17:30 -0500 Subject: [PATCH 14/33] fix: primitive 13: check attribution against actor in notes --- .../src/core/activitypub/ApInboxService.ts | 4 +- .../core/activitypub/models/ApNoteService.ts | 71 ++++++++++++------- .../src/server/api/endpoints/ap/show.ts | 2 +- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index 42c2007799..0f56ad3f78 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -436,7 +436,7 @@ export class ApInboxService { const exist = await this.apNoteService.fetchNote(note); if (exist) return 'skip: note exists'; - await this.apNoteService.createNote(note, resolver, silent); + await this.apNoteService.createNote(note, actor, resolver, silent); return 'ok'; } catch (err) { if (err instanceof StatusError && !err.isRetryable) { @@ -791,7 +791,7 @@ export class ApInboxService { await this.apQuestionService.updateQuestion(object, actor, resolver).catch(err => console.error(err)); return 'ok: Question updated'; } else if (getApType(object) === 'Note') { - await this.apNoteService.updateNote(object, resolver).catch(err => console.error(err)); + await this.apNoteService.updateNote(object, actor, resolver).catch(err => console.error(err)); return 'ok: Note updated'; } else { return `skip: Unknown type: ${getApType(object)}`; diff --git a/packages/backend/src/core/activitypub/models/ApNoteService.ts b/packages/backend/src/core/activitypub/models/ApNoteService.ts index 146ccb11a2..7857bcc28c 100644 --- a/packages/backend/src/core/activitypub/models/ApNoteService.ts +++ b/packages/backend/src/core/activitypub/models/ApNoteService.ts @@ -6,7 +6,7 @@ import { forwardRef, Inject, Injectable } from '@nestjs/common'; import { In } from 'typeorm'; import { DI } from '@/di-symbols.js'; -import type { PollsRepository, EmojisRepository, NotesRepository, MiMeta } from '@/models/_.js'; +import type { UsersRepository, PollsRepository, EmojisRepository, NotesRepository, MiMeta } from '@/models/_.js'; import type { Config } from '@/config.js'; import type { MiRemoteUser } from '@/models/User.js'; import type { MiNote } from '@/models/Note.js'; @@ -49,6 +49,9 @@ export class ApNoteService { @Inject(DI.meta) private meta: MiMeta, + @Inject(DI.usersRepository) + private usersRepository: UsersRepository, + @Inject(DI.pollsRepository) private pollsRepository: PollsRepository, @@ -82,7 +85,13 @@ export class ApNoteService { } @bindThis - public validateNote(object: IObject, uri: string): Error | null { + public validateNote( + object: IObject, + uri: string, + actor?: MiRemoteUser, + user?: MiRemoteUser, + note?: MiNote, + ): Error | null { const expectHost = this.utilityService.extractDbHost(uri); const apType = getApType(object); @@ -99,10 +108,27 @@ export class ApNoteService { return new IdentifiableError('d450b8a9-48e4-4dab-ae36-f4db763fda7c', `invalid Note: attributedTo has different host. expected: ${expectHost}, actual: ${actualHost}`); } + if (actor) { + const attribution = (object.attributedTo) ? getOneApId(object.attributedTo) : actor.uri; + if (attribution !== actor.uri) { + return new IdentifiableError('d450b8a9-48e4-4dab-ae36-f4db763fda7c', `invalid Note: attribution does not match the actor that send it. attribution: ${attribution}, actor: ${actor.uri}`); + } + if (user && attribution !== user.uri) { + return new IdentifiableError('d450b8a9-48e4-4dab-ae36-f4db763fda7c', `invalid Note: updated attribution does not match original attribution. updated attribution: ${user.uri}, original attribution: ${attribution}`); + } + } + if (object.published && !this.idService.isSafeT(new Date(object.published).valueOf())) { return new IdentifiableError('d450b8a9-48e4-4dab-ae36-f4db763fda7c', 'invalid Note: published timestamp is malformed'); } + 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; } @@ -120,14 +146,14 @@ export class ApNoteService { * Noteを作成します。 */ @bindThis - public async createNote(value: string | IObject, resolver?: Resolver, silent = false): Promise { + public async createNote(value: string | IObject, actor?: MiRemoteUser, resolver?: Resolver, silent = false): Promise { // eslint-disable-next-line no-param-reassign if (resolver == null) resolver = this.apResolverService.createResolver(); const object = await resolver.resolve(value); const entryUri = getApId(value); - const err = this.validateNote(object, entryUri); + const err = this.validateNote(object, entryUri, actor); if (err) { this.logger.error(err.message, { resolver: { history: resolver.getHistory() }, @@ -171,8 +197,9 @@ export class ApNoteService { const uri = getOneApId(note.attributedTo); // ローカルで投稿者を検索し、もし凍結されていたらスキップ - const cachedActor = await this.apPersonService.fetchPerson(uri) as MiRemoteUser; - if (cachedActor && cachedActor.isSuspended) { + // eslint-disable-next-line no-param-reassign + actor ??= await this.apPersonService.fetchPerson(uri) as MiRemoteUser | undefined; + if (actor && actor.isSuspended) { throw new IdentifiableError('85ab9bd7-3a41-4530-959d-f07073900109', 'actor has been suspended'); } @@ -204,7 +231,8 @@ export class ApNoteService { } //#endregion - const actor = cachedActor ?? await this.apPersonService.resolvePerson(uri, resolver) as MiRemoteUser; + // eslint-disable-next-line no-param-reassign + actor ??= await this.apPersonService.resolvePerson(uri, resolver) as MiRemoteUser; // 解決した投稿者が凍結されていたらスキップ if (actor.isSuspended) { @@ -345,7 +373,7 @@ export class ApNoteService { * Noteを作成します。 */ @bindThis - public async updateNote(value: string | IObject, resolver?: Resolver, silent = false): Promise { + public async updateNote(value: string | IObject, actor?: MiRemoteUser, resolver?: Resolver, silent = false): Promise { const noteUri = typeof value === 'string' ? value : value.id; if (noteUri == null) throw new Error('uri is null'); @@ -356,6 +384,9 @@ export class ApNoteService { const UpdatedNote = await this.notesRepository.findOneBy({ uri: noteUri }); if (UpdatedNote == null) throw new Error('Note is not registered'); + const user = await this.usersRepository.findOneBy({ id: UpdatedNote.userId }) as MiRemoteUser | null; + if (user == null) throw new Error('Note is not registered'); + // eslint-disable-next-line no-param-reassign if (resolver == null) resolver = this.apResolverService.createResolver(); @@ -372,6 +403,10 @@ export class ApNoteService { throw err; } + // `validateNote` checks that the actor and user are one and the same + // eslint-disable-next-line no-param-reassign + actor ??= user; + const note = object as IPost; this.logger.debug(`Note fetched: ${JSON.stringify(note, null, 2)}`); @@ -402,16 +437,7 @@ export class ApNoteService { this.logger.info(`Creating the Note: ${note.id}`); - // 投稿者をフェッチ - if (note.attributedTo == null) { - throw new Error('invalid note.attributedTo: ' + note.attributedTo); - } - - const uri = getOneApId(note.attributedTo); - - // ローカルで投稿者を検索し、もし凍結されていたらスキップ - const cachedActor = await this.apPersonService.fetchPerson(uri) as MiRemoteUser; - if (cachedActor && cachedActor.isSuspended) { + if (actor.isSuspended) { throw new IdentifiableError('85ab9bd7-3a41-4530-959d-f07073900109', 'actor has been suspended'); } @@ -443,13 +469,6 @@ export class ApNoteService { } //#endregion - const actor = cachedActor ?? await this.apPersonService.resolvePerson(uri, resolver) as MiRemoteUser; - - // 投稿者が凍結されていたらスキップ - if (actor.isSuspended) { - throw new IdentifiableError('85ab9bd7-3a41-4530-959d-f07073900109', 'actor has been suspended'); - } - const noteAudience = await this.apAudienceService.parseAudience(actor, note.to, note.cc, resolver); let visibility = noteAudience.visibility; const visibleUsers = noteAudience.visibleUsers; @@ -610,7 +629,7 @@ export class ApNoteService { // ここでuriの代わりに添付されてきたNote Objectが指定されていると、サーバーフェッチを経ずにノートが生成されるが // 添付されてきたNote Objectは偽装されている可能性があるため、常にuriを指定してサーバーフェッチを行う。 const createFrom = options.sentFrom?.origin === new URL(uri).origin ? value : uri; - return await this.createNote(createFrom, options.resolver, true); + return await this.createNote(createFrom, undefined, options.resolver, true); } finally { unlock(); } diff --git a/packages/backend/src/server/api/endpoints/ap/show.ts b/packages/backend/src/server/api/endpoints/ap/show.ts index a877d1ce0d..4232bc6e39 100644 --- a/packages/backend/src/server/api/endpoints/ap/show.ts +++ b/packages/backend/src/server/api/endpoints/ap/show.ts @@ -140,7 +140,7 @@ export default class extends Endpoint { // eslint- return await this.mergePack( me, isActor(object) ? await this.apPersonService.createPerson(getApId(object)) : null, - isPost(object) ? await this.apNoteService.createNote(getApId(object), undefined, true) : null, + isPost(object) ? await this.apNoteService.createNote(getApId(object), undefined, undefined, true) : null, ); } From cbf8cc376e02e457a96d680dbbf0c110137d55f5 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 21:23:27 -0500 Subject: [PATCH 15/33] fix: primitive 18: `ap/get` bypasses access checks One might argue that we could make this one actually preform access checks against the returned activity object, but I feel like that's a lot more work than just restricting it to administrators, since, to me at least, it seems more like a debugging tool than anything else. --- packages/backend/src/server/api/endpoints/ap/get.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend/src/server/api/endpoints/ap/get.ts b/packages/backend/src/server/api/endpoints/ap/get.ts index d8c55de7ec..14286bc23e 100644 --- a/packages/backend/src/server/api/endpoints/ap/get.ts +++ b/packages/backend/src/server/api/endpoints/ap/get.ts @@ -11,6 +11,7 @@ import { ApResolverService } from '@/core/activitypub/ApResolverService.js'; export const meta = { tags: ['federation'], + requireAdmin: true, requireCredential: true, kind: 'read:federation', From 408e782507837da4c9b2164266d6f6f3e48d1642 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 21:38:17 -0500 Subject: [PATCH 16/33] fix: primitive 19 & 20: respect blocks and hide more Ideally, the user property should also be hidden (as leaving it in leaks information slightly), but given the schema of the note endpoint, I don't think that would be possible without introducing some kind of "ghost" user, who is attributed for posts by users who have you blocked. --- .../src/core/entities/NoteEntityService.ts | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/core/entities/NoteEntityService.ts b/packages/backend/src/core/entities/NoteEntityService.ts index 4dd17c5af3..2855ae78f7 100644 --- a/packages/backend/src/core/entities/NoteEntityService.ts +++ b/packages/backend/src/core/entities/NoteEntityService.ts @@ -11,7 +11,7 @@ import type { Packed } from '@/misc/json-schema.js'; import { awaitAll } from '@/misc/prelude/await-all.js'; import type { MiUser } from '@/models/User.js'; import type { MiNote } from '@/models/Note.js'; -import type { UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, MiMeta } from '@/models/_.js'; +import type { BlockingsRepository, UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, MiMeta } from '@/models/_.js'; import { bindThis } from '@/decorators.js'; import { DebounceLoader } from '@/misc/loader.js'; import { IdService } from '@/core/IdService.js'; @@ -39,6 +39,9 @@ export class NoteEntityService implements OnModuleInit { @Inject(DI.meta) private meta: MiMeta, + @Inject(DI.blockingsRepository) + private blockingsRepository: BlockingsRepository, + @Inject(DI.usersRepository) private usersRepository: UsersRepository, @@ -142,6 +145,17 @@ export class NoteEntityService implements OnModuleInit { } } + if (!hide && meId && packedNote.userId !== meId) { + const isBlocked = await this.blockingsRepository.exists({ + where: { + blockeeId: meId, + blockerId: packedNote.userId, + }, + }); + + if (isBlocked) hide = true; + } + if (hide) { packedNote.visibleUserIds = undefined; packedNote.fileIds = []; @@ -149,6 +163,12 @@ export class NoteEntityService implements OnModuleInit { packedNote.text = null; packedNote.poll = undefined; packedNote.cw = null; + packedNote.repliesCount = 0; + packedNote.reactionAcceptance = null; + packedNote.reactionAndUserPairCache = undefined; + packedNote.reactionCount = 0; + packedNote.reactionEmojis = undefined; + packedNote.reactions = undefined; packedNote.isHidden = true; } } @@ -262,7 +282,13 @@ export class NoteEntityService implements OnModuleInit { return true; } else { // フォロワーかどうか - const [following, user] = await Promise.all([ + const [blocked, following, user] = await Promise.all([ + this.blockingsRepository.exists({ + where: { + blockeeId: meId, + blockerId: note.userId, + }, + }), this.followingsRepository.count({ where: { followeeId: note.userId, @@ -273,6 +299,8 @@ export class NoteEntityService implements OnModuleInit { this.usersRepository.findOneByOrFail({ id: meId }), ]); + if (blocked) return false; + /* If we know the following, everyhting is fine. But if we do not know the following, it might be that both the @@ -284,6 +312,17 @@ export class NoteEntityService implements OnModuleInit { } } + if (meId != null) { + const isBlocked = await this.blockingsRepository.exists({ + where: { + blockeeId: meId, + blockerId: note.userId, + }, + }); + + if (isBlocked) return false; + } + return true; } From 74565f67f77085c2885ece50e6a79b50548029df Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 21:53:16 -0500 Subject: [PATCH 17/33] fix: primitives 21, 22, and 23: reuse resolver This also increases the default `recursionLimit` for `Resolver`, as it theoretically will go higher that it previously would and could possibly fail on non-malicious collection activities. --- .../src/core/activitypub/ApInboxService.ts | 85 +++++++++++-------- .../src/core/activitypub/ApResolverService.ts | 7 +- 2 files changed, 55 insertions(+), 37 deletions(-) diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index 0f56ad3f78..90444a1af3 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -93,19 +93,26 @@ export class ApInboxService { } @bindThis - public async performActivity(actor: MiRemoteUser, activity: IObject): Promise { + public async performActivity(actor: MiRemoteUser, activity: IObject, resolver?: Resolver): Promise { let result = undefined as string | void; if (isCollectionOrOrderedCollection(activity)) { const results = [] as [string, string | void][]; - const resolver = this.apResolverService.createResolver(); - for (const item of toArray(isCollection(activity) ? activity.items : activity.orderedItems)) { + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); + + const items = toArray(isCollection(activity) ? activity.items : activity.orderedItems); + if (items.length >= resolver.getRecursionLimit()) { + throw new Error(`skipping activity: collection would surpass recursion limit: ${this.utilityService.extractDbHost(actor.uri)}`); + } + + for (const item of items) { const act = await resolver.resolve(item); if (act.id == null || this.utilityService.extractDbHost(act.id) !== this.utilityService.extractDbHost(actor.uri)) { this.logger.debug('skipping activity: activity id is null or mismatching'); continue; } try { - results.push([getApId(item), await this.performOneActivity(actor, act)]); + results.push([getApId(item), await this.performOneActivity(actor, act, resolver)]); } catch (err) { if (err instanceof Error || typeof err === 'string') { this.logger.error(err); @@ -120,7 +127,7 @@ export class ApInboxService { result = results.map(([id, reason]) => `${id}: ${reason}`).join('\n'); } } else { - result = await this.performOneActivity(actor, activity); + result = await this.performOneActivity(actor, activity, resolver); } // ついでにリモートユーザーの情報が古かったら更新しておく @@ -135,37 +142,37 @@ export class ApInboxService { } @bindThis - public async performOneActivity(actor: MiRemoteUser, activity: IObject): Promise { + public async performOneActivity(actor: MiRemoteUser, activity: IObject, resolver?: Resolver): Promise { if (actor.isSuspended) return; if (isCreate(activity)) { - return await this.create(actor, activity); + return await this.create(actor, activity, resolver); } else if (isDelete(activity)) { return await this.delete(actor, activity); } else if (isUpdate(activity)) { - return await this.update(actor, activity); + return await this.update(actor, activity, resolver); } else if (isFollow(activity)) { return await this.follow(actor, activity); } else if (isAccept(activity)) { - return await this.accept(actor, activity); + return await this.accept(actor, activity, resolver); } else if (isReject(activity)) { - return await this.reject(actor, activity); + return await this.reject(actor, activity, resolver); } else if (isAdd(activity)) { - return await this.add(actor, activity); + return await this.add(actor, activity, resolver); } else if (isRemove(activity)) { - return await this.remove(actor, activity); + return await this.remove(actor, activity, resolver); } else if (isAnnounce(activity)) { - return await this.announce(actor, activity); + return await this.announce(actor, activity, resolver); } else if (isLike(activity)) { return await this.like(actor, activity); } else if (isUndo(activity)) { - return await this.undo(actor, activity); + return await this.undo(actor, activity, resolver); } else if (isBlock(activity)) { return await this.block(actor, activity); } else if (isFlag(activity)) { return await this.flag(actor, activity); } else if (isMove(activity)) { - return await this.move(actor, activity); + return await this.move(actor, activity, resolver); } else { return `unrecognized activity type: ${activity.type}`; } @@ -207,12 +214,13 @@ export class ApInboxService { } @bindThis - private async accept(actor: MiRemoteUser, activity: IAccept): Promise { + private async accept(actor: MiRemoteUser, activity: IAccept, resolver?: Resolver): Promise { const uri = activity.id ?? activity; this.logger.info(`Accept: ${uri}`); - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const object = await resolver.resolve(activity.object).catch(err => { this.logger.error(`Resolution failed: ${err}`); @@ -249,7 +257,7 @@ export class ApInboxService { } @bindThis - private async add(actor: MiRemoteUser, activity: IAdd): Promise { + private async add(actor: MiRemoteUser, activity: IAdd, resolver?: Resolver): Promise { if (actor.uri !== activity.actor) { return 'invalid actor'; } @@ -260,7 +268,7 @@ export class ApInboxService { if (activity.target === actor.featured) { const object = fromTuple(activity.object); - const note = await this.apNoteService.resolveNote(object); + const note = await this.apNoteService.resolveNote(object, { resolver }); if (note == null) return 'note not found'; await this.notePiningService.addPinned(actor, note.id); return; @@ -270,12 +278,13 @@ export class ApInboxService { } @bindThis - private async announce(actor: MiRemoteUser, activity: IAnnounce): Promise { + private async announce(actor: MiRemoteUser, activity: IAnnounce, resolver?: Resolver): Promise { const uri = getApId(activity); this.logger.info(`Announce: ${uri}`); - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const activityObject = fromTuple(activity.object); if (!activityObject) return 'skip: activity has no object property'; @@ -293,7 +302,7 @@ export class ApInboxService { } @bindThis - private async announceNote(actor: MiRemoteUser, activity: IAnnounce, target: IPost): Promise { + private async announceNote(actor: MiRemoteUser, activity: IAnnounce, target: IPost, resolver?: Resolver): Promise { const uri = getApId(activity); if (actor.isSuspended) { @@ -315,7 +324,7 @@ export class ApInboxService { // Announce対象をresolve let renote; try { - renote = await this.apNoteService.resolveNote(target); + renote = await this.apNoteService.resolveNote(target, { resolver }); if (renote == null) return 'announce target is null'; } catch (err) { // 対象が4xxならスキップ @@ -334,7 +343,7 @@ export class ApInboxService { this.logger.info(`Creating the (Re)Note: ${uri}`); - const activityAudience = await this.apAudienceService.parseAudience(actor, activity.to, activity.cc); + const activityAudience = await this.apAudienceService.parseAudience(actor, activity.to, activity.cc, resolver); const createdAt = activity.published ? new Date(activity.published) : null; if (createdAt && createdAt < this.idService.parse(renote.id).date) { @@ -372,7 +381,7 @@ export class ApInboxService { } @bindThis - private async create(actor: MiRemoteUser, activity: ICreate): Promise { + private async create(actor: MiRemoteUser, activity: ICreate, resolver?: Resolver): Promise { const uri = getApId(activity); this.logger.info(`Create: ${uri}`); @@ -398,7 +407,8 @@ export class ApInboxService { activityObject.attributedTo = activity.actor; } - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const object = await resolver.resolve(activityObject).catch(e => { this.logger.error(`Resolution failed: ${e}`); @@ -574,12 +584,13 @@ export class ApInboxService { } @bindThis - private async reject(actor: MiRemoteUser, activity: IReject): Promise { + private async reject(actor: MiRemoteUser, activity: IReject, resolver?: Resolver): Promise { const uri = activity.id ?? activity; this.logger.info(`Reject: ${uri}`); - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const object = await resolver.resolve(activity.object).catch(e => { this.logger.error(`Resolution failed: ${e}`); @@ -616,7 +627,7 @@ export class ApInboxService { } @bindThis - private async remove(actor: MiRemoteUser, activity: IRemove): Promise { + private async remove(actor: MiRemoteUser, activity: IRemove, resolver?: Resolver): Promise { if (actor.uri !== activity.actor) { return 'invalid actor'; } @@ -627,7 +638,7 @@ export class ApInboxService { if (activity.target === actor.featured) { const activityObject = fromTuple(activity.object); - const note = await this.apNoteService.resolveNote(activityObject); + const note = await this.apNoteService.resolveNote(activityObject, { resolver }); if (note == null) return 'note not found'; await this.notePiningService.removePinned(actor, note.id); return; @@ -637,7 +648,7 @@ export class ApInboxService { } @bindThis - private async undo(actor: MiRemoteUser, activity: IUndo): Promise { + private async undo(actor: MiRemoteUser, activity: IUndo, resolver?: Resolver): Promise { if (actor.uri !== activity.actor) { return 'invalid actor'; } @@ -646,7 +657,8 @@ export class ApInboxService { this.logger.info(`Undo: ${uri}`); - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const object = await resolver.resolve(activity.object).catch(e => { this.logger.error(`Resolution failed: ${e}`); @@ -770,14 +782,15 @@ export class ApInboxService { } @bindThis - private async update(actor: MiRemoteUser, activity: IUpdate): Promise { + private async update(actor: MiRemoteUser, activity: IUpdate, resolver?: Resolver): Promise { if (actor.uri !== activity.actor) { return 'skip: invalid actor'; } this.logger.debug('Update'); - const resolver = this.apResolverService.createResolver(); + // eslint-disable-next-line no-param-reassign + resolver ??= this.apResolverService.createResolver(); const object = await resolver.resolve(activity.object).catch(e => { this.logger.error(`Resolution failed: ${e}`); @@ -799,11 +812,11 @@ export class ApInboxService { } @bindThis - private async move(actor: MiRemoteUser, activity: IMove): Promise { + private async move(actor: MiRemoteUser, activity: IMove, resolver?: Resolver): Promise { // fetch the new and old accounts const targetUri = getApHrefNullable(activity.target); if (!targetUri) return 'skip: invalid activity target'; - return await this.apPersonService.updatePerson(actor.uri) ?? 'skip: nothing to do'; + return await this.apPersonService.updatePerson(actor.uri, resolver) ?? 'skip: nothing to do'; } } diff --git a/packages/backend/src/core/activitypub/ApResolverService.ts b/packages/backend/src/core/activitypub/ApResolverService.ts index a2c7ed19d8..25ccbdac60 100644 --- a/packages/backend/src/core/activitypub/ApResolverService.ts +++ b/packages/backend/src/core/activitypub/ApResolverService.ts @@ -42,7 +42,7 @@ export class Resolver { private apRendererService: ApRendererService, private apDbResolverService: ApDbResolverService, private loggerService: LoggerService, - private recursionLimit = 100, + private recursionLimit = 256, ) { this.history = new Set(); this.logger = this.loggerService.getLogger('ap-resolve'); @@ -53,6 +53,11 @@ export class Resolver { return Array.from(this.history); } + @bindThis + public getRecursionLimit(): number { + return this.recursionLimit; + } + @bindThis public async resolveCollection(value: string | IObject): Promise { const collection = typeof value === 'string' From 5764fa55cb7cb404fed3d029b658c495ec52ecaf Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 22:01:22 -0500 Subject: [PATCH 18/33] fix: primitives 25-33: proper local instance checks --- packages/backend/src/core/RemoteUserResolveService.ts | 4 ++-- packages/backend/src/core/UtilityService.ts | 5 +++++ .../backend/src/core/activitypub/ApDbResolverService.ts | 6 +++++- .../backend/src/core/activitypub/models/ApNoteService.ts | 2 +- .../src/core/activitypub/models/ApPersonService.ts | 9 ++++----- .../src/core/activitypub/models/ApQuestionService.ts | 4 +++- 6 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/backend/src/core/RemoteUserResolveService.ts b/packages/backend/src/core/RemoteUserResolveService.ts index f5a55eb8bc..678da0cfa6 100644 --- a/packages/backend/src/core/RemoteUserResolveService.ts +++ b/packages/backend/src/core/RemoteUserResolveService.ts @@ -54,9 +54,9 @@ export class RemoteUserResolveService { }) as MiLocalUser; } - host = this.utilityService.toPuny(host); + host = this.utilityService.punyHost(host); - if (this.config.host === host) { + if (host === this.utilityService.toPuny(this.config.host)) { this.logger.info(`return local user: ${usernameLower}`); return await this.usersRepository.findOneBy({ usernameLower, host: IsNull() }).then(u => { if (u == null) { diff --git a/packages/backend/src/core/UtilityService.ts b/packages/backend/src/core/UtilityService.ts index 009dd4665f..4c6d539e16 100644 --- a/packages/backend/src/core/UtilityService.ts +++ b/packages/backend/src/core/UtilityService.ts @@ -34,6 +34,11 @@ export class UtilityService { return this.toPuny(this.config.host) === this.toPuny(host); } + @bindThis + public isUriLocal(uri: string): boolean { + return this.punyHost(uri) === this.toPuny(this.config.host); + } + @bindThis public isBlockedHost(blockedHosts: string[], host: string | null): boolean { if (host == null) return false; diff --git a/packages/backend/src/core/activitypub/ApDbResolverService.ts b/packages/backend/src/core/activitypub/ApDbResolverService.ts index 8c97cc8ce8..dd89716d34 100644 --- a/packages/backend/src/core/activitypub/ApDbResolverService.ts +++ b/packages/backend/src/core/activitypub/ApDbResolverService.ts @@ -10,6 +10,7 @@ import type { Config } from '@/config.js'; import { MemoryKVCache } from '@/misc/cache.js'; import type { MiUserPublickey } from '@/models/UserPublickey.js'; import { CacheService } from '@/core/CacheService.js'; +import { UtilityService } from '@/core/UtilityService.js'; import type { MiNote } from '@/models/Note.js'; import { bindThis } from '@/decorators.js'; import type { MiLocalUser, MiRemoteUser } from '@/models/User.js'; @@ -55,6 +56,7 @@ export class ApDbResolverService implements OnApplicationShutdown { private cacheService: CacheService, private apPersonService: ApPersonService, private apLoggerService: ApLoggerService, + private utilityService: UtilityService, ) { this.publicKeyCache = new MemoryKVCache(1000 * 60 * 60 * 12); // 12h this.publicKeyByUserIdCache = new MemoryKVCache(1000 * 60 * 60 * 12); // 12h @@ -65,7 +67,9 @@ export class ApDbResolverService implements OnApplicationShutdown { const separator = '/'; const uri = new URL(getApId(value)); - if (uri.origin !== this.config.url) return { local: false, uri: uri.href }; + if (this.utilityService.toPuny(uri.host) !== this.utilityService.toPuny(this.config.host)) { + return { local: false, uri: uri.href }; + } const [, type, id, ...rest] = uri.pathname.split(separator); return { diff --git a/packages/backend/src/core/activitypub/models/ApNoteService.ts b/packages/backend/src/core/activitypub/models/ApNoteService.ts index 7857bcc28c..a0ddc2075b 100644 --- a/packages/backend/src/core/activitypub/models/ApNoteService.ts +++ b/packages/backend/src/core/activitypub/models/ApNoteService.ts @@ -621,7 +621,7 @@ export class ApNoteService { if (exist) return exist; //#endregion - if (uri.startsWith(this.config.url)) { + if (this.utilityService.isUriLocal(uri)) { throw new StatusError('cannot resolve local note', 400, 'cannot resolve local note'); } diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 7a3bd57d43..1c117795e9 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -296,7 +296,8 @@ export class ApPersonService implements OnModuleInit { public async createPerson(uri: string, resolver?: Resolver): Promise { if (typeof uri !== 'string') throw new Error('uri is not string'); - if (uri.startsWith(this.config.url)) { + const host = this.utilityService.punyHost(uri); + if (host === this.utilityService.toPuny(this.config.host)) { throw new StatusError('cannot resolve local user', 400, 'cannot resolve local user'); } @@ -310,8 +311,6 @@ export class ApPersonService implements OnModuleInit { this.logger.info(`Creating the Person: ${person.id}`); - const host = this.utilityService.punyHost(object.id); - const fields = this.analyzeAttachments(person.attachment ?? []); const tags = extractApHashtags(person.tag).map(normalizeForSearch).splice(0, 32); @@ -500,7 +499,7 @@ export class ApPersonService implements OnModuleInit { if (typeof uri !== 'string') throw new Error('uri is not string'); // URIがこのサーバーを指しているならスキップ - if (uri.startsWith(`${this.config.url}/`)) return; + if (this.utilityService.isUriLocal(uri)) return; //#region このサーバーに既に登録されているか const exist = await this.fetchPerson(uri) as MiRemoteUser | null; @@ -777,7 +776,7 @@ export class ApPersonService implements OnModuleInit { await this.updatePerson(src.movedToUri, undefined, undefined, [...movePreventUris, src.uri]); dst = await this.fetchPerson(src.movedToUri) ?? dst; } else { - if (src.movedToUri.startsWith(`${this.config.url}/`)) { + if (this.utilityService.isUriLocal(src.movedToUri)) { // ローカルユーザーっぽいのにfetchPersonで見つからないということはmovedToUriが間違っている return 'failed: movedTo is local but not found'; } diff --git a/packages/backend/src/core/activitypub/models/ApQuestionService.ts b/packages/backend/src/core/activitypub/models/ApQuestionService.ts index c1aea15ece..83a98d17f9 100644 --- a/packages/backend/src/core/activitypub/models/ApQuestionService.ts +++ b/packages/backend/src/core/activitypub/models/ApQuestionService.ts @@ -11,6 +11,7 @@ import type { IPoll } from '@/models/Poll.js'; import type { MiRemoteUser } from '@/models/User.js'; import type Logger from '@/logger.js'; import { bindThis } from '@/decorators.js'; +import { UtilityService } from '@/core/UtilityService.js'; import { getOneApId, isQuestion } from '../type.js'; import { ApLoggerService } from '../ApLoggerService.js'; import { ApResolverService } from '../ApResolverService.js'; @@ -36,6 +37,7 @@ export class ApQuestionService { private apResolverService: ApResolverService, private apLoggerService: ApLoggerService, + private utilityService: UtilityService, ) { this.logger = this.apLoggerService.logger; } @@ -74,7 +76,7 @@ export class ApQuestionService { if (uri == null) throw new Error('uri is null'); // URIがこのサーバーを指しているならスキップ - if (uri.startsWith(this.config.url + '/')) throw new Error('uri points local'); + if (this.utilityService.isUriLocal(uri)) throw new Error('uri points local'); //#region このサーバーに既に登録されているか const note = await this.notesRepository.findOneBy({ uri }); From cc4e99fdde690cea82c45f0ed9595b78810a1630 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Thu, 14 Nov 2024 23:43:19 -0500 Subject: [PATCH 19/33] fix: Try using `CacheService` to avoid excess db lookups This isn't perfect, theoretically if some massive number of users blocked the user making this request the set lookup could take a long amount of time, but eh, it works, and that scenario is highly unlikely. --- .../src/core/entities/NoteEntityService.ts | 29 +++++-------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/packages/backend/src/core/entities/NoteEntityService.ts b/packages/backend/src/core/entities/NoteEntityService.ts index 2855ae78f7..985245aeb1 100644 --- a/packages/backend/src/core/entities/NoteEntityService.ts +++ b/packages/backend/src/core/entities/NoteEntityService.ts @@ -11,12 +11,13 @@ import type { Packed } from '@/misc/json-schema.js'; import { awaitAll } from '@/misc/prelude/await-all.js'; import type { MiUser } from '@/models/User.js'; import type { MiNote } from '@/models/Note.js'; -import type { BlockingsRepository, UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, MiMeta } from '@/models/_.js'; +import type { UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, MiMeta } from '@/models/_.js'; import { bindThis } from '@/decorators.js'; import { DebounceLoader } from '@/misc/loader.js'; import { IdService } from '@/core/IdService.js'; import { ReactionsBufferingService } from '@/core/ReactionsBufferingService.js'; import type { OnModuleInit } from '@nestjs/common'; +import type { CacheService } from '../CacheService.js'; import type { CustomEmojiService } from '../CustomEmojiService.js'; import type { ReactionService } from '../ReactionService.js'; import type { UserEntityService } from './UserEntityService.js'; @@ -27,6 +28,7 @@ import type { Config } from '@/config.js'; export class NoteEntityService implements OnModuleInit { private userEntityService: UserEntityService; private driveFileEntityService: DriveFileEntityService; + private cacheService: CacheService; private customEmojiService: CustomEmojiService; private reactionService: ReactionService; private reactionsBufferingService: ReactionsBufferingService; @@ -39,9 +41,6 @@ export class NoteEntityService implements OnModuleInit { @Inject(DI.meta) private meta: MiMeta, - @Inject(DI.blockingsRepository) - private blockingsRepository: BlockingsRepository, - @Inject(DI.usersRepository) private usersRepository: UsersRepository, @@ -78,6 +77,7 @@ export class NoteEntityService implements OnModuleInit { onModuleInit() { this.userEntityService = this.moduleRef.get('UserEntityService'); this.driveFileEntityService = this.moduleRef.get('DriveFileEntityService'); + this.cacheService = this.moduleRef.get('CacheService'); this.customEmojiService = this.moduleRef.get('CustomEmojiService'); this.reactionService = this.moduleRef.get('ReactionService'); this.reactionsBufferingService = this.moduleRef.get('ReactionsBufferingService'); @@ -146,12 +146,7 @@ export class NoteEntityService implements OnModuleInit { } if (!hide && meId && packedNote.userId !== meId) { - const isBlocked = await this.blockingsRepository.exists({ - where: { - blockeeId: meId, - blockerId: packedNote.userId, - }, - }); + const isBlocked = (await this.cacheService.userBlockedCache.fetch(meId)).has(packedNote.userId); if (isBlocked) hide = true; } @@ -283,12 +278,7 @@ export class NoteEntityService implements OnModuleInit { } else { // フォロワーかどうか const [blocked, following, user] = await Promise.all([ - this.blockingsRepository.exists({ - where: { - blockeeId: meId, - blockerId: note.userId, - }, - }), + this.cacheService.userBlockingCache.fetch(meId).then((ids) => ids.has(note.userId)), this.followingsRepository.count({ where: { followeeId: note.userId, @@ -313,12 +303,7 @@ export class NoteEntityService implements OnModuleInit { } if (meId != null) { - const isBlocked = await this.blockingsRepository.exists({ - where: { - blockeeId: meId, - blockerId: note.userId, - }, - }); + const isBlocked = (await this.cacheService.userBlockedCache.fetch(meId)).has(note.userId); if (isBlocked) return false; } From f36f4b5398561dcf7365729c530f5b1868c6b994 Mon Sep 17 00:00:00 2001 From: rectcoordsystem Date: Wed, 6 Nov 2024 05:31:11 +0900 Subject: [PATCH 20/33] fix(backend): check target IP before sending HTTP request --- packages/backend/src/core/DownloadService.ts | 22 ----- .../backend/src/core/HttpRequestService.ts | 90 ++++++++++++++++++- 2 files changed, 88 insertions(+), 24 deletions(-) diff --git a/packages/backend/src/core/DownloadService.ts b/packages/backend/src/core/DownloadService.ts index 0e992f05de..05b9e64a37 100644 --- a/packages/backend/src/core/DownloadService.ts +++ b/packages/backend/src/core/DownloadService.ts @@ -6,7 +6,6 @@ import * as fs from 'node:fs'; import * as stream from 'node:stream/promises'; import { Inject, Injectable } from '@nestjs/common'; -import ipaddr from 'ipaddr.js'; import chalk from 'chalk'; import got, * as Got from 'got'; import { parse } from 'content-disposition'; @@ -70,13 +69,6 @@ export class DownloadService { }, enableUnixSockets: false, }).on('response', (res: Got.Response) => { - if ((process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') && !this.config.proxy && res.ip) { - if (this.isPrivateIp(res.ip)) { - this.logger.warn(`Blocked address: ${res.ip}`); - req.destroy(); - } - } - const contentLength = res.headers['content-length']; if (contentLength != null) { const size = Number(contentLength); @@ -139,18 +131,4 @@ export class DownloadService { cleanup(); } } - - @bindThis - private isPrivateIp(ip: string): boolean { - const parsedIp = ipaddr.parse(ip); - - for (const net of this.config.allowedPrivateNetworks ?? []) { - const cidr = ipaddr.parseCIDR(net); - if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { - return false; - } - } - - return parsedIp.range() !== 'unicast'; - } } diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 08e9f46b2d..6c013eacc0 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -6,6 +6,7 @@ import * as http from 'node:http'; import * as https from 'node:https'; import * as net from 'node:net'; +import ipaddr from 'ipaddr.js'; import CacheableLookup from 'cacheable-lookup'; import fetch from 'node-fetch'; import { HttpProxyAgent, HttpsProxyAgent } from 'hpagent'; @@ -25,6 +26,91 @@ export type HttpRequestSendOptions = { validators?: ((res: Response) => void)[]; }; +@Injectable() +class HttpRequestServiceAgent extends http.Agent { + constructor( + @Inject(DI.config) + private config: Config, + + options?: Object + ) { + super(options); + } + + @bindThis + public createConnection(options: Object, callback?: Function): net.Socket { + const socket = super.createConnection(options, callback) + .on('connect', ()=>{ + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } + } + } + }); + return socket; + }; + + @bindThis + private isPrivateIp(ip: string): boolean { + const parsedIp = ipaddr.parse(ip); + + for (const net of this.config.allowedPrivateNetworks ?? []) { + const cidr = ipaddr.parseCIDR(net); + if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { + return false; + } + } + + return parsedIp.range() !== 'unicast'; + } +} + +@Injectable() +class HttpsRequestServiceAgent extends https.Agent { + constructor( + @Inject(DI.config) + private config: Config, + + options?: Object + ) { + super(options); + } + + @bindThis + public createConnection(options: Object, callback?: Function): net.Socket { + const socket = super.createConnection(options, callback) + .on('connect', ()=>{ + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } + } + } + }); + return socket; + }; + + @bindThis + private isPrivateIp(ip: string): boolean { + const parsedIp = ipaddr.parse(ip); + + for (const net of this.config.allowedPrivateNetworks ?? []) { + const cidr = ipaddr.parseCIDR(net); + if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { + return false; + } + } + + return parsedIp.range() !== 'unicast'; + } +} + + @Injectable() export class HttpRequestService { /** @@ -57,14 +143,14 @@ export class HttpRequestService { lookup: false, // nativeのdns.lookupにfallbackしない }); - this.http = new http.Agent({ + this.http = new HttpRequestServiceAgent(config, { keepAlive: true, keepAliveMsecs: 30 * 1000, lookup: cache.lookup as unknown as net.LookupFunction, localAddress: config.outgoingAddress, }); - this.https = new https.Agent({ + this.https = new HttpsRequestServiceAgent(config, { keepAlive: true, keepAliveMsecs: 30 * 1000, lookup: cache.lookup as unknown as net.LookupFunction, From 7ccccf5545c1292c8de8b24bc426b1f9430528ef Mon Sep 17 00:00:00 2001 From: rectcoordsystem Date: Wed, 6 Nov 2024 06:33:44 +0900 Subject: [PATCH 21/33] fix(backend): allow accessing private IP when testing --- packages/backend/src/core/HttpRequestService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 6c013eacc0..8c5e3bdb91 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -42,7 +42,7 @@ class HttpRequestServiceAgent extends http.Agent { const socket = super.createConnection(options, callback) .on('connect', ()=>{ const address = socket.remoteAddress; - if (process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') { + if (process.env.NODE_ENV === 'production') { if (address && ipaddr.isValid(address)) { if (this.isPrivateIp(address)) { socket.destroy(new Error(`Blocked address: ${address}`)); @@ -84,7 +84,7 @@ class HttpsRequestServiceAgent extends https.Agent { const socket = super.createConnection(options, callback) .on('connect', ()=>{ const address = socket.remoteAddress; - if (process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') { + if (process.env.NODE_ENV === 'production') { if (address && ipaddr.isValid(address)) { if (this.isPrivateIp(address)) { socket.destroy(new Error(`Blocked address: ${address}`)); From 663c06be00d5b05d6c3e96559b170190805f38ed Mon Sep 17 00:00:00 2001 From: rectcoordsystem <37621004+rectcoordsystem@users.noreply.github.com> Date: Wed, 13 Nov 2024 03:06:22 +0900 Subject: [PATCH 22/33] Apply suggestions from code review Co-authored-by: anatawa12 --- .../backend/src/core/HttpRequestService.ts | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 8c5e3bdb91..0c8b5b4ef4 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -26,30 +26,33 @@ export type HttpRequestSendOptions = { validators?: ((res: Response) => void)[]; }; -@Injectable() +declare module 'node:http' { + interface Agent { + createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket; + } +} + class HttpRequestServiceAgent extends http.Agent { constructor( - @Inject(DI.config) private config: Config, - - options?: Object + options?: http.AgentOptions, ) { super(options); } @bindThis - public createConnection(options: Object, callback?: Function): net.Socket { + public createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket { const socket = super.createConnection(options, callback) - .on('connect', ()=>{ - const address = socket.remoteAddress; - if (process.env.NODE_ENV === 'production') { - if (address && ipaddr.isValid(address)) { - if (this.isPrivateIp(address)) { - socket.destroy(new Error(`Blocked address: ${address}`)); + .on('connect', () => { + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } } } - } - }); + }); return socket; }; @@ -68,13 +71,10 @@ class HttpRequestServiceAgent extends http.Agent { } } -@Injectable() class HttpsRequestServiceAgent extends https.Agent { constructor( - @Inject(DI.config) private config: Config, - - options?: Object + options?: https.AgentOptions, ) { super(options); } From 360d71278a78169c2b2351bbcf518c1e1654b254 Mon Sep 17 00:00:00 2001 From: rectcoordsystem Date: Wed, 13 Nov 2024 03:27:52 +0900 Subject: [PATCH 23/33] fix(backend): lint and typecheck --- .../backend/src/core/HttpRequestService.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 0c8b5b4ef4..0955d1f5bb 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -80,18 +80,18 @@ class HttpsRequestServiceAgent extends https.Agent { } @bindThis - public createConnection(options: Object, callback?: Function): net.Socket { + public createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket { const socket = super.createConnection(options, callback) - .on('connect', ()=>{ - const address = socket.remoteAddress; - if (process.env.NODE_ENV === 'production') { - if (address && ipaddr.isValid(address)) { - if (this.isPrivateIp(address)) { - socket.destroy(new Error(`Blocked address: ${address}`)); + .on('connect', () => { + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } } } - } - }); + }); return socket; }; @@ -110,7 +110,6 @@ class HttpsRequestServiceAgent extends https.Agent { } } - @Injectable() export class HttpRequestService { /** From 7b3e3f8e25a09430e250096ee01c0f913840623f Mon Sep 17 00:00:00 2001 From: rectcoordsystem Date: Wed, 13 Nov 2024 13:30:01 +0900 Subject: [PATCH 24/33] fix(backend): add isLocalAddressAllowed option to getAgentByUrl and send (HttpRequestService) --- .../backend/src/core/HttpRequestService.ts | 49 ++++++++++++++----- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 0955d1f5bb..0ad5667049 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -112,6 +112,16 @@ class HttpsRequestServiceAgent extends https.Agent { @Injectable() export class HttpRequestService { + /** + * Get http non-proxy agent (without local address filtering) + */ + private httpNative: http.Agent; + + /** + * Get https non-proxy agent (without local address filtering) + */ + private httpsNative: https.Agent; + /** * Get http non-proxy agent */ @@ -142,19 +152,20 @@ export class HttpRequestService { lookup: false, // nativeのdns.lookupにfallbackしない }); - this.http = new HttpRequestServiceAgent(config, { + const agentOption = { keepAlive: true, keepAliveMsecs: 30 * 1000, lookup: cache.lookup as unknown as net.LookupFunction, localAddress: config.outgoingAddress, - }); + }; - this.https = new HttpsRequestServiceAgent(config, { - keepAlive: true, - keepAliveMsecs: 30 * 1000, - lookup: cache.lookup as unknown as net.LookupFunction, - localAddress: config.outgoingAddress, - }); + this.httpNative = new http.Agent(agentOption); + + this.httpsNative = new https.Agent(agentOption); + + this.http = new HttpRequestServiceAgent(config, agentOption); + + this.https = new HttpsRequestServiceAgent(config, agentOption); const maxSockets = Math.max(256, config.deliverJobConcurrency ?? 128); @@ -189,16 +200,22 @@ export class HttpRequestService { * @param bypassProxy Allways bypass proxy */ @bindThis - public getAgentByUrl(url: URL, bypassProxy = false): http.Agent | https.Agent { + public getAgentByUrl(url: URL, bypassProxy = false, isLocalAddressAllowed = false): http.Agent | https.Agent { if (bypassProxy || (this.config.proxyBypassHosts ?? []).includes(url.hostname)) { + if (isLocalAddressAllowed) { + return url.protocol === 'http:' ? this.httpNative : this.httpsNative; + } return url.protocol === 'http:' ? this.http : this.https; } else { + if (isLocalAddressAllowed && (!this.config.proxy)) { + return url.protocol === 'http:' ? this.httpNative : this.httpsNative; + } return url.protocol === 'http:' ? this.httpAgent : this.httpsAgent; } } @bindThis - public async getActivityJson(url: string): Promise { + public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: { @@ -206,6 +223,7 @@ export class HttpRequestService { }, timeout: 5000, size: 1024 * 256, + isLocalAddressAllowed: isLocalAddressAllowed, }, { throwErrorWhenResponseNotOk: true, validators: [validateContentTypeSetAsActivityPub], @@ -220,7 +238,7 @@ export class HttpRequestService { } @bindThis - public async getJson(url: string, accept = 'application/json, */*', headers?: Record): Promise { + public async getJson(url: string, accept = 'application/json, */*', headers?: Record, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: Object.assign({ @@ -228,19 +246,21 @@ export class HttpRequestService { }, headers ?? {}), timeout: 5000, size: 1024 * 256, + isLocalAddressAllowed: isLocalAddressAllowed, }); return await res.json() as T; } @bindThis - public async getHtml(url: string, accept = 'text/html, */*', headers?: Record): Promise { + public async getHtml(url: string, accept = 'text/html, */*', headers?: Record, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: Object.assign({ Accept: accept, }, headers ?? {}), timeout: 5000, + isLocalAddressAllowed: isLocalAddressAllowed, }); return await res.text(); @@ -255,6 +275,7 @@ export class HttpRequestService { headers?: Record, timeout?: number, size?: number, + isLocalAddressAllowed?: boolean, } = {}, extra: HttpRequestSendOptions = { throwErrorWhenResponseNotOk: true, @@ -268,6 +289,8 @@ export class HttpRequestService { controller.abort(); }, timeout); + const isLocalAddressAllowed = args.isLocalAddressAllowed ?? false; + const res = await fetch(url, { method: args.method ?? 'GET', headers: { @@ -276,7 +299,7 @@ export class HttpRequestService { }, body: args.body, size: args.size ?? 10 * 1024 * 1024, - agent: (url) => this.getAgentByUrl(url), + agent: (url) => this.getAgentByUrl(url, false, isLocalAddressAllowed), signal: controller.signal, }); From 776f6fd1f568fc36e1b6b84d9b8e56611be58c5d Mon Sep 17 00:00:00 2001 From: rectcoordsystem Date: Wed, 13 Nov 2024 15:27:17 +0900 Subject: [PATCH 25/33] fix(backend): allow fetchSummaryFromProxy, trueMail to access local addresses --- packages/backend/src/core/EmailService.ts | 1 + packages/backend/src/server/web/UrlPreviewService.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/core/EmailService.ts b/packages/backend/src/core/EmailService.ts index a176474b95..da198d0e42 100644 --- a/packages/backend/src/core/EmailService.ts +++ b/packages/backend/src/core/EmailService.ts @@ -312,6 +312,7 @@ export class EmailService { Accept: 'application/json', Authorization: truemailAuthKey, }, + isLocalAddressAllowed: true, }); const json = (await res.json()) as { diff --git a/packages/backend/src/server/web/UrlPreviewService.ts b/packages/backend/src/server/web/UrlPreviewService.ts index 981fbb4353..47cc09b067 100644 --- a/packages/backend/src/server/web/UrlPreviewService.ts +++ b/packages/backend/src/server/web/UrlPreviewService.ts @@ -170,6 +170,6 @@ export class UrlPreviewService { contentLengthRequired: meta.urlPreviewRequireContentLength, }); - return this.httpRequestService.getJson(`${proxy}?${queryStr}`); + return this.httpRequestService.getJson(`${proxy}?${queryStr}`, 'application/json, */*', undefined, true); } } From 8e90484b3e7ac255cc141000444e2ed6d6fa54f8 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 19:21:57 -0500 Subject: [PATCH 26/33] Bump version --- package.json | 2 +- .../backend/src/server/FileServerService.ts | 89 ------------------- 2 files changed, 1 insertion(+), 90 deletions(-) diff --git a/package.json b/package.json index 2ba3881756..76cfbadb23 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sharkey", - "version": "2024.9.2", + "version": "2024.9.3", "codename": "shonk", "repository": { "type": "git", diff --git a/packages/backend/src/server/FileServerService.ts b/packages/backend/src/server/FileServerService.ts index be196373c4..1a4d0cb48f 100644 --- a/packages/backend/src/server/FileServerService.ts +++ b/packages/backend/src/server/FileServerService.ts @@ -28,11 +28,7 @@ import { bindThis } from '@/decorators.js'; import { isMimeImage } from '@/misc/is-mime-image.js'; import { correctFilename } from '@/misc/correct-filename.js'; import { handleRequestRedirectToOmitSearch } from '@/misc/fastify-hook-handlers.js'; -import { RateLimiterService } from '@/server/api/RateLimiterService.js'; -import { getIpHash } from '@/misc/get-ip-hash.js'; -import { AuthenticateService } from '@/server/api/AuthenticateService.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions } from 'fastify'; -import type Limiter from 'ratelimiter'; const _filename = fileURLToPath(import.meta.url); const _dirname = dirname(_filename); @@ -56,8 +52,6 @@ export class FileServerService { private videoProcessingService: VideoProcessingService, private internalStorageService: InternalStorageService, private loggerService: LoggerService, - private authenticateService: AuthenticateService, - private rateLimiterService: RateLimiterService, ) { this.logger = this.loggerService.getLogger('server', 'gray'); @@ -82,8 +76,6 @@ export class FileServerService { }); fastify.get<{ Params: { key: string; } }>('/files/:key', async (request, reply) => { - if (!await this.checkRateLimit(request, reply, `/files/${request.params.key}`)) return; - return await this.sendDriveFile(request, reply) .catch(err => this.errorHandler(request, reply, err)); }); @@ -97,20 +89,6 @@ export class FileServerService { Params: { url: string; }; Querystring: { url?: string; }; }>('/proxy/:url*', async (request, reply) => { - const url = 'url' in request.query ? request.query.url : 'https://' + request.params.url; - if (!url || !URL.canParse(url)) { - reply.code(400); - return; - } - - const keyUrl = new URL(url); - keyUrl.searchParams.forEach(k => keyUrl.searchParams.delete(k)); - keyUrl.hash = ''; - keyUrl.username = ''; - keyUrl.password = ''; - - if (!await this.checkRateLimit(request, reply, `/proxy/${keyUrl}`)) return; - return await this.proxyHandler(request, reply) .catch(err => this.errorHandler(request, reply, err)); }); @@ -594,71 +572,4 @@ export class FileServerService { path, }; } - - // Based on ApiCallService - private async checkRateLimit( - request: FastifyRequest<{ - Body?: Record | undefined, - Querystring?: Record | undefined, - Params?: Record | unknown, - }>, - reply: FastifyReply, - rateLimitKey: string, - ): Promise { - const body = request.method === 'GET' - ? request.query - : request.body; - - // https://datatracker.ietf.org/doc/html/rfc6750.html#section-2.1 (case sensitive) - const token = request.headers.authorization?.startsWith('Bearer ') - ? request.headers.authorization.slice(7) - : body?.['i']; - if (token != null && typeof token !== 'string') { - reply.code(400); - return false; - } - - // koa will automatically load the `X-Forwarded-For` header if `proxy: true` is configured in the app. - const [user] = await this.authenticateService.authenticate(token); - const actor = user?.id ?? getIpHash(request.ip); - - const limit = { - // Group by resource - key: rateLimitKey, - - // Maximum of 10 requests / 10 minutes - max: 10, - duration: 1000 * 60 * 10, - - // Minimum of 250 ms between each request - minInterval: 250, - }; - - // Rate limit proxy requests - try { - await this.rateLimiterService.limit(limit, actor); - return true; - } catch (err) { - // errはLimiter.LimiterInfoであることが期待される - reply.code(429); - - if (hasRateLimitInfo(err)) { - const cooldownInSeconds = Math.ceil((err.info.resetMs - Date.now()) / 1000); - // もしかするとマイナスになる可能性がなくはないのでマイナスだったら0にしておく - reply.header('Retry-After', Math.max(cooldownInSeconds, 0).toString(10)); - } - - reply.send({ - message: 'Rate limit exceeded. Please try again later.', - code: 'RATE_LIMIT_EXCEEDED', - id: 'd5826d14-3982-4d2e-8011-b9e9f02499ef', - }); - - return false; - } - } -} - -function hasRateLimitInfo(err: unknown): err is { info: Limiter.LimiterInfo } { - return err != null && typeof(err) === 'object' && 'info' in err; } From b0834ebf55d76979313378282d1dfc535b030e5d Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Tue, 19 Nov 2024 22:59:07 -0500 Subject: [PATCH 27/33] prevent DoS from spammed media proxy requests --- .../backend/src/server/FileServerService.ts | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/packages/backend/src/server/FileServerService.ts b/packages/backend/src/server/FileServerService.ts index 1a4d0cb48f..be196373c4 100644 --- a/packages/backend/src/server/FileServerService.ts +++ b/packages/backend/src/server/FileServerService.ts @@ -28,7 +28,11 @@ import { bindThis } from '@/decorators.js'; import { isMimeImage } from '@/misc/is-mime-image.js'; import { correctFilename } from '@/misc/correct-filename.js'; import { handleRequestRedirectToOmitSearch } from '@/misc/fastify-hook-handlers.js'; +import { RateLimiterService } from '@/server/api/RateLimiterService.js'; +import { getIpHash } from '@/misc/get-ip-hash.js'; +import { AuthenticateService } from '@/server/api/AuthenticateService.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions } from 'fastify'; +import type Limiter from 'ratelimiter'; const _filename = fileURLToPath(import.meta.url); const _dirname = dirname(_filename); @@ -52,6 +56,8 @@ export class FileServerService { private videoProcessingService: VideoProcessingService, private internalStorageService: InternalStorageService, private loggerService: LoggerService, + private authenticateService: AuthenticateService, + private rateLimiterService: RateLimiterService, ) { this.logger = this.loggerService.getLogger('server', 'gray'); @@ -76,6 +82,8 @@ export class FileServerService { }); fastify.get<{ Params: { key: string; } }>('/files/:key', async (request, reply) => { + if (!await this.checkRateLimit(request, reply, `/files/${request.params.key}`)) return; + return await this.sendDriveFile(request, reply) .catch(err => this.errorHandler(request, reply, err)); }); @@ -89,6 +97,20 @@ export class FileServerService { Params: { url: string; }; Querystring: { url?: string; }; }>('/proxy/:url*', async (request, reply) => { + const url = 'url' in request.query ? request.query.url : 'https://' + request.params.url; + if (!url || !URL.canParse(url)) { + reply.code(400); + return; + } + + const keyUrl = new URL(url); + keyUrl.searchParams.forEach(k => keyUrl.searchParams.delete(k)); + keyUrl.hash = ''; + keyUrl.username = ''; + keyUrl.password = ''; + + if (!await this.checkRateLimit(request, reply, `/proxy/${keyUrl}`)) return; + return await this.proxyHandler(request, reply) .catch(err => this.errorHandler(request, reply, err)); }); @@ -572,4 +594,71 @@ export class FileServerService { path, }; } + + // Based on ApiCallService + private async checkRateLimit( + request: FastifyRequest<{ + Body?: Record | undefined, + Querystring?: Record | undefined, + Params?: Record | unknown, + }>, + reply: FastifyReply, + rateLimitKey: string, + ): Promise { + const body = request.method === 'GET' + ? request.query + : request.body; + + // https://datatracker.ietf.org/doc/html/rfc6750.html#section-2.1 (case sensitive) + const token = request.headers.authorization?.startsWith('Bearer ') + ? request.headers.authorization.slice(7) + : body?.['i']; + if (token != null && typeof token !== 'string') { + reply.code(400); + return false; + } + + // koa will automatically load the `X-Forwarded-For` header if `proxy: true` is configured in the app. + const [user] = await this.authenticateService.authenticate(token); + const actor = user?.id ?? getIpHash(request.ip); + + const limit = { + // Group by resource + key: rateLimitKey, + + // Maximum of 10 requests / 10 minutes + max: 10, + duration: 1000 * 60 * 10, + + // Minimum of 250 ms between each request + minInterval: 250, + }; + + // Rate limit proxy requests + try { + await this.rateLimiterService.limit(limit, actor); + return true; + } catch (err) { + // errはLimiter.LimiterInfoであることが期待される + reply.code(429); + + if (hasRateLimitInfo(err)) { + const cooldownInSeconds = Math.ceil((err.info.resetMs - Date.now()) / 1000); + // もしかするとマイナスになる可能性がなくはないのでマイナスだったら0にしておく + reply.header('Retry-After', Math.max(cooldownInSeconds, 0).toString(10)); + } + + reply.send({ + message: 'Rate limit exceeded. Please try again later.', + code: 'RATE_LIMIT_EXCEEDED', + id: 'd5826d14-3982-4d2e-8011-b9e9f02499ef', + }); + + return false; + } + } +} + +function hasRateLimitInfo(err: unknown): err is { info: Limiter.LimiterInfo } { + return err != null && typeof(err) === 'object' && 'info' in err; } From fa3cf6c2996741e642955c5e2fca8ad785e83205 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 20:06:46 -0500 Subject: [PATCH 28/33] Fix type error in security fixes --- .../core/activitypub/models/ApPersonService.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 1c117795e9..2119c41569 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -163,13 +163,16 @@ export class ApPersonService implements OnModuleInit { } for (const collection of ['outbox', 'followers', 'following'] as (keyof IActor)[]) { - const collectionUri = getApId((x as IActor)[collection]); - if (typeof collectionUri === 'string' && collectionUri.length > 0) { - if (this.utilityService.punyHost(collectionUri) !== expectHost) { - throw new Error(`invalid Actor: ${collection} has different host`); + const xCollection = (x as IActor)[collection]; + if (xCollection != null) { + const collectionUri = getApId(xCollection); + if (typeof collectionUri === 'string' && collectionUri.length > 0) { + if (this.utilityService.punyHost(collectionUri) !== expectHost) { + throw new Error(`invalid Actor: ${collection} has different host`); + } + } else if (collectionUri != null) { + throw new Error(`invalid Actor: wrong ${collection}`); } - } else if (collectionUri != null) { - throw new Error(`invalid Actor: wrong ${collection}`); } } From 1758f29364eca3cbd13dbb5c84909c93712b3b3b Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 20:16:43 -0500 Subject: [PATCH 29/33] Fix error in test function calls --- packages/backend/test/unit/activitypub.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/test/unit/activitypub.ts b/packages/backend/test/unit/activitypub.ts index 53ced3dab3..73d6186edf 100644 --- a/packages/backend/test/unit/activitypub.ts +++ b/packages/backend/test/unit/activitypub.ts @@ -176,7 +176,7 @@ describe('ActivityPub', () => { resolver.register(actor.id, actor); resolver.register(post.id, post); - const note = await noteService.createNote(post.id, resolver, true); + const note = await noteService.createNote(post.id, undefined, resolver, true); assert.deepStrictEqual(note?.uri, post.id); assert.deepStrictEqual(note.visibility, 'public'); @@ -336,7 +336,7 @@ describe('ActivityPub', () => { resolver.register(actor.featured, featured); resolver.register(firstNote.id, firstNote); - const note = await noteService.createNote(firstNote.id as string, resolver); + const note = await noteService.createNote(firstNote.id as string, undefined, resolver); assert.strictEqual(note?.uri, firstNote.id); }); }); From 23c4aa25714af145098baa7edd74c1d217e51c1a Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 20:24:59 -0500 Subject: [PATCH 30/33] Fix style error --- packages/backend/src/core/HttpRequestService.ts | 10 +++++----- .../src/queue/processors/InboxProcessorService.ts | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 0ad5667049..6dcd0cdff3 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -54,19 +54,19 @@ class HttpRequestServiceAgent extends http.Agent { } }); return socket; - }; + } @bindThis private isPrivateIp(ip: string): boolean { const parsedIp = ipaddr.parse(ip); - + for (const net of this.config.allowedPrivateNetworks ?? []) { const cidr = ipaddr.parseCIDR(net); if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { return false; } } - + return parsedIp.range() !== 'unicast'; } } @@ -98,14 +98,14 @@ class HttpsRequestServiceAgent extends https.Agent { @bindThis private isPrivateIp(ip: string): boolean { const parsedIp = ipaddr.parse(ip); - + for (const net of this.config.allowedPrivateNetworks ?? []) { const cidr = ipaddr.parseCIDR(net); if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { return false; } } - + return parsedIp.range() !== 'unicast'; } } diff --git a/packages/backend/src/queue/processors/InboxProcessorService.ts b/packages/backend/src/queue/processors/InboxProcessorService.ts index f453d7d1ae..102e835e24 100644 --- a/packages/backend/src/queue/processors/InboxProcessorService.ts +++ b/packages/backend/src/queue/processors/InboxProcessorService.ts @@ -192,8 +192,7 @@ export class InboxProcessorService implements OnApplicationShutdown { if (signerHost !== activityIdHost) { throw new Bull.UnrecoverableError(`skip: signerHost(${signerHost}) !== activity.id host(${activityIdHost}`); } - } - else { + } else { throw new Bull.UnrecoverableError('skip: activity id is not a string'); } From 36af07abe28bec670aaebf9f5af5694bb582c29a Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 20:31:22 -0500 Subject: [PATCH 31/33] Fix another style error --- packages/backend/src/core/HttpRequestService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 6dcd0cdff3..083153940a 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -93,7 +93,7 @@ class HttpsRequestServiceAgent extends https.Agent { } }); return socket; - }; + } @bindThis private isPrivateIp(ip: string): boolean { From 6027b516e1c82324d55d6e54d0e17cbd816feb42 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 21:24:35 -0500 Subject: [PATCH 32/33] Fix `.punyHost` misuse --- packages/backend/src/core/RemoteUserResolveService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/core/RemoteUserResolveService.ts b/packages/backend/src/core/RemoteUserResolveService.ts index 678da0cfa6..098b5e1706 100644 --- a/packages/backend/src/core/RemoteUserResolveService.ts +++ b/packages/backend/src/core/RemoteUserResolveService.ts @@ -54,7 +54,7 @@ export class RemoteUserResolveService { }) as MiLocalUser; } - host = this.utilityService.punyHost(host); + host = this.utilityService.toPuny(host); if (host === this.utilityService.toPuny(this.config.host)) { this.logger.info(`return local user: ${usernameLower}`); From 59e160147fd38d49ebad27e0d705e3f2bf2613b4 Mon Sep 17 00:00:00 2001 From: Julia Johannesen Date: Wed, 20 Nov 2024 21:32:12 -0500 Subject: [PATCH 33/33] Bump develop version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 76cfbadb23..6e84882440 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sharkey", - "version": "2024.9.3", + "version": "2024.10.0-dev", "codename": "shonk", "repository": { "type": "git",