From c707699ac9102a3d8a131eccecac783a312f96f0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 20 Feb 2024 19:35:14 +0000 Subject: [PATCH 1/8] build(deps-dev): bump ip from 2.0.0 to 2.0.1 (#1155) Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](https://github.com/indutny/node-ip/compare/v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3e3c218a6..19862ae9f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9300,9 +9300,9 @@ } }, "node_modules/ip": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.0.tgz", - "integrity": "sha512-WKa+XuLG1A1R0UWhl2+1XQSi+fZWMsYKffMZTTYsiZaUD8k2yDAj5atimTUD2TZkyCkNEeYE5NhFZmupOGtjYQ==", + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.1.tgz", + "integrity": "sha512-lJUL9imLTNi1ZfXT+DU6rBBdbiKGBuay9B6xGSPVjUeQwaH1RIGqef8RZkUtHioLmSNpPR5M4HVKJGm1j8FWVQ==", "dev": true, "optional": true }, From ff81a1aac3c6d9aff34cef7594e74d4bb1054338 Mon Sep 17 00:00:00 2001 From: Hsu Zhong Jun <27919917+dcshzj@users.noreply.github.com> Date: Thu, 22 Feb 2024 10:28:41 +0800 Subject: [PATCH 2/8] feat: add validation for homepage frontmatter (#1151) * feat: add validation for homepage frontmatter * tests: update to reject with 400 bad request * chore: define limits as constants * test: update homepage spec as function is no longer called * chore: remove unused object --- src/constants/constants.ts | 6 + .../__tests__/Homepage.spec.js | 14 +- src/routes/v2/authenticatedSites/homepage.js | 4 +- src/validators/RequestSchema.js | 145 +++++++++++++++++- 4 files changed, 154 insertions(+), 15 deletions(-) diff --git a/src/constants/constants.ts b/src/constants/constants.ts index 14576b24e..111dfccc4 100644 --- a/src/constants/constants.ts +++ b/src/constants/constants.ts @@ -90,3 +90,9 @@ export const STAGING_BRANCH = "staging" export const STAGING_LITE_BRANCH = "staging-lite" export const PLACEHOLDER_FILE_NAME = ".keep" export const GIT_SYSTEM_DIRECTORY = ".git" + +// Homepage blocks limits +export const MAX_HERO_KEY_HIGHLIGHTS = 4 +export const MAX_ANNOUNCEMENT_ITEMS = 5 +export const MAX_TEXTCARDS_CARDS = 4 +export const MAX_INFOCOLS_BOXES = 4 diff --git a/src/routes/v2/authenticatedSites/__tests__/Homepage.spec.js b/src/routes/v2/authenticatedSites/__tests__/Homepage.spec.js index c20e5f1f7..758b0b869 100644 --- a/src/routes/v2/authenticatedSites/__tests__/Homepage.spec.js +++ b/src/routes/v2/authenticatedSites/__tests__/Homepage.spec.js @@ -118,25 +118,17 @@ describe("Homepage Router", () => { ) }) - it("accepts valid homepage update requests with additional unspecified fields and returns the details of the file updated", async () => { + it("rejects homepage update requests with additional unspecified fields", async () => { const extraUpdateDetails = { ...updatePageDetails } // Add extra unspecified field extraUpdateDetails.content.frontMatter.extra = "" - const expectedServiceInput = { - content: updatePageDetails.content.pageBody, - frontMatter: updatePageDetails.content.frontMatter, - sha: updatePageDetails.sha, - } await request(app) .post(`/${siteName}/homepage`) .send(extraUpdateDetails) - .expect(200) + .expect(400) - expect(mockHomepagePageService.update).toHaveBeenCalledWith( - mockUserWithSiteSessionData, - expectedServiceInput - ) + expect(mockHomepagePageService.update).not.toHaveBeenCalled() }) }) }) diff --git a/src/routes/v2/authenticatedSites/homepage.js b/src/routes/v2/authenticatedSites/homepage.js index 84d4d7376..fa6c982b9 100644 --- a/src/routes/v2/authenticatedSites/homepage.js +++ b/src/routes/v2/authenticatedSites/homepage.js @@ -33,9 +33,7 @@ class HomepageRouter { async updateHomepage(req, res, next) { const { userWithSiteSessionData } = res.locals - const { error } = UpdateHomepageSchema.validate(req.body, { - allowUnknown: true, - }) + const { error } = UpdateHomepageSchema.validate(req.body) if (error) throw new BadRequestError(error.message) const { content: { frontMatter, pageBody }, diff --git a/src/validators/RequestSchema.js b/src/validators/RequestSchema.js index 0e682f8b3..9a02c5b89 100644 --- a/src/validators/RequestSchema.js +++ b/src/validators/RequestSchema.js @@ -1,5 +1,12 @@ const Joi = require("joi") +const { + MAX_HERO_KEY_HIGHLIGHTS, + MAX_ANNOUNCEMENT_ITEMS, + MAX_TEXTCARDS_CARDS, + MAX_INFOCOLS_BOXES, +} = require("@root/constants") + const FileSchema = Joi.object().keys({ name: Joi.string().required(), type: Joi.string().valid("file").required(), @@ -63,7 +70,143 @@ const UpdateHomepageSchema = Joi.object({ permalink: Joi.string().required(), notification: Joi.string().allow(""), image: Joi.string(), - sections: Joi.array().required(), + sections: Joi.array() + .items( + // Hero section + Joi.object({ + hero: Joi.object({ + variant: Joi.string().allow( + "side", + "image", + "floating", + "center", + "" + ), + backgroundColor: Joi.string().allow("black", "white", "gray", ""), + background: Joi.string().allow(""), + size: Joi.string().allow("sm", "md", ""), + alignment: Joi.string().allow("left", "right", ""), + title: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + button: Joi.string().allow(""), + url: Joi.string().allow(""), + dropdown: Joi.object({ + title: Joi.string().required(), + options: Joi.array().items( + Joi.object({ + title: Joi.string().required(), + url: Joi.string().required(), + }) + ), + }), + key_highlights: Joi.array() + .max(MAX_HERO_KEY_HIGHLIGHTS) + .items( + Joi.object({ + title: Joi.string().required(), + description: Joi.string().allow(""), + url: Joi.string().allow(""), + }) + ), + }), + }), + + // Resources section + Joi.object({ + resources: Joi.object({ + title: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + id: Joi.string().allow(""), + button: Joi.string().allow(""), + }), + }), + + // Infobar section + Joi.object({ + infobar: Joi.object({ + title: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + id: Joi.string().allow(""), + description: Joi.string().allow(""), + button: Joi.string().allow(""), + url: Joi.string().allow(""), + }), + }), + + // Infopic section + Joi.object({ + infopic: Joi.object({ + title: Joi.string(), + subtitle: Joi.string().allow(""), + id: Joi.string().allow(""), + description: Joi.string().allow(""), + button: Joi.string().allow(""), + url: Joi.string().allow(""), + image: Joi.string(), + alt: Joi.string(), + }), + }), + + // Announcements section + Joi.object({ + announcements: Joi.object({ + title: Joi.string().allow(""), + id: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + announcement_items: Joi.array() + .max(MAX_ANNOUNCEMENT_ITEMS) + .items( + Joi.object({ + title: Joi.string().required(), + date: Joi.string().required(), + announcement: Joi.string().required(), + link_text: Joi.string().allow(""), + link_url: Joi.string().allow(""), + }) + ), + }), + }), + + // Textcard section + Joi.object({ + textcards: Joi.object({ + title: Joi.string().required(), + id: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + description: Joi.string().allow(""), + cards: Joi.array() + .max(MAX_TEXTCARDS_CARDS) + .items( + Joi.object({ + title: Joi.string().required(), + description: Joi.string().allow(""), + linktext: Joi.string().allow(""), + url: Joi.string().allow(""), + }) + ), + }), + }), + + // Infocols section + Joi.object({ + infocols: Joi.object({ + title: Joi.string().required(), + id: Joi.string().allow(""), + subtitle: Joi.string().allow(""), + url: Joi.string().allow(""), + linktext: Joi.string().allow(""), + infoboxes: Joi.array() + .max(MAX_INFOCOLS_BOXES) + .items( + Joi.object({ + title: Joi.string().required(), + description: Joi.string().allow(""), + }) + ), + }), + }) + ) + .required(), }).required(), pageBody: Joi.string().allow(""), // Joi does not allow empty string (pageBody: '') for Joi.string() even if not required }).required(), From 47a970073451bd241d98b1d6f4e0d7bef4f56c72 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Thu, 22 Feb 2024 14:52:58 +0800 Subject: [PATCH 3/8] Fix/sanitise urls (#1158) * chore: install url-template * chore: use url-template for all endpoints with user determined params * fix: more endpoints * fix: tests --- package-lock.json | 13 +++ package.json | 2 + src/services/api/NetlifyApi.ts | 6 +- src/services/db/GitHubService.ts | 86 +++++++++----- .../db/__tests__/GitHubService.spec.ts | 27 +++-- src/services/db/review.ts | 106 ++++++++++++------ src/services/identity/PreviewService.ts | 10 +- src/utils/__tests__/media-utils.spec.ts | 16 +-- src/utils/media-utils.ts | 26 +++-- src/utils/utils.js | 68 +++++++---- 10 files changed, 248 insertions(+), 112 deletions(-) diff --git a/package-lock.json b/package-lock.json index 19862ae9f..fba4180ed 100644 --- a/package-lock.json +++ b/package-lock.json @@ -75,6 +75,7 @@ "ts-node": "^10.7.0", "type-fest": "^2.12.0", "umzug": "^3.0.0", + "url-template": "^2.0.8", "uuid": "^3.3.3", "validator": "^13.6.0", "winston": "^3.3.3", @@ -97,6 +98,7 @@ "@types/mock-fs": "^4.13.1", "@types/node": "^17.0.21", "@types/supertest": "^2.0.11", + "@types/url-template": "^2.0.31", "@types/validator": "^13.7.1", "@typescript-eslint/eslint-plugin": "^5.17.0", "@typescript-eslint/parser": "^5.17.0", @@ -4713,6 +4715,12 @@ "resolved": "https://registry.npmjs.org/@types/trusted-types/-/trusted-types-2.0.3.tgz", "integrity": "sha512-NfQ4gyz38SL8sDNrSixxU2Os1a5xcdFxipAFxYEuLUlvU2uDwS4NUpsImcf1//SlWItCVMMLiylsxbmNMToV/g==" }, + "node_modules/@types/url-template": { + "version": "2.0.31", + "resolved": "https://registry.npmjs.org/@types/url-template/-/url-template-2.0.31.tgz", + "integrity": "sha512-mXzP2L5FkK9b+MYqlKYSJG/1RTMFWZT1oBuQ6KPzSY3m1hwKks7PrHg+Yd3QkoedBUJO749JXvK3FgCgGGIx6Q==", + "dev": true + }, "node_modules/@types/validator": { "version": "13.7.17", "resolved": "https://registry.npmjs.org/@types/validator/-/validator-13.7.17.tgz", @@ -16420,6 +16428,11 @@ "requires-port": "^1.0.0" } }, + "node_modules/url-template": { + "version": "2.0.8", + "resolved": "https://registry.npmjs.org/url-template/-/url-template-2.0.8.tgz", + "integrity": "sha512-XdVKMF4SJ0nP/O7XIPB0JwAEuT9lDIYnNsK8yGVe43y0AWoKeJNdv3ZNWh7ksJ6KqQFjOO6ox/VEitLnaVNufw==" + }, "node_modules/url/node_modules/punycode": { "version": "1.3.2", "resolved": "https://registry.npmjs.org/punycode/-/punycode-1.3.2.tgz", diff --git a/package.json b/package.json index 57ea0d0a6..828791cf2 100644 --- a/package.json +++ b/package.json @@ -91,6 +91,7 @@ "ts-node": "^10.7.0", "type-fest": "^2.12.0", "umzug": "^3.0.0", + "url-template": "^2.0.8", "uuid": "^3.3.3", "validator": "^13.6.0", "winston": "^3.3.3", @@ -113,6 +114,7 @@ "@types/mock-fs": "^4.13.1", "@types/node": "^17.0.21", "@types/supertest": "^2.0.11", + "@types/url-template": "^2.0.31", "@types/validator": "^13.7.1", "@typescript-eslint/eslint-plugin": "^5.17.0", "@typescript-eslint/parser": "^5.17.0", diff --git a/src/services/api/NetlifyApi.ts b/src/services/api/NetlifyApi.ts index 4a12b97dc..7fb2b2786 100644 --- a/src/services/api/NetlifyApi.ts +++ b/src/services/api/NetlifyApi.ts @@ -1,4 +1,5 @@ import axios from "axios" +import urlTemplate from "url-template" import { config } from "@config/config" @@ -62,7 +63,10 @@ const updatePasswordAndStopBuildNetlifySite = async ( repoId: string, password: string ) => { - const endpoint = `https://api.netlify.com/api/v1/sites/${repoId}` + const endpointTemplate = urlTemplate.parse( + `https://api.netlify.com/api/v1/sites/{repoId}` + ) + const endpoint = endpointTemplate.expand({ repoId }) const headers = { Authorization: `Bearer ${NETLIFY_ACCESS_TOKEN}`, "Content-Type": "application/json", diff --git a/src/services/db/GitHubService.ts b/src/services/db/GitHubService.ts index f3cfe0c89..939033929 100644 --- a/src/services/db/GitHubService.ts +++ b/src/services/db/GitHubService.ts @@ -1,6 +1,7 @@ import { AxiosCacheInstance } from "axios-cache-interceptor" import { Base64 } from "js-base64" import { okAsync, errAsync, ResultAsync } from "neverthrow" +import urlTemplate from "url-template" import { ConflictError, inputNameConflictErrorMsg } from "@errors/ConflictError" import { NotFoundError } from "@errors/NotFoundError" @@ -97,19 +98,31 @@ export default class GitHubService { fileName: string directoryName: string | undefined }) { - if (!directoryName) - return `${siteName}/contents/${encodeURIComponent(fileName)}` - const encodedDirPath = directoryName - .split("/") - .map((folder: string | number | boolean) => encodeURIComponent(folder)) - .join("/") - return `${siteName}/contents/${encodedDirPath}/${encodeURIComponent( - fileName - )}` + if (!directoryName) { + const endpointTemplate = urlTemplate.parse( + `{siteName}/contents/{fileName}` + ) + return endpointTemplate.expand({ + siteName, + fileName, + }) + } + const endpointTemplate = urlTemplate.parse( + `{siteName}/contents/{directoryName}/{fileName}` + ) + return endpointTemplate.expand({ + siteName, + directoryName, + fileName, + }) } getBlobPath({ siteName, fileSha }: { siteName: string; fileSha: string }) { - return `${siteName}/git/blobs/${fileSha}` + const endpointTemplate = urlTemplate.parse(`{siteName}/git/blobs/{fileSha}`) + return endpointTemplate.expand({ + siteName, + fileSha, + }) } getFolderPath({ @@ -119,11 +132,13 @@ export default class GitHubService { siteName: string directoryName: string }) { - const encodedDirPath = directoryName - .split("/") - .map((folder: string | number | boolean) => encodeURIComponent(folder)) - .join("/") - return `${siteName}/contents/${encodedDirPath}` + const endpointTemplate = urlTemplate.parse( + `{siteName}/contents/{directoryName}` + ) + return endpointTemplate.expand({ + siteName, + directoryName, + }) } async create( @@ -455,7 +470,8 @@ export default class GitHubService { ): Promise { const { siteName } = sessionData const { accessToken } = sessionData - const endpoint = `${siteName}` + const endpointTemplate = urlTemplate.parse(`{siteName}`) + const endpoint = endpointTemplate.expand({ siteName }) const headers = { Authorization: `token ${accessToken}`, } @@ -474,7 +490,8 @@ export default class GitHubService { async getRepoState(sessionData: UserWithSiteSessionData): Promise { const { accessToken } = sessionData const { siteName } = sessionData - const endpoint = `${siteName}/commits` + const endpointTemplate = urlTemplate.parse(`{siteName}/commits`) + const endpoint = endpointTemplate.expand({ siteName }) const headers = { Authorization: `token ${accessToken}`, } @@ -502,7 +519,8 @@ export default class GitHubService { branch: string ) { const { accessToken, siteName } = sessionData - const endpoint = `${siteName}/commits/${branch}` + const endpointTemplate = urlTemplate.parse(`{siteName}/commits/{branch}`) + const endpoint = endpointTemplate.expand({ siteName, branch }) const headers = { Authorization: `token ${accessToken}`, } @@ -529,7 +547,8 @@ export default class GitHubService { path: string ) { const { accessToken, siteName } = sessionData - const endpoint = `${siteName}/commits` + const endpointTemplate = urlTemplate.parse(`{siteName}/commits`) + const endpoint = endpointTemplate.expand({ siteName }) const headers = { Authorization: `token ${accessToken}`, } @@ -556,7 +575,10 @@ export default class GitHubService { const { accessToken } = sessionData const { siteName } = sessionData const { treeSha } = githubSessionData.getGithubState() - const url = `${siteName}/git/trees/${treeSha}` + const urlEndpointTemplate = urlTemplate.parse( + `{siteName}/git/trees/{treeSha}` + ) + const url = urlEndpointTemplate.expand({ siteName, treeSha }) const params = { ref: STAGING_BRANCH, @@ -582,7 +604,8 @@ export default class GitHubService { ) { const { accessToken, siteName, isomerUserId: userId } = sessionData const { treeSha, currentCommitSha } = githubSessionData.getGithubState() - const url = `${siteName}/git/trees` + const urlEndpointTemplate = urlTemplate.parse(`{siteName}/git/trees`) + const url = urlEndpointTemplate.expand({ siteName }) const headers = { Authorization: `token ${accessToken}`, @@ -601,7 +624,8 @@ export default class GitHubService { data: { sha: newTreeSha }, } = resp - const commitEndpoint = `${siteName}/git/commits` + const commitEndpointTemplate = urlTemplate.parse(`{siteName}/git/commits`) + const commitEndpoint = commitEndpointTemplate.expand({ siteName }) const stringifiedMessage = JSON.stringify({ message: message || `isomerCMS updated ${siteName} state`, @@ -784,9 +808,13 @@ export default class GitHubService { ) { const { accessToken } = sessionData const { siteName } = sessionData - const refEndpoint = `${siteName}/git/refs/heads/${ - branchName || STAGING_BRANCH - }` + const refTemplate = urlTemplate.parse( + `{siteName}/git/refs/heads/{branchName}` + ) + const refEndpoint = refTemplate.expand({ + siteName, + branchName: branchName || STAGING_BRANCH, + }) const headers = { Authorization: `token ${accessToken}`, } @@ -802,7 +830,10 @@ export default class GitHubService { const { accessToken } = sessionData const userId = sessionData.githubId const { siteName } = sessionData - const endpoint = `${siteName}/collaborators/${userId}` + const endpointTemplate = urlTemplate.parse( + `{siteName}/collaborators/{userId}` + ) + const endpoint = endpointTemplate.expand({ siteName, userId }) const headers = { Authorization: `token ${accessToken}`, @@ -827,7 +858,8 @@ export default class GitHubService { shouldMakePrivate: boolean ): ResultAsync { const { siteName, isomerUserId } = sessionData - const endpoint = `${siteName}` + const endpointTemplate = urlTemplate.parse(`{siteName}`) + const endpoint = endpointTemplate.expand({ siteName }) // Privatising a repo is restricted to repo admins - an admin token will be inserted in via our axios interceptor const headers = { diff --git a/src/services/db/__tests__/GitHubService.spec.ts b/src/services/db/__tests__/GitHubService.spec.ts index 11d84d90b..f994a30dc 100644 --- a/src/services/db/__tests__/GitHubService.spec.ts +++ b/src/services/db/__tests__/GitHubService.spec.ts @@ -43,6 +43,7 @@ describe("Github Service", () => { const subDirectoryName = `files/parent-file/sub-directory` const subDirectoryFileName = ".keep" const resourceCategoryName = "resources/some-folder" + const resourceCategoryParsedName = "resources%2Fsome-folder" const topLevelDirectoryFileName = "collection.yml" const resourceCategoryFileName = "index.html" @@ -89,13 +90,14 @@ describe("Github Service", () => { it("should retrieve the right subcollection page file path", async () => { const subcollectionPath = `_${collectionName}/${subcollectionName}` + const parsedSubcollectionPath = `_${collectionName}%2F${subcollectionName}` expect( service.getFilePath({ siteName, fileName, directoryName: subcollectionPath, }) - ).toEqual(`${siteName}/contents/${subcollectionPath}/${fileName}`) + ).toEqual(`${siteName}/contents/${parsedSubcollectionPath}/${fileName}`) }) }) @@ -110,7 +112,7 @@ describe("Github Service", () => { const specialDirName = `special?/direct ory` const specialParsedDirName = `${encodeURIComponent( "special?" - )}/${encodeURIComponent("direct ory")}` + )}%2F${encodeURIComponent("direct ory")}` expect( service.getFolderPath({ siteName, directoryName: specialDirName }) ).toEqual(`${siteName}/contents/${specialParsedDirName}`) @@ -118,10 +120,10 @@ describe("Github Service", () => { }) describe("Create", () => { - const fileParentEndpoint = `${siteName}/contents/files/parent-file` + const fileParentParsedEndpoint = `${siteName}/contents/files%2Fparent-file` const folderParentEndpoint = `${siteName}/contents/${directoryName}` const folderEndpoint = `${folderParentEndpoint}/${fileName}` - const resourceRoomEndpoint = `${siteName}/contents/${resourceCategoryName}` + const resourceRoomEndpoint = `${siteName}/contents/${resourceCategoryParsedName}` const encodedContent = Base64.encode(content) const message = JSON.stringify({ @@ -331,13 +333,16 @@ describe("Github Service", () => { isMedia: false, }) ).rejects.toThrowError() - expect(mockAxiosInstance.get).toHaveBeenCalledWith(fileParentEndpoint, { - validateStatus, - headers: authHeader.headers, - params: { - ref: BRANCH_REF, - }, - }) + expect(mockAxiosInstance.get).toHaveBeenCalledWith( + fileParentParsedEndpoint, + { + validateStatus, + headers: authHeader.headers, + params: { + ref: BRANCH_REF, + }, + } + ) }) it("should throw an error if a resource is created while the resource folder is deleted", async () => { diff --git a/src/services/db/review.ts b/src/services/db/review.ts index 81370a692..c764c7e6f 100644 --- a/src/services/db/review.ts +++ b/src/services/db/review.ts @@ -1,5 +1,6 @@ import { AxiosError } from "axios" import _ from "lodash" +import urlTemplate from "url-template" import { config } from "@config/config" @@ -19,12 +20,15 @@ export const getCommitDiff = async ( siteName: string, base = "master", head = "staging" -) => - axiosInstance - .get<{ files: RawFileChangeInfo[]; commits: Commit[] }>( - `${siteName}/compare/${base}...${head}` - ) +) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/compare/{base}...{head}` + ) + const endpoint = endpointTemplate.expand({ siteName, base, head }) + return axiosInstance + .get<{ files: RawFileChangeInfo[]; commits: Commit[] }>(endpoint) .then(({ data }) => data) +} export const createPullRequest = ( siteName: string, @@ -32,56 +36,85 @@ export const createPullRequest = ( description?: string, base = "master", head = "staging" -) => - axiosInstance +) => { + const endpointTemplate = urlTemplate.parse(`{siteName}/pulls`) + const endpoint = endpointTemplate.expand({ siteName }) + return axiosInstance .post<{ number: number }>( - `${siteName}/pulls`, + endpoint, // NOTE: only create body if a valid description is given { title, base, head, ...(description && { body: description }) } ) .then(({ data }) => data.number) +} -export const getPullRequest = (siteName: string, pullRequestNumber: number) => - axiosInstance - .get(`${siteName}/pulls/${pullRequestNumber}`) - .then(({ data }) => data) +export const getPullRequest = (siteName: string, pullRequestNumber: number) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/pulls/{pullRequestNumber}` + ) + const endpoint = endpointTemplate.expand({ siteName, pullRequestNumber }) + return axiosInstance.get(endpoint).then(({ data }) => data) +} export const updatePullRequest = ( siteName: string, pullRequestNumber: number, title: string, description?: string -) => - axiosInstance.patch( - `${siteName}/pulls/${pullRequestNumber}`, +) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/pulls/{pullRequestNumber}` + ) + const endpoint = endpointTemplate.expand({ siteName, pullRequestNumber }) + return axiosInstance.patch( + endpoint, // NOTE: only create body if a valid description is given { title, ...(description !== undefined && { body: description }) } ) +} export const closeReviewRequest = ( siteName: string, pullRequestNumber: number -) => - axiosInstance.patch( - `${siteName}/pulls/${pullRequestNumber}`, +) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/pulls/{pullRequestNumber}` + ) + const endpoint = endpointTemplate.expand({ siteName, pullRequestNumber }) + return axiosInstance.patch( + endpoint, // NOTE: only create body if a valid description is given { state: "closed" } ) +} export const mergePullRequest = ( repoNameInGithub: string, pullRequestNumber: number -) => - axiosInstance.put( - `${repoNameInGithub}/pulls/${pullRequestNumber}/merge` +) => { + const endpointTemplate = urlTemplate.parse( + `{repoNameInGithub}/pulls/{pullRequestNumber}/merge` ) + const endpoint = endpointTemplate.expand({ + repoNameInGithub, + pullRequestNumber, + }) + return axiosInstance.put(endpoint) +} export const approvePullRequest = ( repoNameInGithub: string, pullRequestNumber: number -) => - axiosInstance.post( - `${repoNameInGithub}/pulls/${pullRequestNumber}/reviews`, +) => { + const endpointTemplate = urlTemplate.parse( + `{repoNameInGithub}/pulls/{pullRequestNumber}/reviews` + ) + const endpoint = endpointTemplate.expand({ + repoNameInGithub, + pullRequestNumber, + }) + return axiosInstance.post( + endpoint, { event: "APPROVE", }, @@ -99,13 +132,18 @@ export const approvePullRequest = ( }, } ) +} export const getComments = async ( siteName: string, pullRequestNumber: number ) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/issues/{pullRequestNumber}/comments` + ) + const endpoint = endpointTemplate.expand({ siteName, pullRequestNumber }) const rawComments = await axiosInstance - .get(`${siteName}/issues/${pullRequestNumber}/comments`) + .get(endpoint) .then(({ data }) => data) return _.compact( rawComments.map((rawComment) => { @@ -128,23 +166,26 @@ export const createComment = async ( userId: string, message: string ) => { + const endpointTemplate = urlTemplate.parse( + `{siteName}/pulls/{pullRequestNumber}/comments` + ) + const endpoint = endpointTemplate.expand({ siteName, pullRequestNumber }) const stringifiedMessage = JSON.stringify({ userId, message, }) - return axiosInstance.post( - `${siteName}/issues/${pullRequestNumber}/comments`, - { body: stringifiedMessage } - ) + return axiosInstance.post(endpoint, { body: stringifiedMessage }) } export const getBlob = async ( repo: string, path: string, ref: string -): Promise => - axiosInstance - .get(`${repo}/contents/${path}?ref=${ref}`, { +): Promise => { + const endpointTemplate = urlTemplate.parse(`{repo}/contents/{path}?ref={ref}`) + const endpoint = endpointTemplate.expand({ repo, path, ref }) + return axiosInstance + .get(endpoint, { headers: { Accept: "application/vnd.github.raw", }, @@ -158,3 +199,4 @@ export const getBlob = async ( throw err }) .then(({ data }) => data) +} diff --git a/src/services/identity/PreviewService.ts b/src/services/identity/PreviewService.ts index 4506a8231..d8e96a8a1 100644 --- a/src/services/identity/PreviewService.ts +++ b/src/services/identity/PreviewService.ts @@ -1,15 +1,18 @@ import axios from "axios" import { JSDOM } from "jsdom" import { ResultAsync, okAsync, errAsync } from "neverthrow" +import urlTemplate from "url-template" import PreviewParsingError from "@errors/PreviewParsingError" import { PreviewInfo } from "@root/types/previewInfo" export default class PreviewService { - getImageUrl = (siteUrl: string): ResultAsync => - ResultAsync.fromPromise( - axios.get(siteUrl), + getImageUrl = (siteUrl: string): ResultAsync => { + const endpointTemplate = urlTemplate.parse(`{siteUrl}`) + const endpoint = endpointTemplate.expand({ siteUrl }) + return ResultAsync.fromPromise( + axios.get(endpoint), () => new PreviewParsingError(siteUrl) ).andThen((documentResponse) => { const { window } = new JSDOM(documentResponse.data) @@ -26,6 +29,7 @@ export default class PreviewService { } return errAsync(new PreviewParsingError(siteUrl)) }) + } getPreviewInfo = (siteUrl: string): Promise => this.getImageUrl(siteUrl).match( diff --git a/src/utils/__tests__/media-utils.spec.ts b/src/utils/__tests__/media-utils.spec.ts index 4fa8ffa4d..d6945376d 100644 --- a/src/utils/__tests__/media-utils.spec.ts +++ b/src/utils/__tests__/media-utils.spec.ts @@ -36,7 +36,7 @@ mockGenericAxios.get.mockResolvedValue({ describe("Media utils test", () => { it("should return mediaUrl as raw github information for images in public repos", async () => { const expectedResp = { - mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}`, name: MEDIA_FILE_NAME, @@ -53,9 +53,9 @@ describe("Media utils test", () => { it("should handle nested images in public repos", async () => { const expectedResp = { - mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_SUBDIRECTORY_NAME - )}/${encodeURIComponent(MEDIA_FILE_NAME)}`, + )}%2F${encodeURIComponent(MEDIA_FILE_NAME)}`, name: MEDIA_FILE_NAME, sha: MEDIA_FILE_SHA, mediaPath: `${MEDIA_DIRECTORY_NAME}/${MEDIA_SUBDIRECTORY_NAME}/${MEDIA_FILE_NAME}`, @@ -70,7 +70,7 @@ describe("Media utils test", () => { it("should return mediaUrl as raw github information for svgs with sanitisation in public repos", async () => { const expectedResp = { - mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}.svg?sanitize=true`, name: `${MEDIA_FILE_NAME}.svg`, @@ -87,7 +87,7 @@ describe("Media utils test", () => { it("should return mediaUrl as raw github information for files in public repos", async () => { const expectedResp = { - mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}`, name: MEDIA_FILE_NAME, @@ -117,7 +117,7 @@ describe("Media utils test", () => { expect( mockGenericAxios.get ).toHaveBeenCalledWith( - `https://token@raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + `https://token@raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}`, { responseType: "arraybuffer" } @@ -139,7 +139,7 @@ describe("Media utils test", () => { expect( mockGenericAxios.get ).toHaveBeenCalledWith( - `https://token@raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + `https://token@raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}.svg?sanitize=true`, { responseType: "arraybuffer" } @@ -148,7 +148,7 @@ describe("Media utils test", () => { it("should return mediaUrl as raw github information information for files in private repos", async () => { const expectedResp = { - mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}/${encodeURIComponent( + mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${MEDIA_SITE_NAME}/staging/${MEDIA_DIRECTORY_NAME}%2F${encodeURIComponent( MEDIA_FILE_NAME )}`, name: MEDIA_FILE_NAME, diff --git a/src/utils/media-utils.ts b/src/utils/media-utils.ts index 73d0cc4f7..ca9968318 100644 --- a/src/utils/media-utils.ts +++ b/src/utils/media-utils.ts @@ -1,4 +1,5 @@ import axios from "axios" +import urlTemplate from "url-template" import { config } from "@config/config" @@ -14,15 +15,22 @@ const getEncodedFilePathAsUriComponent = ( accessToken?: string ) => { const isSvg = filePath.endsWith(".svg") - const encodedFilePath = filePath - .split("/") - .map((v) => encodeURIComponent(v)) - .join("/") - return `https://${ - accessToken ? `${accessToken}@` : "" - }raw.githubusercontent.com/${GITHUB_ORG_NAME}/${siteName}/staging/${encodedFilePath}${ - isSvg ? "?sanitize=true" : "" - }` + const endpointTemplate = urlTemplate.parse( + `https://${ + accessToken ? `{accessToken}@` : "" + }raw.githubusercontent.com/{GITHUB_ORG_NAME}/{siteName}/staging/{filePath}${ + isSvg ? "?sanitize=true" : "" + }` + ) + const endpoint = accessToken + ? endpointTemplate.expand({ + accessToken, + GITHUB_ORG_NAME, + siteName, + filePath, + }) + : endpointTemplate.expand({ GITHUB_ORG_NAME, siteName, filePath }) + return endpoint } export const getMediaFileInfo = async ({ diff --git a/src/utils/utils.js b/src/utils/utils.js index 98e30f037..77c023e8b 100644 --- a/src/utils/utils.js +++ b/src/utils/utils.js @@ -1,4 +1,5 @@ const slugify = require("slugify") +const urlTemplate = require("url-template") const { config } = require("@config/config") @@ -11,15 +12,17 @@ async function getCommitAndTreeSha(repo, accessToken, branchRef = "staging") { Authorization: `token ${accessToken}`, } // Get the commits of the repo - const { data: commits } = await genericGitHubAxiosInstance.get( - `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/commits`, - { - params: { - sha: branchRef, - }, - headers, - } + const endpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/commits` ) + const endpoint = endpointTemplate.expand({ GITHUB_ORG_NAME, repo }) + + const { data: commits } = await genericGitHubAxiosInstance.get(endpoint, { + params: { + sha: branchRef, + }, + headers, + }) // Get the tree sha of the latest commit const { commit: { @@ -50,15 +53,16 @@ async function getTree( if (isRecursive) params.recursive = true + const endpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/git/trees/{treeSha}` + ) + const endpoint = endpointTemplate.expand({ GITHUB_ORG_NAME, repo, treeSha }) const { data: { tree: gitTree }, - } = await genericGitHubAxiosInstance.get( - `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/git/trees/${treeSha}`, - { - params, - headers, - } - ) + } = await genericGitHubAxiosInstance.get(endpoint, { + params, + headers, + }) return gitTree } @@ -76,8 +80,12 @@ async function sendTree( const headers = { Authorization: `token ${accessToken}`, } + const endpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/git/trees` + ) + const endpoint = endpointTemplate.expand({ GITHUB_ORG_NAME, repo }) const resp = await genericGitHubAxiosInstance.post( - `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/git/trees`, + endpoint, { tree: gitTree, base_tree: baseTreeSha, @@ -91,9 +99,21 @@ async function sendTree( data: { sha: newTreeSha }, } = resp - const baseRefEndpoint = `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/git/refs` - const baseCommitEndpoint = `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/git/commits` - const refEndpoint = `${baseRefEndpoint}/heads/${branchRef}` + const refEndpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/git/refs/heads/{branchRef}` + ) + const refEndpoint = refEndpointTemplate.expand({ + GITHUB_ORG_NAME, + repo, + branchRef, + }) + const baseCommitEndpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/git/commits` + ) + const baseCommitEndpoint = baseCommitEndpointTemplate.expand({ + GITHUB_ORG_NAME, + repo, + }) const newCommitResp = await genericGitHubAxiosInstance.post( baseCommitEndpoint, @@ -136,8 +156,14 @@ async function revertCommit( Authorization: `token ${accessToken}`, } - const baseRefEndpoint = `https://api.github.com/repos/${GITHUB_ORG_NAME}/${repo}/git/refs` - const refEndpoint = `${baseRefEndpoint}/heads/${branchRef}` + const refEndpointTemplate = urlTemplate.parse( + `https://api.github.com/repos/{GITHUB_ORG_NAME}/{repo}/git/refs/heads/{branchRef}` + ) + const refEndpoint = refEndpointTemplate.expand({ + GITHUB_ORG_NAME, + repo, + branchRef, + }) /** * The `staging` branch reference will now point to `currentCommitSha` From e911e2ef678277b3cff7333ae6cd89618c6f8b1e Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Thu, 22 Feb 2024 14:53:22 +0800 Subject: [PATCH 4/8] Chore/lock repos when repairing (#1149) * chore: lock repos during repair * chore: lock repo for 15 minutes during repair * chore: change var name --- src/routes/formsg/formsgGGsRepair.ts | 21 +++++++++++++++++++-- src/utils/mutex-utils.js | 8 ++++---- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/routes/formsg/formsgGGsRepair.ts b/src/routes/formsg/formsgGGsRepair.ts index 2bf41a5e9..f8a6fce4b 100644 --- a/src/routes/formsg/formsgGGsRepair.ts +++ b/src/routes/formsg/formsgGGsRepair.ts @@ -10,12 +10,15 @@ import { ResultAsync, errAsync, fromPromise, okAsync } from "neverthrow" import { config } from "@config/config" +import { lock, unlock } from "@utils/mutex-utils" + import { EFS_VOL_PATH_STAGING, EFS_VOL_PATH_STAGING_LITE, } from "@root/constants" import GitFileSystemError from "@root/errors/GitFileSystemError" import InitializationError from "@root/errors/InitializationError" +import LockedError from "@root/errors/LockedError" import { consoleLogger } from "@root/logger/console.logger" import logger from "@root/logger/logger" import { attachFormSGHandler } from "@root/middleware" @@ -114,15 +117,20 @@ export class FormsgGGsRepairRouter { } handleGGsFormSubmission = (repoNames: string[], requesterEmail: string) => { - const repairs: ResultAsync[] = [] + const repairs: ResultAsync[] = [] const clonedStagingRepos: string[] = [] const syncedStagingAndStagingLiteRepos: string[] = [] + const LOCK_TIME_SECONDS = 15 * 60 // 15 minutes repoNames.forEach((repoName) => { const repoUrl = `git@github.com:isomerpages/${repoName}.git` repairs.push( - this.doesRepoNeedClone(repoName) + ResultAsync.fromPromise( + lock(repoName, LOCK_TIME_SECONDS), + (err) => new LockedError(`Unable to lock repo ${repoName}`) + ) + .andThen(() => this.doesRepoNeedClone(repoName)) .andThen(() => { const isStaging = true return ( @@ -166,6 +174,15 @@ export class FormsgGGsRepairRouter { return okAsync(result) }) ) + .andThen((result) => { + // Failure to unlock is not blocking + ResultAsync.fromPromise(unlock(repoName), () => { + logger.error( + "Failed to unlock repo - repo will unlock after at most 15 min" + ) + }) + return okAsync(result) + }) ) }) diff --git a/src/utils/mutex-utils.js b/src/utils/mutex-utils.js index fa623c383..b0f19805b 100644 --- a/src/utils/mutex-utils.js +++ b/src/utils/mutex-utils.js @@ -35,10 +35,10 @@ const mockUnlock = (siteName) => { mockMutexObj[siteName] = false } -const lock = async (siteName) => { +const lock = async (siteName, lockLengthSeconds = 60) => { try { - const ONE_MIN_FROM_CURR_DATE_IN_SECONDS_FROM_EPOCH_TIME = - Math.floor(new Date().valueOf() / 1000) + 60 + const expiryTime = + Math.floor(new Date().valueOf() / 1000) + lockLengthSeconds if (isE2eTestRepo(siteName)) return if (!IS_DEV) { @@ -46,7 +46,7 @@ const lock = async (siteName) => { TableName: MUTEX_TABLE_NAME, Item: { repo_id: siteName, - expdate: ONE_MIN_FROM_CURR_DATE_IN_SECONDS_FROM_EPOCH_TIME, + expdate: expiryTime, }, ConditionExpression: "attribute_not_exists(repo_id)", } From 5c56c748062b8cbb03885574f4905c724f512ba5 Mon Sep 17 00:00:00 2001 From: seaerchin <44049504+seaerchin@users.noreply.github.com> Date: Thu, 22 Feb 2024 15:42:18 +0800 Subject: [PATCH 5/8] fix(file-ext): prevent users from bypassing checks on file extensions (#1157) * fix(sanitiser): remove comments from allow list * fix(upload-utils): add detected file type to return value * fix(mediafileservice): construct file + ext from be * test(upload/media): add specs to ensure stuff is always sanitised * test(markdown): update spec for empty string * build(package): install dompurify * refactor(utils): instantiate separate dompurify --- package-lock.json | 21 +++++- package.json | 2 + .../MdPageServices/MediaFileService.js | 34 ++++++++-- .../__tests__/MediaFileService.spec.js | 64 ++++++++++++------- src/services/utilServices/Sanitizer.ts | 2 +- src/utils/__tests__/file-upload-utils.spec.ts | 52 +++++++++++++++ src/utils/__tests__/markdown-utils.spec.ts | 4 +- src/utils/file-upload-utils.js | 30 +++++++-- 8 files changed, 167 insertions(+), 42 deletions(-) create mode 100644 src/utils/__tests__/file-upload-utils.spec.ts diff --git a/package-lock.json b/package-lock.json index fba4180ed..46c23cc1a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,6 +18,7 @@ "@octokit/rest": "^18.12.0", "@opengovsg/formsg-sdk": "^0.11.0", "@opengovsg/sgid-client": "^2.0.0", + "@types/dompurify": "^3.0.5", "auto-bind": "^4.0.0", "aws-lambda": "^1.0.7", "aws-sdk": "^2.1428.0", @@ -37,6 +38,7 @@ "crypto-js": "^4.2.0", "dd-trace": "^4.7.0", "debug": "~2.6.9", + "dompurify": "^3.0.9", "dotenv": "^16.3.1", "eventsource": "^2.0.2", "exponential-backoff": "^3.1.0", @@ -4456,9 +4458,9 @@ } }, "node_modules/@types/dompurify": { - "version": "2.4.0", - "resolved": "https://registry.npmjs.org/@types/dompurify/-/dompurify-2.4.0.tgz", - "integrity": "sha512-IDBwO5IZhrKvHFUl+clZxgf3hn2b/lU6H1KaBShPkQyGJUQ0xwebezIPSuiyGwfz1UzJWQl4M7BDxtHtCCPlTg==", + "version": "3.0.5", + "resolved": "https://registry.npmjs.org/@types/dompurify/-/dompurify-3.0.5.tgz", + "integrity": "sha512-1Wg0g3BtQF7sSb27fJQAKck1HECM6zV1EB66j8JH9i3LCjYabJa0FSdiSgsD5K/RbrsR0SiraKacLB+T8ZVYAg==", "dependencies": { "@types/trusted-types": "*" } @@ -7056,6 +7058,11 @@ "node": ">=8" } }, + "node_modules/dompurify": { + "version": "3.0.9", + "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.0.9.tgz", + "integrity": "sha512-uyb4NDIvQ3hRn6NiC+SIFaP4mJ/MdXlvtunaqK9Bn6dD3RuB/1S/gasEjDHD8eiaqdSael2vBv+hOs7Y+jhYOQ==" + }, "node_modules/dotenv": { "version": "16.3.1", "resolved": "https://registry.npmjs.org/dotenv/-/dotenv-16.3.1.tgz", @@ -9731,6 +9738,14 @@ "node": ">= 10" } }, + "node_modules/isomorphic-dompurify/node_modules/@types/dompurify": { + "version": "2.4.0", + "resolved": "https://registry.npmjs.org/@types/dompurify/-/dompurify-2.4.0.tgz", + "integrity": "sha512-IDBwO5IZhrKvHFUl+clZxgf3hn2b/lU6H1KaBShPkQyGJUQ0xwebezIPSuiyGwfz1UzJWQl4M7BDxtHtCCPlTg==", + "dependencies": { + "@types/trusted-types": "*" + } + }, "node_modules/isomorphic-dompurify/node_modules/acorn": { "version": "8.10.0", "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.10.0.tgz", diff --git a/package.json b/package.json index 828791cf2..7451cdc3b 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "@octokit/rest": "^18.12.0", "@opengovsg/formsg-sdk": "^0.11.0", "@opengovsg/sgid-client": "^2.0.0", + "@types/dompurify": "^3.0.5", "auto-bind": "^4.0.0", "aws-lambda": "^1.0.7", "aws-sdk": "^2.1428.0", @@ -53,6 +54,7 @@ "crypto-js": "^4.2.0", "dd-trace": "^4.7.0", "debug": "~2.6.9", + "dompurify": "^3.0.9", "dotenv": "^16.3.1", "eventsource": "^2.0.2", "exponential-backoff": "^3.1.0", diff --git a/src/services/fileServices/MdPageServices/MediaFileService.js b/src/services/fileServices/MdPageServices/MediaFileService.js index 5a401c093..4e2d1742c 100644 --- a/src/services/fileServices/MdPageServices/MediaFileService.js +++ b/src/services/fileServices/MdPageServices/MediaFileService.js @@ -41,18 +41,31 @@ class MediaFileService { throw new BadRequestError("File did not pass virus scan") } } + // Sanitize and validate file - const sanitizedContent = await validateAndSanitizeFileUpload(content) - if (!sanitizedContent) { + const sanitisationResult = await validateAndSanitizeFileUpload(content) + if (!sanitisationResult) { throw new MediaTypeError(`File extension is not within the approved list`) } + + const { + content: sanitizedContent, + detectedFileType: { ext }, + } = sanitisationResult + // NOTE: We construct the extension based off what we detect as the file type + const constructedFileName = `${fileName + .split(".") + .slice(0, -1) + .join(".")}.${ext}` + const { sha } = await this.repoService.create(sessionData, { content: sanitizedContent, - fileName, + fileName: constructedFileName, directoryName, isMedia: true, }) - return { name: fileName, content, sha } + + return { name: constructedFileName, content, sha } } async read(sessionData, { fileName, directoryName }) { @@ -64,10 +77,17 @@ class MediaFileService { async update(sessionData, { fileName, directoryName, content, sha }) { this.mediaNameChecks({ directoryName, fileName }) - const sanitizedContent = await validateAndSanitizeFileUpload(content) - if (!sanitizedContent) { + const sanitisationResult = await validateAndSanitizeFileUpload(content) + if (!sanitisationResult) { throw new MediaTypeError(`File extension is not within the approved list`) } + const { + content: sanitizedContent, + detectedFileType: { ext }, + } = sanitisationResult + + // NOTE: We can trust the user input here + // as we are removing stuff from our system. await this.repoService.delete(sessionData, { sha, fileName, @@ -75,7 +95,7 @@ class MediaFileService { }) const { sha: newSha } = await this.repoService.create(sessionData, { content: sanitizedContent, - fileName, + fileName: `${fileName.split(".").slice(0, -1).join(".")}.${ext}`, directoryName, isMedia: true, }) diff --git a/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js b/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js index 6319ba6cc..df4cb9175 100644 --- a/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js +++ b/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js @@ -2,6 +2,10 @@ const { config } = require("@config/config") const { BadRequestError } = require("@errors/BadRequestError") +const { + MediaFileService, +} = require("@services/fileServices/MdPageServices/MediaFileService") + const GITHUB_ORG_NAME = config.get("github.orgName") describe("Media File Service", () => { @@ -9,13 +13,14 @@ describe("Media File Service", () => { const accessToken = "test-token" const imageName = "test image.png" const imageEncodedName = encodeURIComponent(imageName) - const fileName = "test file.pdf" + const fileName = "test file.jpg" const fileEncodedName = encodeURIComponent(fileName) const directoryName = "images/subfolder" - const mockContent = "schema, test" - const mockSanitizedContent = "sanitized-test" const sha = "12345" const mockGithubSessionData = "githubData" + const mockContent = + "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wCEAAkGBxMHEBMRBxMRFhUXFhgPGBAWFRYdFxIXFxYWFxUVFhMYHiogGBolGxgVITEiJikrOjEuFyA1OzMsNygtLisBCjxoMj5oZWxsbzwvaDI+Cg==" + const mockSanitizedContent = mockContent.split(",")[1] const sessionData = { siteName, accessToken } @@ -34,22 +39,16 @@ describe("Media File Service", () => { updateRepoState: jest.fn(), } + // NOTE: Mock just the scan function + // as we want to omit network calls. jest.mock("@utils/file-upload-utils", () => ({ - validateAndSanitizeFileUpload: jest - .fn() - .mockReturnValue(mockSanitizedContent), - ALLOWED_FILE_EXTENSIONS: ["pdf"], + ...jest.requireActual("@utils/file-upload-utils"), scanFileForVirus: jest.fn().mockReturnValue({ CleanResult: true }), })) - const { - MediaFileService, - } = require("@services/fileServices/MdPageServices/MediaFileService") - const service = new MediaFileService({ repoService: mockRepoService, }) - const { validateAndSanitizeFileUpload } = require("@utils/file-upload-utils") beforeEach(() => { jest.clearAllMocks() @@ -66,15 +65,16 @@ describe("Media File Service", () => { ).rejects.toThrowError(BadRequestError) }) - mockRepoService.create.mockResolvedValueOnce({ sha }) it("Creating pages works correctly", async () => { - await expect( - service.create(sessionData, { - fileName, - directoryName, - content: mockContent, - }) - ).resolves.toMatchObject({ + // Arrange + mockRepoService.create.mockResolvedValueOnce({ sha }) + + const result = await service.create(sessionData, { + fileName, + directoryName, + content: mockContent, + }) + expect(result).toMatchObject({ name: fileName, content: mockContent, sha, @@ -85,7 +85,24 @@ describe("Media File Service", () => { directoryName, isMedia: true, }) - expect(validateAndSanitizeFileUpload).toHaveBeenCalledWith(mockContent) + }) + + it("should ignore the extension provided by the user", async () => { + // Arrange + mockRepoService.create.mockResolvedValueOnce({ sha }) + const fileNameWithWrongExt = "wrong.html" + + // Act + const result = await service.create(sessionData, { + fileName: fileNameWithWrongExt, + directoryName, + content: mockContent, + }) + + // Assert + // NOTE: The original extension here is not used. + // Instead, we use the inferred extension. + expect(result.name).toBe("wrong.jpg") }) }) @@ -153,8 +170,8 @@ describe("Media File Service", () => { describe("Update", () => { const oldSha = "54321" - mockRepoService.create.mockResolvedValueOnce({ sha }) it("Updating media file content works correctly", async () => { + mockRepoService.create.mockResolvedValueOnce({ sha }) await expect( service.update(sessionData, { fileName, @@ -179,7 +196,6 @@ describe("Media File Service", () => { directoryName, isMedia: true, }) - expect(validateAndSanitizeFileUpload).toHaveBeenCalledWith(mockContent) }) }) @@ -197,7 +213,7 @@ describe("Media File Service", () => { }) describe("Rename", () => { - const oldFileName = "test old file.pdf" + const oldFileName = "test old file.jpg" it("rejects renaming to page names with special characters", async () => { await expect( diff --git a/src/services/utilServices/Sanitizer.ts b/src/services/utilServices/Sanitizer.ts index d3e744f80..4db87076d 100644 --- a/src/services/utilServices/Sanitizer.ts +++ b/src/services/utilServices/Sanitizer.ts @@ -1,7 +1,7 @@ import DOMPurify from "isomorphic-dompurify" DOMPurify.setConfig({ - ADD_TAGS: ["iframe", "#comment", "script"], + ADD_TAGS: ["iframe", "script"], ADD_ATTR: [ "allow", "allowfullscreen", diff --git a/src/utils/__tests__/file-upload-utils.spec.ts b/src/utils/__tests__/file-upload-utils.spec.ts new file mode 100644 index 000000000..754f6066b --- /dev/null +++ b/src/utils/__tests__/file-upload-utils.spec.ts @@ -0,0 +1,52 @@ +import { validateAndSanitizeFileUpload } from "../file-upload-utils" + +describe("file-upload-utils", () => { + it("should preserve the original file type for binary format files", async () => { + // Arrange + const content = + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAIAAAAC6CAYAAABm30UAAAAMO2lDQ1BJQ0MgUHJvZmlsZQAASImVVwdYU8kWnltSIbQAAlJCb4J0AkgJoQWQXgQbIQkQSoyBoGJHFxVcu1jAhq6KKHZA7IidRbD3xYKAsi4W7MqbFNB1X/ne5JuZP/+c+c+Zc+eWAUD9FFcszkU1AMgTFUjiQgMZY1JSGaQuQIY/DKBAk8vLF7NiYiIBLIP938u7mwCR9dccZFr/HP+vRZMvyOcBgMRAnM7P5+VBfAgAvJInlhQAQJTx5lMKxDIMK9CWwAAhXijDmQpcKcPpCrxPbpMQx4a4GQCyKpcryQRArQ3yjEJeJtRQ64PYScQXigBQZ0Dsl5c3iQ9xGsQ20EYMsUyfmf6DTubfNNOHNLnczCGsWIu8kIOE+eJc7rT/Mx3/u+TlSgd9WMGqmiUJi5OtGebtds6kCBlWhbhXlB4VDbEWxB+EfLk9xCg1SxqWqLBHDXn5bJgzoAuxE58bFAGxIcQhotyoSCWfniEM4UAMdwg6VVjASYBYD+KFgvzgeKXNZsmkOKUvtC5DwmYp+QtcidyvzNdDaU4iS6n/OkvAUepjakVZCckQUyG2KBQmRUGsBrFjfk58hNJmVFEWO2rQRiKNk8VvAXGcQBQaqNDHCjMkIXFK+9K8/MH1YpuzhJwoJT5QkJUQpsgP1szjyuOHa8HaBCJW4qCOIH9M5OBa+IKgYMXasW6BKDFeqfNBXBAYp5iLU8W5MUp73EyQGyrjzSB2yy+MV87FkwrghlTo4xnigpgERZx4UTY3PEYRD74MRAI2CAIMIIU1HUwC2UDY2lvfC/8pRkIAF0hAJhAAByUzOCNZPiKCbTwoAn9CJAD5Q/MC5aMCUAj5r0OsonUAGfLRQvmMHPAM4jwQAXLhf6l8lmjIWxJ4ChnhP7xzYeXBeHNhlY3/e36Q/c6wIBOpZKSDHhnqg5bEYGIQMYwYQrTFDXA/3AePhG0ArC44E/caXMd3e8IzQjvhMeEGoYNwZ6KwWPJTlKNBB9QPUeYi/cdc4FZQ0x0PxH2hOlTGdXED4IC7QT8s3B96docsWxm3LCuMn7T/toIfrobSjuJEQSnDKAEUm59nqtmpuQ+pyHL9Y34UsaYP5Zs9NPKzf/YP2efDPuJnS2whdhA7j53GLmLHsHrAwE5iDVgLdlyGh3bXU/nuGvQWJ48nB+oI/+Fv8MrKMpnvVOPU4/RFMVYgmCp7RgP2JPE0iTAzq4DBgm8EAYMj4jmOYLg4ubgCIHu/KB5fb2Ll7w1Et+U7N+8PAHxPDgwMHP3OhZ8EYL8nvP2PfOdsmPDVoQLAhSM8qaRQweGyhgCfEurwTtMHxsAc2MD1uAAP4AMCQDAIB9EgAaSACTD6LLjPJWAKmAHmghJQBpaB1WA92AS2gp1gDzgA6sExcBqcA5dBG7gB7sHd0wlegD7wDnxGEISE0BA6oo+YIJaIPeKCMBE/JBiJROKQFCQNyUREiBSZgcxDypAVyHpkC1KN7EeOIKeRi0g7cgd5hPQgr5FPKIaqotqoEWqFjkSZKAuNQBPQ8WgmOhktQuejS9C1aBW6G61DT6OX0RtoB/oC7ccApoLpYqaYA8bE2Fg0loplYBJsFlaKlWNVWC3WCK/zNawD68U+4kScjjNwB7iDw/BEnIdPxmfhi/H1+E68Dm/Gr+GP8D78G4FGMCTYE7wJHMIYQiZhCqGEUE7YTjhMOAvvpU7COyKRqEu0JnrCezGFmE2cTlxM3EDcSzxFbCc+IfaTSCR9kj3JlxRN4pIKSCWkdaTdpJOkq6RO0geyCtmE7EIOIaeSReRicjl5F/kE+Sq5i/yZokGxpHhToil8yjTKUso2SiPlCqWT8pmqSbWm+lITqNnUudS11FrqWep96hsVFRUzFS+VWBWhyhyVtSr7VC6oPFL5qKqlaqfKVh2nKlVdorpD9ZTqHdU3NBrNihZAS6UV0JbQqmlnaA9pH9Toao5qHDW+2my1CrU6tatqL9Up6pbqLPUJ6kXq5eoH1a+o92pQNKw02BpcjVkaFRpHNG5p9GvSNZ01ozXzNBdr7tK8qNmtRdKy0grW4mvN19qqdUbrCR2jm9PZdB59Hn0b/Sy9U5uoba3N0c7WLtPeo92q3aejpeOmk6QzVadC57hOhy6ma6XL0c3VXap7QPem7qdhRsNYwwTDFg2rHXZ12Hu94XoBegK9Ur29ejf0Pukz9IP1c/SX69frPzDADewMYg2mGGw0OGvQO1x7uM9w3vDS4QeG3zVEDe0M4wynG241bDHsNzI2CjUSG60zOmPUa6xrHGCcbbzK+IRxjwndxM9EaLLK5KTJc4YOg8XIZaxlNDP6TA1Nw0ylpltMW00/m1mbJZoVm+01e2BONWeaZ5ivMm8y77MwsRhtMcOixuKuJcWSaZllucbyvOV7K2urZKsFVvVW3dZ61hzrIusa6/s2NBt/m8k2VTbXbYm2TNsc2w22bXaonbtdll2F3RV71N7DXmi/wb59BGGE1wjRiKoRtxxUHVgOhQ41Do8cdR0jHYsd6x1fjrQYmTpy+cjzI785uTvlOm1zuues5RzuXOzc6Pzaxc6F51Lhct2V5hriOtu1wfWVm72bwG2j2213uvto9wXuTe5fPTw9JB61Hj2eFp5pnpWet5jazBjmYuYFL4JXoNdsr2NeH709vAu8D3j/5ePgk+Ozy6d7lPUowahto574mvlyfbf4dvgx/NL8Nvt1+Jv6c/2r/B8HmAfwA7YHdLFsWdms3ayXgU6BksDDge/Z3uyZ7FNBWFBoUGlQa7BWcGLw+uCHIWYhmSE1IX2h7qHTQ0+FEcIiwpaH3eIYcXicak5fuGf4zPDmCNWI+Ij1EY8j7SIlkY2j0dHho1eOvh9lGSWKqo8G0ZzoldEPYqxjJsccjSXGxsRWxD6Lc46bEXc+nh4/MX5X/LuEwISlCfcSbRKliU1J6knjkqqT3icHJa9I7hgzcszMMZdTDFKEKQ2ppNSk1O2p/WODx64e2znOfVzJuJvjrcdPHX9xgsGE3AnHJ6pP5E48mEZIS07blfaFG82t4vanc9Ir0/t4bN4a3gt+AH8Vv0fgK1gh6MrwzViR0Z3pm7kysyfLP6s8q1fIFq4XvsoOy96U/T4nOmdHzkBucu7ePHJeWt4RkZYoR9Q8yXjS1EntYntxibhjsvfk1ZP7JBGS7flI/vj8hgJt+CHfIrWR/iJ9VOhXWFH4YUrSlINTNaeKprZMs5u2aFpXUUjRb9Px6bzpTTNMZ8yd8Wgma+aWWcis9FlNs81nz5/dOSd0zs651Lk5c38vdipeUfx2XvK8xvlG8+fMf/JL6C81JWolkpJbC3wWbFqILxQubF3kumjdom+l/NJLZU5l5WVfFvMWX/rV+de1vw4syVjSutRj6cZlxGWiZTeX+y/fuUJzRdGKJytHr6xbxVhVuurt6omrL5a7lW9aQ10jXdOxNnJtwzqLdcvWfVmftf5GRWDF3krDykWV7zfwN1zdGLCxdpPRprJNnzYLN9/eErqlrsqqqnwrcWvh1mfbkrad/435W/V2g+1l27/uEO3o2Bm3s7nas7p6l+GupTVojbSmZ/e43W17gvY01DrUbtmru7dsH9gn3fd8f9r+mwciDjQdZB6sPWR5qPIw/XBpHVI3ra6vPqu+oyGlof1I+JGmRp/Gw0cdj+44Znqs4rjO8aUnqCfmnxg4WXSy/5T4VO/pzNNPmiY23Tsz5sz15tjm1rMRZy+cCzl35jzr/MkLvheOXfS+eOQS81L9ZY/LdS3uLYd/d//9cKtHa90VzysNbV5tje2j2k9c9b96+lrQtXPXOdcv34i60X4z8ebtW+Nuddzm3+6+k3vn1d3Cu5/vzblPuF/6QONB+UPDh1V/2P6xt8Oj4/ijoEctj+Mf33vCe/Liaf7TL53zn9GelXeZdFV3u3Qf6wnpaXs+9nnnC/GLz70lf2r+WfnS5uWhvwL+aukb09f5SvJq4PXiN/pvdrx1e9vUH9P/8F3eu8/vSz/of9j5kfnx/KfkT12fp3whfVn71fZr47eIb/cH8gYGxFwJV/4pgMGKZmQA8HoHALQUAOjwfEYdqzj/yQuiOLPKEfhPWHFGlBcPAGrh93tsL/y6uQXAvm3w+AX11ccBEEMDIMELoK6uQ3XwrCY/V8oKEZ4DNkd9Tc9LB/+mKM6cP8T9cw9kqm7g5/5fxPh8LqaJ18oAAACKZVhJZk1NACoAAAAIAAQBGgAFAAAAAQAAAD4BGwAFAAAAAQAAAEYBKAADAAAAAQACAACHaQAEAAAAAQAAAE4AAAAAAAAAkAAAAAEAAACQAAAAAQADkoYABwAAABIAAAB4oAIABAAAAAEAAACAoAMABAAAAAEAAAC6AAAAAEFTQ0lJAAAAU2NyZWVuc2hvdCoD2FIAAAAJcEhZcwAAFiUAABYlAUlSJPAAAAHWaVRYdFhNTDpjb20uYWRvYmUueG1wAAAAAAA8eDp4bXBtZXRhIHhtbG5zOng9ImFkb2JlOm5zOm1ldGEvIiB4OnhtcHRrPSJYTVAgQ29yZSA2LjAuMCI+CiAgIDxyZGY6UkRGIHhtbG5zOnJkZj0iaHR0cDovL3d3dy53My5vcmcvMTk5OS8wMi8yMi1yZGYtc3ludGF4LW5zIyI+CiAgICAgIDxyZGY6RGVzY3JpcHRpb24gcmRmOmFib3V0PSIiCiAgICAgICAgICAgIHhtbG5zOmV4aWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20vZXhpZi8xLjAvIj4KICAgICAgICAgPGV4aWY6UGl4ZWxZRGltZW5zaW9uPjE4NjwvZXhpZjpQaXhlbFlEaW1lbnNpb24+CiAgICAgICAgIDxleGlmOlBpeGVsWERpbWVuc2lvbj4xMjg8L2V4aWY6UGl4ZWxYRGltZW5zaW9uPgogICAgICAgICA8ZXhpZjpVc2VyQ29tbWVudD5TY3JlZW5zaG90PC9leGlmOlVzZXJDb21tZW50PgogICAgICA8L3JkZjpEZXNjcmlwdGlvbj4KICAgPC9yZGY6UkRGPgo8L3g6eG1wbWV0YT4KvhKblQAAABxpRE9UAAAAAgAAAAAAAABdAAAAKAAAAF0AAABdAAACymm9DPkAAAKWSURBVHgB7JoBasJQFAT15HpJz5Mm0g+lBJufZGnmvwmUSKvbzc6gtXif5uPmUXaBuwKUZf++cAWozf+mAArg3wCVHfAZoDL9+doVQAF8CajsgM8Alen7ElCcvgIogC8BxR1QAAXwXUBlB7qeAV6vV+Wthrx2BRgS6/aL6hJge6z3pCygABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngoQGpYSqwAUUqGeChAalhKrABRSoZ4KEBqWEqsAFFKhngrwPezz+XzfaufQ3teLnTymx+MxzWTeX8vtSset0sWuXetP+BUlKC3AGvxqEpQV4BP8ShKUFGAL/CoSlBOgB34FCUoJsAf+6BKUEeAv+MvPt9xn7Z0E+XslBOgB23NfMvjWfXgB9gDd85g2KO08tABHQB55LEmCYQU4A+AZGVeXYUgBzgR3ZtYVZSgnwAK09/gkwZ683t+fvP+QAiyDrUE7AuvsvCTUnuxhBfgtwRH4bdCfEpyR13L/8zz8B0LaBzzaef7P3qGj5bTzobALPHh4AS6w8aUrfAEAAP//IRJFrAAAAYpJREFU7dfBDYNADERR6L8+6iFWOtjDHpb/co4izDx54vudz+WTfQM3ANns/4MD0M7/AgAA/wHKBmyAcvozOwAAqICyARugnL4KiKcPAAAqIG4AAABcAWUDNkA5/ZkdAABUQNmADVBOXwXE0wcAABUQNwAAAK6AsgEboJz+zA4AACqgbMAGKKevAuLpAwCACogbAAAAV0DZgA1QTn9mBwAAFVA2YAOU01cB8fQBAEAFxA0AAIAroGzABiinP7MDAIAKKBuwAcrpq4B4+gAAoALiBgAAwBVQNmADlNOf2QEAQAWUDdgA5fRVQDx9AABQAXEDAADgCigbsAHK6c/sAACgAsoGbIBy+iognj4AAKiAuAEAAHAFlA3YAOX0Z3YAAFABZQM2QDl9FRBPHwAAVEDcAAAAuALKBmyAcvozOwAAqICyARugnL4KiKcPAAAqIG4AAABcAWUDNkA5/Zl9CcDzPPHX9b3xAfhepksTLQFY+mVfPuINAHBETPseEoB97/aIXwbgiJj2PeQP2QBly+h6jzkAAAAASUVORK5CYII=" + const expectedExtension = "png" + + // Act + const result = await validateAndSanitizeFileUpload(content) + + // Assert + expect(result).toBeDefined() + expect(result?.detectedFileType.ext).toBe(expectedExtension) + }) + + it("should sanitize svgs with alerts", async () => { + // Arrange + // NOTE: This is a svg with an `alert` in html comments + const maliciousContent = `data:application/pdf;base64,PCEtLQphbGVydChkb2N1bWVudC5kb21haW4pCi8qCi0tPgo8c3ZnIGlkPSJzdmcyIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCA5MDAgOTAwIiB2ZXJzaW9uPSIxLjEiPgo8L3N2Zz4KPCEtLSAqLyAKLS0+Cg==` + const expectedExtension = "svg" + + // Act + const result = await validateAndSanitizeFileUpload(maliciousContent) + const sanitisedContent = Buffer.from(result?.content, "base64").toString() + + // Assert + expect(result).toBeDefined() + expect(result?.detectedFileType.ext).toBe(expectedExtension) + expect(sanitisedContent).not.toContain("alert") + }) + + it("should sanitize svgs with scripts", async () => { + // Arrange + // NOTE: Our sanitizer for files/html allows script tag + // however, the svg sanitizer does not as it is stricter + // and to prevent xss attacks. + const maliciousContent = `data:image/png;base64,${Buffer.from( + `` + ).toString("base64")}` + const expectedExtension = "svg" + + // Act + const result = await validateAndSanitizeFileUpload(maliciousContent) + + // Assert + expect(result).toBeDefined() + expect(result?.detectedFileType.ext).toBe(expectedExtension) + expect(result).not.toContain("script") + }) +}) diff --git a/src/utils/__tests__/markdown-utils.spec.ts b/src/utils/__tests__/markdown-utils.spec.ts index 85101269c..9475ea8ad 100644 --- a/src/utils/__tests__/markdown-utils.spec.ts +++ b/src/utils/__tests__/markdown-utils.spec.ts @@ -103,8 +103,8 @@ describe("Sanitized markdown utils test", () => { ) }) - it("should inject a html comment tag when the string is empty", () => { - expect(sanitizer.sanitize("")).toBe(HTML_COMMENT_TAG) + it("should not perform sanitisation when the string is empty", () => { + expect(sanitizer.sanitize("")).toBe("") }) }) }) diff --git a/src/utils/file-upload-utils.js b/src/utils/file-upload-utils.js index e00291ded..d77170b85 100644 --- a/src/utils/file-upload-utils.js +++ b/src/utils/file-upload-utils.js @@ -1,12 +1,14 @@ const CloudmersiveVirusApiClient = require("cloudmersive-virus-api-client") +const DOMPurify = require("dompurify") const FileType = require("file-type") const isSvg = require("is-svg") +const { JSDOM } = require("jsdom") const { config } = require("@config/config") -const logger = require("@logger/logger").default +const { window } = new JSDOM("") -const { sanitizer } = require("@services/utilServices/Sanitizer") +const logger = require("@logger/logger").default const CLOUDMERSIVE_API_KEY = config.get("cloudmersiveKey") @@ -31,6 +33,12 @@ apikey.apiKey = CLOUDMERSIVE_API_KEY const apiInstance = new CloudmersiveVirusApiClient.ScanApi() +// NOTE: This is NOT the default sanitiser; +// instead, we are creaitng our own instance of DOMPurify +// so that we can make it stricter solely +// for fileuploads. +const sanitizer = DOMPurify(window) + const scanFileForVirus = (fileBuffer, timeout) => { if (timeout) { defaultCloudmersiveClient.timeout = timeout @@ -54,16 +62,28 @@ const validateAndSanitizeFileUpload = async (data) => { const [, content] = data.split(",") const fileBuffer = Buffer.from(content, "base64") const detectedFileType = await FileType.fromBuffer(fileBuffer) - + // NOTE: This check is required for svg files. + // This is because svg files are a string based data type + // and not binary based. + // Hence, `FileType` wouldn't be able to detect the correct + // file type for svg files. if (isSvg(fileBuffer)) { + // NOTE: `isSvg` checks only using the first element, + // which is insufficient to guarantee that this file is safe. + // We run it thru the sanitizer again to ensure that the output + // is safe. const sanitizedBuffer = sanitizer.sanitize(fileBuffer) - return Buffer.from(sanitizedBuffer, "utf8").toString("base64") + return { + content: Buffer.from(sanitizedBuffer, "utf8").toString("base64"), + detectedFileType: { ext: "svg" }, + } } + if ( detectedFileType && ALLOWED_FILE_EXTENSIONS.includes(detectedFileType.ext) ) { - return content + return { content, detectedFileType } } return undefined From 56b3dbde2aa9a22c27503b4e0f33f443f49ad1dd Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Thu, 22 Feb 2024 15:47:58 +0800 Subject: [PATCH 6/8] fix(dompurify): further limit src (#1156) * fix(dompurify): further limit src * fix(dompurify): rm unallowed src * fix(scriptTag): disallow href --- src/services/utilServices/Sanitizer.ts | 29 ++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/services/utilServices/Sanitizer.ts b/src/services/utilServices/Sanitizer.ts index 4db87076d..42bd67b6b 100644 --- a/src/services/utilServices/Sanitizer.ts +++ b/src/services/utilServices/Sanitizer.ts @@ -1,5 +1,17 @@ import DOMPurify from "isomorphic-dompurify" +/** + * While we do have a CSP in place, we want to further restrict the content that + * that the user can input. + * NOTE: Any changes to this list should also be updated in frontend code + */ +const ALLOWED_SRC = [ + "//www.instagram.com/embed.js", + "/jquery/resize-tables.js", + "/jquery/jquery.min.js", + "/jquery/bp-menu-new-tab.js", +] + DOMPurify.setConfig({ ADD_TAGS: ["iframe", "script"], ADD_ATTR: [ @@ -17,11 +29,20 @@ DOMPurify.setConfig({ }) DOMPurify.addHook("uponSanitizeElement", (node, data) => { // Allow script tags if it has a src attribute - // Script sources are handled by our CSP sanitiser - if ( + + const hasUnallowedSrcValue = data.tagName === "script" && - !(node.hasAttribute("src") && node.innerHTML === "") - ) { + !( + node.hasAttribute("src") && + node.innerHTML === "" && + ALLOWED_SRC.includes(node.getAttribute("src") ?? "") + ) + + const hasUnallowedScriptAttribute = + data.tagName === "script" && + (node.hasAttribute("href") || node.hasAttribute("xlink:href")) + + if (hasUnallowedSrcValue || hasUnallowedScriptAttribute) { // Adapted from https://github.com/cure53/DOMPurify/blob/e0970d88053c1c564b6ccd633b4af7e7d9a10375/src/purify.js#L719-L736 DOMPurify.removed.push({ element: node }) try { From 18568589d039b29a7e1892348dad532f248eab4c Mon Sep 17 00:00:00 2001 From: seaerchin <44049504+seaerchin@users.noreply.github.com> Date: Thu, 22 Feb 2024 16:46:40 +0800 Subject: [PATCH 7/8] fix(start-commands): update commands to be different by env (#1159) * fix(start-commands): update commands to be different by env * fix(package): remove unsupported option * fix(ts-node): update commadn to use node intsead --- Dockerfile | 3 ++- package.json | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 2da4bd204..c104d82e5 100644 --- a/Dockerfile +++ b/Dockerfile @@ -24,6 +24,7 @@ RUN echo "[user]" > /root/.gitconfig RUN echo " name = Isomer Admin" >> /root/.gitconfig RUN echo " email = admin@isomer.gov.sg" >> /root/.gitconfig +RUN chmod +x ./scripts/02_fetch_ssh_keys.sh EXPOSE "8081" -CMD ["bash", "-c", "chmod +x ./scripts/02_fetch_ssh_keys.sh && bash ./scripts/02_fetch_ssh_keys.sh & npm run start:ecs"] +CMD ["bash", "-c", "bash ./scripts/02_fetch_ssh_keys.sh & npm run start:ecs:$NODE_ENV"] diff --git a/package.json b/package.json index 7451cdc3b..14969f254 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,9 @@ "scripts": { "build": "tsc -p tsconfig.build.json", "start": "node --unhandled-rejections=warn -r ts-node/register/transpile-only -r tsconfig-paths/register -r dotenv/config build/server.js dotenv_config_path=/efs/isomer/.isomer.env", - "start:ecs": "ts-node-dev --unhandled-rejections=warn --respawn src/server.js", + "start:ecs:prod": "node --unhandled-rejections=warn -r ts-node/register/transpile-only -r tsconfig-paths/register src/server.js", + "start:ecs:staging": "node --unhandled-rejections=warn -r ts-node/register/transpile-only -r tsconfig-paths/register src/server.js", + "start::ecs:dev": "ts-node-dev --respawn src/server.js", "dev": "source .env && docker compose -f docker-compose.dev.yml up", "test:docker": "docker run -d -p 54321:5432 --name postgres -e POSTGRES_USER=isomer -e POSTGRES_PASSWORD=password -e POSTGRES_DB=isomercms_test postgres:latest", "test": "source .env.test && jest --runInBand", From 4390e5f5ef08d592359049342fc191c7a2c36742 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Thu, 22 Feb 2024 16:47:42 +0800 Subject: [PATCH 8/8] 0.67.0 --- CHANGELOG.md | 28 +++++++++++++++++++++++----- package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f54c460d..2fdebb0b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,28 @@ All notable changes to this project will be documented in this file. Dates are d Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). +#### [v0.67.0](https://github.com/isomerpages/isomercms-backend/compare/v0.66.4...v0.67.0) + +- fix(start-commands): update commands to be different by env [`#1159`](https://github.com/isomerpages/isomercms-backend/pull/1159) +- fix(dompurify): further limit src [`#1156`](https://github.com/isomerpages/isomercms-backend/pull/1156) +- fix(file-ext): prevent users from bypassing checks on file extensions [`#1157`](https://github.com/isomerpages/isomercms-backend/pull/1157) +- Chore/lock repos when repairing [`#1149`](https://github.com/isomerpages/isomercms-backend/pull/1149) +- Fix/sanitise urls [`#1158`](https://github.com/isomerpages/isomercms-backend/pull/1158) +- feat: add validation for homepage frontmatter [`#1151`](https://github.com/isomerpages/isomercms-backend/pull/1151) +- build(deps-dev): bump ip from 2.0.0 to 2.0.1 [`#1155`](https://github.com/isomerpages/isomercms-backend/pull/1155) +- release(0.66.4): merge to dev [`#1154`](https://github.com/isomerpages/isomercms-backend/pull/1154) +- release(0.66.3): merge to dev [`#1146`](https://github.com/isomerpages/isomercms-backend/pull/1146) +- Release/0.66.2 [`#1144`](https://github.com/isomerpages/isomercms-backend/pull/1144) +- release(0.66.1): merge to develop [`#1142`](https://github.com/isomerpages/isomercms-backend/pull/1142) +- 0.66.0 [`#1140`](https://github.com/isomerpages/isomercms-backend/pull/1140) +- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb) + #### [v0.66.4](https://github.com/isomerpages/isomercms-backend/compare/v0.66.3...v0.66.4) +> 20 February 2024 + - fix(ci): use workflwo [`a5a225d`](https://github.com/isomerpages/isomercms-backend/commit/a5a225dd203826726ad9f8e39c420bdb4a8e0d2c) -- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb) +- fix(tsak-def): add env vars [`ad02a2f`](https://github.com/isomerpages/isomercms-backend/commit/ad02a2f630d657b468bb48fd48f0572051ae85e3) #### [v0.66.3](https://github.com/isomerpages/isomercms-backend/compare/v0.66.2...v0.66.3) @@ -70,12 +88,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - fix: remove unnecessary push logs [`#1109`](https://github.com/isomerpages/isomercms-backend/pull/1109) - fix(rr): skip checking the existence of review request [`#1102`](https://github.com/isomerpages/isomercms-backend/pull/1102) - release/0.61.0 [`#1104`](https://github.com/isomerpages/isomercms-backend/pull/1104) +- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106) #### [v0.61.0](https://github.com/isomerpages/isomercms-backend/compare/v0.60.0...v0.61.0) -> 11 January 2024 +> 10 January 2024 -- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106) - chore: upgrade axios [`#1100`](https://github.com/isomerpages/isomercms-backend/pull/1100) - build(deps): bump follow-redirects from 1.15.2 to 1.15.4 [`#1101`](https://github.com/isomerpages/isomercms-backend/pull/1101) - fix(ci): reverts ci changes to allow staging updates [`#1084`](https://github.com/isomerpages/isomercms-backend/pull/1084) @@ -157,12 +175,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - fix(siteCreate): add redirect rules [`#1036`](https://github.com/isomerpages/isomercms-backend/pull/1036) - chore: remove extra and unused submodules [`#1031`](https://github.com/isomerpages/isomercms-backend/pull/1031) - release/0.54.0 [`#1033`](https://github.com/isomerpages/isomercms-backend/pull/1033) +- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035) #### [v0.54.0](https://github.com/isomerpages/isomercms-backend/compare/v0.53.0...v0.54.0) > 14 November 2023 -- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035) - fix(pagination): total length [`#1032`](https://github.com/isomerpages/isomercms-backend/pull/1032) - fix(staging-lite): apps were created for wrong br [`#1014`](https://github.com/isomerpages/isomercms-backend/pull/1014) - fix(cm): extra timeout [`#1027`](https://github.com/isomerpages/isomercms-backend/pull/1027) @@ -289,12 +307,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - build(deps-dev): bump @babel/traverse from 7.22.8 to 7.23.2 [`#984`](https://github.com/isomerpages/isomercms-backend/pull/984) - release/v0.48.0 [`#979`](https://github.com/isomerpages/isomercms-backend/pull/979) - feat(staging-id): add column to store the id [`#983`](https://github.com/isomerpages/isomercms-backend/pull/983) +- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978) #### [v0.48.0](https://github.com/isomerpages/isomercms-backend/compare/v0.47.0...v0.48.0) > 18 October 2023 -- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978) - chore(commitService): proper naming [`#975`](https://github.com/isomerpages/isomercms-backend/pull/975) - Feat/is 585 govt sgid login rollout [`#976`](https://github.com/isomerpages/isomercms-backend/pull/976) - test(quickie): unit tests [`#973`](https://github.com/isomerpages/isomercms-backend/pull/973) diff --git a/package-lock.json b/package-lock.json index 46c23cc1a..91c0bd143 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "isomercms", - "version": "0.66.4", + "version": "0.67.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "isomercms", - "version": "0.66.4", + "version": "0.67.0", "dependencies": { "@aws-sdk/client-amplify": "^3.370.0", "@aws-sdk/client-cloudwatch-logs": "^3.370.0", diff --git a/package.json b/package.json index 14969f254..d2410929c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.66.4", + "version": "0.67.0", "private": true, "scripts": { "build": "tsc -p tsconfig.build.json",