summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorElvis Angelaccio <[email protected]>2016-06-10 14:52:09 +0200
committerElvis Angelaccio <[email protected]>2016-06-10 14:56:50 +0200
commit41b251278be5508af3c479d330bd16f883499e08 (patch)
tree158511cd740efd23532c1c9885058ab710053b0b
parent3dccd03157056107fcfd9c161034877adee60751 (diff)
Add new jobs to preview/open a file
The goal of task T916 is to stop disabling the Part UI while extracting an entry. To do so, we need first to make sure that no race condition would occur if extracting two or more entries in parallel. A race condition might occur in Part::slotOpenExtractedEntry(), where m_openFileMode is a global member that could be accessed concurrently between different threads. We can stop using this variable if we introduce dedicated jobs for preview/opening of files. This way we can simply do a dynamic_cast on the local job variable, to check whether the user wants to open or open-with the file. Preview is totally unrelated so it can be moved into a different slot. We can also make sure that the PreviewJob deletes the temporary directory upon completion. Differential Revision: D1749
-rw-r--r--autotests/kerfuffle/data/archive-malicious.json24
-rw-r--r--autotests/kerfuffle/jobstest.cpp18
-rw-r--r--kerfuffle/archive_kerfuffle.cpp30
-rw-r--r--kerfuffle/archive_kerfuffle.h7
-rw-r--r--kerfuffle/jobs.cpp82
-rw-r--r--kerfuffle/jobs.h83
-rw-r--r--part/archivemodel.cpp24
-rw-r--r--part/archivemodel.h4
-rw-r--r--part/part.cpp83
-rw-r--r--part/part.h1
10 files changed, 308 insertions, 48 deletions
diff --git a/autotests/kerfuffle/data/archive-malicious.json b/autotests/kerfuffle/data/archive-malicious.json
new file mode 100644
index 0000000..d576a57
--- /dev/null
+++ b/autotests/kerfuffle/data/archive-malicious.json
@@ -0,0 +1,24 @@
+[
+ {
+ "FileName": "aDir/",
+ "IsDirectory": true
+ },
+ {
+ "FileName": "aDir/b.txt"
+ },
+ {
+ "FileName": "anotherDir/",
+ "IsDirectory": true
+ },
+ {
+ "FileName": "anotherDir/..",
+ "IsDirectory": true
+ },
+ {
+ "FileName": "anotherDir/../..",
+ "IsDirectory": true
+ },
+ {
+ "FileName": "anotherDir/../../file.txt"
+ }
+]
diff --git a/autotests/kerfuffle/jobstest.cpp b/autotests/kerfuffle/jobstest.cpp
index a6cd2e8..8effddc 100644
--- a/autotests/kerfuffle/jobstest.cpp
+++ b/autotests/kerfuffle/jobstest.cpp
@@ -51,6 +51,7 @@ private Q_SLOTS:
// ExtractJob-related tests
void testExtractJobAccessors();
+ void testTempExtractJob();
// DeleteJob-related tests
void testRemoveEntries_data();
@@ -249,6 +250,23 @@ void JobsTest::testExtractJobAccessors()
delete job;
}
+void JobsTest::testTempExtractJob()
+{
+ JSONArchiveInterface *iface = createArchiveInterface(QFINDTESTDATA("data/archive-malicious.json"));
+ PreviewJob *job = new PreviewJob(QStringLiteral("anotherDir/../../file.txt"), false, iface);
+
+ QVERIFY(job->validatedFilePath().endsWith(QLatin1String("anotherDir/file.txt")));
+ QVERIFY(job->extractionOptions()[QStringLiteral("PreservePaths")].toBool());
+
+ job->setAutoDelete(false);
+ startAndWaitForResult(job);
+
+ QVERIFY(job->validatedFilePath().endsWith(QLatin1String("anotherDir/file.txt")));
+ QVERIFY(job->extractionOptions()[QStringLiteral("PreservePaths")].toBool());
+
+ delete job;
+}
+
void JobsTest::testRemoveEntries_data()
{
QTest::addColumn<QString>("jsonArchive");
diff --git a/kerfuffle/archive_kerfuffle.cpp b/kerfuffle/archive_kerfuffle.cpp
index cebbd73..fbb5a29 100644
--- a/kerfuffle/archive_kerfuffle.cpp
+++ b/kerfuffle/archive_kerfuffle.cpp
@@ -366,6 +366,36 @@ ExtractJob* Archive::copyFiles(const QList<QVariant>& files, const QString& dest
return newJob;
}
+PreviewJob *Archive::preview(const QString &file)
+{
+ if (!isValid()) {
+ return Q_NULLPTR;
+ }
+
+ PreviewJob *job = new PreviewJob(file, (encryptionType() != Unencrypted), m_iface);
+ return job;
+}
+
+OpenJob *Archive::open(const QString &file)
+{
+ if (!isValid()) {
+ return Q_NULLPTR;
+ }
+
+ OpenJob *job = new OpenJob(file, (encryptionType() != Unencrypted), m_iface);
+ return job;
+}
+
+OpenWithJob *Archive::openWith(const QString &file)
+{
+ if (!isValid()) {
+ return Q_NULLPTR;
+ }
+
+ OpenWithJob *job = new OpenWithJob(file, (encryptionType() != Unencrypted), m_iface);
+ return job;
+}
+
void Archive::encrypt(const QString &password, bool encryptHeader)
{
if (!isValid()) {
diff --git a/kerfuffle/archive_kerfuffle.h b/kerfuffle/archive_kerfuffle.h
index 0219375..3cdacf3 100644
--- a/kerfuffle/archive_kerfuffle.h
+++ b/kerfuffle/archive_kerfuffle.h
@@ -47,7 +47,10 @@ class DeleteJob;
class AddJob;
class CommentJob;
class TestJob;
+class OpenJob;
+class OpenWithJob;
class Plugin;
+class PreviewJob;
class Query;
class ReadOnlyArchiveInterface;
@@ -219,6 +222,10 @@ public:
ExtractJob* copyFiles(const QList<QVariant> &files, const QString &destinationDir, const ExtractionOptions &options = ExtractionOptions());
+ PreviewJob* preview(const QString &file);
+ OpenJob* open(const QString &file);
+ OpenWithJob* openWith(const QString &file);
+
/**
* @param password The password to encrypt the archive with.
* @param encryptHeader Whether to encrypt also the list of files.
diff --git a/kerfuffle/jobs.cpp b/kerfuffle/jobs.cpp
index b61e126..c14e514 100644
--- a/kerfuffle/jobs.cpp
+++ b/kerfuffle/jobs.cpp
@@ -336,6 +336,88 @@ ExtractionOptions ExtractJob::extractionOptions() const
return m_options;
}
+TempExtractJob::TempExtractJob(const QString &file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface)
+ : Job(interface)
+ , m_file(file)
+ , m_passwordProtectedHint(passwordProtectedHint)
+{
+}
+
+
+QString TempExtractJob::validatedFilePath() const
+{
+ QString path = extractionDir() + QLatin1Char('/') + m_file;
+
+ // Make sure a maliciously crafted archive with parent folders named ".." do
+ // not cause the previewed file path to be located outside the temporary
+ // directory, resulting in a directory traversal issue.
+ path.remove(QStringLiteral("../"));
+
+ return path;
+}
+
+ExtractionOptions TempExtractJob::extractionOptions() const
+{
+ ExtractionOptions options;
+ options[QStringLiteral("PreservePaths")] = true;
+
+ if (m_passwordProtectedHint) {
+ options[QStringLiteral("PasswordProtectedHint")] = true;
+ }
+
+ return options;
+}
+
+void TempExtractJob::doWork()
+{
+ emit description(this, i18n("Extracting one file"));
+
+ connectToArchiveInterfaceSignals();
+
+ qCDebug(ARK) << "Extracting:" << m_file;
+
+ bool ret = archiveInterface()->copyFiles({ QVariant::fromValue(fileRootNodePair(m_file)) }, extractionDir(), extractionOptions());
+
+ if (!archiveInterface()->waitForFinishedSignal()) {
+ onFinished(ret);
+ }
+}
+
+PreviewJob::PreviewJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface)
+ : TempExtractJob(file, passwordProtectedHint, interface)
+{
+ qCDebug(ARK) << "PreviewJob started";
+}
+
+QString PreviewJob::extractionDir() const
+{
+ return m_tmpExtractDir.path();
+}
+
+OpenJob::OpenJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface)
+ : TempExtractJob(file, passwordProtectedHint, interface)
+{
+ qCDebug(ARK) << "OpenJob started";
+
+ m_tmpExtractDir = new QTemporaryDir();
+}
+
+QTemporaryDir *OpenJob::tempDir() const
+{
+ return m_tmpExtractDir;
+}
+
+QString OpenJob::extractionDir() const
+{
+ return m_tmpExtractDir->path();
+}
+
+OpenWithJob::OpenWithJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface)
+ : OpenJob(file, passwordProtectedHint, interface)
+{
+ qCDebug(ARK) << "OpenWithJob started";
+}
+
AddJob::AddJob(const QStringList& files, const CompressionOptions& options , ReadWriteArchiveInterface *interface)
: Job(interface)
, m_files(files)
diff --git a/kerfuffle/jobs.h b/kerfuffle/jobs.h
index 95f1d59..8633b87 100644
--- a/kerfuffle/jobs.h
+++ b/kerfuffle/jobs.h
@@ -34,10 +34,9 @@
#include "queries.h"
#include <KJob>
-#include <QList>
-#include <QVariant>
-#include <QString>
+
#include <QElapsedTimer>
+#include <QTemporaryDir>
namespace Kerfuffle
{
@@ -140,6 +139,84 @@ private:
ExtractionOptions m_options;
};
+/**
+ * Abstract base class for jobs that extract a single file to a temporary dir.
+ * It's not possible to pass extraction options and paths will be always preserved.
+ * The only option that the job needs to know is whether the file is password protected.
+ */
+class KERFUFFLE_EXPORT TempExtractJob : public Job
+{
+ Q_OBJECT
+
+public:
+ TempExtractJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface);
+
+ /**
+ * @return The absolute path of the extracted file.
+ * The path is validated in order to prevent directory traversal attacks.
+ */
+ QString validatedFilePath() const;
+
+ ExtractionOptions extractionOptions() const;
+
+public slots:
+ virtual void doWork() Q_DECL_OVERRIDE;
+
+private:
+ virtual QString extractionDir() const = 0;
+
+ QString m_file;
+ bool m_passwordProtectedHint;
+};
+
+/**
+ * This TempExtractJob can be used to preview a file.
+ * The temporary extraction directory will be deleted upon job's completion.
+ */
+class KERFUFFLE_EXPORT PreviewJob : public TempExtractJob
+{
+ Q_OBJECT
+
+public:
+ PreviewJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface);
+
+private:
+ QString extractionDir() const Q_DECL_OVERRIDE;
+
+ QTemporaryDir m_tmpExtractDir;
+};
+
+/**
+ * This TempExtractJob can be used to open a file in its dedicated application.
+ * For this reason, the temporary extraction directory will NOT be deleted upon job's completion.
+ */
+class KERFUFFLE_EXPORT OpenJob : public TempExtractJob
+{
+ Q_OBJECT
+
+public:
+ OpenJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface);
+
+ /**
+ * @return The temporary dir used for the extraction.
+ * It is safe to delete this pointer in order to remove the directory.
+ */
+ QTemporaryDir *tempDir() const;
+
+private:
+ QString extractionDir() const Q_DECL_OVERRIDE;
+
+ QTemporaryDir *m_tmpExtractDir;
+};
+
+class KERFUFFLE_EXPORT OpenWithJob : public OpenJob
+{
+ Q_OBJECT
+
+public:
+ OpenWithJob(const QString& file, bool passwordProtectedHint, ReadOnlyArchiveInterface *interface);
+};
+
class KERFUFFLE_EXPORT AddJob : public Job
{
Q_OBJECT
diff --git a/part/archivemodel.cpp b/part/archivemodel.cpp
index 1318cc3..b8aec0e 100644
--- a/part/archivemodel.cpp
+++ b/part/archivemodel.cpp
@@ -909,6 +909,30 @@ ExtractJob* ArchiveModel::extractFiles(const QList<QVariant>& files, const QStri
return newJob;
}
+Kerfuffle::PreviewJob *ArchiveModel::preview(const QString& file) const
+{
+ Q_ASSERT(m_archive);
+ PreviewJob *job = m_archive->preview(file);
+ connect(job, &Job::userQuery, this, &ArchiveModel::slotUserQuery);
+ return job;
+}
+
+OpenJob *ArchiveModel::open(const QString& file) const
+{
+ Q_ASSERT(m_archive);
+ OpenJob *job = m_archive->open(file);
+ connect(job, &Job::userQuery, this, &ArchiveModel::slotUserQuery);
+ return job;
+}
+
+OpenWithJob *ArchiveModel::openWith(const QString& file) const
+{
+ Q_ASSERT(m_archive);
+ OpenWithJob *job = m_archive->openWith(file);
+ connect(job, &Job::userQuery, this, &ArchiveModel::slotUserQuery);
+ return job;
+}
+
AddJob* ArchiveModel::addFiles(const QStringList & filenames, const CompressionOptions& options)
{
if (!m_archive) {
diff --git a/part/archivemodel.h b/part/archivemodel.h
index cfb1951..9a2b551 100644
--- a/part/archivemodel.h
+++ b/part/archivemodel.h
@@ -72,6 +72,10 @@ public:
Kerfuffle::ExtractJob* extractFile(const QVariant& fileName, const QString& destinationDir, const Kerfuffle::ExtractionOptions& options = Kerfuffle::ExtractionOptions()) const;
Kerfuffle::ExtractJob* extractFiles(const QList<QVariant>& files, const QString& destinationDir, const Kerfuffle::ExtractionOptions& options = Kerfuffle::ExtractionOptions()) const;
+ Kerfuffle::PreviewJob* preview(const QString& file) const;
+ Kerfuffle::OpenJob* open(const QString& file) const;
+ Kerfuffle::OpenWithJob* openWith(const QString& file) const;
+
Kerfuffle::AddJob* addFiles(const QStringList & paths, const Kerfuffle::CompressionOptions& options = Kerfuffle::CompressionOptions());
Kerfuffle::DeleteJob* deleteFiles(const QList<QVariant> & files);
diff --git a/part/part.cpp b/part/part.cpp
index a00dedd..c2a29b0 100644
--- a/part/part.cpp
+++ b/part/part.cpp
@@ -856,37 +856,36 @@ void Part::slotOpenEntry(int mode)
// Extract the entry.
if (!entry.isEmpty()) {
- Kerfuffle::ExtractionOptions options;
- options[QStringLiteral("PreservePaths")] = true;
- m_tmpOpenDirList.append(new QTemporaryDir);
m_openFileMode = static_cast<OpenFileMode>(mode);
- ExtractJob *job = m_model->extractFile(entry[InternalID], m_tmpOpenDirList.last()->path(), options);
+ KJob *job = Q_NULLPTR;
+
+ if (m_openFileMode == Preview) {
+ job = m_model->preview(entry[InternalID].toString());
+ connect(job, &KJob::result, this, &Part::slotPreviewExtractedEntry);
+ } else {
+ const QString file = entry[InternalID].toString();
+ job = (m_openFileMode == OpenFile) ? m_model->open(file) : m_model->openWith(file);
+ connect(job, &KJob::result, this, &Part::slotOpenExtractedEntry);
+ }
registerJob(job);
- connect(job, &KJob::result,
- this, &Part::slotOpenExtractedEntry);
job->start();
}
}
void Part::slotOpenExtractedEntry(KJob *job)
{
- // FIXME: the error checking here isn't really working
- // if there's an error or an overwrite dialog,
- // the preview dialog will be launched anyway
if (!job->error()) {
- const ArchiveEntry& entry =
- m_model->entryForIndex(m_view->selectionModel()->currentIndex());
- ExtractJob *extractJob = qobject_cast<ExtractJob*>(job);
- Q_ASSERT(extractJob);
- QString fullName = extractJob->destinationDirectory() + QLatin1Char('/') + entry[FileName].toString();
+ OpenJob *openJob = qobject_cast<OpenJob*>(job);
+ Q_ASSERT(openJob);
+
+ // Since the user could modify the file (unlike the Preview case),
+ // we'll need to manually delete the temp dir in the Part destructor.
+ m_tmpOpenDirList << openJob->tempDir();
- // Make sure a maliciously crafted archive with parent folders named ".." do
- // not cause the previewed file path to be located outside the temporary
- // directory, resulting in a directory traversal issue.
- fullName.remove(QStringLiteral("../"));
+ const QString fullName = openJob->validatedFilePath();
bool isWritable = m_model->archive() && !m_model->archive()->isReadOnly();
@@ -896,39 +895,33 @@ void Part::slotOpenExtractedEntry(KJob *job)
QFile::setPermissions(fullName, QFileDevice::ReadOwner | QFileDevice::ReadGroup | QFileDevice::ReadOther);
}
- // TODO: get rid of m_openFileMode by extending ExtractJob with a
- // Preview/OpenJob. This would prevent race conditions if we ever stop
- // disabling the whole UI while extracting a file to preview it.
- if (m_openFileMode != Preview && isWritable) {
+ if (isWritable) {
m_fileWatcher = new QFileSystemWatcher;
connect(m_fileWatcher, &QFileSystemWatcher::fileChanged, this, &Part::slotWatchedFileModified);
+ m_fileWatcher->addPath(fullName);
}
- QMimeDatabase db;
- switch (m_openFileMode) {
-
- case Preview:
- ArkViewer::view(fullName);
- break;
- case OpenFile:
- KRun::runUrl(QUrl::fromUserInput(fullName,
- QString(),
- QUrl::AssumeLocalFile),
- db.mimeTypeForFile(fullName).name(),
+ if (qobject_cast<OpenWithJob*>(job)) {
+ const QList<QUrl> urls = {QUrl::fromUserInput(fullName, QString(), QUrl::AssumeLocalFile)};
+ KRun::displayOpenWithDialog(urls, widget());
+ } else {
+ KRun::runUrl(QUrl::fromUserInput(fullName, QString(), QUrl::AssumeLocalFile),
+ QMimeDatabase().mimeTypeForFile(fullName).name(),
widget());
- break;
- case OpenFileWith:
- QList<QUrl> list;
- list.append(QUrl::fromUserInput(fullName,
- QString(),
- QUrl::AssumeLocalFile));
- KRun::displayOpenWithDialog(list, widget(), false);
-
- break;
- }
- if (m_openFileMode != Preview && isWritable) {
- m_fileWatcher->addPath(fullName);
}
+ } else if (job->error() != KJob::KilledJobError) {
+ KMessageBox::error(widget(), job->errorString());
+ }
+ setReadyGui();
+}
+
+void Part::slotPreviewExtractedEntry(KJob *job)
+{
+ if (!job->error()) {
+ PreviewJob *previewJob = qobject_cast<PreviewJob*>(job);
+ Q_ASSERT(previewJob);
+
+ ArkViewer::view(previewJob->validatedFilePath());
} else if (job->error() != KJob::KilledJobError) {
KMessageBox::error(widget(), job->errorString());
diff --git a/part/part.h b/part/part.h
index 20953e0..aa52074 100644
--- a/part/part.h
+++ b/part/part.h
@@ -96,6 +96,7 @@ private slots:
void slotLoadingStarted();
void slotLoadingFinished(KJob *job);
void slotOpenExtractedEntry(KJob*);
+ void slotPreviewExtractedEntry(KJob* job);
void slotOpenEntry(int mode);
void slotError(const QString& errorMessage, const QString& details);
void slotExtractArchive();