DfsFsck: Check that .gitmodules in the repository have valid contents

Previous commits block the addition to the repo of dangerous .gitmodules
files, but some could have been committed before those safeguards where
in place.

Add a check in DfsFsck to validate the .gitmodules files in the repo.
Use the same validation than the ReceivePack, translating the
results to FsckErrors.

Note that *all* .gitmodules files in the storage will be checked, not
only the latest version.

Change-Id: I040cf1f31a779419aad0292ba5e6e76eb7f32b66
Signed-off-by: Ivan Frade <ifrade@google.com>

5 files changed, 109 insertions, 1 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsFsckTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsFsckTest.java
index 804d744ae2..c1811251c6 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsFsckTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsFsckTest.java
@@ -266,4 +266,59 @@ public class DfsFsckTest {
 				"refs/heads/master");
 	}
 
+	private ObjectId insertGitModules(String contents) throws IOException {
+		ObjectId blobId = ins.insert(Constants.OBJ_BLOB,
+				Constants.encode(contents));
+
+		byte[] blobIdBytes = new byte[OBJECT_ID_LENGTH];
+		blobId.copyRawTo(blobIdBytes, 0);
+		byte[] data = concat(encodeASCII("100644 .gitmodules\0"), blobIdBytes);
+		ins.insert(Constants.OBJ_TREE, data);
+		ins.flush();
+
+		return blobId;
+	}
+
+	@Test
+	public void testInvalidGitModules() throws Exception {
+		String fakeGitmodules = new StringBuilder()
+				.append("[submodule \"test\"]\n")
+				.append("    path = xlib\n")
+				.append("    url = https://example.com/repo/xlib.git\n\n")
+				.append("[submodule \"test2\"]\n")
+				.append("    path = zlib\n")
+				.append("    url = -upayload.sh\n")
+				.toString();
+
+		ObjectId blobId = insertGitModules(fakeGitmodules);
+
+		DfsFsck fsck = new DfsFsck(repo);
+		FsckError errors = fsck.check(null);
+		assertEquals(errors.getCorruptObjects().size(), 1);
+
+		CorruptObject error = errors.getCorruptObjects().iterator().next();
+		assertEquals(error.getId(), blobId);
+		assertEquals(error.getType(), Constants.OBJ_BLOB);
+		assertEquals(error.getErrorType(), ErrorType.GITMODULES_URL);
+	}
+
+
+	@Test
+	public void testValidGitModules() throws Exception {
+		String fakeGitmodules = new StringBuilder()
+				.append("[submodule \"test\"]\n")
+				.append("    path = xlib\n")
+				.append("    url = https://example.com/repo/xlib.git\n\n")
+				.append("[submodule \"test2\"]\n")
+				.append("    path = zlib\n")
+				.append("    url = ok/path\n")
+				.toString();
+
+		insertGitModules(fakeGitmodules);
+
+		DfsFsck fsck = new DfsFsck(repo);
+		FsckError errors = fsck.check(null);
+		assertEquals(errors.getCorruptObjects().size(), 0);
+	}
+
 }
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 3f1d21289f..40915b7b1f 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -769,6 +769,7 @@ uriNotFound={0} not found
 uriNotFoundWithMessage={0} not found: {1}
 URINotSupported=URI not supported: {0}
 userConfigFileInvalid=User config file {0} invalid {1}
+validatingGitModules=Validating .gitmodules files
 walkFailure=Walk failure.
 wantNotValid=want {0} not valid
 weeksAgo={0} weeks ago
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 c11ae5a526..7a9d523de5 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -830,6 +830,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String uriNotFoundWithMessage;
 	/***/ public String URINotSupported;
 	/***/ public String userConfigFileInvalid;
+	/***/ public String validatingGitModules;
 	/***/ public String walkFailure;
 	/***/ public String wantNotValid;
 	/***/ public String weeksAgo;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsFsck.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsFsck.java
index 3f96d0919b..c0e24c02cf 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsFsck.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsFsck.java
@@ -43,6 +43,7 @@
 
 package org.eclipse.jgit.internal.storage.dfs;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
 
