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>

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 804d744ae23fdfa34b4cfb5a81f7a22c6fa3d5e5..c1811251c629c147378dfdb30431372612a75d26 100644 (file)
--- 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 3f1d21289fdd649923a830029bc67f5df1f16908..40915b7b1f6da41fb9acafb0e6cea09fc2ede8ee 100644 (file)
--- 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 c11ae5a526fd516c32c804906554f9febe8bd709..7a9d523de580269bde3bf6d50d21349820e0ac60 100644 (file)
--- 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 3f96d0919be03399f8b5f6048c8aa867bc0e7a22..c0e24c02cf720813f4c789d74db4a12acd6d2731 100644 (file)
--- 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 825f81e69155d228469f0e15722f10e996638d00..d942d6906850adb8581c2986f21d4345b7cc503a 100644 (file)
--- 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();
+       }
 }