From 585dcb7a1ce7fd9e9cbd8f61f8bc6ab9afcb329c Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 16 Apr 2010 17:03:54 -0700 Subject: [PATCH] ReceivePack: Clarify the check reachable option This option was mis-named from day 1. Its not checking that the objects provided by the client are reachable, its actually doing a scan to prove that objects referenced by the client are already reachable through another reference on the server, or were sent as part of the pack from the client. Rename it checkReferencedObjectsAreReachable, since we really are trying to validate that objects referenced by the client's actions are reachable to the client. We also need to ensure we run checkConnectivity() anytime this is enabled, even if the caller didn't turn on fsck for object formats. Otherwise the check would be completely bypassed. Change-Id: Ic352ddb0ca8464d407c6da5c83573093e018af19 Signed-off-by: Shawn O. Pearce --- .../jgit/transport/ReceivePackRefFilterTest.java | 12 ++--- .../org/eclipse/jgit/transport/ReceivePack.java | 62 ++++++++++++++-------- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java index 824eecff..2ec0acaa 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java @@ -186,7 +186,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final ReceivePack rp = super.createReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); return rp; } @@ -229,7 +229,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); final ReceivePack rp = new ReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); rp.receive(new ByteArrayInputStream(inBuf.toByteArray()), outBuf, null); @@ -264,7 +264,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); final ReceivePack rp = new ReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); rp.receive(new ByteArrayInputStream(inBuf.toByteArray()), outBuf, null); @@ -305,7 +305,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); final ReceivePack rp = new ReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); rp.receive(new ByteArrayInputStream(inBuf.toByteArray()), outBuf, null); @@ -347,7 +347,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); final ReceivePack rp = new ReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); rp.receive(new ByteArrayInputStream(inBuf.toByteArray()), outBuf, null); @@ -386,7 +386,7 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); final ReceivePack rp = new ReceivePack(dst); rp.setCheckReceivedObjects(true); - rp.setEnsureProvidedObjectsVisible(true); + rp.setCheckReferencedObjectsAreReachable(true); rp.setRefFilter(new HidePrivateFilter()); rp.receive(new ByteArrayInputStream(inBuf.toByteArray()), outBuf, null); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java index 7c113d65..4e62d742 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java @@ -184,7 +184,7 @@ public class ReceivePack { /** Lock around the received pack file, while updating refs. */ private PackLock packLock; - private boolean ensureObjectsProvidedVisible; + private boolean checkReferencedIsReachable; /** * Create a new pack receive for an open repository. @@ -252,23 +252,36 @@ public class ReceivePack { } /** - * Configure this receive pack instance to ensure that the provided - * objects are visible to the user. + * @return true if this instance will validate all referenced, but not + * supplied by the client, objects are reachable from another + * reference. + */ + public boolean isCheckReferencedObjectsAreReachable() { + return checkReferencedIsReachable; + } + + /** + * Validate all referenced but not supplied objects are reachable. *

- * By default, a receive pack assumes that its user will only provide - * references to objects that it can see. Setting this flag to {@code true} - * will add an additional check that verifies that the objects that were - * provided are reachable by a tree or a commit that the user can see. + * If enabled, this instance will verify that references to objects not + * contained within the received pack are already reachable through at least + * one other reference selected by the {@link #getRefFilter()} and displayed + * as part of {@link #getAdvertisedRefs()}. *

- * This option is useful when the code doesn't trust the client not to - * provide a forged SHA-1 reference to an object in an attempt to access - * parts of the DAG that they aren't allowed to see, via the configured - * {@link RefFilter}. + * This feature is useful when the application doesn't trust the client to + * not provide a forged SHA-1 reference to an object, in an attempt to + * access parts of the DAG that they aren't allowed to see and which have + * been hidden from them via the configured {@link RefFilter}. + *

+ * Enabling this feature may imply at least some, if not all, of the same + * functionality performed by {@link #setCheckReceivedObjects(boolean)}. + * Applications are encouraged to enable both features, if desired. * - * @param b {@code true} to enable the additional check. + * @param b + * {@code true} to enable the additional check. */ - public void setEnsureProvidedObjectsVisible(boolean b) { - this.ensureObjectsProvidedVisible = b; + public void setCheckReferencedObjectsAreReachable(boolean b) { + this.checkReferencedIsReachable = b; } /** @@ -611,7 +624,7 @@ public class ReceivePack { if (needPack()) { try { receivePack(); - if (isCheckReceivedObjects()) + if (needCheckConnectivity()) checkConnectivity(); ip = null; unpackError = null; @@ -761,8 +774,8 @@ public class ReceivePack { ip = IndexPack.create(db, rawIn); ip.setFixThin(true); - ip.setNeedNewObjectIds(ensureObjectsProvidedVisible); - ip.setNeedBaseObjectIds(ensureObjectsProvidedVisible); + ip.setNeedNewObjectIds(checkReferencedIsReachable); + ip.setNeedBaseObjectIds(checkReferencedIsReachable); ip.setObjectChecking(isCheckReceivedObjects()); ip.index(NullProgressMonitor.INSTANCE); @@ -775,11 +788,16 @@ public class ReceivePack { timeoutIn.setTimeout(timeout * 1000); } + private boolean needCheckConnectivity() { + return isCheckReceivedObjects() + || isCheckReferencedObjectsAreReachable(); + } + private void checkConnectivity() throws IOException { ObjectIdSubclassMap baseObjects = null; ObjectIdSubclassMap providedObjects = null; - if (ensureObjectsProvidedVisible) { + if (checkReferencedIsReachable) { baseObjects = ip.getBaseObjectIds(); providedObjects = ip.getNewObjectIds(); } @@ -797,7 +815,7 @@ public class ReceivePack { RevObject o = ow.parseAny(ref.getObjectId()); ow.markUninteresting(o); - if (ensureObjectsProvidedVisible && !baseObjects.isEmpty()) { + if (checkReferencedIsReachable && !baseObjects.isEmpty()) { while (o instanceof RevTag) o = ((RevTag) o).getObject(); if (o instanceof RevCommit) @@ -807,7 +825,7 @@ public class ReceivePack { } } - if (ensureObjectsProvidedVisible) { + if (checkReferencedIsReachable) { for (ObjectId id : baseObjects) { RevObject b = ow.lookupAny(id, Constants.OBJ_BLOB); if (!b.has(RevFlag.UNINTERESTING)) @@ -817,13 +835,13 @@ public class ReceivePack { RevCommit c; while ((c = ow.next()) != null) { - if (ensureObjectsProvidedVisible && !providedObjects.contains(c)) + if (checkReferencedIsReachable && !providedObjects.contains(c)) throw new MissingObjectException(c, Constants.TYPE_COMMIT); } RevObject o; while ((o = ow.nextObject()) != null) { - if (ensureObjectsProvidedVisible) { + if (checkReferencedIsReachable) { if (providedObjects.contains(o)) continue; else -- 2.11.4.GIT