@@ -54,12 +55,18 @@ import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.fsck.FsckError;
 import org.eclipse.jgit.internal.fsck.FsckError.CorruptIndex;
+import org.eclipse.jgit.internal.fsck.FsckError.CorruptObject;
 import org.eclipse.jgit.internal.fsck.FsckPackParser;
 import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
+import org.eclipse.jgit.internal.submodule.SubmoduleValidator;
+import org.eclipse.jgit.internal.submodule.SubmoduleValidator.SubmoduleValidationException;
+import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.GitmoduleEntry;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectChecker;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.ObjectWalk;
@@ -102,6 +109,7 @@ public class DfsFsck {
 
 		FsckError errors = new FsckError();
 		if (!connectivityOnly) {
+			objChecker.reset();
 			checkPacks(pm, errors);
 		}
 		checkConnectivity(pm, errors);
@@ -128,6 +136,8 @@ public class DfsFsck {
 				}
 			}
 		}
+
+		checkGitModules(pm, errors);
 	}
 
 	private void verifyPack(ProgressMonitor pm, FsckError errors, DfsReader ctx,
@@ -142,6 +152,28 @@ public class DfsFsck {
 		fpp.verifyIndex(pack.getPackIndex(ctx));
 	}
 
+	private void checkGitModules(ProgressMonitor pm, FsckError errors)
+			throws IOException {
+		pm.beginTask(JGitText.get().validatingGitModules,
+				objChecker.getGitsubmodules().size());
+		for (GitmoduleEntry entry : objChecker.getGitsubmodules()) {
+			AnyObjectId blobId = entry.getBlobId();
+			ObjectLoader blob = objdb.open(blobId, Constants.OBJ_BLOB);
+
+			try {
+				SubmoduleValidator.assertValidGitModulesFile(
+						new String(blob.getBytes(), UTF_8));
+			} catch (SubmoduleValidationException e) {
+				CorruptObject co = new FsckError.CorruptObject(
+						blobId.toObjectId(), Constants.OBJ_BLOB,
+						e.getFsckMessageId());
+				errors.getCorruptObjects().add(co);
+			}
+			pm.update(1);
+		}
+		pm.endTask();
+	}
+
 	private void checkConnectivity(ProgressMonitor pm, FsckError errors)
 			throws IOException {
 		pm.beginTask(JGitText.get().countingObjects, ProgressMonitor.UNKNOWN);
@@ -179,6 +211,9 @@ public class DfsFsck {
 	 * Use a customized object checker instead of the default one. Caller can
 	 * specify a skip list to ignore some errors.
 	 *
+	 * It will be reset at the start of each {{@link #check(ProgressMonitor)}
+	 * call.
+	 *
 	 * @param objChecker
 	 *            A customized object checker.
 	 */
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectChecker.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectChecker.java
index 825f81e691..d942d69068 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectChecker.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectChecker.java
@@ -109,7 +109,8 @@ import org.eclipse.jgit.util.StringUtils;
  * the caller can provide both of these validations on its own.
  * <p>
  * Instances of this class are not thread safe, but they may be reused to
- * perform multiple object validations.
+ * perform multiple object validations, calling {@link #reset()} between them to
+ * clear the internal state (e.g. {@link #getGitsubmodules()})
  */
 public class ObjectChecker {
 	/** Header "tree " */
@@ -1258,4 +1259,19 @@ public class ObjectChecker {
 	public List<GitmoduleEntry> getGitsubmodules() {
 		return gitsubmodules;
 	}
+
+	/**
+	 * Reset the invocation-specific state from this instance. Specifically this
+	 * clears the list of .gitmodules files encountered (see
+	 * {@link #getGitsubmodules()})
+	 *
+	 * Configurations like errors to filter, skip lists or the specified O.S.
+	 * (set via {@link #setSafeForMacOS(boolean)} or
+	 * {@link #setSafeForWindows(boolean)}) are NOT cleared.
+	 *
+	 * @since 5.2
+	 */
+	public void reset() {
+		gitsubmodules.clear();
+	}
 }