Skip to content

Commit 13f2670

Browse files
chirag-madlanigitar-bot
chirag-madlani
and
gitar-bot
authored
chore(ui): reduce intial loading with assets via adding compression (#25576)
* chore(ui): reduce intial loading with assets via adding compression * fix: resolve checkstyle and CodeQL security issues - Fix import ordering by moving static imports to the end - Add path traversal validation to prevent security vulnerability - Normalize paths and validate against resource directory to prevent directory traversal attacks - Handle null returns from getPathToCheck for invalid paths Co-authored-by: chirag-madlani <[email protected]> * enable compressed api response for saving load time * fix: address code review findings in OpenMetadataAssetServlet 1. Security: Enhanced path traversal protection - Add early rejection of paths containing '..' - Add logging for path traversal attempts - Add additional check for '..' in normalized paths 2. Quality: Improved exception handling - Add Slf4j logging annotation - Replace silent exception swallowing with debug logging - Log errors when compressed asset serving fails 3. Edge Case: Proper Accept-Encoding parsing - Add supportsEncoding() method to handle q-values - Reject encodings with q=0 (explicitly disabled) - Handle comma-separated encoding lists properly Co-authored-by: chirag-madlani <[email protected]> * fix build issue * add options to compression --------- Co-authored-by: Gitar <[email protected]> Co-authored-by: chirag-madlani <[email protected]>
1 parent 64c98ef commit 13f2670

File tree

11 files changed

+351
-7
lines changed

11 files changed

+351
-7
lines changed

‎conf/openmetadata.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ server:
6666

6767
# Response compression disabled for maximum throughput
6868
gzip:
69-
enabled: false
69+
enabled: true
7070

7171
# Thread pool configuration
7272
# With virtual threads enabled (Java 21+), these settings become less critical

‎openmetadata-service/src/main/java/org/openmetadata/service/socket/OpenMetadataAssetServlet.java‎

Lines changed: 149 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,24 @@
1818
import io.dropwizard.servlets.assets.AssetServlet;
1919
import jakarta.servlet.ServletException;
2020
import jakarta.servlet.http.HttpServletRequest;
21+
import jakarta.servlet.http.HttpServletRequestWrapper;
2122
import jakarta.servlet.http.HttpServletResponse;
23+
import jakarta.servlet.http.HttpServletResponseWrapper;
2224
import java.io.IOException;
25+
import java.net.URL;
2326
import java.nio.charset.StandardCharsets;
27+
import java.nio.file.Path;
28+
import java.nio.file.Paths;
29+
import lombok.extern.slf4j.Slf4j;
2430
import org.jetbrains.annotations.Nullable;
2531
import org.openmetadata.service.config.OMWebConfiguration;
2632
import org.openmetadata.service.resources.system.IndexResource;
2733

34+
@Slf4j
2835
public class OpenMetadataAssetServlet extends AssetServlet {
2936
private final OMWebConfiguration webConfiguration;
3037
private final String basePath;
38+
private final String resourcePath;
3139

3240
public OpenMetadataAssetServlet(
3341
String basePath,
@@ -36,6 +44,7 @@ public OpenMetadataAssetServlet(
3644
@Nullable String indexFile,
3745
OMWebConfiguration webConf) {
3846
super(resourcePath, uriPath, indexFile, "text/html", StandardCharsets.UTF_8);
47+
this.resourcePath = resourcePath;
3948
this.webConfiguration = webConf;
4049
this.basePath = basePath;
4150
}
@@ -54,6 +63,41 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
5463
return;
5564
}
5665

66+
String acceptEncoding = req.getHeader("Accept-Encoding");
67+
68+
// 1. Check for Brotli (br)
69+
if (supportsEncoding(acceptEncoding, "br")) {
70+
try {
71+
String fullResourcePath = getPathToCheck(req, requestUri, ".br");
72+
if (fullResourcePath != null) {
73+
URL url = this.getClass().getResource(fullResourcePath);
74+
if (url != null) {
75+
serveCompressed(req, resp, requestUri, "br", "br");
76+
return;
77+
}
78+
}
79+
} catch (Exception e) {
80+
LOG.debug("Failed to serve Brotli compressed asset for {}: {}", requestUri, e.getMessage());
81+
}
82+
}
83+
84+
// 2. Check for Gzip
85+
if (supportsEncoding(acceptEncoding, "gzip")) {
86+
try {
87+
String fullResourcePath = getPathToCheck(req, requestUri, ".gz");
88+
if (fullResourcePath != null) {
89+
URL url = this.getClass().getResource(fullResourcePath);
90+
91+
if (url != null) {
92+
serveCompressed(req, resp, requestUri, "gzip", "gz");
93+
return;
94+
}
95+
}
96+
} catch (Exception e) {
97+
LOG.debug("Failed to serve Gzip compressed asset for {}: {}", requestUri, e.getMessage());
98+
}
99+
}
100+
57101
super.doGet(req, resp);
58102

59103
// For SPA routing: serve index.html for 404s that don't look like static asset requests
@@ -70,10 +114,112 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
70114
}
71115

72116
/**
73-
* Check if the request URI looks like a SPA route (not a static asset)
117+
* Check if the Accept-Encoding header supports the given encoding with non-zero quality value.
118+
* Handles q-values properly (e.g., "br;q=0" means encoding is explicitly disabled).
119+
*/
120+
private boolean supportsEncoding(String acceptEncoding, String encoding) {
121+
if (acceptEncoding == null || acceptEncoding.isEmpty()) {
122+
return false;
123+
}
124+
125+
// Split by comma to handle multiple encodings
126+
String[] encodings = acceptEncoding.toLowerCase().split(",");
127+
for (String enc : encodings) {
128+
enc = enc.trim();
129+
130+
// Check if this encoding matches
131+
if (enc.startsWith(encoding)) {
132+
// Check for q=0 which explicitly disables the encoding
133+
return !enc.contains("q=0");
134+
}
135+
}
136+
return false;
137+
}
138+
139+
private String getPathToCheck(HttpServletRequest req, String requestUri, String extension) {
140+
String pathToCheck = requestUri;
141+
String contextPath = req.getContextPath();
142+
if (contextPath != null && requestUri.startsWith(contextPath)) {
143+
pathToCheck = requestUri.substring(contextPath.length());
144+
}
145+
146+
// Reject path traversal attempts early
147+
if (pathToCheck.contains("..")) {
148+
LOG.warn("Path traversal attempt detected in request: {}", requestUri);
149+
return null;
150+
}
151+
152+
String fullPath =
153+
this.resourcePath + (pathToCheck.startsWith("/") ? "" : "/") + pathToCheck + extension;
154+
155+
// Validate against path traversal attacks
156+
try {
157+
Path normalizedPath = Paths.get(fullPath).normalize();
158+
Path baseResourcePath = Paths.get(this.resourcePath).normalize();
159+
160+
// Check path is within resource directory
161+
if (!normalizedPath.startsWith(baseResourcePath)) {
162+
LOG.warn("Path traversal attempt detected: {} escaped resource directory", requestUri);
163+
return null;
164+
}
165+
166+
// Additional check: normalized path should not go backwards
167+
if (normalizedPath.toString().contains("..")) {
168+
LOG.warn("Path contains .. after normalization: {}", requestUri);
169+
return null;
170+
}
171+
} catch (Exception e) {
172+
LOG.debug("Path validation failed for {}: {}", requestUri, e.getMessage());
173+
return null;
174+
}
175+
176+
return fullPath;
177+
}
178+
179+
private void serveCompressed(
180+
HttpServletRequest req,
181+
HttpServletResponse resp,
182+
String requestUri,
183+
String contentEncoding,
184+
String extension)
185+
throws ServletException, IOException {
186+
resp.setHeader("Content-Encoding", contentEncoding);
187+
String mimeType = req.getServletContext().getMimeType(requestUri);
188+
189+
HttpServletRequestWrapper compressedReq =
190+
new HttpServletRequestWrapper(req) {
191+
@Override
192+
public String getPathInfo() {
193+
String pathInfo = super.getPathInfo();
194+
return pathInfo != null ? pathInfo + "." + extension : null;
195+
}
196+
197+
@Override
198+
public String getRequestURI() {
199+
return super.getRequestURI() + "." + extension;
200+
}
201+
};
202+
203+
HttpServletResponseWrapper compressedResp =
204+
new HttpServletResponseWrapper(resp) {
205+
@Override
206+
public void setContentType(String type) {
207+
if (mimeType != null) {
208+
super.setContentType(mimeType);
209+
} else {
210+
super.setContentType(type);
211+
}
212+
}
213+
};
214+
215+
super.doGet(compressedReq, compressedResp);
216+
}
217+
218+
/**
219+
* Check if the request URI looks like an SPA route (not a static asset)
74220
* Static assets typically have file extensions, SPA routes don't
75221
* @param requestUri The request URI to check
76-
* @return true if this should be treated as a SPA route, false if it's a static asset
222+
* @return true if this should be treated as an SPA route, false if it's a static asset
77223
*/
78224
private boolean isSpaRoute(String requestUri) {
79225
// Remove base path if present
@@ -90,11 +236,8 @@ private boolean isSpaRoute(String requestUri) {
90236
// If path has a file extension, it's likely a static asset
91237
// Don't serve index.html for these
92238
String fileName = pathToCheck.substring(pathToCheck.lastIndexOf('/') + 1);
93-
if (fileName.contains(".")) {
94-
return false; // Has extension, likely a static asset
95-
}
239+
return !fileName.contains("."); // Has extension, likely a static asset
96240

97241
// No file extension, treat as SPA route
98-
return true;
99242
}
100243
}

‎openmetadata-service/src/test/java/org/openmetadata/service/socket/OpenMetadataAssetServletTest.java‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
package org.openmetadata.service.socket;
2+
3+
import static org.mockito.ArgumentMatchers.anyString;
4+
import static org.mockito.ArgumentMatchers.eq;
5+
import static org.mockito.Mockito.*;
6+
7+
import jakarta.servlet.ServletContext;
8+
import jakarta.servlet.ServletOutputStream;
9+
import jakarta.servlet.http.HttpServletRequest;
10+
import jakarta.servlet.http.HttpServletResponse;
11+
import org.junit.jupiter.api.BeforeEach;
12+
import org.junit.jupiter.api.Test;
13+
import org.mockito.Mock;
14+
import org.mockito.MockitoAnnotations;
15+
import org.openmetadata.service.config.OMWebConfiguration;
16+
17+
public class OpenMetadataAssetServletTest {
18+
19+
private OpenMetadataAssetServlet servlet;
20+
21+
@Mock private HttpServletRequest request;
22+
@Mock private HttpServletResponse response;
23+
@Mock private ServletContext servletContext;
24+
@Mock private OMWebConfiguration webConfiguration;
25+
@Mock private ServletOutputStream outputStream;
26+
27+
@BeforeEach
28+
public void setup() throws Exception {
29+
MockitoAnnotations.openMocks(this);
30+
when(request.getServletContext()).thenReturn(servletContext);
31+
when(response.getOutputStream()).thenReturn(outputStream);
32+
33+
// Initialize servlet with /assets as resource path
34+
servlet = new OpenMetadataAssetServlet("/", "/assets", "/", "index.html", webConfiguration);
35+
}
36+
37+
@Test
38+
public void testServeGzipAsset() throws Exception {
39+
// Setup request for test.js
40+
String path = "/test.js";
41+
when(request.getRequestURI()).thenReturn(path);
42+
when(request.getContextPath()).thenReturn("");
43+
when(request.getPathInfo()).thenReturn(path);
44+
when(request.getServletPath()).thenReturn("");
45+
when(request.getHeader("Accept-Encoding")).thenReturn("gzip, deflate");
46+
when(request.getMethod()).thenReturn("GET");
47+
when(request.getDateHeader(anyString())).thenReturn(-1L);
48+
when(request.getHeader("If-None-Match")).thenReturn(null);
49+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
50+
when(servletContext.getMimeType(anyString())).thenReturn("application/javascript");
51+
52+
try {
53+
servlet.doGet(request, response);
54+
} catch (Exception e) {
55+
e.printStackTrace();
56+
throw e;
57+
}
58+
59+
// Verify Content-Encoding header is set
60+
verify(response).setHeader("Content-Encoding", "gzip");
61+
// Verify Content-Type is set to javascript (not octet-stream or whatever .gz might imply)
62+
verify(response).setContentType("application/javascript");
63+
}
64+
65+
@Test
66+
public void testServeNormalAssetIfGzipMissing() throws Exception {
67+
// Setup request for normal.js (which has no .gz version)
68+
String path = "/normal.js";
69+
when(request.getRequestURI()).thenReturn(path);
70+
when(request.getContextPath()).thenReturn("");
71+
when(request.getPathInfo()).thenReturn(path);
72+
when(request.getServletPath()).thenReturn("");
73+
when(request.getHeader("Accept-Encoding")).thenReturn("gzip, deflate");
74+
when(request.getMethod()).thenReturn("GET");
75+
when(request.getDateHeader(anyString())).thenReturn(-1L);
76+
when(request.getHeader("If-None-Match")).thenReturn(null);
77+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
78+
79+
servlet.doGet(request, response);
80+
81+
// Verify Content-Encoding is NOT set
82+
verify(response, never()).setHeader(eq("Content-Encoding"), anyString());
83+
}
84+
85+
@Test
86+
public void testServeNormalAssetIfGzipNotAccepted() throws Exception {
87+
// Setup request for test.js (which HAS .gz version)
88+
String path = "/test.js";
89+
when(request.getRequestURI()).thenReturn(path);
90+
when(request.getContextPath()).thenReturn("");
91+
when(request.getPathInfo()).thenReturn(path);
92+
when(request.getServletPath()).thenReturn("");
93+
when(request.getHeader("Accept-Encoding")).thenReturn(null);
94+
when(request.getMethod()).thenReturn("GET");
95+
when(request.getDateHeader(anyString())).thenReturn(-1L);
96+
when(request.getHeader("If-None-Match")).thenReturn(null);
97+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
98+
99+
servlet.doGet(request, response);
100+
101+
// Verify Content-Encoding is NOT set
102+
verify(response, never()).setHeader(eq("Content-Encoding"), anyString());
103+
}
104+
105+
@Test
106+
public void testServeBrotliAsset() throws Exception {
107+
// Setup request for test.js (which has .br version)
108+
String path = "/test.js";
109+
when(request.getRequestURI()).thenReturn(path);
110+
when(request.getContextPath()).thenReturn("");
111+
when(request.getPathInfo()).thenReturn(path);
112+
when(request.getServletPath()).thenReturn("");
113+
when(request.getHeader("Accept-Encoding")).thenReturn("br"); // Only asking for br
114+
when(request.getMethod()).thenReturn("GET");
115+
when(request.getDateHeader(anyString())).thenReturn(-1L);
116+
when(request.getHeader("If-None-Match")).thenReturn(null);
117+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
118+
when(servletContext.getMimeType(anyString())).thenReturn("application/javascript");
119+
120+
try {
121+
servlet.doGet(request, response);
122+
} catch (Exception e) {
123+
e.printStackTrace();
124+
throw e;
125+
}
126+
127+
// Verify Content-Encoding is br
128+
verify(response).setHeader("Content-Encoding", "br");
129+
verify(response).setContentType("application/javascript");
130+
}
131+
132+
@Test
133+
public void testPrioritizeBrotliOverGzip() throws Exception {
134+
// Setup request for test.js (which has BOTH .br and .gz)
135+
String path = "/test.js";
136+
when(request.getRequestURI()).thenReturn(path);
137+
when(request.getContextPath()).thenReturn("");
138+
when(request.getPathInfo()).thenReturn(path);
139+
when(request.getServletPath()).thenReturn("");
140+
when(request.getHeader("Accept-Encoding")).thenReturn("gzip, deflate, br"); // Asking for both
141+
when(request.getMethod()).thenReturn("GET");
142+
when(request.getDateHeader(anyString())).thenReturn(-1L);
143+
when(request.getHeader("If-None-Match")).thenReturn(null);
144+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
145+
when(servletContext.getMimeType(anyString())).thenReturn("application/javascript");
146+
147+
try {
148+
servlet.doGet(request, response);
149+
} catch (Exception e) {
150+
e.printStackTrace();
151+
throw e;
152+
}
153+
154+
// Verify Content-Encoding is br (Prioritized)
155+
verify(response).setHeader("Content-Encoding", "br");
156+
verify(response).setContentType("application/javascript");
157+
}
158+
159+
@Test
160+
public void testFallbackToGzipIfBrotliMissing() throws Exception {
161+
// Setup request for gzip_only.js (which has .gz but NO .br)
162+
String path = "/gzip_only.js";
163+
when(request.getRequestURI()).thenReturn(path);
164+
when(request.getContextPath()).thenReturn("");
165+
when(request.getPathInfo()).thenReturn(path);
166+
when(request.getServletPath()).thenReturn("");
167+
when(request.getHeader("Accept-Encoding")).thenReturn("gzip, deflate, br"); // Asking for both
168+
when(request.getMethod()).thenReturn("GET");
169+
when(request.getDateHeader(anyString())).thenReturn(-1L);
170+
when(request.getHeader("If-None-Match")).thenReturn(null);
171+
when(request.getHeader("If-Modified-Since")).thenReturn(null);
172+
when(servletContext.getMimeType(anyString())).thenReturn("application/javascript");
173+
174+
try {
175+
servlet.doGet(request, response);
176+
} catch (Exception e) {
177+
e.printStackTrace();
178+
throw e;
179+
}
180+
181+
// Verify Content-Encoding is gzip (Fallback)
182+
verify(response).setHeader("Content-Encoding", "gzip");
183+
verify(response).setContentType("application/javascript");
184+
}
185+
}

‎openmetadata-service/src/test/resources/assets/gzip_only.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.log('gzip only');

0 commit comments

Comments
 (0)