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>

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);
+	}
+
 }

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

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;

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.
 	 */

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();
+	}
 }