RoboHydra tests, URL sanitizing

* RoboHydra tests
* intelligent URL sanitizing (fixes #58, fixes #49, see issue #11, should fix #45)

src/at/bitfire/davdroid/URIUtils.java

@@ -3,21 +3,47 @@ package at.bitfire.davdroid;
 import java.net.URI;
 import java.net.URISyntaxException;
 
+import android.annotation.SuppressLint;
+import android.util.Log;
+
+@SuppressLint("DefaultLocale")
 public class URIUtils {
+	private static final String TAG = "davdroid.URIUtils";
+	
 	public static boolean isSame(URI a, URI b) {
 	     try {
-	    	a = new URI(a.getScheme(), null, a.getHost(), a.getPort(), a.getPath(), a.getQuery(), a.getFragment());
+	    	a = new URI(a.getScheme(), null, a.getHost(), a.getPort(), sanitize(a.getPath()), sanitize(a.getQuery()), null);
-	    	b = new URI(b.getScheme(), null, b.getHost(), b.getPort(), b.getPath(), b.getQuery(), b.getFragment());
+	    	b = new URI(b.getScheme(), null, b.getHost(), b.getPort(), sanitize(b.getPath()), sanitize(b.getQuery()), null);
 			return a.equals(b);
 		} catch (URISyntaxException e) {
 			return false;
 		}
 	}
 	
-	public static URI resolve(URI parent, String member) {
+	// handles invalid URLs/paths as good as possible
-		if (!member.startsWith("/") && !member.startsWith("http:") && !member.startsWith("https:"))
+	public static String sanitize(String original) {
-			member = "./" + member;
+		if (original == null)
+			return null;
 		
-		return parent.resolve(member);
+		String url = original;
+
+		// ":" is reserved as scheme/port separator, but we assume http:// and https:// URLs only
+		// and will encode ":" in URLs without one of these schemata
+		if (!url.toLowerCase().startsWith("http://") &&		// ":" is valid scheme separator
+			!url.toLowerCase().startsWith("https://") &&	// ":" is valid scheme separator
+			!url.startsWith("//"))							// ":" may be valid port separator
+			url = url.replace(":", "%3A");					// none of these -> ":" is not used for reserved purpose -> must be encoded 
+		
+		// rewrite reserved characters:
+		// 	"@" should be used for user name/password, but this case shouldn't appear in our URLs
+		// rewrite unsafe characters, too:
+		//	" ", "<", ">", """, "#", "{", "}", "|", "\", "^", "~", "["], "]", "`"
+		// do not rewrite "%" because we assume that URLs should be already encoded correctly
+		for (char c : new char[] { '@', ' ', '<', '>', '"', '#', '{', '}', '|', '\\', '^', '~', '[', ']', '`' })
+			url = url.replace(String.valueOf(c), "%" + Integer.toHexString(c));
+		
+		if (!url.equals(original))
+			Log.w(TAG, "Tried to repair invalid URL/URL path: " + original + " -> " + url);
+		return url;
 	}
 }

src/at/bitfire/davdroid/syncadapter/EnterCredentialsFragment.java

@@ -26,7 +26,9 @@ import android.widget.CheckBox;
 import android.widget.EditText;
 import android.widget.Spinner;
 import android.widget.TextView;
+import android.widget.Toast;
 import at.bitfire.davdroid.R;
+import at.bitfire.davdroid.URIUtils;
 
 public class EnterCredentialsFragment extends Fragment implements TextWatcher {
 	String protocol;
@@ -102,9 +104,10 @@ public class EnterCredentialsFragment extends Fragment implements TextWatcher {
 	void queryServer() {
 		FragmentTransaction ft = getFragmentManager().beginTransaction();
 		
-		String host_path = editBaseURL.getText().toString();
+		String host_path = URIUtils.sanitize(editBaseURL.getText().toString());
 		
 		Bundle args = new Bundle();
+		
 		args.putString(QueryServerDialogFragment.EXTRA_BASE_URL, protocol + host_path);
 		args.putString(QueryServerDialogFragment.EXTRA_USER_NAME, editUserName.getText().toString());
 		args.putString(QueryServerDialogFragment.EXTRA_PASSWORD, editPassword.getText().toString());
@@ -122,6 +125,7 @@ public class EnterCredentialsFragment extends Fragment implements TextWatcher {
 	public void onPrepareOptionsMenu(Menu menu) {
 		boolean ok =
 			editBaseURL.getText().length() > 0 &&
+			!editBaseURL.getText().toString().startsWith("/") &&		// host name required
 			editUserName.getText().length() > 0 &&
 			editPassword.getText().length() > 0;
 			

src/at/bitfire/davdroid/webdav/WebDavResource.java

@@ -128,7 +128,7 @@ public class WebDavResource {
 	}
 	
 	public WebDavResource(WebDavResource parent, String member) {
-		location = URIUtils.resolve(parent.location, member);
+		location = parent.location.resolve(URIUtils.sanitize(member));
 		client = parent.client;
 	}
 	
@@ -285,7 +285,7 @@ public class WebDavResource {
 		
 		multiget.hrefs = new ArrayList<DavHref>(names.length);
 		for (String name : names)
-			multiget.hrefs.add(new DavHref(URIUtils.resolve(location, name).getPath()));
+			multiget.hrefs.add(new DavHref(location.resolve(name).getPath()));
 		
 		Serializer serializer = new Persister();
 		StringWriter writer = new StringWriter();
@@ -371,7 +371,7 @@ public class WebDavResource {
 		for (DavResponse singleResponse : multistatus.response) {
 			URI href;
 			try {
-				href = URIUtils.resolve(location, singleResponse.getHref().href);
+				href = location.resolve(URIUtils.sanitize(singleResponse.getHref().href));
 			} catch(IllegalArgumentException ex) {
 				Log.w(TAG, "Ignoring illegal member URI in multi-status response", ex);
 				continue;

test/robohydra/davdroid.conf

@@ -1,5 +1,6 @@
 {"plugins":[
 	"assets",
 	"redirect",
-	"dav-default"
+	"dav-default",
+	"dav-invalid"
 ]}

test/robohydra/plugins/dav-invalid/index.js

@@ -12,7 +12,7 @@ exports.getBodyParts = function(conf) {
 						res.write('\<?xml version="1.0" encoding="utf-8" ?>\
 							<multistatus xmlns="DAV:">\
 								<response>\
-									<href>/dav/addressbooks/user@domain/My Contacts.vcf/</href>\
+									<href>/dav/addressbooks/user@domain/My Contacts:1.vcf/</href>\
 									<propstat>\
 										<prop xmlns:CARD="urn:ietf:params:xml:ns:carddav">\
 											<resourcetype>\
@@ -20,7 +20,22 @@ exports.getBodyParts = function(conf) {
 												<CARD:addressbook/>\
 											</resourcetype>\
 											<CARD:addressbook-description>\
-												Address Book with @ and space in URL\
+												Address Book with dubious characters in path\
+											</CARD:addressbook-description>\
+										</prop>\
+										<status>HTTP/1.1 200 OK</status>\
+									</propstat>\
+								</response>\
+								<response>\
+									<href>HTTPS://example.com/user@domain/absolute-url.vcf</href>\
+									<propstat>\
+										<prop xmlns:CARD="urn:ietf:params:xml:ns:carddav">\
+											<resourcetype>\
+												<collection/>\
+												<CARD:addressbook/>\
+											</resourcetype>\
+											<CARD:addressbook-description>\
+												Address Book with absolute URL\
 											</CARD:addressbook-description>\
 										</prop>\
 										<status>HTTP/1.1 200 OK</status>\

test/src/at/bitfire/davdroid/test/URIUtilsTest.java

@@ -22,18 +22,8 @@ public class URIUtilsTest extends InstrumentationTestCase {
 		assertTrue(URIUtils.isSame(new URI(ROOT_URI + "my@email/"), new URI(ROOT_URI + "my%40email/")));
 	}
 
-	public void testResolve() {
+	public void testSanitize() {
-		// resolve absolute URL
+		assertEquals("/my%40email.com/dir", URIUtils.sanitize("/my@email.com/dir"));
-		assertEquals(ROOT_URI + "file", URIUtils.resolve(baseURI, "/file").toString());
+		assertEquals("my%3Afile.vcf", URIUtils.sanitize("my:file.vcf"));
-		
-		// resolve relative URL (default case)
-		assertEquals(BASE_URI + "file", URIUtils.resolve(baseURI, "file").toString());
-		
-		// resolve relative URL with special characters
-		assertEquals(BASE_URI + "fi:le", URIUtils.resolve(baseURI, "fi:le").toString());
-		assertEquals(BASE_URI + "fi@le", URIUtils.resolve(baseURI, "fi@le").toString());
-		
-		// resolve URL with other schema
-		assertEquals("https://server", URIUtils.resolve(baseURI, "https://server").toString());
 	}
 }

test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java

@@ -3,6 +3,7 @@ package at.bitfire.davdroid.webdav.test;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.List;
 
 import org.apache.commons.io.IOUtils;
 import org.apache.http.HttpException;
@@ -23,7 +24,8 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 	
 	AssetManager assetMgr;
 	WebDavResource simpleFile,
-		davCollection, davNonExistingFile, davExistingFile;
+		davCollection, davNonExistingFile, davExistingFile,
+		davInvalid;
 
 	@Override
 	protected void setUp() throws Exception {
@@ -34,6 +36,8 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		davCollection = new WebDavResource(new URI(ROBOHYDRA_BASE + "dav"), true);
 		davNonExistingFile = new WebDavResource(davCollection, "collection/new.file");
 		davExistingFile = new WebDavResource(davCollection, "collection/existing.file");
+		
+		davInvalid = new WebDavResource(new URI(ROBOHYDRA_BASE + "dav-invalid"), true);
 	}
 	
 
@@ -117,7 +121,7 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 	}
 	
 	
-	/* test normal HTTP */
+	/* test normal HTTP/WebDAV */
 	
 	public void testDontFollowRedirections() throws URISyntaxException, IOException {
 		WebDavResource redirection = new WebDavResource(new URI(ROBOHYDRA_BASE + "redirect"), false);
@@ -175,4 +179,20 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		}
 		fail();
 	}
+	
+	
+	/* test CalDAV/CardDAV */
+	
+	
+	/* special test */
+	
+	public void testInvalidURLs() throws IOException, HttpException {
+		WebDavResource dav = new WebDavResource(davInvalid, "addressbooks/user%40domain/");
+		dav.propfind(HttpPropfind.Mode.MEMBERS_COLLECTIONS);
+		List<WebDavResource> members = dav.getMembers();
+		assertEquals(2, members.size());
+		assertEquals(ROBOHYDRA_BASE + "dav/addressbooks/user%40domain/My%20Contacts%3A1.vcf/", members.get(0).getLocation().toString());
+		assertEquals("HTTPS://example.com/user%40domain/absolute-url.vcf", members.get(1).getLocation().toString());
+	}
+	
 }