diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index d246bc3a8fa80e91d37c9c2d7b5affef0af0006f..17c739103fe323f7787cfc8d699aaa11ec64dcc4 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -4851,7 +4851,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve */ public boolean isSubjectToMandatoryReadPermissionCheck(String restOfPath) { for (String name : ALWAYS_READABLE_PATHS) { - if (restOfPath.startsWith(name)) { + if (restOfPath.startsWith("/" + name + "/") || restOfPath.equals("/" + name)) { return false; } } @@ -5393,19 +5393,28 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve * *

See also:{@link #getUnprotectedRootActions}. */ - private static final ImmutableSet ALWAYS_READABLE_PATHS = ImmutableSet.of( - "/login", - "/logout", - "/accessDenied", - "/adjuncts/", - "/error", - "/oops", - "/signup", - "/tcpSlaveAgentListener", - "/federatedLoginService/", - "/securityRealm", - "/instance-identity" - ); + private static final Set ALWAYS_READABLE_PATHS = new HashSet<>(ImmutableSet.of( + "login", + "loginError", + "logout", + "accessDenied", + "adjuncts", + "error", + "oops", + "signup", + "tcpSlaveAgentListener", + "federatedLoginService", + "securityRealm", + "instance-identity" + )); + + static { + final String paths = SystemProperties.getString(Jenkins.class.getName() + ".additionalReadablePaths"); + if (paths != null) { + LOGGER.log(INFO, "SECURITY-2047 override: Adding the following paths to ALWAYS_READABLE_PATHS: " + paths); + ALWAYS_READABLE_PATHS.addAll(Arrays.stream(paths.split(",")).map(String::trim).collect(Collectors.toSet())); + } + } /** * {@link Authentication} object that represents the anonymous user. diff --git a/test/src/test/java/jenkins/model/JenkinsSEC2047Test.java b/test/src/test/java/jenkins/model/JenkinsSEC2047Test.java new file mode 100644 index 0000000000000000000000000000000000000000..3d460ddd9b498e026b3f6da3b8d178c009382829 --- /dev/null +++ b/test/src/test/java/jenkins/model/JenkinsSEC2047Test.java @@ -0,0 +1,83 @@ +package jenkins.model; + +import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.RootAction; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.jvnet.hudson.test.MockAuthorizationStrategy; +import org.jvnet.hudson.test.TestExtension; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.fail; + +//TODO merge back to JenkinsTest (or put it somewhere else) +public class JenkinsSEC2047Test { + + @Rule public JenkinsRule j = new JenkinsRule(); + + @Issue("SECURITY-2047") + @Test + public void testLogin123() throws Exception { + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()); + WebClient wc = j.createWebClient(); + + try { + HtmlPage login123 = wc.goTo("login123"); + fail("Page should be protected."); + } catch (FailingHttpStatusCodeException e) { + assertThat(e.getStatusCode(), is(403)); + } + } + + @Issue("SECURITY-2047") + @Test + public void testLogin123WithRead() throws Exception { + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). + grant(Jenkins.READ).everywhere().to("bob")); + WebClient wc = j.createWebClient(); + + wc.login("bob"); + HtmlPage login123 = wc.goTo("login123"); + assertThat(login123.getWebResponse().getStatusCode(), is(200)); + assertThat(login123.getWebResponse().getContentAsString(), containsString("This should be protected")); + } + + @Test + public void testLogin() throws Exception { + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). + grant(Jenkins.READ).everywhere().to("bob")); + WebClient wc = j.createWebClient(); + + HtmlPage login = wc.goTo("login"); + assertThat(login.getWebResponse().getStatusCode(), is(200)); + assertThat(login.getWebResponse().getContentAsString(), containsString("login")); + } + + @TestExtension({"testLogin123", "testLogin123WithRead"}) + public static class ProtectedRootAction implements RootAction { + @Override + public String getIconFileName() { + return "document.png"; + } + + @Override + public String getDisplayName() { + return "I am PROTECTED"; + } + + @Override + public String getUrlName() { + return "login123"; + } + } + +} diff --git a/test/src/test/resources/jenkins/model/JenkinsSEC2047Test/ProtectedRootAction/index.jelly b/test/src/test/resources/jenkins/model/JenkinsSEC2047Test/ProtectedRootAction/index.jelly new file mode 100644 index 0000000000000000000000000000000000000000..9d747775aaed767081b92e357ed038c4db7a9f99 --- /dev/null +++ b/test/src/test/resources/jenkins/model/JenkinsSEC2047Test/ProtectedRootAction/index.jelly @@ -0,0 +1,9 @@ + + + + +

Protected Root Action

+

This should be protected

+ + +