Config: Remove the include functionality

The Config class must be safe to run against untrusted input files.
Reading arbitrary local system paths using include.path is risky for
servers, including Gerrit Code Review.

This was fixed on master [1] by making "readIncludedConfig" a noop
by default. This allows only FileBasedConfig, which originated from
local disk, to read local system paths.

However, the "readIncludedConfig" method was only introduced in [2]
which was needed by [3], both of which are only on the master branch.
On the stable branch only Config supports includes. Therefore this
commit simply disables the include functionality.

[1] https://git.eclipse.org/r/#/c/113371/
[2] https://git.eclipse.org/r/#/c/111847/
[3] https://git.eclipse.org/r/#/c/111848/

Bug: 528781
Change-Id: I9a3be3f1d07c4b6772bff535a2556e699a61381c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java
index e9505f67d0145cf68c732e1cc0420353e2afbb6c..a12831a1493cd5c120769a461a1ebd2a0ff4bc5d 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java
@@ -80,6 +80,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.SystemReader;
 import org.junit.After;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -766,6 +767,7 @@ public class ConfigTest {
        }
 
        @Test
+       @Ignore
        public void testIncludeInvalidName() throws ConfigInvalidException {
                expectedEx.expect(ConfigInvalidException.class);
                expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile);
@@ -773,6 +775,7 @@ public class ConfigTest {
        }
 
        @Test
+       @Ignore
        public void testIncludeNoValue() throws ConfigInvalidException {
                expectedEx.expect(ConfigInvalidException.class);
                expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile);
@@ -780,6 +783,7 @@ public class ConfigTest {
        }
 
        @Test
+       @Ignore
        public void testIncludeEmptyValue() throws ConfigInvalidException {
                expectedEx.expect(ConfigInvalidException.class);
                expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile);
@@ -816,6 +820,7 @@ public class ConfigTest {
        }
 
        @Test
+       @Ignore
        public void testIncludeTooManyRecursions() throws IOException {
                File config = tmp.newFile("config");
                String include = "[include]\npath=" + config.toPath() + "\n";
@@ -832,27 +837,14 @@ public class ConfigTest {
        }
 
        @Test
-       public void testInclude() throws IOException, ConfigInvalidException {
+       public void testIncludeIsNoop() throws IOException, ConfigInvalidException {
                File config = tmp.newFile("config");
-               File more = tmp.newFile("config.more");
-               File other = tmp.newFile("config.other");
 
                String fooBar = "[foo]\nbar=true\n";
-               String includeMore = "[include]\npath=" + more.toPath() + "\n";
-               String includeOther = "path=" + other.toPath() + "\n";
-               String fooPlus = fooBar + includeMore + includeOther;
-               Files.write(config.toPath(), fooPlus.getBytes());
-
-               String fooMore = "[foo]\nmore=bar\n";
-               Files.write(more.toPath(), fooMore.getBytes());
-
-               String otherMore = "[other]\nmore=bar\n";
-               Files.write(other.toPath(), otherMore.getBytes());
+               Files.write(config.toPath(), fooBar.getBytes());
 
                Config parsed = parse("[include]\npath=" + config.toPath() + "\n");
-               assertTrue(parsed.getBoolean("foo", "bar", false));
-               assertEquals("bar", parsed.getString("foo", null, "more"));
-               assertEquals("bar", parsed.getString("other", null, "more"));
+               assertFalse(parsed.getBoolean("foo", "bar", false));
        }
 
        private static void assertReadLong(long exp) throws ConfigInvalidException {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
index 6281bcfb3d3639b67d0ac3d89b31a70866010fd8..b0f5c2cf4ea8bdbf534049140814ef594a4ab0f7 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
@@ -51,9 +51,6 @@
 
 package org.eclipse.jgit.lib;
 
-import java.io.File;
-import java.io.FileNotFoundException;
-import java.io.IOException;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -70,8 +67,6 @@ import org.eclipse.jgit.events.ListenerHandle;
 import org.eclipse.jgit.events.ListenerList;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.transport.RefSpec;
-import org.eclipse.jgit.util.IO;
-import org.eclipse.jgit.util.RawParseUtils;
 
 /**
  * Git style {@code .config}, {@code .gitconfig}, {@code .gitmodules} file.
@@ -1073,10 +1068,6 @@ public class Config {
                                        e.value = MAGIC_EMPTY_VALUE;
                                } else
                                        e.value = readValue(in, false, -1);
-
-                               if (e.section.equals("include")) { //$NON-NLS-1$
-                                       addIncludedConfig(newEntries, e, depth);
-                               }
                        } else
                                throw new ConfigInvalidException(JGitText.get().invalidLineInConfigFile);
                }
@@ -1084,36 +1075,6 @@ public class Config {
                return newEntries;
        }
 
-       private void addIncludedConfig(final List<ConfigLine> newEntries,
-                       ConfigLine line, int depth) throws ConfigInvalidException {
-               if (!line.name.equals("path") || //$NON-NLS-1$
-                               line.value == null || line.value.equals(MAGIC_EMPTY_VALUE)) {
-                       throw new ConfigInvalidException(
-                                       JGitText.get().invalidLineInConfigFile);
-               }
-               File path = new File(line.value);
-               try {
-                       byte[] bytes = IO.readFully(path);
-                       String decoded;
-                       if (isUtf8(bytes)) {
-                               decoded = RawParseUtils.decode(RawParseUtils.UTF8_CHARSET,
-                                               bytes, 3, bytes.length);
-                       } else {
-                               decoded = RawParseUtils.decode(bytes);
-                       }
-                       newEntries.addAll(fromTextRecurse(decoded, depth + 1));
-               } catch (FileNotFoundException fnfe) {
-                       if (path.exists()) {
-                               throw new ConfigInvalidException(MessageFormat
-                                               .format(JGitText.get().cannotReadFile, path), fnfe);
-                       }
-               } catch (IOException ioe) {
-                       throw new ConfigInvalidException(
-                                       MessageFormat.format(JGitText.get().cannotReadFile, path),
-                                       ioe);
-               }
-       }
-
        private ConfigSnapshot newState() {
                return new ConfigSnapshot(Collections.<ConfigLine> emptyList(),
                                getBaseState());