diff --git a/package.json b/package.json index dcb240b9a..b85636b65 100644 --- a/package.json +++ b/package.json @@ -59,7 +59,6 @@ "memoizee": "^0.4.15", "pinejs-client-core": "^6.13.0", "randomstring": "^1.2.3", - "type-is": "^1.6.18", "typed-error": "^3.2.2" }, "devDependencies": { diff --git a/src/sbvr-api/hooks.ts b/src/sbvr-api/hooks.ts index 3f7e5ed04..ebbf1722d 100644 --- a/src/sbvr-api/hooks.ts +++ b/src/sbvr-api/hooks.ts @@ -29,6 +29,7 @@ export interface HookReq { custom?: AnyObject; tx?: Tx; hooks?: InstantiatedHooks; + is?: (type: string | string[]) => string | false | null; } export interface HookArgs { req: HookReq; diff --git a/src/webresource-handler/index.ts b/src/webresource-handler/index.ts index 1fabe9c1e..c4594fa8d 100644 --- a/src/webresource-handler/index.ts +++ b/src/webresource-handler/index.ts @@ -1,9 +1,9 @@ import type * as Express from 'express'; import * as busboy from 'busboy'; -import * as is from 'type-is'; import * as stream from 'stream'; import * as uriParser from '../sbvr-api/uri-parser'; import * as sbvrUtils from '../sbvr-api/sbvr-utils'; +import type { HookArgs } from '../sbvr-api/hooks'; import { getApiRoot, getModel } from '../sbvr-api/sbvr-utils'; import { checkPermissions } from '../sbvr-api/permissions'; import { NoopHandler } from './handlers/NoopHandler'; @@ -108,7 +108,7 @@ export const getUploaderMiddlware = ( handler: WebResourceHandler, ): Express.RequestHandler => { return async (req, res, next) => { - if (!is(req, ['multipart'])) { + if (!req.is('multipart')) { return next(); } const uploadedFilePaths: string[] = []; @@ -226,10 +226,29 @@ const deleteFiles = async ( await Promise.all(promises); }; +const throwIfWebresourceNotInMultipart = ( + webResourceFields: string[], + { req, request }: HookArgs, +) => { + if (!req.is?.('multipart')) { + for (const field of webResourceFields) { + if (request.values[field] != null) { + throw new errors.BadRequestError( + 'Use multipart requests to upload a file.', + ); + } + } + } +}; + const getCreateWebResourceHooks = ( webResourceHandler: WebResourceHandler, ): sbvrUtils.Hooks => { return { + PRERUN: (hookArgs) => { + const webResourceFields = getWebResourceFields(hookArgs.request); + throwIfWebresourceNotInMultipart(webResourceFields, hookArgs); + }, 'POSTRUN-ERROR': async ({ tx, request }) => { tx?.on('rollback', () => { void deleteRollbackPendingFields(request, webResourceHandler); @@ -272,6 +291,8 @@ const getRemoveWebResourceHooks = ( const { api, request, tx } = args; let webResourceFields = getWebResourceFields(request); + throwIfWebresourceNotInMultipart(webResourceFields, args); + // Request failed on DB roundtrip (e.g. DB constraint) and pending files need to be deleted tx.on('rollback', () => { void deleteRollbackPendingFields(request, webResourceHandler); diff --git a/test/06-webresource.test.ts b/test/06-webresource.test.ts index dba30fa8b..69ca5be33 100644 --- a/test/06-webresource.test.ts +++ b/test/06-webresource.test.ts @@ -484,6 +484,77 @@ describe('06 webresources tests', function () { expect(await isEventuallyDeleted(uniqueFilename)).to.be.true; }); + it('should not accept webresource payload on application/json requests', async () => { + const uniqueFilename = `${randomUUID()}_${filename}`; + + const res = await supertest(testLocalServer) + .post(`/${resourceName}/organization`) + .send({ + name: 'John', + [resourcePath]: { + filename: uniqueFilename, + content_type: contentType, + size: fileSize, + href: 'http://dummy/bucket/other_href', + }, + }) + .expect(400); + + expect(res.body).to.equal('Use multipart requests to upload a file.'); + }); + + it('does not modify stored file if uploading with application/json requests', async () => { + const uniqueFilename = `${randomUUID()}_${filename}`; + + const { body: organization } = await supertest(testLocalServer) + .post(`/${resourceName}/organization`) + .field('name', 'john') + .attach(resourcePath, filePath, { + filename: uniqueFilename, + contentType, + }) + .expect(201); + + const href = organization[resourcePath].href; + + await supertest(testLocalServer) + .patch(`/${resourceName}/organization(${organization.id})`) + .send({ name: 'test' }) + .expect(200); + + const getRes = await supertest(testLocalServer) + .get(`/${resourceName}/organization(${organization.id})`) + .expect(200); + + expect(getRes.body.d[0].name).to.be.eq('test'); + expect(getRes.body.d[0][resourcePath].href).to.be.eq(href); + }); + + it('should delete resource in S3 when passing null in application/json request', async () => { + const uniqueFilename = `${randomUUID()}_${filename}`; + + const { body: organization } = await supertest(testLocalServer) + .post(`/${resourceName}/organization`) + .field('name', 'john') + .attach(resourcePath, filePath, { + filename: uniqueFilename, + contentType, + }) + .expect(201); + + await supertest(testLocalServer) + .patch(`/${resourceName}/organization(${organization.id})`) + .send({ [resourcePath]: null }) + .expect(200); + + const getRes = await supertest(testLocalServer) + .get(`/${resourceName}/organization(${organization.id})`) + .expect(200); + + expect(getRes.body.d[0][resourcePath]).to.be.null; + expect(await isEventuallyDeleted(uniqueFilename)).to.be.true; + }); + it('does not fail to serve if S3 resource is deleted but entry exists', async () => { // This tests the current behavior, but we might want to change it in the future // because the current behavior allows for a dangling reference to exist @@ -584,6 +655,26 @@ describe('06 webresources tests', function () { otherUniqueFilename = newOtherUniqueFilename; }); + it('can patch with application/json null on one without modifying the other', async () => { + await supertest(testLocalServer) + .patch(`/${resourceName}/organization(${organization.id})`) + .send({ [firstResourcePath]: null }) + .expect(200); + const { + body: { + d: [org], + }, + } = await supertest(testLocalServer) + .get(`/${resourceName}/organization(${organization.id})`) + .expect(200); + + expect(org[firstResourcePath]).to.be.null; + expect(org[secondResourcePath].filename).to.not.be.null; + + expect(await isEventuallyDeleted(uniqueFilename)).to.be.true; + await expectToExist(otherUniqueFilename); + }); + it('can patch multiple web resources on the same organization', async () => { const newUniqueFilename = `${randomUUID()}_${filename}`; const newOtherUniqueFilename = `${randomUUID()}_other-image.png`; diff --git a/test/fixtures/06-webresource/translations/v1/hooks.ts b/test/fixtures/06-webresource/translations/v1/hooks.ts index a6d3c659e..c4cb2e098 100644 --- a/test/fixtures/06-webresource/translations/v1/hooks.ts +++ b/test/fixtures/06-webresource/translations/v1/hooks.ts @@ -10,7 +10,7 @@ const addHook = ( addHook(['PUT', 'POST', 'PATCH'], 'organization', { async POSTPARSE({ request }) { - if (request.values.other_image) { + if (request.values.other_image !== undefined) { request.values.logo_image = request.values.other_image; delete request.values.other_image; }