From 1b4daa2994a16bad5b9b77b24d8ce1f1d25f78fa Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 23 Aug 2017 11:50:05 +0200 Subject: [PATCH] Cleanup: message reporting for HTTP redirect handling The addition of "tooManyRedirects" in commit 7ac1bfc ("Do authentication re-tries on HTTP POST") was an error I didn't catch after rebasing that change. That message had been renamed in the earlier commit e17bfc9 ("Add support to follow HTTP redirects") to "redirectLimitExceeded". Also make sure we always use the TransportException(URIish, ...) constructor; it'll prefix the message given with the sanitized URI. Change messages to remove the explicit mention of that URI inside the message. Adapt tests that check the expected exception message text. For the info logging of redirects, remove a potentially present password component in the URI to avoid leaking it into the log. Change-Id: I517112404757a9a947e92aaace743c6541dce6aa Signed-off-by: Thomas Wolf --- .../http/test/SmartClientSmartServerTest.java | 8 ++--- .../eclipse/jgit/internal/JGitText.properties | 11 +++---- .../org/eclipse/jgit/internal/JGitText.java | 1 - .../eclipse/jgit/transport/TransportHttp.java | 33 +++++++++++-------- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index 1b0c6949a..51b79903c 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -595,9 +595,9 @@ public class SmartClientSmartServerTest extends HttpTestCase { t.fetch(NullProgressMonitor.INSTANCE, mirror(master)); fail("Should have failed (too many redirects)"); } catch (TransportException e) { - String expectedMessageBegin = MessageFormat.format( - JGitText.get().redirectLimitExceeded, remoteUri, "3", - remoteUri.replace("/4/", "/1/") + '/', ""); + String expectedMessageBegin = remoteUri.toString() + ": " + + MessageFormat.format(JGitText.get().redirectLimitExceeded, + "3", remoteUri.replace("/4/", "/1/") + '/', ""); String message = e.getMessage(); if (message.length() > expectedMessageBegin.length()) { message = message.substring(0, expectedMessageBegin.length()); @@ -616,7 +616,7 @@ public class SmartClientSmartServerTest extends HttpTestCase { t.fetch(NullProgressMonitor.INSTANCE, mirror(master)); fail("Should have failed (redirect loop)"); } catch (TransportException e) { - assertTrue(e.getMessage().contains("redirected more than")); + assertTrue(e.getMessage().contains("Redirected more than")); } } 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 c532b328d..8d3931493 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -365,7 +365,7 @@ invalidPathContainsSeparator=Invalid path (contains separator ''{0}''): {1} invalidPathPeriodAtEndWindows=Invalid path (period at end is ignored by Windows): {0} invalidPathSpaceAtEndWindows=Invalid path (space at end is ignored by Windows): {0} invalidPathReservedOnWindows=Invalid path (''{0}'' is reserved on Windows): {1} -invalidRedirectLocation=Invalid redirect location {1} -> {2} +invalidRedirectLocation=Invalid redirect location {0} -> {1} invalidReflogRevision=Invalid reflog revision: {0} invalidRefName=Invalid ref name: {0} invalidReftableBlock=Invalid reftable block @@ -535,11 +535,11 @@ receivePackObjectTooLarge2=Object too large ({0} bytes), rejecting the pack. Max receivePackInvalidLimit=Illegal limit parameter value {0} receivePackTooLarge=Pack exceeds the limit of {0} bytes, rejecting the pack receivingObjects=Receiving objects -redirectBlocked=URI ''{0}'' redirection blocked: redirect {1} -> {2} not allowed +redirectBlocked=Redirection blocked: redirect {0} -> {1} not allowed redirectHttp=URI ''{0}'': following HTTP redirect #{1} {2} -> {3} -redirectLimitExceeded=URI ''{0}'' redirected more than {1} times; aborted at {2} -> {3} -redirectLocationMissing=Invalid redirect of ''{0}'': no redirect location for {1} -redirectsOff=Cannot redirect ''{0}'': http.followRedirects is false (HTTP status {1}) +redirectLimitExceeded=Redirected more than {0} times; aborted at {1} -> {2} +redirectLocationMissing=Invalid redirect: no redirect location for {0} +redirectsOff=Cannot redirect because http.followRedirects is false (HTTP status {0}) refAlreadyExists=already exists refAlreadyExists1=Ref {0} already exists reflogEntryNotFound=Entry {0} not found in reflog for ''{1}'' @@ -636,7 +636,6 @@ timeIsUncertain=Time is uncertain timerAlreadyTerminated=Timer already terminated tooManyCommands=Too many commands tooManyIncludeRecursions=Too many recursions; circular includes in config file(s)? -tooManyRedirects=Too many redirects; stopped after {0} redirects at ''{1}'' topologicalSortRequired=Topological sort required. transactionAborted=transaction aborted transportExceptionBadRef=Empty ref: {0}: {1} 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 5acf05876..07666eb93 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -696,7 +696,6 @@ public class JGitText extends TranslationBundle { /***/ public String timerAlreadyTerminated; /***/ public String tooManyCommands; /***/ public String tooManyIncludeRecursions; - /***/ public String tooManyRedirects; /***/ public String topologicalSortRequired; /***/ public String transportExceptionBadRef; /***/ public String transportExceptionEmptyRef; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java index 1647200e6..26612120c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java @@ -604,7 +604,8 @@ public class TransportHttp extends HttpTransport implements WalkTransport, case HttpConnection.HTTP_FORBIDDEN: throw new TransportException(uri, MessageFormat.format( - JGitText.get().serviceNotPermitted, service)); + JGitText.get().serviceNotPermitted, baseUrl, + service)); case HttpConnection.HTTP_MOVED_PERM: case HttpConnection.HTTP_MOVED_TEMP: @@ -614,9 +615,10 @@ public class TransportHttp extends HttpTransport implements WalkTransport, // and in general should occur only on POST requests. But it // doesn't hurt to accept it here as a redirect. if (http.followRedirects == HttpRedirectMode.FALSE) { - throw new TransportException(MessageFormat.format( - JGitText.get().redirectsOff, uri, - Integer.valueOf(status))); + throw new TransportException(uri, + MessageFormat.format( + JGitText.get().redirectsOff, + Integer.valueOf(status))); } URIish newUri = redirect(conn.getHeaderField(HDR_LOCATION), Constants.INFO_REFS, redirects++); @@ -655,31 +657,34 @@ public class TransportHttp extends HttpTransport implements WalkTransport, private URIish redirect(String location, String checkFor, int redirects) throws TransportException { if (location == null || location.isEmpty()) { - throw new TransportException(MessageFormat.format( - JGitText.get().redirectLocationMissing, uri, baseUrl)); + throw new TransportException(uri, + MessageFormat.format(JGitText.get().redirectLocationMissing, + baseUrl)); } if (redirects >= http.maxRedirects) { - throw new TransportException(MessageFormat.format( - JGitText.get().redirectLimitExceeded, uri, + throw new TransportException(uri, + MessageFormat.format(JGitText.get().redirectLimitExceeded, Integer.valueOf(http.maxRedirects), baseUrl, location)); } try { if (!isValidRedirect(baseUrl, location, checkFor)) { - throw new TransportException( + throw new TransportException(uri, MessageFormat.format(JGitText.get().redirectBlocked, - uri, baseUrl, location)); + baseUrl, location)); } location = location.substring(0, location.indexOf(checkFor)); URIish result = new URIish(location); if (LOG.isInfoEnabled()) { - LOG.info(MessageFormat.format(JGitText.get().redirectHttp, uri, + LOG.info(MessageFormat.format(JGitText.get().redirectHttp, + uri.setPass(null), Integer.valueOf(redirects), baseUrl, result)); } return result; } catch (URISyntaxException e) { - throw new TransportException(MessageFormat.format( - JGitText.get().invalidRedirectLocation, - uri, baseUrl, location), e); + throw new TransportException(uri, + MessageFormat.format(JGitText.get().invalidRedirectLocation, + baseUrl, location), + e); } }