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

feat: add more validation to file upload (#18112) #18909

Merged
merged 1 commit into from
Oct 25, 2024
Merged

feat: add more validation to file upload (#18112) #18909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions ...ces/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/FileResourceBlocklist.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* @author Lars Helge Overland
*/
public class FileResourceBlocklist {
private static final Set<String> CONTENT_TYPES =
private static final Set<String> BLOCKED_CONTENT_TYPES =
Set.of(
// Web
"text/html",
Expand All @@ -54,7 +54,7 @@ public class FileResourceBlocklist {
"application/x-sh",
"application/x-csh");

private static final Set<String> FILE_EXTENSIONS =
private static final Set<String> BLOCKED_FILE_EXTENSIONS =
Set.of(
// Web
"html",
Expand Down Expand Up @@ -93,11 +93,11 @@ public static boolean isValid(FileResource fileResource) {
return false;
}

if (CONTENT_TYPES.contains(fileResource.getContentType().toLowerCase())) {
if (BLOCKED_CONTENT_TYPES.contains(fileResource.getContentType().toLowerCase())) {
return false;
}

if (FILE_EXTENSIONS.contains(
if (BLOCKED_FILE_EXTENSIONS.contains(
FilenameUtils.getExtension(fileResource.getName().toLowerCase()))) {
return false;
}
Expand Down
17 changes: 10 additions & 7 deletions ...est-web-api/src/test/java/org/hisp/dhis/webapi/controller/FileResourceControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,11 @@ void testSaveGoodAvatar() throws IOException {
}

@Test
void testSaveOrgUnitImage() {
void testSaveOrgUnitImage() throws IOException {
File file = new ClassPathResource("file/dhis2.png").getFile();
MockMultipartFile image =
new MockMultipartFile(
"file", "OU_profile_image.png", "image/png", "<<png data>>".getBytes());
"file", "OU_profile_image.png", "image/png", Files.readAllBytes(file.toPath()));
HttpResponse response = POST_MULTIPART("/fileResources?domain=ORG_UNIT", image);
JsonObject savedObject =
response.content(HttpStatus.ACCEPTED).getObject("response").getObject("fileResource");
Expand All @@ -119,10 +120,11 @@ void testSaveOrgUnitImage() {
}

@Test
void testSaveOrgUnitImageWithUid() {
void testSaveOrgUnitImageWithUid() throws IOException {
File file = new ClassPathResource("file/dhis2.png").getFile();
MockMultipartFile image =
new MockMultipartFile(
"file", "OU_profile_image.png", "image/png", "<<png data>>".getBytes());
"file", "OU_profile_image.png", "image/png", Files.readAllBytes(file.toPath()));
HttpResponse response = POST_MULTIPART("/fileResources?domain=ORG_UNIT&uid=0123456789a", image);
JsonObject savedObject =
response.content(HttpStatus.ACCEPTED).getObject("response").getObject("fileResource");
Expand All @@ -131,10 +133,11 @@ void testSaveOrgUnitImageWithUid() {
}

@Test
void testSaveOrgUnitImageWithUid_Update() {
void testSaveOrgUnitImageWithUid_Update() throws IOException {
File file = new ClassPathResource("file/dhis2.png").getFile();
MockMultipartFile image =
new MockMultipartFile(
"file", "OU_profile_image.png", "image/png", "<<png data>>".getBytes());
"file", "OU_profile_image.png", "image/png", Files.readAllBytes(file.toPath()));
HttpResponse response = POST_MULTIPART("/fileResources?domain=ORG_UNIT&uid=0123456789x", image);
JsonObject savedObject =
response.content(HttpStatus.ACCEPTED).getObject("response").getObject("fileResource");
Expand All @@ -144,7 +147,7 @@ void testSaveOrgUnitImageWithUid_Update() {
// now update the resource with a different image but the same UID
MockMultipartFile image2 =
new MockMultipartFile(
"file", "OU_profile_image2.png", "image/png", "<<png data>>".getBytes());
"file", "OU_profile_image2.png", "image/png", Files.readAllBytes(file.toPath()));

JsonWebMessage message =
assertWebMessage(
Expand Down
10 changes: 10 additions & 0 deletions ...-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/FileResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,12 @@ public void getFileResourceData(
}

response.setContentType(fileResource.getContentType());

response.setHeader(
HttpHeaders.CONTENT_LENGTH,
String.valueOf(fileResourceService.getFileResourceContentLength(fileResource)));
response.setHeader(HttpHeaders.CONTENT_DISPOSITION, "filename=" + fileResource.getName());

HeaderUtils.setSecurityHeaders(
response, dhisConfig.getProperty(ConfigurationKey.CSP_HEADER_VALUE));

Expand All @@ -174,10 +176,18 @@ public WebMessage saveFileResource(
if (domain.equals(FileResourceDomain.ICON)) {
validateCustomIconFile(file);
fileResource = fileResourceUtils.saveFileResource(uid, resizeIconToDefaultSize(file), domain);

} else if (domain.equals(FileResourceDomain.USER_AVATAR)) {
fileResourceUtils.validateUserAvatar(file);
fileResource =
fileResourceUtils.saveFileResource(uid, resizeAvatarToDefaultSize(file), domain);

} else if (domain.equals(FileResourceDomain.ORG_UNIT)) {
fileResourceUtils.validateOrgUnitImage(file);
fileResource =
fileResourceUtils.saveFileResource(
uid, fileResourceUtils.resizeOrgToDefaultSize(file), domain);

} else {
fileResource = fileResourceUtils.saveFileResource(uid, file, domain);
}
Expand Down
61 changes: 36 additions & 25 deletions dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/utils/FileResourceUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,34 @@ public class FileResourceUtils {
private static final int AVATAR_TARGET_HEIGHT = 200;
private static final int AVATAR_TARGET_WIDTH = 200;

private static final long MAX_AVATAR_FILE_SIZE_IN_BYTES = 2_000_000;
private static final int ORGUNIT_TARGET_HEIGHT = 800;
private static final int ORGUNIT_TARGET_WIDTH = 800;

private static final List<String> ALLOWED_AVATAR_FILE_EXTENSIONS =
private static final long MAX_AVATAR_IMAGE_SIZE_IN_BYTES = 2_000_000;
private static final long MAX_ORGUNIT_IMAGE_SIZE_IN_BYTES = 8_000_000;

private static final List<String> ALLOWED_IMAGE_FILE_EXTENSIONS =
List.of("jpg", "jpeg", "png", "gif");
private static final List<String> ALLOWED_AVATAR_MIME_TYPES =
private static final List<String> ALLOWED_IMAGE_MIME_TYPES =
List.of("image/jpeg", "image/png", "image/gif");

private static class MultipartFileByteSource extends ByteSource {
private MultipartFile file;

public MultipartFileByteSource(MultipartFile file) {
this.file = file;
}

@Override
public InputStream openStream() {
try {
return file.getInputStream();
} catch (IOException ioe) {
return new NullInputStream(0);
}
}
}

/**
* Transfers the given multipart file content to a local temporary file.
*
Expand Down Expand Up @@ -232,31 +253,16 @@ public FileResource saveFileResource(String uid, MultipartFile file, FileResourc
return fileResource;
}

// -------------------------------------------------------------------------
// Inner classes
// -------------------------------------------------------------------------

private class MultipartFileByteSource extends ByteSource {
private MultipartFile file;

public MultipartFileByteSource(MultipartFile file) {
this.file = file;
}

@Override
public InputStream openStream() {
try {
return file.getInputStream();
} catch (IOException ioe) {
return new NullInputStream(0);
}
}
public void validateOrgUnitImage(MultipartFile file) {
validateContentType(file.getContentType(), ALLOWED_IMAGE_MIME_TYPES);
validateFileExtension(file.getOriginalFilename(), ALLOWED_IMAGE_FILE_EXTENSIONS);
validateFileSize(file, MAX_ORGUNIT_IMAGE_SIZE_IN_BYTES);
}

public void validateUserAvatar(@Nonnull MultipartFile file) {
validateContentType(file.getContentType(), ALLOWED_AVATAR_MIME_TYPES);
validateFileExtension(file.getOriginalFilename(), ALLOWED_AVATAR_FILE_EXTENSIONS);
validateFileSize(file, MAX_AVATAR_FILE_SIZE_IN_BYTES);
validateContentType(file.getContentType(), ALLOWED_IMAGE_MIME_TYPES);
validateFileExtension(file.getOriginalFilename(), ALLOWED_IMAGE_FILE_EXTENSIONS);
validateFileSize(file, MAX_AVATAR_IMAGE_SIZE_IN_BYTES);
}

private void validateContentType(String contentType, @Nonnull List<String> validExtensions) {
Expand Down Expand Up @@ -342,4 +348,9 @@ public static MultipartFile resizeAvatarToDefaultSize(MultipartFile multipartFil
return resizeImageToCustomSize(
multipartFile, AVATAR_TARGET_WIDTH, AVATAR_TARGET_HEIGHT, Mode.AUTOMATIC);
}

public MultipartFile resizeOrgToDefaultSize(MultipartFile multipartFile) throws IOException {
return resizeImageToCustomSize(
multipartFile, ORGUNIT_TARGET_WIDTH, ORGUNIT_TARGET_HEIGHT, Mode.AUTOMATIC);
}
}
Loading