Skip to content

Commit 7bc9061

Browse files
jknackjknack
authored
Merge commit from fork
Fix path traversal and suffix bypass in Spring MVC integration
2 parents 3e27b0b + 61f4342 commit 7bc9061

4 files changed

Lines changed: 236 additions & 17 deletions

File tree

‎handlebars-springmvc/src/main/java/com/github/jknack/handlebars/springmvc/HandlebarsViewResolver.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,16 +163,23 @@ protected AbstractUrlBasedView buildView(final String viewName) throws Exception
163163
protected AbstractUrlBasedView configure(final HandlebarsView view) throws IOException {
164164
String url = view.getUrl();
165165
// Remove prefix & suffix.
166-
url = url.substring(getPrefix().length(), url.length() - getSuffix().length());
166+
String strippedUrl = url.substring(getPrefix().length(), url.length() - getSuffix().length());
167+
168+
// Strict web-layer rejection: View names from controllers should never contain
169+
// protocols, fragments, or traversal sequences.
170+
if (strippedUrl.contains(":") || strippedUrl.contains("#") || strippedUrl.contains("..")) {
171+
throw new IllegalArgumentException("Unsafe Spring MVC view name detected: " + strippedUrl);
172+
}
173+
167174
// Compile the template.
168175
try {
169-
view.setTemplate(handlebars.compile(url));
176+
view.setTemplate(handlebars.compile(strippedUrl));
170177
view.setValueResolver(valueResolvers.toArray(new ValueResolver[0]));
171178
} catch (IOException ex) {
172179
if (failOnMissingFile) {
173180
throw ex;
174181
}
175-
logger.debug("File not found: " + url);
182+
logger.debug("File not found: " + strippedUrl);
176183
}
177184
return view;
178185
}

‎handlebars-springmvc/src/main/java/com/github/jknack/handlebars/springmvc/SpringTemplateLoader.java‎

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import org.springframework.context.ApplicationContext;
1414
import org.springframework.core.io.Resource;
1515
import org.springframework.core.io.ResourceLoader;
16-
import org.springframework.util.ResourceUtils;
1716

1817
import com.github.jknack.handlebars.io.URLTemplateLoader;
1918

