From b522499140bafe99de097fa878c811b001e0694e Mon Sep 17 00:00:00 2001 From: James Dumay Date: Sat, 5 Nov 2016 17:18:57 +1100 Subject: [PATCH] GitHubWebHookCrumbExclusion should be more forgiving if the user leaves off the trailing slash --- .../jenkins/GitHubWebHookCrumbExclusion.java | 15 +++-- .../GitHubWebHookCrumbExclusionTest.java | 67 +++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 src/test/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusionTest.java diff --git a/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java b/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java index b102a5ed4..e342e1261 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java @@ -9,6 +9,8 @@ import javax.servlet.http.HttpServletResponse; import java.io.IOException; +import static org.apache.commons.lang3.StringUtils.isEmpty; + @Extension public class GitHubWebHookCrumbExclusion extends CrumbExclusion { @@ -16,11 +18,16 @@ public class GitHubWebHookCrumbExclusion extends CrumbExclusion { 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 new file mode 100644 index 000000000..fcf8317e1 --- /dev/null +++ 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); + } +}