From e38368d4285bd9ab2823ce020a3ea6fbde749197 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 24 May 2024 00:46:50 +0800 Subject: [PATCH] Move token and relpath validity checks to edit locally verification job Signed-off-by: Claudio Cambra --- src/gui/editlocallyjob.cpp | 68 ++--------------------- src/gui/editlocallyjob.h | 4 +- src/gui/editlocallyverificationjob.cpp | 74 +++++++++++++++++++++++--- src/gui/editlocallyverificationjob.h | 3 ++ 4 files changed, 75 insertions(+), 74 deletions(-) diff --git a/src/gui/editlocallyjob.cpp b/src/gui/editlocallyjob.cpp index e2d7f5639..8f0a90674 100644 --- a/src/gui/editlocallyjob.cpp +++ b/src/gui/editlocallyjob.cpp @@ -44,9 +44,9 @@ void EditLocallyJob::startSetup() { if (_token.isEmpty() || _relPath.isEmpty() || _userId.isEmpty()) { qCWarning(lcEditLocallyJob) << "Could not start setup." - << "token:" << _token - << "relPath:" << _relPath - << "userId" << _userId; + << "token:" << _token + << "relPath:" << _relPath + << "userId" << _userId; return; } @@ -54,35 +54,14 @@ void EditLocallyJob::startSetup() // verified the token Systray::instance()->createEditFileLocallyLoadingDialog({}); - // We check the input data locally first, without modifying any state or - // showing any potentially misleading data to the user - if (!isTokenValid(_token)) { - qCWarning(lcEditLocallyJob) << "Edit locally request is missing a valid token, will not open file. " - << "Token received was:" << _token; - showError(tr("Invalid token received."), tr("Please try again.")); - return; - } - - if (!isRelPathValid(_relPath)) { - qCWarning(lcEditLocallyJob) << "Provided relPath was:" << _relPath << "which is not canonical."; - showError(tr("Invalid file path was provided."), tr("Please try again.")); - return; - } - _accountState = AccountManager::instance()->accountFromUserId(_userId); - if (!_accountState) { - qCWarning(lcEditLocallyJob) << "Could not find an account " << _userId << " to edit file " << _relPath << " locally."; - showError(tr("Could not find an account for local editing."), tr("Please try again.")); - return; - } - // We now ask the server to verify the token, before we again modify any // state or look at local files - startTokenRemoteCheck(); + startCheck(); } -void EditLocallyJob::startTokenRemoteCheck() +void EditLocallyJob::startCheck() { const auto verificationJob = new EditLocallyVerificationJob(_accountState, _relPath, _token); connect(verificationJob, &EditLocallyVerificationJob::error, this, &EditLocallyJob::showError); @@ -300,43 +279,6 @@ const QString EditLocallyJob::getRelativePathParent() const return QStringLiteral("/"); } -bool EditLocallyJob::isTokenValid(const QString &token) -{ - if (token.isEmpty()) { - return false; - } - - // Token is an alphanumeric string 128 chars long. - // Ensure that is what we received and what we are sending to the server. - static const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$"); - const auto regexMatch = tokenRegex.match(token); - - return regexMatch.hasMatch(); -} - -bool EditLocallyJob::isRelPathValid(const QString &relPath) -{ - if (relPath.isEmpty()) { - return false; - } - - // We want to check that the path is canonical and not relative - // (i.e. that it doesn't contain ../../) but we always receive - // a relative path, so let's make it absolute by prepending a - // slash - const auto slashPrefixedPath = prefixSlashToPath(relPath); - - // Let's check that the filepath is canonical, and that the request - // contains no funny behaviour regarding paths - const auto cleanedPath = QDir::cleanPath(slashPrefixedPath); - - if (cleanedPath != slashPrefixedPath) { - return false; - } - - return true; -} - OCC::Folder *EditLocallyJob::findFolderForFile(const QString &relPath, const QString &userId) { if (relPath.isEmpty()) { diff --git a/src/gui/editlocallyjob.h b/src/gui/editlocallyjob.h index 84bb11123..32f096ab8 100644 --- a/src/gui/editlocallyjob.h +++ b/src/gui/editlocallyjob.h @@ -37,8 +37,6 @@ public: const QString &token, QObject *parent = nullptr); - [[nodiscard]] static bool isTokenValid(const QString &token); - [[nodiscard]] static bool isRelPathValid(const QString &relPath); [[nodiscard]] static OCC::Folder *findFolderForFile(const QString &relPath, const QString &userId); [[nodiscard]] static QString prefixSlashToPath(const QString &path); @@ -55,7 +53,7 @@ private slots: void fetchRemoteFileParentInfo(); void startSyncBeforeOpening(); - void startTokenRemoteCheck(); + void startCheck(); void proceedWithSetup(); void findAfolderAndConstructPaths(); diff --git a/src/gui/editlocallyverificationjob.cpp b/src/gui/editlocallyverificationjob.cpp index 861c4c09d..8331a8dda 100644 --- a/src/gui/editlocallyverificationjob.cpp +++ b/src/gui/editlocallyverificationjob.cpp @@ -14,11 +14,21 @@ #include "editlocallyverificationjob.h" +#include #include #include #include "libsync/networkjobs.h" +namespace { + +QString prefixSlashToPath(const QString &path) +{ + return path.startsWith('/') ? path : '/' + path; +} + +} + namespace OCC { @@ -35,16 +45,64 @@ EditLocallyVerificationJob::EditLocallyVerificationJob(const AccountStatePtr &ac { } +bool EditLocallyVerificationJob::isTokenValid(const QString &token) +{ + if (token.isEmpty()) { + return false; + } + + // Token is an alphanumeric string 128 chars long. + // Ensure that is what we received and what we are sending to the server. + static const QRegularExpression tokenRegex("^[a-zA-Z0-9]{128}$"); + const auto regexMatch = tokenRegex.match(token); + + return regexMatch.hasMatch(); +} + +bool EditLocallyVerificationJob::isRelPathValid(const QString &relPath) +{ + if (relPath.isEmpty()) { + return false; + } + + // We want to check that the path is canonical and not relative + // (i.e. that it doesn't contain ../../) but we always receive + // a relative path, so let's make it absolute by prepending a + // slash + const auto slashPrefixedPath = prefixSlashToPath(relPath); + + // Let's check that the filepath is canonical, and that the request + // contains no funny behaviour regarding paths + const auto cleanedPath = QDir::cleanPath(slashPrefixedPath); + + if (cleanedPath != slashPrefixedPath) { + return false; + } + + return true; +} + void EditLocallyVerificationJob::start() { - if (!_accountState || _relPath.isEmpty() || _token.isEmpty()) { - qCWarning(lcEditLocallyVerificationJob) << "Could not start token check." - << "accountState:" << _accountState - << "relPath:" << _relPath - << "token:" << _token; + // We check the input data locally first, without modifying any state or + // showing any potentially misleading data to the user + if (!isTokenValid(_token)) { + qCWarning(lcEditLocallyVerificationJob) << "Edit locally request is missing a valid token, will not open file. " + << "Token received was:" << _token; + emit error(tr("Invalid token received."), tr("Please try again.")); + return; + } - emit error(tr("Could not start editing locally."), - tr("An error occurred trying to verify the request to edit locally.")); + if (!isRelPathValid(_relPath)) { + qCWarning(lcEditLocallyVerificationJob) << "Provided relPath was:" << _relPath + << "which is not canonical."; + emit error(tr("Invalid file path was provided."), tr("Please try again.")); + return; + } + + if (!_accountState) { + qCWarning(lcEditLocallyVerificationJob) << "No account found to edit file " << _relPath << " locally."; + emit error(tr("Could not find an account for local editing."), tr("Please try again.")); return; } @@ -52,7 +110,7 @@ void EditLocallyVerificationJob::start() const auto encodedRelPath = QUrl::toPercentEncoding(_relPath); // Sanitise the relPath const auto checkTokenJob = new SimpleApiJob(_accountState->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(encodedToken)); - const auto slashedPath = encodedRelPath.startsWith('/') ? encodedRelPath : '/' + encodedRelPath; + const auto slashedPath = prefixSlashToPath(encodedRelPath); QUrlQuery params; params.addQueryItem(QStringLiteral("path"), slashedPath); diff --git a/src/gui/editlocallyverificationjob.h b/src/gui/editlocallyverificationjob.h index a7ca22429..1bd5b1395 100644 --- a/src/gui/editlocallyverificationjob.h +++ b/src/gui/editlocallyverificationjob.h @@ -30,6 +30,9 @@ public: const QString &token, QObject *const parent = nullptr); + [[nodiscard]] static bool isTokenValid(const QString &token); + [[nodiscard]] static bool isRelPathValid(const QString &relPath); + signals: void error(const QString &message, const QString &informativeText); void finished(); -- 2.30.2