Skip to content

Conversation

ChristophWurst
Copy link
Member

Due to the strict typing, parse_url fails when a non-string value is passed.
To prevent that, a is_string check is added. Additionally, the referrer is
now read directly in the controller instead of injecting via the DI container.

Fixes https://sentry.io/share/issue/83a7268d63f145eca3d7f4640dd26636/

Due to the strict typing, `parse_url` fails when a non-string value is passed.
To prevent that, a `is_string` check is added. Additionally, the referrer is
now read directly in the controller instead of injecting via the DI container.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
$this->clientService = $this->getMockBuilder('\OCP\Http\Client\IClientService')->getMock();
$this->request = $this->createMock([
IRequest::class,
ArrayAccess::class,
Copy link
Member Author

@ChristophWurst ChristophWurst Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API issue in the server, will report this and/or prepare a PR. cc @rullzer as discussed.

It's not. I thought the ArrayAccess would make it possible to access server value, but it's actually implemented via a magic getter.

For other parameters this is still an issue, but has no effect on this.

@ChristophWurst ChristophWurst mentioned this pull request Aug 8, 2018
4 tasks
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Aug 8, 2018

There were 6 failures:
1) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #0 ('http://nextcloud.com', 'http://anotherhostname.com', false)
Expectation failed for method name is equal to <string:offsetGet> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
2) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #1 ('https://nextcloud.com', 'http://anotherhostname.com', false)
Expectation failed for method name is equal to <string:offsetGet> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
3) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #2 ('http://nextcloud.com', 'https://example.com', true)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\TemplateResponse Object (
     'templateName' => 'redirect'
     'params' => Array (
-        'authorizedRedirect' => true
+        'authorizedRedirect' => false
         'url' => 'http://nextcloud.com'
         'urlHost' => 'nextcloud.com'
         'mailURL' => 'mail-route'
     )
     'renderAs' => 'guest'
     'appName' => 'mail'
     'headers' => Array (...)
     'cookies' => Array ()
     'status' => 200
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => null
     'throttled' => false
     'throttleMetadata' => Array ()
 )
/home/travis/build/nextcloud/core/apps/mail/tests/Controller/ProxyControllerTest.php:135
4) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #3 ('http://example.com', 'https://example.com', true)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\TemplateResponse Object (
     'templateName' => 'redirect'
     'params' => Array (
-        'authorizedRedirect' => true
+        'authorizedRedirect' => false
         'url' => 'http://example.com'
         'urlHost' => 'example.com'
         'mailURL' => 'mail-route'
     )
     'renderAs' => 'guest'
     'appName' => 'mail'
     'headers' => Array (...)
     'cookies' => Array ()
     'status' => 200
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => null
     'throttled' => false
     'throttleMetadata' => Array ()
 )
/home/travis/build/nextcloud/core/apps/mail/tests/Controller/ProxyControllerTest.php:135
5) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #4 ('https://example.com', 'https://example.com', true)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\TemplateResponse Object (
     'templateName' => 'redirect'
     'params' => Array (
-        'authorizedRedirect' => true
+        'authorizedRedirect' => false
         'url' => 'https://example.com'
         'urlHost' => 'example.com'
         'mailURL' => 'mail-route'
     )
     'renderAs' => 'guest'
     'appName' => 'mail'
     'headers' => Array (...)
     'cookies' => Array ()
     'status' => 200
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => null
     'throttled' => false
     'throttleMetadata' => Array ()
 )
/home/travis/build/nextcloud/core/apps/mail/tests/Controller/ProxyControllerTest.php:135
6) OCA\Mail\Tests\Controller\ProxyControllerTest::testRedirect with data set #5 ('ftp://example.com', 'https://example.com', true)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\TemplateResponse Object (
     'templateName' => 'redirect'
     'params' => Array (
-        'authorizedRedirect' => true
+        'authorizedRedirect' => false
         'url' => 'ftp://example.com'
         'urlHost' => 'example.com'
         'mailURL' => 'mail-route'
     )
     'renderAs' => 'guest'
     'appName' => 'mail'
     'headers' => Array (...)
     'cookies' => Array ()
     'status' => 200
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => null
     'throttled' => false
     'throttleMetadata' => Array ()
 )
/home/travis/build/nextcloud/core/apps/mail/tests/Controller/ProxyControllerTest.php:135
FAILURES!
Tests: 375, Assertions: 839, Failures: 6.

These tests pass locally ❤️ turns out they don't.

IRequest exports the server values as magic read-only property and
not via the array access.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

My bad. The IRequest works different than I assumed. Should be good now.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@ChristophWurst ChristophWurst merged commit 731b7a4 into master Aug 8, 2018
@ChristophWurst ChristophWurst deleted the fix/no-referrer-proxy-redirect branch August 8, 2018 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants