Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: server to randomize image and logo filenames before generating presigned url #3927

Merged
merged 6 commits into from
May 31, 2022
Merged

fix: server to randomize image and logo filenames before generating presigned url #3927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca8faa7
chore: update package-lock
tshuli May 31, 2022
d979d0c
fix: backend to append random string in front of user-supplied fileid
tshuli May 31, 2022
8a68173
fix: frontend to use returned key to construct s3 url instead of clie…
tshuli May 31, 2022
cd5c0d1
chore: update tests
tshuli May 31, 2022
2d4de98
chore: update frontend tests
tshuli May 31, 2022
d154c6e
chore: tighten regex check for key returned
tshuli May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

100 changes: 100 additions & 0 deletions src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,7 @@ describe('admin-form.controller', () => {
fileId: 'any file id',
fileMd5Hash: 'any hash',
fileType: 'any type',
isNewClient: true, // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
},
})

Expand Down Expand Up @@ -987,6 +988,55 @@ describe('admin-form.controller', () => {
expect(mockRes.json).toHaveBeenCalledWith(expectedPresignedPost)
})

it('should return 200 with presigned POST URL object when successful for old clients', async () => {
// TODO (#128): Test to be removed after isNewClient flag has been removed
// Arrange
const MOCK_REQ_OLD = expressHandler.mockRequest({
params: {
formId: MOCK_FORM_ID,
},
session: {
user: {
_id: MOCK_USER_ID,
},
},
body: {
fileId: 'any file id',
fileMd5Hash: 'any hash',
fileType: 'any type',
},
})

const mockRes = expressHandler.mockResponse()
// Mock various services to return expected results.
MockUserService.getPopulatedUserById.mockReturnValueOnce(
okAsync(MOCK_USER),
)
MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce(
okAsync(MOCK_FORM),
)
const expectedPresignedPost: PresignedPost = {
fields: {
'X-Amz-Signature': 'some-amz-signature',
Policy: 'some policy',
},
url: 'some url',
}
MockAdminFormService.createPresignedPostUrlForImages.mockReturnValueOnce(
okAsync(expectedPresignedPost),
)

// Act
await AdminFormController.createPresignedPostUrlForImages(
MOCK_REQ_OLD,
mockRes,
jest.fn(),
)

// Assert
expect(mockRes.json).toHaveBeenCalledWith(expectedPresignedPost)
})

