From e82cb5a6d32666a2e01a48088a0b830213a9ff3e Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 10 Oct 2018 17:02:49 -0700 Subject: [PATCH] FirstWant: tighten first-want line validation First-want line parsing accepts lines with an optional whitespace, when the spec is strict requiring a white space. Validate the line enforcing that there is a white space between oid and capabilities list. Change-Id: I45ada67030e0720f9b402c298be18c7518c799b1 Signed-off-by: Ivan Frade --- .../transport/parser/FirstWantTest.java | 45 ++++++++++++++++++- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../internal/transport/parser/FirstWant.java | 19 +++++--- .../eclipse/jgit/transport/UploadPack.java | 7 ++- 5 files changed, 63 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java index 572ec8bae..627079eac 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java @@ -44,17 +44,20 @@ package org.eclipse.jgit.internal.transport.parser; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Set; +import org.eclipse.jgit.errors.PackProtocolException; import org.junit.Test; public class FirstWantTest { @Test - public void testFirstWantWithOptions() { + public void testFirstWantWithOptions() throws PackProtocolException { String line = "want b9d4d1eb2f93058814480eae9e1b67550f46ee38 " + "no-progress include-tag ofs-delta agent=JGit/unknown"; @@ -69,7 +72,7 @@ public class FirstWantTest { } @Test - public void testFirstWantWithoutOptions() { + public void testFirstWantWithoutOptions() throws PackProtocolException { String line = "want b9d4d1eb2f93058814480eae9e1b67550f46ee38"; FirstWant r = FirstWant.fromLine(line); @@ -77,4 +80,42 @@ public class FirstWantTest { r.getLine()); assertTrue(r.getCapabilities().isEmpty()); } + + private String makeFirstWantLine(String capability) { + return String.format("want b9d4d1eb2f93058814480eae9e1b67550f46ee38 %s", capability); + } + + @Test + public void testFirstWantNoWhitespace() { + try { + FirstWant.fromLine( + "want b9d4d1eb2f93058814480eae9e1b67550f400000capability"); + fail("Accepting first want line without SP between oid and first capability"); + } catch (PackProtocolException e) { + // pass + } + } + + @Test + public void testFirstWantOnlyWhitespace() throws PackProtocolException { + FirstWant r = FirstWant + .fromLine("want b9d4d1eb2f93058814480eae9e1b67550f46ee38 "); + assertEquals("want b9d4d1eb2f93058814480eae9e1b67550f46ee38", + r.getLine()); + } + + @Test + public void testFirstWantValidCapabilityNames() + throws PackProtocolException { + List validNames = Arrays.asList( + "c", "cap", "C", "CAP", "1", "1cap", "cap-64k_test", + "-", "-cap", + "_", "_cap", "agent=pack.age/Version"); + + for (String capability: validNames) { + FirstWant r = FirstWant.fromLine(makeFirstWantLine(capability)); + assertEquals(r.getCapabilities().size(), 1); + assertTrue(r.getCapabilities().contains(capability)); + } + } } 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 40915b7b1..b3f7b89c9 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -771,6 +771,7 @@ URINotSupported=URI not supported: {0} userConfigFileInvalid=User config file {0} invalid {1} validatingGitModules=Validating .gitmodules files walkFailure=Walk failure. +wantNoSpaceWithCapabilities=No space between oid and first capability in first want line wantNotValid=want {0} not valid weeksAgo={0} weeks ago windowSizeMustBeLesserThanLimit=Window size must be < limit 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 7a9d523de..9f0e3e0c2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -832,6 +832,7 @@ public class JGitText extends TranslationBundle { /***/ public String userConfigFileInvalid; /***/ public String validatingGitModules; /***/ public String walkFailure; + /***/ public String wantNoSpaceWithCapabilities; /***/ public String wantNotValid; /***/ public String weeksAgo; /***/ public String windowSizeMustBeLesserThanLimit; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java index ea0fbed1d..1ac9b1887 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java @@ -42,10 +42,14 @@ */ package org.eclipse.jgit.internal.transport.parser; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.Set; +import org.eclipse.jgit.errors.PackProtocolException; +import org.eclipse.jgit.internal.JGitText; + /** * In the pack negotiation phase (protocol v0/v1), the client sends a list of * wants. The first "want" line is special, as it (can) have a list of @@ -74,20 +78,21 @@ public class FirstWant { * @param line * line from the client. * @return an instance of FirstWant + * @throws PackProtocolException + * if the line doesn't follow the protocol format. */ - public static FirstWant fromLine(String line) { + public static FirstWant fromLine(String line) throws PackProtocolException { String wantLine; Set capabilities; if (line.length() > 45) { - final HashSet opts = new HashSet<>(); String opt = line.substring(45); - if (opt.startsWith(" ")) { //$NON-NLS-1$ - opt = opt.substring(1); - } - for (String c : opt.split(" ")) { //$NON-NLS-1$ - opts.add(c); + if (!opt.startsWith(" ")) { //$NON-NLS-1$ + throw new PackProtocolException(JGitText.get().wantNoSpaceWithCapabilities); } + opt = opt.substring(1); + HashSet opts = new HashSet<>( + Arrays.asList(opt.split(" "))); //$NON-NLS-1$ wantLine = line.substring(0, 45); capabilities = Collections.unmodifiableSet(opts); } else { 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 8d2158d56..82e174548 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -69,6 +69,7 @@ import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.UncheckedIOException; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collection; @@ -193,7 +194,11 @@ public class UploadPack { * line from the client. */ public FirstLine(String line) { - firstWant = FirstWant.fromLine(line); + try { + firstWant = FirstWant.fromLine(line); + } catch (PackProtocolException e) { + throw new UncheckedIOException(e); + } } /** @return non-capabilities part of the line. */