Skip to content

Commit 1c05b0b

Browse files
lachlan-robertssbordet
lachlan-roberts
and
sbordet
authored
Fixes #6263 - Review URI encoding in ConcatServlet & WelcomeFilter.
Review URI encoding in ConcatServlet & WelcomeFilter and improve testing. Signed-off-by: Lachlan Roberts <[email protected]> Signed-off-by: Simone Bordet <[email protected]> Co-authored-by: Simone Bordet <[email protected]>
1 parent 9cb9343 commit 1c05b0b

File tree

6 files changed

+353
-31
lines changed

6 files changed

+353
-31
lines changed

‎jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ public boolean doGet(HttpServletRequest request, HttpServletResponse response)
240240
// Find the content
241241
content = _contentFactory.getContent(pathInContext, response.getBufferSize());
242242
if (LOG.isDebugEnabled())
243-
LOG.info("content={}", content);
243+
LOG.debug("content={}", content);
244244

245245
// Not found?
246246
if (content == null || !content.getResource().exists())
@@ -430,7 +430,7 @@ protected void sendWelcome(HttpContent content, String pathInContext, boolean en
430430
return;
431431
}
432432

433-
RequestDispatcher dispatcher = context.getRequestDispatcher(welcome);
433+
RequestDispatcher dispatcher = context.getRequestDispatcher(URIUtil.encodePath(welcome));
434434
if (dispatcher != null)
435435
{
436436
// Forward to the index

‎jetty-servlets/src/main/java/org/eclipse/jetty/servlets/ConcatServlet.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
* appropriate. This means that when not in development mode, the servlet must be
6262
* restarted before changed content will be served.</p>
6363
*/
64+
@Deprecated
6465
public class ConcatServlet extends HttpServlet
6566
{
6667
private boolean _development;
@@ -125,7 +126,8 @@ else if (!type.equals(t))
125126
}
126127
}
127128

128-
RequestDispatcher dispatcher = getServletContext().getRequestDispatcher(path);
129+
// Use the original string and not the decoded path as the Dispatcher will decode again.
130+
RequestDispatcher dispatcher = getServletContext().getRequestDispatcher(part);
129131
if (dispatcher != null)
130132
dispatchers.add(dispatcher);
131133
}

‎jetty-servlets/src/main/java/org/eclipse/jetty/servlets/WelcomeFilter.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
import javax.servlet.ServletResponse;
2828
import javax.servlet.http.HttpServletRequest;
2929

