Skip to content

Soften the cookie check if no cookies are sent #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LukasReschke merged 1 commit into from
Jun 20, 2016
Merged

Soften the cookie check if no cookies are sent #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/private/appframework/http/request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,16 @@ public function passesCSRFCheck() {
}

/**
* Checks if the strict cookie has been sent with the request
* Checks if the strict cookie has been sent with the request if the request
* is including any cookies.
*
* @return bool
* @since 9.1.0
*/
public function passesStrictCookieCheck() {
if(count($this->cookies) === 0) {
return true;
}
if($this->getCookie('nc_sameSiteCookiestrict') === 'true'
&& $this->passesLaxCookieCheck()) {
return true;
Expand All @@ -483,12 +487,17 @@ public function passesStrictCookieCheck() {
}

/**
* Checks if the lax cookie has been sent with the request
* Checks if the lax cookie has been sent with the request if the request
* is including any cookies.
*
* @return bool
* @since 9.1.0
*/
public function passesLaxCookieCheck() {
if(count($this->cookies) === 0) {
return true;
}

if($this->getCookie('nc_sameSiteCookielax') === 'true') {
return true;
}
Expand Down
16 changes: 9 additions & 7 deletions lib/public/irequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,18 +144,20 @@ public function getCookie($key);
public function passesCSRFCheck();

/**
* Checks if the strict cookie has been sent with the request
*
* @return bool
* @since 9.0.0
*/
* Checks if the strict cookie has been sent with the request if the request
* is including any cookies.
*
* @return bool
* @since 9.0.0
*/
public function passesStrictCookieCheck();

/**
* Checks if the lax cookie has been sent with the request
* Checks if the lax cookie has been sent with the request if the request
* is including any cookies.
*
* @return bool
* @since 9.1.0
* @since 9.0.0
*/
public function passesLaxCookieCheck();

Expand Down
53 changes: 41 additions & 12 deletions tests/lib/appframework/http/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,7 +1404,7 @@ public function testPassesCSRFCheckWithHeader() {
$this->assertTrue($request->passesCSRFCheck());
}

public function testFailsCSRFCheckWithGetAndWithoutCookies() {
public function testPassesCSRFCheckWithGetAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
Expand All @@ -1421,13 +1421,14 @@ public function testFailsCSRFCheckWithGetAndWithoutCookies() {
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
->expects($this->once())
->method('isTokenValid')
->willReturn(true);

$this->assertFalse($request->passesCSRFCheck());
$this->assertTrue($request->passesCSRFCheck());
}

public function testFailsCSRFCheckWithPostAndWithoutCookies() {
public function testPassesCSRFCheckWithPostAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
Expand All @@ -1444,13 +1445,14 @@ public function testFailsCSRFCheckWithPostAndWithoutCookies() {
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
->expects($this->once())
->method('isTokenValid')
->willReturn(true);

$this->assertFalse($request->passesCSRFCheck());
$this->assertTrue($request->passesCSRFCheck());
}

public function testFailsCSRFCheckWithHeaderAndWithoutCookies() {
public function testPassesCSRFCheckWithHeaderAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
Expand All @@ -1467,10 +1469,11 @@ public function testFailsCSRFCheckWithHeaderAndWithoutCookies() {
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
->expects($this->once())
->method('isTokenValid')
->willReturn(true);

$this->assertFalse($request->passesCSRFCheck());
$this->assertTrue($request->passesCSRFCheck());
}

public function testFailsCSRFCheckWithHeaderAndNotAllChecksPassing() {
Expand Down Expand Up @@ -1523,6 +1526,32 @@ public function testPassesStrictCookieCheckWithAllCookies() {
$this->assertTrue($request->passesStrictCookieCheck());
}

public function testFailsSRFCheckWithPostAndWithCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'post' => [
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'foo' => 'bar',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');

$this->assertFalse($request->passesCSRFCheck());
}

public function testFailStrictCookieCheckWithOnlyLaxCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
Expand Down