Skip to content

Commit

Permalink
Merge pull request #950 from nextcloud/bugfix/noid/prevent_delivering…
Browse files Browse the repository at this point in the history 
…_broken_ics_data_via_proxycontroller

prevent delivering broken ics data via proxycontroller
  • Loading branch information
@georgehrke
georgehrke committed Nov 23, 2018
2 parents af4954e + e72267c commit 38e9979
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 121 deletions.
1 change: 0 additions & 1 deletion Makefile
View file
Expand Up @@ -147,7 +147,6 @@ appstore:
$(copy_command) --parents -r \
"appinfo" \
"controller" \
"http" \
"img" \
"l10n" \
"templates" \
Expand Down
22 changes: 17 additions & 5 deletions controller/proxycontroller.php
View file
Expand Up @@ -25,15 +25,15 @@
use GuzzleHttp\Exception\ClientException;
use GuzzleHttp\Exception\ConnectException;

use OCA\Calendar\Http\StreamResponse;

use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\DataDisplayResponse;
use OCP\AppFramework\Controller;
use OCP\Http\Client\IClientService;
use OCP\IL10N;
use OCP\ILogger;
use OCP\IRequest;
use Sabre\VObject\Reader;

class ProxyController extends Controller {

Expand Down Expand Up @@ -72,7 +72,7 @@ public function __construct($appName, IRequest $request,
* @NoAdminRequired
*
* @param $url
* @return StreamResponse|JSONResponse
* @return DataDisplayResponse|JSONResponse
*/
public function proxy($url) {
$client = $this->client->newClient();
Expand All @@ -86,7 +86,6 @@ public function proxy($url) {
// try to find a chain of 301s
do {
$clientResponse = $client->get($queryUrl, [
'stream' => true,
'allow_redirects' => $allow_redirects,
]);

Expand Down Expand Up @@ -118,7 +117,20 @@ public function proxy($url) {
'new_url' => $queryUrl,
], Http::STATUS_BAD_REQUEST);
} elseif ($done) {
$response = new StreamResponse($clientResponse->getBody());
$icsData = $clientResponse->getBody();

try {
Reader::read($icsData, Reader::OPTION_FORGIVING);
} catch(\Exception $ex) {
$response = new JSONResponse([
'message' => $this->l10n->t('The remote server did not give us access to the calendar (HTTP 404 error)'),
'proxy_code' => 404
], Http::STATUS_UNPROCESSABLE_ENTITY);

return $response;
}

$response = new DataDisplayResponse($icsData, 200);
$response->setHeaders([
'Content-Type' => 'text/calendar',
]);
Expand Down
50 changes: 0 additions & 50 deletions http/streamresponse.php
View file

This file was deleted.

35 changes: 13 additions & 22 deletions tests/php/controller/proxycontrollerTest.php
View file
Expand Up @@ -21,6 +21,7 @@
*/
namespace OCA\Calendar\Controller;

use OCP\AppFramework\Http\DataDisplayResponse;
use PHPUnit\Framework\TestCase;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Exception\ClientException;
Expand Down Expand Up @@ -112,19 +113,23 @@ public function testProxy() {
$this->newClient->expects($this->once())
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
$this->response0->expects($this->once())
->method('getStatusCode')
->with()
->will($this->returnValue(200));
$this->response0->expects($this->once())
->method('getBody')
->with()
->will($this->returnValue("BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//Sabre//Sabre VObject 4.1.6//EN\r\nCALSCALE:GREGORIAN\r\nEND:VCALENDAR\r\n"));

$actual = $this->controller->proxy($testUrl);

$this->assertInstanceOf('OCA\Calendar\Http\StreamResponse', $actual);
$this->assertInstanceOf(DataDisplayResponse::class, $actual);
$this->assertEquals('text/calendar', $actual->getHeaders()['Content-Type']);
$this->assertEquals("BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//Sabre//Sabre VObject 4.1.6//EN\r\nCALSCALE:GREGORIAN\r\nEND:VCALENDAR\r\n", $actual->getData());
}

public function testProxyClientException() {
Expand All @@ -136,7 +141,6 @@ public function testProxyClientException() {
$this->newClient->expects($this->once())
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->throwException(new ClientException('Exception Message foo bar 42',
Expand Down Expand Up @@ -171,7 +175,6 @@ public function testProxyConnectException() {
$this->newClient->expects($this->once())
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->throwException(new ConnectException('Exception Message foo bar 42',
Expand Down Expand Up @@ -203,7 +206,6 @@ public function testProxyRequestExceptionHTTP() {
$this->newClient->expects($this->once())
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->throwException(new RequestException('Exception Message foo bar 42',
Expand Down Expand Up @@ -235,7 +237,6 @@ public function testProxyRequestExceptionHTTPS() {
$this->newClient->expects($this->once())
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->throwException(new RequestException('Exception Message foo bar 42',
Expand Down Expand Up @@ -267,7 +268,6 @@ public function testProxyRedirect() {
$this->newClient->expects($this->at(0))
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
Expand All @@ -282,7 +282,6 @@ public function testProxyRedirect() {
$this->newClient->expects($this->at(1))
->method('get')
->with('http://def.abc/foobar?456', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
Expand All @@ -309,7 +308,6 @@ public function testProxyRedirectNonPermanent() {
$this->newClient->expects($this->at(0))
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
Expand All @@ -320,7 +318,6 @@ public function testProxyRedirectNonPermanent() {
$this->newClient->expects($this->at(1))
->method('get')
->with('http://abc.def/foobar?123' , [
'stream' => true,
'allow_redirects' => [
'max' => 5,
],
Expand All @@ -330,11 +327,16 @@ public function testProxyRedirectNonPermanent() {
->method('getStatusCode')
->with()
->will($this->returnValue(200));
$this->response1->expects($this->once())
->method('getBody')
->with()
->will($this->returnValue("BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//Sabre//Sabre VObject 4.1.6//EN\r\nCALSCALE:GREGORIAN\r\nEND:VCALENDAR\r\n"));

$actual = $this->controller->proxy($testUrl);

$this->assertInstanceOf('OCA\Calendar\Http\StreamResponse', $actual);
$this->assertInstanceOf(DataDisplayResponse::class, $actual);
$this->assertEquals('text/calendar', $actual->getHeaders()['Content-Type']);
$this->assertEquals("BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//Sabre//Sabre VObject 4.1.6//EN\r\nCALSCALE:GREGORIAN\r\nEND:VCALENDAR\r\n", $actual->getData());
}

public function testProxyMultipleRedirects() {
Expand All @@ -346,21 +348,18 @@ public function testProxyMultipleRedirects() {
$this->newClient->expects($this->at(0))
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
$this->newClient->expects($this->at(1))
->method('get')
->with('http://def.abc/foobar?456' , [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response1));
$this->newClient->expects($this->at(2))
->method('get')
->with('http://xyz.abc/foobar?789' , [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response2));
Expand Down Expand Up @@ -403,14 +402,12 @@ public function testProxyMultipleRedirectsNonPermanent() {
$this->newClient->expects($this->at(0))
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
$this->newClient->expects($this->at(1))
->method('get')
->with('http://def.abc/foobar?456' , [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response1));
Expand Down Expand Up @@ -445,42 +442,36 @@ public function testProxyAtMostFiveRedirects() {
$this->newClient->expects($this->at(0))
->method('get')
->with($testUrl, [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response0));
$this->newClient->expects($this->at(1))
->method('get')
->with('http://def.abc/foobar?456-0', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response1));
$this->newClient->expects($this->at(2))
->method('get')
->with('http://def.abc/foobar?456-1', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response2));
$this->newClient->expects($this->at(3))
->method('get')
->with('http://def.abc/foobar?456-2', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response3));
$this->newClient->expects($this->at(4))
->method('get')
->with('http://def.abc/foobar?456-3', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response4));
$this->newClient->expects($this->at(5))
->method('get')
->with('http://def.abc/foobar?456-4', [
'stream' => true,
'allow_redirects' => false,
])
->will($this->returnValue($this->response5));
Expand Down
43 changes: 0 additions & 43 deletions tests/php/http/streamresponseTest.php
View file

This file was deleted.

0 comments on commit 38e9979

Please sign in to comment.