Merge "Fix error occurring when SecurityManager is enabled"

7 files changed, 364 insertions, 51 deletions

diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/SeparateClassloaderTestRunner.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/SeparateClassloaderTestRunner.java
new file mode 100644
index 0000000000..926a19599e
--- /dev/null
+++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/SeparateClassloaderTestRunner.java
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2019 Nail Samatov <sanail@yandex.ru>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.eclipse.jgit.junit;
+
+import static java.lang.ClassLoader.getSystemClassLoader;
+
+import java.net.URL;
+import java.net.URLClassLoader;
+
+import org.junit.runners.BlockJUnit4ClassRunner;
+import org.junit.runners.model.InitializationError;
+
+/**
+ * This class is used when it's required to load jgit classes in separate
+ * classloader for each test class. It can be needed to isolate static field
+ * initialization between separate tests.
+ */
+public class SeparateClassloaderTestRunner extends BlockJUnit4ClassRunner {
+
+	/**
+	 * Creates a SeparateClassloaderTestRunner to run {@code klass}.
+	 *
+	 * @param klass
+	 *            test class to run.
+	 * @throws InitializationError
+	 *             if the test class is malformed or can't be found.
+	 */
+	public SeparateClassloaderTestRunner(Class<?> klass)
+			throws InitializationError {
+		super(loadNewClass(klass));
+	}
+
+	private static Class<?> loadNewClass(Class<?> klass)
+			throws InitializationError {
+		try {
+			URL[] urls = ((URLClassLoader) getSystemClassLoader()).getURLs();
+			ClassLoader testClassLoader = new URLClassLoader(urls) {
+
+				@Override
+				public Class<?> loadClass(String name)
+						throws ClassNotFoundException {
+					if (name.startsWith("org.eclipse.jgit.")) {
+						return super.findClass(name);
+					}
+
+					return super.loadClass(name);
+				}
+			};
+			return Class.forName(klass.getName(), true, testClassLoader);
+		} catch (ClassNotFoundException e) {
+			throw new InitializationError(e);
+		}
+	}
+}
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerTest.java
new file mode 100644
index 0000000000..62d3530338
--- /dev/null
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerTest.java
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2019 Nail Samatov <sanail@yandex.ru>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.eclipse.jgit.api;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.FilePermission;
+import java.io.IOException;
+import java.lang.reflect.ReflectPermission;
+import java.nio.file.Files;
+import java.security.Permission;
+import java.security.SecurityPermission;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.PropertyPermission;
+import java.util.logging.LoggingPermission;
+
+import javax.security.auth.AuthPermission;
+
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.junit.JGitTestUtil;
+import org.eclipse.jgit.junit.MockSystemReader;
+import org.eclipse.jgit.junit.SeparateClassloaderTestRunner;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.util.FileUtils;
+import org.eclipse.jgit.util.SystemReader;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/**
+ * <p>
+ * Tests if jgit works if SecurityManager is enabled.
+ * </p>
+ *
+ * <p>
+ * Note: JGit's classes shouldn't be used before SecurityManager is configured.
+ * If you use some JGit's class before SecurityManager is replaced then part of
+ * the code can be invoked outside of our custom SecurityManager and this test
+ * becomes useless.
+ * </p>
+ *
+ * <p>
+ * For example the class {@link org.eclipse.jgit.util.FS} is used widely in jgit
+ * sources. It contains DETECTED static field. At the first usage of the class
+ * FS the field DETECTED is initialized and during initialization many system
+ * operations that SecurityManager can forbid are invoked.
+ * </p>
+ *
+ * <p>
+ * For this reason this test doesn't extend LocalDiskRepositoryTestCase (it uses
+ * JGit's classes in setUp() method) and other JGit's utility classes. It's done
+ * to affect SecurityManager as less as possible.
+ * </p>
+ *
+ * <p>
+ * We use SeparateClassloaderTestRunner to isolate FS.DETECTED field
+ * initialization between different tests run.
+ * </p>
+ */
+@RunWith(SeparateClassloaderTestRunner.class)
+public class SecurityManagerTest {
+	private File root;
+
+	private SecurityManager originalSecurityManager;
+
+	private List<Permission> permissions = new ArrayList<>();
+
+	@Before
+	public void setUp() throws Exception {
+		// Create working directory
+		SystemReader.setInstance(new MockSystemReader());
+		root = Files.createTempDirectory("jgit-security").toFile();
+
+		// Add system permissions
+		permissions.add(new RuntimePermission("*"));
+		permissions.add(new SecurityPermission("*"));
+		permissions.add(new AuthPermission("*"));
+		permissions.add(new ReflectPermission("*"));
+		permissions.add(new PropertyPermission("*", "read,write"));
+		permissions.add(new LoggingPermission("control", null));
+
+		permissions.add(new FilePermission(
+				System.getProperty("java.home") + "/-", "read"));
+
+		String tempDir = System.getProperty("java.io.tmpdir");
+		permissions.add(new FilePermission(tempDir, "read,write,delete"));
+		permissions
+				.add(new FilePermission(tempDir + "/-", "read,write,delete"));
+
+		// Add permissions to dependent jar files.
+		String classPath = System.getProperty("java.class.path");
+		if (classPath != null) {
+			for (String path : classPath.split(File.pathSeparator)) {
+				permissions.add(new FilePermission(path, "read"));
+			}
+		}
+		// Add permissions to jgit class files.
+		String jgitSourcesRoot = new File(System.getProperty("user.dir"))
+				.getParent();
+		permissions.add(new FilePermission(jgitSourcesRoot + "/-", "read"));
+
+		// Add permissions to working dir for jgit. Our git repositories will be
+		// initialized and cloned here.
+		permissions.add(new FilePermission(root.getPath() + "/-",
+				"read,write,delete,execute"));
+
+		// Replace Security Manager
+		originalSecurityManager = System.getSecurityManager();
+		System.setSecurityManager(new SecurityManager() {
+
+			@Override
+			public void checkPermission(Permission requested) {
+				for (Permission permission : permissions) {
+					if (permission.implies(requested)) {
+						return;
+					}
+				}
+
+				super.checkPermission(requested);
+			}
+		});
+	}
+
+	@After
+	public void tearDown() throws Exception {
+		System.setSecurityManager(originalSecurityManager);
+
+		// Note: don't use this method before security manager is replaced in
+		// setUp() method. The method uses FS.DETECTED internally and can affect
+		// the test.
+		FileUtils.delete(root, FileUtils.RECURSIVE | FileUtils.RETRY);
+	}
+
+	@Test
+	public void testInitAndClone() throws IOException, GitAPIException {
+		File remote = new File(root, "remote");
+		File local = new File(root, "local");
+
+		try (Git git = Git.init().setDirectory(remote).call()) {
+			JGitTestUtil.write(new File(remote, "hello.txt"), "Hello world!");
+			git.add().addFilepattern(".").call();
+			git.commit().setMessage("Initial commit").call();
+		}
+
+		try (Git git = Git.cloneRepository().setURI(remote.toURI().toString())
+				.setDirectory(local).call()) {
+			assertTrue(new File(local, ".git").exists());
+
+			JGitTestUtil.write(new File(local, "hi.txt"), "Hi!");
+			git.add().addFilepattern(".").call();
+			RevCommit commit1 = git.commit().setMessage("Commit on local repo")
+					.call();
+			assertEquals("Commit on local repo", commit1.getFullMessage());
+			assertNotNull(TreeWalk.forPath(git.getRepository(), "hello.txt",
+					commit1.getTree()));
+		}
+
+	}
+
+}
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 2a2ee8ddc7..5a4d9bb990 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -587,6 +587,8 @@ readFileStoreAttributesFailed=Reading FileStore attributes from user config fail
 readerIsRequired=Reader is required
 readingObjectsFromLocalRepositoryFailed=reading objects from local repository failed: {0}
 readLastModifiedFailed=Reading lastModified of {0} failed
