Merge pull request #17008 from owncloud/fix-17006

Improve splitting of username and remote adress when username contains an `@`

7 files changed, 152 insertions, 49 deletions

diff --git a/apps/files_sharing/api/local.php b/apps/files_sharing/api/local.php
index 03812800c17..2d848f8b16e 100644
--- a/apps/files_sharing/api/local.php
+++ b/apps/files_sharing/api/local.php
@@ -27,6 +27,8 @@
 
 namespace OCA\Files_Sharing\API;
 
+use OC\HintException;
+
 class Local {
 
 	/**
@@ -294,6 +296,8 @@ class Local {
 					$shareWith,
 					$permissions
 					);
+		} catch (HintException $e) {
+			return new \OC_OCS_Result(null, 400, $e->getHint());
 		} catch (\Exception $e) {
 			return new \OC_OCS_Result(null, 403, $e->getMessage());
 		}
diff --git a/core/ajax/share.php b/core/ajax/share.php
index 22f483ec0e1..fa75f37587f 100644
--- a/core/ajax/share.php
+++ b/core/ajax/share.php
@@ -66,6 +66,8 @@ if (isset($_POST['action']) && isset($_POST['itemType']) && isset($_POST['itemSo
 					} else {
 						OC_JSON::success();
 					}
+				} catch (\OC\HintException $exception) {
+					OC_JSON::error(array('data' => array('message' => $exception->getHint())));
 				} catch (Exception $exception) {
 					OC_JSON::error(array('data' => array('message' => $exception->getMessage())));
 				}
diff --git a/lib/private/share/helper.php b/lib/private/share/helper.php
index 65167dd7549..1988324a996 100644
--- a/lib/private/share/helper.php
+++ b/lib/private/share/helper.php
@@ -27,6 +27,8 @@
 
 namespace OC\Share;
 
+use OC\HintException;
+
 class Helper extends \OC\Share\Constants {
 
 	/**
@@ -216,32 +218,74 @@ class Helper extends \OC\Share\Constants {
 	}
 
 	/**
-	 * Extracts the necessary remote name from a given link
+	 * Strips away a potential file names and trailing slashes:
+	 * - http://localhost
+	 * - http://localhost/
+	 * - http://localhost/index.php
+	 * - http://localhost/index.php/s/{shareToken}
 	 *
-	 * Strips away a potential file name, to allow
-	 * - user
-	 * - user@localhost
-	 * - user@http://localhost
-	 * - user@http://localhost/
-	 * - user@http://localhost/index.php
-	 * - user@http://localhost/index.php/s/{shareToken}
+	 * all return: http://localhost
 	 *
 	 * @param string $shareWith
 	 * @return string
 	 */
-	public static function fixRemoteURLInShareWith($shareWith) {
-		if (strpos($shareWith, '@')) {
-			list($user, $remote) = explode('@', $shareWith, 2);
+	protected static function fixRemoteURL($remote) {
+		$remote = str_replace('\\', '/', $remote);
+		if ($fileNamePosition = strpos($remote, '/index.php')) {
+			$remote = substr($remote, 0, $fileNamePosition);
+		}
+		$remote = rtrim($remote, '/');
 
-			$remote = str_replace('\\', '/', $remote);
-			if ($fileNamePosition = strpos($remote, '/index.php')) {
-				$remote = substr($remote, 0, $fileNamePosition);
-			}
-			$remote = rtrim($remote, '/');
+		return $remote;
+	}
+
+	/**
+	 * split user and remote from federated cloud id
+	 *
+	 * @param string $id
+	 * @return array
+	 * @throws HintException
+	 */
+	public static function splitUserRemote($id) {
+		if (strpos($id, '@') === false) {
+			$l = \OC::$server->getL10N('core');
+			$hint = $l->t('Invalid Federated Cloud ID');
+			throw new HintException('Invalid Federated Cloud ID', $hint);
+		}
+
+		// Find the first character that is not allowed in user names
+		$id = str_replace('\\', '/', $id);
+		$posSlash = strpos($id, '/');
+		$posColon = strpos($id, ':');
+
+		if ($posSlash === false && $posColon === false) {
+			$invalidPos = strlen($id);
+		} else if ($posSlash === false) {
+			$invalidPos = $posColon;
+		} else if ($posColon === false) {
+			$invalidPos = $posSlash;
+		} else {
+			$invalidPos = min($posSlash, $posColon);
+		}
+
+		// Find the last @ before $invalidPos
+		$pos = $lastAtPos = 0;
+		while ($lastAtPos !== false && $lastAtPos <= $invalidPos) {
+			$pos = $lastAtPos;
+			$lastAtPos = strpos($id, '@', $pos + 1);
+		}
 
-			$shareWith = $user . '@' . $remote;
+		if ($pos !== false) {
+			$user = substr($id, 0, $pos);
+			$remote = substr($id, $pos + 1);
+			$remote = self::fixRemoteURL($remote);
+			if (!empty($user) && !empty($remote)) {
+				return array($user, $remote);
+			}
 		}
 
-		return rtrim($shareWith, '/');
+		$l = \OC::$server->getL10N('core');
+		$hint = $l->t('Invalid Federated Cloud ID');
+		throw new HintException('Invalid Fededrated Cloud ID', $hint);
 	}
 }
diff --git a/lib/private/share/share.php b/lib/private/share/share.php
index b2ac9ee6a42..fd24fc686b1 100644
--- a/lib/private/share/share.php
+++ b/lib/private/share/share.php
@@ -554,6 +554,7 @@ class Share extends Constants {
 	 * @param string $itemSourceName
 	 * @param \DateTime $expirationDate
 	 * @return boolean|string Returns true on success or false on failure, Returns token on success for links
+	 * @throws \OC\HintException when the share type is remote and the shareWith is invalid
 	 * @throws \Exception
 	 */
 	public static function shareItem($itemType, $itemSource, $shareType, $shareWith, $permissions, $itemSourceName = null, \DateTime $expirationDate = null) {
@@ -749,7 +750,8 @@ class Share extends Constants {
 			$token = \OC::$server->getSecureRandom()->getMediumStrengthGenerator()->generate(self::TOKEN_LENGTH, \OCP\Security\ISecureRandom::CHAR_LOWER . \OCP\Security\ISecureRandom::CHAR_UPPER .
 				\OCP\Security\ISecureRandom::CHAR_DIGITS);
 
-			$shareWith = Helper::fixRemoteURLInShareWith($shareWith);
+			list($user, $remote) = Helper::splitUserRemote($shareWith);
+			$shareWith = $user . '@' . $remote;
 			$shareId = self::put($itemType, $itemSource, $shareType, $shareWith, $uidOwner, $permissions, null, $token, $itemSourceName);
 
 			$send = false;
@@ -1300,8 +1302,8 @@ class Share extends Constants {
 		$hookParams['deletedShares'] = $deletedShares;
 		\OC_Hook::emit('OCP\Share', 'post_unshare', $hookParams);
 		if ((int)$item['share_type'] === \OCP\Share::SHARE_TYPE_REMOTE && \OC::$server->getUserSession()->getUser()) {
-			$urlParts = explode('@', $item['share_with'], 2);
-			self::sendRemoteUnshare($urlParts[1], $item['id'], $item['token']);
+			list(, $remote) = Helper::splitUserRemote($item['share_with']);
+			self::sendRemoteUnshare($remote, $item['id'], $item['token']);
 		}
 	}
 
@@ -2436,10 +2438,10 @@ class Share extends Constants {
 	 */
 	private static function sendRemoteShare($token, $shareWith, $name, $remote_id, $owner) {
 
-		list($user, $remote) = explode('@', $shareWith, 2);
+		list($user, $remote) = Helper::splitUserRemote($shareWith);
 
 		if ($user && $remote) {
-			$url = rtrim($remote, '/') . self::BASE_PATH_TO_SHARE_API . '?format=' . self::RESPONSE_FORMAT;
+			$url = $remote . self::BASE_PATH_TO_SHARE_API . '?format=' . self::RESPONSE_FORMAT;
 
 			$local = \OC::$server->getURLGenerator()->getAbsoluteURL('/');
 
diff --git a/lib/public/share.php b/lib/public/share.php
index 6c8b82f4293..797972bc1cb 100644
--- a/lib/public/share.php
+++ b/lib/public/share.php
@@ -255,6 +255,7 @@ class Share extends \OC\Share\Constants {
 	 * @param string $itemSourceName
 	 * @param \DateTime $expirationDate
 	 * @return bool|string Returns true on success or false on failure, Returns token on success for links
+	 * @throws \OC\HintException when the share type is remote and the shareWith is invalid
 	 * @throws \Exception
 	 * @since 5.0.0 - parameter $itemSourceName was added in 6.0.0, parameter $expirationDate was added in 7.0.0
 	 */
diff --git a/tests/lib/share/helper.php b/tests/lib/share/helper.php
index 0385263fd91..e37a3db8bf0 100644
--- a/tests/lib/share/helper.php
+++ b/tests/lib/share/helper.php
@@ -50,44 +50,31 @@ class Test_Share_Helper extends \Test\TestCase {
 		$this->assertSame($expected, $result);
 	}
 
-	public function fixRemoteURLInShareWithData() {
-		$userPrefix = ['test@', 'na/me@'];
+	public function dataTestSplitUserRemote() {
+		$userPrefix = ['user@name', 'username'];
 		$protocols = ['', 'http://', 'https://'];
 		$remotes = [
 			'localhost',
-			'test:foobar@localhost',
 			'local.host',
 			'dev.local.host',
 			'dev.local.host/path',
+			'dev.local.host/at@inpath',
 			'127.0.0.1',
 			'::1',
 			'::192.0.2.128',
+			'::192.0.2.128/at@inpath',
 		];
 
-		$testCases = [
-			['test', 'test'],
-			['na/me', 'na/me'],
-			['na/me/', 'na/me'],
-			['na/index.php', 'na/index.php'],
-			['http://localhost', 'http://localhost'],
-			['http://localhost/', 'http://localhost'],
-			['http://localhost/index.php', 'http://localhost/index.php'],
-			['http://localhost/index.php/s/token', 'http://localhost/index.php/s/token'],
-			['http://test:foobar@localhost', 'http://test:foobar@localhost'],
-			['http://test:foobar@localhost/', 'http://test:foobar@localhost'],
-			['http://test:foobar@localhost/index.php', 'http://test:foobar@localhost'],
-			['http://test:foobar@localhost/index.php/s/token', 'http://test:foobar@localhost'],
-		];
-
+		$testCases = [];
 		foreach ($userPrefix as $user) {
 			foreach ($remotes as $remote) {
 				foreach ($protocols as $protocol) {
-					$baseUrl = $user . $protocol . $remote;
+					$baseUrl = $user . '@' . $protocol . $remote;
 
-					$testCases[] = [$baseUrl, $baseUrl];
-					$testCases[] = [$baseUrl . '/', $baseUrl];
-					$testCases[] = [$baseUrl . '/index.php', $baseUrl];
-					$testCases[] = [$baseUrl . '/index.php/s/token', $baseUrl];
+					$testCases[] = [$baseUrl, $user, $protocol . $remote];
+					$testCases[] = [$baseUrl . '/', $user, $protocol . $remote];
+					$testCases[] = [$baseUrl . '/index.php', $user, $protocol . $remote];
+					$testCases[] = [$baseUrl . '/index.php/s/token', $user, $protocol . $remote];
 				}
 			}
 		}
@@ -95,9 +82,43 @@ class Test_Share_Helper extends \Test\TestCase {
 	}
 
 	/**
-	 * @dataProvider fixRemoteURLInShareWithData
+	 * @dataProvider dataTestSplitUserRemote
+	 *
+	 * @param string $remote
+	 * @param string $expectedUser
+	 * @param string $expectedUrl
+	 */
+	public function testSplitUserRemote($remote, $expectedUser, $expectedUrl) {
+		list($remoteUser, $remoteUrl) = \OC\Share\Helper::splitUserRemote($remote);
+		$this->assertSame($expectedUser, $remoteUser);
+		$this->assertSame($expectedUrl, $remoteUrl);
+	}
+
+	public function dataTestSplitUserRemoteError() {
+		return array(
+			// Invalid path
+			array('user@'),
+
+			// Invalid user
+			array('@server'),
+			array('us/er@server'),
+			array('us:er@server'),
+
+			// Invalid splitting
+			array('user'),
+			array(''),
+			array('us/erserver'),
+			array('us:erserver'),
+		);
+	}
+
+	/**
+	 * @dataProvider dataTestSplitUserRemoteError
+	 *
+	 * @param string $id
+	 * @expectedException \OC\HintException
 	 */
-	public function testFixRemoteURLInShareWith($remote, $expected) {
-		$this->assertSame($expected, \OC\Share\Helper::fixRemoteURLInShareWith($remote));
+	public function testSplitUserRemoteError($id) {
+		\OC\Share\Helper::splitUserRemote($id);
 	}
 }
diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php
index f03ed43e7fc..92f6b612688 100644
--- a/tests/lib/share/share.php
+++ b/tests/lib/share/share.php
@@ -968,6 +968,35 @@ class Test_Share extends \Test\TestCase {
 
 	}
 
+	public function dataShareWithRemoteUserAndRemoteIsInvalid() {
+		return [
+			// Invalid path
+			array('user@'),
+
+			// Invalid user
+			array('@server'),
+			array('us/er@server'),
+			array('us:er@server'),
+
+			// Invalid splitting
+			array('user'),
+			array(''),
+			array('us/erserver'),
+			array('us:erserver'),
+		];
+	}
+
+	/**
+	 * @dataProvider dataShareWithRemoteUserAndRemoteIsInvalid
+	 *
+	 * @param string $remoteId
+	 * @expectedException \OC\HintException
+	 */
+	public function testShareWithRemoteUserAndRemoteIsInvalid($remoteId) {
+		OC_User::setUserId($this->user1);
+		OCP\Share::shareItem('test', 'test.txt', OCP\Share::SHARE_TYPE_REMOTE, $remoteId, \OCP\Constants::PERMISSION_ALL);
+	}
+
 	public function testUnshareAll() {
 		$this->shareUserTestFileWithUser($this->user1, $this->user2);
 		$this->shareUserTestFileWithUser($this->user2, $this->user3);