From 801c4ae5ac998c11c0542cccecba941ed368ff7f Mon Sep 17 00:00:00 2001 From: thamsimun <39295749+thamsimun@users.noreply.github.com> Date: Tue, 22 Aug 2023 10:32:10 +0800 Subject: [PATCH 1/2] fix: stored xss via file upload (#2258) * fix: stored xss via file upload * chore: refactor tests * fix: e2e test * fix: update relative changed file path in e2e --- src/server/api/user/index.ts | 6 +- src/server/constants.ts | 1 + src/server/inversify.config.ts | 8 +- .../modules/threat/FileCheckController.ts | 27 +++--- .../__tests__/FileCheckController.test.ts | 89 ++++++++++++++++--- .../interfaces/FileTypeFilterService.ts | 16 ++-- .../threat/services/FileTypeFilterService.ts | 39 +++++--- .../__tests__/FileTypeFilterService.test.ts | 88 +++++++++++------- test/end-to-end/UrlCreation.test.ts | 2 +- test/end-to-end/util/config.ts | 4 +- 10 files changed, 198 insertions(+), 82 deletions(-) diff --git a/src/server/api/user/index.ts b/src/server/api/user/index.ts index 8895619dc..7d37531b3 100644 --- a/src/server/api/user/index.ts +++ b/src/server/api/user/index.ts @@ -99,7 +99,7 @@ router.post( preprocessFormData, validator.body(urlSchema), fileCheckController.singleFileCheck, - fileCheckController.fileExtensionCheck(), + fileCheckController.fileExtensionAndMimeTypeCheck(), fileCheckController.fileVirusCheck, urlCheckController.singleUrlCheck, userController.createUrl, @@ -117,7 +117,7 @@ router.post( preprocessFormData, validator.body(urlBulkSchema), fileCheckController.singleFileCheck, - fileCheckController.fileExtensionCheck(['csv']), + fileCheckController.fileExtensionAndMimeTypeCheck(['csv']), fileCheckController.fileVirusCheck, bulkController.validateAndParseCsv, urlCheckController.bulkUrlCheck, @@ -144,7 +144,7 @@ router.patch( preprocessFormData, validator.body(urlEditSchema), fileCheckController.singleFileCheck, - fileCheckController.fileExtensionCheck(), + fileCheckController.fileExtensionAndMimeTypeCheck(), fileCheckController.fileVirusCheck, urlCheckController.singleUrlCheck, userController.updateUrl, diff --git a/src/server/constants.ts b/src/server/constants.ts index 3525c6b18..8cb9fd7fe 100644 --- a/src/server/constants.ts +++ b/src/server/constants.ts @@ -51,6 +51,7 @@ export const DependencyIds = { urlHistoryRepository: Symbol.for('urlHistoryRepository'), deviceCheckService: Symbol.for('deviceCheckService'), allowedFileExtensions: Symbol.for('allowedFileExtensions'), + fileExtensionsMimeTypeMap: Symbol.for('fileExtensionsMimeTypeMap'), fileTypeFilterService: Symbol.for('fileTypeFilterService'), cloudmersiveKey: Symbol.for('cloudmersiveKey'), cloudmersiveClient: Symbol.for('cloudmersiveClient'), diff --git a/src/server/inversify.config.ts b/src/server/inversify.config.ts index 08ab8e7fa..f0ea23481 100644 --- a/src/server/inversify.config.ts +++ b/src/server/inversify.config.ts @@ -72,7 +72,10 @@ import { UrlHistoryRepository } from './modules/audit/repositories' import { SafeBrowsingMapper } from './modules/threat/mappers' import { SafeBrowsingRepository } from './modules/threat/repositories/SafeBrowsingRepository' -import { DEFAULT_ALLOWED_FILE_EXTENSIONS } from './modules/threat/services/FileTypeFilterService' +import { + DEFAULT_ALLOWED_FILE_EXTENSIONS, + FILE_EXTENSION_MIME_TYPE_MAP, +} from './modules/threat/services/FileTypeFilterService' import { CloudmersiveScanService, FileTypeFilterService, @@ -153,6 +156,9 @@ export default () => { container .bind(DependencyIds.allowedFileExtensions) .toConstantValue(DEFAULT_ALLOWED_FILE_EXTENSIONS) + container + .bind(DependencyIds.fileExtensionsMimeTypeMap) + .toConstantValue(FILE_EXTENSION_MIME_TYPE_MAP) bindIfUnbound(DependencyIds.fileTypeFilterService, FileTypeFilterService) if (cloudmersiveKey) { diff --git a/src/server/modules/threat/FileCheckController.ts b/src/server/modules/threat/FileCheckController.ts index 30a9ab821..b77c8e9fa 100644 --- a/src/server/modules/threat/FileCheckController.ts +++ b/src/server/modules/threat/FileCheckController.ts @@ -43,19 +43,26 @@ export class FileCheckController { next() } - public fileExtensionCheck = + public fileExtensionAndMimeTypeCheck = (allowedExtensions?: string[]) => async (req: Request, res: Response, next: NextFunction): Promise => { const file = req.files?.file as fileUpload.UploadedFile | undefined - if ( - file && - !(await this.fileTypeFilterService.hasAllowedType( - file, - allowedExtensions, - )) - ) { - res.unsupportedMediaType(jsonMessage('File type disallowed.')) - return + + if (file) { + const fileTypeData = + await this.fileTypeFilterService.getExtensionAndMimeType(file) + if ( + fileTypeData.extension === '' || + !(await this.fileTypeFilterService.hasAllowedExtensionType( + fileTypeData.extension, + allowedExtensions, + )) + ) { + res.unsupportedMediaType(jsonMessage('File type disallowed.')) + return + } + + file.mimetype = fileTypeData.mimeType } next() diff --git a/src/server/modules/threat/__tests__/FileCheckController.test.ts b/src/server/modules/threat/__tests__/FileCheckController.test.ts index 74de75aa0..24e527efc 100644 --- a/src/server/modules/threat/__tests__/FileCheckController.test.ts +++ b/src/server/modules/threat/__tests__/FileCheckController.test.ts @@ -1,6 +1,7 @@ /* eslint-disable import/prefer-default-export */ import httpMocks from 'node-mocks-http' import { Request } from 'express' +import fileUpload from 'express-fileupload' import { FileCheckController } from '..' @@ -18,19 +19,20 @@ export function createRequestWithFile(file: any): Request { describe('FileCheckController test', () => { const file = { data: Buffer.from('data'), name: 'file.csv' } - const getExtension = jest.fn() - const hasAllowedType = jest.fn() + const getExtensionAndMimeType = jest.fn() + const hasAllowedExtensionType = jest.fn() const scanFile = jest.fn() const controller = new FileCheckController( - { getExtension, hasAllowedType }, + { getExtensionAndMimeType, hasAllowedExtensionType }, { scanFile }, ) const badRequest = jest.fn() beforeEach(() => { - hasAllowedType.mockClear() + hasAllowedExtensionType.mockClear() + getExtensionAndMimeType.mockClear() scanFile.mockClear() badRequest.mockClear() }) @@ -42,10 +44,15 @@ describe('FileCheckController test', () => { const afterFileExtensionCheck = jest.fn() const afterFileVirusCheck = jest.fn() await controller.singleFileCheck(req, res, afterSingleFileCheck) - await controller.fileExtensionCheck()(req, res, afterFileExtensionCheck) + await controller.fileExtensionAndMimeTypeCheck()( + req, + res, + afterFileExtensionCheck, + ) await controller.fileVirusCheck(req, res, afterFileVirusCheck) - expect(hasAllowedType).not.toHaveBeenCalled() + expect(hasAllowedExtensionType).not.toHaveBeenCalled() + expect(getExtensionAndMimeType).not.toHaveBeenCalled() expect(scanFile).not.toHaveBeenCalled() expect(afterSingleFileCheck).toHaveBeenCalled() expect(afterFileExtensionCheck).toHaveBeenCalled() @@ -72,12 +79,22 @@ describe('FileCheckController test', () => { const res = httpMocks.createResponse() as any const afterFileExtensionCheck = jest.fn() - hasAllowedType.mockResolvedValue(false) + getExtensionAndMimeType.mockResolvedValue({ + extension: 'svg', + mimeType: 'text/plain', + }) + hasAllowedExtensionType.mockResolvedValue(false) + res.unsupportedMediaType = badRequest - await controller.fileExtensionCheck()(req, res, afterFileExtensionCheck) + await controller.fileExtensionAndMimeTypeCheck()( + req, + res, + afterFileExtensionCheck, + ) - expect(hasAllowedType).toHaveBeenCalled() + expect(hasAllowedExtensionType).toHaveBeenCalled() + expect(getExtensionAndMimeType).toHaveBeenCalled() expect(res.unsupportedMediaType).toHaveBeenCalled() expect(afterFileExtensionCheck).not.toHaveBeenCalled() }) @@ -120,7 +137,12 @@ describe('FileCheckController test', () => { const afterFileExtensionCheck = jest.fn() const afterFileVirusCheck = jest.fn() - hasAllowedType.mockResolvedValue(true) + hasAllowedExtensionType.mockResolvedValue(true) + getExtensionAndMimeType.mockResolvedValue({ + extension: 'csv', + mimeType: 'text/csv', + }) + scanFile.mockResolvedValue(false) res.badRequest = badRequest @@ -129,11 +151,20 @@ describe('FileCheckController test', () => { res.unsupportedMediaType = badRequest await controller.singleFileCheck(req, res, afterSingleFileCheck) - await controller.fileExtensionCheck()(req, res, afterFileExtensionCheck) + await controller.fileExtensionAndMimeTypeCheck()( + req, + res, + afterFileExtensionCheck, + ) await controller.fileVirusCheck(req, res, afterFileVirusCheck) - - expect(hasAllowedType).toHaveBeenCalled() + expect(getExtensionAndMimeType).toHaveBeenCalled() + expect(hasAllowedExtensionType).toHaveBeenCalled() expect(scanFile).toHaveBeenCalled() + const fileUploaded = req.files?.file as fileUpload.UploadedFile | undefined + expect(fileUploaded).toBeDefined() + if (fileUploaded) { + expect(fileUploaded.mimetype).toEqual('text/csv') + } expect(res.badRequest).not.toHaveBeenCalled() expect(res.serverError).not.toHaveBeenCalled() @@ -144,4 +175,36 @@ describe('FileCheckController test', () => { expect(afterFileExtensionCheck).toHaveBeenCalled() expect(afterFileVirusCheck).toHaveBeenCalled() }) + + it('unsupportedMediaType when file extension is empty', async () => { + const req = createRequestWithFile(file) + const res = httpMocks.createResponse() as any + + const afterSingleFileCheck = jest.fn() + const afterFileExtensionCheck = jest.fn() + + scanFile.mockResolvedValue(false) + + res.badRequest = badRequest + res.serverError = badRequest + res.unprocessableEntity = badRequest + res.unsupportedMediaType = badRequest + + getExtensionAndMimeType.mockResolvedValue({ + extension: '', + mimeType: 'text/plain', + }) + + await controller.singleFileCheck(req, res, afterSingleFileCheck) + await controller.fileExtensionAndMimeTypeCheck()( + req, + res, + afterFileExtensionCheck, + ) + + expect(getExtensionAndMimeType).toHaveBeenCalled() + expect(hasAllowedExtensionType).not.toHaveBeenCalled() + expect(res.unsupportedMediaType).toHaveBeenCalled() + expect(afterFileExtensionCheck).not.toHaveBeenCalled() + }) }) diff --git a/src/server/modules/threat/interfaces/FileTypeFilterService.ts b/src/server/modules/threat/interfaces/FileTypeFilterService.ts index 8750ed0ac..a7b3bd275 100644 --- a/src/server/modules/threat/interfaces/FileTypeFilterService.ts +++ b/src/server/modules/threat/interfaces/FileTypeFilterService.ts @@ -1,16 +1,18 @@ export interface FileTypeFilterService { - getExtension: (file: { + getExtensionAndMimeType: (file: { name: string data: Buffer - }) => Promise + }) => Promise - hasAllowedType: ( - file: { - name: string - data: Buffer - }, + hasAllowedExtensionType: ( + extension: string, allowedExtensions?: string[], ) => Promise } +export interface FileTypeData { + extension: string + mimeType: string +} + export default FileTypeFilterService diff --git a/src/server/modules/threat/services/FileTypeFilterService.ts b/src/server/modules/threat/services/FileTypeFilterService.ts index 3b78a2036..db09b3b0c 100644 --- a/src/server/modules/threat/services/FileTypeFilterService.ts +++ b/src/server/modules/threat/services/FileTypeFilterService.ts @@ -2,6 +2,7 @@ import FileType from 'file-type' import { inject, injectable } from 'inversify' import * as interfaces from '../interfaces' import { DependencyIds } from '../../../constants' +import { FileTypeData } from '../interfaces/FileTypeFilterService' export const DEFAULT_ALLOWED_FILE_EXTENSIONS = [ 'avi', @@ -28,35 +29,49 @@ export const DEFAULT_ALLOWED_FILE_EXTENSIONS = [ 'zip', ] +export const FILE_EXTENSION_MIME_TYPE_MAP = new Map([ + ['csv', 'text/csv'], + ['dwf', 'application/x-dwf'], + ['dxf', 'application/dxf'], +]) + @injectable() export class FileTypeFilterService implements interfaces.FileTypeFilterService { allowedFileExtensions: string[] + fileExtensionsMimeTypeMap: Map + constructor( @inject(DependencyIds.allowedFileExtensions) allowedFileExtensions: string[], + @inject(DependencyIds.fileExtensionsMimeTypeMap) + fileExtensionsMimeTypeMap: Map, ) { this.allowedFileExtensions = allowedFileExtensions + this.fileExtensionsMimeTypeMap = fileExtensionsMimeTypeMap } - getExtension: (file: { + getExtensionAndMimeType: (file: { name: string data: Buffer - }) => Promise = async ({ name, data }) => { + }) => Promise = async ({ name, data }) => { const fileType = await FileType.fromBuffer(data) - return fileType?.ext || name.split('.').pop() + let ext: string | undefined = fileType?.ext + let mimeType: string | undefined = fileType?.mime + if (!ext || !mimeType) { + ext = name.split('.').pop() + mimeType = this.fileExtensionsMimeTypeMap.get(ext ?? '') + } + return { + extension: ext ?? '', + mimeType: mimeType ?? 'text/plain', + } } - hasAllowedType: ( - file: { - name: string - data: Buffer - }, + hasAllowedExtensionType: ( + extension: string, allowedExtensions?: string[], - ) => Promise = async (file, allowedExtensions) => { - const extension = await this.getExtension(file) - if (!extension) return false - + ) => Promise = async (extension, allowedExtensions) => { if (allowedExtensions && allowedExtensions.length > 0) { return allowedExtensions.includes(extension) } diff --git a/src/server/modules/threat/services/__tests__/FileTypeFilterService.test.ts b/src/server/modules/threat/services/__tests__/FileTypeFilterService.test.ts index fc1f2a7ff..fce2889ac 100644 --- a/src/server/modules/threat/services/__tests__/FileTypeFilterService.test.ts +++ b/src/server/modules/threat/services/__tests__/FileTypeFilterService.test.ts @@ -1,60 +1,82 @@ import { FileTypeFilterService } from '..' describe('FileTypeFilterService', () => { - const service = new FileTypeFilterService(['csv', 'xml']) + const service = new FileTypeFilterService( + ['csv', 'xml'], + new Map([ + ['csv', 'text/csv'], + ['dwf', 'application/x-dwf'], + ['dxf', 'application/dxf'], + ]), + ) - it('allows files not detected by file-type but via extension', async () => { - const result = await service.hasAllowedType({ + it('get extension and mime type from csv file not detected by file-type', async () => { + const fileTypeData = await service.getExtensionAndMimeType({ data: Buffer.from('name,phone\nabc,123\n'), name: 'file.csv', }) - expect(result).toBeTruthy() + + expect(fileTypeData.extension).toEqual('csv') + expect(fileTypeData.mimeType).toEqual('text/csv') }) - it('allows files detected by file-type', async () => { - const result = await service.hasAllowedType({ - data: Buffer.from(''), - name: 'file.notreallyxml', + it('get extension and mime type from dwf file not detected by file-type', async () => { + const fileTypeData = await service.getExtensionAndMimeType({ + data: Buffer.from('(DWF V06.00)PK'), + name: 'test.dwf', }) - expect(result).toBeTruthy() + + expect(fileTypeData.extension).toEqual('dwf') + expect(fileTypeData.mimeType).toEqual('application/x-dwf') }) - it('disallows files not via file-type but via extension', async () => { - const result = await service.hasAllowedType({ - data: Buffer.from(''), - name: 'file.jpg', + it('get extension and mime type from dxf file not detected by file-type', async () => { + const fileTypeData = await service.getExtensionAndMimeType({ + data: Buffer.from('0\nSECTION\n2\nENTITIES\n0\nLINE\n8'), + name: 'test.dxf', }) - expect(result).toBeFalsy() + + expect(fileTypeData.extension).toEqual('dxf') + expect(fileTypeData.mimeType).toEqual('application/dxf') + }) + + it('get extension and mime type from svg file not detected by file-type', async () => { + const fileTypeData = await service.getExtensionAndMimeType({ + data: Buffer.from(''), + name: 'test.svg', + }) + + expect(fileTypeData.extension).toEqual('svg') + expect(fileTypeData.mimeType).toEqual('text/plain') }) - it('disallows files via file-type', async () => { - const noXML = new FileTypeFilterService(['csv']) - const result = await noXML.hasAllowedType({ + it('get extension and mime type from file detected by file', async () => { + const fileTypeData = await service.getExtensionAndMimeType({ data: Buffer.from(''), name: 'file.notreallyxml', }) + + expect(fileTypeData.extension).toEqual('xml') + expect(fileTypeData.mimeType).toEqual('application/xml') + }) + + it('allows file extension type that are in allowed extension type', async () => { + const result = await service.hasAllowedExtensionType('csv') + expect(result).toBeTruthy() + }) + + it('disallows file extension type that are not in allowed extension type', async () => { + const result = await service.hasAllowedExtensionType('svg') expect(result).toBeFalsy() }) - it('allow files not detected by file-type from custom file extensions', async () => { - const result = await service.hasAllowedType( - { - data: Buffer.from('name,phone\nabc,123\n'), - name: 'file.png', - }, - ['png'], - ) + it('allow file extension type that are from custom file extensions', async () => { + const result = await service.hasAllowedExtensionType('png', ['png']) expect(result).toBeTruthy() }) - it('does not allow files not detected by file-type from custom file extensions', async () => { - const result = await service.hasAllowedType( - { - data: Buffer.from('name,phone\nabc,123\n'), - name: 'file.jpeg', - }, - ['png'], - ) + it('does not allow file extension type not from custom file extensions', async () => { + const result = await service.hasAllowedExtensionType('jpeg', ['png']) expect(result).toBeFalsy() }) }) diff --git a/test/end-to-end/UrlCreation.test.ts b/test/end-to-end/UrlCreation.test.ts index 8d7622060..be5aa87f2 100644 --- a/test/end-to-end/UrlCreation.test.ts +++ b/test/end-to-end/UrlCreation.test.ts @@ -325,7 +325,7 @@ test('The update file test', async (t) => { const generatedfileUrl = await shortUrlTextField.value const fileRow = Selector(`h6[title="${generatedfileUrl}"]`) - const directoryPath = `${process.env.HOME}/Downloads/${generatedfileUrl}.pdf` + const directoryPath = `${process.env.HOME}/Downloads/${generatedfileUrl}.csv` // Generate 1mb file await createEmptyFileOfSize(dummyFilePath, smallFileSize) diff --git a/test/end-to-end/util/config.ts b/test/end-to-end/util/config.ts index 9fa3fbb7e..69b8c9634 100644 --- a/test/end-to-end/util/config.ts +++ b/test/end-to-end/util/config.ts @@ -16,8 +16,8 @@ export const dummyMaliciousFilePath = './test/end-to-end/eicar.com.txt' export const dummyMaliciousRelativePath = './eicar.com.txt' export const dummyFilePath = './test/end-to-end/anotherDummy.txt' export const dummyRelativePath = './anotherDummy.txt' -export const dummyChangedFilePath = './test/end-to-end/changedDummy.pdf' -export const dummyRelativeChangedFilePath = './changedDummy.pdf' +export const dummyChangedFilePath = './test/end-to-end/changedDummy.csv' +export const dummyRelativeChangedFilePath = './changedDummy.csv' export const dummyBulkCsv = './test/end-to-end/bulkCsv.csv' export const dummyBulkCsvRelativePath = './bulkCsv.csv' export const smallFileSize = 1024 * 1024 * 1 From feab32cb216e5bd6d9ac7e65fa8be5212a094ce3 Mon Sep 17 00:00:00 2001 From: thamsimun Date: Thu, 7 Sep 2023 15:08:16 +0800 Subject: [PATCH 2/2] 1.77.1 --- CHANGELOG.md | 7 +++++++ package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73bbacab2..065b870c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,15 @@ 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). +#### [v1.77.1](https://github.com/opengovsg/GoGovSG/compare/v1.77.0...v1.77.1) + +- fix: stored xss via file upload [`#2258`](https://github.com/opengovsg/GoGovSG/pull/2258) +- Develop 1.77.0 [`#2250`](https://github.com/opengovsg/GoGovSG/pull/2250) + #### [v1.77.0](https://github.com/opengovsg/GoGovSG/compare/v1.76.1...v1.77.0) +> 3 August 2023 + - Experiment go <> sgid integration [`#2247`](https://github.com/opengovsg/GoGovSG/pull/2247) - [develop] 1.76.1 [`#2241`](https://github.com/opengovsg/GoGovSG/pull/2241) diff --git a/package-lock.json b/package-lock.json index 3fad5af14..fd3f788b1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,11 +1,11 @@ { "name": "GoGovSG", - "version": "1.77.0", + "version": "1.77.1", "lockfileVersion": 2, "requires": true, "packages": { "": { - "version": "1.77.0", + "version": "1.77.1", "license": "MIT", "dependencies": { "@datadog/browser-rum": "^4.15.0", diff --git a/package.json b/package.json index cc9258b35..0fe54477f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "GoGovSG", - "version": "1.77.0", + "version": "1.77.1", "description": "Link shortener for Singapore government.", "main": "src/server/index.js", "scripts": {