+readPipeIsNotAllowed=FS.readPipe() isn't allowed for command ''{0}''. Working directory: ''{1}''.
+readPipeIsNotAllowedRequiredPermission=FS.readPipe() isn't allowed for command ''{0}''. Working directory: ''{1}''. Required permission: {2}.
 readTimedOut=Read timed out after {0} ms
 receivePackObjectTooLarge1=Object too large, rejecting the pack. Max object size limit is {0} bytes.
 receivePackObjectTooLarge2=Object too large ({0} bytes), rejecting the pack. Max object size limit is {1} bytes.
@@ -662,6 +664,7 @@ signingNotSupportedOnTag=Signing isn't supported on tag operations yet.
 similarityScoreMustBeWithinBounds=Similarity score must be between 0 and 100.
 sizeExceeds2GB=Path {0} size {1} exceeds 2 GiB limit.
 skipMustBeNonNegative=skip must be >= 0
+skipNotAccessiblePath=The path ''{0}'' isn't accessible. Skip it.
 smartHTTPPushDisabled=smart HTTP push disabled
 sourceDestinationMustMatch=Source/Destination must match.
 sourceIsNotAWildcard=Source is not a wildcard.
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 649f77724e..b80b7498b1 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -648,6 +648,8 @@ public class JGitText extends TranslationBundle {
 	/***/ public String readerIsRequired;
 	/***/ public String readingObjectsFromLocalRepositoryFailed;
 	/***/ public String readLastModifiedFailed;
+	/***/ public String readPipeIsNotAllowed;
+	/***/ public String readPipeIsNotAllowedRequiredPermission;
 	/***/ public String readTimedOut;
 	/***/ public String receivePackObjectTooLarge1;
 	/***/ public String receivePackObjectTooLarge2;
@@ -723,6 +725,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String similarityScoreMustBeWithinBounds;
 	/***/ public String sizeExceeds2GB;
 	/***/ public String skipMustBeNonNegative;
+	/***/ public String skipNotAccessiblePath;
 	/***/ public String smartHTTPPushDisabled;
 	/***/ public String sourceDestinationMustMatch;
 	/***/ public String sourceIsNotAWildcard;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
index 90305013f5..29519298c4 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -64,6 +64,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
+import java.security.AccessControlException;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.text.MessageFormat;
@@ -122,6 +123,8 @@ public abstract class FS {
 	 */
 	protected static final Entry[] NO_ENTRIES = {};
 
+	private volatile Boolean supportSymlinks;
+
 	/**
 	 * This class creates FS instances. It will be overridden by a Java7 variant
 	 * if such can be detected in {@link #detect(Boolean)}.
@@ -276,15 +279,19 @@ public abstract class FS {
 		 * @return FileStoreAttributes for the given path.
 		 */
 		public static FileStoreAttributes get(Path path) {
-			path = path.toAbsolutePath();
-			Path dir = Files.isDirectory(path) ? path : path.getParent();
-			FileStoreAttributes cached = attrCacheByPath.get(dir);
-			if (cached != null) {
-				return cached;
+			try {
+				path = path.toAbsolutePath();
+				Path dir = Files.isDirectory(path) ? path : path.getParent();
+				FileStoreAttributes cached = attrCacheByPath.get(dir);
+				if (cached != null) {
+					return cached;
+				}
+				FileStoreAttributes attrs = getFileStoreAttributes(dir);
+				attrCacheByPath.put(dir, attrs);
+				return attrs;
+			} catch (SecurityException e) {
+				return FALLBACK_FILESTORE_ATTRIBUTES;
 			}
-			FileStoreAttributes attrs = getFileStoreAttributes(dir);
-			attrCacheByPath.put(dir, attrs);
-			return attrs;
 		}
 
 		private static FileStoreAttributes getFileStoreAttributes(Path dir) {
@@ -813,7 +820,32 @@ public abstract class FS {
 	 * @since 3.0
 	 */
 	public boolean supportsSymlinks() {
-		return false;
+		if (supportSymlinks == null) {
+			detectSymlinkSupport();
+		}
+		return Boolean.TRUE.equals(supportSymlinks);
+	}
+
+	private void detectSymlinkSupport() {
+		File tempFile = null;
+		try {
+			tempFile = File.createTempFile("tempsymlinktarget", ""); //$NON-NLS-1$ //$NON-NLS-2$
+			File linkName = new File(tempFile.getParentFile(), "tempsymlink"); //$NON-NLS-1$
+			createSymLink(linkName, tempFile.getPath());
+			supportSymlinks = Boolean.TRUE;
+			linkName.delete();
+		} catch (IOException | UnsupportedOperationException | SecurityException
+				| InternalError e) {
+			supportSymlinks = Boolean.FALSE;
+		} finally {
+			if (tempFile != null) {
+				try {
+					FileUtils.delete(tempFile);
+				} catch (IOException e) {
+					throw new RuntimeException(e); // panic
+				}
+			}
+		}
 	}
 
 	/**
@@ -1067,9 +1099,16 @@ public abstract class FS {
 
 		for (String p : path.split(File.pathSeparator)) {
 			for (String command : lookFor) {
-				final File e = new File(p, command);
-				if (e.isFile())
-					return e.getAbsoluteFile();
+				final File file = new File(p, command);
+				try {
+					if (file.isFile()) {
+						return file.getAbsoluteFile();
+					}
+				} catch (SecurityException e) {
+					LOG.warn(MessageFormat.format(
+							JGitText.get().skipNotAccessiblePath,
+							file.getPath()));
+				}
 			}
 		}
 		return null;
@@ -1172,6 +1211,13 @@ public abstract class FS {
 			}
 		} catch (IOException e) {
 			LOG.error("Caught exception in FS.readPipe()", e); //$NON-NLS-1$
+		} catch (AccessControlException e) {
+			LOG.warn(MessageFormat.format(
+					JGitText.get().readPipeIsNotAllowedRequiredPermission,
+					command, dir, e.getPermission()));
+		} catch (SecurityException e) {
+			LOG.warn(MessageFormat.format(JGitText.get().readPipeIsNotAllowed,
+					command, dir));
 		}
 		if (debug) {
 			LOG.debug("readpipe returns null"); //$NON-NLS-1$
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
index a485389a9a..6a1eef2d66 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
@@ -287,12 +287,6 @@ public class FS_POSIX extends FS {
 
 	/** {@inheritDoc} */
 	@Override
-	public boolean supportsSymlinks() {
-		return true;
-	}
-
-	/** {@inheritDoc} */
-	@Override
 	public void setHidden(File path, boolean hidden) throws IOException {
 		// no action on POSIX
 	}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java
index 7fe80bb21a..1e64a38bb1 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java
@@ -74,8 +74,6 @@ import org.slf4j.LoggerFactory;
 public class FS_Win32 extends FS {
 	private final static Logger LOG = LoggerFactory.getLogger(FS_Win32.class);
 
-	private volatile Boolean supportSymlinks;
-
 	/**
 	 * Constructor
 	 */
@@ -239,37 +237,6 @@ public class FS_Win32 extends FS {
 
 	/** {@inheritDoc} */
 	@Override
-	public boolean supportsSymlinks() {
-		if (supportSymlinks == null) {
-			detectSymlinkSupport();
-		}
-		return Boolean.TRUE.equals(supportSymlinks);
-	}
-
-	private void detectSymlinkSupport() {
-		File tempFile = null;
-		try {
-			tempFile = File.createTempFile("tempsymlinktarget", ""); //$NON-NLS-1$ //$NON-NLS-2$
-			File linkName = new File(tempFile.getParentFile(), "tempsymlink"); //$NON-NLS-1$
-			createSymLink(linkName, tempFile.getPath());
-			supportSymlinks = Boolean.TRUE;
-			linkName.delete();
-		} catch (IOException | UnsupportedOperationException
-				| InternalError e) {
-			supportSymlinks = Boolean.FALSE;
-		} finally {
-			if (tempFile != null) {
-				try {
-					FileUtils.delete(tempFile);
-				} catch (IOException e) {
-					throw new RuntimeException(e); // panic
-				}
-			}
-		}
-	}
-
-	/** {@inheritDoc} */
-	@Override
 	public Attributes getAttributes(File path) {
 		return FileUtils.getFileAttributesBasic(this, path);
 	}