Merge branch 'master' into jetty-9.4.x-Feature

jetty-client/src/main/java/org/eclipse/jetty/client/RedirectProtocolHandler.java

@@ -21,6 +21,8 @@ package org.eclipse.jetty.client;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
+import org.eclipse.jetty.http.HttpField;
+import org.eclipse.jetty.http.HttpHeader;
 
 /**
  * <p>A protocol handler that handles redirect status codes 301, 302, 303, 307 and 308.</p>
@@ -54,6 +56,14 @@ public class RedirectProtocolHandler extends Response.Listener.Adapter implement
         return this;
     }
 
+    @Override
+    public boolean onHeader(Response response, HttpField field)
+    {
+        // Avoid that the content is decoded, which could generate
+        // errors, since we are discarding the content anyway.
+        return field.getHeader() != HttpHeader.CONTENT_ENCODING;
+    }
+
     @Override
     public void onComplete(Result result)
     {

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java

@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.net.URLDecoder;
 import java.nio.ByteBuffer;
 import java.nio.channels.UnresolvedAddressException;
+import java.nio.charset.StandardCharsets;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -44,7 +45,6 @@ import org.eclipse.jetty.toolchain.test.IO;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -55,15 +55,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         super(sslContextFactory);
     }
 
-    @Before
-    public void prepare() throws Exception
-    {
-        start(new RedirectHandler());
-    }
-
     @Test
     public void test_303() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/done")
@@ -77,6 +73,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_303_302() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/302/localhost/done")
@@ -90,6 +88,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_303_302_OnDifferentDestinations() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/127.0.0.1/302/localhost/done")
@@ -103,6 +103,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_301() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .method(HttpMethod.HEAD)
@@ -117,6 +119,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_301_WithWrongMethod() throws Exception
     {
+        start(new RedirectHandler());
+
         try
         {
             client.newRequest("localhost", connector.getLocalPort())
@@ -140,6 +144,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_307_WithRequestContent() throws Exception
     {
+        start(new RedirectHandler());
+
         byte[] data = new byte[]{0, 1, 2, 3, 4, 5, 6, 7};
         ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
@@ -157,6 +163,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testMaxRedirections() throws Exception
     {
+        start(new RedirectHandler());
         client.setMaxRedirects(1);
 
         try
@@ -181,6 +188,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void test_303_WithConnectionClose_WithBigRequest() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/done?close=true")
@@ -194,6 +203,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testDontFollowRedirects() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .followRedirects(false)
@@ -208,6 +219,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testRelativeLocation() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/done?relative=true")
@@ -221,6 +234,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testAbsoluteURIPathWithSpaces() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/a+space?decode=true")
@@ -234,6 +249,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testRelativeURIPathWithSpaces() throws Exception
     {
+        start(new RedirectHandler());
+
         Response response = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/303/localhost/a+space?relative=true&decode=true")
@@ -247,7 +264,6 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testRedirectWithWrongScheme() throws Exception
     {
-        dispose();
         start(new AbstractHandler()
         {
             @Override
@@ -264,14 +280,10 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
                 .scheme(scheme)
                 .path("/path")
                 .timeout(5, TimeUnit.SECONDS)
-                .send(new Response.CompleteListener()
+                .send(result ->
-                {
-                    @Override
-                    public void onComplete(Result result)
                 {
                     Assert.assertTrue(result.isFailed());
                     latch.countDown();
-                    }
                 });
         Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
@@ -281,6 +293,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     public void testRedirectFailed() throws Exception
     {
         // TODO this test is failing with timout after an ISP upgrade??  DNS dependent?
+        start(new RedirectHandler());
+
         try
         {
             client.newRequest("localhost", connector.getLocalPort())
@@ -370,6 +384,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @Test
     public void testHttpRedirector() throws Exception
     {
+        start(new RedirectHandler());
         final HttpRedirector redirector = new HttpRedirector(client);
 
         org.eclipse.jetty.client.api.Request request1 = client.newRequest("localhost", connector.getLocalPort())
@@ -390,20 +405,52 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         Assert.assertTrue(redirector.isRedirect(response2));
 
         final CountDownLatch latch = new CountDownLatch(1);
-        redirector.redirect(request2, response2, new Response.CompleteListener()
+        redirector.redirect(request2, response2, r ->
         {
-            @Override
+            Response response3 = r.getResponse();
-            public void onComplete(Result result)
-            {
-                Response response3 = result.getResponse();
             Assert.assertEquals(200, response3.getStatus());
             Assert.assertFalse(redirector.isRedirect(response3));
             latch.countDown();
-            }
         });
         Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
 
+    @Test
+    public void testRedirectWithCorruptedBody() throws Exception
+    {
+        byte[] bytes = "ok".getBytes(StandardCharsets.UTF_8);
+        start(new AbstractHandler()
+        {
+            @Override
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+            {
+                baseRequest.setHandled(true);
+                if (target.startsWith("/redirect"))
+                {
+                    response.setStatus(HttpStatus.SEE_OTHER_303);
+                    response.setHeader(HttpHeader.LOCATION.asString(), scheme + "://localhost:" + connector.getLocalPort() + "/ok");
+                    // Say that we send gzipped content, but actually don't.
+                    response.setHeader(HttpHeader.CONTENT_ENCODING.asString(), "gzip");
+                    response.getOutputStream().write("redirect".getBytes(StandardCharsets.UTF_8));
+                }
+                else
+                {
+                    response.setStatus(HttpStatus.OK_200);
+                    response.getOutputStream().write(bytes);
+                }
+            }
+        });
+
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+                .scheme(scheme)
+                .path("/redirect")
+                .timeout(5, TimeUnit.SECONDS)
+                .send();
+
+        Assert.assertEquals(200, response.getStatus());
+        Assert.assertArrayEquals(bytes, response.getContent());
+    }
+
     private void testSameMethodRedirect(final HttpMethod method, int redirectCode) throws Exception
     {
         testMethodRedirect(method, method, redirectCode);
@@ -416,6 +463,8 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
 
     private void testMethodRedirect(final HttpMethod requestMethod, final HttpMethod redirectMethod, int redirectCode) throws Exception
     {
+        start(new RedirectHandler());
+
         final AtomicInteger passes = new AtomicInteger();
         client.getRequestListeners().add(new org.eclipse.jetty.client.api.Request.Listener.Adapter()
         {

jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java

@@ -841,17 +841,13 @@ public class HttpParser
                 switch (_header)
                 {
                     case CONTENT_LENGTH:
-                        if (_endOfContent != EndOfContent.CHUNKED_CONTENT)
+                        if (_endOfContent == EndOfContent.CONTENT_LENGTH)
                         {
-                            try
+                            throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Duplicate Content-Length");
-                            {
-                                _contentLength=Long.parseLong(_valueString);
                         }
-                            catch(NumberFormatException e)
+                        else if (_endOfContent != EndOfContent.CHUNKED_CONTENT)
                         {
-                                LOG.ignore(e);
+                            _contentLength=convertContentLength(_valueString);
-                                throw new BadMessageException(HttpStatus.BAD_REQUEST_400,"Bad Content-Length");
-                            }
                             if (_contentLength <= 0)
                                 _endOfContent=EndOfContent.NO_CONTENT;
                             else
@@ -861,16 +857,17 @@ public class HttpParser
 
                     case TRANSFER_ENCODING:
                         if (_value==HttpHeaderValue.CHUNKED)
+                        {
                             _endOfContent=EndOfContent.CHUNKED_CONTENT;
+                            _contentLength=-1;
+                        }
                         else
                         {
                             if (_valueString.endsWith(HttpHeaderValue.CHUNKED.toString()))
                                 _endOfContent=EndOfContent.CHUNKED_CONTENT;
                             else if (_valueString.contains(HttpHeaderValue.CHUNKED.toString()))
-                            {
                                 throw new BadMessageException(HttpStatus.BAD_REQUEST_400,"Bad chunking");
                         }
-                        }
                         break;
 
                     case HOST:
@@ -919,6 +916,18 @@ public class HttpParser
         _field=null;
     }
 
+    private long convertContentLength(String valueString)
+    {
+        try
+        {
+            return Long.parseLong(valueString);
+        }
+        catch(NumberFormatException e)
+        {
+            LOG.ignore(e);
+            throw new BadMessageException(HttpStatus.BAD_REQUEST_400,"Invalid Content-Length Value");
+        }
+    }
 
     /* ------------------------------------------------------------------------------- */
     /*

jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/LdapLoginModule.java

@@ -415,11 +415,14 @@ public class LdapLoginModule extends AbstractLoginModule
                 return isAuthenticated();
             }
 
+            boolean authed = false;
+
             if (_forceBindingLogin)
             {
-                return bindingLogin(webUserName, webCredential);
+                authed = bindingLogin(webUserName, webCredential);
             }
-
+            else
+            {
                 // This sets read and the credential
                 UserInfo userInfo = getUserInfo(webUserName);
 
@@ -431,11 +434,11 @@ public class LdapLoginModule extends AbstractLoginModule
 
                 setCurrentUser(new JAASUserInfo(userInfo));
 
-            boolean authed = false;            
                 if (webCredential instanceof String)
                     authed = credentialLogin(Credential.getCredential((String) webCredential));
                 else
                     authed = credentialLogin(webCredential);
+            }
 
             //only fetch roles if authenticated
             if (authed)

jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java

@@ -24,12 +24,14 @@ import java.io.DataOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
+import java.io.FilenameFilter;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.ObjectOutputStream;
 import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -98,11 +100,10 @@ public class FileSessionDataStore extends AbstractSessionDataStore
         File file = null;
         if (_storeDir != null)
         {
-            file = new File(_storeDir, getFileName(id));
+            file = getFile(_storeDir, id);
-            if (file.exists() && file.getParentFile().equals(_storeDir))
+            if (file != null && file.exists() && file.getParentFile().equals(_storeDir))
             {
-                file.delete();
+                return file.delete();
-                return true;
             }
         }
          
@@ -114,12 +115,55 @@ public class FileSessionDataStore extends AbstractSessionDataStore
      * @see org.eclipse.jetty.server.session.SessionDataStore#getExpired(Set, int)
      */
     @Override
