From 1a55d701353c7b64fc07487c136a7f8c8641578d Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Thu, 23 Nov 2023 17:02:35 +0100 Subject: [PATCH 1/5] sanitise sharedb data using dompurify --- editor.planx.uk/package.json | 2 ++ editor.planx.uk/pnpm-lock.yaml | 22 +++++++++++++++++++--- editor.planx.uk/src/@planx/graph/index.ts | 5 +++-- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/editor.planx.uk/package.json b/editor.planx.uk/package.json index 0a41be1b8c..71e3bd9d23 100644 --- a/editor.planx.uk/package.json +++ b/editor.planx.uk/package.json @@ -45,6 +45,7 @@ "classnames": "^2.3.2", "core-js": "^3.31.0", "date-fns": "^2.30.0", + "dompurify": "^3.0.6", "dotenv": "^16.3.1", "formik": "^2.4.2", "graphql": "^16.8.1", @@ -116,6 +117,7 @@ "@testing-library/jest-dom": "^5.16.5", "@testing-library/react": "^13.4.0", "@testing-library/user-event": "^14.4.3", + "@types/dompurify": "^3.0.5", "@types/draft-js": "^0.11.12", "@types/jest": "^27.5.2", "@types/jest-axe": "^3.5.5", diff --git a/editor.planx.uk/pnpm-lock.yaml b/editor.planx.uk/pnpm-lock.yaml index f51f8eb03c..7221962afb 100644 --- a/editor.planx.uk/pnpm-lock.yaml +++ b/editor.planx.uk/pnpm-lock.yaml @@ -137,6 +137,9 @@ dependencies: date-fns: specifier: ^2.30.0 version: 2.30.0 + dompurify: + specifier: ^3.0.6 + version: 3.0.6 dotenv: specifier: ^16.3.1 version: 16.3.1 @@ -346,6 +349,9 @@ devDependencies: '@testing-library/user-event': specifier: ^14.4.3 version: 14.4.3(@testing-library/dom@8.20.1) + '@types/dompurify': + specifier: ^3.0.5 + version: 3.0.5 '@types/draft-js': specifier: ^0.11.12 version: 0.11.12 @@ -4641,7 +4647,7 @@ packages: '@babel/runtime': 7.23.2 '@emotion/is-prop-valid': 1.2.1 '@mui/types': 7.2.9(@types/react@18.2.20) - '@mui/utils': 5.14.5(react@18.2.0) + '@mui/utils': 5.14.18(@types/react@18.2.20)(react@18.2.0) '@popperjs/core': 2.11.8 '@types/react': 18.2.20 clsx: 2.0.0 @@ -7333,6 +7339,12 @@ packages: resolution: {integrity: sha512-w5jZ0ee+HaPOaX25X2/2oGR/7rgAQSYII7X7pp0m9KgBfMP7uKfMfTvcpl5Dj+eDBbpxKGiqE+flqDr6XTd2RA==} dev: true + /@types/dompurify@3.0.5: + resolution: {integrity: sha512-1Wg0g3BtQF7sSb27fJQAKck1HECM6zV1EB66j8JH9i3LCjYabJa0FSdiSgsD5K/RbrsR0SiraKacLB+T8ZVYAg==} + dependencies: + '@types/trusted-types': 2.0.6 + dev: true + /@types/draft-js@0.11.12: resolution: {integrity: sha512-J/e4QYz8wCXvPpiCaiKcJrtLo65px4nnfFVZ/0EKHoKnQ4nWdzXwGHOQLIePAJM+Ho4V9/mb4Nhw4v/08y98jQ==} dependencies: @@ -9643,7 +9655,7 @@ packages: dev: false /concat-map@0.0.1: - resolution: {integrity: sha512-/Srv4dswyQNBfohGpz9o6Yb3Gz3SrUDqBH5rTuhGR7ahtlbYKnVxw2bCFMRljaA7EXHaXZ8wsHdodFvbkhKmqg==} + resolution: {integrity: sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=} /concat-stream@1.6.2: resolution: {integrity: sha512-27HBghJxjiZtIk3Ycvn/4kbJk/1uZuJFfuPEns6LaEvpvG1f0hTea8lilrouyo9mVc2GWdcEZ8OLoGmSADlrCw==} @@ -10497,6 +10509,10 @@ packages: dev: false optional: true + /dompurify@3.0.6: + resolution: {integrity: sha512-ilkD8YEnnGh1zJ240uJsW7AzE+2qpbOUYjacomn3AvJ6J4JhKGSZ2nh4wUIXPZrEPppaCLx5jFe8T89Rk8tQ7w==} + dev: false + /domutils@1.7.0: resolution: {integrity: sha512-Lgd2XcJ/NjEw+7tFvfKxOzCYKZsdct5lczQ2ZaQY8Djz7pfAD3Gbp8ySJWtreII/vDlMVmxwa6pHmdxIYgttDg==} dependencies: @@ -13665,7 +13681,7 @@ packages: resolution: {integrity: sha512-Qczi5xnTNjkhcIB0Yy75Txt+Ez51xdhOxsukN7awzq2auZQGPHcQrJ623PZj0ECDEMOk2soxWx05EXdXGd1CbA==} engines: {node: ^10.13.0 || ^12.13.0 || ^14.15.0 || >=15.0.0} dependencies: - chalk: 4.1.0 + chalk: 4.1.2 jest-diff: 27.5.1 jest-get-type: 27.5.1 pretty-format: 27.5.1 diff --git a/editor.planx.uk/src/@planx/graph/index.ts b/editor.planx.uk/src/@planx/graph/index.ts index a45bc780b6..5785aede4e 100644 --- a/editor.planx.uk/src/@planx/graph/index.ts +++ b/editor.planx.uk/src/@planx/graph/index.ts @@ -1,7 +1,8 @@ import { TYPES } from "@planx/components/types"; +import DOMPurify from "dompurify"; import { enablePatches, produceWithPatches } from "immer"; -import { isEqual } from "lodash"; import difference from "lodash/difference"; +import isEqual from "lodash/isEqual"; import trim from "lodash/trim"; import zip from "lodash/zip"; import { customAlphabet } from "nanoid-good"; @@ -46,7 +47,7 @@ const isExternalPortalNodeType = (id: string, graph: Graph): boolean => const sanitize = (x: any) => { if ((x && typeof x === "string") || x instanceof String) { - return trim(x.replace(/[\u200B-\u200D\uFEFF↵]/g, "")); + return DOMPurify.sanitize(trim(x.replace(/[\u200B-\u200D\uFEFF↵]/g, ""))); } else if ((x && typeof x === "object") || x instanceof Object) { return Object.entries(x).reduce((acc, [k, v]) => { v = sanitize(v); From a822e726ea321ca5928e86c10848733b29241925 Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Thu, 23 Nov 2023 21:01:57 +0100 Subject: [PATCH 2/5] stub out unit tests --- .../src/@planx/graph/__tests__/add.test.ts | 27 +++++++++++++++++++ .../@planx/graph/__tests__/sanitize.test.ts | 13 +++++++++ editor.planx.uk/src/@planx/graph/index.ts | 5 ++-- 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts diff --git a/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts index d6dd86c546..a344457a2f 100644 --- a/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts +++ b/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts @@ -137,6 +137,33 @@ test("ignores empty values", () => { ]); }); +test.skip("sanitizes unsafe input", () => { + const [graph, ops] = add({ + id: "test", + type: 100, + data: { + text: "efef", + description: "

Test

" + }, + })({}); + expect(graph).toEqual({ + _root: { + edges: ["test"], + }, + test: { + type: 100, + data: { + text: "efef", + description: `

Test

`, + }, + }, + }); + expect(ops).toEqual([ + { oi: { edges: ["test"] }, p: ["_root"] }, + { oi: { data: { text: "efef", description: `

Test

`}}, p: ["test"] }, + ]); +}); + test("empty graph", () => { const [graph, ops] = add({ id: "a" })(); expect(graph).toEqual({ diff --git a/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts new file mode 100644 index 0000000000..55359e142c --- /dev/null +++ b/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts @@ -0,0 +1,13 @@ +import { sanitize } from ".."; + +test("sanitizes string", () => { + const bad = "

Test

" + expect(sanitize(bad)).toEqual(`

Test

`); +}); + +test.skip("sanitizes data object values", () => { + const badData = { + description: "

Test

" + }; + expect(sanitize(badData)).toEqual({ description: `

Test

` }); +}); diff --git a/editor.planx.uk/src/@planx/graph/index.ts b/editor.planx.uk/src/@planx/graph/index.ts index 5785aede4e..dcab6b7fd7 100644 --- a/editor.planx.uk/src/@planx/graph/index.ts +++ b/editor.planx.uk/src/@planx/graph/index.ts @@ -45,9 +45,10 @@ const isSectionNodeType = (id: string, graph: Graph): boolean => const isExternalPortalNodeType = (id: string, graph: Graph): boolean => graph[id]?.type === TYPES.ExternalPortal; -const sanitize = (x: any) => { +export const sanitize = (x: any) => { if ((x && typeof x === "string") || x instanceof String) { - return DOMPurify.sanitize(trim(x.replace(/[\u200B-\u200D\uFEFF↵]/g, ""))); + x = DOMPurify.sanitize(x as string); + return trim(x.replace(/[\u200B-\u200D\uFEFF↵]/g, "")); } else if ((x && typeof x === "object") || x instanceof Object) { return Object.entries(x).reduce((acc, [k, v]) => { v = sanitize(v); From 7b1c927781f28630b16ff891d9c7427fa69c16f4 Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Fri, 24 Nov 2023 09:55:11 +0100 Subject: [PATCH 3/5] new tests are passing --- editor.planx.uk/src/@planx/graph/__tests__/add.test.ts | 6 +++--- editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts | 6 +++--- editor.planx.uk/src/@planx/graph/index.ts | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts index a344457a2f..15e1eaa2f5 100644 --- a/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts +++ b/editor.planx.uk/src/@planx/graph/__tests__/add.test.ts @@ -137,13 +137,13 @@ test("ignores empty values", () => { ]); }); -test.skip("sanitizes unsafe input", () => { +test("sanitizes unsafe input", () => { const [graph, ops] = add({ id: "test", type: 100, data: { text: "efef", - description: "

Test

" + description: "

Test

", }, })({}); expect(graph).toEqual({ @@ -160,7 +160,7 @@ test.skip("sanitizes unsafe input", () => { }); expect(ops).toEqual([ { oi: { edges: ["test"] }, p: ["_root"] }, - { oi: { data: { text: "efef", description: `

Test

`}}, p: ["test"] }, + { oi: { data: { text: "efef", description: `

Test

` }, type: 100 }, p: ["test"] }, ]); }); diff --git a/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts index 55359e142c..ea0cb8de68 100644 --- a/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts +++ b/editor.planx.uk/src/@planx/graph/__tests__/sanitize.test.ts @@ -1,13 +1,13 @@ import { sanitize } from ".."; test("sanitizes string", () => { - const bad = "

Test

" + const bad = "

Test

"; expect(sanitize(bad)).toEqual(`

Test

`); }); -test.skip("sanitizes data object values", () => { +test("sanitizes data object values", () => { const badData = { - description: "

Test

" + description: "

Test

", }; expect(sanitize(badData)).toEqual({ description: `

Test

` }); }); diff --git a/editor.planx.uk/src/@planx/graph/index.ts b/editor.planx.uk/src/@planx/graph/index.ts index dcab6b7fd7..34c042b630 100644 --- a/editor.planx.uk/src/@planx/graph/index.ts +++ b/editor.planx.uk/src/@planx/graph/index.ts @@ -52,6 +52,7 @@ export const sanitize = (x: any) => { } else if ((x && typeof x === "object") || x instanceof Object) { return Object.entries(x).reduce((acc, [k, v]) => { v = sanitize(v); + acc[k] = v; if ( !isSomething(v) || (typeof v === "object" && Object.keys(v as object).length === 0) From 6437fa576ab86532190555d79a9438ccf7b39e32 Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Fri, 24 Nov 2023 10:30:02 +0100 Subject: [PATCH 4/5] add _update unit test too --- .../src/@planx/graph/__tests__/update.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/editor.planx.uk/src/@planx/graph/__tests__/update.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/update.test.ts index 7c5e1dd1a0..ff2a581e24 100644 --- a/editor.planx.uk/src/@planx/graph/__tests__/update.test.ts +++ b/editor.planx.uk/src/@planx/graph/__tests__/update.test.ts @@ -26,6 +26,29 @@ describe("updating", () => { expect(ops).toEqual([]); }); + test("doesn't save unsafe data", () => { + const [graph, ops] = update("a", { + description: "

Test

", + })({ + a: { + data: { + text: "efef", + }, + }, + }); + + expect(graph).toEqual({ + a: { + data: { + text: "efef", + description: `

Test

`, + }, + }, + }); + + expect(ops).toEqual([{ oi: `

Test

`, p: ["a", "data", "description"] }]); + }); + test("add a field to a without affecting existing data", () => { const [graph, ops] = update("a", { foo: "bar" })({ a: { From 42f36717c6989a3fed7374ef6c32be24103b22da Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Fri, 24 Nov 2023 15:55:15 +0100 Subject: [PATCH 5/5] try adding dompurify direct to sharedb too --- sharedb.planx.uk/package.json | 1 + sharedb.planx.uk/pnpm-lock.yaml | 7 +++++++ sharedb.planx.uk/sharedb-postgresql.js | 21 +++++++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/sharedb.planx.uk/package.json b/sharedb.planx.uk/package.json index 98c64ac37f..2334b43c31 100644 --- a/sharedb.planx.uk/package.json +++ b/sharedb.planx.uk/package.json @@ -4,6 +4,7 @@ "private": true, "dependencies": { "@teamwork/websocket-json-stream": "^2.0.0", + "dompurify": "^3.0.6", "jsonwebtoken": "^8.5.1", "pg": "^8.11.3", "sharedb": "^3.3.1", diff --git a/sharedb.planx.uk/pnpm-lock.yaml b/sharedb.planx.uk/pnpm-lock.yaml index c437ee6d45..27a2a30d8a 100644 --- a/sharedb.planx.uk/pnpm-lock.yaml +++ b/sharedb.planx.uk/pnpm-lock.yaml @@ -13,6 +13,9 @@ dependencies: '@teamwork/websocket-json-stream': specifier: ^2.0.0 version: 2.0.0 + dompurify: + specifier: ^3.0.6 + version: 3.0.6 jsonwebtoken: specifier: '>=9.0.0' version: 9.0.1 @@ -64,6 +67,10 @@ packages: resolution: {integrity: sha512-XRRe6Glud4rd/ZGQfiV1ruXSfbvfJedlV9Y6zOlP+2K04vBYiJEte6stfFkCP03aMnY5tsipamumUjL14fofug==} dev: true + /dompurify@3.0.6: + resolution: {integrity: sha512-ilkD8YEnnGh1zJ240uJsW7AzE+2qpbOUYjacomn3AvJ6J4JhKGSZ2nh4wUIXPZrEPppaCLx5jFe8T89Rk8tQ7w==} + dev: false + /dynamic-dedupe@0.3.0: resolution: {integrity: sha512-ssuANeD+z97meYOqd50e04Ze5qp4bPqo8cCkI4TRjZkzAUgIDTrXV1R8QCdINpiI+hw14+rYazvTRdQrz0/rFQ==} dependencies: diff --git a/sharedb.planx.uk/sharedb-postgresql.js b/sharedb.planx.uk/sharedb-postgresql.js index fbabf13bc2..699ffbef43 100644 --- a/sharedb.planx.uk/sharedb-postgresql.js +++ b/sharedb.planx.uk/sharedb-postgresql.js @@ -1,5 +1,6 @@ const { Pool } = require("pg"); const { DB } = require("sharedb"); +const DOMPurify = require("dompurify"); function PostgresDB(options) { if (!(this instanceof PostgresDB)) { @@ -25,6 +26,22 @@ function rollback(client, done) { client.query("ROLLBACK", (err) => done(err)); } +// Also see editor.planx.uk/src/@planx/graph/index.ts +// This is a simplified implementation that only handles purification of unsafe values, it does not handle empty values +function sanitize(x) { + if ((x && typeof x === "string") || x instanceof String) { + return DOMPurify.sanitize(x); + } else if ((x && typeof x === "object") || x instanceof Object) { + return Object.entries(x).reduce((acc, [k, v]) => { + v = sanitize(v); + acc[k] = v; + return acc; + }, x); + } else { + return x; + } +} + // Persists an op and snapshot if it is for the next version. Calls back with // callback(err, succeeded) PostgresDB.prototype.commit = function ( @@ -35,6 +52,10 @@ PostgresDB.prototype.commit = function ( _options, callback ) { + // Remove any unsafe values from this operation before committing + op = sanitize(op); + console.log('sanitised op', op); + const { uId: actorId } = op.m; /*