Improving URL check for organisation info

4 files changed, 74 insertions, 22 deletions

diff --git a/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/main/java/org/apache/archiva/admin/repository/admin/DefaultArchivaAdministration.java b/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/main/java/org/apache/archiva/admin/repository/admin/DefaultArchivaAdministration.java
index 1ba104863..3be9f58dd 100644
--- a/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/main/java/org/apache/archiva/admin/repository/admin/DefaultArchivaAdministration.java
+++ b/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/main/java/org/apache/archiva/admin/repository/admin/DefaultArchivaAdministration.java
@@ -21,20 +21,14 @@ package org.apache.archiva.admin.repository.admin;
 import org.apache.archiva.admin.model.AuditInformation;
 import org.apache.archiva.admin.model.RepositoryAdminException;
 import org.apache.archiva.admin.model.admin.ArchivaAdministration;
-import org.apache.archiva.admin.model.beans.FileType;
-import org.apache.archiva.admin.model.beans.LegacyArtifactPath;
-import org.apache.archiva.admin.model.beans.NetworkConfiguration;
-import org.apache.archiva.admin.model.beans.OrganisationInformation;
-import org.apache.archiva.admin.model.beans.UiConfiguration;
+import org.apache.archiva.admin.model.beans.*;
 import org.apache.archiva.admin.repository.AbstractRepositoryAdmin;
 import org.apache.archiva.configuration.Configuration;
 import org.apache.archiva.configuration.UserInterfaceOptions;
 import org.apache.archiva.configuration.WebappConfiguration;
 import org.apache.archiva.metadata.model.facets.AuditEvent;
-import org.apache.commons.codec.net.URLCodec;
 import org.apache.commons.lang.StringEscapeUtils;
 import org.apache.commons.lang.StringUtils;
-import org.apache.http.impl.conn.PoolingClientConnectionManager;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 import org.apache.maven.wagon.providers.http.HttpWagon;
 import org.springframework.stereotype.Service;
@@ -42,10 +36,8 @@ import org.springframework.util.ResourceUtils;
 
 import javax.annotation.PostConstruct;
 import javax.annotation.PreDestroy;
-import java.io.UnsupportedEncodingException;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLEncoder;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -328,14 +320,21 @@ public class DefaultArchivaAdministration
         return getModelMapper().map( organisationInformation, OrganisationInformation.class );
     }
 
