Skip to content

Commit eab5e5e

Browse files
raushan3737raushan3737
committed
fix:34329-fixed upload svg logo in the workspace api
1 parent daaaa93 commit eab5e5e

File tree

4 files changed

+120
-2
lines changed

4 files changed

+120
-2
lines changed

‎app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import com.appsmith.server.services.AnalyticsService;
88
import lombok.RequiredArgsConstructor;
99
import lombok.extern.slf4j.Slf4j;
10+
import org.apache.commons.io.IOUtils;
1011
import org.springframework.core.io.buffer.DataBuffer;
1112
import org.springframework.core.io.buffer.DataBufferUtils;
1213
import org.springframework.core.io.buffer.DefaultDataBufferFactory;
@@ -25,9 +26,11 @@
2526
import java.awt.image.BufferedImage;
2627
import java.io.ByteArrayOutputStream;
2728
import java.io.IOException;
29+
import java.nio.charset.StandardCharsets;
2830
import java.util.List;
2931
import java.util.Objects;
3032
import java.util.Set;
33+
import java.util.regex.Pattern;
3134
import java.util.stream.Collectors;
3235

3336
import static com.appsmith.server.constants.Constraint.THUMBNAIL_PHOTO_DIMENSION;
@@ -43,11 +46,12 @@ public class AssetServiceCEImpl implements AssetServiceCE {
4346
private static final Set<MediaType> ALLOWED_CONTENT_TYPES = Set.of(
4447
MediaType.IMAGE_JPEG,
4548
MediaType.IMAGE_PNG,
49+
MediaType.valueOf("image/svg+xml"),
4650
MediaType.valueOf("image/x-icon"),
4751
MediaType.valueOf("image/vnd.microsoft.icon"));
4852

4953
private static final Set<String> ALLOWED_CONTENT_TYPES_STR =
50-
Set.of(MediaType.IMAGE_JPEG_VALUE, MediaType.IMAGE_PNG_VALUE);
54+
Set.of(MediaType.IMAGE_JPEG_VALUE, MediaType.IMAGE_PNG_VALUE, "image/svg+xml");
5155

5256
@Override
5357
public Mono<Asset> getById(String id) {
@@ -65,13 +69,46 @@ private Boolean checkImageTypeValidation(DataBuffer dataBuffer, MediaType conten
6569
means the file is not an image type file rather any other corrupted file but the extension has been
6670
changed to .png or .jpeg to upload the flawed file. This is a security vulnerability hence reject
6771
*/
68-
if (ALLOWED_CONTENT_TYPES_STR.contains(contentType.toString())) {
72+
Boolean isSvgType = MediaType.valueOf("image/svg+xml").equals(contentType);
73+
if (isSvgType) {
74+
String svgContent = IOUtils.toString(dataBuffer.asInputStream(), StandardCharsets.UTF_8);
75+
dataBuffer.readPosition(0);
76+
77+
if (!isSvgSafe(svgContent)) {
78+
// Throwing validation exception to return separate response for svg type.
79+
throw new AppsmithException(AppsmithError.VALIDATION_FAILURE, "Please upload a valid svg.");
80+
}
81+
} else if (ALLOWED_CONTENT_TYPES_STR.contains(contentType.toString())) {
6982
return false;
7083
}
7184
}
7285
return true;
7386
}
7487

88+
private boolean isSvgSafe(String svgContent) {
89+
// Array of regex patterns to check any malicious content like(style,link,script etc.) against SVG content
90+
String[] patterns = {
91+
"<script\\b[^<]*(?:(?!<\\/script>)<[^<]*)*<\\/script>",
92+
"<style\\b[^<]*(?:(?!<\\/style>)<[^<]*)*<\\/style>",
93+
"<a\\b[^>]*>",
94+
"</a>",
95+
"\\s(on[a-zA-Z]+|style)\\s*=\\s*(['\"]).*?\\2",
96+
"\\s(href|src)\\s*=\\s*(['\"])javascript:[^'\"]*\\2"
97+
};
98+
99+
// Compile patterns and check each against svgContent to detect any malicious code.
100+
for (String pattern : patterns) {
101+
if (Pattern.compile(pattern, Pattern.CASE_INSENSITIVE | Pattern.DOTALL)
102+
.matcher(svgContent)
103+
.find()) {
104+
return false; // Unsafe SVG
105+
}
106+
}
107+
108+
// If none of the patterns match, consider the SVG safe
109+
return true;
110+
}
111+
75112
@Override
76113
public Mono<Asset> upload(List<Part> fileParts, int maxFileSizeKB, boolean isThumbnail) {
77114
fileParts = fileParts.stream().filter(Objects::nonNull).collect(Collectors.toList());

‎app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1522,6 +1522,66 @@ public void testUpdateAndDeleteLogo_validLogo() throws IOException {
15221522
.verifyComplete();
15231523
}
15241524

1525+
@Test
1526+
@WithUserDetails(value = "api_user")
1527+
public void uploadWorkspaceLogo_validSvg() throws IOException {
1528+
FilePart filepart = Mockito.mock(FilePart.class, Mockito.RETURNS_DEEP_STUBS);
1529+
Flux<DataBuffer> dataBufferFlux = DataBufferUtils.read(
1530+
new ClassPathResource("test_assets/valid.svg"), new DefaultDataBufferFactory(), 4096)
1531+
.cache();
1532+
assertThat(dataBufferFlux.count().block())
1533+
.isLessThanOrEqualTo((int) Math.ceil(Constraint.WORKSPACE_LOGO_SIZE_KB / 4.0));
1534+
1535+
Mockito.when(filepart.content()).thenReturn(dataBufferFlux);
1536+
Mockito.when(filepart.headers().getContentType()).thenReturn(MediaType.valueOf("image/svg+xml"));
1537+
1538+
Mono<Workspace> createWorkspace = workspaceService.create(workspace).cache();
1539+
1540+
final Mono<Tuple2<Workspace, Asset>> resultMono = createWorkspace.flatMap(workspace -> workspaceService
1541+
.uploadLogo(workspace.getId(), filepart)
1542+
.flatMap(workspaceWithLogo -> Mono.zip(
1543+
Mono.just(workspaceWithLogo), assetRepository.findById(workspaceWithLogo.getLogoAssetId()))));
1544+
1545+
StepVerifier.create(resultMono)
1546+
.assertNext(tuple -> {
1547+
final Workspace workspaceWithLogo = tuple.getT1();
1548+
assertThat(workspaceWithLogo.getLogoUrl()).isNotNull();
1549+
assertThat(workspaceWithLogo.getLogoUrl()).contains(workspaceWithLogo.getLogoAssetId());
1550+
1551+
final Asset asset = tuple.getT2();
1552+
assertThat(asset).isNotNull();
1553+
})
1554+
.verifyComplete();
1555+
}
1556+
1557+
@Test
1558+
@WithUserDetails(value = "api_user")
1559+
public void uploadWorkspaceLogo_invalidSvg() throws IOException {
1560+
FilePart filepart = Mockito.mock(FilePart.class, Mockito.RETURNS_DEEP_STUBS);
1561+
Flux<DataBuffer> dataBufferFlux = DataBufferUtils.read(
1562+
new ClassPathResource("test_assets/invalid.svg"), new DefaultDataBufferFactory(), 4096)
1563+
.cache();
1564+
assertThat(dataBufferFlux.count().block())
1565+
.isLessThanOrEqualTo((int) Math.ceil(Constraint.WORKSPACE_LOGO_SIZE_KB / 4.0));
1566+
1567+
Mockito.when(filepart.content()).thenReturn(dataBufferFlux);
1568+
Mockito.when(filepart.headers().getContentType()).thenReturn(MediaType.valueOf("image/svg+xml"));
1569+
1570+
Mono<Workspace> createWorkspace = workspaceService.create(workspace).cache();
1571+
1572+
final Mono<Tuple2<Workspace, Asset>> resultMono = createWorkspace.flatMap(workspace -> workspaceService
1573+
.uploadLogo(workspace.getId(), filepart)
1574+
.flatMap(workspaceWithLogo -> Mono.zip(
1575+
Mono.just(workspaceWithLogo), assetRepository.findById(workspaceWithLogo.getLogoAssetId()))));
1576+
1577+
StepVerifier.create(resultMono)
1578+
.expectErrorMatches(throwable -> throwable instanceof AppsmithException
1579+
&& throwable
1580+
.getMessage()
1581+
.equals(AppsmithError.VALIDATION_FAILURE.getMessage("Please upload a valid svg.")))
1582+
.verify();
1583+
}
1584+
15251585
@Test
15261586
@WithUserDetails(value = "api_user")
15271587
public void delete_WhenWorkspaceHasApp_ThrowsException() {

‎app/server/appsmith-server/src/test/resources/test_assets/invalid.svg

Lines changed: 18 additions & 0 deletions
Loading

‎app/server/appsmith-server/src/test/resources/test_assets/valid.svg

Lines changed: 3 additions & 0 deletions
Loading

0 commit comments

Comments
 (0)