Skip to content
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.

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

fix(files_sharing): show proper share not found error message #46967

Merged
merged 1 commit into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions apps/files_sharing/lib/Controller/ShareController.php
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,11 @@
} catch (ShareNotFound $e) {
// The share does not exists, we do not emit an ShareLinkAccessedEvent
$this->emitAccessShareHook($this->getToken(), 404, 'Share not found');
throw new NotFoundException();
throw new NotFoundException($this->l10n->t('This share does not exist or is no longer available'));
}

if (!$this->validateShare($share)) {
throw new NotFoundException();
throw new NotFoundException($this->l10n->t('This share does not exist or is no longer available'));
}

$shareNode = $share->getNode();
Expand All @@ -309,15 +309,15 @@
} catch (NotFoundException $e) {
$this->emitAccessShareHook($share, 404, 'Share not found');
$this->emitShareAccessEvent($share, ShareController::SHARE_ACCESS, 404, 'Share not found');
throw new NotFoundException();
throw new NotFoundException($this->l10n->t('This share does not exist or is no longer available'));
}

// We can't get the path of a file share
try {
if ($shareNode instanceof \OCP\Files\File && $path !== '') {
$this->emitAccessShareHook($share, 404, 'Share not found');
$this->emitShareAccessEvent($share, self::SHARE_ACCESS, 404, 'Share not found');
throw new NotFoundException();
throw new NotFoundException($this->l10n->t('This share does not exist or is no longer available'));
}
} catch (\Exception $e) {
$this->emitAccessShareHook($share, 404, 'Share not found');
Expand Down Expand Up @@ -427,7 +427,7 @@
&& !isset($downloadStartSecret[32])
&& preg_match('!^[a-zA-Z0-9]+$!', $downloadStartSecret) === 1) {
// FIXME: set on the response once we use an actual app framework response
setcookie('ocDownloadStarted', $downloadStartSecret, time() + 20, '/');

Check failure on line 430 in apps/files_sharing/lib/Controller/ShareController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedCookie

apps/files_sharing/lib/Controller/ShareController.php:430:35: TaintedCookie: Detected tainted cookie (see https://psalm.dev/257)
}

$this->emitAccessShareHook($share);
Expand Down
23 changes: 23 additions & 0 deletions apps/files_sharing/templates/sharenotfound.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/

use OCP\IURLGenerator;
use OCP\Server;

$urlGenerator = Server::get(IURLGenerator::class);
?>
<div class="body-login-container update">
<div>
<svg xmlns="http://www.w3.org/2000/svg" height="70" viewBox="0 -960 960 960" width="70">
<path fill="currentColor" d="m674-456-50-50 69-70-69-69 50-51 70 70 69-70 51 51-70 69 70 70-51 50-69-69-70 69Zm-290-24q-60 0-102-42t-42-102q0-60 42-102t102-42q60 0 102 42t42 102q0 60-42 102t-102 42ZM96-192v-92q0-26 12.5-47.5T143-366q55-32 116-49t125-17q64 0 125 17t116 49q22 13 34.5 34.5T672-284v92H96Z"/>
</svg>
</div>
<h2><?php p($l->t('Share not found')); ?></h2>
<p class="infogroup"><?php p($_['message'] ?: $l->t('This share does not exist or is no longer available')); ?></p>
<p><a class="button primary" href="<?php p($urlGenerator->linkTo('', 'index.php')) ?>">
<?php p($l->t('Back to %s', [$theme->getName()])); ?>
</a></p>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
new OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware(
$c->get(IRequest::class),
$c->get(ISession::class),
$c->get(\OCP\IConfig::class),
$c->get(IConfig::class),
$c->get(IThrottler::class)
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
namespace OC\AppFramework\Middleware\PublicShare;

use OC\AppFramework\Middleware\PublicShare\Exceptions\NeedAuthenticationException;
use OCA\Files_Sharing\AppInfo\Application;
use OCP\AppFramework\AuthPublicShareController;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\PublicShareController;
use OCP\Files\NotFoundException;
Expand All @@ -17,23 +19,13 @@
use OCP\Security\Bruteforce\IThrottler;

class PublicShareMiddleware extends Middleware {
/** @var IRequest */
private $request;

/** @var ISession */
private $session;

/** @var IConfig */
private $config;

/** @var IThrottler */
private $throttler;

public function __construct(IRequest $request, ISession $session, IConfig $config, IThrottler $throttler) {
$this->request = $request;
$this->session = $session;
$this->config = $config;
$this->throttler = $throttler;
public function __construct(
private IRequest $request,
private ISession $session,
private IConfig $config,
private IThrottler $throttler
) {
}

public function beforeController($controller, $methodName) {
Expand Down Expand Up @@ -92,7 +84,9 @@ public function afterException($controller, $methodName, \Exception $exception)
}

if ($exception instanceof NotFoundException) {
return new NotFoundResponse();
return new TemplateResponse(Application::APP_ID, 'sharenotfound', [
'message' => $exception->getMessage(),
], 'guest', Http::STATUS_NOT_FOUND);
}

if ($controller instanceof AuthPublicShareController && $exception instanceof NeedAuthenticationException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
use OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware;
use OCP\AppFramework\AuthPublicShareController;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\PublicShareController;
use OCP\Files\NotFoundException;
use OCP\IConfig;
Expand Down Expand Up @@ -236,7 +237,8 @@ public function testAfterExceptionPublicShareControllerNotFoundException() {
$exception = new NotFoundException();

$result = $this->middleware->afterException($controller, 'method', $exception);
$this->assertInstanceOf(NotFoundResponse::class, $result);
$this->assertInstanceOf(TemplateResponse::class, $result);
$this->assertEquals($result->getStatus(), Http::STATUS_NOT_FOUND);
}

public function testAfterExceptionPublicShareController() {
Expand Down
Loading