-    private void checkUrl(String url, String propertyName)  throws RepositoryAdminException {
+    private String fixUrl(String url, String propertyName)  throws RepositoryAdminException {
         if ( StringUtils.isNotEmpty( url ) )
         {
             if ( !ResourceUtils.isUrl( url ) )
             {
                 throw new RepositoryAdminException( "Bad URL in " + propertyName + ": " + url );
             }
+            try {
+                URI urlToCheck = new URI(url);
+                return urlToCheck.toString();
+            } catch (URISyntaxException e) {
+                throw new RepositoryAdminException( "Bad URL in " + propertyName + ": " + url );
+            }
         }
+        return url;
 
     }
 
@@ -347,8 +346,9 @@ public class DefaultArchivaAdministration
     public void setOrganisationInformation( OrganisationInformation organisationInformation )
         throws RepositoryAdminException
     {
-        checkUrl(organisationInformation.getUrl(), "url");
-        checkUrl( organisationInformation.getLogoLocation(), "logoLocation" );
+
+        organisationInformation.setUrl(fixUrl(organisationInformation.getUrl(), "url"));
+        organisationInformation.setLogoLocation(fixUrl( organisationInformation.getLogoLocation(), "logoLocation" ));
         Configuration configuration = getArchivaConfiguration( ).getConfiguration( );
         if ( organisationInformation != null )
         {
diff --git a/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/test/java/org/apache/archiva/admin/repository/admin/ArchivaAdministrationTest.java b/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/test/java/org/apache/archiva/admin/repository/admin/ArchivaAdministrationTest.java
index 9bb9ed443..e597de438 100644
--- a/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/test/java/org/apache/archiva/admin/repository/admin/ArchivaAdministrationTest.java
+++ b/archiva-modules/archiva-base/archiva-repository-admin/archiva-repository-admin-default/src/test/java/org/apache/archiva/admin/repository/admin/ArchivaAdministrationTest.java
@@ -222,7 +222,7 @@ public class ArchivaAdministrationTest
         try
         {
             OrganisationInformation newOrganisationInformation = new OrganisationInformation( );
-            newOrganisationInformation.setLogoLocation( "'/><svg/onload=alert(/logoLocation_xss/)>" );
+            newOrganisationInformation.setLogoLocation( "http://www.foo.com'/><svg/onload=alert(/logoLocation_xss/)>" );
             newOrganisationInformation.setName( "foo org" );
             newOrganisationInformation.setUrl( "http://foo.com" );
             archivaAdministration.setOrganisationInformation( newOrganisationInformation );
@@ -240,7 +240,7 @@ public class ArchivaAdministrationTest
         try
         {
             OrganisationInformation newOrganisationInformation = new OrganisationInformation( );
-            newOrganisationInformation.setUrl( "'/><svg/onload=alert(/url_xss/)>" );
+            newOrganisationInformation.setUrl( "http://foo.com'/><svg/onload=alert(/url_xss/)>" );
             newOrganisationInformation.setName( "foo org" );
             newOrganisationInformation.setLogoLocation( "http://foo.com/bar.png" );
             archivaAdministration.setOrganisationInformation( newOrganisationInformation );
diff --git a/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/main/java/org/apache/archiva/rest/services/DefaultArchivaAdministrationService.java b/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/main/java/org/apache/archiva/rest/services/DefaultArchivaAdministrationService.java
index ff1d3fd18..51144e87a 100644
--- a/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/main/java/org/apache/archiva/rest/services/DefaultArchivaAdministrationService.java
+++ b/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/main/java/org/apache/archiva/rest/services/DefaultArchivaAdministrationService.java
@@ -20,11 +20,7 @@ package org.apache.archiva.rest.services;
 
 import org.apache.archiva.admin.model.RepositoryAdminException;
 import org.apache.archiva.admin.model.admin.ArchivaAdministration;
-import org.apache.archiva.admin.model.beans.FileType;
-import org.apache.archiva.admin.model.beans.LegacyArtifactPath;
-import org.apache.archiva.admin.model.beans.NetworkConfiguration;
-import org.apache.archiva.admin.model.beans.OrganisationInformation;
-import org.apache.archiva.admin.model.beans.UiConfiguration;
+import org.apache.archiva.admin.model.beans.*;
 import org.apache.archiva.repository.scanner.RepositoryContentConsumers;
 import org.apache.archiva.rest.api.model.AdminRepositoryConsumer;
 import org.apache.archiva.rest.api.services.ArchivaAdministrationService;
@@ -319,7 +315,7 @@ public class DefaultArchivaAdministrationService
         }
         catch ( RepositoryAdminException e )
         {
-            throw new ArchivaRestServiceException( e.getMessage(), e );
+            throw new ArchivaRestServiceException( e.getMessage(), 400, e );
         }
     }
 
diff --git a/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/test/java/org/apache/archiva/rest/services/ArchivaAdministrationServiceTest.java b/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/test/java/org/apache/archiva/rest/services/ArchivaAdministrationServiceTest.java
index e3377f21a..e6a87b003 100644
--- a/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/test/java/org/apache/archiva/rest/services/ArchivaAdministrationServiceTest.java
+++ b/archiva-modules/archiva-web/archiva-rest/archiva-rest-services/src/test/java/org/apache/archiva/rest/services/ArchivaAdministrationServiceTest.java
@@ -23,9 +23,11 @@ import org.apache.archiva.admin.model.beans.LegacyArtifactPath;
 import org.apache.archiva.admin.model.beans.OrganisationInformation;
 import org.apache.archiva.admin.model.beans.UiConfiguration;
 import org.apache.archiva.rest.api.model.AdminRepositoryConsumer;
+import org.apache.archiva.rest.api.services.ArchivaRestServiceException;
 import org.apache.commons.lang.StringUtils;
 import org.junit.Test;
 
+import javax.ws.rs.BadRequestException;
 import java.util.Arrays;
 import java.util.List;
 
@@ -93,6 +95,60 @@ public class ArchivaAdministrationServiceTest
     }
 
     @Test
+    public void badOrganizationLogoLocation()
+            throws Exception
+    {
+        OrganisationInformation organisationInformation =
+                getArchivaAdministrationService().getOrganisationInformation();
+
+        // rest return an empty bean
+        assertNotNull( organisationInformation );
+        assertTrue( StringUtils.isBlank( organisationInformation.getLogoLocation() ) );
+        assertTrue( StringUtils.isBlank( organisationInformation.getName() ) );
+        assertTrue( StringUtils.isBlank( organisationInformation.getUrl() ) );
+
+        organisationInformation = new OrganisationInformation();
+        organisationInformation.setLogoLocation( "http://foo.com'/><svg/onload=alert(/logoLocation_xss/)>" );
+        organisationInformation.setName( "foo org" );
+        organisationInformation.setUrl( "http://foo.com" );
+
+        try {
+            getArchivaAdministrationService().setOrganisationInformation(organisationInformation);
+            fail("RepositoryAdminException expected. Bad URL content should not be allowed for logo location.");
+        } catch (BadRequestException e) {
+            // OK
+        }
+
+    }
+
+    @Test
+    public void  badOrganizationUrl()
+            throws Exception
+    {
+        OrganisationInformation organisationInformation =
+                getArchivaAdministrationService().getOrganisationInformation();
+
+        // rest return an empty bean
+        assertNotNull( organisationInformation );
+        assertTrue( StringUtils.isBlank( organisationInformation.getLogoLocation() ) );
+        assertTrue( StringUtils.isBlank( organisationInformation.getName() ) );
+        assertTrue( StringUtils.isBlank( organisationInformation.getUrl() ) );
+
+        organisationInformation = new OrganisationInformation();
+        organisationInformation.setLogoLocation( "http://foo.com/logo.jpg" );
+        organisationInformation.setName( "foo org" );
+        organisationInformation.setUrl( "http://foo.com'/><svg/onload=alert(/url_xss/)>" );
+
+        try {
+            getArchivaAdministrationService().setOrganisationInformation(organisationInformation);
+            fail("RepositoryAdminException expected. Bad URL content should not be allowed for logo location.");
+        } catch (BadRequestException e) {
+            // OK
+        }
+
+    }
+
+    @Test
     public void uiConfigurationReadUpdate()
         throws Exception
     {