[JENKINS-51158] Improve Root URL validation

- remove the bug for ipv6 - remove the support of query that was not expected - add support for trailing dots - add lots of unit tests (cherry picked from commit c79f3f7)
jenkinsci · Jun 7, 2018 · f031194 · f031194
1 parent 7bd6d1e
commit f031194
Show file tree

Hide file tree

Showing 2 changed files with 130 additions and 15 deletions.
diff --git a/core/src/main/java/jenkins/util/UrlHelper.java b/core/src/main/java/jenkins/util/UrlHelper.java
@@ -28,16 +28,38 @@
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
+/**
+ * Objective is to validate an URL in a lenient way sufficiently strict to avoid too weird URL
+ * but to still allow particular internal URL to be accepted
+ */
 @Restricted(NoExternalUse.class)
 public class UrlHelper {
     /**
-     * Authorize the {@code _} and {@code -} characters in domain
+     * Authorize the {@code _} and {@code -} characters in *domain*
      * <p>
-     * Avoid {@code -} and {@code .} to be first or last
+     * Avoid {@code -} to be first or last, and {@code .} to be first (but can be last)
+     * 
+     * Lenient version of 
+     * - https://tools.ietf.org/html/rfc952 GRAMMATICAL HOST TABLE SPECIFICATION
+     * - https://www.ietf.org/rfc/rfc1034.txt 3.5
+     * - https://www.ietf.org/rfc/rfc1738.txt 3.1, host
+     * - https://tools.ietf.org/html/rfc1123 2.1
+     * 
+     * Deliberately allow:
+     * - short domain name (often there are rules like minimum of 3 characters)
+     * - long domain name (normally limit on whole domain of 255 and for each subdomain/label of 63)
+     * - starting by numbers (disallowed by RFC-952 and RFC-1034, but nowadays it's supported by RFC-1123)
+     * - use of underscore (not explicitly allowed in RFC but could occur in internal network, we do not speak about path here, just domain)
+     * - custom TLD like "intern" that is not standard but could be registered locally in a network
      */
     private static String DOMAIN_REGEX = System.getProperty(
             UrlHelper.class.getName() + ".DOMAIN_REGEX", 
-            "(\\w(\\.?-?\\w+)*)(:\\d{1,5})?"
+            "^" + 
+            "\\w" + // must start with letter / number / underscore
+                "(-*(\\.|\\w))*" +// dashes are allowed but not as last character
+                "\\.*" + // can end with zero (most common), one or multiple dots 
+                "(:\\d{1,5})?" + // and potentially the port specification
+                "$"
     );
 
     public static boolean isValidRootUrl(String url) {
@@ -52,13 +74,22 @@ private CustomUrlValidator() {
 
         @Override 
         protected boolean isValidAuthority(String authority) {
-            // to support ipv6
             boolean superResult = super.isValidAuthority(authority);
+            if(superResult && authority.contains("[")){
+                // to support ipv6
+                return true;
+            }
             if(!superResult && authority == null){
                 return false;
             }
             String authorityASCII = DomainValidator.unicodeToASCII(authority);
             return authorityASCII.matches(DOMAIN_REGEX);
         }
+
+        @Override 
+        protected boolean isValidQuery(String query) {
+            // does not accept query
+            return query == null;
+        }
     }
 }
diff --git a/core/src/test/java/jenkins/util/UrlHelperTest.java b/core/src/test/java/jenkins/util/UrlHelperTest.java
@@ -10,56 +10,140 @@ public class UrlHelperTest {
 
     @Test
     @Issue("JENKINS-31661")
-    public void isValid() {
+    public void regularCases() {
         assertTrue(UrlHelper.isValidRootUrl("http://www.google.com"));
+        // trailing slash is optional
         assertTrue(UrlHelper.isValidRootUrl("http://www.google.com/"));
+        // path is allowed
         assertTrue(UrlHelper.isValidRootUrl("http://www.google.com/jenkins"));
+        // port is allowed to be precised
         assertTrue(UrlHelper.isValidRootUrl("http://www.google.com:8080"));
         assertTrue(UrlHelper.isValidRootUrl("http://www.google.com:8080/jenkins"));
+        // http or https are only valid schemes
         assertTrue(UrlHelper.isValidRootUrl("https://www.google.com:8080/jenkins"));
+        // also with their UPPERCASE equivalent
+        assertTrue(UrlHelper.isValidRootUrl("HTTP://www.google.com:8080/jenkins"));
+        assertTrue(UrlHelper.isValidRootUrl("HTTPS://www.google.com:8080/jenkins"));
+
         assertTrue(UrlHelper.isValidRootUrl("http://localhost:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://localhost:8080/jenkins/"));
         assertTrue(UrlHelper.isValidRootUrl("http://my_server:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://MY_SERVER_IN_PRIVATE_NETWORK:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://jenkins"));
-
+        assertTrue(UrlHelper.isValidRootUrl("http://j"));
+        assertTrue(UrlHelper.isValidRootUrl("http://j.io"));
+
         assertFalse(UrlHelper.isValidRootUrl("http://jenkins::"));
         assertFalse(UrlHelper.isValidRootUrl("http://jenkins::80"));
+        // scheme must be correctly spelled (missing :)
         assertFalse(UrlHelper.isValidRootUrl("http//jenkins"));
-
+
+        // scheme is mandatory
         assertFalse(UrlHelper.isValidRootUrl("com."));
+        // spaces are forbidden
         assertFalse(UrlHelper.isValidRootUrl("http:// "));
-        
+
         // examples not passing with a simple `new URL(url).toURI()` check
         assertFalse(UrlHelper.isValidRootUrl("http://jenkins//context"));
         assertFalse(UrlHelper.isValidRootUrl("http:/jenkins"));
         assertFalse(UrlHelper.isValidRootUrl("http://.com"));
-        assertFalse(UrlHelper.isValidRootUrl("http://com."));
         assertFalse(UrlHelper.isValidRootUrl("http:/:"));
         assertFalse(UrlHelper.isValidRootUrl("http://..."));
         assertFalse(UrlHelper.isValidRootUrl("http://::::@example.com"));
         assertFalse(UrlHelper.isValidRootUrl("ftp://jenkins"));
-        // this url will be used as a root url and so will be concatenated with other part, fragments are not allowed
+    }
+
+    @Test
+    public void fragmentIsForbidden(){
+        // this url will be used as a root url and so will be concatenated with other part, fragment part is not allowed
         assertFalse(UrlHelper.isValidRootUrl("http://jenkins#fragment"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenkins.com#fragment"));
+    }
+
+    @Test
+    public void queryIsForbidden(){
+        // this url will be used as a root url and so will be concatenated with other part, query part is not allowed
+        assertFalse(UrlHelper.isValidRootUrl("http://jenkins?param=test"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenkins.com?param=test"));
+    }
+
+    @Test
+    public void otherCharactersAreForbidden(){
+        // other characters are not allowed
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk@ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk(ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk)ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk[ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk]ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk%ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk$ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk!ins.com"));
+        assertFalse(UrlHelper.isValidRootUrl("http://jenk?ins.com"));
+    }
+
+    @Test
+    public void ipv4Allowed(){
+        assertTrue(UrlHelper.isValidRootUrl("http://172.52.125.12"));
+        assertTrue(UrlHelper.isValidRootUrl("http://172.52.125.12/jenkins"));
+        assertTrue(UrlHelper.isValidRootUrl("http://172.52.125.12:8080"));
+        assertTrue(UrlHelper.isValidRootUrl("http://172.52.125.12:8080/jenkins"));
+    }
+
+    @Test
+    public void ipv6Allowed() {
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]"));
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:0000:0000:3210:FEDC:BA98:7654:3210]"));
+        // 0000 can be reduced to 0
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:0:0:3210:FEDC:BA98:7654:3210]"));
+        // an unique sequence of multiple fragments with 0's could be omitted completely
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC::3210:FEDC:BA98:7654:3210]"));
+        // but only one sequence
+        assertFalse(UrlHelper.isValidRootUrl("http://[2001::85a3::ac1f]"));
+
+        // port and path are still allowed
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:0:0:3210:FEDC:BA98:7654:3210]:8001/jenkins"));
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:0:0:3210:FEDC:BA98:7654:3210]:8001"));
+        assertTrue(UrlHelper.isValidRootUrl("http://[FEDC:0:0:3210:FEDC:BA98:7654:3210]/jenkins"));
+        // dashes are not allowed inside ipv6
+        assertFalse(UrlHelper.isValidRootUrl("http://[FEDC:0:0:32-10:FEDC:BA98:7654:3210]:8001/jenkins"));
+        assertFalse(UrlHelper.isValidRootUrl("http://[FEDC:0:0:3210:-FEDC:BA98:7654:3210]:8001/jenkins"));
     }
 
     @Test
     @Issue("JENKINS-51064")
-    public void isAlsoValid() {
+    public void withCustomDomain() {
         assertTrue(UrlHelper.isValidRootUrl("http://my-server:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://jenkins.internal/"));
         assertTrue(UrlHelper.isValidRootUrl("http://jenkins.otherDomain/"));
         assertTrue(UrlHelper.isValidRootUrl("http://my-server.domain:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://my-ser_ver.do_m-ain:8080/jenkins"));
         assertTrue(UrlHelper.isValidRootUrl("http://my-ser_ver.do_m-ain:8080/jenkins"));
-
+        
         // forbidden to start or end domain with - or .
         assertFalse(UrlHelper.isValidRootUrl("http://-jenkins.com"));
         assertFalse(UrlHelper.isValidRootUrl("http://jenkins.com-"));
         assertFalse(UrlHelper.isValidRootUrl("http://.jenkins.com"));
-        assertFalse(UrlHelper.isValidRootUrl("http://jenkins.com."));
+
+        // allowed to have multiple dots in chain
+        assertTrue(UrlHelper.isValidRootUrl("http://jen..kins.com"));
+    }
 
-        // forbidden to have multiple dots in chain
-        assertFalse(UrlHelper.isValidRootUrl("http://jen..kins.com"));
+    @Test
+    public void multipleConsecutiveDashesAreAllowed() {
+        assertTrue(UrlHelper.isValidRootUrl("http://jenk--ins.internal/"));
+        assertTrue(UrlHelper.isValidRootUrl("http://www.go-----ogle.com/"));
+        // even with subdomain being just a dash
+        assertTrue(UrlHelper.isValidRootUrl("http://www.go.-.--.--ogle.com/"));
+    }
+
+    @Test
+    @Issue("JENKINS-51158")
+    public void trailingDotsAreAccepted() {
+        assertTrue(UrlHelper.isValidRootUrl("http://jenkins.internal./"));
+        assertTrue(UrlHelper.isValidRootUrl("http://jenkins.internal......./"));
+        assertTrue(UrlHelper.isValidRootUrl("http://my-server.domain.:8080/jenkins"));
+        assertTrue(UrlHelper.isValidRootUrl("http://my-server.domain......:8080/jenkins"));
+        assertTrue(UrlHelper.isValidRootUrl("http://jenkins.com."));
+        assertTrue(UrlHelper.isValidRootUrl("http://jenkins.com......"));
     }
 }