Merge pull request #2266 from opengovsg/release-1.77.1

[Release] Release 1.77.1

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)
 

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": {

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,

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'),

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) {

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<void> => {
       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()

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()
+  })
 })

src/server/modules/threat/interfaces/FileTypeFilterService.ts

@@ -1,16 +1,18 @@
 export interface FileTypeFilterService {
-  getExtension: (file: {
+  getExtensionAndMimeType: (file: {
     name: string
     data: Buffer
-  }) => Promise<string | undefined>
+  }) => Promise<FileTypeData>
 
-  hasAllowedType: (
-    file: {
-      name: string
-      data: Buffer
-    },
+  hasAllowedExtensionType: (
+    extension: string,
     allowedExtensions?: string[],
   ) => Promise<boolean>
 }
 
+export interface FileTypeData {
+  extension: string
+  mimeType: string
+}
+
 export default FileTypeFilterService

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<string, string>([
+  ['csv', 'text/csv'],
+  ['dwf', 'application/x-dwf'],
+  ['dxf', 'application/dxf'],
+])
+
 @injectable()
 export class FileTypeFilterService implements interfaces.FileTypeFilterService {
   allowedFileExtensions: string[]
 
+  fileExtensionsMimeTypeMap: Map<string, string>
+
   constructor(
     @inject(DependencyIds.allowedFileExtensions)
     allowedFileExtensions: string[],
+    @inject(DependencyIds.fileExtensionsMimeTypeMap)
+    fileExtensionsMimeTypeMap: Map<string, string>,
   ) {
     this.allowedFileExtensions = allowedFileExtensions
+    this.fileExtensionsMimeTypeMap = fileExtensionsMimeTypeMap
   }
 
-  getExtension: (file: {
+  getExtensionAndMimeType: (file: {
     name: string
     data: Buffer
-  }) => Promise<string | undefined> = async ({ name, data }) => {
+  }) => Promise<FileTypeData> = 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<boolean> = async (file, allowedExtensions) => {
-    const extension = await this.getExtension(file)
-    if (!extension) return false
-
+  ) => Promise<boolean> = async (extension, allowedExtensions) => {
     if (allowedExtensions && allowedExtensions.length > 0) {
       return allowedExtensions.includes(extension)
     }