Merge pull request #14534 from jdalsem/6.0feats

feat(responses): response forward urls are now secure by default

engine/classes/Elgg/Http/RedirectResponse.php

@@ -10,12 +10,15 @@ class RedirectResponse extends Response {
 	/**
 	 * Constructor
 	 *
-	 * @param string $forward_url Forward url
-	 * @param int    $status_code HTTP status code
+	 * @param string $forward_url        Forward url
+	 * @param int    $status_code        HTTP status code
+	 * @param bool   $secure_forward_url If true the forward url will be validated to be an on-site url
 	 *
 	 * @see elgg_redirect_response()
 	 */
-	public function __construct(string $forward_url = REFERRER, int $status_code = ELGG_HTTP_FOUND) {
+	public function __construct(string $forward_url = REFERRER, int $status_code = ELGG_HTTP_FOUND, bool $secure_forward_url = true) {
+		$this->secure_forward_url = $secure_forward_url;
+
 		$this->setForwardURL($forward_url);
 		$this->setStatusCode($status_code);
 	}

engine/classes/Elgg/Http/Response.php

@@ -13,30 +13,17 @@
  */
 abstract class Response implements ResponseBuilder {
 
-	/**
-	 * @var string
-	 */
 	protected $content;
 
-	/**
-	 * @var int
-	 */
-	protected $status_code;
+	protected int $status_code;
 
-	/**
-	 * @var string
-	 */
-	protected $forward_url;
+	protected ?string $forward_url = null;
 
-	/**
-	 * @var array
-	 */
-	protected $headers;
+	protected array $headers = [];
 
-	/**
-	 * @var \Exception
-	 */
-	protected $exception;
+	protected ?\Exception $exception = null;
+
+	protected bool $secure_forward_url = true;
 
 	/**
 	 * {@inheritdoc}
@@ -58,15 +45,15 @@ public function getContent() {
 	}
 
 	/**
-	 * {@inheritDoc}
+	 * {@inheritdoc}
 	 */
 	public function setException(\Exception $e) {
 		$this->exception = $e;
 		return $this;
 	}
 
 	/**
-	 * {@inheritDoc}
+	 * {@inheritdoc}
 	 */
 	public function getException() {
 		return $this->exception;
@@ -103,7 +90,16 @@ public function setForwardURL(string $forward_url = REFERRER) {
 	 * {@inheritdoc}
 	 */
 	public function getForwardURL(): ?string {
-		return $this->forward_url;
+		if (!isset($this->forward_url)) {
+			return null;
+		}
+
+		$forward_url = $this->forward_url;
+		if ($forward_url === REFERRER || !$this->secure_forward_url) {
+			return $forward_url;
+		}
+
+		return elgg_normalize_site_url($forward_url) !== null ? $forward_url : '';
 	}
 
 	/**
@@ -118,7 +114,7 @@ public function setHeaders(array $headers = []) {
 	 * {@inheritdoc}
 	 */
 	public function getHeaders() {
-		return (array) $this->headers;
+		return $this->headers;
 	}
 
 	/**

engine/lib/pagehandler.php

@@ -256,18 +256,19 @@ function elgg_error_response(string|array $message = '', string $forward_url = R
 /**
  * Prepare a silent redirect response to be returned by a page or an action handler
  *
- * @param string $forward_url Redirection URL
- *                            Relative or absolute URL to redirect the client to
- * @param int    $status_code HTTP status code
- *                            Status code of the HTTP response
- *                            Note that the Router and AJAX API will treat these responses
- *                            as redirection in spite of the HTTP code assigned
- *                            Note that non-redirection HTTP codes will throw an exception
+ * @param string $forward_url        Redirection URL
+ *                                   Relative or absolute URL to redirect the client to
+ * @param int    $status_code        HTTP status code
+ *                                   Status code of the HTTP response
+ *                                   Note that the Router and AJAX API will treat these responses
+ *                                   as redirection in spite of the HTTP code assigned
+ *                                   Note that non-redirection HTTP codes will throw an exception
+ * @param bool   $secure_forward_url Determines if the forward url needs to be secure
  *
  * @return \Elgg\Http\RedirectResponse
  */
-function elgg_redirect_response(string $forward_url = REFERRER, int $status_code = ELGG_HTTP_FOUND): \Elgg\Http\RedirectResponse {
-	return new Elgg\Http\RedirectResponse($forward_url, $status_code);
+function elgg_redirect_response(string $forward_url = REFERRER, int $status_code = ELGG_HTTP_FOUND, bool $secure_forward_url = true): \Elgg\Http\RedirectResponse {
+	return new Elgg\Http\RedirectResponse($forward_url, $status_code, $secure_forward_url);
 }
 
 /**

engine/tests/classes/Elgg/Http/ResponseUnitTestCase.php

@@ -144,8 +144,8 @@ public function validForwardURLsProvider() {
 			[REFERRER],
 			['foo'],
 			['/foo'],
-			['http://localhost/'],
-			['?foo=bar'],
+			[self::getTestingConfig()->wwwroot],
+			['a?foo=bar'],
 		];
 	}
 

engine/tests/classes/Elgg/Testing.php

@@ -42,16 +42,13 @@ public function normalizeTestFilePath($filename = '') {
 	 * @return Request
 	 */
 	public static function prepareHttpRequest($uri = '', $method = 'GET', $parameters = [], $ajax = 0, $add_csrf_tokens = false) {
-		$site_url = elgg_get_site_url();
-		$path = '/' . ltrim(substr(elgg_normalize_url($uri), strlen($site_url)), '/');
-
 		if ($add_csrf_tokens) {
 			$ts = _elgg_services()->csrf->getCurrentTime()->getTimestamp();
 			$parameters['__elgg_ts'] = $ts;
 			$parameters['__elgg_token'] = _elgg_services()->csrf->generateActionToken($ts);
 		}
 
-		$request = Request::create($path, $method, $parameters);
+		$request = Request::create(elgg_normalize_url($uri), $method, $parameters);
 
 		$cookie_name = _elgg_services()->config->getCookieConfig()['session']['name'];
 		$session_id = _elgg_services()->session->getID();

engine/tests/phpunit/unit/Elgg/Http/RedirectResponseUnitTest.php

@@ -30,6 +30,18 @@ public function testCanConstructWithArguments() {
 		$this->assertEquals($forward_url, $response->getForwardURL());
 		$this->assertEquals([], $response->getHeaders());
 	}
+
+	public function testCanNotSetInsecureForwardURL() {
+		$test_class = $this->getReponseClassName();
+		$response = new $test_class('http://unsafedomain.com');
+		$this->assertEquals('', $response->getForwardURL());
+	}
+
+	public function testCanSetInsecureForwardURL() {
+		$test_class = $this->getReponseClassName();
+		$response = new $test_class('http://unsafedomain.com', ELGG_HTTP_FOUND, false);
+		$this->assertEquals('http://unsafedomain.com', $response->getForwardURL());
+	}
 
 	// Remaining tests are identical to ResponseUnitTest
 }

views/default/elements/forms.css.php

@@ -192,7 +192,7 @@
 	.elgg-fieldset-horizontal {
 		display: flex;
 
-		.elgg-field {
+		> .elgg-field {
 			margin: 0 1rem 0 0;
 			vertical-align: top;
 
@@ -231,23 +231,23 @@
 		&.elgg-fieldset-wrap {
 			flex-wrap: wrap;
 
-			.elgg-field {
+			> .elgg-field {
 				margin-bottom: 0.5rem;
 			}
 		}
 
 		&.elgg-justify-right {
 			justify-content: flex-end;
 
-			.elgg-field {
+			> .elgg-field {
 				margin: 0 0 0 1rem;
 			}
 		}
 
 		&.elgg-justify-center {
 			justify-content: center;
 
-			.elgg-field {
+			> .elgg-field {
 				margin: 0 5px;
 			}
 		}