fix: make attachment path-containment guard separator-aware

Attachment::saveTo()'s escape guard compared paths with a bare
QString::startsWith(), which is not a path-boundary test: "/tmp/safe-evil"
textually starts with "/tmp/safe", so a sibling directory whose name merely
extends the target's name would incorrectly pass as contained within it.

Extract the check into Attachment::isPathInsideDirectory(), comparing
QDir::cleanPath()'d absolute paths and requiring an exact match or a prefix
ending at a '/' boundary. Not exploitable today since safeFilename() always
reduces the name to a bare basename before saveTo() builds the target, so
the guard is unreachable via saveTo()'s public interface; comments on both
now say so plainly instead of implying it is currently load-bearing.

Add pathInsideDirectoryRejectsSiblingPrefix, testing the guard directly
(independent of safeFilename(), which would mask a broken guard by never
producing an escaping path), and safeFilenameStripsPathComponents, testing
the sanitiser that actually stops traversal today.

4 files changed, 119 insertions, 9 deletions

diff --git a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md
index b264986..391bcb6 100644
--- a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md
+++ b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md
@@ -1000,6 +1000,7 @@ private slots:
     void missingFileIsReported();
     void hostileFilenameIsSanitised();
     void savedAttachmentMatchesBytes();
+    void safeFilenameStripsPathComponents();
 
 private:
     QString fixture(const QString &name) const
@@ -1384,12 +1385,26 @@ QString Attachment::saveTo(const QString &directory, QString *error) const
     const QDir dir(directory);
     const QString target = dir.absoluteFilePath(safeFilename());
 
-    // Belt and braces: confirm the resolved path really is inside directory,
-    // so a future change to safeFilename() cannot silently reintroduce escape.
-    const QString canonicalDir = QDir(directory).absolutePath();
-    if (!QFileInfo(target).absolutePath().startsWith(canonicalDir)) {
+    // Belt and braces, and currently UNREACHABLE through this function:
+    // safeFilename() above already reduces any name to a basename, so no
+    // caller-supplied filename can produce a target outside `directory`.
+    // The guard exists so that a future change which stops sanitising, or
+    // which lets a caller pass a subpath, still cannot escape. Do not write
+    // a test that drives saveTo() expecting a refusal: it cannot happen
+    // while safeFilename() runs first. Test safeFilename() instead, which
+    // is the control that actually stops traversal today.
+    //
+    // The comparison must be separator-aware. A bare startsWith() on the
+    // strings would accept "/tmp/safe-evil/x" as being inside "/tmp/safe",
+    // since one is a string prefix of the other with no path boundary
+    // between them. cleanPath() also resolves ".." before comparison rather
+    // than leaving it to be compared textually.
+    const QString cleanDir = QDir::cleanPath(QDir(directory).absolutePath());
+    const QString cleanTarget = QDir::cleanPath(target);
+    if (cleanTarget != cleanDir
+        && !cleanTarget.startsWith(cleanDir + QLatin1Char('/'))) {
         if (error)
-            *error = QStringLiteral("Refusing to write outside %1").arg(canonicalDir);
+            *error = QStringLiteral("Refusing to write outside %1").arg(cleanDir);
         return {};
     }
 
diff --git a/src/mimeparser.cpp b/src/mimeparser.cpp
index 7393a96..0c457c2 100644
--- a/src/mimeparser.cpp
+++ b/src/mimeparser.cpp
@@ -146,15 +146,34 @@ QString Attachment::safeFilename() const
     return name;
 }
 
