From da37ead215d001cc7468a86f823aa3285ea88172 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 24 Jul 2023 17:03:26 -0700 Subject: [PATCH] Civi::url() - Split $fragment and $fragmentQuery. Define clear/consistent rule for escaping inputs. --- Civi.php | 15 +- Civi/Core/Url.php | 214 +++++++++++++++++++++++----- tests/phpunit/Civi/Core/UrlTest.php | 58 +++++++- 3 files changed, 243 insertions(+), 44 deletions(-) diff --git a/Civi.php b/Civi.php index c7c48001d9..1a23839e58 100644 --- a/Civi.php +++ b/Civi.php @@ -219,12 +219,21 @@ class Civi { } /** - * Construct a URL based on a logical service address. + * Construct a URL based on a logical service address. URL building follows a few rules: * - * Ex: Link to constituent's dashboard (on frontend UI) + * 1. You should initialize with a baseline URL (e.g. 'frontend://civicrm/profile/view?id=123&gid=456'). + * 2. The scheme indicates the general type of URL ('frontend://', 'backend://', 'asset://', 'assetBuilder://'). + * 3. The URL object provides getters, setters, and adders (e.g. `getScheme()`, `setPath(...)`, `addQuery(...)`) + * 4. Strings are raw. Arrays are auto-encoded. (`addQuery('name=John+Doughnut')` or `addQuery(['name' => 'John Doughnut'])`) + * 5. You may use variable expressions (`id=[contact]&gid=[profile]`). + * 6. The URL can be cast to string (aka `__toString()`). + * + * Here are several examples: + * + * Ex: Link to constituent's dashboard (specifically on frontend UI) * $url = Civi::url('frontend://civicrm/user?reset=1'); * - * Ex: Link to constituent's dashboard (on frontend UI or backend UI -- whatever matches current page-view) + * Ex: Link to constituent's dashboard (on frontend UI or backend UI -- based on the active scheme of current page-view) * $url = Civi::url('//civicrm/user?reset=1'); * * Ex: Link to constituent's dashboard (with method calls - good for dynamic options) diff --git a/Civi/Core/Url.php b/Civi/Core/Url.php index 91b20d81ce..da014967ff 100644 --- a/Civi/Core/Url.php +++ b/Civi/Core/Url.php @@ -12,6 +12,25 @@ namespace Civi\Core; * The typical way to construct a URL object is through `Civi::url()`, which features more * documentation and examples. * + * This class-model has several properties. Most properties follow one of two patterns: + * + * - URL components (such as `path`, `query`, `fragment`, `fragmentQuery`). + * These have getter/setter/adder methods. They are stored as raw URL substrings. + * - Smart flags (such as `preferFormat`, `ssl`, `cacheCode`). + * These have getter/setter methods. They are stored as simple types (booleans or strings). + * They also have aliases via `__construct(...$flags)` and `useFlags($flags)` + * + * URI components (`path`, `query`, etc) can be understood as raw-strings or data-arrays. Compare: + * + * - "Path": "civicrm/foo+bar/whiz+bang" vs ['civicrm', 'foo bar', 'whiz bang'] + * - "Query: "a=100&b=Hello+world" vs ["a" => 100, "b" => "Hello world"] + * - "Fragment": "#/mailing/new" vs ["/mailing", "/new"] + * - "Fragment Query": "angularDebug=1" vs ["angularDebug" => 1] + * + * The raw-string is supported from all angles (storage+getters+setters+adders). + * Additionally, the setters+adders accept arrays. + * + * This cl * @see \Civi::url() */ final class Url { @@ -39,6 +58,11 @@ final class Url { */ private $fragment; + /** + * @var string|null + */ + private $fragmentQuery; + /** * Whether to auto-append the cache-busting resource code. * @@ -104,7 +128,9 @@ final class Url { $this->path .= $parsed['path']; } $this->query = $parsed['query'] ?? NULL; - $this->fragment = $parsed['fragment'] ?? NULL; + $fragmentParts = isset($parsed['fragment']) ? explode('?', $parsed['fragment'], 2) : []; + $this->fragment = $fragmentParts[0] ?? NULL; + $this->fragmentQuery = $fragmentParts[1] ?? NULL; if ($flags !== NULL) { $this->useFlags($flags); @@ -113,6 +139,7 @@ final class Url { /** * @return string + * Ex: 'frontend' or 'backend' */ public function getScheme() { return $this->scheme; @@ -120,6 +147,7 @@ final class Url { /** * @param string $scheme + * Ex: 'frontend' or 'backend' */ public function setScheme(string $scheme): Url { $this->scheme = $scheme; @@ -127,90 +155,159 @@ final class Url { } /** - * @return mixed + * @return string|null + * Ex: 'civicrm/event/info' + * Ex: 'civicrm/hello+world%3F' */ public function getPath() { return $this->path; } /** - * @param string $path + * @param string|string[]|null $path + * Ex: 'civicrm/event/info' + * Ex: 'civicrm/hello+world%3F' + * Ex: ['civicrm', 'hello world?'] */ - public function setPath(string $path): Url { - $this->path = $path; + public function setPath($path): Url { + $this->path = static::encodePath($path); return $this; } /** - * @param string|string[] $pathParts + * Add new sections to the path. + * + * When adding new parts to the path, there is an implicit delimiter ('/') between parts. + * + * @param string|string[] $path + * Ex: 'civicrm/event/info' + * Ex: 'civicrm/hello+world%3F' + * Ex: ['civicrm', 'hello world?'] * @return $this */ - public function addPath($pathParts): Url { - $suffix = implode('/', (array) $pathParts); - if ($this->path === NULL) { - $this->path = $suffix; - } - else { - $this->path = rtrim($this->path, '/') . '/' . $suffix; - } + public function addPath($path): Url { + static::appendString($this->path, '/', static::encodePath($path)); return $this; } /** * @return string|null + * Ex: 'name=John+Doughnut&id=9' */ public function getQuery(): ?string { return $this->query; } /** - * @param string|array|null $query + * @param string|string[]|null $query + * Ex: 'name=John+Doughnut&id=9' + * Ex: ['name' => 'John Doughnut', 'id' => 9] + * @return $this */ public function setQuery($query): Url { - $this->query = \CRM_Utils_System::makeQueryString($query); + if (is_array($query)) { + $query = \CRM_Utils_System::makeQueryString($query); + } + $this->query = $query; return $this; } /** - * @param string|array $query + * @param string|string[] $query + * Ex: 'name=John+Doughnut&id=9' + * Ex: ['name' => 'John Doughnut', 'id' => 9] * @return $this */ public function addQuery($query): Url { - if ($this->query === NULL) { - $this->query = \CRM_Utils_System::makeQueryString($query); - } - else { - $this->query .= '&' . \CRM_Utils_System::makeQueryString($query); + if (is_array($query)) { + $query = \CRM_Utils_System::makeQueryString($query); } + static::appendString($this->query, '&', $query); return $this; } /** + * Get the primary fragment. + * + * NOTE: This is the primary fragment identifier (as in `#id` or `#/client/side/route`). + * and does not include fregment queries. (as in '#?angularDebug=1'). + * * @return string|null + * Ex: '/mailing/new' + * Ex: '/foo+bar%3F/newish%3F' + * @see Url::getFragmentQuery() + * @see Url::composeFragment() */ public function getFragment(): ?string { return $this->fragment; } /** - * @param string|null $fragment + * Replace the fragment. + * + * NOTE: This is the primary fragment identifier (as in `#id` or `#/client/side/route`). + * and does not include fregment queries. (as in '#?angularDebug=1'). + * + * @param string|string[]|null $fragment + * Ex: '/mailing/new' + * Ex: '/foo+bar/newish%3F' + * Ex: ['', 'foo bar', 'newish?'] + * @return $this + * @see Url::setFragmentQuery() + * @see url::composeFragment() */ - public function setFragment(?string $fragment): Url { - $this->fragment = \CRM_Utils_System::makeQueryString($fragment); + public function setFragment($fragment): Url { + $this->fragment = static::encodePath($fragment); return $this; } /** - * @param string|array $fragment + * Add to fragment. + * + * @param string|string[] $fragment + * Ex: 'mailing/new' + * Ex: 'foo+bar/newish%3F' + * Ex: ['foo bar', 'newish?'] * @return $this */ public function addFragment($fragment): Url { - if ($this->fragment === NULL) { - $this->fragment = \CRM_Utils_System::makeQueryString($fragment); + static::appendString($this->fragment, '/', static::encodePath($fragment)); + return $this; + } + + /** + * @return string|null + * Ex: 'name=John+Doughnut&id=9' + */ + public function getFragmentQuery(): ?string { + return $this->fragmentQuery; + } + + /** + * @param string|string[]|null $fragmentQuery + * Ex: 'name=John+Doughnut&id=9' + * Ex: ['name' => 'John Doughnut', 'id' => 9] + * @return $this + */ + public function setFragmentQuery($fragmentQuery) { + if (is_array($fragmentQuery)) { + $fragmentQuery = \CRM_Utils_System::makeQueryString($fragmentQuery); } - else { - $this->fragment .= '&' . \CRM_Utils_System::makeQueryString($fragment); + $this->fragmentQuery = $fragmentQuery; + return $this; + } + + /** + * @param string|array $fragmentQuery + * Ex: 'name=John+Doughnut&id=9' + * Ex: ['name' => 'John Doughnut', 'id' => 9] + * @return $this + */ + public function addFragmentQuery($fragmentQuery): Url { + if (is_array($fragmentQuery)) { + $fragmentQuery = \CRM_Utils_System::makeQueryString($fragmentQuery); } + static::appendString($this->fragmentQuery, '&', $fragmentQuery); return $this; } @@ -402,11 +499,11 @@ final class Url { switch ($scheme) { case 'frontend': case 'service': - $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->getFragment(), TRUE, FALSE, FALSE); + $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->composeFragment(), TRUE, FALSE, FALSE); break; case 'backend': - $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->getFragment(), FALSE, TRUE, FALSE); + $result = $userSystem->url($this->getPath(), $this->getQuery(), $preferFormat === 'absolute', $this->composeFragment(), FALSE, TRUE, FALSE); break; case 'assetBuilder': @@ -465,15 +562,24 @@ final class Url { private function composeSuffix(): string { $result = ''; - if ($this->query) { + if ($this->query !== NULL && $this->query !== '') { $result .= '?' . $this->query; } - if ($this->fragment) { - $result .= '#' . $this->fragment; + $fragment = $this->composeFragment(); + if ($fragment !== NULL && $fragment !== '') { + $result .= '#' . $fragment; } return $result; } + private function composeFragment(): ?string { + $fragment = $this->fragment; + if ($this->fragmentQuery !== NULL && $this->fragmentQuery !== '') { + $fragment = ($fragment ?: '') . '?' . $this->fragmentQuery; + } + return $fragment; + } + private static function detectFormat(): string { // Some environments may override default - e.g. cv-cli prefers absolute URLs // WISHLIST: If handling `Job.*`, then 'absolute' @@ -502,4 +608,42 @@ final class Url { return \CRM_Core_Config::singleton()->userSystem->isFrontEndPage() ? 'frontend' : 'backend'; } + /** + * @param string|string[]|null $path + * Ex: 'greet/hello+world/en' + * Ex: ['greet', 'hello world', 'en'] + * @return string|null + * Ex: 'greet/hello+world/en' + */ + private static function encodePath($path): ?string { + if (is_array($path)) { + $encodedArray = array_map('urlencode', $path); + return implode('/', $encodedArray); + } + else { + return $path; + } + } + + private static function appendString(?string &$var, string $separator, ?string $value): void { + if ($value === NULL) { + return; + } + + if ($var === NULL) { + $var = $value; + return; + } + + // Dedupe separators + if (str_ends_with($var, $separator)) { + $var = rtrim($var, $separator); + } + if ($value[0] === $separator) { + $value = ltrim($value, $separator); + } + + $var = $var . $separator . $value; + } + } diff --git a/tests/phpunit/Civi/Core/UrlTest.php b/tests/phpunit/Civi/Core/UrlTest.php index 35c261f8af..2fdcc779d6 100644 --- a/tests/phpunit/Civi/Core/UrlTest.php +++ b/tests/phpunit/Civi/Core/UrlTest.php @@ -51,15 +51,17 @@ class UrlTest extends \CiviUnitTestCase { public function testPath() { $examples = []; $examples[] = ['civicrm/ajax/api4', Civi::url('service://civicrm/ajax/api4')]; - $examples[] = ['civicrm/ajax/api4/Contact/get', Civi::url('service://civicrm/ajax/api4')->addPath('Contact/get')]; - $examples[] = ['civicrm/ajax/api4/Contact/get', Civi::url('service://civicrm/ajax/api4')->addPath('Contact')->addPath('get')]; + $examples[] = ['civicrm/ajax/api4/Contact/get+stuff', Civi::url('service://civicrm/ajax/api4/Contact/get+stuff')]; + $examples[] = ['civicrm/ajax/api4/Contact/get+stuff', Civi::url('service://civicrm/ajax/api4')->addPath(['Contact', 'get stuff'])]; + $examples[] = ['civicrm/ajax/api4/Contact/get+stuff', Civi::url('service://civicrm/ajax/api4/Contact')->addPath('get+stuff')]; + $examples[] = ['civicrm/ajax/api4/Contact/get+stuff', Civi::url('service://civicrm/ajax/api4/Contact')->addPath(['get stuff'])]; $examples[] = ['civicrm/new-path', Civi::url('service://civicrm/old-path')->setPath('civicrm/new-path')]; foreach ($examples as $key => $example) { /** @var \Civi\Core\Url $url */ [$expected, $url] = $example; - $this->assertEquals($expected, $url->getPath()); - $this->assertStringContainsString($expected, (string) $url); + $this->assertEquals($expected, $url->getPath(), sprintf("%s at %d should be have matching property", __FUNCTION__, $key)); + $this->assertStringContainsString($expected, (string) $url, sprintf("%s at %d should be have matching output", __FUNCTION__, $key)); } } @@ -69,15 +71,59 @@ class UrlTest extends \CiviUnitTestCase { $examples[] = ['reset=1&id=9', Civi::url('frontend://civicrm/profile/view')->addQuery('reset=1&id=9')]; $examples[] = ['reset=1&id=9', Civi::url('frontend://civicrm/profile/view')->addQuery(['reset' => 1, 'id' => 9])]; $examples[] = ['noise=Hello+world%3F', Civi::url('frontend://civicrm/profile/view?noise=Hello+world%3F')]; + $examples[] = ['noise=Hello+world%3F', Civi::url('frontend://civicrm/profile/view')->addQuery('noise=Hello+world%3F')]; $examples[] = ['noise=Hello+world%3F', Civi::url('frontend://civicrm/profile/view')->addQuery(['noise' => 'Hello world?'])]; $examples[] = ['reset=1&id=9', Civi::url('frontend://civicrm/profile/view?forget=this')->setQuery('reset=1&id=9')]; + $examples[] = ['reset=1&id=9', Civi::url('frontend://civicrm/profile/view?forget=this')->setQuery(['reset' => 1, 'id' => 9])]; $examples[] = ['reset=1&id=9', Civi::url('frontend://civicrm/profile/view?forget=this')->setQuery('reset=1')->addQuery('id=9')]; foreach ($examples as $key => $example) { /** @var \Civi\Core\Url $url */ [$expected, $url] = $example; - $this->assertEquals($expected, $url->getQuery()); - $this->assertStringContainsString($expected, (string) $url); + $this->assertEquals($expected, $url->getQuery(), sprintf("%s at %d should be have matching property", __FUNCTION__, $key)); + $this->assertStringContainsString($expected, (string) $url, sprintf("%s at %d should be have matching output", __FUNCTION__, $key)); + } + } + + public function testFragment() { + $examples = []; + $examples[] = ['/mailing/new', Civi::url('frontend://civicrm/a/#/mailing/new')]; + $examples[] = ['/mailing/new', Civi::url('frontend://civicrm/a/#/')->addFragment('mailing/new')]; + $examples[] = ['/mailing/new', Civi::url('frontend://civicrm/a/#/')->addFragment('/mailing/new')]; + $examples[] = ['/mailing/new', Civi::url('frontend://civicrm/a/#/')->addFragment(['mailing', 'new'])]; + $examples[] = [NULL, Civi::url('frontend://civicrm/a/#/mailing/new')->setFragment(NULL)]; + $examples[] = ['/mailing/new+stuff', Civi::url('frontend://civicrm/a/#/mailing/new+stuff?extra=1')]; + $examples[] = ['/mailing/new+stuff', Civi::url('frontend://civicrm/a/#/mailing?extra=1')->addFragment('new+stuff')]; + $examples[] = ['/mailing/new+stuff', Civi::url('frontend://civicrm/a/#/mailing?extra=1')->addFragment(['new stuff'])]; + $examples[] = ['/mailing/new+stuff', Civi::url('frontend://civicrm/a/#/ignore?extra=1')->setFragment('/mailing/new+stuff')]; + $examples[] = ['/mailing/new+stuff', Civi::url('frontend://civicrm/a/#/ignore?extra=1')->setFragment(['', 'mailing', 'new stuff'])]; + + foreach ($examples as $key => $example) { + /** @var \Civi\Core\Url $url */ + [$expected, $url] = $example; + $this->assertEquals($expected, $url->getFragment(), sprintf("%s at %d should be have matching property", __FUNCTION__, $key)); + if ($expected !== NULL) { + $this->assertStringContainsString($expected, (string) $url, sprintf("%s at %d should be have matching output", __FUNCTION__, $key)); + } + } + } + + public function testFragmentQuery() { + $examples = []; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new?angularDebug=1&extra=hello+world%3F')]; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new?angularDebug=1')->addFragmentQuery('extra=hello+world%3F')]; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new')->addFragmentQuery('angularDebug=1&extra=hello+world%3F')]; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new')->addFragmentQuery(['angularDebug' => 1, 'extra' => 'hello world?'])]; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new')->setFragmentQuery('angularDebug=1&extra=hello+world%3F')]; + $examples[] = ['angularDebug=1&extra=hello+world%3F', Civi::url('frontend://civicrm/a/#/mailing/new')->setFragmentQuery(['angularDebug' => 1, 'extra' => 'hello world?'])]; + + foreach ($examples as $key => $example) { + /** @var \Civi\Core\Url $url */ + [$expected, $url] = $example; + $this->assertEquals($expected, $url->getFragmentQuery(), sprintf("%s at %d should be have matching property", __FUNCTION__, $key)); + if ($expected !== NULL) { + $this->assertStringContainsString($expected, (string) $url, sprintf("%s at %d should be have matching output", __FUNCTION__, $key)); + } } } -- 2.25.1