From 39b27f7c7b99499702db6e86ccfacf448a3231bf Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Mon, 8 Oct 2018 15:42:47 -0700 Subject: [PATCH] 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 --- .../internal/storage/dfs/DfsFsckTest.java | 55 +++++++++++++++++++ .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../jgit/internal/storage/dfs/DfsFsck.java | 35 ++++++++++++ .../org/eclipse/jgit/lib/ObjectChecker.java | 18 +++++- 5 files changed, 109 insertions(+), 1 deletion(-) 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 804d744ae..c1811251c 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 3f1d21289..40915b7b1 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 c11ae5a52..7a9d523de 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 3f96d0919..c0e24c02c 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 825f81e69..d942d6906 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. *

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