@@ -55,24 +54,61 @@ public SpringTemplateLoader(final ApplicationContext applicationContext) {
5554

5655
@Override
5756
protected URL getResource(final String location) throws IOException {
57+
// 1. Logical Bounds Check (Parity with d177cdee)
58+
// Spring locations often contain protocols (classpath:). We must strip it to normalize the
59+
// path.
60+
String pathPart = location;
61+
int protocolIndex = location.indexOf(":");
62+
if (protocolIndex != -1) {
63+
pathPart = location.substring(protocolIndex + 1);
64+
}
65+
66+
String resolvedPath =
67+
java.nio.file.Paths.get(pathPart)
68+
.normalize()
69+
.toString()
70+
.replace(java.io.File.separatorChar, '/');
71+
if (pathPart.startsWith("/") && !resolvedPath.startsWith("/")) {
72+
resolvedPath = "/" + resolvedPath;
73+
}
74+
75+
// Extract the raw path from the configured prefix
76+
String prefixPath = getPrefix();
77+
int prefixProtocolIndex = prefixPath.indexOf(":");
78+
if (prefixProtocolIndex != -1) {
79+
prefixPath = prefixPath.substring(prefixProtocolIndex + 1);
80+
}
81+
82+
// Enforce the boundary
83+
if (!prefixPath.equals("/") && !resolvedPath.startsWith(prefixPath)) {
84+
throw new IllegalArgumentException(
85+
"Path traversal attempt detected. Resolved path escapes Spring base prefix: " + location);
86+
}
87+
88+
// 2. Delegate to Spring
5889
Resource resource = loader.getResource(location);
5990
if (!resource.exists()) {
6091
return null;
6192
}
62-
return resource.getURL();
93+
94+
// 3. Post-resolution URL Component Validation (Fragment/Query injection)
95+
URL url = resource.getURL();
96+
validateNoUnsafeUrlComponents(url);
97+
98+
return url;
6399
}
64100

65-
@Override
66-
public String resolve(final String location) {
67-
String protocol = null;
68-
if (location.startsWith(ResourceUtils.CLASSPATH_URL_PREFIX)) {
69-
protocol = ResourceUtils.CLASSPATH_URL_PREFIX;
70-
} else if (location.startsWith(ResourceUtils.FILE_URL_PREFIX)) {
71-
protocol = ResourceUtils.FILE_URL_PREFIX;
101+
/**
102+
* Verifies that the resolved URL does not contain components that can bypass suffix validation.
103+
*
104+
* @param url The resolved URL.
105+
*/
106+
private void validateNoUnsafeUrlComponents(final URL url) {
107+
if (url.getRef() != null) {
108+
throw new IllegalArgumentException("Template URL must not contain a fragment: " + url);
72109
}
73-
if (protocol == null) {
74-
return super.resolve(location);
110+
if (url.getQuery() != null) {
111+
throw new IllegalArgumentException("Template URL must not contain a query: " + url);
75112
}
76-
return protocol + super.resolve(location.substring(protocol.length()));
77113
}
78114
}

‎handlebars-springmvc/src/test/java/com/github/jknack/handlebars/springmvc/HandlebarsViewResolverTest.java‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Handlebars.java: https://github.com/jknack/handlebars.java
3+
* Apache License Version 2.0 http://www.apache.org/licenses/LICENSE-2.0
4+
* Copyright (c) 2012 Edgar Espina
5+
*/
6+
package com.github.jknack.handlebars.springmvc;
7+
8+
import static org.junit.jupiter.api.Assertions.assertThrows;
9+
import static org.junit.jupiter.api.Assertions.assertTrue;
10+
import static org.mockito.Mockito.mock;
11+
import static org.mockito.Mockito.when;
12+
13+
import org.junit.jupiter.api.BeforeEach;
14+
import org.junit.jupiter.api.Test;
15+
16+
import com.github.jknack.handlebars.Handlebars;
17+
import com.github.jknack.handlebars.Template;
18+
19+
public class HandlebarsViewResolverTest {
20+
21+
private HandlebarsViewResolver viewResolver;
22+
private Handlebars handlebars;
23+
24+
@BeforeEach
25+
public void setUp() {
26+
handlebars = mock(Handlebars.class);
27+
viewResolver = new HandlebarsViewResolver(handlebars);
28+
viewResolver.setPrefix("/WEB-INF/views/");
29+
viewResolver.setSuffix(".hbs");
30+
}
31+
32+
@Test
33+
public void shouldRejectViewNameWithProtocolInjection() {
34+
HandlebarsView view = new HandlebarsView();
35+
// Simulate a view name resolving to a file protocol
36+
view.setUrl("/WEB-INF/views/file:/etc/passwd.hbs");
37+
38+
IllegalArgumentException exception =
39+
assertThrows(
40+
IllegalArgumentException.class,
41+
() -> {
42+
viewResolver.buildView("file:/etc/passwd"); // Triggers configure()
43+
});
44+
45+
assertTrue(exception.getMessage().contains("Unsafe Spring MVC view name detected"));
46+
}
47+
48+
@Test
49+
public void shouldRejectViewNameWithDirectoryTraversal() {
50+
HandlebarsView view = new HandlebarsView();
51+
view.setUrl("/WEB-INF/views/../../etc/passwd.hbs");
52+
53+
IllegalArgumentException exception =
54+
assertThrows(
55+
IllegalArgumentException.class,
56+
() -> {
57+
viewResolver.buildView("../../etc/passwd");
58+
});
59+
60+
assertTrue(exception.getMessage().contains("Unsafe Spring MVC view name detected"));
61+
}
62+
63+
@Test
64+
public void shouldRejectViewNameWithUrlFragment() {
65+
HandlebarsView view = new HandlebarsView();
66+
view.setUrl("/WEB-INF/views/secret.hbs#bypass.hbs");
67+
68+
IllegalArgumentException exception =
69+
assertThrows(
70+
IllegalArgumentException.class,
71+
() -> {
72+
viewResolver.buildView("secret.hbs#bypass");
73+
});
74+
75+
assertTrue(exception.getMessage().contains("Unsafe Spring MVC view name detected"));
76+
}
77+
78+
@Test
79+
public void shouldAcceptValidViewName() throws Exception {
80+
Template template = mock(Template.class);
81+
HandlebarsView view = new HandlebarsView();
82+
view.setUrl("/WEB-INF/views/home/index.hbs");
83+
84+
// Mock the compiler to prevent actual file I/O during this test
85+
when(handlebars.compile("home/index")).thenReturn(template);
86+
87+
// This should execute successfully without throwing an IllegalArgumentException
88+
viewResolver.buildView("home/index");
89+
}
90+
}

‎handlebars-springmvc/src/test/java/com/github/jknack/handlebars/springmvc/SpringTemplateLoaderTest.java‎

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,35 @@
55
*/
66
package com.github.jknack.handlebars.springmvc;
77

8-
import static org.junit.jupiter.api.Assertions.assertNotNull;
9-
import static org.junit.jupiter.api.Assertions.assertThrows;
8+
import static org.junit.jupiter.api.Assertions.*;
9+
import static org.mockito.ArgumentMatchers.anyString;
10+
import static org.mockito.Mockito.mock;
11+
import static org.mockito.Mockito.when;
1012

1113
import java.io.IOException;
14+
import java.net.URL;
1215

16+
import org.junit.jupiter.api.BeforeEach;
1317
import org.junit.jupiter.api.Test;
1418
import org.springframework.core.io.DefaultResourceLoader;
19+
import org.springframework.core.io.Resource;
20+
import org.springframework.core.io.ResourceLoader;
1521

1622
import com.github.jknack.handlebars.io.TemplateSource;
1723

1824
public class SpringTemplateLoaderTest {
1925

26+
private ResourceLoader resourceLoader;
27+
private SpringTemplateLoader templateLoader;
28+
29+
@BeforeEach
30+
public void setUp() {
31+
resourceLoader = mock(ResourceLoader.class);
32+
templateLoader = new SpringTemplateLoader(resourceLoader);
33+
templateLoader.setPrefix("classpath:/templates/");
34+
templateLoader.setSuffix(".hbs");
35+
}
36+
2037
@Test
2138
public void sourceAt() throws IOException {
2239
SpringTemplateLoader loader = new SpringTemplateLoader(new DefaultResourceLoader());
@@ -32,4 +49,73 @@ public void fileNotFound() throws IOException {
3249
IOException.class,
3350
() -> new SpringTemplateLoader(new DefaultResourceLoader()).sourceAt("missingFile"));
3451
}
52+
53+
@Test
54+
public void shouldEnforceLogicalBoundaryAgainstTraversal() throws IOException {
55+
// The attacker only injects the traversal sequence, not the prefix.
56+
String maliciousPath = "../../application.properties";
57+
58+
IllegalArgumentException exception =
59+
assertThrows(
60+
IllegalArgumentException.class,
61+
() -> {
62+
// sourceAt triggers resolve() and then getResource()
63+
templateLoader.sourceAt(maliciousPath);
64+
});
65+
66+
assertTrue(exception.getMessage().contains("escapes Spring base prefix"));
67+
}
68+
69+
@Test
70+
public void shouldBlockUrlFragmentInjection() throws IOException {
71+
Resource mockResource = mock(Resource.class);
72+
when(mockResource.exists()).thenReturn(true);
73+
74+
// Simulate Spring returning a URL that contains a fragment (#)
75+
URL maliciousUrl = new URL("file:///etc/passwd#.hbs");
76+
when(mockResource.getURL()).thenReturn(maliciousUrl);
77+
78+
// We must mock the exact string that the template loader will ask Spring for
79+
when(resourceLoader.getResource(anyString())).thenReturn(mockResource);
80+
81+
IllegalArgumentException exception =
82+
assertThrows(
83+
IllegalArgumentException.class,
84+
() -> {
85+
templateLoader.sourceAt("classpath:/templates/hack#.hbs");
86+
});
87+
88+
assertTrue(exception.getMessage().contains("Template URL must not contain a fragment"));
89+
}
90+
91+
@Test
92+
public void shouldBlockUrlQueryInjection() throws IOException {
93+
Resource mockResource = mock(Resource.class);
94+
when(mockResource.exists()).thenReturn(true);
95+
96+
// Simulate Spring returning a URL that contains a query (?)
97+
URL maliciousUrl = new URL("file:///etc/passwd?.hbs");
98+
when(mockResource.getURL()).thenReturn(maliciousUrl);
99+
100+
when(resourceLoader.getResource(anyString())).thenReturn(mockResource);
101+
102+
IllegalArgumentException exception =
103+
assertThrows(
104+
IllegalArgumentException.class,
105+
() -> {
106+
templateLoader.sourceAt("classpath:/templates/hack?.hbs");
107+
});
108+
109+
assertTrue(exception.getMessage().contains("Template URL must not contain a query"));
110+
}
111+
112+
@Test
113+
public void resolveShouldNotPreserveDynamicProtocols() {
114+
// The resolve method should simply append the prefix and suffix,
115+
// it should NOT extract "file:" and place it at the front of the string anymore.
116+
String resolved = templateLoader.resolve("file:/etc/passwd");
117+
118+
// Prefix + Input + Suffix
119+
assertEquals("classpath:/templates/file:/etc/passwd.hbs", resolved);
120+
}
35121
}

0 commit comments

Comments
 (0)