-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590
Changes from 1 commit
778f2ed
7b0bf37
6091dab
a1d9b05
0045c65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,8 @@ THE SOFTWARE. | |
<properties> | ||
<commons-fileupload2.version>2.0.0-M2</commons-fileupload2.version> | ||
<slf4jVersion>2.0.15</slf4jVersion> | ||
<stapler.version>1892.v73465f3d074d</stapler.version> | ||
<!-- TODO JENKINS-73122 https://github.com/jenkinsci/stapler/pull/537 --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
<stapler.version>1894.v29804a_df796e</stapler.version> | ||
<groovy.version>2.4.21</groovy.version> | ||
</properties> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ THE SOFTWARE. | |
<module>bom</module> | ||
<module>websocket/spi</module> | ||
<module>websocket/jetty10</module> | ||
<module>websocket/jetty12-ee8</module> | ||
<module>core</module> | ||
<module>war</module> | ||
<module>test</module> | ||
|
@@ -97,7 +98,8 @@ THE SOFTWARE. | |
<bridge-method-injector.version>1.29</bridge-method-injector.version> | ||
<spotless.check.skip>false</spotless.check.skip> | ||
<!-- Make sure to keep the jetty-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: --> | ||
<winstone.version>6.21</winstone.version> | ||
<!-- TODO JENKINS-73126 https://github.com/jenkinsci/winstone/pull/383 --> | ||
<winstone.version>7.0-rc921.b_dc6422f61b_6</winstone.version> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
</properties> | ||
|
||
<!-- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,7 +149,7 @@ THE SOFTWARE. | |
<dependency> | ||
<groupId>${project.groupId}</groupId> | ||
<artifactId>jenkins-test-harness</artifactId> | ||
<version>2225.2230.v6210cb_b_827f9</version> | ||
<version>2250.v03a_1295b_0a_30</version> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latest version of Jenkins Test Harness (JTH), which requires Java 17 and is based on Jetty 12, using reflection to choose between EE 8 and EE 9. |
||
<scope>test</scope> | ||
<exclusions> | ||
<exclusion> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,7 @@ public class PluginTest { | |
r.createWebClient().assertFails("plugin/matrix-auth/images/%2e%2e%2fWEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST); | ||
r.createWebClient().assertFails("plugin/matrix-auth/images/%2e.%2fWEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST); | ||
r.createWebClient().assertFails("plugin/matrix-auth/images/..%2f..%2f..%2f" + r.jenkins.getRootDir().getName() + "%2fsecrets%2fmaster.key", HttpServletResponse.SC_BAD_REQUEST); | ||
r.createWebClient().assertFails("plugin/matrix-auth/" + r.jenkins.getRootDir() + "/secrets/master.key", /* ./ prepended anyway */ HttpServletResponse.SC_NOT_FOUND); | ||
r.createWebClient().assertFails("plugin/matrix-auth/" + r.jenkins.getRootDir() + "/secrets/master.key", /* ./ prepended anyway */ Functions.isWindows() ? HttpServletResponse.SC_BAD_REQUEST : HttpServletResponse.SC_NOT_FOUND); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See JENKINS-73128 for an explanation of this change. Newer versions of the Servlet API are more secure by default and reject suspicious sequences on Windows with a 400 error rather than a 404. We appreciate this increased security, but our test must be adapted to expect the new error code on Windows. |
||
// SECURITY-155: | ||
r.createWebClient().assertFails("plugin/matrix-auth/WEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST); | ||
r.createWebClient().assertFails("plugin/matrix-auth/META-INF/MANIFEST.MF", HttpServletResponse.SC_BAD_REQUEST); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,8 +149,16 @@ public void doubleDots2() throws Exception { | |
p.getBuildersList().add(new Shell("mkdir abc; touch abc/def.bin")); | ||
j.buildAndAssertSuccess(p); | ||
|
||
// can we see it? | ||
j.createWebClient().goTo("job/" + p.getName() + "/ws/abc%5Cdef.bin", "application/octet-stream"); | ||
try (JenkinsRule.WebClient wc = j.createWebClient()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See JENKINS-73129 for an explanation of this change. Newer versions of the Servlet API are more secure by default and reject suspicious sequences with a 400 error rather than a 404. We appreciate this increased security, but our test must be adapted to expect the new error code on Windows. |
||
// normal path provided by the UI succeeds | ||
wc.goTo("job/" + p.getName() + "/ws/abc/def.bin", "application/octet-stream"); | ||
|
||
// suspicious path is rejected with 400 | ||
wc.setThrowExceptionOnFailingStatusCode(false); | ||
HtmlPage page = wc.goTo("job/" + p.getName() + "/ws/abc%5Cdef.bin"); | ||
assertEquals(400, page.getWebResponse().getStatusCode()); | ||
assertEquals("Error 400 Suspicious Path Character", page.getTitleText()); | ||
} | ||
} | ||
|
||
@Test | ||
|
@@ -1108,10 +1116,13 @@ public void windows_cannotViewAbsolutePath() throws Exception { | |
String content = "random data provided as fixed value"; | ||
Files.writeString(targetTmpPath, content, StandardCharsets.UTF_8); | ||
|
||
JenkinsRule.WebClient wc = j.createWebClient().withThrowExceptionOnFailingStatusCode(false); | ||
Page page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*", null); | ||
|
||
MatcherAssert.assertThat(page.getWebResponse().getStatusCode(), equalTo(404)); | ||
try (JenkinsRule.WebClient wc = j.createWebClient()) { | ||
// suspicious path is rejected with 400 | ||
wc.setThrowExceptionOnFailingStatusCode(false); | ||
HtmlPage page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*"); | ||
assertEquals(400, page.getWebResponse().getStatusCode()); | ||
assertEquals("Error 400 Suspicious Path Character", page.getTitleText()); | ||
} | ||
} | ||
|
||
@Test | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ | |
import java.net.URI; | ||
import java.net.URISyntaxException; | ||
import java.net.URL; | ||
import java.nio.ByteBuffer; | ||
import java.nio.charset.StandardCharsets; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
|
@@ -55,16 +56,18 @@ | |
import java.util.jar.Manifest; | ||
import java.util.zip.ZipEntry; | ||
import java.util.zip.ZipInputStream; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import jenkins.model.Jenkins; | ||
import jenkins.security.UpdateSiteWarningsConfiguration; | ||
import jenkins.security.UpdateSiteWarningsMonitor; | ||
import org.apache.commons.io.FilenameUtils; | ||
import org.eclipse.jetty.http.HttpHeader; | ||
import org.eclipse.jetty.http.HttpStatus; | ||
import org.eclipse.jetty.server.Handler; | ||
import org.eclipse.jetty.server.Request; | ||
import org.eclipse.jetty.server.Response; | ||
import org.eclipse.jetty.server.Server; | ||
import org.eclipse.jetty.server.ServerConnector; | ||
import org.eclipse.jetty.server.handler.AbstractHandler; | ||
import org.eclipse.jetty.util.Callback; | ||
import org.junit.After; | ||
import org.junit.Before; | ||
import org.junit.Rule; | ||
|
@@ -115,19 +118,21 @@ public void setUpWebServer() throws Exception { | |
server = new Server(); | ||
ServerConnector connector = new ServerConnector(server); | ||
server.addConnector(connector); | ||
server.setHandler(new AbstractHandler() { | ||
server.setHandler(new Handler.Abstract() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adapting to new Jetty 12 APIs as per the migration guide. |
||
@Override | ||
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
public boolean handle(Request request, Response response, Callback callback) throws IOException { | ||
String target = request.getHttpURI().getPath(); | ||
if (target.startsWith(RELATIVE_BASE)) { | ||
target = target.substring(RELATIVE_BASE.length()); | ||
} | ||
String responseBody = getResource(target); | ||
if (responseBody != null) { | ||
baseRequest.setHandled(true); | ||
response.setContentType("text/plain; charset=utf-8"); | ||
response.setStatus(HttpServletResponse.SC_OK); | ||
response.getOutputStream().write(responseBody.getBytes(StandardCharsets.UTF_8)); | ||
response.getHeaders().add(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8"); | ||
response.setStatus(HttpStatus.OK_200); | ||
response.write(true, ByteBuffer.wrap(responseBody.getBytes(StandardCharsets.UTF_8)), callback); | ||
return true; | ||
} | ||
return false; | ||
} | ||
}); | ||
server.start(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ | |
import java.io.IOException; | ||
import java.net.MalformedURLException; | ||
import java.net.URL; | ||
import java.nio.ByteBuffer; | ||
import java.nio.charset.StandardCharsets; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
|
@@ -50,15 +51,16 @@ | |
import java.security.cert.X509Certificate; | ||
import java.util.HashSet; | ||
import java.util.Set; | ||
import javax.servlet.ServletException; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import jenkins.model.Jenkins; | ||
import jenkins.util.JSONSignatureValidator; | ||
import org.eclipse.jetty.http.HttpHeader; | ||
import org.eclipse.jetty.http.HttpStatus; | ||
import org.eclipse.jetty.server.Handler; | ||
import org.eclipse.jetty.server.Request; | ||
import org.eclipse.jetty.server.Response; | ||
import org.eclipse.jetty.server.Server; | ||
import org.eclipse.jetty.server.ServerConnector; | ||
import org.eclipse.jetty.server.handler.AbstractHandler; | ||
import org.eclipse.jetty.util.Callback; | ||
import org.htmlunit.Page; | ||
import org.junit.Before; | ||
import org.junit.Rule; | ||
|
@@ -336,7 +338,7 @@ protected JSONSignatureValidator getJsonSignatureValidator(String name) { | |
} | ||
} | ||
|
||
private static class RemoteUpdateSiteHandler extends AbstractHandler { | ||
private static class RemoteUpdateSiteHandler extends Handler.Abstract { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
private String serverContext; | ||
private boolean includeSignature; | ||
|
||
|
@@ -347,15 +349,18 @@ private static class RemoteUpdateSiteHandler extends AbstractHandler { | |
} | ||
|
||
@Override | ||
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { | ||
String responseBody = getWebServerResource(target, request.getParameter("version")); | ||
public boolean handle(Request request, Response response, Callback callback) throws IOException { | ||
String target = request.getHttpURI().getPath(); | ||
String version = Request.extractQueryParameters(request).get("version").getValue(); | ||
String responseBody = getWebServerResource(target, version); | ||
if (responseBody != null) { | ||
baseRequest.setHandled(true); | ||
response.setContentType("text/plain; charset=utf-8"); | ||
response.setStatus(HttpServletResponse.SC_OK); | ||
response.getOutputStream().write(responseBody.getBytes(StandardCharsets.UTF_8)); | ||
response.getHeaders().add(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8"); | ||
response.setStatus(HttpStatus.OK_200); | ||
response.write(true, ByteBuffer.wrap(responseBody.getBytes(StandardCharsets.UTF_8)), callback); | ||
return true; | ||
} else { | ||
response.sendError(404); | ||
Response.writeError(request, response, callback, HttpStatus.NOT_FOUND_404); | ||
return true; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,14 @@ public HttpResponse doSubmitMultipart(StaplerRequest req) throws FileUploadExcep | |
return processMultipartAndUnwrap(req); | ||
} else { | ||
actualWrapped = Assert.assertThrows(expectedWrapped, () -> processMultipartAndUnwrap(req)); | ||
|
||
// The client might still be sending us more of the request, but we have had enough of it already and | ||
// have decided to stop processing it. Drain the read end of the socket so that the client can finish | ||
// sending its request in order to read the response we are about to provide. | ||
try (OutputStream os = OutputStream.nullOutputStream()) { | ||
req.getInputStream().transferTo(os); | ||
} | ||
Comment on lines
+276
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See JENKINS-73127 for an explanation of this change. |
||
|
||
return HttpResponses.ok(); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ | |
import static org.hamcrest.Matchers.nullValue; | ||
|
||
import javax.servlet.ServletContextEvent; | ||
import org.eclipse.jetty.server.handler.ContextHandler; | ||
import org.eclipse.jetty.ee8.webapp.WebAppContext; | ||
import org.hamcrest.Matchers; | ||
import org.junit.After; | ||
import org.junit.Assume; | ||
|
@@ -103,7 +103,7 @@ public void shouldReturnWebAppPropertyIfSystemPropertyNotSetAndDefaultIsSet() th | |
* @param value value of the property | ||
*/ | ||
protected void setWebAppInitParameter(String property, String value) { | ||
Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(ContextHandler.Context.class)); | ||
((ContextHandler.Context) j.jenkins.servletContext).getContextHandler().getInitParams().put(property, value); | ||
Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(WebAppContext.Context.class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adapting to new Jetty 12 APIs as per the migration guide. |
||
((WebAppContext.Context) j.jenkins.servletContext).getContextHandler().getInitParams().put(property, value); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,11 @@ THE SOFTWARE. | |
<artifactId>websocket-jetty10</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.main</groupId> | ||
<artifactId>websocket-jetty12-ee8</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> | ||
<!-- | ||
We bundle slf4j binding since we got some components (sshd for example) | ||
|
@@ -157,6 +162,7 @@ THE SOFTWARE. | |
<exclude>org.jenkins-ci.main:cli</exclude> | ||
<exclude>org.jenkins-ci.main:jenkins-core</exclude> | ||
<exclude>org.jenkins-ci.main:websocket-jetty10</exclude> | ||
<exclude>org.jenkins-ci.main:websocket-jetty12-ee8</exclude> | ||
<exclude>org.jenkins-ci.main:websocket-spi</exclude> | ||
<exclude>org.jvnet.winp:winp</exclude> | ||
<exclude>org.kohsuke.stapler:stapler</exclude> | ||
|
@@ -630,22 +636,24 @@ THE SOFTWARE. | |
</configuration> | ||
</plugin> | ||
<plugin> | ||
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-maven-plugin</artifactId> | ||
<version>10.0.20</version> | ||
<groupId>org.eclipse.jetty.ee8</groupId> | ||
<artifactId>jetty-ee8-maven-plugin</artifactId> | ||
<version>12.0.12</version> | ||
<configuration> | ||
<!-- | ||
Reload webapp when you hit ENTER. (See JETTY-282 for more) | ||
--> | ||
<reload>manual</reload> | ||
<scan>0</scan> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
<httpConnector> | ||
<host>${host}</host> | ||
<port>${port}</port> | ||
</httpConnector> | ||
<loginServices> | ||
<loginService implementation="org.eclipse.jetty.security.HashLoginService"> | ||
<name>default</name> | ||
<config>${basedir}/src/realm.properties</config> | ||
<config implementation="org.eclipse.jetty.maven.MavenResource"> | ||
<resourceAsString>${basedir}/src/realm.properties</resourceAsString> | ||
</config> | ||
Comment on lines
+654
to
+656
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
</loginService> | ||
</loginServices> | ||
<systemProperties> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exclusion remains valid, but not for the reason currently given. The exclusion remains valid for the reason given in the previous exclusion for
jakarta.servlet:jakarta.servlet-api
. Indeed, that reason applies to bothjakarta.servlet:jakarta.servlet-api
andjakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api
, so move these exclusions to the same section.