From 3561b79d6510e52234e3356de68862851b32ba86 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Sat, 13 Jan 2024 10:55:30 +0530 Subject: [PATCH] Zod schemas for routes under `/plugins` (#6378) * fix: schema for slack routes * fix: slack.post * fix: email --- plugins/email/server/auth/email.test.ts | 9 ++ plugins/email/server/auth/email.ts | 95 ++++++++++--------- plugins/email/server/auth/schema.ts | 21 ++++ plugins/slack/server/auth/schema.ts | 31 ++++++ plugins/slack/server/auth/slack.test.ts | 39 ++++++++ plugins/slack/server/auth/slack.ts | 64 +++++++------ server/models/helpers/AuthenticationHelper.ts | 2 +- 7 files changed, 185 insertions(+), 76 deletions(-) create mode 100644 plugins/email/server/auth/schema.ts create mode 100644 plugins/slack/server/auth/schema.ts create mode 100644 plugins/slack/server/auth/slack.test.ts diff --git a/plugins/email/server/auth/email.test.ts b/plugins/email/server/auth/email.test.ts index 19e06bd084b1..bc3fc4d3629f 100644 --- a/plugins/email/server/auth/email.test.ts +++ b/plugins/email/server/auth/email.test.ts @@ -8,6 +8,15 @@ import { getTestServer } from "@server/test/support"; const server = getTestServer(); describe("email", () => { + it("should fail with status 400 bad request if email is invalid", async () => { + const res = await server.post("/auth/email", { + body: { email: "invalid" }, + }); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("email: Invalid email"); + }); + it("should require email param", async () => { const res = await server.post("/auth/email", { body: {}, diff --git a/plugins/email/server/auth/email.ts b/plugins/email/server/auth/email.ts index a67fddbfcfdd..6e61545f2ee0 100644 --- a/plugins/email/server/auth/email.ts +++ b/plugins/email/server/auth/email.ts @@ -1,5 +1,5 @@ import Router from "koa-router"; -import { Client, NotificationEventType } from "@shared/types"; +import { NotificationEventType } from "@shared/types"; import { parseDomain } from "@shared/utils/domains"; import InviteAcceptedEmail from "@server/emails/templates/InviteAcceptedEmail"; import SigninEmail from "@server/emails/templates/SigninEmail"; @@ -7,20 +7,22 @@ import WelcomeEmail from "@server/emails/templates/WelcomeEmail"; import env from "@server/env"; import { AuthorizationError } from "@server/errors"; import { rateLimiter } from "@server/middlewares/rateLimiter"; +import validate from "@server/middlewares/validate"; import { User, Team } from "@server/models"; +import { APIContext } from "@server/types"; import { RateLimiterStrategy } from "@server/utils/RateLimiter"; import { signIn } from "@server/utils/authentication"; import { getUserForEmailSigninToken } from "@server/utils/jwt"; -import { assertEmail, assertPresent } from "@server/validation"; +import * as T from "./schema"; const router = new Router(); router.post( "email", rateLimiter(RateLimiterStrategy.TenPerHour), - async (ctx) => { - const { email, client } = ctx.request.body; - assertEmail(email, "email is required"); + validate(T.EmailSchema), + async (ctx: APIContext) => { + const { email, client } = ctx.input.body; const domain = parseDomain(ctx.request.hostname); @@ -71,7 +73,7 @@ router.post( to: user.email, token: user.getEmailSigninToken(), teamUrl: team.url, - client: client === Client.Desktop ? Client.Desktop : Client.Web, + client, }).schedule(); user.lastSigninEmailSentAt = new Date(); @@ -84,52 +86,57 @@ router.post( } ); -router.get("email.callback", async (ctx) => { - const { token, client } = ctx.request.query; - assertPresent(token, "token is required"); +router.get( + "email.callback", + validate(T.EmailCallbackSchema), + async (ctx: APIContext) => { + const { token, client } = ctx.input.query; - let user!: User; + let user!: User; - try { - user = await getUserForEmailSigninToken(token as string); - } catch (err) { - ctx.redirect(`/?notice=expired-token`); - return; - } - - if (!user.team.emailSigninEnabled) { - return ctx.redirect("/?notice=auth-error"); - } + try { + user = await getUserForEmailSigninToken(token as string); + } catch (err) { + ctx.redirect(`/?notice=expired-token`); + return; + } - if (user.isSuspended) { - return ctx.redirect("/?notice=user-suspended"); - } + if (!user.team.emailSigninEnabled) { + return ctx.redirect("/?notice=auth-error"); + } - if (user.isInvited) { - await new WelcomeEmail({ - to: user.email, - teamUrl: user.team.url, - }).schedule(); + if (user.isSuspended) { + return ctx.redirect("/?notice=user-suspended"); + } - const inviter = await user.$get("invitedBy"); - if (inviter?.subscribedToEventType(NotificationEventType.InviteAccepted)) { - await new InviteAcceptedEmail({ - to: inviter.email, - inviterId: inviter.id, - invitedName: user.name, + if (user.isInvited) { + await new WelcomeEmail({ + to: user.email, teamUrl: user.team.url, }).schedule(); + + const inviter = await user.$get("invitedBy"); + if ( + inviter?.subscribedToEventType(NotificationEventType.InviteAccepted) + ) { + await new InviteAcceptedEmail({ + to: inviter.email, + inviterId: inviter.id, + invitedName: user.name, + teamUrl: user.team.url, + }).schedule(); + } } - } - // set cookies on response and redirect to team subdomain - await signIn(ctx, "email", { - user, - team: user.team, - isNewTeam: false, - isNewUser: false, - client: client === Client.Desktop ? Client.Desktop : Client.Web, - }); -}); + // set cookies on response and redirect to team subdomain + await signIn(ctx, "email", { + user, + team: user.team, + isNewTeam: false, + isNewUser: false, + client, + }); + } +); export default router; diff --git a/plugins/email/server/auth/schema.ts b/plugins/email/server/auth/schema.ts new file mode 100644 index 000000000000..700266ab728d --- /dev/null +++ b/plugins/email/server/auth/schema.ts @@ -0,0 +1,21 @@ +import { z } from "zod"; +import { Client } from "@shared/types"; +import { BaseSchema } from "@server/routes/api/schema"; + +export const EmailSchema = BaseSchema.extend({ + body: z.object({ + email: z.string().email(), + client: z.nativeEnum(Client).default(Client.Web), + }), +}); + +export type EmailReq = z.infer; + +export const EmailCallbackSchema = BaseSchema.extend({ + query: z.object({ + token: z.string(), + client: z.nativeEnum(Client).default(Client.Web), + }), +}); + +export type EmailCallbackReq = z.infer; diff --git a/plugins/slack/server/auth/schema.ts b/plugins/slack/server/auth/schema.ts new file mode 100644 index 000000000000..5b5223f17c3f --- /dev/null +++ b/plugins/slack/server/auth/schema.ts @@ -0,0 +1,31 @@ +import isEmpty from "lodash/isEmpty"; +import { z } from "zod"; +import { BaseSchema } from "@server/routes/api/schema"; + +export const SlackCommandsSchema = BaseSchema.extend({ + query: z + .object({ + code: z.string().nullish(), + state: z.string().uuid().nullish(), + error: z.string().nullish(), + }) + .refine((req) => !(isEmpty(req.code) && isEmpty(req.error)), { + message: "one of code or error is required", + }), +}); + +export type SlackCommandsReq = z.infer; + +export const SlackPostSchema = BaseSchema.extend({ + query: z + .object({ + code: z.string().nullish(), + state: z.string().uuid().nullish(), + error: z.string().nullish(), + }) + .refine((req) => !(isEmpty(req.code) && isEmpty(req.error)), { + message: "one of code or error is required", + }), +}); + +export type SlackPostReq = z.infer; diff --git a/plugins/slack/server/auth/slack.test.ts b/plugins/slack/server/auth/slack.test.ts new file mode 100644 index 000000000000..87da48cff512 --- /dev/null +++ b/plugins/slack/server/auth/slack.test.ts @@ -0,0 +1,39 @@ +import { getTestServer } from "@server/test/support"; + +const server = getTestServer(); + +describe("#slack.commands", () => { + it("should fail with status 400 bad request if query param state is not a uuid", async () => { + const res = await server.get("/auth/slack.commands?state=123"); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("state: Invalid uuid"); + }); + + it("should fail with status 400 bad request when both code and error are missing in query params", async () => { + const res = await server.get( + "/auth/slack.commands?state=182d14d5-0dbd-4521-ac52-25484c25c96e" + ); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("query: one of code or error is required"); + }); +}); + +describe("#slack.post", () => { + it("should fail with status 400 bad request if query param state is not a uuid", async () => { + const res = await server.get("/auth/slack.post?state=123"); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("state: Invalid uuid"); + }); + + it("should fail with status 400 bad request when both code and error are missing in query params", async () => { + const res = await server.get( + "/auth/slack.post?state=182d14d5-0dbd-4521-ac52-25484c25c96e" + ); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("query: one of code or error is required"); + }); +}); diff --git a/plugins/slack/server/auth/slack.ts b/plugins/slack/server/auth/slack.ts index d780208a9419..c80e052076b0 100644 --- a/plugins/slack/server/auth/slack.ts +++ b/plugins/slack/server/auth/slack.ts @@ -9,6 +9,7 @@ import accountProvisioner from "@server/commands/accountProvisioner"; import env from "@server/env"; import auth from "@server/middlewares/authentication"; import passportMiddleware from "@server/middlewares/passport"; +import validate from "@server/middlewares/validate"; import { IntegrationAuthentication, Collection, @@ -16,14 +17,14 @@ import { Team, User, } from "@server/models"; -import { AppContext, AuthenticationResult } from "@server/types"; +import { APIContext, AuthenticationResult } from "@server/types"; import { getClientFromContext, getTeamFromContext, StateStore, } from "@server/utils/passport"; -import { assertPresent, assertUuid } from "@server/validation"; import * as Slack from "../slack"; +import * as T from "./schema"; type SlackProfile = Profile & { team: { @@ -132,10 +133,10 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { auth({ optional: true, }), - async (ctx: AppContext) => { - const { code, state, error } = ctx.request.query; + validate(T.SlackCommandsSchema), + async (ctx: APIContext) => { + const { code, state: teamId, error } = ctx.input.query; const { user } = ctx.state.auth; - assertPresent(code || error, "code is required"); if (error) { ctx.redirect(integrationSettingsPath(`slack?error=${error}`)); @@ -146,9 +147,9 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { // access authentication for subdomains. We must forward to the appropriate // subdomain to complete the oauth flow if (!user) { - if (state) { + if (teamId) { try { - const team = await Team.findByPk(String(state), { + const team = await Team.findByPk(teamId, { rejectOnEmpty: true, }); return redirectOnClient( @@ -168,7 +169,8 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { } const endpoint = `${env.URL}/auth/slack.commands`; - const data = await Slack.oauthAccess(String(code), endpoint); + // validation middleware ensures that code is non-null at this point + const data = await Slack.oauthAccess(code!, endpoint); const authentication = await IntegrationAuthentication.create({ service: IntegrationService.Slack, userId: user.id, @@ -195,14 +197,10 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { auth({ optional: true, }), - async (ctx: AppContext) => { - const { code, error, state } = ctx.request.query; + validate(T.SlackPostSchema), + async (ctx: APIContext) => { + const { code, error, state: collectionId } = ctx.input.query; const { user } = ctx.state.auth; - assertPresent(code || error, "code is required"); - - // FIX ME! What about having zod like schema in place here? - const collectionId = state as string; - assertUuid(collectionId, "collectionId must be an uuid"); if (error) { ctx.redirect(integrationSettingsPath(`slack?error=${error}`)); @@ -213,21 +211,24 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { // access authentication for subdomains. We must forward to the // appropriate subdomain to complete the oauth flow if (!user) { - try { - const collection = await Collection.findOne({ - where: { - id: String(state), - }, - rejectOnEmpty: true, - }); - const team = await Team.findByPk(collection.teamId, { - rejectOnEmpty: true, - }); - return redirectOnClient( - ctx, - `${team.url}/auth/slack.post?${ctx.request.querystring}` - ); - } catch (err) { + if (collectionId) { + try { + const collection = await Collection.findByPk(collectionId, { + rejectOnEmpty: true, + }); + const team = await Team.findByPk(collection.teamId, { + rejectOnEmpty: true, + }); + return redirectOnClient( + ctx, + `${team.url}/auth/slack.post?${ctx.request.querystring}` + ); + } catch (err) { + return ctx.redirect( + integrationSettingsPath(`slack?error=unauthenticated`) + ); + } + } else { return ctx.redirect( integrationSettingsPath(`slack?error=unauthenticated`) ); @@ -235,7 +236,8 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { } const endpoint = `${env.URL}/auth/slack.post`; - const data = await Slack.oauthAccess(code as string, endpoint); + // validation middleware ensures that code is non-null at this point + const data = await Slack.oauthAccess(code!, endpoint); const authentication = await IntegrationAuthentication.create({ service: IntegrationService.Slack, userId: user.id, diff --git a/server/models/helpers/AuthenticationHelper.ts b/server/models/helpers/AuthenticationHelper.ts index f99f2789ab57..34b831e01a7a 100644 --- a/server/models/helpers/AuthenticationHelper.ts +++ b/server/models/helpers/AuthenticationHelper.ts @@ -32,7 +32,7 @@ export default class AuthenticationHelper { const rootDir = env.ENVIRONMENT === "test" ? "" : "build"; glob - .sync(path.join(rootDir, "plugins/*/server/auth/!(*.test).[jt]s")) + .sync(path.join(rootDir, "plugins/*/server/auth/!(*.test|schema).[jt]s")) .forEach((filePath: string) => { const { default: authProvider, name } = require(path.join( process.cwd(),