Skip to content

Commit ed3b757

Browse files
phil-davisDeepDiver1975
phil-davis
authored and
DeepDiver1975
committed
Merge pull request #40856 from owncloud/fix/header-evaluation
fix: use empty() to check if header exists
1 parent 2d6c6bf commit ed3b757

File tree

5 files changed

+46
-20
lines changed

5 files changed

+46
-20
lines changed

‎apps/files_sharing/lib/Controller/Share20OcsController.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ protected function formatShare(IShare $share, $received = false) {
281281
* Get a specific share by id
282282
*
283283
* @NoAdminRequired
284+
* @NoCSRFRequired
284285
*
285286
* @param string $id
286287
* @return Result
@@ -664,6 +665,7 @@ private function getSharedWithMe($node = null, $includeTags, $requestedShareType
664665
* the function will return an empty list.
665666
*
666667
* @NoAdminRequired
668+
* @NoCSRFRequired
667669
*
668670
* - Get shares by the current user
669671
* - Get shares by the current user and reshares (?reshares=true)

‎changelog/unreleased/40856‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Bugfix: Request header can hold an empty string if not set
2+
3+
Due to Apache rewrite rules originally not existing headers can hold an empty
4+
string.
5+
6+
https://github.com/owncloud/core/pull/40856

‎core/Controller/OcsController.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ public function checkPerson($login, $password) {
110110
* test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute
111111
*
112112
* @NoAdminRequired
113+
* @NoCSRFRequired
113114
*
114115
* @return Result
115116
*/

‎lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,8 @@ public function beforeController($controller, $methodName) {
143143
// CSRF check - also registers the CSRF token since the session may be closed later
144144
Util::callRegister();
145145
if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
146-
if (!$this->request->passesCSRFCheck() && $this->request->getHeader("Authorization") === null) {
146+
$hasNoAuthHeader = ($this->request->getHeader("Authorization") === null || trim($this->request->getHeader("Authorization")) === '');
147+
if (!$this->request->passesCSRFCheck() && $hasNoAuthHeader) {
147148
throw new CrossSiteRequestForgeryException();
148149
}
149150
}

‎tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
use OCP\ISession;
4141
use OCP\AppFramework\Controller;
4242
use OCP\IUserSession;
43+
use ReflectionException;
4344
use Test\TestCase;
4445
use OCP\AppFramework\Http\Response;
4546
use OCP\IConfig;
@@ -136,7 +137,7 @@ private function getMiddleware($isLoggedIn, $isAdminUser) {
136137
* @PublicPage
137138
* @NoCSRFRequired
138139
* @throws SecurityException
139-
* @throws \ReflectionException
140+
* @throws ReflectionException
140141
*/
141142
public function testSetNavigationEntry() {
142143
$this->navigationManager->expects($this->once())
@@ -151,7 +152,7 @@ public function testSetNavigationEntry() {
151152
* @param string $method
152153
* @param string $test
153154
* @param $status
154-
* @throws \ReflectionException
155+
* @throws ReflectionException
155156
*/
156157
private function ajaxExceptionStatus($method, $test, $status) {
157158
$isLoggedIn = false;
@@ -179,7 +180,7 @@ private function ajaxExceptionStatus($method, $test, $status) {
179180
}
180181

181182
/**
182-
* @throws \ReflectionException
183+
* @throws ReflectionException
183184
*/
184185
public function testAjaxStatusLoggedInCheck() {
185186
$this->ajaxExceptionStatus(
@@ -191,7 +192,7 @@ public function testAjaxStatusLoggedInCheck() {
191192

192193
/**
193194
* @NoCSRFRequired
194-
* @throws \ReflectionException
195+
* @throws ReflectionException
195196
*/
196197
public function testAjaxNotAdminCheck() {
197198
$this->ajaxExceptionStatus(
@@ -203,7 +204,7 @@ public function testAjaxNotAdminCheck() {
203204

204205
/**
205206
* @PublicPage
206-
* @throws \ReflectionException
207+
* @throws ReflectionException
207208
*/
208209
public function testAjaxStatusCSRFCheck() {
209210
$this->ajaxExceptionStatus(
@@ -216,10 +217,7 @@ public function testAjaxStatusCSRFCheck() {
216217
/**
217218
* @PublicPage
218219
* @NoCSRFRequired
219-
* @throws \ReflectionException
220-
* @throws \ReflectionException
221-
* @throws \ReflectionException
222-
* @throws \ReflectionException
220+
* @throws ReflectionException
223221
*/
224222
public function testAjaxStatusAllGood() {
225223
$this->ajaxExceptionStatus(
@@ -248,7 +246,7 @@ public function testAjaxStatusAllGood() {
248246
* @PublicPage
249247
* @NoCSRFRequired
250248
* @throws SecurityException
251-
* @throws \ReflectionException
249+
* @throws ReflectionException
252250
*/
253251
public function testNoChecks() {
254252
$this->request->expects($this->never())
@@ -266,7 +264,7 @@ public function testNoChecks() {
266264
* @param string $expects
267265
* @param bool $shouldFail
268266
* @throws SecurityException
269-
* @throws \ReflectionException
267+
* @throws ReflectionException
270268
*/
271269
private function securityCheck($method, $expects, $shouldFail=false) {
272270
// admin check requires login
@@ -293,10 +291,10 @@ private function securityCheck($method, $expects, $shouldFail=false) {
293291
/**
294292
* @PublicPage
295293
* @throws SecurityException
296-
* @throws \ReflectionException
294+
* @throws ReflectionException
297295
*/
298296
public function testCsrfCheck() {
299-
$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class);
297+
$this->expectException(CrossSiteRequestForgeryException::class);
300298

301299
$this->request->expects($this->once())
302300
->method('passesCSRFCheck')
@@ -310,7 +308,7 @@ public function testCsrfCheck() {
310308
* @PublicPage
311309
* @NoCSRFRequired
312310
* @throws SecurityException
313-
* @throws \ReflectionException
311+
* @throws ReflectionException
314312
*/
315313
public function testNoCsrfCheck() {
316314
$this->request->expects($this->never())
@@ -324,7 +322,7 @@ public function testNoCsrfCheck() {
324322
/**
325323
* @PublicPage
326324
* @throws SecurityException
327-
* @throws \ReflectionException
325+
* @throws ReflectionException
328326
*/
329327
public function testFailCsrfCheck() {
330328
$this->request->expects($this->once())
@@ -335,11 +333,29 @@ public function testFailCsrfCheck() {
335333
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
336334
}
337335

336+
/**
337+
* @PublicPage
338+
* @throws SecurityException
339+
* @throws ReflectionException
340+
*/
341+
public function testFailCsrfCheckWithoutAuthHeader(): void {
342+
$this->expectException(CrossSiteRequestForgeryException::class);
343+
$this->request->expects($this->once())
344+
->method('passesCSRFCheck')
345+
->willReturn(false);
346+
$this->request
347+
->method('getHeader')
348+
->willReturn('');
349+
350+
$this->reader->reflect(__CLASS__, __FUNCTION__);
351+
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
352+
}
353+
338354
/**
339355
* @NoCSRFRequired
340356
* @NoAdminRequired
341357
* @throws SecurityException
342-
* @throws \ReflectionException
358+
* @throws ReflectionException
343359
*/
344360
public function testLoggedInCheck() {
345361
$this->securityCheck(__FUNCTION__, 'isLoggedIn');
@@ -349,7 +365,7 @@ public function testLoggedInCheck() {
349365
* @NoCSRFRequired
350366
* @NoAdminRequired
351367
* @throws SecurityException
352-
* @throws \ReflectionException
368+
* @throws ReflectionException
353369
*/
354370
public function testFailLoggedInCheck() {
355371
$this->securityCheck(__FUNCTION__, 'isLoggedIn', true);
@@ -358,7 +374,7 @@ public function testFailLoggedInCheck() {
358374
/**
359375
* @NoCSRFRequired
360376
* @throws SecurityException
361-
* @throws \ReflectionException
377+
* @throws ReflectionException
362378
*/
363379
public function testIsAdminCheck() {
364380
$this->securityCheck(__FUNCTION__, 'isAdminUser');
@@ -367,7 +383,7 @@ public function testIsAdminCheck() {
367383
/**
368384
* @NoCSRFRequired
369385
* @throws SecurityException
370-
* @throws \ReflectionException
386+
* @throws ReflectionException
371387
*/
372388
public function testFailIsAdminCheck() {
373389
$this->securityCheck(__FUNCTION__, 'isAdminUser', true);

0 commit comments

Comments
 (0)