From d218b48cf64a22aba6e1e1e0569d6d86c2608d75 Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Fri, 11 Feb 2022 16:56:40 +0530 Subject: [PATCH] fix: Increase supported header size for multipart data (#11058) * Increase supported header size for multipart data to 128kB * Update error to be thrown from the global exception handler --- .../server/configurations/WebConfig.java | 28 ++++ .../server/exceptions/AppsmithError.java | 2 + .../exceptions/GlobalExceptionHandler.java | 11 ++ .../ImportExportApplicationServiceCEImpl.java | 3 +- .../ApplicationControllerTest.java | 122 ++++++++++++++++++ 5 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/WebConfig.java create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/WebConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/WebConfig.java new file mode 100644 index 0000000000..a96ca85ed6 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/WebConfig.java @@ -0,0 +1,28 @@ +package com.appsmith.server.configurations; + +import org.springframework.context.annotation.Configuration; +import org.springframework.http.codec.HttpMessageReader; +import org.springframework.http.codec.ServerCodecConfigurer; +import org.springframework.http.codec.multipart.DefaultPartHttpMessageReader; +import org.springframework.http.codec.multipart.MultipartHttpMessageReader; +import org.springframework.http.codec.multipart.Part; +import org.springframework.web.reactive.config.WebFluxConfigurer; + +@Configuration +public class WebConfig implements WebFluxConfigurer { + + @Override + public void configureHttpMessageCodecs(ServerCodecConfigurer configurer) { + configurer + .defaultCodecs() + .configureDefaultCodec(codec -> { + if (codec instanceof MultipartHttpMessageReader) { + HttpMessageReader partReader = ((MultipartHttpMessageReader) codec).getPartReader(); + if (partReader instanceof DefaultPartHttpMessageReader) { + // Set max file part header size to 128kB + ((DefaultPartHttpMessageReader) partReader).setMaxHeadersSize(128 * 1024); + } + } + }); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index b8242c9509..f87b8126f3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -136,6 +136,8 @@ public enum AppsmithError { SSH_KEY_GENERATION_ERROR(500, 5015, "Failed to generate SSH keys, please contact Appsmith support for more details", AppsmithErrorAction.DEFAULT, null, ErrorType.GIT_CONFIGURATION_ERROR, null), GIT_UPSTREAM_CHANGES(400, 4048, "Looks like there are pending upstream changes. To prevent you from losing history, we will pull the changes and push them to your repo.", AppsmithErrorAction.DEFAULT, null, ErrorType.GIT_ACTION_EXECUTION_ERROR, ErrorReferenceDocUrl.GIT_UPSTREAM_CHANGES), GIT_GENERIC_ERROR(504, 5016, "Git command execution error: {0}", AppsmithErrorAction.DEFAULT, null, ErrorType.GIT_ACTION_EXECUTION_ERROR, null), + GENERIC_JSON_IMPORT_ERROR(400, 4049, "Unable to import application in organization {0} with error {1}", AppsmithErrorAction.DEFAULT, null, ErrorType.BAD_REQUEST, null), + FILE_PART_DATA_BUFFER_ERROR(500, 5017, "Failed to upload file with error: {0}", AppsmithErrorAction.DEFAULT, null, ErrorType.BAD_REQUEST, null), ; private final Integer httpErrorCode; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java index 6554414bd9..055d0ffce2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java @@ -8,6 +8,7 @@ import com.appsmith.server.dtos.ResponseDTO; import io.sentry.Sentry; import io.sentry.SentryLevel; import lombok.extern.slf4j.Slf4j; +import org.springframework.core.io.buffer.DataBufferLimitException; import org.springframework.http.HttpStatus; import org.springframework.security.access.AccessDeniedException; import org.springframework.validation.FieldError; @@ -162,6 +163,16 @@ public class GlobalExceptionHandler { appsmithError.getMessage()))); } + @ExceptionHandler + @ResponseBody + public Mono> catchDataBufferLimitException(DataBufferLimitException e, ServerWebExchange exchange) { + AppsmithError appsmithError = AppsmithError.FILE_PART_DATA_BUFFER_ERROR; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); + doLog(e); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage(e.getMessage())))); + } + /** * This function catches the generic Exception class and is meant to be a catch all to ensure that we don't leak * any information to the client. Ideally, the function #catchAppsmithException should be used diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ImportExportApplicationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ImportExportApplicationServiceCEImpl.java index e58eb5db55..8283df650c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ImportExportApplicationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ImportExportApplicationServiceCEImpl.java @@ -498,7 +498,8 @@ public class ImportExportApplicationServiceCEImpl implements ImportExportApplica }.getType(); ApplicationJson jsonFile = gson.fromJson(data, fileType); return importApplicationInOrganization(orgId, jsonFile); - }); + }) + .onErrorResume(error -> Mono.error(new AppsmithException(AppsmithError.GENERIC_JSON_IMPORT_ERROR, orgId, error.getMessage()))); return Mono.create(sink -> importedApplicationMono .subscribe(sink::success, sink::error, null, sink.currentContext()) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java new file mode 100644 index 0000000000..43fa23da85 --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java @@ -0,0 +1,122 @@ +package com.appsmith.server.controllers; + +import com.appsmith.server.configurations.SecurityTestConfig; +import com.appsmith.server.constants.Url; +import com.appsmith.server.domains.Application; +import com.appsmith.server.services.ApplicationPageService; +import com.appsmith.server.services.ApplicationService; +import com.appsmith.server.services.ThemeService; +import com.appsmith.server.solutions.ApplicationFetcher; +import com.appsmith.server.solutions.ApplicationForkingService; +import com.appsmith.server.solutions.ImportExportApplicationService; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.reactive.WebFluxTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Import; +import org.springframework.core.io.ClassPathResource; +import org.springframework.http.MediaType; +import org.springframework.http.client.MultipartBodyBuilder; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.web.reactive.server.WebTestClient; +import org.springframework.web.reactive.function.BodyInserters; +import reactor.core.publisher.Mono; + +import java.io.IOException; + +@RunWith(SpringRunner.class) +@WebFluxTest(ApplicationController.class) +@Import(SecurityTestConfig.class) +public class ApplicationControllerTest { + @Autowired + private WebTestClient webTestClient; + + @MockBean + ApplicationService applicationService; + + @MockBean + ApplicationPageService applicationPageService; + + @MockBean + ApplicationFetcher applicationFetcher; + + @MockBean + ApplicationForkingService applicationForkingService; + + @MockBean + ImportExportApplicationService importExportApplicationService; + + @MockBean + ThemeService themeService; + + private String getFileName(int length) { + StringBuilder fileName = new StringBuilder(); + for (int count = 0; count < length; count++) { + fileName.append("i"); + } + fileName.append(".json"); + return fileName.toString(); + } + + private MultipartBodyBuilder createBodyBuilder(String fileName) throws IOException { + MultipartBodyBuilder bodyBuilder = new MultipartBodyBuilder(); + + bodyBuilder + .part("file", new ClassPathResource("test_assets/ImportExportServiceTest/invalid-json-without-app.json").getFile(), MediaType.APPLICATION_JSON) + .header("Content-Disposition", "form-data; name=\"file\"; filename=" + fileName) + .header("Content-Type", "application/json"); + return bodyBuilder; + } + + @Test + @WithMockUser + public void whenFileUploadedWithLongHeader_thenVerifyErrorStatus() throws IOException { + + Mockito.when(importExportApplicationService.extractFileAndSaveApplication(Mockito.any(), Mockito.any())) + .thenReturn(Mono.just(new Application())); + + final String fileName = getFileName(130 * 1024); + MultipartBodyBuilder bodyBuilder = createBodyBuilder(fileName); + + webTestClient.post() + .uri(Url.APPLICATION_URL + "/import/orgId") + .contentType(MediaType.MULTIPART_FORM_DATA) + .body(BodyInserters.fromMultipartData(bodyBuilder.build())) + .exchange() + .expectStatus() + .isEqualTo(500) + .expectBody() + .json("{\n" + + " \"responseMeta\": {\n" + + " \"status\": 500,\n" + + " \"success\": false,\n" + + " \"error\": {\n" + + " \"code\": 5016,\n" + + " \"message\": \"Failed to upload file with error: Part headers exceeded the memory usage limit of 131072 bytes\"\n" + + " }\n" + + " }\n" + + "}"); + } + + @Test + @WithMockUser + public void whenFileUploadedWithShortHeader_thenVerifySuccessStatus() throws IOException { + + Mockito.when(importExportApplicationService.extractFileAndSaveApplication(Mockito.any(), Mockito.any())) + .thenReturn(Mono.just(new Application())); + + final String fileName = getFileName(2 * 1024); + MultipartBodyBuilder bodyBuilder = createBodyBuilder(fileName); + + webTestClient.post() + .uri(Url.APPLICATION_URL + "/import/orgId") + .contentType(MediaType.MULTIPART_FORM_DATA) + .body(BodyInserters.fromMultipartData(bodyBuilder.build())) + .exchange() + .expectStatus() + .isEqualTo(200); + } +}