Merge pull request #12770 from nextcloud/fix/drop-dead-redirect

fix: Remove unused proxy redirect route

Author: ChristophWurst

appinfo/routes.php

@@ -295,11 +295,6 @@
 			'url' => '/api/avatars/image/{email}',
 			'verb' => 'GET'
 		],
-		[
-			'name' => 'proxy#redirect',
-			'url' => '/redirect',
-			'verb' => 'GET'
-		],
 		[
 			'name' => 'proxy#proxy',
 			'url' => '/proxy',

css/redirect.css

@@ -10,7 +10,6 @@
 
 namespace OCA\Mail\Controller;
 
-use Exception;
 use OCA\Mail\Html\ProxyHmacGenerator;
 use OCA\Mail\Http\ProxyDownloadResponse;
 use OCA\Mail\Service\MailManager;
@@ -20,7 +19,6 @@
 use OCP\AppFramework\Http\Attribute\OpenAPI;
 use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\Response;
-use OCP\AppFramework\Http\TemplateResponse;
 use OCP\Http\Client\IClientService;
 use OCP\Http\Client\LocalServerException;
 use OCP\IRequest;
@@ -61,38 +59,6 @@ public function __construct(string $appName,
 		$this->userId = $userId;
 	}
 
-	/**
-	 * @NoAdminRequired
-	 * @NoCSRFRequired
-	 *
-	 * @param string $src
-	 *
-	 * @throws \Exception If the URL is not valid.
-	 * @return TemplateResponse
-	 */
-	public function redirect(string $src): TemplateResponse {
-		$authorizedRedirect = false;
-
-		if (!str_starts_with($src, 'http://')
-			&& !str_starts_with($src, 'https://')
-			&& !str_starts_with($src, 'ftp://')) {
-			throw new Exception('URL is not valid.', 1);
-		}
-
-		// If strict cookies are set it means we come from the same domain so no open redirect
-		if ($this->request->passesStrictCookieCheck()) {
-			$authorizedRedirect = true;
-		}
-
-		$params = [
-			'authorizedRedirect' => $authorizedRedirect,
-			'url' => $src,
-			'urlHost' => parse_url($src, PHP_URL_HOST),
-			'mailURL' => $this->urlGenerator->linkToRoute('mail.page.index'),
-		];
-		return new TemplateResponse($this->appName, 'redirect', $params, 'guest');
-	}
-
 	/**
 	 * @NoAdminRequired
 	 * @NoCSRFRequired

lib/Controller/ProxyController.php

@@ -149,15 +149,6 @@ describe('text', () => {
 			expect(actual).toEqual(expected)
 		})
 
-		it('does not leak internal redirection URLs', () => {
-			const source = html('<a href="https://localhost/apps/mail/redirect?src=domain.tld">domain.tld</a>')
-			const expected = plain('domain.tld')
-
-			const actual = toPlain(source)
-
-			expect(actual).toEqual(expected)
-		})
-
 		it('preserves quotes', () => {
 			const source = html('<blockquote><div><b>yes.</b></div><div><br /></div><div>Am Montag, den 21.10.2019, 16:51 +0200 schrieb Christoph Wurst:</div><blockquote style="margin:0 0 0 .8ex;border-left:2px #729fcf solid;padding-left:1ex;"><div>ok cool</div><div><br /></div><div>Am Montag, den 21.10.2019, 16:51 +0200 schrieb Christoph Wurst:</div><blockquote style="margin:0 0 0 .8ex;border-left:2px #729fcf solid;padding-left:1ex;"><div>Hello</div><div><br /></div><div>this is some t<i>e</i>xt</div><div><br /></div><div>yes</div><div><br /></div><div>cheers</div><br></blockquote><br></blockquote></blockquote>')
 			const expected = plain(`> yes.

src/autoredirect.js

@@ -11,15 +11,13 @@
 namespace OCA\Mail\Tests\Unit\Controller;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
-use Exception;
 use OCA\Mail\Controller\ProxyController;
 use OCA\Mail\Html\ProxyHmacGenerator;
 use OCA\Mail\Http\ProxyDownloadResponse;
 use OCA\Mail\Service\MailManager;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Response;
-use OCP\AppFramework\Http\TemplateResponse;
 use OCP\Http\Client\IClient;
 use OCP\Http\Client\IClientService;
 use OCP\Http\Client\IResponse;
@@ -73,98 +71,6 @@ protected function setUp(): void {
 		$this->logger = new NullLogger();
 	}
 
-	public function redirectDataProvider() {
-		return [
-			[
-				'http://nextcloud.com',
-				false,
-				false
-			],
-			[
-				'https://nextcloud.com',
-				false,
-				false
-			],
-			[
-				'http://nextcloud.com',
-				true,
-				true
-			],
-			[
-				'http://example.com',
-				false,
-				false
-			],
-			[
-				'https://example.com',
-				true,
-				true
-			],
-			[
-				'ftp://example.com',
-				true,
-				true
-			],
-		];
-	}
-
-	/**
-	 * @dataProvider redirectDataProvider
-	 */
-	public function testRedirect(string $url,
-		bool $passesTest,
-		bool $authorized) {
-		$this->urlGenerator->expects($this->once())
-			->method('linkToRoute')
-			->with('mail.page.index')
-			->will($this->returnValue('mail-route'));
-		$this->request->expects($this->once())
-			->method('passesStrictCookieCheck')
-			->willReturn($passesTest);
-		$this->controller = new ProxyController(
-			$this->appName,
-			$this->request,
-			$this->urlGenerator,
-			$this->session,
-			$this->clientService,
-			$this->hmacGenerator,
-			$this->logger,
-			$this->mailManager,
-			$this->userId,
-		);
-		$expected = new TemplateResponse(
-			$this->appName,
-			'redirect',
-			[
-				'authorizedRedirect' => $authorized,
-				'url' => $url,
-				'urlHost' => parse_url($url, PHP_URL_HOST),
-				'mailURL' => 'mail-route'
-			],
-			'guest'
-		);
-
-		$response = $this->controller->redirect($url);
-
-		$this->assertEquals($expected, $response);
-	}
-
-	public function testRedirectInvalidUrl() {
-		$this->controller = new ProxyController(
-			$this->appName,
-			$this->request,
-			$this->urlGenerator,
-			$this->session,
-			$this->clientService,
-			$this->hmacGenerator,
-			$this->logger,
-			$this->mailManager,
-			$this->userId,
-		);
-		$this->expectException(Exception::class);
-
-		$this->controller->redirect('ftps://example.com');
-	}
 
 	public function testProxyWithoutCookies(): void {
 		$src = 'http://example.com';

src/tests/unit/util/text.spec.js

@@ -40,7 +40,6 @@ const plugins = [
 
 module.exports = async () => ({
 	entry: {
-		autoredirect: path.join(__dirname, 'src/autoredirect.js'),
 		mail: path.join(__dirname, 'src/main.js'),
 		oauthpopup: path.join(__dirname, 'src/main-oauth-popup.js'),
 		settings: path.join(__dirname, 'src/main-settings'),