GitHubWebHookCrumbExclusion should be more forgiving if the user leaves off the trailing slash (jenkinsci#152)

James William Dumay · lanwen · commit f784e19fce77 · 2016-11-16T11:37:47.000+02:00
diff --git a/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java b/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java
@@ -9,18 +9,25 @@
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 
+import static org.apache.commons.lang3.StringUtils.isEmpty;
+
 @Extension
 public class GitHubWebHookCrumbExclusion extends CrumbExclusion {
 
     @Override
     public boolean process(HttpServletRequest req, HttpServletResponse resp, FilterChain chain)
             throws IOException, ServletException {
         String pathInfo = req.getPathInfo();
-        if (pathInfo != null && pathInfo.equals(getExclusionPath())) {
-            chain.doFilter(req, resp);
-            return true;
+        if (isEmpty(pathInfo)) {
+            return false;
+        }
+        // Github will not follow redirects https://github.com/isaacs/github/issues/574
+        pathInfo = pathInfo.endsWith("/") ? pathInfo : pathInfo + '/';
+        if (!pathInfo.equals(getExclusionPath())) {
+            return false;
         }
-        return false;
+        chain.doFilter(req, resp);
+        return true;
     }
 
     public String getExclusionPath() {
diff --git a/src/test/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusionTest.java b/src/test/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusionTest.java
@@ -0,0 +1,67 @@
+package com.cloudbees.jenkins;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.servlet.FilterChain;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import static junit.framework.Assert.assertFalse;
+import static junit.framework.TestCase.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class GitHubWebHookCrumbExclusionTest {
+
+    private GitHubWebHookCrumbExclusion exclusion;
+    private HttpServletRequest req;
+    private HttpServletResponse resp;
+    private FilterChain chain;
+
+    @Before
+    public void before() {
+        exclusion = new GitHubWebHookCrumbExclusion();
+        req = mock(HttpServletRequest.class);
+        resp = mock(HttpServletResponse.class);
+        chain = mock(FilterChain.class);
+    }
+
+    @Test
+    public void testFullPath() throws Exception {
+        when(req.getPathInfo()).thenReturn("/github-webhook/");
+        assertTrue(exclusion.process(req, resp, chain));
+        verify(chain, times(1)).doFilter(req, resp);
+    }
+
+    @Test
+    public void testFullPathWithoutSlash() throws Exception {
+        when(req.getPathInfo()).thenReturn("/github-webhook");
+        assertTrue(exclusion.process(req, resp, chain));
+        verify(chain, times(1)).doFilter(req, resp);
+    }
+
+    @Test
+    public void testInvalidPath() throws Exception {
+        when(req.getPathInfo()).thenReturn("/some-other-url/");
+        assertFalse(exclusion.process(req, resp, chain));
+        verify(chain, never()).doFilter(req, resp);
+    }
+
+    @Test
+    public void testNullPath() throws Exception {
+        when(req.getPathInfo()).thenReturn(null);
+        assertFalse(exclusion.process(req, resp, chain));
+        verify(chain, never()).doFilter(req, resp);
+    }
+
+    @Test
+    public void testEmptyPath() throws Exception {
+        when(req.getPathInfo()).thenReturn("");
+        assertFalse(exclusion.process(req, resp, chain));
+        verify(chain, never()).doFilter(req, resp);
+    }
+}
