From 55713fcd657983add090a7788c6ffd984cbbd15f Mon Sep 17 00:00:00 2001 From: かっこかり <67428053+kakkokari-gtyih@users.noreply.github.com> Date: Wed, 8 Jan 2025 19:35:09 +0900 Subject: fix(backend): apOrHtml Constraintが正しく評価されない問題を修正 (#15213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(backend/ActivityPubServerService): apOrHtml Constraintが正しく評価されない問題を修正 (MisskeyIO#869) * Update Changelog * indent --------- Co-authored-by: あわわわとーにゅ <17376330+u1-liquid@users.noreply.github.com> --- packages/backend/src/server/ActivityPubServerService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'packages/backend/src/server/ActivityPubServerService.ts') diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index f34f6583d3..8c4b13a40a 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -519,8 +519,8 @@ export class ActivityPubServerService { }, deriveConstraint(request: IncomingMessage) { const accepted = accepts(request).type(['html', ACTIVITY_JSON, LD_JSON]); - const isAp = typeof accepted === 'string' && !accepted.match(/html/); - return isAp ? 'ap' : 'html'; + if (accepted === false) return null; + return accepted !== 'html' ? 'ap' : 'html'; }, }); -- cgit v1.2.3-freya From 980ea858f20bba92b932128c40f3336d727ac1cd Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Tue, 4 Feb 2025 10:38:50 -0500 Subject: fix import order in ActivityPubServerService.ts --- packages/backend/src/server/ActivityPubServerService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'packages/backend/src/server/ActivityPubServerService.ts') diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index a4dddbd6c3..1f838ffdbe 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -34,10 +34,10 @@ import { bindThis } from '@/decorators.js'; import { IActivity } from '@/core/activitypub/type.js'; import { isQuote, isRenote } from '@/misc/is-renote.js'; import * as Acct from '@/misc/acct.js'; -import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; -import type { FindOptionsWhere } from 'typeorm'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; +import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; +import type { FindOptionsWhere } from 'typeorm'; const ACTIVITY_JSON = 'application/activity+json; charset=utf-8'; const LD_JSON = 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"; charset=utf-8'; -- cgit v1.2.3-freya From 09669d72e7e2474141a2712a12c6dafe290ccf88 Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sun, 2 Feb 2025 22:02:08 -0500 Subject: lookup and cache rate limit factors directly within SkRateLimiterService --- packages/backend/src/core/CoreModule.ts | 6 ++ .../backend/src/server/ActivityPubServerService.ts | 4 +- packages/backend/src/server/FileServerService.ts | 21 +++---- packages/backend/src/server/api/ApiCallService.ts | 37 ++++++------ .../backend/src/server/api/SkRateLimiterService.ts | 45 ++++++++++----- .../src/server/api/StreamingApiServerService.ts | 16 ++---- .../unit/server/api/SkRateLimiterServiceTests.ts | 66 ++++++++++++++++++---- 7 files changed, 124 insertions(+), 71 deletions(-) (limited to 'packages/backend/src/server/ActivityPubServerService.ts') diff --git a/packages/backend/src/core/CoreModule.ts b/packages/backend/src/core/CoreModule.ts index 141f905d7f..8c9f419c44 100644 --- a/packages/backend/src/core/CoreModule.ts +++ b/packages/backend/src/core/CoreModule.ts @@ -232,6 +232,8 @@ const $FanoutTimelineEndpointService: Provider = { provide: 'FanoutTimelineEndpo const $ChannelFollowingService: Provider = { provide: 'ChannelFollowingService', useExisting: ChannelFollowingService }; const $RegistryApiService: Provider = { provide: 'RegistryApiService', useExisting: RegistryApiService }; const $ReversiService: Provider = { provide: 'ReversiService', useExisting: ReversiService }; +const $TimeService: Provider = { provide: 'TimeService', useExisting: TimeService }; +const $EnvService: Provider = { provide: 'EnvService', useExisting: EnvService }; const $ChartLoggerService: Provider = { provide: 'ChartLoggerService', useExisting: ChartLoggerService }; const $FederationChart: Provider = { provide: 'FederationChart', useExisting: FederationChart }; @@ -538,6 +540,8 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp $ChannelFollowingService, $RegistryApiService, $ReversiService, + $TimeService, + $EnvService, $ChartLoggerService, $FederationChart, @@ -839,6 +843,8 @@ const $SponsorsService: Provider = { provide: 'SponsorsService', useExisting: Sp $ChannelFollowingService, $RegistryApiService, $ReversiService, + $TimeService, + $EnvService, $FederationChart, $NotesChart, diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 815bf278c7..19049e528c 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -34,10 +34,10 @@ import { bindThis } from '@/decorators.js'; import { IActivity } from '@/core/activitypub/type.js'; import { isQuote, isRenote } from '@/misc/is-renote.js'; import * as Acct from '@/misc/acct.js'; -import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; -import type { FindOptionsWhere } from 'typeorm'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; +import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; +import type { FindOptionsWhere } from 'typeorm'; const ACTIVITY_JSON = 'application/activity+json; charset=utf-8'; const LD_JSON = 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"; charset=utf-8'; diff --git a/packages/backend/src/server/FileServerService.ts b/packages/backend/src/server/FileServerService.ts index 5293d529ad..18cdda5430 100644 --- a/packages/backend/src/server/FileServerService.ts +++ b/packages/backend/src/server/FileServerService.ts @@ -11,7 +11,7 @@ import rename from 'rename'; import sharp from 'sharp'; import { sharpBmp } from '@misskey-dev/sharp-read-bmp'; import type { Config } from '@/config.js'; -import type { MiDriveFile, DriveFilesRepository } from '@/models/_.js'; +import type { MiDriveFile, DriveFilesRepository, MiUser } from '@/models/_.js'; import { DI } from '@/di-symbols.js'; import { createTemp } from '@/misc/create-temp.js'; import { FILE_TYPE_BROWSERSAFE } from '@/const.js'; @@ -30,7 +30,6 @@ import { correctFilename } from '@/misc/correct-filename.js'; import { handleRequestRedirectToOmitSearch } from '@/misc/fastify-hook-handlers.js'; import { getIpHash } from '@/misc/get-ip-hash.js'; import { AuthenticateService } from '@/server/api/AuthenticateService.js'; -import { RoleService } from '@/core/RoleService.js'; import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { Keyed, RateLimit, sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions } from 'fastify'; @@ -59,7 +58,6 @@ export class FileServerService { private loggerService: LoggerService, private authenticateService: AuthenticateService, private rateLimiterService: SkRateLimiterService, - private roleService: RoleService, ) { this.logger = this.loggerService.getLogger('server', 'gray'); @@ -625,14 +623,13 @@ export class FileServerService { // 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 factor = user ? (await this.roleService.getUserPolicies(user.id)).rateLimitFactor : 1; + const actor = user ?? getIpHash(request.ip); // Call both limits: the per-resource limit and the shared cross-resource limit - return await this.checkResourceLimit(reply, actor, group, resource, factor) && await this.checkSharedLimit(reply, actor, group, factor); + return await this.checkResourceLimit(reply, actor, group, resource) && await this.checkSharedLimit(reply, actor, group); } - private async checkResourceLimit(reply: FastifyReply, actor: string, group: string, resource: string, factor = 1): Promise { + private async checkResourceLimit(reply: FastifyReply, actor: string | MiUser, group: string, resource: string): Promise { const limit: Keyed = { // Group by resource key: `${group}${resource}`, @@ -643,10 +640,10 @@ export class FileServerService { dripRate: 1000 * 60, }; - return await this.checkLimit(reply, actor, limit, factor); + return await this.checkLimit(reply, actor, limit); } - private async checkSharedLimit(reply: FastifyReply, actor: string, group: string, factor = 1): Promise { + private async checkSharedLimit(reply: FastifyReply, actor: string | MiUser, group: string): Promise { const limit: Keyed = { key: group, type: 'bucket', @@ -655,11 +652,11 @@ export class FileServerService { size: 3600, }; - return await this.checkLimit(reply, actor, limit, factor); + return await this.checkLimit(reply, actor, limit); } - private async checkLimit(reply: FastifyReply, actor: string, limit: Keyed, factor = 1): Promise { - const info = await this.rateLimiterService.limit(limit, actor, factor); + private async checkLimit(reply: FastifyReply, actor: string | MiUser, limit: Keyed): Promise { + const info = await this.rateLimiterService.limit(limit, actor); sendRateLimitHeaders(reply, info); diff --git a/packages/backend/src/server/api/ApiCallService.ts b/packages/backend/src/server/api/ApiCallService.ts index 03f25a51fe..9c3952d541 100644 --- a/packages/backend/src/server/api/ApiCallService.ts +++ b/packages/backend/src/server/api/ApiCallService.ts @@ -313,35 +313,30 @@ export class ApiCallService implements OnApplicationShutdown { // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (endpointLimit) { // koa will automatically load the `X-Forwarded-For` header if `proxy: true` is configured in the app. - let limitActor: string; + let limitActor: string | MiLocalUser; if (user) { - limitActor = user.id; + limitActor = user; } else { limitActor = getIpHash(request.ip); } - // TODO: 毎リクエスト計算するのもあれだしキャッシュしたい - const factor = user ? (await this.roleService.getUserPolicies(user.id)).rateLimitFactor : 1; + const limit = { + key: ep.name, + ...endpointLimit, + }; - if (factor > 0) { - const limit = { - key: ep.name, - ...endpointLimit, - }; + // Rate limit + const info = await this.rateLimiterService.limit(limit, limitActor); - // Rate limit - const info = await this.rateLimiterService.limit(limit, limitActor, factor); + sendRateLimitHeaders(reply, info); - sendRateLimitHeaders(reply, info); - - if (info.blocked) { - throw new ApiError({ - message: 'Rate limit exceeded. Please try again later.', - code: 'RATE_LIMIT_EXCEEDED', - id: 'd5826d14-3982-4d2e-8011-b9e9f02499ef', - httpStatusCode: 429, - }, info); - } + if (info.blocked) { + throw new ApiError({ + message: 'Rate limit exceeded. Please try again later.', + code: 'RATE_LIMIT_EXCEEDED', + id: 'd5826d14-3982-4d2e-8011-b9e9f02499ef', + httpStatusCode: 429, + }, info); } } diff --git a/packages/backend/src/server/api/SkRateLimiterService.ts b/packages/backend/src/server/api/SkRateLimiterService.ts index 38c97b63df..70103222f3 100644 --- a/packages/backend/src/server/api/SkRateLimiterService.ts +++ b/packages/backend/src/server/api/SkRateLimiterService.ts @@ -5,36 +5,59 @@ import { Inject, Injectable } from '@nestjs/common'; import Redis from 'ioredis'; -import { TimeService } from '@/core/TimeService.js'; -import { EnvService } from '@/core/EnvService.js'; +import type { TimeService } from '@/core/TimeService.js'; +import type { EnvService } from '@/core/EnvService.js'; import { BucketRateLimit, LegacyRateLimit, LimitInfo, RateLimit, hasMinLimit, isLegacyRateLimit, Keyed, hasMaxLimit, disabledLimitInfo, MaxLegacyLimit, MinLegacyLimit } from '@/misc/rate-limit-utils.js'; import { DI } from '@/di-symbols.js'; +import { MemoryKVCache } from '@/misc/cache.js'; +import type { MiUser } from '@/models/_.js'; +import type { RoleService } from '@/core/RoleService.js'; + +// Sentinel value used for caching the default role template. +// Required because MemoryKVCache doesn't support null keys. +const defaultUserKey = ''; @Injectable() export class SkRateLimiterService { + // 1-minute cache interval + private readonly factorCache = new MemoryKVCache(1000 * 60); private readonly disabled: boolean; constructor( - @Inject(TimeService) + @Inject('TimeService') private readonly timeService: TimeService, @Inject(DI.redis) private readonly redisClient: Redis.Redis, - @Inject(EnvService) + @Inject('RoleService') + private readonly roleService: RoleService, + + @Inject('EnvService') envService: EnvService, ) { this.disabled = envService.env.NODE_ENV === 'test'; } /** - * Check & increment a rate limit + * Check & increment a rate limit for a client * @param limit The limit definition - * @param actor Client who is calling this limit - * @param factor Scaling factor - smaller = larger limit (less restrictive) + * @param actorOrUser authenticated client user or IP hash */ - public async limit(limit: Keyed, actor: string, factor = 1): Promise { - if (this.disabled || factor === 0) { + public async limit(limit: Keyed, actorOrUser: string | MiUser): Promise { + if (this.disabled) { + return disabledLimitInfo; + } + + const actor = typeof(actorOrUser) === 'object' ? actorOrUser.id : actorOrUser; + const userCacheKey = typeof(actorOrUser) === 'object' ? actorOrUser.id : defaultUserKey; + const userRoleKey = typeof(actorOrUser) === 'object' ? actorOrUser.id : null; + const factor = this.factorCache.get(userCacheKey) ?? await this.factorCache.fetch(userCacheKey, async () => { + const role = await this.roleService.getUserPolicies(userRoleKey); + return role.rateLimitFactor; + }); + + if (factor === 0) { return disabledLimitInfo; } @@ -42,10 +65,6 @@ export class SkRateLimiterService { throw new Error(`Rate limit factor is zero or negative: ${factor}`); } - return await this.tryLimit(limit, actor, factor); - } - - private async tryLimit(limit: Keyed, actor: string, factor: number): Promise { if (isLegacyRateLimit(limit)) { return await this.limitLegacy(limit, actor, factor); } else { diff --git a/packages/backend/src/server/api/StreamingApiServerService.ts b/packages/backend/src/server/api/StreamingApiServerService.ts index e3fd1312ae..f30bbb928b 100644 --- a/packages/backend/src/server/api/StreamingApiServerService.ts +++ b/packages/backend/src/server/api/StreamingApiServerService.ts @@ -18,7 +18,6 @@ import { CacheService } from '@/core/CacheService.js'; import { MiLocalUser } from '@/models/User.js'; import { UserService } from '@/core/UserService.js'; import { ChannelFollowingService } from '@/core/ChannelFollowingService.js'; -import { RoleService } from '@/core/RoleService.js'; import { getIpHash } from '@/misc/get-ip-hash.js'; import { LoggerService } from '@/core/LoggerService.js'; import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; @@ -49,7 +48,6 @@ export class StreamingApiServerService { private usersService: UserService, private channelFollowingService: ChannelFollowingService, private rateLimiterService: SkRateLimiterService, - private roleService: RoleService, private loggerService: LoggerService, ) { } @@ -57,22 +55,18 @@ export class StreamingApiServerService { @bindThis private async rateLimitThis( user: MiLocalUser | null | undefined, - requestIp: string | undefined, + requestIp: string, limit: IEndpointMeta['limit'] & { key: NonNullable }, ) : Promise { - let limitActor: string; + let limitActor: string | MiLocalUser; if (user) { - limitActor = user.id; + limitActor = user; } else { - limitActor = getIpHash(requestIp || 'wtf'); + limitActor = getIpHash(requestIp); } - const factor = user ? (await this.roleService.getUserPolicies(user.id)).rateLimitFactor : 1; - - if (factor <= 0) return false; - // Rate limit - const rateLimit = await this.rateLimiterService.limit(limit, limitActor, factor); + const rateLimit = await this.rateLimiterService.limit(limit, limitActor); return rateLimit.blocked; } diff --git a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts index d13dbd2a71..6d4f117c87 100644 --- a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts +++ b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts @@ -4,6 +4,8 @@ */ import type Redis from 'ioredis'; +import type { MiUser } from '@/models/User.js'; +import type { RolePolicies, RoleService } from '@/core/RoleService.js'; import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { BucketRateLimit, Keyed, LegacyRateLimit } from '@/misc/rate-limit-utils.js'; @@ -15,6 +17,8 @@ describe(SkRateLimiterService, () => { let mockRedisExec: (batch: [string, ...unknown[]][]) => Promise<[Error | null, unknown][] | null> = null!; let mockEnvironment: Record = null!; let serviceUnderTest: () => SkRateLimiterService = null!; + let mockDefaultUserPolicies: Partial = null!; + let mockUserPolicies: Record> = null!; beforeEach(() => { mockTimeService = { @@ -69,9 +73,18 @@ describe(SkRateLimiterService, () => { env: mockEnvironment, }; + mockDefaultUserPolicies = { rateLimitFactor: 1 }; + mockUserPolicies = {}; + const mockRoleService = { + getUserPolicies(key: string | null) { + const policies = key != null ? mockUserPolicies[key] : null; + return Promise.resolve(policies ?? mockDefaultUserPolicies); + }, + } as unknown as RoleService; + let service: SkRateLimiterService | undefined = undefined; serviceUnderTest = () => { - return service ??= new SkRateLimiterService(mockTimeService, mockRedisClient, mockEnvService); + return service ??= new SkRateLimiterService(mockTimeService, mockRedisClient, mockRoleService, mockEnvService); }; }); @@ -284,11 +297,12 @@ describe(SkRateLimiterService, () => { }); it('should scale limit by factor', async () => { + mockDefaultUserPolicies.rateLimitFactor = 0.5; limitCounter = 1; limitTimestamp = 0; - const i1 = await serviceUnderTest().limit(limit, actor, 0.5); // 1 + 1 = 2 - const i2 = await serviceUnderTest().limit(limit, actor, 0.5); // 2 + 1 = 3 + const i1 = await serviceUnderTest().limit(limit, actor); // 1 + 1 = 2 + const i2 = await serviceUnderTest().limit(limit, actor); // 2 + 1 = 3 expect(i1.blocked).toBeFalsy(); expect(i2.blocked).toBeTruthy(); @@ -330,14 +344,18 @@ describe(SkRateLimiterService, () => { }); it('should skip if factor is zero', async () => { - const info = await serviceUnderTest().limit(limit, actor, 0); + mockDefaultUserPolicies.rateLimitFactor = 0; + + const info = await serviceUnderTest().limit(limit, actor); expect(info.blocked).toBeFalsy(); expect(info.remaining).toBe(Number.MAX_SAFE_INTEGER); }); it('should throw if factor is negative', async () => { - const promise = serviceUnderTest().limit(limit, actor, -1); + mockDefaultUserPolicies.rateLimitFactor = -1; + + const promise = serviceUnderTest().limit(limit, actor); await expect(promise).rejects.toThrow(/factor is zero or negative/); }); @@ -426,6 +444,19 @@ describe(SkRateLimiterService, () => { expect(info.fullResetMs).toBe(2000); expect(info.fullResetSec).toBe(2); }); + + it('should look up factor by user ID', async () => { + const userActor = { id: actor } as unknown as MiUser; + mockUserPolicies[actor] = { rateLimitFactor: 0.5 }; + limitCounter = 1; + limitTimestamp = 0; + + const i1 = await serviceUnderTest().limit(limit, userActor); // 1 + 1 = 2 + const i2 = await serviceUnderTest().limit(limit, userActor); // 2 + 1 = 3 + + expect(i1.blocked).toBeFalsy(); + expect(i2.blocked).toBeTruthy(); + }); }); describe('with min interval', () => { @@ -529,11 +560,12 @@ describe(SkRateLimiterService, () => { }); it('should scale interval by factor', async () => { + mockDefaultUserPolicies.rateLimitFactor = 0.5; limitCounter = 1; limitTimestamp = 0; mockTimeService.now += 500; - const info = await serviceUnderTest().limit(limit, actor, 0.5); + const info = await serviceUnderTest().limit(limit, actor); expect(info.blocked).toBeFalsy(); }); @@ -574,14 +606,18 @@ describe(SkRateLimiterService, () => { }); it('should skip if factor is zero', async () => { - const info = await serviceUnderTest().limit(limit, actor, 0); + mockDefaultUserPolicies.rateLimitFactor = 0; + + const info = await serviceUnderTest().limit(limit, actor); expect(info.blocked).toBeFalsy(); expect(info.remaining).toBe(Number.MAX_SAFE_INTEGER); }); it('should throw if factor is negative', async () => { - const promise = serviceUnderTest().limit(limit, actor, -1); + mockDefaultUserPolicies.rateLimitFactor = -1; + + const promise = serviceUnderTest().limit(limit, actor); await expect(promise).rejects.toThrow(/factor is zero or negative/); }); @@ -701,10 +737,11 @@ describe(SkRateLimiterService, () => { }); it('should scale limit by factor', async () => { + mockDefaultUserPolicies.rateLimitFactor = 0.5; limitCounter = 10; limitTimestamp = 0; - const info = await serviceUnderTest().limit(limit, actor, 0.5); // 10 + 1 = 11 + const info = await serviceUnderTest().limit(limit, actor); // 10 + 1 = 11 expect(info.blocked).toBeTruthy(); }); @@ -760,14 +797,18 @@ describe(SkRateLimiterService, () => { }); it('should skip if factor is zero', async () => { - const info = await serviceUnderTest().limit(limit, actor, 0); + mockDefaultUserPolicies.rateLimitFactor = 0; + + const info = await serviceUnderTest().limit(limit, actor); expect(info.blocked).toBeFalsy(); expect(info.remaining).toBe(Number.MAX_SAFE_INTEGER); }); it('should throw if factor is negative', async () => { - const promise = serviceUnderTest().limit(limit, actor, -1); + mockDefaultUserPolicies.rateLimitFactor = -1; + + const promise = serviceUnderTest().limit(limit, actor); await expect(promise).rejects.toThrow(/factor is zero or negative/); }); @@ -890,11 +931,12 @@ describe(SkRateLimiterService, () => { }); it('should scale limit and interval by factor', async () => { + mockDefaultUserPolicies.rateLimitFactor = 0.5; limitCounter = 5; limitTimestamp = 0; mockTimeService.now += 500; - const info = await serviceUnderTest().limit(limit, actor, 0.5); + const info = await serviceUnderTest().limit(limit, actor); expect(info.blocked).toBeFalsy(); }); -- cgit v1.2.3-freya From b171c1c4408c1438976f3473c8104b68305a53f9 Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Wed, 5 Feb 2025 11:18:53 -0500 Subject: move imports to fix git diff in ActivityPubServerService.ts --- packages/backend/src/server/ActivityPubServerService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'packages/backend/src/server/ActivityPubServerService.ts') diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 19049e528c..6e8c249471 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -30,12 +30,12 @@ import type { MiNote } from '@/models/Note.js'; import { QueryService } from '@/core/QueryService.js'; import { UtilityService } from '@/core/UtilityService.js'; import { UserEntityService } from '@/core/entities/UserEntityService.js'; +import type Logger from '@/logger.js'; +import { LoggerService } from '@/core/LoggerService.js'; import { bindThis } from '@/decorators.js'; import { IActivity } from '@/core/activitypub/type.js'; import { isQuote, isRenote } from '@/misc/is-renote.js'; import * as Acct from '@/misc/acct.js'; -import type Logger from '@/logger.js'; -import { LoggerService } from '@/core/LoggerService.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; import type { FindOptionsWhere } from 'typeorm'; -- cgit v1.2.3-freya From 6c2034a3736aa1541be9fa257b1bf0ee9162284b Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Wed, 12 Feb 2025 14:13:00 -0500 Subject: append default CW when rendering AP `Note` objects --- packages/backend/src/core/PollService.ts | 2 +- .../src/core/activitypub/ApRendererService.ts | 16 ++-- .../src/core/activitypub/ApResolverService.ts | 5 +- .../backend/src/server/ActivityPubServerService.ts | 17 ++-- packages/backend/test/unit/activitypub.ts | 92 +++++++++++++++++++++- 5 files changed, 117 insertions(+), 15 deletions(-) (limited to 'packages/backend/src/server/ActivityPubServerService.ts') diff --git a/packages/backend/src/core/PollService.ts b/packages/backend/src/core/PollService.ts index 6c96ab16cf..d6364613bd 100644 --- a/packages/backend/src/core/PollService.ts +++ b/packages/backend/src/core/PollService.ts @@ -100,7 +100,7 @@ export class PollService { if (user == null) throw new Error('note not found'); if (this.userEntityService.isLocalUser(user)) { - const content = this.apRendererService.addContext(this.apRendererService.renderUpdate(await this.apRendererService.renderNote(note, false), user)); + const content = this.apRendererService.addContext(this.apRendererService.renderUpdate(await this.apRendererService.renderNote(note, user, false), user)); this.apDeliverManagerService.deliverToFollowers(user, content); this.relayService.deliverToRelays(user, content); } diff --git a/packages/backend/src/core/activitypub/ApRendererService.ts b/packages/backend/src/core/activitypub/ApRendererService.ts index 721cb77b2f..22cb6b8282 100644 --- a/packages/backend/src/core/activitypub/ApRendererService.ts +++ b/packages/backend/src/core/activitypub/ApRendererService.ts @@ -28,6 +28,7 @@ import type { UsersRepository, UserProfilesRepository, NotesRepository, DriveFil import { bindThis } from '@/decorators.js'; import { CustomEmojiService } from '@/core/CustomEmojiService.js'; import { IdService } from '@/core/IdService.js'; +import { appendContentWarning } from '@/misc/append-content-warning.js'; import { JsonLdService } from './JsonLdService.js'; import { ApMfmService } from './ApMfmService.js'; import { CONTEXT } from './misc/contexts.js'; @@ -339,7 +340,7 @@ export class ApRendererService { } @bindThis - public async renderNote(note: MiNote, dive = true): Promise { + public async renderNote(note: MiNote, author: MiUser, dive = true): Promise { const getPromisedFiles = async (ids: string[]): Promise => { if (ids.length === 0) return []; const items = await this.driveFilesRepository.findBy({ id: In(ids) }); @@ -353,14 +354,14 @@ export class ApRendererService { inReplyToNote = await this.notesRepository.findOneBy({ id: note.replyId }); if (inReplyToNote != null) { - const inReplyToUserExist = await this.usersRepository.exists({ where: { id: inReplyToNote.userId } }); + const inReplyToUser = await this.usersRepository.findOneBy({ id: inReplyToNote.userId }); - if (inReplyToUserExist) { + if (inReplyToUser) { if (inReplyToNote.uri) { inReplyTo = inReplyToNote.uri; } else { if (dive) { - inReplyTo = await this.renderNote(inReplyToNote, false); + inReplyTo = await this.renderNote(inReplyToNote, inReplyToUser, false); } else { inReplyTo = `${this.config.url}/notes/${inReplyToNote.id}`; } @@ -423,7 +424,12 @@ export class ApRendererService { apAppend += `\n\nRE: ${quote}`; } - const summary = note.cw === '' ? String.fromCharCode(0x200B) : note.cw; + let summary = note.cw === '' ? String.fromCharCode(0x200B) : note.cw; + + // Apply mandatory CW, if applicable + if (author.mandatoryCW) { + summary = appendContentWarning(summary, author.mandatoryCW); + } const { content } = this.apMfmService.getNoteHtml(note, apAppend); diff --git a/packages/backend/src/core/activitypub/ApResolverService.ts b/packages/backend/src/core/activitypub/ApResolverService.ts index a0c3a4846c..dfeee5ac7f 100644 --- a/packages/backend/src/core/activitypub/ApResolverService.ts +++ b/packages/backend/src/core/activitypub/ApResolverService.ts @@ -156,11 +156,12 @@ export class Resolver { case 'notes': return this.notesRepository.findOneByOrFail({ id: parsed.id }) .then(async note => { + const author = await this.usersRepository.findOneByOrFail({ id: note.userId }); if (parsed.rest === 'activity') { // this refers to the create activity and not the note itself - return this.apRendererService.addContext(this.apRendererService.renderCreate(await this.apRendererService.renderNote(note), note)); + return this.apRendererService.addContext(this.apRendererService.renderCreate(await this.apRendererService.renderNote(note, author), note)); } else { - return this.apRendererService.renderNote(note); + return this.apRendererService.renderNote(note, author); } }); case 'users': diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 72faa3318c..10dba1660f 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -103,15 +103,16 @@ export class ActivityPubServerService { /** * Pack Create or Announce Activity * @param note Note + * @param author Author of the note */ @bindThis - private async packActivity(note: MiNote): Promise { + private async packActivity(note: MiNote, author: MiUser): Promise { if (isRenote(note) && !isQuote(note)) { const renote = await this.notesRepository.findOneByOrFail({ id: note.renoteId }); return this.apRendererService.renderAnnounce(renote.uri ? renote.uri : `${this.config.url}/notes/${renote.id}`, note); } - return this.apRendererService.renderCreate(await this.apRendererService.renderNote(note, false), note); + return this.apRendererService.renderCreate(await this.apRendererService.renderNote(note, author, false), note); } @bindThis @@ -506,7 +507,7 @@ export class ActivityPubServerService { this.notesRepository.findOneByOrFail({ id: pining.noteId })))) .filter(note => !note.localOnly && ['public', 'home'].includes(note.visibility)); - const renderedNotes = await Promise.all(pinnedNotes.map(note => this.apRendererService.renderNote(note))); + const renderedNotes = await Promise.all(pinnedNotes.map(note => this.apRendererService.renderNote(note, user))); const rendered = this.apRendererService.renderOrderedCollection( `${this.config.url}/users/${userId}/collections/featured`, @@ -579,7 +580,7 @@ export class ActivityPubServerService { if (sinceId) notes.reverse(); - const activities = await Promise.all(notes.map(note => this.packActivity(note))); + const activities = await Promise.all(notes.map(note => this.packActivity(note, user))); const rendered = this.apRendererService.renderOrderedCollectionPage( `${partOf}?${url.query({ page: 'true', @@ -723,7 +724,9 @@ export class ActivityPubServerService { if (!this.config.checkActivityPubGetSignature) reply.header('Cache-Control', 'public, max-age=180'); this.setResponseType(request, reply); - return this.apRendererService.addContext(await this.apRendererService.renderNote(note, false)); + + const author = await this.usersRepository.findOneByOrFail({ id: note.userId }); + return this.apRendererService.addContext(await this.apRendererService.renderNote(note, author, false)); }); // note activity @@ -746,7 +749,9 @@ export class ActivityPubServerService { if (!this.config.checkActivityPubGetSignature) reply.header('Cache-Control', 'public, max-age=180'); this.setResponseType(request, reply); - return (this.apRendererService.addContext(await this.packActivity(note))); + + const author = await this.usersRepository.findOneByOrFail({ id: note.userId }); + return (this.apRendererService.addContext(await this.packActivity(note, author))); }); // outbox diff --git a/packages/backend/test/unit/activitypub.ts b/packages/backend/test/unit/activitypub.ts index 73d6186edf..105a3292bf 100644 --- a/packages/backend/test/unit/activitypub.ts +++ b/packages/backend/test/unit/activitypub.ts @@ -3,6 +3,8 @@ * SPDX-License-Identifier: AGPL-3.0-only */ +import { IdService } from '@/core/IdService.js'; + process.env.NODE_ENV = 'test'; import * as assert from 'assert'; @@ -20,7 +22,7 @@ import { CoreModule } from '@/core/CoreModule.js'; import { FederatedInstanceService } from '@/core/FederatedInstanceService.js'; import { LoggerService } from '@/core/LoggerService.js'; import type { IActor, IApDocument, ICollection, IObject, IPost } from '@/core/activitypub/type.js'; -import { MiMeta, MiNote, UserProfilesRepository } from '@/models/_.js'; +import { MiMeta, MiNote, MiUser, UserProfilesRepository } from '@/models/_.js'; import { DI } from '@/di-symbols.js'; import { secureRndstr } from '@/misc/secure-rndstr.js'; import { DownloadService } from '@/core/DownloadService.js'; @@ -93,6 +95,7 @@ describe('ActivityPub', () => { let rendererService: ApRendererService; let jsonLdService: JsonLdService; let resolver: MockResolver; + let idService: IdService; const metaInitial = { cacheRemoteFiles: true, @@ -140,6 +143,7 @@ describe('ActivityPub', () => { imageService = app.get(ApImageService); jsonLdService = app.get(JsonLdService); resolver = new MockResolver(await app.resolve(LoggerService)); + idService = app.get(IdService); // Prevent ApPersonService from fetching instance, as it causes Jest import-after-test error const federatedInstanceService = app.get(FederatedInstanceService); @@ -477,4 +481,90 @@ describe('ActivityPub', () => { }); }); }); + + describe(ApRendererService, () => { + describe('renderNote', () => { + let note: MiNote; + let author: MiUser; + + beforeEach(() => { + author = new MiUser({ + id: idService.gen(), + }); + note = new MiNote({ + id: idService.gen(), + userId: author.id, + visibility: 'public', + localOnly: false, + text: 'Note text', + cw: null, + renoteCount: 0, + repliesCount: 0, + clippedCount: 0, + reactions: {}, + fileIds: [], + attachedFileTypes: [], + visibleUserIds: [], + mentions: [], + // This is fucked tbh - it's JSON stored in a TEXT column that gets parsed/serialized all over the place + mentionedRemoteUsers: '[]', + reactionAndUserPairCache: [], + emojis: [], + tags: [], + hasPoll: false, + }); + }); + + describe('summary', () => { + // I actually don't know why it does this, but the logic was already there so I've preserved it. + it('should be special character when CW is empty string', async () => { + note.cw = ''; + + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBe(String.fromCharCode(0x200B)); + }); + + it('should be undefined when CW is null', async () => { + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBeUndefined(); + }); + + it('should be CW when present without mandatoryCW', async () => { + note.cw = 'original'; + + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBe('original'); + }); + + it('should be mandatoryCW when present without CW', async () => { + author.mandatoryCW = 'mandatory'; + + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBe('mandatory'); + }); + + it('should be merged when CW and mandatoryCW are both present', async () => { + note.cw = 'original'; + author.mandatoryCW = 'mandatory'; + + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBe('original, mandatory'); + }); + + it('should be CW when CW includes mandatoryCW', async () => { + note.cw = 'original and mandatory'; + author.mandatoryCW = 'mandatory'; + + const result = await rendererService.renderNote(note, author, false); + + expect(result.summary).toBe('original and mandatory'); + }); + }); + }); + }); }); -- cgit v1.2.3-freya