Skip to content

Commit c85f6a6

Browse files
susnuxsusnux
authored
Merge pull request #53040 from nextcloud/backport/52810/stable31
[stable31] fix: Move CSRF check from base to PublicAuth for public.php
2 parents 512da4f + 617af2f commit c85f6a6

File tree

7 files changed

+63
-43
lines changed

7 files changed

+63
-43
lines changed

‎apps/dav/appinfo/v2/publicremote.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
use OCP\IRequest;
2727
use OCP\ISession;
2828
use OCP\ITagManager;
29+
use OCP\IURLGenerator;
2930
use OCP\IUserSession;
3031
use OCP\L10N\IFactory;
3132
use OCP\Security\Bruteforce\IThrottler;
@@ -53,7 +54,8 @@
5354
Server::get(IManager::class),
5455
$session,
5556
Server::get(IThrottler::class),
56-
Server::get(LoggerInterface::class)
57+
Server::get(LoggerInterface::class),
58+
Server::get(IURLGenerator::class),
5759
);
5860
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
5961

‎apps/dav/lib/Connector/Sabre/PublicAuth.php‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OCP\Defaults;
1515
use OCP\IRequest;
1616
use OCP\ISession;
17+
use OCP\IURLGenerator;
1718
use OCP\Security\Bruteforce\IThrottler;
1819
use OCP\Share\Exceptions\ShareNotFound;
1920
use OCP\Share\IManager;
@@ -22,6 +23,7 @@
2223
use Sabre\DAV\Auth\Backend\AbstractBasic;
2324
use Sabre\DAV\Exception\NotAuthenticated;
2425
use Sabre\DAV\Exception\NotFound;
26+
use Sabre\DAV\Exception\PreconditionFailed;
2527
use Sabre\DAV\Exception\ServiceUnavailable;
2628
use Sabre\HTTP;
2729
use Sabre\HTTP\RequestInterface;
@@ -44,24 +46,25 @@ public function __construct(
4446
private ISession $session,
4547
private IThrottler $throttler,
4648
private LoggerInterface $logger,
49+
private IURLGenerator $urlGenerator,
4750
) {
4851
// setup realm
4952
$defaults = new Defaults();
5053
$this->realm = $defaults->getName();
5154
}
5255

