From 428b3c95539d27daf7cddb6a74da1203578c358d Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Thu, 28 Dec 2023 14:46:50 -0400 Subject: [PATCH] chore: Ensure comment data is validated before persisting (#6322) Fix flash on render of comment create --- app/stores/CommentsStore.ts | 9 +- app/stores/base/Store.ts | 25 ++++-- server/routes/api/BaseSchema.ts | 10 --- server/routes/api/apiKeys/schema.ts | 2 +- server/routes/api/attachments/schema.ts | 2 +- server/routes/api/auth/schema.ts | 2 +- .../api/authenticationProviders/schema.ts | 2 +- server/routes/api/collections/schema.ts | 2 +- .../__snapshots__/comments.test.ts.snap | 9 ++ server/routes/api/comments/comments.test.ts | 83 +++++++++++++++++++ server/routes/api/comments/schema.ts | 6 +- server/routes/api/cron/schema.ts | 2 +- server/routes/api/developer/schema.ts | 2 +- server/routes/api/documents/schema.ts | 2 +- server/routes/api/events/schema.ts | 2 +- server/routes/api/fileOperations/schema.ts | 2 +- server/routes/api/integrations/schema.ts | 2 +- server/routes/api/notifications/schema.ts | 2 +- server/routes/api/pins/schema.ts | 2 +- server/routes/api/revisions/schema.ts | 2 +- server/routes/api/schema.ts | 21 +++++ server/routes/api/searches/schema.ts | 2 +- server/routes/api/shares/schema.ts | 2 +- server/routes/api/stars/schema.ts | 2 +- server/routes/api/subscriptions/schema.ts | 2 +- server/routes/api/teams/schema.ts | 2 +- server/routes/api/urls/schema.ts | 2 +- server/routes/api/users/schema.ts | 2 +- server/routes/api/views/schema.ts | 2 +- server/types.ts | 2 +- 30 files changed, 163 insertions(+), 46 deletions(-) delete mode 100644 server/routes/api/BaseSchema.ts create mode 100644 server/routes/api/schema.ts diff --git a/app/stores/CommentsStore.ts b/app/stores/CommentsStore.ts index d1c664c1e..8f42dfb73 100644 --- a/app/stores/CommentsStore.ts +++ b/app/stores/CommentsStore.ts @@ -7,9 +7,16 @@ import Document from "~/models/Document"; import { PaginationParams } from "~/types"; import { client } from "~/utils/ApiClient"; import RootStore from "./RootStore"; -import Store from "./base/Store"; +import Store, { RPCAction } from "./base/Store"; export default class CommentsStore extends Store { + actions = [ + RPCAction.List, + RPCAction.Create, + RPCAction.Update, + RPCAction.Delete, + ]; + constructor(rootStore: RootStore) { super(rootStore, Comment); } diff --git a/app/stores/base/Store.ts b/app/stores/base/Store.ts index 059ce0d8f..b32e01eb0 100644 --- a/app/stores/base/Store.ts +++ b/app/stores/base/Store.ts @@ -157,9 +157,11 @@ export default abstract class Store { ...options, }); - invariant(res?.data, "Data should be available"); - this.addPolicies(res.policies); - return this.add(res.data); + return runInAction(`create#${this.modelName}`, () => { + invariant(res?.data, "Data should be available"); + this.addPolicies(res.policies); + return this.add(res.data); + }); } finally { this.isSaving = false; } @@ -182,9 +184,11 @@ export default abstract class Store { ...options, }); - invariant(res?.data, "Data should be available"); - this.addPolicies(res.policies); - return this.add(res.data); + return runInAction(`update#${this.modelName}`, () => { + invariant(res?.data, "Data should be available"); + this.addPolicies(res.policies); + return this.add(res.data); + }); } finally { this.isSaving = false; } @@ -229,9 +233,12 @@ export default abstract class Store { const res = await client.post(`/${this.apiEndpoint}.info`, { id, }); - invariant(res?.data, "Data should be available"); - this.addPolicies(res.policies); - return this.add(res.data); + + return runInAction(`info#${this.modelName}`, () => { + invariant(res?.data, "Data should be available"); + this.addPolicies(res.policies); + return this.add(res.data); + }); } catch (err) { if (err instanceof AuthorizationError || err instanceof NotFoundError) { this.remove(id); diff --git a/server/routes/api/BaseSchema.ts b/server/routes/api/BaseSchema.ts deleted file mode 100644 index f78137358..000000000 --- a/server/routes/api/BaseSchema.ts +++ /dev/null @@ -1,10 +0,0 @@ -import formidable from "formidable"; -import { z } from "zod"; - -const BaseSchema = z.object({ - body: z.unknown(), - query: z.unknown(), - file: z.custom().optional(), -}); - -export default BaseSchema; diff --git a/server/routes/api/apiKeys/schema.ts b/server/routes/api/apiKeys/schema.ts index 4cae67078..f17762b83 100644 --- a/server/routes/api/apiKeys/schema.ts +++ b/server/routes/api/apiKeys/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const APIKeysCreateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/attachments/schema.ts b/server/routes/api/attachments/schema.ts index 6bc3edcd5..ea206cc79 100644 --- a/server/routes/api/attachments/schema.ts +++ b/server/routes/api/attachments/schema.ts @@ -1,7 +1,7 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; import { AttachmentPreset } from "@shared/types"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const AttachmentsCreateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/auth/schema.ts b/server/routes/api/auth/schema.ts index f721f447d..11ad01ff6 100644 --- a/server/routes/api/auth/schema.ts +++ b/server/routes/api/auth/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const AuthConfigSchema = BaseSchema; diff --git a/server/routes/api/authenticationProviders/schema.ts b/server/routes/api/authenticationProviders/schema.ts index 2fd6e1169..efebdcd10 100644 --- a/server/routes/api/authenticationProviders/schema.ts +++ b/server/routes/api/authenticationProviders/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const AuthenticationProvidersInfoSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/collections/schema.ts b/server/routes/api/collections/schema.ts index 0084371ae..17df1648d 100644 --- a/server/routes/api/collections/schema.ts +++ b/server/routes/api/collections/schema.ts @@ -5,7 +5,7 @@ import { CollectionPermission, FileOperationFormat } from "@shared/types"; import { colorPalette } from "@shared/utils/collections"; import { Collection } from "@server/models"; import { ValidateColor, ValidateIcon, ValidateIndex } from "@server/validation"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const CollectionsCreateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/comments/__snapshots__/comments.test.ts.snap b/server/routes/api/comments/__snapshots__/comments.test.ts.snap index 614104c04..ca889ce85 100644 --- a/server/routes/api/comments/__snapshots__/comments.test.ts.snap +++ b/server/routes/api/comments/__snapshots__/comments.test.ts.snap @@ -1,5 +1,14 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`#comments.create should require authentication 1`] = ` +{ + "error": "authentication_required", + "message": "Authentication required", + "ok": false, + "status": 401, +} +`; + exports[`#comments.list should require authentication 1`] = ` { "error": "authentication_required", diff --git a/server/routes/api/comments/comments.test.ts b/server/routes/api/comments/comments.test.ts index 1cae8c60e..472e07081 100644 --- a/server/routes/api/comments/comments.test.ts +++ b/server/routes/api/comments/comments.test.ts @@ -16,6 +16,7 @@ describe("#comments.list", () => { expect(res.status).toEqual(401); expect(body).toMatchSnapshot(); }); + it("should return all comments for a document", async () => { const team = await buildTeam(); const user = await buildUser({ teamId: team.id }); @@ -42,6 +43,7 @@ describe("#comments.list", () => { expect(body.policies.length).toEqual(1); expect(body.policies[0].abilities.read).toEqual(true); }); + it("should return all comments for a collection", async () => { const team = await buildTeam(); const user = await buildUser({ teamId: team.id }); @@ -73,6 +75,7 @@ describe("#comments.list", () => { expect(body.policies.length).toEqual(1); expect(body.policies[0].abilities.read).toEqual(true); }); + it("should return all comments", async () => { const team = await buildTeam(); const user = await buildUser({ teamId: team.id }); @@ -121,3 +124,83 @@ describe("#comments.list", () => { expect(body.policies[1].abilities.read).toEqual(true); }); }); + +describe("#comments.create", () => { + it("should require authentication", async () => { + const res = await server.post("/api/comments.create"); + const body = await res.json(); + expect(res.status).toEqual(401); + expect(body).toMatchSnapshot(); + }); + + it("should create a comment", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const document = await buildDocument({ + userId: user.id, + teamId: user.teamId, + }); + + const comment = await buildComment({ + userId: user.id, + teamId: team.id, + documentId: document.id, + }); + + const res = await server.post("/api/comments.create", { + body: { + token: user.getJwtToken(), + documentId: document.id, + data: comment.data, + }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.data.data).toEqual(comment.data); + expect(body.policies.length).toEqual(1); + expect(body.policies[0].abilities.read).toEqual(true); + expect(body.policies[0].abilities.update).toEqual(true); + expect(body.policies[0].abilities.delete).toEqual(true); + }); + + it("should not allow empty comment data", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const document = await buildDocument({ + userId: user.id, + teamId: user.teamId, + }); + + const res = await server.post("/api/comments.create", { + body: { + token: user.getJwtToken(), + documentId: document.id, + data: null, + }, + }); + + expect(res.status).toEqual(400); + }); + + it("should not allow invalid comment data", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const document = await buildDocument({ + userId: user.id, + teamId: user.teamId, + }); + + const res = await server.post("/api/comments.create", { + body: { + token: user.getJwtToken(), + documentId: document.id, + data: { + type: "nonsense", + }, + }, + }); + + expect(res.status).toEqual(400); + }); +}); diff --git a/server/routes/api/comments/schema.ts b/server/routes/api/comments/schema.ts index f686ba45f..4407e4f97 100644 --- a/server/routes/api/comments/schema.ts +++ b/server/routes/api/comments/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema, ProsemirrorSchema } from "@server/routes/api/schema"; const CollectionsSortParamsSchema = z.object({ /** Specifies the attributes by which documents will be sorted in the list */ @@ -27,7 +27,7 @@ export const CommentsCreateSchema = BaseSchema.extend({ parentCommentId: z.string().uuid().optional(), /** Create comment with this data */ - data: z.any(), + data: ProsemirrorSchema, }), }); @@ -39,7 +39,7 @@ export const CommentsUpdateSchema = BaseSchema.extend({ id: z.string().uuid(), /** Update comment with this data */ - data: z.any(), + data: ProsemirrorSchema, }), }); diff --git a/server/routes/api/cron/schema.ts b/server/routes/api/cron/schema.ts index 7c7989359..cd742e9e5 100644 --- a/server/routes/api/cron/schema.ts +++ b/server/routes/api/cron/schema.ts @@ -1,6 +1,6 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const CronSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/developer/schema.ts b/server/routes/api/developer/schema.ts index 2dd054ceb..b19a276c7 100644 --- a/server/routes/api/developer/schema.ts +++ b/server/routes/api/developer/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const CreateTestUsersSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/documents/schema.ts b/server/routes/api/documents/schema.ts index e8280696d..647266142 100644 --- a/server/routes/api/documents/schema.ts +++ b/server/routes/api/documents/schema.ts @@ -4,7 +4,7 @@ import isEmpty from "lodash/isEmpty"; import isUUID from "validator/lib/isUUID"; import { z } from "zod"; import { SHARE_URL_SLUG_REGEX } from "@shared/utils/urlHelpers"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; const DocumentsSortParamsSchema = z.object({ /** Specifies the attributes by which documents will be sorted in the list */ diff --git a/server/routes/api/events/schema.ts b/server/routes/api/events/schema.ts index 66b39248a..04b248e60 100644 --- a/server/routes/api/events/schema.ts +++ b/server/routes/api/events/schema.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const EventsListSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/fileOperations/schema.ts b/server/routes/api/fileOperations/schema.ts index 36299df00..556019374 100644 --- a/server/routes/api/fileOperations/schema.ts +++ b/server/routes/api/fileOperations/schema.ts @@ -2,7 +2,7 @@ import isEmpty from "lodash/isEmpty"; import z from "zod"; import { FileOperationType } from "@shared/types"; import { FileOperation } from "@server/models"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; const CollectionsSortParamsSchema = z.object({ /** The attribute to sort by */ diff --git a/server/routes/api/integrations/schema.ts b/server/routes/api/integrations/schema.ts index 657194e40..2bc947186 100644 --- a/server/routes/api/integrations/schema.ts +++ b/server/routes/api/integrations/schema.ts @@ -2,7 +2,7 @@ import { z } from "zod"; import { IntegrationType } from "@shared/types"; import { Integration } from "@server/models"; import { UserCreatableIntegrationService } from "@server/models/Integration"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const IntegrationsListSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/notifications/schema.ts b/server/routes/api/notifications/schema.ts index ac0cfa636..e0a5074aa 100644 --- a/server/routes/api/notifications/schema.ts +++ b/server/routes/api/notifications/schema.ts @@ -1,7 +1,7 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; import { NotificationEventType } from "@shared/types"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const NotificationSettingsCreateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/pins/schema.ts b/server/routes/api/pins/schema.ts index 9f6808773..d3592a80f 100644 --- a/server/routes/api/pins/schema.ts +++ b/server/routes/api/pins/schema.ts @@ -1,7 +1,7 @@ import isUUID from "validator/lib/isUUID"; import { z } from "zod"; import { SLUG_URL_REGEX } from "@shared/utils/urlHelpers"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const PinsCreateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/revisions/schema.ts b/server/routes/api/revisions/schema.ts index 76b874d9a..85c88974d 100644 --- a/server/routes/api/revisions/schema.ts +++ b/server/routes/api/revisions/schema.ts @@ -1,7 +1,7 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; import { Revision } from "@server/models"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const RevisionsInfoSchema = BaseSchema.extend({ body: z diff --git a/server/routes/api/schema.ts b/server/routes/api/schema.ts new file mode 100644 index 000000000..ec9e16826 --- /dev/null +++ b/server/routes/api/schema.ts @@ -0,0 +1,21 @@ +import formidable from "formidable"; +import { Node } from "prosemirror-model"; +import { z } from "zod"; +import { ProsemirrorData as TProsemirrorData } from "@shared/types"; +import { schema } from "@server/editor"; + +export const BaseSchema = z.object({ + body: z.unknown(), + query: z.unknown(), + file: z.custom().optional(), +}); + +export const ProsemirrorSchema = z.custom((val) => { + try { + const node = Node.fromJSON(schema, val); + node.check(); + return true; + } catch { + return false; + } +}, "not valid data"); diff --git a/server/routes/api/searches/schema.ts b/server/routes/api/searches/schema.ts index 8349bd71a..7568e9425 100644 --- a/server/routes/api/searches/schema.ts +++ b/server/routes/api/searches/schema.ts @@ -1,6 +1,6 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const SearchesDeleteSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/shares/schema.ts b/server/routes/api/shares/schema.ts index f5db3a652..8ff0210fc 100644 --- a/server/routes/api/shares/schema.ts +++ b/server/routes/api/shares/schema.ts @@ -3,7 +3,7 @@ import isUUID from "validator/lib/isUUID"; import { z } from "zod"; import { SHARE_URL_SLUG_REGEX, SLUG_URL_REGEX } from "@shared/utils/urlHelpers"; import { Share } from "@server/models"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const SharesInfoSchema = BaseSchema.extend({ body: z diff --git a/server/routes/api/stars/schema.ts b/server/routes/api/stars/schema.ts index 42573081f..6fa67ae68 100644 --- a/server/routes/api/stars/schema.ts +++ b/server/routes/api/stars/schema.ts @@ -1,7 +1,7 @@ import isEmpty from "lodash/isEmpty"; import { z } from "zod"; import { ValidateDocumentId, ValidateIndex } from "@server/validation"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const StarsCreateSchema = BaseSchema.extend({ body: z diff --git a/server/routes/api/subscriptions/schema.ts b/server/routes/api/subscriptions/schema.ts index 7382365dd..d1aa7dd60 100644 --- a/server/routes/api/subscriptions/schema.ts +++ b/server/routes/api/subscriptions/schema.ts @@ -1,6 +1,6 @@ import { z } from "zod"; import { ValidateDocumentId } from "@server/validation"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const SubscriptionsListSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/teams/schema.ts b/server/routes/api/teams/schema.ts index e32392d2e..7d7e9a393 100644 --- a/server/routes/api/teams/schema.ts +++ b/server/routes/api/teams/schema.ts @@ -1,6 +1,6 @@ import { z } from "zod"; import { UserRole } from "@shared/types"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; export const TeamsUpdateSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/routes/api/urls/schema.ts b/server/routes/api/urls/schema.ts index f9ecf4866..ef2b30266 100644 --- a/server/routes/api/urls/schema.ts +++ b/server/routes/api/urls/schema.ts @@ -2,7 +2,7 @@ import isNil from "lodash/isNil"; import { z } from "zod"; import { isUrl } from "@shared/utils/urls"; import { ValidateURL } from "@server/validation"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const UrlsUnfurlSchema = BaseSchema.extend({ body: z diff --git a/server/routes/api/users/schema.ts b/server/routes/api/users/schema.ts index 290f82db4..fc3275ad1 100644 --- a/server/routes/api/users/schema.ts +++ b/server/routes/api/users/schema.ts @@ -1,7 +1,7 @@ import { z } from "zod"; import { NotificationEventType, UserPreference, UserRole } from "@shared/types"; import User from "@server/models/User"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; const BaseIdSchema = z.object({ id: z.string().uuid(), diff --git a/server/routes/api/views/schema.ts b/server/routes/api/views/schema.ts index a76a689f8..dd2ad9f38 100644 --- a/server/routes/api/views/schema.ts +++ b/server/routes/api/views/schema.ts @@ -1,5 +1,5 @@ import z from "zod"; -import BaseSchema from "../BaseSchema"; +import { BaseSchema } from "../schema"; export const ViewsListSchema = BaseSchema.extend({ body: z.object({ diff --git a/server/types.ts b/server/types.ts index b07bbd16c..d8deb1123 100644 --- a/server/types.ts +++ b/server/types.ts @@ -8,7 +8,7 @@ import { Client, CollectionPermission, } from "@shared/types"; -import BaseSchema from "@server/routes/api/BaseSchema"; +import { BaseSchema } from "@server/routes/api/schema"; import { AccountProvisionerResult } from "./commands/accountProvisioner"; import { FileOperation, Team, User } from "./models";