30+
import org.eclipse.jetty.util.URIUtil;
31+
3032
/**
3133
* Welcome Filter
3234
* This filter can be used to server an index file for a directory
@@ -41,6 +43,7 @@
4143
*
4244
* Requests to "/some/directory" will be redirected to "/some/directory/".
4345
*/
46+
@Deprecated
4447
public class WelcomeFilter implements Filter
4548
{
4649
private String welcome;
@@ -61,7 +64,10 @@ public void doFilter(ServletRequest request,
6164
{
6265
String path = ((HttpServletRequest)request).getServletPath();
6366
if (welcome != null && path.endsWith("/"))
64-
request.getRequestDispatcher(path + welcome).forward(request, response);
67+
{
68+
String uriInContext = URIUtil.encodePath(URIUtil.addPaths(path, welcome));
69+
request.getRequestDispatcher(uriInContext).forward(request, response);
70+
}
6571
else
6672
chain.doFilter(request, response);
6773
}

‎jetty-servlets/src/test/java/org/eclipse/jetty/servlets/ConcatServletTest.java‎

Lines changed: 56 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import java.nio.charset.StandardCharsets;
2727
import java.nio.file.Files;
2828
import java.nio.file.Path;
29+
import java.util.stream.Stream;
2930
import javax.servlet.RequestDispatcher;
3031
import javax.servlet.ServletException;
3132
import javax.servlet.http.HttpServlet;
@@ -41,7 +42,12 @@
4142
import org.junit.jupiter.api.AfterEach;
4243
import org.junit.jupiter.api.BeforeEach;
4344
import org.junit.jupiter.api.Test;
45+
import org.junit.jupiter.params.ParameterizedTest;
46+
import org.junit.jupiter.params.provider.Arguments;
47+
import org.junit.jupiter.params.provider.MethodSource;
4448

49+
import static org.hamcrest.MatcherAssert.assertThat;
50+
import static org.hamcrest.Matchers.startsWith;
4551
import static org.junit.jupiter.api.Assertions.assertEquals;
4652
import static org.junit.jupiter.api.Assertions.assertNotNull;
4753
import static org.junit.jupiter.api.Assertions.assertNull;
@@ -112,7 +118,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
112118
}
113119

114120
@Test
115-
public void testWEBINFResourceIsNotServed() throws Exception
121+
public void testDirectoryNotAccessible() throws Exception
116122
{
117123
File directoryFile = MavenTestingUtils.getTargetTestingDir();
118124
Path directoryPath = directoryFile.toPath();
@@ -134,45 +140,68 @@ public void testWEBINFResourceIsNotServed() throws Exception
134140
// Verify that I can get the file programmatically, as required by the spec.
135141
assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js"));
136142

137-
// Having a path segment and then ".." triggers a special case
138-
// that the ConcatServlet must detect and avoid.
139-
String uri = contextPath + concatPath + "?/trick/../WEB-INF/one.js";
143+
// Make sure ConcatServlet cannot see file system files.
144+
String uri = contextPath + concatPath + "?/trick/../../" + directoryFile.getName();
140145
String request =
141146
"GET " + uri + " HTTP/1.1\r\n" +
142147
"Host: localhost\r\n" +
143148
"Connection: close\r\n" +
144149
"\r\n";
145150
String response = connector.getResponse(request);
146151
assertTrue(response.startsWith("HTTP/1.1 404 "));
152+
}
147153

148-
// Make sure ConcatServlet behaves well if it's case insensitive.
149-
uri = contextPath + concatPath + "?/trick/../web-inf/one.js";
150-
request =
151-
"GET " + uri + " HTTP/1.1\r\n" +
152-
"Host: localhost\r\n" +
153-
"Connection: close\r\n" +
154-
"\r\n";
155-
response = connector.getResponse(request);
156-
assertTrue(response.startsWith("HTTP/1.1 404 "));
154+
public static Stream<Arguments> webInfTestExamples()
155+
{
156+
return Stream.of(
157+
// Cannot access WEB-INF.
158+
Arguments.of("?/WEB-INF/", "HTTP/1.1 404 "),
159+
Arguments.of("?/WEB-INF/one.js", "HTTP/1.1 404 "),
160+
161+
// Having a path segment and then ".." triggers a special case that the ConcatServlet must detect and avoid.
162+
Arguments.of("?/trick/../WEB-INF/one.js", "HTTP/1.1 404 "),
163+
164+
// Make sure ConcatServlet behaves well if it's case insensitive.
165+
Arguments.of("?/trick/../web-inf/one.js", "HTTP/1.1 404 "),
166+
167+
// Make sure ConcatServlet behaves well if encoded.
168+
Arguments.of("?/trick/..%2FWEB-INF%2Fone.js", "HTTP/1.1 404 "),
169+
Arguments.of("?/%2557EB-INF/one.js", "HTTP/1.1 500 "),
170+
Arguments.of("?/js/%252e%252e/WEB-INF/one.js", "HTTP/1.1 500 ")
171+
);
172+
}
157173

158-
// Make sure ConcatServlet behaves well if encoded.
159-
uri = contextPath + concatPath + "?/trick/..%2FWEB-INF%2Fone.js";
160-
request =
161-
"GET " + uri + " HTTP/1.1\r\n" +
162-
"Host: localhost\r\n" +
163-
"Connection: close\r\n" +
164-
"\r\n";
165-
response = connector.getResponse(request);
166-
assertTrue(response.startsWith("HTTP/1.1 404 "));
174+
@ParameterizedTest
175+
@MethodSource("webInfTestExamples")
176+
public void testWEBINFResourceIsNotServed(String querystring, String expectedStatus) throws Exception
177+
{
178+
File directoryFile = MavenTestingUtils.getTargetTestingDir();
179+
Path directoryPath = directoryFile.toPath();
180+
Path hiddenDirectory = directoryPath.resolve("WEB-INF");
181+
Files.createDirectories(hiddenDirectory);
182+
Path hiddenResource = hiddenDirectory.resolve("one.js");
183+
try (OutputStream output = Files.newOutputStream(hiddenResource))
184+
{
185+
output.write("function() {}".getBytes(StandardCharsets.UTF_8));
186+
}
167187

168-
// Make sure ConcatServlet cannot see file system files.
169-
uri = contextPath + concatPath + "?/trick/../../" + directoryFile.getName();
170-
request =
188+
String contextPath = "";
189+
WebAppContext context = new WebAppContext(server, directoryPath.toString(), contextPath);
190+
server.setHandler(context);
191+
String concatPath = "/concat";
192+
context.addServlet(ConcatServlet.class, concatPath);
193+
server.start();
194+
195+
// Verify that I can get the file programmatically, as required by the spec.
196+
assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js"));
197+
198+
String uri = contextPath + concatPath + querystring;
199+
String request =
171200
"GET " + uri + " HTTP/1.1\r\n" +
172201
"Host: localhost\r\n" +
173202
"Connection: close\r\n" +
174203
"\r\n";
175-
response = connector.getResponse(request);
176-
assertTrue(response.startsWith("HTTP/1.1 404 "));
204+
String response = connector.getResponse(request);
205+
assertThat(response, startsWith(expectedStatus));
177206
}
178207
}

