Added code to ensure that passwords are not included git.remote.origin.url #241

Merged
merged 1 commit into from Mar 26, 2016

Projects

None yet

3 participants

@damnhandy
Contributor

PR for issue #240

@ceefour
ceefour commented Mar 26, 2016

👍 this is a security issue

@ktoso ktoso commented on the diff Mar 26, 2016
...va/pl/project13/maven/git/UriUserInfoRemoverTest.java
@@ -0,0 +1,54 @@
+package pl.project13.maven.git;
@ktoso
ktoso Mar 26, 2016 Owner

missing license header, I'll add

@ktoso ktoso commented on the diff Mar 26, 2016
...va/pl/project13/maven/git/UriUserInfoRemoverTest.java
+package pl.project13.maven.git;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.http.client.utils.URIBuilder;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
+
+/**
+ * Created by ryan on 3/21/16.
+ */
@ktoso
ktoso Mar 26, 2016 Owner

I try to not include any @author or Created by since it goes somewhat against collective ownership of code (if there's still any left in the code - let's remove those as well).

I'll clean that up.

@ktoso ktoso commented on the diff Mar 26, 2016
...va/pl/project13/maven/git/UriUserInfoRemoverTest.java
+ @Test
+ public void testHttpsUriWithoutUserInfo() throws Exception {
+ String result = GitDataProvider.stripCredentialsFromOriginUrl("https://example.com");
+ assertEquals("https://example.com", result);
+ }
+
+ @Test
+ public void testHttpsUriWithUserInfo() throws Exception {
+ String result = GitDataProvider.stripCredentialsFromOriginUrl("https://user@example.com");
+ assertEquals("https://user@example.com", result);
+ }
+
+ @Test
+ public void testHttpsUriWithUserInfoAndPassword() throws Exception {
+ String result = GitDataProvider.stripCredentialsFromOriginUrl("https://user:password@example.com");
+ assertEquals("https://user@example.com", result);
@ktoso
Owner
ktoso commented Mar 26, 2016

LGTM, thanks a lot for noticing and the fix - I would not have noticed as currently not using Maven in any of the projects I maintain (except this one).

I'll merge and cut a release shortly after today as you're right that it's a security issue.

@ktoso ktoso merged commit c2997b8 into ktoso:master Mar 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damnhandy
Contributor

Thanks for taking the change. This plugin is extremely useful to me and my teams. Interestingly, the core issue is really not in this plugin, but your code was easier to jump in and make a fix. It's well organized and has great test cases.

@ktoso
Owner
ktoso commented Mar 26, 2016

This plugin is extremely useful to me and my teams. Interestingly, the core issue is really not in this plugin, but your code was easier to jump in and make a fix. It's well organized and has great test cases.

I'm very glad to hear that. Yeah, it's not really our fault but let's clean up the problem since we can.

Thanks a lot for the PR!
I've released 2.2.1 with this fix - it'll be on central soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment