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
This commit is contained in:
parent
44ce1d4b5a
commit
d218b48cf6
|
|
@ -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<Part> partReader = ((MultipartHttpMessageReader) codec).getPartReader();
|
||||||
|
if (partReader instanceof DefaultPartHttpMessageReader) {
|
||||||
|
// Set max file part header size to 128kB
|
||||||
|
((DefaultPartHttpMessageReader) partReader).setMaxHeadersSize(128 * 1024);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -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),
|
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_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),
|
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;
|
private final Integer httpErrorCode;
|
||||||
|
|
|
||||||
|
|
@ -8,6 +8,7 @@ import com.appsmith.server.dtos.ResponseDTO;
|
||||||
import io.sentry.Sentry;
|
import io.sentry.Sentry;
|
||||||
import io.sentry.SentryLevel;
|
import io.sentry.SentryLevel;
|
||||||
import lombok.extern.slf4j.Slf4j;
|
import lombok.extern.slf4j.Slf4j;
|
||||||
|
import org.springframework.core.io.buffer.DataBufferLimitException;
|
||||||
import org.springframework.http.HttpStatus;
|
import org.springframework.http.HttpStatus;
|
||||||
import org.springframework.security.access.AccessDeniedException;
|
import org.springframework.security.access.AccessDeniedException;
|
||||||
import org.springframework.validation.FieldError;
|
import org.springframework.validation.FieldError;
|
||||||
|
|
@ -162,6 +163,16 @@ public class GlobalExceptionHandler {
|
||||||
appsmithError.getMessage())));
|
appsmithError.getMessage())));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ExceptionHandler
|
||||||
|
@ResponseBody
|
||||||
|
public Mono<ResponseDTO<ErrorDTO>> 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
|
* 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
|
* any information to the client. Ideally, the function #catchAppsmithException should be used
|
||||||
|
|
|
||||||
|
|
@ -498,7 +498,8 @@ public class ImportExportApplicationServiceCEImpl implements ImportExportApplica
|
||||||
}.getType();
|
}.getType();
|
||||||
ApplicationJson jsonFile = gson.fromJson(data, fileType);
|
ApplicationJson jsonFile = gson.fromJson(data, fileType);
|
||||||
return importApplicationInOrganization(orgId, jsonFile);
|
return importApplicationInOrganization(orgId, jsonFile);
|
||||||
});
|
})
|
||||||
|
.onErrorResume(error -> Mono.error(new AppsmithException(AppsmithError.GENERIC_JSON_IMPORT_ERROR, orgId, error.getMessage())));
|
||||||
|
|
||||||
return Mono.create(sink -> importedApplicationMono
|
return Mono.create(sink -> importedApplicationMono
|
||||||
.subscribe(sink::success, sink::error, null, sink.currentContext())
|
.subscribe(sink::success, sink::error, null, sink.currentContext())
|
||||||
|
|
|
||||||
|
|
@ -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);
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in New Issue
Block a user