‎jetty-servlets/src/test/java/org/eclipse/jetty/servlets/WelcomeFilterTest.java‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
//
2+
// ========================================================================
3+
// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others.
4+
// ------------------------------------------------------------------------
5+
// All rights reserved. This program and the accompanying materials
6+
// are made available under the terms of the Eclipse Public License v1.0
7+
// and Apache License v2.0 which accompanies this distribution.
8+
//
9+
// The Eclipse Public License is available at
10+
// http://www.eclipse.org/legal/epl-v10.html
11+
//
12+
// The Apache License v2.0 is available at
13+
// http://www.opensource.org/licenses/apache2.0.php
14+
//
15+
// You may elect to redistribute this code under either of these licenses.
16+
// ========================================================================
17+
//
18+
19+
package org.eclipse.jetty.servlets;
20+
21+
import java.io.OutputStream;
22+
import java.nio.charset.StandardCharsets;
23+
import java.nio.file.Files;
24+
import java.nio.file.Path;
25+
import java.util.EnumSet;
26+
import java.util.stream.Stream;
27+
import javax.servlet.DispatcherType;
28+
29+
import org.eclipse.jetty.server.LocalConnector;
30+
import org.eclipse.jetty.server.Server;
31+
import org.eclipse.jetty.servlet.FilterHolder;
32+
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
33+
import org.eclipse.jetty.webapp.WebAppContext;
34+
import org.junit.jupiter.api.AfterEach;
35+
import org.junit.jupiter.api.BeforeEach;
36+
import org.junit.jupiter.params.ParameterizedTest;
37+
import org.junit.jupiter.params.provider.Arguments;
38+
import org.junit.jupiter.params.provider.MethodSource;
39+
40+
import static org.hamcrest.MatcherAssert.assertThat;
41+
import static org.hamcrest.Matchers.containsString;
42+
import static org.junit.jupiter.api.Assertions.assertNotNull;
43+
44+
public class WelcomeFilterTest
45+
{
46+
private Server server;
47+
private LocalConnector connector;
48+
49+
@BeforeEach
50+
public void prepareServer() throws Exception
51+
{
52+
server = new Server();
53+
connector = new LocalConnector(server);
54+
server.addConnector(connector);
55+
56+
Path directoryPath = MavenTestingUtils.getTargetTestingDir().toPath();
57+
Files.createDirectories(directoryPath);
58+
Path welcomeResource = directoryPath.resolve("welcome.html");
59+
try (OutputStream output = Files.newOutputStream(welcomeResource))
60+
{
61+
output.write("<h1>welcome page</h1>".getBytes(StandardCharsets.UTF_8));
62+
}
63+
64+
Path otherResource = directoryPath.resolve("other.html");
65+
try (OutputStream output = Files.newOutputStream(otherResource))
66+
{
67+
output.write("<h1>other resource</h1>".getBytes(StandardCharsets.UTF_8));
68+
}
69+
70+
Path hiddenDirectory = directoryPath.resolve("WEB-INF");
71+
Files.createDirectories(hiddenDirectory);
72+
Path hiddenResource = hiddenDirectory.resolve("one.js");
73+
try (OutputStream output = Files.newOutputStream(hiddenResource))
74+
{
75+
output.write("CONFIDENTIAL".getBytes(StandardCharsets.UTF_8));
76+
}
77+
78+
Path hiddenWelcome = hiddenDirectory.resolve("index.html");
79+
try (OutputStream output = Files.newOutputStream(hiddenWelcome))
80+
{
81+
output.write("CONFIDENTIAL".getBytes(StandardCharsets.UTF_8));
82+
}
83+
84+
WebAppContext context = new WebAppContext(server, directoryPath.toString(), "/");
85+
server.setHandler(context);
86+
String concatPath = "/*";
87+
88+
FilterHolder filterHolder = new FilterHolder(new WelcomeFilter());
89+
filterHolder.setInitParameter("welcome", "welcome.html");
90+
context.addFilter(filterHolder, concatPath, EnumSet.of(DispatcherType.REQUEST));
91+
server.start();
92+
93+
// Verify that I can get the file programmatically, as required by the spec.
94+
assertNotNull(context.getServletContext().getResource("/WEB-INF/one.js"));
95+
}
96+
97+
@AfterEach
98+
public void destroy() throws Exception
99+
{
100+
if (server != null)
101+
server.stop();
102+
}
103+
104+
public static Stream<Arguments> argumentsStream()
105+
{
106+
return Stream.of(
107+
// Normal requests for the directory are redirected to the welcome page.
108+
Arguments.of("/", new String[]{"HTTP/1.1 200 ", "<h1>welcome page</h1>"}),
109+
110+
// Try a normal resource (will bypass the filter).
111+
Arguments.of("/other.html", new String[]{"HTTP/1.1 200 ", "<h1>other resource</h1>"}),
112+
113+
// Cannot access files in WEB-INF.
114+
Arguments.of("/WEB-INF/one.js", new String[]{"HTTP/1.1 404 "}),
115+
116+
// Cannot serve welcome from WEB-INF.
117+
Arguments.of("/WEB-INF/", new String[]{"HTTP/1.1 404 "}),
118+
119+
// Try to trick the filter into serving a protected resource.
120+
Arguments.of("/WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}),
121+
Arguments.of("/js/../WEB-INF/one.js#/", new String[]{"HTTP/1.1 404 "}),
122+
123+
// Test the URI is not double decoded in the dispatcher.
124+
Arguments.of("/%2557EB-INF/one.js%23/", new String[]{"HTTP/1.1 404 "})
125+
);
126+
}
127+
128+
@ParameterizedTest
129+
@MethodSource("argumentsStream")
130+
public void testWelcomeFilter(String uri, String[] contains) throws Exception
131+
{
132+
String request =
133+
"GET " + uri + " HTTP/1.1\r\n" +
134+
"Host: localhost\r\n" +
135+
"Connection: close\r\n" +
136+
"\r\n";
137+
String response = connector.getResponse(request);
138+
for (String s : contains)
139+
{
140+
assertThat(response, containsString(s));
141+
}
142+
}
143+
}

0 commit comments

Comments
 (0)