it('should return 400 when InvalidFileTypeError is returned when creating presigned POST URL', async () => {
// Arrange
// Mock various services to return expected results.
Expand Down Expand Up @@ -1187,6 +1237,7 @@ describe('admin-form.controller', () => {
fileId: 'any file id',
fileMd5Hash: 'any hash',
fileType: 'any type',
isNewClient: true, // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
},
})

Expand Down Expand Up @@ -1222,6 +1273,55 @@ describe('admin-form.controller', () => {
expect(mockRes.json).toHaveBeenCalledWith(expectedPresignedPost)
})

it('should return 200 with presigned POST URL object when successful for old clients', async () => {
// TODO (#128): Test to be removed after isNewClient flag has been removed
// Arrange
const MOCK_REQ_OLD = expressHandler.mockRequest({
params: {
formId: MOCK_FORM_ID,
},
session: {
user: {
_id: MOCK_USER_ID,
},
},
body: {
fileId: 'any file id',
fileMd5Hash: 'any hash',
fileType: 'any type',
},
})
// Arrange
const mockRes = expressHandler.mockResponse()
// Mock various services to return expected results.
MockUserService.getPopulatedUserById.mockReturnValueOnce(
okAsync(MOCK_USER),
)
MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce(
okAsync(MOCK_FORM),
)
const expectedPresignedPost: PresignedPost = {
fields: {
'X-Amz-Signature': 'some-amz-signature',
Policy: 'some policy',
},
url: 'some url',
}
MockAdminFormService.createPresignedPostUrlForLogos.mockReturnValueOnce(
okAsync(expectedPresignedPost),
)

// Act
await AdminFormController.createPresignedPostUrlForLogos(
MOCK_REQ_OLD,
mockRes,
jest.fn(),
)

// Assert
expect(mockRes.json).toHaveBeenCalledWith(expectedPresignedPost)
})

it('should return 400 when InvalidFileTypeError is returned when creating presigned POST URL', async () => {
// Arrange
// Mock error
Expand Down
100 changes: 96 additions & 4 deletions src/app/modules/form/admin-form/__tests__/admin-form.routes.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4906,9 +4906,17 @@ describe('admin-form.routes', () => {
fileId: 'some file id',
fileMd5Hash: SparkMD5.hash('test file name'),
fileType: VALID_UPLOAD_FILE_TYPES[0],
isNewClient: true, // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
}

it('should return 200 with presigned POST URL object', async () => {
const DEFAULT_POST_PARAMS_OLD_CLIENT = {
// TODO (#128): Clean up tests after isNewClient flag has been removed.
fileId: 'some other file id',
fileMd5Hash: SparkMD5.hash('test file name again'),
fileType: VALID_UPLOAD_FILE_TYPES[2],
}

it('should return 200 with presigned POST URL object and append an objectId to the key', async () => {
// Arrange
const form = await EncryptFormModel.create({
title: 'form',
Expand All @@ -4929,7 +4937,45 @@ describe('admin-form.routes', () => {
fields: expect.objectContaining({
'Content-MD5': DEFAULT_POST_PARAMS.fileMd5Hash,
'Content-Type': DEFAULT_POST_PARAMS.fileType,
key: DEFAULT_POST_PARAMS.fileId,
key: expect.any(String),
// Should have correct permissions.
acl: 'public-read',
bucket: expect.any(String),
}),
})
expect(response.body.fields.key).toEqual(
expect.stringContaining(DEFAULT_POST_PARAMS.fileId),
)
expect(DEFAULT_POST_PARAMS.fileId.length).toEqual(
response.body.fields.key.length - 25,
)

expect(response.body.fields.key).toMatch(/^[a-fA-F0-9]{24}-/)
})

it('should return 200 with presigned POST URL object and NOT append an objectId to the key if !isNewClient', async () => {
// TODO (#128): Test to be removed after isNewClient flag has been removed.
// Arrange
const form = await EncryptFormModel.create({
title: 'form',
admin: defaultUser._id,
publicKey: 'does not matter',
})

// Act
const response = await request
.post(`/${form._id}/adminform/logos`)
.send(DEFAULT_POST_PARAMS_OLD_CLIENT)

// Assert
expect(response.status).toEqual(200)
// Should equal mocked result.
expect(response.body).toEqual({
url: expect.any(String),
fields: expect.objectContaining({
'Content-MD5': DEFAULT_POST_PARAMS_OLD_CLIENT.fileMd5Hash,
'Content-Type': DEFAULT_POST_PARAMS_OLD_CLIENT.fileType,
key: DEFAULT_POST_PARAMS_OLD_CLIENT.fileId,
// Should have correct permissions.
acl: 'public-read',
bucket: expect.any(String),
Expand Down Expand Up @@ -5144,9 +5190,17 @@ describe('admin-form.routes', () => {
fileId: 'some other file id',
fileMd5Hash: SparkMD5.hash('test file name again'),
fileType: VALID_UPLOAD_FILE_TYPES[2],
isNewClient: true, // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
}

it('should return 200 with presigned POST URL object', async () => {
const DEFAULT_POST_PARAMS_OLD_CLIENT = {
// TODO (#128): Clean up tests after isNewClient flag has been removed.
fileId: 'some other file id',
fileMd5Hash: SparkMD5.hash('test file name again'),
fileType: VALID_UPLOAD_FILE_TYPES[2],
}

it('should return 200 with presigned POST URL object and append an objectId to the key', async () => {
// Arrange
const form = await EncryptFormModel.create({
title: 'form',
Expand All @@ -5167,7 +5221,45 @@ describe('admin-form.routes', () => {
fields: expect.objectContaining({
'Content-MD5': DEFAULT_POST_PARAMS.fileMd5Hash,
'Content-Type': DEFAULT_POST_PARAMS.fileType,
key: DEFAULT_POST_PARAMS.fileId,
key: expect.any(String),
// Should have correct permissions.
acl: 'public-read',
bucket: expect.any(String),
}),
})
expect(response.body.fields.key).toEqual(
expect.stringContaining(DEFAULT_POST_PARAMS.fileId),
)
expect(DEFAULT_POST_PARAMS.fileId.length).toEqual(
response.body.fields.key.length - 25,
)

expect(response.body.fields.key).toMatch(/^[a-fA-F0-9]{24}-/)
})

it('should return 200 with presigned POST URL object and NOT append an objectId to the key if !isNewClient', async () => {
// TODO (#128): Test to be removed after isNewClient flag has been removed.
// Arrange
const form = await EncryptFormModel.create({
title: 'form',
admin: defaultUser._id,
publicKey: 'does not matter',
})

// Act
const response = await request
.post(`/${form._id}/adminform/logos`)
.send(DEFAULT_POST_PARAMS_OLD_CLIENT)

// Assert
expect(response.status).toEqual(200)
// Should equal mocked result.
expect(response.body).toEqual({
url: expect.any(String),
fields: expect.objectContaining({
'Content-MD5': DEFAULT_POST_PARAMS_OLD_CLIENT.fileMd5Hash,
'Content-Type': DEFAULT_POST_PARAMS_OLD_CLIENT.fileType,
key: DEFAULT_POST_PARAMS_OLD_CLIENT.fileId,
// Should have correct permissions.
acl: 'public-read',
bucket: expect.any(String),
Expand Down
22 changes: 18 additions & 4 deletions src/app/modules/form/admin-form/admin-form.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import JoiDate from '@joi/date'
import { ObjectId } from 'bson'
import { celebrate, Joi as BaseJoi, Segments } from 'celebrate'
import { AuthedSessionData } from 'express-session'
import { StatusCodes } from 'http-status-codes'
Expand Down Expand Up @@ -188,6 +189,7 @@ const fileUploadValidator = celebrate({
fileType: Joi.string()
.valid(...VALID_UPLOAD_FILE_TYPES)
.required(),
isNewClient: Joi.boolean().optional(),
},
})

Expand Down Expand Up @@ -383,12 +385,18 @@ export const createPresignedPostUrlForImages: ControllerHandler<
fileId: string
fileMd5Hash: string
fileType: string
isNewClient?: boolean // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
}
> = async (req, res) => {
const { formId } = req.params
const { fileId, fileMd5Hash, fileType } = req.body
const { fileId, fileMd5Hash, fileType, isNewClient } = req.body
const sessionUserId = (req.session as AuthedSessionData).user._id

// Adding random objectId ensures fileId is unpredictable by client
const randomizedFileId = isNewClient
? `${String(new ObjectId())}-${fileId}`
: fileId // TODO (#128): if !isNewClient, returns fileId for backward compatability. To remove fallback for !isNewClient 2 weeks after release.

return (
// Step 1: Retrieve currently logged in user.
UserService.getPopulatedUserById(sessionUserId)
Expand All @@ -403,7 +411,7 @@ export const createPresignedPostUrlForImages: ControllerHandler<
// Step 3: Has write permissions, generate presigned POST URL.
.andThen(() =>
AdminFormService.createPresignedPostUrlForImages({
fileId,
fileId: randomizedFileId,
fileMd5Hash,
fileType,
}),
Expand Down Expand Up @@ -448,12 +456,18 @@ export const createPresignedPostUrlForLogos: ControllerHandler<
fileId: string
fileMd5Hash: string
fileType: string
isNewClient?: boolean // TODO (#128): Flag for server to know whether to append random object ID in front. To remove 2 weeks after release.
}
> = async (req, res) => {
const { formId } = req.params
const { fileId, fileMd5Hash, fileType } = req.body
const { fileId, fileMd5Hash, fileType, isNewClient } = req.body
const sessionUserId = (req.session as AuthedSessionData).user._id

// Adding random objectId ensures fileId is unpredictable by client
const randomizedFileId = isNewClient
? `${String(new ObjectId())}-${fileId}`
: fileId // TODO (#128): if !isNewClient, returns fileId for backward compatability. To remove fallback for !isNewClient 2 weeks after release.

return (
// Step 1: Retrieve currently logged in user.
UserService.getPopulatedUserById(sessionUserId)
Expand All @@ -468,7 +482,7 @@ export const createPresignedPostUrlForLogos: ControllerHandler<
// Step 3: Has write permissions, generate presigned POST URL.
.andThen(() =>
AdminFormService.createPresignedPostUrlForLogos({
fileId,
fileId: randomizedFileId,
fileMd5Hash,
fileType,
}),
Expand Down
Loading