Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent potential email abuse on invitations endpoint #4569

Merged
merged 4 commits into from
Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions front/lib/invitations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Maxmimum allowed number of unconsumed invitations per workspace.
export const MAX_UNCONSUMED_INVITATIONS = 50;
// If the user already received an invitation from this workspace and hasn't consumed it yet, we won't send another one
// before this cooldown period.
export const UNCONSUMED_INVITATION_COOLDOWN_PER_EMAIL_MS = 1000 * 60 * 60 * 24; // 1 day
49 changes: 48 additions & 1 deletion front/pages/api/w/[wId]/invitations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { isLeft } from "fp-ts/lib/Either";
import * as t from "io-ts";
import * as reporter from "io-ts-reporters";
import type { NextApiRequest, NextApiResponse } from "next";
import { Op } from "sequelize";

import {
sendWorkspaceInvitationEmail,
Expand All @@ -15,6 +16,11 @@ import {
import { getPendingInvitations } from "@app/lib/api/invitation";
import { getMembers } from "@app/lib/api/workspace";
import { Authenticator, getSession } from "@app/lib/auth";
import {
MAX_UNCONSUMED_INVITATIONS,
UNCONSUMED_INVITATION_COOLDOWN_PER_EMAIL_MS,
} from "@app/lib/invitations";
import { MembershipInvitation } from "@app/lib/models";
import { MembershipResource } from "@app/lib/resources/membership_resource";
import { isEmailValid } from "@app/lib/utils";
import logger from "@app/logger/logger";
Expand Down Expand Up @@ -152,7 +158,48 @@ async function handler(
});
}
const existingMembers = await getMembers(auth);

const unconsumedInvitations = await MembershipInvitation.findAll({
fontanierh marked this conversation as resolved.
Show resolved Hide resolved
where: {
workspaceId: owner.id,
status: ["pending", "revoked"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we account for revoked in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah because otherwise you can just revoke and re-send. Maybe we should discard revoked ones that are too old ? TBH if you have more than a few revoked it's fishy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what happens when you have revoked 50 invitations, you can't invite anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified the whole thing. Now were just looking at invitations from the last 24 hours. IMO it's enough to limit abuse.

createdAt: {
[Op.gte]: new Date(new Date().setHours(0, 0, 0, 0)),
fontanierh marked this conversation as resolved.
Show resolved Hide resolved
},
},
});
if (unconsumedInvitations.length > MAX_UNCONSUMED_INVITATIONS) {
return apiError(req, res, {
status_code: 400,
api_error: {
type: "invalid_request_error",
message: `Too many unconsumed invitations. Please ask your members to consume their invitations before sending more.`,
},
});
}
const emailsWithRecentUnconsumedInvitations = new Set(
unconsumedInvitations
.filter(
(i) =>
i.createdAt.getTime() >
new Date().getTime() - UNCONSUMED_INVITATION_COOLDOWN_PER_EMAIL_MS
)
.map((i) => i.inviteEmail.toLowerCase().trim())
);
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming one email address already has a fresh recent invitation, the entire operation will fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, would you prefer that we only filter those out ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean as long as we have an explicit error message in the UI and the user does not lose its email addresses list, I'm fine.

invitationRequests.some((r) =>
emailsWithRecentUnconsumedInvitations.has(
r.email.toLowerCase().trim()
)
)
) {
return apiError(req, res, {
status_code: 400,
api_error: {
type: "invalid_request_error",
message: `Some of the emails have already received an invitation in the last 24 hours. Please wait before sending another invitation.`,
},
});
}
const invitationResults = await Promise.all(
invitationRequests.map(async ({ email, role }) => {
if (existingMembers.find((m) => m.email === email)) {
Expand Down
10 changes: 10 additions & 0 deletions front/pages/w/[wId]/members/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
PRO_PLAN_29_COST,
} from "@app/lib/client/subscription";
import { withDefaultUserAuthRequirements } from "@app/lib/iam/session";
import { MAX_UNCONSUMED_INVITATIONS } from "@app/lib/invitations";
import { isUpgraded, PRO_PLAN_SEAT_29_CODE } from "@app/lib/plans/plan_codes";
import { useMembers, useWorkspaceInvitations } from "@app/lib/swr";
import { classNames, isEmailValid } from "@app/lib/utils";
Expand Down Expand Up @@ -484,6 +485,15 @@ function InviteEmailModal({
async function handleSendInvitations(
inviteEmailsList: string[]
): Promise<void> {
if (inviteEmailsList.length > MAX_UNCONSUMED_INVITATIONS) {
sendNotification({
type: "error",
title: "Too many invitations",
description: `Your cannot send more than ${MAX_UNCONSUMED_INVITATIONS} invitations.`,
});
return;
}

const invitesByCase = {
activeSameRole: members.filter((m) =>
inviteEmailsList.find(
Expand Down
Loading