From 61f0bd54d058789d73a8db0dfb8d23310aa8c411 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Tue, 18 Dec 2018 09:20:54 -0800 Subject: Call AdvertiseRefsHook before validating wants AdvertiseRefsHook is used to limit the visibility of the refs in Gerrit. If this hook is not called, then all refs are treated as visible, causing the server to serve commits reachable from branches the client should not be able to access, if asked to via a request naming a guessed object id. This bug was introduced in v2.0.0.201206130900-r~123 (Modify refs in UploadPack/ReceivePack using a hook interface, 2012-02-08). Stateful bidirectional transports are not affected. Fix it by moving the AdvertiseRefsHook call to getAdvertisedOrDefaultRefs, ensuring the hook is called in all cases. [jn: backported to stable-4.5 by splitting out tests and the protocol v2 specific parts] Change-Id: I159f396216354f2eda3968d17802e166d8c8ec2d Signed-off-by: Masaya Suzuki Signed-off-by: Jonathan Nieder Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/transport/UploadPack.java | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index d1fd67e977..0b1848c311 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -82,7 +82,6 @@ import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.AsyncRevObjectQueue; import org.eclipse.jgit.revwalk.DepthWalk; @@ -706,8 +705,14 @@ public class UploadPack { } private Map getAdvertisedOrDefaultRefs() throws IOException { - if (refs == null) - setAdvertisedRefs(db.getRefDatabase().getRefs(RefDatabase.ALL)); + if (refs != null) { + return refs; + } + + advertiseRefsHook.advertiseRefs(this); + if (refs == null) { + setAdvertisedRefs(db.getRefDatabase().getRefs(ALL)); + } return refs; } @@ -866,15 +871,7 @@ public class UploadPack { */ public void sendAdvertisedRefs(final RefAdvertiser adv) throws IOException, ServiceMayNotContinueException { - try { - advertiseRefsHook.advertiseRefs(this); - } catch (ServiceMayNotContinueException fail) { - if (fail.getMessage() != null) { - adv.writeOne("ERR " + fail.getMessage()); //$NON-NLS-1$ - fail.setOutput(); - } - throw fail; - } + Map advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs(); adv.init(db); adv.advertiseCapability(OPTION_INCLUDE_TAG); @@ -899,7 +896,6 @@ public class UploadPack { adv.advertiseCapability(OPTION_ALLOW_REACHABLE_SHA1_IN_WANT); adv.advertiseCapability(OPTION_AGENT, UserAgent.get()); adv.setDerefTags(true); - Map advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs(); findSymrefs(adv, advertisedOrDefaultRefs); advertised = adv.send(advertisedOrDefaultRefs); if (adv.isEmpty()) -- cgit v1.2.3 From 8eecb4f8b746bc01f09df02870e89d4bc4e118b9 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Tue, 18 Dec 2018 09:20:54 -0800 Subject: Call AdvertiseRefsHook for protocol v2 AdvertiseRefsHook is used to limit the visibility of the refs in Gerrit. If this hook is not called, then all refs are treated as visible. In protocol v2, the hook is not called, causing the server to advertise all refs. This bug was introduced in v5.0.0.201805221745-rc1~1^2~9 (Execute AdvertiseRefsHook only for protocol v0 and v1, 2018-05-14). Even before then, the hook was not called in requests after the capability advertisement, so in transports like HTTP that do not retain state between round-trips, the server would advertise all refs in response to an ls-refs (ls-remote) request. Fix both cases by using getAdvertisedOrDefaultRefs to retrieve the advertised refs in lsRefs, ensuring the hook is called in all cases that use its result. [jn: backported to stable-5.0; split out from a larger patch that also fixes protocol v0; avoided filtering this.refs by ref prefix] Change-Id: I64bce0e72d15b90baccc235c067e57b6af21b55f Signed-off-by: Masaya Suzuki Signed-off-by: Jonathan Nieder Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/transport/UploadPack.java | 60 +++++++++++++++++----- 1 file changed, 46 insertions(+), 14 deletions(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 3255a71b3e..1cdf8f7f1b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -43,8 +43,9 @@ package org.eclipse.jgit.transport; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; import static org.eclipse.jgit.lib.Constants.R_TAGS; -import static org.eclipse.jgit.lib.RefDatabase.ALL; import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_FETCH; import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_LS_REFS; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT; @@ -261,12 +262,18 @@ public class UploadPack { private OutputStream msgOut = NullOutputStream.INSTANCE; - /** The refs we advertised as existing at the start of the connection. */ + /** + * Refs eligible for advertising to the client, set using + * {@link #setAdvertisedRefs}. + */ private Map refs; /** Hook used while advertising the refs to the client. */ private AdvertiseRefsHook advertiseRefsHook = AdvertiseRefsHook.DEFAULT; + /** Whether the {@link #advertiseRefsHook} has been invoked. */ + private boolean advertiseRefsHookCalled; + /** Filter used while advertising the refs to the client. */ private RefFilter refFilter = RefFilter.DEFAULT; @@ -784,12 +791,47 @@ public class UploadPack { } advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; if (refs == null) { - setAdvertisedRefs(db.getRefDatabase().getRefs(ALL)); + // Fall back to all refs. + setAdvertisedRefs( + db.getRefDatabase().getRefs().stream() + .collect(toMap(Ref::getName, identity()))); } return refs; } + private Map getFilteredRefs(Collection refPrefixes) + throws IOException { + if (refPrefixes.isEmpty()) { + return getAdvertisedOrDefaultRefs(); + } + if (refs == null && !advertiseRefsHookCalled) { + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + } + if (refs == null) { + // Fast path: the advertised refs hook did not set advertised refs. + Map rs = new HashMap<>(); + for (String p : refPrefixes) { + for (Ref r : db.getRefDatabase().getRefsByPrefix(p)) { + rs.put(r.getName(), r); + } + } + if (refFilter != RefFilter.DEFAULT) { + return refFilter.filter(rs); + } + return transferConfig.getRefFilter().filter(rs); + } + + // Slow path: filter the refs provided by the advertised refs hook. + // refFilter has already been applied to refs. + return refs.values().stream() + .filter(ref -> refPrefixes.stream() + .anyMatch(ref.getName()::startsWith)) + .collect(toMap(Ref::getName, identity())); + } + private void service() throws IOException { boolean sendPack = false; // If it's a non-bidi request, we need to read the entire request before @@ -913,17 +955,7 @@ public class UploadPack { } rawOut.stopBuffering(); - Map refsToSend; - if (refPrefixes.isEmpty()) { - refsToSend = getAdvertisedOrDefaultRefs(); - } else { - refsToSend = new HashMap<>(); - for (String refPrefix : refPrefixes) { - for (Ref ref : db.getRefDatabase().getRefsByPrefix(refPrefix)) { - refsToSend.put(ref.getName(), ref); - } - } - } + Map refsToSend = getFilteredRefs(refPrefixes); if (needToFindSymrefs) { findSymrefs(adv, refsToSend); -- cgit v1.2.3 From 1638a2fce8e2a71f4cdfdee278e0cb9699860add Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 18 Dec 2018 19:44:39 -0800 Subject: UploadPack: Defer want-ref resolution to after parsing ProtocolV2Parser explains: // TODO(ifrade): This validation should be done after the // protocol parsing. It is not a protocol problem asking for an // unexisting ref and we wouldn't need the ref database here. Do so. This way all ref database accesses are in one place, in the UploadPack class. No user-visible change intended --- this is just to make the code easier to manipulate. Change-Id: I68e87dff7b9a63ccc169bd0836e8e8baaf5d1048 Signed-off-by: Jonathan Nieder Signed-off-by: Matthias Sohn --- .../jgit/transport/ProtocolV2ParserTest.java | 45 ++++++++-------------- .../org/eclipse/jgit/transport/FetchV2Request.java | 22 +++++------ .../eclipse/jgit/transport/ProtocolV2Parser.java | 32 ++------------- .../src/org/eclipse/jgit/transport/UploadPack.java | 28 +++++++++++--- 4 files changed, 50 insertions(+), 77 deletions(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java index 4d1150844c..bf67d46d51 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java @@ -42,7 +42,6 @@ */ package org.eclipse.jgit.transport; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItems; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; @@ -160,8 +159,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertTrue(request.getOptions() .contains(GitProtocolConstants.OPTION_THIN_PACK)); assertTrue(request.getOptions() @@ -191,8 +189,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -211,8 +208,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -229,8 +225,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -244,8 +239,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(0, request.getFilterBlobLimit()); } @@ -256,8 +250,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(15, request.getFilterBlobLimit()); } @@ -270,8 +263,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(0, request.getFilterBlobLimit()); } @@ -282,8 +274,7 @@ public class ProtocolV2ParserTest { "filter blob:limit=12", PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + parser.parseFetchRequest(pckIn); } @Test @@ -300,23 +291,16 @@ public class ProtocolV2ParserTest { ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowRefInWant().done()); - - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(1, request.getWantedRefs().size()); - assertThat(request.getWantedRefs().keySet(), - hasItems("refs/heads/branchA")); - assertEquals(2, request.getWantsIds().size()); + assertThat(request.getWantedRefs(), hasItems("refs/heads/branchA")); + assertEquals(1, request.getWantsIds().size()); assertThat(objIdsAsStrings(request.getWantsIds()), - hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819", - one.getName())); + hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819")); } @Test public void testFetchWithRefInWantUnknownRef() throws Exception { - thrown.expect(PackProtocolException.class); - thrown.expectMessage(containsString("refs/heads/branchC")); - PacketLineIn pckIn = formatAsPacketLine(PacketLineIn.DELIM, "want e4980cdc48cfa1301493ca94eb70523f6788b819", "want-ref refs/heads/branchC", @@ -329,8 +313,9 @@ public class ProtocolV2ParserTest { testRepo.update("branchA", one); testRepo.update("branchB", two); - parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); + assertEquals(1, request.getWantedRefs().size()); + assertThat(request.getWantedRefs(), hasItems("refs/heads/branchC")); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java index 853d96905c..34f3484951 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java @@ -45,9 +45,7 @@ package org.eclipse.jgit.transport; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.TreeMap; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.lib.ObjectId; @@ -63,7 +61,7 @@ import org.eclipse.jgit.lib.ObjectId; public final class FetchV2Request { private final List peerHas; - private final TreeMap wantedRefs; + private final List wantedRefs; private final Set wantsIds; @@ -82,7 +80,7 @@ public final class FetchV2Request { private final boolean doneReceived; private FetchV2Request(List peerHas, - TreeMap wantedRefs, Set wantsIds, + List wantedRefs, Set wantsIds, Set clientShallowCommits, int deepenSince, List deepenNotRefs, int depth, long filterBlobLimit, boolean doneReceived, Set options) { @@ -110,12 +108,12 @@ public final class FetchV2Request { * @return list of references in the "want-ref" lines of the request */ @NonNull - Map getWantedRefs() { - return this.wantedRefs; + List getWantedRefs() { + return wantedRefs; } /** - * @return object ids in the "want" (and "want-ref") lines of the request + * @return object ids in the "want" (but not "want-ref") lines of the request */ @NonNull Set getWantsIds() { @@ -198,7 +196,7 @@ public final class FetchV2Request { static final class Builder { List peerHas = new ArrayList<>(); - TreeMap wantedRefs = new TreeMap<>(); + List wantedRefs = new ArrayList<>(); Set wantsIds = new HashSet<>(); @@ -234,12 +232,10 @@ public final class FetchV2Request { * * @param refName * reference name - * @param oid - * object id * @return the builder */ - Builder addWantedRef(String refName, ObjectId oid) { - wantedRefs.put(refName, oid); + Builder addWantedRef(String refName) { + wantedRefs.add(refName); return this; } @@ -258,7 +254,7 @@ public final class FetchV2Request { * from a "want" line in a fetch request * @return the builder */ - Builder addWantsIds(ObjectId objectId) { + Builder addWantsId(ObjectId objectId) { wantsIds.add(objectId); return this; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java index eae2c6edb9..2cc50a7f38 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java @@ -57,8 +57,6 @@ import java.text.MessageFormat; import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefDatabase; /** * Parse the incoming git protocol lines from the wire and translate them into a @@ -79,24 +77,17 @@ final class ProtocolV2Parser { * Parse the incoming fetch request arguments from the wire. The caller must * be sure that what is comings is a fetch request before coming here. * - * This operation requires the reference database to validate incoming - * references. - * * @param pckIn * incoming lines - * @param refdb - * reference database (to validate that received references exist - * and point to valid objects) * @return A FetchV2Request populated with information received from the * wire. * @throws PackProtocolException * incompatible options, wrong type of arguments or other issues * where the request breaks the protocol. * @throws IOException - * an IO error prevented reading the incoming message or - * accessing the ref database. + * an IO error prevented reading the incoming message. */ - FetchV2Request parseFetchRequest(PacketLineIn pckIn, RefDatabase refdb) + FetchV2Request parseFetchRequest(PacketLineIn pckIn) throws PackProtocolException, IOException { FetchV2Request.Builder reqBuilder = FetchV2Request.builder(); @@ -116,25 +107,10 @@ final class ProtocolV2Parser { boolean filterReceived = false; while ((line = pckIn.readString()) != PacketLineIn.END) { if (line.startsWith("want ")) { //$NON-NLS-1$ - reqBuilder.addWantsIds(ObjectId.fromString(line.substring(5))); + reqBuilder.addWantsId(ObjectId.fromString(line.substring(5))); } else if (transferConfig.isAllowRefInWant() && line.startsWith(OPTION_WANT_REF + " ")) { //$NON-NLS-1$ - String refName = line.substring(OPTION_WANT_REF.length() + 1); - // TODO(ifrade): This validation should be done after the - // protocol parsing. It is not a protocol problem asking for an - // unexisting ref and we wouldn't need the ref database here - Ref ref = refdb.exactRef(refName); - if (ref == null) { - throw new PackProtocolException(MessageFormat - .format(JGitText.get().invalidRefName, refName)); - } - ObjectId oid = ref.getObjectId(); - if (oid == null) { - throw new PackProtocolException(MessageFormat - .format(JGitText.get().invalidRefName, refName)); - } - reqBuilder.addWantedRef(refName, oid); - reqBuilder.addWantsIds(oid); + reqBuilder.addWantedRef(line.substring(OPTION_WANT_REF.length() + 1)); } else if (line.startsWith("have ")) { //$NON-NLS-1$ reqBuilder.addPeerHas(ObjectId.fromString(line.substring(5))); } else if (line.equals("done")) { //$NON-NLS-1$ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index c753bcdc7a..7e29c9b7a2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -79,6 +79,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeMap; import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.CorruptObjectException; @@ -994,8 +995,7 @@ public class UploadPack { } ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig); - FetchV2Request req = parser.parseFetchRequest(pckIn, - db.getRefDatabase()); + FetchV2Request req = parser.parseFetchRequest(pckIn); rawOut.stopBuffering(); protocolV2Hook.onFetch(req); @@ -1003,13 +1003,29 @@ public class UploadPack { // TODO(ifrade): Refactor to pass around the Request object, instead of // copying data back to class fields options = req.getOptions(); - wantIds.addAll(req.getWantsIds()); clientShallowCommits = req.getClientShallowCommits(); depth = req.getDepth(); shallowSince = req.getDeepenSince(); filterBlobLimit = req.getFilterBlobLimit(); deepenNotRefs = req.getDeepenNotRefs(); + wantIds.addAll(req.getWantsIds()); + Map wantedRefs = new TreeMap<>(); + for (String refName : req.getWantedRefs()) { + Ref ref = db.getRefDatabase().exactRef(refName); + if (ref == null) { + throw new PackProtocolException(MessageFormat + .format(JGitText.get().invalidRefName, refName)); + } + ObjectId oid = ref.getObjectId(); + if (oid == null) { + throw new PackProtocolException(MessageFormat + .format(JGitText.get().invalidRefName, refName)); + } + wantIds.add(oid); + wantedRefs.put(refName, oid); + } + boolean sectionSent = false; @Nullable List shallowCommits = null; List unshallowCommits = new ArrayList<>(); @@ -1059,13 +1075,13 @@ public class UploadPack { sectionSent = true; } - if (!req.getWantedRefs().isEmpty()) { + if (!wantedRefs.isEmpty()) { if (sectionSent) { pckOut.writeDelim(); } pckOut.writeString("wanted-refs\n"); //$NON-NLS-1$ - for (Map.Entry entry : req.getWantedRefs() - .entrySet()) { + for (Map.Entry entry : + wantedRefs.entrySet()) { pckOut.writeString(entry.getValue().getName() + ' ' + entry.getKey() + '\n'); } -- cgit v1.2.3 From fcafdcc4048dbd61754b3f1d3ae19395fb788d45 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 18 Dec 2018 21:00:42 -0800 Subject: UploadPack: Filter refs used for want-ref resolution In the longer term, we can add support for this to the RequestValidator interface. In the short term, this is a minimal band-aid to ensure any refs the client requests are visible to the client. Change-Id: I0683c7a00e707cf97eef6c6bb782671d0a550ffe Reported-by: Ivan Frade Signed-off-by: Jonathan Nieder Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/transport/TransferConfig.java | 10 +++++++ .../src/org/eclipse/jgit/transport/UploadPack.java | 33 +++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java index 59740c4dc8..db95396047 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java @@ -326,6 +326,16 @@ public class TransferConfig { }; } + /** + * Like {@code getRefFilter() == RefFilter.DEFAULT}, but faster. + * + * @return {@code true} if no ref filtering is needed because there + * are no configured hidden refs. + */ + boolean hasDefaultRefFilter() { + return hideRefs.length == 0; + } + static class FsckKeyNameHolder { private static final Map errors; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 7e29c9b7a2..bac877a272 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -847,6 +847,37 @@ public class UploadPack { .collect(toMap(Ref::getName, identity())); } + /** + * Read a ref on behalf of the client. + *

+ * This checks that the ref is present in the ref advertisement since + * otherwise the client might not be supposed to be able to read it. + * + * @param name + * the unabbreviated name of the reference. + * @return the requested Ref, or {@code null} if it is not visible or + * does not exist. + * @throws java.io.IOException + * on failure to read the ref or check it for visibility. + */ + @Nullable + private Ref getRef(String name) throws IOException { + if (refs != null) { + return refs.get(name); + } + if (!advertiseRefsHookCalled) { + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + } + if (refs == null && + refFilter == RefFilter.DEFAULT && + transferConfig.hasDefaultRefFilter()) { + // Fast path: no ref filtering is needed. + return db.getRefDatabase().exactRef(name); + } + return getAdvertisedOrDefaultRefs().get(name); + } + private void service() throws IOException { boolean sendPack = false; // If it's a non-bidi request, we need to read the entire request before @@ -1012,7 +1043,7 @@ public class UploadPack { wantIds.addAll(req.getWantsIds()); Map wantedRefs = new TreeMap<>(); for (String refName : req.getWantedRefs()) { - Ref ref = db.getRefDatabase().exactRef(refName); + Ref ref = getRef(refName); if (ref == null) { throw new PackProtocolException(MessageFormat .format(JGitText.get().invalidRefName, refName)); -- cgit v1.2.3 From c961e2d7730b5e0e63841e16bf82ec3d9f589133 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 26 Dec 2018 12:40:28 -0800 Subject: UploadPack: Avoid calling AdvertiseRefsHook twice The AdvertiseRefsHook can be called twice if the following conditions hold: 1. This AdvertiseRefsHook doesn't set this.refs. 2. getAdvertisedOrDefaultRefs is called after getFilteredRefs. For example, this can happen when fetchV2 is called after lsRefsV2 when using a stateful bidirectional transport. The second call does not accomplish anything useful. Guard it with 'if (!advertiseRefsHookCalled)' to avoid wasted work. Reported-by: Jonathan Tan Change-Id: Ib746582e4ef645b767a5b3fb969596df99ac2ab5 Signed-off-by: Jonathan Nieder --- org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 1cdf8f7f1b..6a3d9a1cd4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -790,8 +790,10 @@ public class UploadPack { return refs; } - advertiseRefsHook.advertiseRefs(this); - advertiseRefsHookCalled = true; + if (!advertiseRefsHookCalled) { + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + } if (refs == null) { // Fall back to all refs. setAdvertisedRefs( -- cgit v1.2.3 From eb4c63fbbf0972d64af09ba31fdd758944a677cb Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 22 Dec 2018 13:53:10 -0800 Subject: UploadPack: Filter refs used for deepen-not resolution Clients can use --shallow-exclude to obtain information about what commits are reachable from refs they are not supposed to be able to see. Plug the hole by allowing the AdvertiseRefsHook and RefFilter to take effect here, too. Change-Id: If2b8e95344fa49e10a6a202144318b60d002490e Signed-off-by: Jonathan Nieder --- .../src/org/eclipse/jgit/transport/UploadPack.java | 35 +++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) (limited to 'org.eclipse.jgit/src') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 62e8ae0cce..2fbcaa2928 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -98,6 +98,7 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.AsyncRevObjectQueue; import org.eclipse.jgit.revwalk.BitmapWalker; @@ -878,6 +879,38 @@ public class UploadPack { return getAdvertisedOrDefaultRefs().get(name); } + /** + * Find a ref in the usual search path on behalf of the client. + *

+ * This checks that the ref is present in the ref advertisement since + * otherwise the client might not be supposed to be able to read it. + * + * @param name + * short name of the ref to find, e.g. "master" to find + * "refs/heads/master". + * @return the requested Ref, or {@code null} if it is not visible or + * does not exist. + * @throws java.io.IOException + * on failure to read the ref or check it for visibility. + */ + @Nullable + private Ref findRef(String name) throws IOException { + if (refs != null) { + return RefDatabase.findRef(refs, name); + } + if (!advertiseRefsHookCalled) { + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + } + if (refs == null && + refFilter == RefFilter.DEFAULT && + transferConfig.hasDefaultRefFilter()) { + // Fast path: no ref filtering is needed. + return db.getRefDatabase().getRef(name); + } + return RefDatabase.findRef(getAdvertisedOrDefaultRefs(), name); + } + private void service() throws IOException { boolean sendPack = false; // If it's a non-bidi request, we need to read the entire request before @@ -1033,7 +1066,7 @@ public class UploadPack { // copying data back to class fields List deepenNots = new ArrayList<>(); for (String s : req.getDeepenNotRefs()) { - Ref ref = db.getRefDatabase().getRef(s); + Ref ref = findRef(s); if (ref == null) { throw new PackProtocolException(MessageFormat .format(JGitText.get().invalidRefName, s)); -- cgit v1.2.3