Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

["Shawn O. Pearce" <spearce@spearce.org>]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> index e022e57..632c8ad 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> @@ -318,7 +318,7 @@ public class URIish {
>  			r.append(getUser());
>  			if (getPass() != null) {
>  				r.append(':');
> -				r.append(getPass());
> +				r.append("PASSWORD");
>  			}
>  		}
>  

I agree in theory, but the implementation isn't quite correct.
How about this instead?

--8<--
Avoid password leak from URIIsh

The toString() method is commonly used for dumping information. We
never ever want to use toString when the password is needed. By
masking out the password we avoid unintentional password leaks.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/transport/URIishTest.java |    3 ++-
 .../org/spearce/jgit/transport/RemoteConfig.java   |    2 +-
 .../src/org/spearce/jgit/transport/URIish.java     |   15 ++++++++++++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
index a460418..2e5e847 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
@@ -214,7 +214,8 @@ public class URIishTest extends TestCase {
 		assertEquals("user", u.getUser());
 		assertEquals("pass", u.getPass());
 		assertEquals(33, u.getPort());
-		assertEquals(str, u.toString());
+		assertEquals(str, u.toPrivateString());
+		assertEquals(u.setPass(null).toPrivateString(), u.toString());
 		assertEquals(u, new URIish(str));
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
index b0fd5b4..bb21511 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
@@ -150,7 +150,7 @@ public class RemoteConfig {
 
 		vlst.clear();
 		for (final URIish u : getURIs())
-			vlst.add(u.toString());
+			vlst.add(u.toPrivateString());
 		rc.setStringList(SECTION, getName(), KEY_URL, vlst);
 
 		vlst.clear();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
index e022e57..54a0811 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
@@ -307,7 +307,20 @@ public class URIish {
 		return a.equals(b);
 	}
 
+	/**
+	 * Obtain the string form of the URI, with the password included.
+	 * 
+	 * @return the URI, including its password field, if any.
+	 */
+	public String toPrivateString() {
+		return format(true);
+	}
+
 	public String toString() {
+		return format(false);
+	}
+
+	private String format(final boolean includePassword) {
 		final StringBuilder r = new StringBuilder();
 		if (getScheme() != null) {
 			r.append(getScheme());
@@ -316,7 +329,7 @@ public class URIish {
 
 		if (getUser() != null) {
 			r.append(getUser());
-			if (getPass() != null) {
+			if (includePassword && getPass() != null) {
 				r.append(':');
 				r.append(getPass());
 			}
-- 
1.5.6.2.393.g45096

-- 
Shawn.