[SECURITY-2047]

jenkinsci · Jan 11, 2021 · fe9091f · fe9091f
1 parent a890d68
commit fe9091f
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 14 deletions.
diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -4851,7 +4851,7 @@ public Object getTarget() {
      */
     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 boolean shouldShowStackTrace() {
      *
      * <p>See also:{@link #getUnprotectedRootActions}.
      */
-    private static final ImmutableSet<String> ALWAYS_READABLE_PATHS = ImmutableSet.of(
-        "/login",
-        "/logout",
-        "/accessDenied",
-        "/adjuncts/",
-        "/error",
-        "/oops",
-        "/signup",
-        "/tcpSlaveAgentListener",
-        "/federatedLoginService/",
-        "/securityRealm",
-        "/instance-identity"
-    );
+    private static final Set<String> 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
@@ -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
@@ -0,0 +1,9 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
+  <l:layout title="Protected Action">
+    <l:main-panel>
+      <h1>Protected Root Action</h1>
+      <p>This should be protected</p>
+    </l:main-panel>
+  </l:layout>
+</j:jelly>