From ffc2737478c6f9efd5de9fbaf526b13164727f87 Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sat, 7 Dec 2024 10:22:45 -0500 Subject: implement SkRateLimiterService with Leaky Bucket rate limiting --- packages/backend/src/server/api/RateLimiterService.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'packages/backend/src/server/api/RateLimiterService.ts') diff --git a/packages/backend/src/server/api/RateLimiterService.ts b/packages/backend/src/server/api/RateLimiterService.ts index e9afb9d05a..33db016a7c 100644 --- a/packages/backend/src/server/api/RateLimiterService.ts +++ b/packages/backend/src/server/api/RateLimiterService.ts @@ -10,28 +10,28 @@ import { DI } from '@/di-symbols.js'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; import { bindThis } from '@/decorators.js'; +import type { LimitInfo } from '@/server/api/SkRateLimiterService.js'; +import { EnvService } from '@/core/EnvService.js'; import type { IEndpointMeta } from './endpoints.js'; @Injectable() export class RateLimiterService { - private logger: Logger; - private disabled = false; + protected readonly logger: Logger; + protected readonly disabled: boolean; constructor( @Inject(DI.redis) - private redisClient: Redis.Redis, + protected readonly redisClient: Redis.Redis, private loggerService: LoggerService, + envService: EnvService, ) { this.logger = this.loggerService.getLogger('limiter'); - - if (process.env.NODE_ENV !== 'production') { - this.disabled = true; - } + this.disabled = envService.env.NODE_ENV !== 'production'; } @bindThis - public limit(limitation: IEndpointMeta['limit'] & { key: NonNullable }, actor: string, factor = 1) { + public limit(limitation: IEndpointMeta['limit'] & { key: NonNullable }, actor: string, factor = 1): Promise { return new Promise((ok, reject) => { if (this.disabled) ok(); -- cgit v1.2.3-freya From f6b256620b9637ffe4bd29a07cfba1a7880c9bb1 Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sat, 7 Dec 2024 13:13:19 -0500 Subject: separate SkRateLimiterService from RateLimiterService and update all usages --- packages/backend/src/misc/rate-limit-utils.ts | 21 +++++++++ packages/backend/src/server/FileServerService.ts | 45 ++++++++---------- packages/backend/src/server/ServerModule.ts | 7 ++- packages/backend/src/server/api/ApiCallService.ts | 55 ++++------------------ .../backend/src/server/api/RateLimiterService.ts | 1 + .../backend/src/server/api/SigninApiService.ts | 17 ++++--- .../src/server/api/SigninWithPasskeyApiService.ts | 17 ++++--- .../backend/src/server/api/SkRateLimiterService.ts | 28 ++++------- .../src/server/api/StreamingApiServerService.ts | 20 ++++---- .../test/unit/SigninWithPasskeyApiService.ts | 15 ++++-- 10 files changed, 102 insertions(+), 124 deletions(-) create mode 100644 packages/backend/src/misc/rate-limit-utils.ts (limited to 'packages/backend/src/server/api/RateLimiterService.ts') diff --git a/packages/backend/src/misc/rate-limit-utils.ts b/packages/backend/src/misc/rate-limit-utils.ts new file mode 100644 index 0000000000..0f38755502 --- /dev/null +++ b/packages/backend/src/misc/rate-limit-utils.ts @@ -0,0 +1,21 @@ +/* + * SPDX-FileCopyrightText: hazelnoot and other Sharkey contributors + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import { FastifyReply } from 'fastify'; +import { LimitInfo } from '@/server/api/SkRateLimiterService.js'; + +export function sendRateLimitHeaders(reply: FastifyReply, info: LimitInfo): void { + // Number of seconds until the limit has fully reset. + reply.header('X-RateLimit-Clear', info.fullResetSec.toString()); + // Number of calls that can be made before being limited. + reply.header('X-RateLimit-Remaining', info.remaining.toString()); + + if (info.blocked) { + // Number of seconds to wait before trying again. Left for backwards compatibility. + reply.header('Retry-After', info.resetSec.toString()); + // Number of milliseconds to wait before trying again. + reply.header('X-RateLimit-Reset', info.resetMs.toString()); + } +} diff --git a/packages/backend/src/server/FileServerService.ts b/packages/backend/src/server/FileServerService.ts index ca1a6ebe2e..9903113f43 100644 --- a/packages/backend/src/server/FileServerService.ts +++ b/packages/backend/src/server/FileServerService.ts @@ -28,13 +28,12 @@ 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 { IEndpointMeta } from '@/server/api/endpoints.js'; import { RoleService } from '@/core/RoleService.js'; +import { RateLimit, SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; +import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions } from 'fastify'; -import type Limiter from 'ratelimiter'; const _filename = fileURLToPath(import.meta.url); const _dirname = dirname(_filename); @@ -59,7 +58,7 @@ export class FileServerService { private internalStorageService: InternalStorageService, private loggerService: LoggerService, private authenticateService: AuthenticateService, - private rateLimiterService: RateLimiterService, + private rateLimiterService: SkRateLimiterService, private roleService: RoleService, ) { this.logger = this.loggerService.getLogger('server', 'gray'); @@ -634,42 +633,37 @@ export class FileServerService { } private async checkResourceLimit(reply: FastifyReply, actor: string, group: string, resource: string, factor = 1): Promise { - const limit = { + const limit: RateLimit = { // Group by resource key: `${group}${resource}`, + type: 'bucket', - // Maximum of 10 requests / 10 minutes - max: 10, - duration: 1000 * 60 * 10, + // Maximum of 10 requests, average rate of 1 per minute + size: 10, + dripRate: 1000 * 60, }; return await this.checkLimit(reply, actor, limit, factor); } private async checkSharedLimit(reply: FastifyReply, actor: string, group: string, factor = 1): Promise { - const limit = { + const limit: RateLimit = { key: group, + type: 'bucket', - // Maximum of 3600 requests per hour, which is an average of 1 per second. - max: 3600, - duration: 1000 * 60 * 60, + // Maximum of 3600 requests, average rate of 1 per second. + size: 3600, }; return await this.checkLimit(reply, actor, limit, factor); } - private async checkLimit(reply: FastifyReply, actor: string, limit: IEndpointMeta['limit'] & { key: NonNullable }, factor = 1): Promise { - try { - await this.rateLimiterService.limit(limit, actor, factor); - return true; - } catch (err) { - // errはLimiter.LimiterInfoであることが期待される - if (hasRateLimitInfo(err)) { - const cooldownInSeconds = Math.ceil((err.info.resetMs - Date.now()) / 1000); - // もしかするとマイナスになる可能性がなくはないのでマイナスだったら0にしておく - reply.header('Retry-After', Math.max(cooldownInSeconds, 0).toString(10)); - } + private async checkLimit(reply: FastifyReply, actor: string, limit: RateLimit, factor = 1): Promise { + const info = await this.rateLimiterService.limit(limit, actor, factor); + + sendRateLimitHeaders(reply, info); + if (info.blocked) { reply.code(429); reply.send({ message: 'Rate limit exceeded. Please try again later.', @@ -679,9 +673,8 @@ export class FileServerService { return false; } + + return true; } } -function hasRateLimitInfo(err: unknown): err is { info: Limiter.LimiterInfo } { - return err != null && typeof(err) === 'object' && 'info' in err; -} diff --git a/packages/backend/src/server/ServerModule.ts b/packages/backend/src/server/ServerModule.ts index 890447a47f..c1d7c088f1 100644 --- a/packages/backend/src/server/ServerModule.ts +++ b/packages/backend/src/server/ServerModule.ts @@ -74,10 +74,9 @@ import { SigninWithPasskeyApiService } from './api/SigninWithPasskeyApiService.j ApiLoggerService, ApiServerService, AuthenticateService, - { - provide: RateLimiterService, - useClass: SkRateLimiterService, - }, + SkRateLimiterService, + // No longer used, but kept for backwards compatibility + RateLimiterService, SigninApiService, SigninWithPasskeyApiService, SigninService, diff --git a/packages/backend/src/server/api/ApiCallService.ts b/packages/backend/src/server/api/ApiCallService.ts index 14367e02bb..38d33c761d 100644 --- a/packages/backend/src/server/api/ApiCallService.ts +++ b/packages/backend/src/server/api/ApiCallService.ts @@ -8,7 +8,6 @@ import * as fs from 'node:fs'; import * as stream from 'node:stream/promises'; import { Inject, Injectable } from '@nestjs/common'; import * as Sentry from '@sentry/node'; -import { LimiterInfo } from 'ratelimiter'; import { DI } from '@/di-symbols.js'; import { getIpHash } from '@/misc/get-ip-hash.js'; import type { MiLocalUser, MiUser } from '@/models/User.js'; @@ -19,9 +18,9 @@ import { createTemp } from '@/misc/create-temp.js'; import { bindThis } from '@/decorators.js'; import { RoleService } from '@/core/RoleService.js'; import type { Config } from '@/config.js'; -import { isLimitInfo } from '@/server/api/SkRateLimiterService.js'; +import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; +import { LegacyRateLimit, SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { ApiError } from './error.js'; -import { RateLimiterService } from './RateLimiterService.js'; import { ApiLoggerService } from './ApiLoggerService.js'; import { AuthenticateService, AuthenticationError } from './AuthenticateService.js'; import type { FastifyRequest, FastifyReply } from 'fastify'; @@ -51,7 +50,7 @@ export class ApiCallService implements OnApplicationShutdown { private userIpsRepository: UserIpsRepository, private authenticateService: AuthenticateService, - private rateLimiterService: RateLimiterService, + private rateLimiterService: SkRateLimiterService, private roleService: RoleService, private apiLoggerService: ApiLoggerService, ) { @@ -67,21 +66,6 @@ export class ApiCallService implements OnApplicationShutdown { let statusCode = err.httpStatusCode; if (err.httpStatusCode === 401) { reply.header('WWW-Authenticate', 'Bearer realm="Misskey"'); - } else if (err.code === 'RATE_LIMIT_EXCEEDED') { - const info: unknown = err.info; - const unixEpochInSeconds = Date.now(); - if (isLimitInfo(info)) { - // Number of seconds to wait before trying again. Left for backwards compatibility. - reply.header('Retry-After', info.resetSec.toString()); - // Number of milliseconds to wait before trying again. - reply.header('X-RateLimit-Reset', info.resetMs.toString()); - } else if (typeof(info) === 'object' && info && 'resetMs' in info && typeof(info.resetMs) === 'number') { - const cooldownInSeconds = Math.ceil((info.resetMs - unixEpochInSeconds) / 1000); - // もしかするとマイナスになる可能性がなくはないのでマイナスだったら0にしておく - reply.header('Retry-After', Math.max(cooldownInSeconds, 0).toString(10)); - } else { - this.logger.warn(`rate limit information has unexpected type: ${JSON.stringify(info)}`); - } } else if (err.kind === 'client') { reply.header('WWW-Authenticate', `Bearer realm="Misskey", error="invalid_request", error_description="${err.message}"`); statusCode = statusCode ?? 400; @@ -347,40 +331,17 @@ export class ApiCallService implements OnApplicationShutdown { if (factor > 0) { // Rate limit - const info = await this.rateLimiterService.limit(limit as IEndpointMeta['limit'] & { key: NonNullable }, limitActor, factor) - .then(info => { - // We always want these headers, because clients need them for pacing. - // Conditional check in case we somehow revert to the old limiter, which does not return info. - if (info) { - // Number of seconds until the limit has fully reset. - reply.header('X-RateLimit-Clear', info.fullResetSec.toString()); - // Number of calls that can be made before being limited. - reply.header('X-RateLimit-Remaining', info.remaining.toString()); - - // Only forward the info object if it's blocked, otherwise we'll reject *all* requests - if (info.blocked) { - return info; - } - } - - return undefined; - }) - .catch(err => { - // The old limiter throws info instead of returning it. - if ('info' in err) { - return err.info as LimiterInfo; - } else { - throw err; - } - }); + const info = await this.rateLimiterService.limit(limit as LegacyRateLimit, limitActor, factor); - if (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); + }); } } } diff --git a/packages/backend/src/server/api/RateLimiterService.ts b/packages/backend/src/server/api/RateLimiterService.ts index 33db016a7c..6037f9bf92 100644 --- a/packages/backend/src/server/api/RateLimiterService.ts +++ b/packages/backend/src/server/api/RateLimiterService.ts @@ -14,6 +14,7 @@ import type { LimitInfo } from '@/server/api/SkRateLimiterService.js'; import { EnvService } from '@/core/EnvService.js'; import type { IEndpointMeta } from './endpoints.js'; +/** @deprecated Use SkRateLimiterService instead */ @Injectable() export class RateLimiterService { protected readonly logger: Logger; diff --git a/packages/backend/src/server/api/SigninApiService.ts b/packages/backend/src/server/api/SigninApiService.ts index 64af7da7a6..1a4ce0a54c 100644 --- a/packages/backend/src/server/api/SigninApiService.ts +++ b/packages/backend/src/server/api/SigninApiService.ts @@ -21,12 +21,13 @@ import { IdService } from '@/core/IdService.js'; import { bindThis } from '@/decorators.js'; import { WebAuthnService } from '@/core/WebAuthnService.js'; import { UserAuthService } from '@/core/UserAuthService.js'; -import { RateLimiterService } from './RateLimiterService.js'; +import { isSystemAccount } from '@/misc/is-system-account.js'; +import type { MiMeta } from '@/models/_.js'; +import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; +import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import { SigninService } from './SigninService.js'; import type { AuthenticationResponseJSON } from '@simplewebauthn/types'; import type { FastifyReply, FastifyRequest } from 'fastify'; -import { isSystemAccount } from '@/misc/is-system-account.js'; -import type { MiMeta } from '@/models/_.js'; @Injectable() export class SigninApiService { @@ -47,7 +48,7 @@ export class SigninApiService { private signinsRepository: SigninsRepository, private idService: IdService, - private rateLimiterService: RateLimiterService, + private rateLimiterService: SkRateLimiterService, private signinService: SigninService, private userAuthService: UserAuthService, private webAuthnService: WebAuthnService, @@ -79,10 +80,12 @@ export class SigninApiService { return { error }; } - try { // not more than 1 attempt per second and not more than 10 attempts per hour - await this.rateLimiterService.limit({ key: 'signin', duration: 60 * 60 * 1000, max: 10, minInterval: 1000 }, getIpHash(request.ip)); - } catch (err) { + const rateLimit = await this.rateLimiterService.limit({ key: 'signin', duration: 60 * 60 * 1000, max: 10, minInterval: 1000 }, getIpHash(request.ip)); + + sendRateLimitHeaders(reply, rateLimit); + + if (rateLimit.blocked) { reply.code(429); return { error: { diff --git a/packages/backend/src/server/api/SigninWithPasskeyApiService.ts b/packages/backend/src/server/api/SigninWithPasskeyApiService.ts index 9ba23c54e2..ad08dad79c 100644 --- a/packages/backend/src/server/api/SigninWithPasskeyApiService.ts +++ b/packages/backend/src/server/api/SigninWithPasskeyApiService.ts @@ -21,10 +21,11 @@ import { WebAuthnService } from '@/core/WebAuthnService.js'; import Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; import type { IdentifiableError } from '@/misc/identifiable-error.js'; -import { RateLimiterService } from './RateLimiterService.js'; +import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { SigninService } from './SigninService.js'; import type { AuthenticationResponseJSON } from '@simplewebauthn/types'; import type { FastifyReply, FastifyRequest } from 'fastify'; +import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; @Injectable() export class SigninWithPasskeyApiService { @@ -43,7 +44,7 @@ export class SigninWithPasskeyApiService { private signinsRepository: SigninsRepository, private idService: IdService, - private rateLimiterService: RateLimiterService, + private rateLimiterService: SkRateLimiterService, private signinService: SigninService, private webAuthnService: WebAuthnService, private loggerService: LoggerService, @@ -84,11 +85,13 @@ export class SigninWithPasskeyApiService { return error(status ?? 500, failure ?? { id: '4e30e80c-e338-45a0-8c8f-44455efa3b76' }); }; - try { - // Not more than 1 API call per 250ms and not more than 100 attempts per 30min - // NOTE: 1 Sign-in require 2 API calls - await this.rateLimiterService.limit({ key: 'signin-with-passkey', duration: 60 * 30 * 1000, max: 200, minInterval: 250 }, getIpHash(request.ip)); - } catch (err) { + // Not more than 1 API call per 250ms and not more than 100 attempts per 30min + // NOTE: 1 Sign-in require 2 API calls + const rateLimit = await this.rateLimiterService.limit({ key: 'signin-with-passkey', duration: 60 * 30 * 1000, max: 200, minInterval: 250 }, getIpHash(request.ip)); + + sendRateLimitHeaders(reply, rateLimit); + + if (rateLimit.blocked) { reply.code(429); return { error: { diff --git a/packages/backend/src/server/api/SkRateLimiterService.ts b/packages/backend/src/server/api/SkRateLimiterService.ts index 763de0029b..943348c41d 100644 --- a/packages/backend/src/server/api/SkRateLimiterService.ts +++ b/packages/backend/src/server/api/SkRateLimiterService.ts @@ -10,7 +10,7 @@ import { LoggerService } from '@/core/LoggerService.js'; import { TimeService } from '@/core/TimeService.js'; import { EnvService } from '@/core/EnvService.js'; import { DI } from '@/di-symbols.js'; -import { RateLimiterService } from './RateLimiterService.js'; +import type Logger from '@/logger.js'; /** * Metadata about the current status of a rate limiter @@ -51,18 +51,6 @@ export interface LimitInfo { fullResetMs: number; } -export function isLimitInfo(info: unknown): info is LimitInfo { - if (info == null) return false; - if (typeof(info) !== 'object') return false; - if (!('blocked' in info) || typeof(info.blocked) !== 'boolean') return false; - if (!('remaining' in info) || typeof(info.remaining) !== 'number') return false; - if (!('resetSec' in info) || typeof(info.resetSec) !== 'number') return false; - if (!('resetMs' in info) || typeof(info.resetMs) !== 'number') return false; - if (!('fullResetSec' in info) || typeof(info.fullResetSec) !== 'number') return false; - if (!('fullResetMs' in info) || typeof(info.fullResetMs) !== 'number') return false; - return true; -} - /** * Rate limit based on "leaky bucket" logic. * The bucket count increases with each call, and decreases gradually at a given rate. @@ -99,10 +87,10 @@ export interface RateLimit { } export type SupportedRateLimit = RateLimit | LegacyRateLimit; -export type LegacyRateLimit = IEndpointMeta['limit'] & { key: NonNullable, type: undefined | 'legacy' }; +export type LegacyRateLimit = IEndpointMeta['limit'] & { key: NonNullable, type?: undefined }; export function isLegacyRateLimit(limit: SupportedRateLimit): limit is LegacyRateLimit { - return limit.type === undefined || limit.type === 'legacy'; + return limit.type === undefined; } export function hasMinLimit(limit: LegacyRateLimit): limit is LegacyRateLimit & { minInterval: number } { @@ -110,13 +98,16 @@ export function hasMinLimit(limit: LegacyRateLimit): limit is LegacyRateLimit & } @Injectable() -export class SkRateLimiterService extends RateLimiterService { +export class SkRateLimiterService { + private readonly logger: Logger; + private readonly disabled: boolean; + constructor( @Inject(TimeService) private readonly timeService: TimeService, @Inject(DI.redis) - redisClient: Redis.Redis, + private readonly redisClient: Redis.Redis, @Inject(LoggerService) loggerService: LoggerService, @@ -124,7 +115,8 @@ export class SkRateLimiterService extends RateLimiterService { @Inject(EnvService) envService: EnvService, ) { - super(redisClient, loggerService, envService); + this.logger = loggerService.getLogger('limiter'); + this.disabled = envService.env.NODE_ENV !== 'production'; } public async limit(limit: SupportedRateLimit, actor: string, factor = 1): Promise { diff --git a/packages/backend/src/server/api/StreamingApiServerService.ts b/packages/backend/src/server/api/StreamingApiServerService.ts index 9b8464f705..e3fd1312ae 100644 --- a/packages/backend/src/server/api/StreamingApiServerService.ts +++ b/packages/backend/src/server/api/StreamingApiServerService.ts @@ -7,6 +7,8 @@ import { EventEmitter } from 'events'; import { Inject, Injectable } from '@nestjs/common'; import * as Redis from 'ioredis'; import * as WebSocket from 'ws'; +import proxyAddr from 'proxy-addr'; +import ms from 'ms'; import { DI } from '@/di-symbols.js'; import type { UsersRepository, MiAccessToken } from '@/models/_.js'; import { NoteReadService } from '@/core/NoteReadService.js'; @@ -16,18 +18,15 @@ 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'; import { AuthenticateService, AuthenticationError } from './AuthenticateService.js'; import MainStreamConnection from './stream/Connection.js'; import { ChannelsService } from './stream/ChannelsService.js'; -import { RateLimiterService } from './RateLimiterService.js'; -import { RoleService } from '@/core/RoleService.js'; -import { getIpHash } from '@/misc/get-ip-hash.js'; -import proxyAddr from 'proxy-addr'; -import ms from 'ms'; import type * as http from 'node:http'; import type { IEndpointMeta } from './endpoints.js'; -import { LoggerService } from '@/core/LoggerService.js'; -import type Logger from '@/logger.js'; @Injectable() export class StreamingApiServerService { @@ -49,7 +48,7 @@ export class StreamingApiServerService { private notificationService: NotificationService, private usersService: UserService, private channelFollowingService: ChannelFollowingService, - private rateLimiterService: RateLimiterService, + private rateLimiterService: SkRateLimiterService, private roleService: RoleService, private loggerService: LoggerService, ) { @@ -73,9 +72,8 @@ export class StreamingApiServerService { if (factor <= 0) return false; // Rate limit - return await this.rateLimiterService.limit(limit, limitActor, factor) - .then(() => { return false; }) - .catch(err => { return true; }); + const rateLimit = await this.rateLimiterService.limit(limit, limitActor, factor); + return rateLimit.blocked; } @bindThis diff --git a/packages/backend/test/unit/SigninWithPasskeyApiService.ts b/packages/backend/test/unit/SigninWithPasskeyApiService.ts index 4d73ba5af1..f0630f2d2b 100644 --- a/packages/backend/test/unit/SigninWithPasskeyApiService.ts +++ b/packages/backend/test/unit/SigninWithPasskeyApiService.ts @@ -17,16 +17,23 @@ import { GlobalModule } from '@/GlobalModule.js'; import { DI } from '@/di-symbols.js'; import { CoreModule } from '@/core/CoreModule.js'; import { SigninWithPasskeyApiService } from '@/server/api/SigninWithPasskeyApiService.js'; -import { RateLimiterService } from '@/server/api/RateLimiterService.js'; import { WebAuthnService } from '@/core/WebAuthnService.js'; import { SigninService } from '@/server/api/SigninService.js'; import { IdentifiableError } from '@/misc/identifiable-error.js'; +import { LimitInfo, SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; const moduleMocker = new ModuleMocker(global); class FakeLimiter { - public async limit() { - return; + public async limit(): Promise { + return { + blocked: false, + remaining: Number.MAX_SAFE_INTEGER, + resetMs: 0, + resetSec: 0, + fullResetMs: 0, + fullResetSec: 0, + }; } } @@ -90,7 +97,7 @@ describe('SigninWithPasskeyApiService', () => { imports: [GlobalModule, CoreModule], providers: [ SigninWithPasskeyApiService, - { provide: RateLimiterService, useClass: FakeLimiter }, + { provide: SkRateLimiterService, useClass: FakeLimiter }, { provide: SigninService, useClass: FakeSigninService }, ], }).useMocker((token) => { -- cgit v1.2.3-freya From fc5399a67d64df4098b8cc4a6b13e2834b424e0d Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sun, 8 Dec 2024 07:47:52 -0500 Subject: revert un-needed changes to RateLimiterService --- packages/backend/src/server/api/RateLimiterService.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'packages/backend/src/server/api/RateLimiterService.ts') diff --git a/packages/backend/src/server/api/RateLimiterService.ts b/packages/backend/src/server/api/RateLimiterService.ts index 6037f9bf92..305dcc1d6d 100644 --- a/packages/backend/src/server/api/RateLimiterService.ts +++ b/packages/backend/src/server/api/RateLimiterService.ts @@ -10,29 +10,29 @@ import { DI } from '@/di-symbols.js'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; import { bindThis } from '@/decorators.js'; -import type { LimitInfo } from '@/server/api/SkRateLimiterService.js'; -import { EnvService } from '@/core/EnvService.js'; import type { IEndpointMeta } from './endpoints.js'; /** @deprecated Use SkRateLimiterService instead */ @Injectable() export class RateLimiterService { - protected readonly logger: Logger; - protected readonly disabled: boolean; + private logger: Logger; + private disabled = false; constructor( @Inject(DI.redis) - protected readonly redisClient: Redis.Redis, + private redisClient: Redis.Redis, private loggerService: LoggerService, - envService: EnvService, ) { this.logger = this.loggerService.getLogger('limiter'); - this.disabled = envService.env.NODE_ENV !== 'production'; + + if (process.env.NODE_ENV !== 'production') { + this.disabled = true; + } } @bindThis - public limit(limitation: IEndpointMeta['limit'] & { key: NonNullable }, actor: string, factor = 1): Promise { + public limit(limitation: IEndpointMeta['limit'] & { key: NonNullable }, actor: string, factor = 1) { return new Promise((ok, reject) => { if (this.disabled) ok(); -- cgit v1.2.3-freya From 2946f85592022f5aaa490d3f40d6e3068957d33f Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sun, 8 Dec 2024 13:07:55 -0500 Subject: fix type errors from new rate limit definitions --- packages/backend/src/misc/rate-limit-utils.ts | 5 +++-- packages/backend/src/server/FileServerService.ts | 8 ++++---- packages/backend/src/server/api/ApiCallService.ts | 4 ++-- packages/backend/src/server/api/RateLimiterService.ts | 3 ++- .../backend/src/server/api/SkRateLimiterService.ts | 16 ++++++++-------- packages/backend/src/server/api/endpoints.ts | 2 +- .../test/unit/server/api/SkRateLimiterServiceTests.ts | 18 +++++------------- 7 files changed, 25 insertions(+), 31 deletions(-) (limited to 'packages/backend/src/server/api/RateLimiterService.ts') diff --git a/packages/backend/src/misc/rate-limit-utils.ts b/packages/backend/src/misc/rate-limit-utils.ts index 6336f29cb7..9909bb97fa 100644 --- a/packages/backend/src/misc/rate-limit-utils.ts +++ b/packages/backend/src/misc/rate-limit-utils.ts @@ -6,6 +6,7 @@ import { FastifyReply } from 'fastify'; export type RateLimit = BucketRateLimit | LegacyRateLimit; +export type Keyed = T & { key: string }; /** * Rate limit based on "leaky bucket" logic. @@ -16,7 +17,7 @@ export interface BucketRateLimit { /** * Unique key identifying the particular resource (or resource group) being limited. */ - key: string; + key?: string; /** * Constant value identifying the type of rate limit. @@ -50,7 +51,7 @@ export interface LegacyRateLimit { /** * Unique key identifying the particular resource (or resource group) being limited. */ - key: string; + key?: string; /** * Constant value identifying the type of rate limit. diff --git a/packages/backend/src/server/FileServerService.ts b/packages/backend/src/server/FileServerService.ts index 3a03cd8c00..5293d529ad 100644 --- a/packages/backend/src/server/FileServerService.ts +++ b/packages/backend/src/server/FileServerService.ts @@ -32,7 +32,7 @@ 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 { RateLimit, sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; +import { Keyed, RateLimit, sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions } from 'fastify'; const _filename = fileURLToPath(import.meta.url); @@ -633,7 +633,7 @@ export class FileServerService { } private async checkResourceLimit(reply: FastifyReply, actor: string, group: string, resource: string, factor = 1): Promise { - const limit: RateLimit = { + const limit: Keyed = { // Group by resource key: `${group}${resource}`, type: 'bucket', @@ -647,7 +647,7 @@ export class FileServerService { } private async checkSharedLimit(reply: FastifyReply, actor: string, group: string, factor = 1): Promise { - const limit: RateLimit = { + const limit: Keyed = { key: group, type: 'bucket', @@ -658,7 +658,7 @@ export class FileServerService { return await this.checkLimit(reply, actor, limit, factor); } - private async checkLimit(reply: FastifyReply, actor: string, limit: RateLimit, factor = 1): Promise { + private async checkLimit(reply: FastifyReply, actor: string, limit: Keyed, factor = 1): Promise { const info = await this.rateLimiterService.limit(limit, actor, factor); sendRateLimitHeaders(reply, info); diff --git a/packages/backend/src/server/api/ApiCallService.ts b/packages/backend/src/server/api/ApiCallService.ts index 6ad4bc8cb5..c6c33f7303 100644 --- a/packages/backend/src/server/api/ApiCallService.ts +++ b/packages/backend/src/server/api/ApiCallService.ts @@ -18,7 +18,7 @@ import { createTemp } from '@/misc/create-temp.js'; import { bindThis } from '@/decorators.js'; import { RoleService } from '@/core/RoleService.js'; import type { Config } from '@/config.js'; -import { RateLimit, sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; +import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import { SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { ApiError } from './error.js'; import { ApiLoggerService } from './ApiLoggerService.js'; @@ -327,7 +327,7 @@ export class ApiCallService implements OnApplicationShutdown { const limit = { key: ep.name, ...endpointLimit, - } as RateLimit; + }; // Rate limit const info = await this.rateLimiterService.limit(limit, limitActor, factor); diff --git a/packages/backend/src/server/api/RateLimiterService.ts b/packages/backend/src/server/api/RateLimiterService.ts index 305dcc1d6d..879529090f 100644 --- a/packages/backend/src/server/api/RateLimiterService.ts +++ b/packages/backend/src/server/api/RateLimiterService.ts @@ -10,6 +10,7 @@ import { DI } from '@/di-symbols.js'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; import { bindThis } from '@/decorators.js'; +import { LegacyRateLimit } from '@/misc/rate-limit-utils.js'; import type { IEndpointMeta } from './endpoints.js'; /** @deprecated Use SkRateLimiterService instead */ @@ -32,7 +33,7 @@ export class RateLimiterService { } @bindThis - public limit(limitation: IEndpointMeta['limit'] & { key: NonNullable }, actor: string, factor = 1) { + public limit(limitation: LegacyRateLimit & { key: NonNullable }, actor: string, factor = 1) { return new Promise((ok, reject) => { if (this.disabled) ok(); diff --git a/packages/backend/src/server/api/SkRateLimiterService.ts b/packages/backend/src/server/api/SkRateLimiterService.ts index 1aee8aa799..6415ee905c 100644 --- a/packages/backend/src/server/api/SkRateLimiterService.ts +++ b/packages/backend/src/server/api/SkRateLimiterService.ts @@ -10,7 +10,7 @@ import { TimeService } from '@/core/TimeService.js'; import { EnvService } from '@/core/EnvService.js'; import { DI } from '@/di-symbols.js'; import type Logger from '@/logger.js'; -import { BucketRateLimit, LegacyRateLimit, LimitInfo, RateLimit, hasMinLimit, isLegacyRateLimit } from '@/misc/rate-limit-utils.js'; +import { BucketRateLimit, LegacyRateLimit, LimitInfo, RateLimit, hasMinLimit, isLegacyRateLimit, Keyed } from '@/misc/rate-limit-utils.js'; @Injectable() export class SkRateLimiterService { @@ -34,7 +34,7 @@ export class SkRateLimiterService { this.disabled = envService.env.NODE_ENV !== 'production'; // TODO disable in TEST *only* } - public async limit(limit: RateLimit, actor: string, factor = 1): Promise { + public async limit(limit: Keyed, actor: string, factor = 1): Promise { if (this.disabled || factor === 0) { return { blocked: false, @@ -57,7 +57,7 @@ export class SkRateLimiterService { } } - private async limitLegacy(limit: LegacyRateLimit, actor: string, factor: number): Promise { + private async limitLegacy(limit: Keyed, actor: string, factor: number): Promise { const promises: Promise[] = []; // The "min" limit - if present - is handled directly. @@ -90,7 +90,7 @@ export class SkRateLimiterService { }; } - private async limitMin(limit: LegacyRateLimit & { minInterval: number }, actor: string, factor: number): Promise { + private async limitMin(limit: Keyed & { minInterval: number }, actor: string, factor: number): Promise { if (limit.minInterval === 0) return null; if (limit.minInterval < 0) throw new Error(`Invalid rate limit ${limit.key}: minInterval is negative (${limit.minInterval})`); @@ -126,7 +126,7 @@ export class SkRateLimiterService { return limitInfo; } - private async limitBucket(limit: BucketRateLimit, actor: string, factor: number): Promise { + private async limitBucket(limit: Keyed, actor: string, factor: number): Promise { if (limit.size < 1) throw new Error(`Invalid rate limit ${limit.key}: size is less than 1 (${limit.size})`); if (limit.dripRate != null && limit.dripRate < 1) throw new Error(`Invalid rate limit ${limit.key}: dripRate is less than 1 (${limit.dripRate})`); if (limit.dripSize != null && limit.dripSize < 1) throw new Error(`Invalid rate limit ${limit.key}: dripSize is less than 1 (${limit.dripSize})`); @@ -166,7 +166,7 @@ export class SkRateLimiterService { return limitInfo; } - private async getLimitCounter(limit: RateLimit, actor: string, subject: string): Promise { + private async getLimitCounter(limit: Keyed, actor: string, subject: string): Promise { const key = createLimitKey(limit, actor, subject); const value = await this.redisClient.get(key); @@ -177,7 +177,7 @@ export class SkRateLimiterService { return JSON.parse(value); } - private async setLimitCounter(limit: RateLimit, actor: string, counter: LimitCounter, expiration: number, subject: string): Promise { + private async setLimitCounter(limit: Keyed, actor: string, counter: LimitCounter, expiration: number, subject: string): Promise { const key = createLimitKey(limit, actor, subject); const value = JSON.stringify(counter); const expirationSec = Math.max(expiration, 1); @@ -185,7 +185,7 @@ export class SkRateLimiterService { } } -function createLimitKey(limit: RateLimit, actor: string, subject: string): string { +function createLimitKey(limit: Keyed, actor: string, subject: string): string { return `rl_${actor}_${limit.key}_${subject}`; } diff --git a/packages/backend/src/server/api/endpoints.ts b/packages/backend/src/server/api/endpoints.ts index a4193a64ec..7eb18fbfe2 100644 --- a/packages/backend/src/server/api/endpoints.ts +++ b/packages/backend/src/server/api/endpoints.ts @@ -856,7 +856,7 @@ interface IEndpointMetaBase { * エンドポイントのリミテーションに関するやつ * 省略した場合はリミテーションは無いものとして解釈されます。 */ - readonly limit?: Readonly>; + readonly limit?: Readonly; /** * ファイルの添付を必要とするか否か diff --git a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts index 910a3e5582..dbf7795fc6 100644 --- a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts +++ b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts @@ -8,7 +8,7 @@ import { jest } from '@jest/globals'; import type Redis from 'ioredis'; import { LimitCounter, SkRateLimiterService } from '@/server/api/SkRateLimiterService.js'; import { LoggerService } from '@/core/LoggerService.js'; -import { BucketRateLimit, LegacyRateLimit } from '@/misc/rate-limit-utils.js'; +import { BucketRateLimit, Keyed, LegacyRateLimit } from '@/misc/rate-limit-utils.js'; /* eslint-disable @typescript-eslint/no-non-null-assertion */ /* eslint-disable @typescript-eslint/no-unnecessary-condition */ @@ -142,7 +142,7 @@ describe(SkRateLimiterService, () => { }); describe('with bucket limit', () => { - let limit: BucketRateLimit = null!; + let limit: Keyed = null!; beforeEach(() => { limit = { @@ -387,7 +387,7 @@ describe(SkRateLimiterService, () => { }); describe('with min interval', () => { - let limit: MutableLegacyRateLimit = null!; + let limit: Keyed = null!; beforeEach(() => { limit = { @@ -570,7 +570,7 @@ describe(SkRateLimiterService, () => { }); describe('with legacy limit', () => { - let limit: MutableLegacyRateLimit = null!; + let limit: Keyed = null!; beforeEach(() => { limit = { @@ -726,7 +726,7 @@ describe(SkRateLimiterService, () => { }); describe('with legacy limit and min interval', () => { - let limit: MutableLegacyRateLimit = null!; + let limit: Keyed = null!; beforeEach(() => { limit = { @@ -855,11 +855,3 @@ describe(SkRateLimiterService, () => { }); }); }); - -// The same thing, but mutable -interface MutableLegacyRateLimit extends LegacyRateLimit { - key: string; - duration?: number; - max?: number; - minInterval?: number; -} -- cgit v1.2.3-freya