From 142e229ff36185c46f93089be6346fd183a82125 Mon Sep 17 00:00:00 2001 From: R Hirner Date: Fri, 26 Dec 2014 18:53:22 +0100 Subject: [PATCH] Handle relative URIs with ":" in path names correctly --- app/build.gradle | 3 -- app/proguard-rules.txt | 1 - .../at/bitfire/davdroid/URLUtilsTest.java | 26 ++++++++------ .../davdroid/webdav/WebDavResourceTest.java | 32 ++++++++--------- app/src/androidTest/robohydra/davdroid.conf | 3 +- .../robohydra/plugins/dav-invalid/index.js | 36 ------------------- .../java/at/bitfire/davdroid/URIUtils.java | 31 ++++++++++++---- .../davdroid/resource/RemoteCollection.java | 2 +- .../davdroid/webdav/DavRedirectStrategy.java | 6 ++-- .../davdroid/webdav/TlsSniSocketFactory.java | 2 +- .../davdroid/webdav/WebDavResource.java | 14 +++++--- 11 files changed, 72 insertions(+), 84 deletions(-) delete mode 100644 app/src/androidTest/robohydra/plugins/dav-invalid/index.js diff --git a/app/build.gradle b/app/build.gradle index 7531836b..20a40f2c 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -12,9 +12,6 @@ android { buildTypes { debug { - //minifyEnabled false - minifyEnabled true - proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.txt' } release { minifyEnabled true diff --git a/app/proguard-rules.txt b/app/proguard-rules.txt index 43c316c5..58f4c609 100644 --- a/app/proguard-rules.txt +++ b/app/proguard-rules.txt @@ -32,4 +32,3 @@ # DAVdroid -keep class at.bitfire.davdroid.** { *; } # all DAVdroid code is required - diff --git a/app/src/androidTest/java/at/bitfire/davdroid/URLUtilsTest.java b/app/src/androidTest/java/at/bitfire/davdroid/URLUtilsTest.java index 96bf8657..9c597271 100644 --- a/app/src/androidTest/java/at/bitfire/davdroid/URLUtilsTest.java +++ b/app/src/androidTest/java/at/bitfire/davdroid/URLUtilsTest.java @@ -35,7 +35,7 @@ public class URLUtilsTest extends TestCase { assertEquals("/test/", URIUtils.ensureTrailingSlash("/test")); assertEquals("/test/", URIUtils.ensureTrailingSlash("/test/")); - String withoutSlash = "http://www.test.at/dav/collection", + String withoutSlash = "http://www.test.example/dav/collection", withSlash = withoutSlash + "/"; assertEquals(new URI(withSlash), URIUtils.ensureTrailingSlash(new URI(withoutSlash))); assertEquals(new URI(withSlash), URIUtils.ensureTrailingSlash(new URI(withSlash))); @@ -44,18 +44,24 @@ public class URLUtilsTest extends TestCase { public void testParseURI() throws Exception { // don't escape valid characters String validPath = "/;:@&=$-_.+!*'(),"; - assertEquals(new URI("https://www.test.at:123" + validPath), URIUtils.parseURI("https://www.test.at:123" + validPath)); - assertEquals(new URI(validPath), URIUtils.parseURI(validPath)); + assertEquals(new URI("https://www.test.example:123" + validPath), URIUtils.parseURI("https://www.test.example:123" + validPath, false)); + assertEquals(new URI(validPath), URIUtils.parseURI(validPath, true)); // keep literal IPv6 addresses (only in host name) - assertEquals(new URI("https://[1:2::1]/"), URIUtils.parseURI("https://[1:2::1]/")); + assertEquals(new URI("https://[1:2::1]/"), URIUtils.parseURI("https://[1:2::1]/", false)); - // ~ as home directory - assertEquals(new URI("http://www.test.at/~user1/"), URIUtils.parseURI("http://www.test.at/~user1/")); - assertEquals(new URI("http://www.test.at/~user1/"), URIUtils.parseURI("http://www.test.at/%7euser1/")); + // "~" as home directory + assertEquals(new URI("http://www.test.example/~user1/"), URIUtils.parseURI("http://www.test.example/~user1/", false)); + assertEquals(new URI("/~user1/"), URIUtils.parseURI("/%7euser1/", true)); - // @ in directory name - assertEquals(new URI("http://www.test.at/user@server.com/"), URIUtils.parseURI("http://www.test.at/user@server.com/")); - assertEquals(new URI("http://www.test.at/user@server.com/"), URIUtils.parseURI("http://www.test.at/user%40server.com/")); + // "@" in directory name + assertEquals(new URI("http://www.test.example/user@server.com/"), URIUtils.parseURI("http://www.test.example/user@server.com/", false)); + assertEquals(new URI("/user@server.com/"), URIUtils.parseURI("/user%40server.com/", true)); + assertEquals(new URI("user@server.com"), URIUtils.parseURI("user%40server.com", true)); + + // ":" in path names + assertEquals(new URI("http://www.test.example/my:cal.ics"), URIUtils.parseURI("http://www.test.example/my:cal.ics", false)); + assertEquals(new URI("/my:cal.ics"), URIUtils.parseURI("/my%3Acal.ics", true)); + assertEquals(new URI(null, null, "my:cal.ics", null, null), URIUtils.parseURI("my%3Acal.ics", true)); } } diff --git a/app/src/androidTest/java/at/bitfire/davdroid/webdav/WebDavResourceTest.java b/app/src/androidTest/java/at/bitfire/davdroid/webdav/WebDavResourceTest.java index f49db332..0dc9321c 100644 --- a/app/src/androidTest/java/at/bitfire/davdroid/webdav/WebDavResourceTest.java +++ b/app/src/androidTest/java/at/bitfire/davdroid/webdav/WebDavResourceTest.java @@ -36,9 +36,8 @@ public class WebDavResourceTest extends InstrumentationTestCase { CloseableHttpClient httpClient; WebDavResource baseDAV; - WebDavResource simpleFile, - davCollection, davNonExistingFile, davExistingFile, - davInvalid; + WebDavResource davAssets, + davCollection, davNonExistingFile, davExistingFile; @Override protected void setUp() throws Exception { @@ -47,14 +46,11 @@ public class WebDavResourceTest extends InstrumentationTestCase { assetMgr = getInstrumentation().getContext().getResources().getAssets(); baseDAV = new WebDavResource(httpClient, TestConstants.roboHydra.resolve("/dav/")); - - simpleFile = new WebDavResource(httpClient, TestConstants.roboHydra.resolve("assets/test.random")); - + davAssets = new WebDavResource(httpClient, TestConstants.roboHydra.resolve("assets/")); davCollection = new WebDavResource(httpClient, TestConstants.roboHydra.resolve("dav/")); + davNonExistingFile = new WebDavResource(davCollection, "collection/new.file"); davExistingFile = new WebDavResource(davCollection, "collection/existing.file"); - - davInvalid = new WebDavResource(httpClient, TestConstants.roboHydra.resolve("dav-invalid/")); } @Override @@ -80,7 +76,8 @@ public class WebDavResourceTest extends InstrumentationTestCase { public void testPropfindCurrentUserPrincipal() throws Exception { davCollection.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL); assertEquals("/dav/principals/users/test", davCollection.getCurrentUserPrincipal()); - + + WebDavResource simpleFile = new WebDavResource(davAssets, "test.random"); try { simpleFile.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL); fail(); @@ -152,6 +149,7 @@ public class WebDavResourceTest extends InstrumentationTestCase { } public void testGet() throws Exception { + WebDavResource simpleFile = new WebDavResource(davAssets, "test.random"); simpleFile.get("*/*"); @Cleanup InputStream is = assetMgr.open("test.random", AssetManager.ACCESS_STREAMING); byte[] expected = IOUtils.toByteArray(is); @@ -163,7 +161,7 @@ public class WebDavResourceTest extends InstrumentationTestCase { boolean sniWorking = false; try { - file.get("*/*"); + file.get("* /*"); sniWorking = true; } catch (SSLPeerUnverifiedException e) { } @@ -222,12 +220,14 @@ public class WebDavResourceTest extends InstrumentationTestCase { /* special test */ - public void testInvalidURLs() throws Exception { - WebDavResource dav = new WebDavResource(davInvalid, "addressbooks/%7euser1/"); - dav.propfind(HttpPropfind.Mode.CARDDAV_COLLECTIONS); - List members = dav.getMembers(); - assertEquals(1, members.size()); - assertEquals(TestConstants.ROBOHYDRA_BASE + "dav-invalid/addressbooks/~user1/My%20Contacts:1.vcf/", members.get(0).getLocation().toASCIIString()); + public void testGetSpecialURLs() throws Exception { + WebDavResource dav = new WebDavResource(davAssets, "member-with:colon.vcf"); + try { + dav.get("*/*"); + fail(); + } catch(NotFoundException e) { + assertTrue(true); + } } } diff --git a/app/src/androidTest/robohydra/davdroid.conf b/app/src/androidTest/robohydra/davdroid.conf index 37807fd7..8e7823e2 100644 --- a/app/src/androidTest/robohydra/davdroid.conf +++ b/app/src/androidTest/robohydra/davdroid.conf @@ -1,6 +1,5 @@ {"plugins":[ "assets", "redirect", - "dav", - "dav-invalid" + "dav" ]} diff --git a/app/src/androidTest/robohydra/plugins/dav-invalid/index.js b/app/src/androidTest/robohydra/plugins/dav-invalid/index.js deleted file mode 100644 index 5fd1c3fc..00000000 --- a/app/src/androidTest/robohydra/plugins/dav-invalid/index.js +++ /dev/null @@ -1,36 +0,0 @@ -var roboHydraHeadDAV = require("../headdav"); - -exports.getBodyParts = function(conf) { - return { - heads: [ - /* address-book home set */ - new RoboHydraHeadDAV({ - path: "/dav-invalid/addressbooks/~user1/", - handler: function(req,res,next) { - if (req.method == "PROPFIND" && req.rawBody.toString().match(/addressbook-description/)) { - res.statusCode = 207; - res.write('\\ - \ - \ - /dav-invalid/addressbooks/~user1/My%20Contacts:1.vcf/\ - \ - \ - \ - \ - \ - \ - \ - Address Book with dubious characters in path\ - \ - \ - HTTP/1.1 200 OK\ - \ - \ - \ - '); - } - } - }) - ] - }; -}; diff --git a/app/src/main/java/at/bitfire/davdroid/URIUtils.java b/app/src/main/java/at/bitfire/davdroid/URIUtils.java index 509bb582..4adf5686 100644 --- a/app/src/main/java/at/bitfire/davdroid/URIUtils.java +++ b/app/src/main/java/at/bitfire/davdroid/URIUtils.java @@ -39,15 +39,32 @@ public class URIUtils { /** * Parse a received absolute/relative URL and generate a normalized URI that can be compared. - * @param original URI to be parsed, may be absolute or relative - * @return normalized URI + * @param original URI to be parsed, may be absolute or relative + * @param mustBePath true if it's known that original is a path (may contain ":") and not an URI, i.e. ":" is not the scheme separator + * @return normalized URI * @throws URISyntaxException */ - public static URI parseURI(String original) throws URISyntaxException { - URI raw = URI.create(original); - URI uri = new URI(raw.getScheme(), raw.getAuthority(), raw.getPath(), raw.getQuery(), raw.getFragment()); - Log.v(TAG, "Normalized URL " + original + " -> " + uri.toASCIIString()); - return uri; + public static URI parseURI(String original, boolean mustBePath) throws URISyntaxException { + if (mustBePath) { + // may contain ":" + // case 1: "my:file" won't be parsed by URI correctly because it would consider "my" as URI scheme + // case 2: "path/my:file" will be parsed by URI correctly + // case 3: "my:path/file" won't be parsed by URI correctly because it would consider "my" as URI scheme + int idxSlash = original.indexOf('/'), + idxColon = original.indexOf(':'); + if (idxColon != -1) { + // colon present + if ((idxSlash != -1) && idxSlash < idxColon) // There's a slash, and it's before the colon → everything OK + ; + else // No slash before the colon; we have to put it there + original = "./" + original; + } + } + + URI uri = new URI(original); + URI normalized = new URI(uri.getScheme(), uri.getAuthority(), uri.getPath(), uri.getQuery(), uri.getFragment()); + Log.v(TAG, "Normalized URL " + original + " -> " + normalized.toASCIIString()); + return normalized; } } diff --git a/app/src/main/java/at/bitfire/davdroid/resource/RemoteCollection.java b/app/src/main/java/at/bitfire/davdroid/resource/RemoteCollection.java index 1a2fb414..2682e606 100644 --- a/app/src/main/java/at/bitfire/davdroid/resource/RemoteCollection.java +++ b/app/src/main/java/at/bitfire/davdroid/resource/RemoteCollection.java @@ -52,7 +52,7 @@ public abstract class RemoteCollection { public RemoteCollection(CloseableHttpClient httpClient, String baseURL, String user, String password, boolean preemptiveAuth) throws URISyntaxException { this.httpClient = httpClient; - collection = new WebDavResource(httpClient, URIUtils.parseURI(baseURL), user, password, preemptiveAuth); + collection = new WebDavResource(httpClient, URIUtils.parseURI(baseURL, false), user, password, preemptiveAuth); } diff --git a/app/src/main/java/at/bitfire/davdroid/webdav/DavRedirectStrategy.java b/app/src/main/java/at/bitfire/davdroid/webdav/DavRedirectStrategy.java index 9bdda8da..124ee644 100644 --- a/app/src/main/java/at/bitfire/davdroid/webdav/DavRedirectStrategy.java +++ b/app/src/main/java/at/bitfire/davdroid/webdav/DavRedirectStrategy.java @@ -31,7 +31,7 @@ import at.bitfire.davdroid.URIUtils; */ public class DavRedirectStrategy implements RedirectStrategy { private final static String TAG = "davdroid.DavRedirectStrategy"; - final static DavRedirectStrategy INSTANCE = new DavRedirectStrategy(); + public final static DavRedirectStrategy INSTANCE = new DavRedirectStrategy(); protected final static String REDIRECTABLE_METHODS[] = { "OPTIONS", "GET", "PUT", "DELETE" @@ -82,12 +82,12 @@ public class DavRedirectStrategy implements RedirectStrategy { return null; } try { - URI location = URIUtils.parseURI(locationHdr.getValue()); + URI location = URIUtils.parseURI(locationHdr.getValue(), false); // some servers don't return absolute URLs as required by RFC 2616 if (!location.isAbsolute()) { Log.w(TAG, "Received invalid redirection to relative URL, repairing"); - URI originalURI = URIUtils.parseURI(request.getRequestLine().getUri()); + URI originalURI = URIUtils.parseURI(request.getRequestLine().getUri(), false); if (!originalURI.isAbsolute()) { final HttpHost target = HttpClientContext.adapt(context).getTargetHost(); if (target != null) diff --git a/app/src/main/java/at/bitfire/davdroid/webdav/TlsSniSocketFactory.java b/app/src/main/java/at/bitfire/davdroid/webdav/TlsSniSocketFactory.java index e2addbc5..99af712f 100644 --- a/app/src/main/java/at/bitfire/davdroid/webdav/TlsSniSocketFactory.java +++ b/app/src/main/java/at/bitfire/davdroid/webdav/TlsSniSocketFactory.java @@ -36,7 +36,7 @@ import javax.net.ssl.SSLSocket; public class TlsSniSocketFactory implements LayeredConnectionSocketFactory { private static final String TAG = "davdroid.SNISocketFactory"; - final static TlsSniSocketFactory INSTANCE = new TlsSniSocketFactory(); + public final static TlsSniSocketFactory INSTANCE = new TlsSniSocketFactory(); private final static SSLCertificateSocketFactory sslSocketFactory = (SSLCertificateSocketFactory)SSLCertificateSocketFactory.getDefault(0); diff --git a/app/src/main/java/at/bitfire/davdroid/webdav/WebDavResource.java b/app/src/main/java/at/bitfire/davdroid/webdav/WebDavResource.java index e4818986..0c1fddc0 100644 --- a/app/src/main/java/at/bitfire/davdroid/webdav/WebDavResource.java +++ b/app/src/main/java/at/bitfire/davdroid/webdav/WebDavResource.java @@ -123,7 +123,7 @@ public class WebDavResource { } } - WebDavResource(WebDavResource parent) { // copy constructor: based on existing WebDavResource, reuse settings + public WebDavResource(WebDavResource parent) { // copy constructor: based on existing WebDavResource, reuse settings httpClient = parent.httpClient; context = parent.context; location = parent.location; @@ -133,10 +133,16 @@ public class WebDavResource { this(parent); location = parent.location.resolve(url); } - + + /** + * Creates a WebDavResource representing a member of the parent collection. + * @param parent Parent collection + * @param member File name of the member. This may contain ":" without leading "./"! + * @throws URISyntaxException + */ public WebDavResource(WebDavResource parent, String member) throws URISyntaxException { this(parent); - location = parent.location.resolve(URIUtils.parseURI(member)); + location = parent.location.resolve(URIUtils.parseURI(member, true)); } public WebDavResource(WebDavResource parent, String member, String ETag) throws URISyntaxException { @@ -454,7 +460,7 @@ public class WebDavResource { for (DavResponse singleResponse : multiStatus.response) { URI href; try { - href = location.resolve(URIUtils.parseURI(singleResponse.getHref().href)); + href = location.resolve(URIUtils.parseURI(singleResponse.getHref().href, false)); } catch(Exception ex) { Log.w(TAG, "Ignoring illegal member URI in multi-status response", ex); continue;