-    public Set<String> doGetExpired(Set<String> candidates, int expiryTimeoutSec)
+    public Set<String> doGetExpired(final Set<String> candidates, final int expiryTimeoutSec)
     {
         //we don't want to open up each file and check, so just leave it up to the SessionStore
         //TODO as the session manager is likely to be a lazy loader, if a session is never requested, its
         //file will stay forever after a restart
-        return candidates;
+        final long now = System.currentTimeMillis();
+        HashSet<String> expired = new HashSet<String>();
+        
+        File[] files = _storeDir.listFiles(new FilenameFilter() 
+        {
+
+            @Override
+            public boolean accept(File dir, String name)
+            {
+                if (dir != _storeDir)
+                    return false;
+                
+                String s = name.substring(0, name.indexOf('_'));
+                long expiry = (s==null?0:Long.parseLong(s));
+                    
+                if (expiry > 0 && expiry < now)
+                    return true;
+                else
+                    return false;
+            }
+        });
+        
+        if (files != null)
+        {
+            for (File f:files)
+            {
+                expired.add(getIdFromFile(f));
+            }
+        }
+        
+        //check candidates that were not found to be expired, perhaps they no
+        //longer exist and they should be expired
+        for (String c:candidates)
+        {
+            if (!expired.contains(c))
+            {
+                //check if the file exists
+                File f = getFile(_storeDir, c);
+                if (f == null || !f.exists())
+                    expired.add(c);
+            }
+        }
+        
+        return expired;
     }
 
 