5356
/**
54-
* @param RequestInterface $request
55-
* @param ResponseInterface $response
56-
*
57-
* @return array
5857
* @throws NotAuthenticated
5958
* @throws ServiceUnavailable
6059
*/
6160
public function check(RequestInterface $request, ResponseInterface $response): array {
6261
try {
6362
$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION);
6463

64+
if (count($_COOKIE) > 0 && !$this->request->passesStrictCookieCheck() && $this->getShare()->getPassword() !== null) {
65+
throw new PreconditionFailed('Strict cookie check failed');
66+
}
67+
6568
$auth = new HTTP\Auth\Basic(
6669
$this->realm,
6770
$request,
@@ -77,6 +80,15 @@ public function check(RequestInterface $request, ResponseInterface $response): a
7780
return $this->checkToken();
7881
} catch (NotAuthenticated $e) {
7982
throw $e;
83+
} catch (PreconditionFailed $e) {
84+
$response->setHeader(
85+
'Location',
86+
$this->urlGenerator->linkToRoute(
87+
'files_sharing.share.showShare',
88+
[ 'token' => $this->getToken() ],
89+
),
90+
);
91+
throw $e;
8092
} catch (\Exception $e) {
8193
$class = get_class($e);
8294
$msg = $e->getMessage();
@@ -87,7 +99,6 @@ public function check(RequestInterface $request, ResponseInterface $response): a
8799

88100
/**
89101
* Extract token from request url
90-
* @return string
91102
* @throws NotFound
92103
*/
93104
private function getToken(): string {
@@ -104,7 +115,7 @@ private function getToken(): string {
104115

105116
/**
106117
* Check token validity
107-
* @return array
118+
*
108119
* @throws NotFound
109120
* @throws NotAuthenticated
110121
*/
@@ -152,15 +163,13 @@ private function checkToken(): array {
152163
protected function validateUserPass($username, $password) {
153164
$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION);
154165

155-
$token = $this->getToken();
156166
try {
157-
$share = $this->shareManager->getShareByToken($token);
167+
$share = $this->getShare();
158168
} catch (ShareNotFound $e) {
159169
$this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress());
160170
return false;
161171
}
162172

163-
$this->share = $share;
164173
\OC_User::setIncognitoMode(true);
165174

166175
// check if the share is password protected
@@ -203,7 +212,13 @@ protected function validateUserPass($username, $password) {
203212
}
204213

205214
public function getShare(): IShare {
206-
assert($this->share !== null);
215+
$token = $this->getToken();
216+
217+
if ($this->share === null) {
218+
$share = $this->shareManager->getShareByToken($token);
219+
$this->share = $share;
220+
}
221+
207222
return $this->share;
208223
}
209224
}

‎apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@
1010
use OCA\DAV\Connector\Sabre\PublicAuth;
1111
use OCP\IRequest;
1212
use OCP\ISession;
13+
use OCP\IURLGenerator;
1314
use OCP\Security\Bruteforce\IThrottler;
1415
use OCP\Share\Exceptions\ShareNotFound;
1516
use OCP\Share\IManager;
1617
use OCP\Share\IShare;
18+
use PHPUnit\Framework\MockObject\MockObject;
1719
use Psr\Log\LoggerInterface;
1820

1921
/**
@@ -25,21 +27,15 @@
2527
*/
2628
class PublicAuthTest extends \Test\TestCase {
2729

28-
/** @var ISession|MockObject */
29-
private $session;
30-
/** @var IRequest|MockObject */
31-
private $request;
32-
/** @var IManager|MockObject */
33-
private $shareManager;
34-
/** @var PublicAuth */
35-
private $auth;
36-
/** @var IThrottler|MockObject */
37-
private $throttler;
38-
/** @var LoggerInterface|MockObject */
39-
private $logger;
40-
41-
/** @var string */
42-
private $oldUser;
30+
private ISession&MockObject $session;
31+
private IRequest&MockObject $request;
32+
private IManager&MockObject $shareManager;
33+
private PublicAuth $auth;
34+
private IThrottler&MockObject $throttler;
35+
private LoggerInterface&MockObject $logger;
36+
private IURLGenerator&MockObject $urlGenerator;
37+
38+
private string $oldUser;
4339

4440
protected function setUp(): void {
4541
parent::setUp();
@@ -49,13 +45,15 @@ protected function setUp(): void {
4945
$this->shareManager = $this->createMock(IManager::class);
5046
$this->throttler = $this->createMock(IThrottler::class);
5147
$this->logger = $this->createMock(LoggerInterface::class);
48+
$this->urlGenerator = $this->createMock(IURLGenerator::class);
5249

5350
$this->auth = new PublicAuth(
5451
$this->request,
5552
$this->shareManager,
5653
$this->session,
5754
$this->throttler,
5855
$this->logger,
56+
$this->urlGenerator,
5957
);
6058

6159
// Store current user
@@ -137,7 +135,7 @@ public function testCheckTokenAlreadyAuthenticated(): void {
137135

138136
$this->session->method('exists')->with('public_link_authenticated')->willReturn(true);
139137
$this->session->method('get')->with('public_link_authenticated')->willReturn('42');
140-
138+
141139
$result = $this->invokePrivate($this->auth, 'checkToken');
142140
$this->assertSame([true, 'principals/GX9HSGQrGE'], $result);
143141
}
@@ -158,7 +156,7 @@ public function testCheckTokenPasswordNotAuthenticated(): void {
158156
->willReturn($share);
159157

160158
$this->session->method('exists')->with('public_link_authenticated')->willReturn(false);
161-
159+
162160
$this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class);
163161
$this->invokePrivate($this->auth, 'checkToken');
164162
}
@@ -180,7 +178,7 @@ public function testCheckTokenPasswordAuthenticatedWrongShare(): void {
180178

181179
$this->session->method('exists')->with('public_link_authenticated')->willReturn(false);
182180
$this->session->method('get')->with('public_link_authenticated')->willReturn('43');
183-
181+
184182
$this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class);
185183
$this->invokePrivate($this->auth, 'checkToken');
186184
}

‎apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,7 @@ public function renderPage(IShare $share, string $token, string $path): Template
150150
$headerActions = [];
151151
if ($view !== 'public-file-drop' && !$share->getHideDownload()) {
152152
// The download URL is used for the "download" header action as well as in some cases for the direct link
153-
$downloadUrl = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.downloadShare', [
154-
'token' => $token,
155-
'filename' => ($shareNode instanceof File) ? $shareNode->getName() : null,
156-
]);
153+
$downloadUrl = $this->urlGenerator->getAbsoluteURL('/public.php/dav/files/' . $token . '/?accept=zip');
157154

158155
// If not a file drop, then add the download header action
159156
$headerActions[] = new SimpleMenuAction('download', $this->l10n->t('Download'), 'icon-download', $downloadUrl, 0, (string)$shareNode->getSize());

‎apps/files_sharing/tests/Controller/ShareControllerTest.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,12 @@ public function testShowShare(): void {
261261
['files_sharing.sharecontroller.showShare', ['token' => 'token'], 'shareUrl'],
262262
// this share is not an image to the default preview is used
263263
['files_sharing.PublicPreview.getPreview', ['x' => 256, 'y' => 256, 'file' => $share->getTarget(), 'token' => 'token'], 'previewUrl'],
264-
// for the direct link
265-
['files_sharing.sharecontroller.downloadShare', ['token' => 'token', 'filename' => $filename ], 'downloadUrl'],
264+
]);
265+
266+
$this->urlGenerator->expects($this->once())
267+
->method('getAbsoluteURL')
268+
->willReturnMap([
269+
['/public.php/dav/files/token/?accept=zip', 'downloadUrl'],
266270
]);
267271

268272
$this->previewManager->method('isMimeSupported')->with('text/plain')->willReturn(true);
@@ -552,8 +556,12 @@ public function testShowShareWithPrivateName(): void {
552556
['files_sharing.sharecontroller.showShare', ['token' => 'token'], 'shareUrl'],
553557
// this share is not an image to the default preview is used
554558
['files_sharing.PublicPreview.getPreview', ['x' => 256, 'y' => 256, 'file' => $share->getTarget(), 'token' => 'token'], 'previewUrl'],
555-
// for the direct link
556-
['files_sharing.sharecontroller.downloadShare', ['token' => 'token', 'filename' => $filename ], 'downloadUrl'],
559+
]);
560+
561+
$this->urlGenerator->expects($this->once())
562+
->method('getAbsoluteURL')
563+
->willReturnMap([
564+
['/public.php/dav/files/token/?accept=zip', 'downloadUrl'],
557565
]);
558566

559567
$this->previewManager->method('isMimeSupported')->with('text/plain')->willReturn(true);

‎cypress/e2e/files_sharing/public-share/header-menu.cy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ describe('files_sharing: Public share - header actions menu', { testIsolation: t
5353
cy.findByRole('menuitem', { name: 'Direct link' })
5454
.should('be.visible')
5555
.and('have.attr', 'href')
56-
.then((attribute) => expect(attribute).to.match(/^http:\/\/.+\/download$/))
56+
.then((attribute) => expect(attribute).to.match(new RegExp(`^${Cypress.env('baseUrl')}/public.php/dav/files/.+/?accept=zip$`)))
5757
// see menu closes on click
5858
cy.findByRole('menuitem', { name: 'Direct link' })
5959
.click()
@@ -188,7 +188,7 @@ describe('files_sharing: Public share - header actions menu', { testIsolation: t
188188
cy.findByRole('menuitem', { name: 'Direct link' })
189189
.should('be.visible')
190190
.and('have.attr', 'href')
191-
.then((attribute) => expect(attribute).to.match(/^http:\/\/.+\/download$/))
191+
.then((attribute) => expect(attribute).to.match(new RegExp(`^${Cypress.env('baseUrl')}/public.php/dav/files/.+/?accept=zip$`)))
192192
// See remote share works
193193
cy.findByRole('menuitem', { name: /Add to your/i })
194194
.should('be.visible')

‎lib/base.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -548,10 +548,10 @@ private static function performSameSiteCookieProtection(\OCP\IConfig $config): v
548548
$processingScript = explode('/', $requestUri);
549549
$processingScript = $processingScript[count($processingScript) - 1];
550550

551-
// index.php routes are handled in the middleware
552-
// and cron.php does not need any authentication at all
553-
if ($processingScript === 'index.php'
554-
|| $processingScript === 'cron.php') {
551+
if ($processingScript === 'index.php' // index.php routes are handled in the middleware
552+
|| $processingScript === 'cron.php' // and cron.php does not need any authentication at all
553+
|| $processingScript === 'public.php' // For public.php, auth for password protected shares is done in the PublicAuth plugin
554+
) {
555555
return;
556556
}
557557

0 commit comments

Comments
 (0)