From bb23c80004f156e3f117255d6b662fb38e5af561 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 19 Nov 2019 14:20:36 -0700 Subject: [PATCH] feat: generalized rate limiting for graphql (#2709) --- src/core/common/constants.ts | 6 -- .../server/graph/common/directives/auth.ts | 44 ++---------- .../server/graph/common/directives/helpers.ts | 35 ++++++++++ .../server/graph/tenant/directives/rate.ts | 69 +++++++++++++++++++ src/core/server/graph/tenant/schema/index.ts | 3 +- .../server/graph/tenant/schema/schema.graphql | 38 ++++++---- src/core/server/services/comments/comments.ts | 9 +-- .../comments/pipeline/phases/index.ts | 2 - .../comments/pipeline/phases/userRateLimit.ts | 59 ---------------- src/core/server/services/users/users.ts | 68 ------------------ 10 files changed, 139 insertions(+), 194 deletions(-) create mode 100644 src/core/server/graph/common/directives/helpers.ts create mode 100644 src/core/server/graph/tenant/directives/rate.ts delete mode 100644 src/core/server/services/comments/pipeline/phases/userRateLimit.ts diff --git a/src/core/common/constants.ts b/src/core/common/constants.ts index 6b2e6246b..0f9dc3df9 100644 --- a/src/core/common/constants.ts +++ b/src/core/common/constants.ts @@ -53,12 +53,6 @@ export const ALLOWED_USERNAME_CHANGE_FREQUENCY = 14 * 86400; */ export const SCHEDULED_DELETION_TIMESPAN_DAYS = 14; -/** - * COMMENT_LIMIT_WINDOW_SECONDS is the number of seconds that a user has to - * wait in-between writing comments. - */ -export const COMMENT_LIMIT_WINDOW_SECONDS = 3; - /** * DEFAULT_SESSION_LENTTH is the length of time in seconds a session is valid for unless configured in tenant. */ diff --git a/src/core/server/graph/common/directives/auth.ts b/src/core/server/graph/common/directives/auth.ts index 1876827ec..b5727f04a 100644 --- a/src/core/server/graph/common/directives/auth.ts +++ b/src/core/server/graph/common/directives/auth.ts @@ -7,16 +7,18 @@ import { UserSuspended, } from "coral-server/errors"; import CommonContext from "coral-server/graph/common/context"; -import { - GQLUSER_AUTH_CONDITIONS, - GQLUSER_ROLE, -} from "coral-server/graph/tenant/schema/__generated__/types"; import { consolidateUserStatus, consolidateUserSuspensionStatus, User, } from "coral-server/models/user"; -import { GraphQLResolveInfo, ResponsePath } from "graphql"; + +import { + GQLUSER_AUTH_CONDITIONS, + GQLUSER_ROLE, +} from "coral-server/graph/tenant/schema/__generated__/types"; + +import { calculateLocationKey } from "./helpers"; // Replace `memoize.Cache`. memoize.Cache = WeakMap; @@ -58,38 +60,6 @@ function calculateAuthConditions( return conditions.sort(); } -/** - * calculateLocationKey will reduce the resolve information to determine the - * path to where the key that is being accessed. - * - * @param info the info from the graph request - */ -function calculateLocationKey(info: Pick): string { - // Guard against invalid input. - if (!info || !info.path || !info.path.key) { - return ""; - } - - // Grab the first part of the path. - const parts: string[] = [info.path.key.toString()]; - - // Grab the parent previous part of the path. - let prev: ResponsePath | undefined = info.path.prev; - - // While there is still a previous part of the path, keep looping to find the - // all the parts. - while (prev && prev.key) { - // Push the key into the front of the array. - parts.unshift(prev.key.toString()); - - // Change the selection to the previous path element. - prev = prev.prev; - } - - // Join it together with a dotted path. - return parts.join("."); -} - const calculateAuthConditionsMemoized = memoize(calculateAuthConditions); const auth: DirectiveResolverFn< diff --git a/src/core/server/graph/common/directives/helpers.ts b/src/core/server/graph/common/directives/helpers.ts new file mode 100644 index 000000000..2c8ee3ea8 --- /dev/null +++ b/src/core/server/graph/common/directives/helpers.ts @@ -0,0 +1,35 @@ +import { GraphQLResolveInfo, ResponsePath } from "graphql"; + +/** + * calculateLocationKey will reduce the resolve information to determine the + * path to where the key that is being accessed. + * + * @param info the info from the graph request + */ +export function calculateLocationKey( + info: Pick +): string { + // Guard against invalid input. + if (!info || !info.path || !info.path.key) { + return ""; + } + + // Grab the first part of the path. + const parts: string[] = [info.path.key.toString()]; + + // Grab the parent previous part of the path. + let prev: ResponsePath | undefined = info.path.prev; + + // While there is still a previous part of the path, keep looping to find the + // all the parts. + while (prev && prev.key) { + // Push the key into the front of the array. + parts.unshift(prev.key.toString()); + + // Change the selection to the previous path element. + prev = prev.prev; + } + + // Join it together with a dotted path. + return parts.join("."); +} diff --git a/src/core/server/graph/tenant/directives/rate.ts b/src/core/server/graph/tenant/directives/rate.ts new file mode 100644 index 000000000..e2e95e779 --- /dev/null +++ b/src/core/server/graph/tenant/directives/rate.ts @@ -0,0 +1,69 @@ +import { DirectiveResolverFn } from "graphql-tools"; +import { DateTime } from "luxon"; + +import { RateLimitExceeded } from "coral-server/errors"; +import { calculateLocationKey } from "coral-server/graph/common/directives/helpers"; + +import TenantContext from "../context"; + +export interface RateDirectiveArgs { + max?: number; + seconds?: number; + key?: string; +} + +const rate: DirectiveResolverFn< + Record, + TenantContext +> = async ( + next, + src, + { max = 1, seconds, key: forceResource }: RateDirectiveArgs, + { user, tenant, now, redis, config }, + info +) => { + // If we're in development mode and rate limiters are disabled, then just + // continue anyways now. + if ( + config.get("env") === "development" && + config.get("disable_rate_limiters") + ) { + return next(); + } + + // Check if the rate limiting makes sense. + if (!seconds) { + return next(); + } + + // Current implementations do not handle anonymous requests. + if (!user) { + // TODO: (wyattjoh) handle anonymous requests + return next(); + } + + // Compute the resource key for this element. + const resource = forceResource || calculateLocationKey(info); + + // TODO: (wyattjoh) depending on `resource`, maybe override (max, seconds) + + // Calculate the storage key from the resource and user identifiers. + const key = `${tenant.id}:rl:${user.id}:${info.operation.operation}.${resource}`; + + // Perform the rate limiting check. + const [[, tries]] = await redis + .multi() + .incr(key) + .expire(key, seconds) + .exec(); + if (tries && tries > max) { + const resetsAt = DateTime.fromJSDate(now) + .plus({ seconds }) + .toJSDate(); + throw new RateLimitExceeded(key, max, resetsAt, tries); + } + + return next(); +}; + +export default rate; diff --git a/src/core/server/graph/tenant/schema/index.ts b/src/core/server/graph/tenant/schema/index.ts index 923b1745b..df717134a 100644 --- a/src/core/server/graph/tenant/schema/index.ts +++ b/src/core/server/graph/tenant/schema/index.ts @@ -7,13 +7,14 @@ import { import { loadSchema } from "coral-common/graphql"; import auth from "coral-server/graph/common/directives/auth"; import constraint from "coral-server/graph/common/directives/constraint"; +import rate from "coral-server/graph/tenant/directives/rate"; import resolvers from "coral-server/graph/tenant/resolvers"; export default function getTenantSchema() { const schema = loadSchema("tenant", resolvers as IResolvers); // Attach the directive resolvers. - attachDirectiveResolvers(schema, { auth }); + attachDirectiveResolvers(schema, { auth, rate }); // Attach the constraint directive. SchemaDirectiveVisitor.visitSchemaDirectives(schema, { diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 939a10613..ce4be9a75 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -60,6 +60,11 @@ arguments to parameters passed in to operations. """ directive @constraint(min: Int, max: Int) on ARGUMENT_DEFINITION +""" +rate enforces a rate limit on requests made by the user. +""" +directive @rate(max: Int = 1, seconds: Int!, key: String) on FIELD_DEFINITION + ################################################################################ ## Custom Scalar Types ################################################################################ @@ -5119,6 +5124,7 @@ type RequestUserCommentsDownloadPayload { """ archiveURL: String! } + ################## ## Mutation ################## @@ -5127,7 +5133,9 @@ type Mutation { """ createComment will create a Comment as the current logged in User. """ - createComment(input: CreateCommentInput!): CreateCommentPayload! @auth + createComment(input: CreateCommentInput!): CreateCommentPayload! + @auth + @rate(seconds: 3, key: "createComment") """ createCommentReply will create a Comment as the current logged in User that is @@ -5135,7 +5143,7 @@ type Mutation { """ createCommentReply( input: CreateCommentReplyInput! - ): CreateCommentReplyPayload! @auth + ): CreateCommentReplyPayload! @auth @rate(seconds: 3, key: "createComment") """ editComment will allow the author of a comment to change the body within the @@ -5162,7 +5170,7 @@ type Mutation { """ createCommentReaction( input: CreateCommentReactionInput! - ): CreateCommentReactionPayload @auth + ): CreateCommentReactionPayload @auth @rate(max: 2, seconds: 1) """ removeCommentReaction will remove a Reaction authored by the current logged in @@ -5170,7 +5178,7 @@ type Mutation { """ removeCommentReaction( input: RemoveCommentReactionInput! - ): RemoveCommentReactionPayload @auth + ): RemoveCommentReactionPayload @auth @rate(max: 2, seconds: 1) """ createCommentDontAgree will create a DontAgree authored by the current logged in @@ -5178,7 +5186,7 @@ type Mutation { """ createCommentDontAgree( input: CreateCommentDontAgreeInput! - ): CreateCommentDontAgreePayload @auth + ): CreateCommentDontAgreePayload @auth @rate(seconds: 3) """ removeCommentDontAgree will remove a DontAgree authored by the current logged in @@ -5186,7 +5194,7 @@ type Mutation { """ removeCommentDontAgree( input: RemoveCommentDontAgreeInput! - ): RemoveCommentDontAgreePayload @auth + ): RemoveCommentDontAgreePayload @auth @rate(seconds: 3) """ createCommentFlag will create a Flag authored by the current logged in User on @@ -5194,6 +5202,7 @@ type Mutation { """ createCommentFlag(input: CreateCommentFlagInput!): CreateCommentFlagPayload! @auth + @rate(seconds: 3) """ featureComment will mark a given Comment as featured. @@ -5286,11 +5295,11 @@ type Mutation { ) """ - updateUsername will set the username on the current User if they have not set one - before. This mutation will fail if the username is already set. + updateUsername will update the users username. """ updateUsername(input: UpdateUsernameInput!): UpdateUsernamePayload! @auth(permit: [SUSPENDED, BANNED, PENDING_DELETION]) + @rate(seconds: 10) """ setEmail will set the email address on the current User if they have not set @@ -5311,7 +5320,9 @@ type Mutation { updatePassword allows the current logged in User to change their password if they already have one associated with them. """ - updatePassword(input: UpdatePasswordInput!): UpdatePasswordPayload! @auth + updatePassword(input: UpdatePasswordInput!): UpdatePasswordPayload! + @auth + @rate(seconds: 10) """ requestAccountDeletion allows the current logged in User to request to @@ -5319,7 +5330,7 @@ type Mutation { """ requestAccountDeletion( input: RequestAccountDeletionInput! - ): RequestAccountDeletionPayload! @auth + ): RequestAccountDeletionPayload! @auth @rate(seconds: 10) """ deleteUserAccount will delete the target user now. @@ -5359,10 +5370,11 @@ type Mutation { ): UpdateUserUsernamePayload! @auth(roles: [ADMIN]) """ - updateEmail allows administrators to update a given User's email address - to the one provided. + updateEmail will update the current users email address. """ - updateEmail(input: UpdateEmailInput!): UpdateEmailPayload! @auth + updateEmail(input: UpdateEmailInput!): UpdateEmailPayload! + @auth + @rate(seconds: 10) """ updateNotificationSettings can be used to update the notification settings for diff --git a/src/core/server/services/comments/comments.ts b/src/core/server/services/comments/comments.ts index 6b7ed3643..b46452e49 100644 --- a/src/core/server/services/comments/comments.ts +++ b/src/core/server/services/comments/comments.ts @@ -49,10 +49,7 @@ import { import { AugmentedRedis } from "coral-server/services/redis"; import { Request } from "coral-server/types/express"; -import { - updateUserLastCommentID, - updateUserLastWroteCommentTimestamp, -} from "../users"; +import { updateUserLastCommentID } from "../users"; import { addCommentActions, CreateAction } from "./actions"; import { calculateCounts, calculateCountsDiff } from "./moderation/counts"; import { PhaseResult, processForModeration } from "./pipeline"; @@ -170,10 +167,6 @@ export async function create( actionCounts = encodeActionCounts(...deDuplicatedActions); } - // Create the comment action in our rate limiter. This will throw an error if - // there is a rate limit error. - await updateUserLastWroteCommentTimestamp(redis, tenant, author, now); - // Create the comment! const comment = await createComment( mongo, diff --git a/src/core/server/services/comments/pipeline/phases/index.ts b/src/core/server/services/comments/pipeline/phases/index.ts index 0895cb926..baf14c50f 100644 --- a/src/core/server/services/comments/pipeline/phases/index.ts +++ b/src/core/server/services/comments/pipeline/phases/index.ts @@ -13,14 +13,12 @@ import { spam } from "./spam"; import { staff } from "./staff"; import { storyClosed } from "./storyClosed"; import { toxic } from "./toxic"; -import { userRateLimit } from "./userRateLimit"; import { wordList } from "./wordList"; /** * The moderation phases to apply for each comment being processed. */ export const moderationPhases: IntermediateModerationPhase[] = [ - userRateLimit, commentLength, storyClosed, commentingDisabled, diff --git a/src/core/server/services/comments/pipeline/phases/userRateLimit.ts b/src/core/server/services/comments/pipeline/phases/userRateLimit.ts deleted file mode 100644 index 4ee0c497d..000000000 --- a/src/core/server/services/comments/pipeline/phases/userRateLimit.ts +++ /dev/null @@ -1,59 +0,0 @@ -import { DateTime } from "luxon"; - -import { COMMENT_LIMIT_WINDOW_SECONDS } from "coral-common/constants"; -import { RateLimitExceeded } from "coral-server/errors"; -import { - IntermediateModerationPhase, - IntermediatePhaseResult, - ModerationPhaseContext, -} from "coral-server/services/comments/pipeline"; -import { retrieveUserLastWroteCommentTimestamp } from "coral-server/services/users"; - -export const userRateLimit: IntermediateModerationPhase = async ({ - action, - author, - redis, - config, - tenant, - now, -}: Pick< - ModerationPhaseContext, - "author" | "redis" | "config" | "now" | "tenant" | "action" ->): Promise => { - // If we're in development mode and rate limiters are disabled, then just - // continue anyways now. - if ( - config.get("env") === "development" && - config.get("disable_rate_limiters") - ) { - return; - } - - // If this is an edit, we don't need to process this again here. - if (action === "EDIT") { - return; - } - - // Check when the last comment was written by this user. - const timestamp = await retrieveUserLastWroteCommentTimestamp( - redis, - tenant, - author - ); - if (!timestamp) { - // There is no timestamp written for this user, they are definitely allowed - // to write a comment! - return; - } - - // Check to see if this timestamp is still within the limit window. If it is, - // reject the comment. - const nextEditTime = DateTime.fromJSDate(timestamp) - .plus({ seconds: COMMENT_LIMIT_WINDOW_SECONDS }) - .toJSDate(); - if (nextEditTime > now) { - throw new RateLimitExceeded("createComment", 1, nextEditTime); - } - - return; -}; diff --git a/src/core/server/services/users/users.ts b/src/core/server/services/users/users.ts index 14e8f0bcb..b85c865ca 100644 --- a/src/core/server/services/users/users.ts +++ b/src/core/server/services/users/users.ts @@ -3,7 +3,6 @@ import { Db } from "mongodb"; import { ALLOWED_USERNAME_CHANGE_FREQUENCY, - COMMENT_LIMIT_WINDOW_SECONDS, COMMENT_REPEAT_POST_TIMESPAN, DOWNLOAD_LIMIT_TIMEFRAME, } from "coral-common/constants"; @@ -17,7 +16,6 @@ import { LocalProfileAlreadySetError, LocalProfileNotSetError, PasswordIncorrect, - RateLimitExceeded, TokenNotFoundError, UserAlreadyBannedError, UserAlreadyPremoderated, @@ -1185,13 +1183,6 @@ export async function updateNotificationSettings( return updateUserNotificationSettings(mongo, tenant.id, user.id, settings); } -function userLastWroteCommentTimestampKey( - tenant: Pick, - user: Pick -) { - return `${tenant.id}:lastCommentTimestamp:${user.id}`; -} - function userLastCommentIDKey( tenant: Pick, user: Pick @@ -1199,65 +1190,6 @@ function userLastCommentIDKey( return `${tenant.id}:lastCommentID:${user.id}`; } -/** - * retrieveUserLastWroteCommentTimestamp will return the timestamp (if set) that - * the user last wrote a comment on. This will return null if the comment was - * written more than COMMENT_LIMIT_WINDOW_SECONDS seconds ago. - * - * @param redis the Redis instance that Coral interacts with - * @param tenant the Tenant to operate on - * @param user the User that we're looking up the limit for - */ -export async function retrieveUserLastWroteCommentTimestamp( - redis: AugmentedRedis, - tenant: Tenant, - user: User -): Promise { - // Try to get the timestamp for the author. - const timestamp: string | null = await redis.get( - userLastWroteCommentTimestampKey(tenant, user) - ); - if (!timestamp) { - return null; - } - - return DateTime.fromISO(timestamp).toJSDate(); -} - -/** - * updateUserLastWroteCommentTimestamp will update the last time that the user - * wrote a comment, and will throw an error if the rate limit was exceeded. If - * this throws an error, it means that the user has written a comment within - * COMMENT_LIMIT_WINDOW_SECONDS seconds, and should be prevented from writing - * another comment. - * - * @param redis the Redis instance that Coral interacts with - * @param tenant the Tenant to operate on - * @param user the User that we're setting the limit for - * @param when the date that the user wrote the comment - */ -export async function updateUserLastWroteCommentTimestamp( - redis: AugmentedRedis, - tenant: Tenant, - user: User, - when: Date -) { - const key = userLastWroteCommentTimestampKey(tenant, user); - - // Try to set the last wrote comment timestamp. - const [[, set]] = await redis - .multi() - .setnx(key, when.toISOString()) - .expire(key, COMMENT_LIMIT_WINDOW_SECONDS) - .exec(); - if (!set) { - const resetsAt = DateTime.fromJSDate(when) - .plus({ seconds: COMMENT_LIMIT_WINDOW_SECONDS }) - .toJSDate(); - throw new RateLimitExceeded("createComment", 1, resetsAt); - } -} - /** * updateUserLastCommentID will update the id of the users most recent comment. *