@@ -136,9 +180,9 @@ public class FileSessionDataStore extends AbstractSessionDataStore
         {
             public void run ()
             {
-                File file = new File(_storeDir,getFileName(id));
+                File file = getFile(_storeDir,id);
 
-                if (!file.exists())
+                if (file == null || !file.exists())
                 {
                     if (LOG.isDebugEnabled())
                         LOG.debug("No file: {}",file);
@@ -187,10 +231,14 @@ public class FileSessionDataStore extends AbstractSessionDataStore
         File file = null;
         if (_storeDir != null)
         {
-            file = new File(_storeDir, getFileName(id));
+            //remove any existing file for the session
-            if (file.exists())
+            file = getFile(_storeDir, id);
+            if (file != null && file.exists())
                 file.delete();
             
+            //make a fresh file using the latest session expiry
+            file = new File(_storeDir, getFileNameWithExpiry(data));
+
             try(FileOutputStream fos = new FileOutputStream(file,false))
             {
                 save(fos, id, data);
@@ -265,6 +313,49 @@ public class FileSessionDataStore extends AbstractSessionDataStore
         return _context.getCanonicalContextPath()+"_"+_context.getVhost()+"_"+id;
     }
     
+    private String getFileNameWithExpiry (SessionData data)
+    {
+        return ""+data.getExpiry()+"_"+getFileName(data.getId());
+    }
+    
+    private String getIdFromFile (File file)
+    {
+        if (file == null)
+            return null;
+        String name = file.getName();
+        
+        return name.substring(name.lastIndexOf('_')+1);
+    }
+
+    
+    /**
+     * Find a File for the session id for the current context.
+     * 
+     * @param storeDir
+     * @param id
+     * @return
+     */
+    private File getFile (final File storeDir, final String id)
+    {
+        File[] files = storeDir.listFiles (new FilenameFilter() {
+
+            /** 
+             * @see java.io.FilenameFilter#accept(java.io.File, java.lang.String)
+             */
+            @Override
+            public boolean accept(File dir, String name)
+            {
+                if (dir != storeDir)
+                    return false;
+                return (name.contains(getFileName(id)));
+            }
+            
+        });
+        
+        if (files == null || files.length < 1)
+            return null;
+        return files[0];
+    }
 
     /**
      * @param is inputstream containing session data

jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java

@@ -408,6 +408,8 @@ public class Session implements SessionManager.SessionIf
             _sessionData.setDirty(true);
             if (secs <= 0)
                 LOG.warn("Session {} is now immortal (maxInactiveInterval={})", _sessionData.getId(), secs);
+            else if (LOG.isDebugEnabled())
+                LOG.debug("Session {} maxInactiveInterval={}", _sessionData.getId(), secs);
         }
     }
 

jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionManager.java

@@ -663,9 +663,10 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
     public void setMaxInactiveInterval(int seconds)
     {
         _dftMaxIdleSecs=seconds;
-        if (_dftMaxIdleSecs < 0)
+        if (_dftMaxIdleSecs <= 0)
             LOG.warn("Sessions created by this manager are immortal (default maxInactiveInterval={})"+_dftMaxIdleSecs);
-
+        else if (LOG.isDebugEnabled())
+            LOG.debug("SessionManager default maxInactiveInterval={}", _dftMaxIdleSecs);
     }
 
     /* ------------------------------------------------------------ */

jetty-server/src/test/java/org/eclipse/jetty/server/session/FileSessionManagerTest.java

@@ -19,6 +19,7 @@
 package org.eclipse.jetty.server.session;
 
 import java.io.File;
+import java.io.FilenameFilter;
 
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
@@ -153,8 +154,21 @@ public class FileSessionManagerTest
         manager.setMaxInactiveInterval(30); // change max inactive interval for *new* sessions
         manager.stop();
         
-        String expectedFilename = "_0.0.0.0_"+session.getId();
+        final String expectedFilename = "_0.0.0.0_"+session.getId();
-        Assert.assertTrue("File should exist!", new File(testDir, expectedFilename).exists());
+    
+        File[] files = testDir.listFiles(new FilenameFilter(){
+
+            @Override
+            public boolean accept(File dir, String name)
+            {
+               return name.contains(expectedFilename);
+            }
+            
+        });
+        Assert.assertNotNull(files);
+        Assert.assertEquals(1, files.length);
+        Assert.assertTrue("File should exist!", files[0].exists());
+        
        
         
         manager.start();

pom.xml

@@ -269,7 +269,7 @@
         <plugin>
           <groupId>org.eclipse.jetty.toolchain</groupId>
           <artifactId>jetty-version-maven-plugin</artifactId>
-          <version>1.0.10</version>
+          <version>1.1</version>
         </plugin>
         <plugin>
           <groupId>org.apache.maven.plugins</groupId>

tests/test-sessions/test-mongodb-sessions/src/test/java/org/eclipse/jetty/nosql/mongodb/SessionExpiryTest.java

@@ -215,6 +215,57 @@ public class SessionExpiryTest extends AbstractSessionExpiryTest
     }
     
     
+    @Test
+    public void testChangeNewSessionTimeout () throws Exception
+    {
+        String contextPath = "";
+        String servletMapping = "/server";
+        int inactivePeriod = 10;
+        int scavengePeriod = 1;
+        int inspectPeriod = 1;
+        int idlePassivatePeriod = 0;
+        AbstractTestServer server1 = createServer(0, inactivePeriod, scavengePeriod,inspectPeriod, idlePassivatePeriod);
+        ImmediateChangeTimeoutServlet servlet = new ImmediateChangeTimeoutServlet();
+        ServletHolder holder = new ServletHolder(servlet);
+        ServletContextHandler context = server1.addContext(contextPath);
+        context.addServlet(holder, servletMapping);
+        TestHttpSessionListener listener = new TestHttpSessionListener();
+        
+        context.getSessionHandler().addEventListener(listener);
+        
+        server1.start();
+        int port1 = server1.getPort();
+
+        try
+        {
+            HttpClient client = new HttpClient();
+            client.start();
+            String url = "http://localhost:" + port1 + contextPath + servletMapping;
+
+            inactivePeriod = 5; //change from the sessionmanager configured default
+            
+            //make a request to set up a session on the server and change its inactive setting straight away
+            ContentResponse response1 = client.GET(url + "?action=init&val="+inactivePeriod);
+            assertEquals(HttpServletResponse.SC_OK,response1.getStatus());
+            String sessionCookie = response1.getHeaders().get("Set-Cookie");
+            assertTrue(sessionCookie != null);
+            // Mangle the cookie, replacing Path with $Path, etc.
+            sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path=");
+            
+            String sessionId = AbstractTestServer.extractSessionId(sessionCookie);     
+            
+            DBCollection sessions = ((MongoSessionIdManager)((MongoTestServer)server1).getServer().getSessionIdManager()).getSessions();
+            verifySessionCreated(listener,sessionId);
+            //verify that the session timeout is the new value and not the default
+            verifySessionTimeout(sessions, sessionId, inactivePeriod);             
+        }
+        finally
+        {
+            server1.stop();
+        }     
+    }
+    
+    
     public void verifySessionTimeout (DBCollection sessions, String id, int sec) throws Exception
     {
         long val;
@@ -300,4 +351,30 @@ public class SessionExpiryTest extends AbstractSessionExpiryTest
         }
     }
     
+    public static class ImmediateChangeTimeoutServlet extends HttpServlet
+    {
+
+        @Override
+        protected void doGet(HttpServletRequest request, HttpServletResponse httpServletResponse) throws ServletException, IOException
+        {
+            String action = request.getParameter("action");
+            if ("init".equals(action))
+            {
+                HttpSession session = request.getSession(true);
+                assertNotNull(session);
+                String tmp = request.getParameter("val");
+                int val = (StringUtil.isBlank(tmp)?0:Integer.valueOf(tmp.trim()));
+                session.setMaxInactiveInterval(val);
+            }
+            else if ("change".equals(action))
+            {
+                String tmp = request.getParameter("val");
+                int val = (StringUtil.isBlank(tmp)?0:Integer.valueOf(tmp.trim()));
+                HttpSession session = request.getSession(false);
+                assertNotNull(session);
+                session.setMaxInactiveInterval(val);
+            }
+        }
+    }
+
 }