+bool Attachment::isPathInsideDirectory(const QString &directory, const QString &candidatePath)
+{
+    // Compare candidatePath itself, not QFileInfo(candidatePath).absolutePath()
+    // (which would be its *parent* directory) -- candidatePath may itself be
+    // the directory being tested, as in the "is directory itself" case this
+    // function documents.
+    const QString canonicalDir = QDir::cleanPath(QDir(directory).absolutePath());
+    const QString canonicalTarget =
+        QDir::cleanPath(QFileInfo(candidatePath).absoluteFilePath());
+    return canonicalTarget == canonicalDir
+        || canonicalTarget.startsWith(canonicalDir + QLatin1Char('/'));
+}
+
 QString Attachment::saveTo(const QString &directory, QString *error) const
 {
     const QDir dir(directory);
     const QString target = dir.absoluteFilePath(safeFilename());
 
-    // Belt and braces: confirm the resolved path really is inside directory,
-    // so a future change to safeFilename() cannot silently reintroduce escape.
-    const QString canonicalDir = QDir(directory).absolutePath();
-    if (!QFileInfo(target).absolutePath().startsWith(canonicalDir)) {
+    // Defence-in-depth, not currently load-bearing: safeFilename() always
+    // reduces the name to a plain basename before target is built above, so
+    // this check cannot actually be failed via saveTo()'s public interface
+    // today (dir.absoluteFilePath(basename) can't escape dir). It exists so
+    // that a future change which stops sanitising the name, or which starts
+    // accepting a caller-supplied subpath instead of a bare filename, still
+    // cannot write outside directory. See Attachment::isPathInsideDirectory
+    // for the containment logic and its own direct tests.
+    if (!isPathInsideDirectory(directory, target)) {
+        const QString canonicalDir = QDir::cleanPath(QDir(directory).absolutePath());
         if (error)
             *error = QStringLiteral("Refusing to write outside %1").arg(canonicalDir);
         return {};
diff --git a/src/mimeparser.h b/src/mimeparser.h
index 27a239f..891478a 100644
--- a/src/mimeparser.h
+++ b/src/mimeparser.h
@@ -27,6 +27,24 @@ struct Attachment
     /// Writes the attachment into directory. Returns the full path written, or
     /// an empty string on failure with *error set.
     QString saveTo(const QString &directory, QString *error) const;
+
+    /// True if candidatePath (need not exist) is directory itself or strictly
+    /// beneath it, by path-boundary comparison after QDir::cleanPath on both
+    /// sides (so ".." segments are resolved rather than compared textually).
+    /// A bare QString::startsWith() is NOT sufficient here: it would let
+    /// "/tmp/safe-evil" pass against "/tmp/safe" since one string is a
+    /// textual prefix of the other despite being sibling directories.
+    ///
+    /// This is defence-in-depth, not currently load-bearing: saveTo() always
+    /// sanitises the name with safeFilename() first, which reduces it to a
+    /// plain basename, so no path reaching this check via saveTo()'s public
+    /// interface can actually fail it today. It exists for a future change
+    /// that stops sanitising, or that accepts a caller-supplied subpath.
+    /// Exposed as its own function so that guarantee can be tested directly,
+    /// independent of safeFilename() — a test driven purely through saveTo()
+    /// cannot exercise this comparison at all, since safeFilename() always
+    /// runs first and never produces a path that could fail it.
+    static bool isPathInsideDirectory(const QString &directory, const QString &candidatePath);
 };
 
 struct ParsedMessage
diff --git a/tests/test_mimeparser.cpp b/tests/test_mimeparser.cpp
index 72bdff7..030f23e 100644
--- a/tests/test_mimeparser.cpp
+++ b/tests/test_mimeparser.cpp
@@ -19,6 +19,8 @@ private slots:
     void missingFileIsReported();
     void hostileFilenameIsSanitised();
     void savedAttachmentMatchesBytes();
+    void safeFilenameStripsPathComponents();
+    void pathInsideDirectoryRejectsSiblingPrefix();
 
 private:
     QString fixture(const QString &name) const
@@ -167,5 +169,61 @@ void TestMimeParser::savedAttachmentMatchesBytes()
     QCOMPARE(f.readAll(), msg.attachments.first().data);
 }
 
+void TestMimeParser::safeFilenameStripsPathComponents()
+{
+    // This is the control that genuinely stops traversal: saveTo() always
+    // routes through safeFilename() first, so whatever this function
+    // guarantees is what actually protects a write to disk. Constructed by
+    // hand since these are adversarial names not tied to any fixture.
+    Attachment a;
+    a.mimeType = QStringLiteral("text/plain");
+    a.data = QByteArrayLiteral("x");
+
+    a.filename = QStringLiteral("../../../../tmp/pwned.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("pwned.txt"));
+
+    a.filename = QStringLiteral("../xyz-evil/x.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("x.txt"));
+
+    a.filename = QStringLiteral("..\\..\\windows\\evil.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("evil.txt"));
+
+    a.filename = QStringLiteral("plain.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("plain.txt"));
+
+    // Nothing usable remains: a generated name is produced instead. Assert
+    // its shape rather than an exact value, since it embeds a fresh UUID.
+    a.filename = QStringLiteral("..");
+    QString generated = a.safeFilename();
+    QVERIFY(!generated.isEmpty());
+    QVERIFY(generated != QStringLiteral(".."));
+    QVERIFY(!generated.contains(QLatin1Char('/')));
+
+    a.filename = QString();
+    generated = a.safeFilename();
+    QVERIFY(!generated.isEmpty());
+    QVERIFY(!generated.contains(QLatin1Char('/')));
+}
+
+void TestMimeParser::pathInsideDirectoryRejectsSiblingPrefix()
+{
+    // Direct test of the containment guard's own comparison, independent of
+    // safeFilename() (which always runs first inside saveTo() and would
+    // mask a broken guard, since it never produces an escaping path). This
+    // targets exactly the defect that was found: a plain string
+    // startsWith() incorrectly treats a sibling directory whose name merely
+    // extends the target's name (e.g. "/tmp/safe-evil") as contained within
+    // it (e.g. "/tmp/safe").
+    const QString base = QStringLiteral("/tmp/safe");
+
+    QVERIFY(Attachment::isPathInsideDirectory(base, base + QStringLiteral("/notes.txt")));
+    QVERIFY(Attachment::isPathInsideDirectory(base, base + QStringLiteral("/sub/notes.txt")));
+    QVERIFY(Attachment::isPathInsideDirectory(base, base));
+
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/tmp/safe-evil/x")));
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/tmp/safe/../etc/passwd")));
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/etc/passwd")));
+}
+
 QTEST_MAIN(TestMimeParser)
 #include "test_mimeparser.moc"