Blocks updating and creating webresources with non multipart-formdata…

… requests

Change-type: patch

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

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;

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

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`;

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;
 		}