diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-21 10:12:26 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-21 10:12:26 +0200 |
| commit | 6488810c779970094b86079c8688d83d8529fab0 (patch) | |
| tree | e9ae7fc96d82cb6728d95c99958a947c18748788 | |
| parent | 9b1b371856c148dc668587254c51134ca9d4b605 (diff) | |
| download | qtmaildir-6488810c779970094b86079c8688d83d8529fab0.tar.gz qtmaildir-6488810c779970094b86079c8688d83d8529fab0.zip | |
feat(compose): hand outgoing mail to the send command, item 123
MessageSender runs the configured command with the message on stdin and
judges the result by its exit status alone. Nothing here waits on the
event loop, so a send does not block the GUI thread; a 1.6MB payload was
probed through a reading stub without deadlocking the pipe buffer.
The command is split and passed to QProcess as a program and an argument
list, never through a shell. A test asserts that by giving the command
shell metacharacters and checking that the marker file a shell would have
created does not exist, so the property fails a mutation rather than
resting on a comment.
Four corrections to the plan's draft. splitCommand handles double quotes
only, so a single-quoted argument splits wrongly and the header now says
so. A crashing command delivers finished(11, CrashExit) and would have
been reported as "exited with status 11", so a crash branch was added. A
command that exits without draining a large stdin emits WriteError before
finished(), which the draft handled correctly and by luck, untested. And
an empty send_command is checked after trimming.
Two contract gaps found in review, both about what this class promises
rather than what it does.
The exactly-once guarantee covers the EMIT, not what a caller receives: a
long-lived sender plus a connect() inside each send accumulates
receivers, and the second result then runs the first send's lambda too,
filing a sent copy of the wrong message. The header now scopes the
promise and requires Qt::SingleShotConnection. The plan's Task 11 call
site already had that flag, sixty-nine lines below the connect and
outside anything a reader would see, so the plan gained a note where
someone retyping it will read it.
And destruction mid-send killed the command with no report, announced
only by a Qt warning: a live SMTP conversation abandoned, possibly
partially delivered, while the user believes it was cancelled. The
destructor now closes stdin, waits a bounded five seconds, and only then
kills. It emits nothing either way, because the outcome after a kill is
genuinely unknown and reporting "not sent" for a message that may have
gone out is the mailsync.sh mistake pointing the other way. Claiming
m_reported before kill() is what makes that true, since kill() delivers
finished(CrashExit), which would otherwise emit exactly that untruth.
No timeout on the send itself: killing a slow but working send is worse
than waiting. Task 10 owns the popup, and deliberately offers no cancel
after commit, so this class promises none either.
Also refreshes the translations Task 5 left out. That gap was invisible
because test_translations builds its rows from the .ts file, so a string
that never entered it is never asserted on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QP2g3b3kuLx6AYFCNEz6UR
| -rw-r--r-- | docs/superpowers/plans/2026-08-20-compose-and-send.md | 28 | ||||
| -rw-r--r-- | src/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | src/messagesender.cpp | 197 | ||||
| -rw-r--r-- | src/messagesender.h | 164 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | tests/test_messagesender.cpp | 532 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 27 |
7 files changed, 950 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-08-20-compose-and-send.md b/docs/superpowers/plans/2026-08-20-compose-and-send.md index e7eddfa..7d5f6f1 100644 --- a/docs/superpowers/plans/2026-08-20-compose-and-send.md +++ b/docs/superpowers/plans/2026-08-20-compose-and-send.md @@ -4008,6 +4008,20 @@ private: `src/composewindow.cpp`. The full file is long; these are the parts that carry decisions, and the rest is ordinary widget assembly. +**One thing in this block is load-bearing and easy to drop while retyping it: +the `Qt::SingleShotConnection` on the `MessageSender::finished` connect inside +the `committed` handler.** `m_sender` is a long-lived member, so a plain +`connect()` beside a `send()` call leaks a receiver per send and the second +result runs every earlier lambda, each still holding an earlier message's bytes +by value: a sent copy of the wrong message, and `accept()` on a destroyed +dialog. `MessageSender`'s own once-only guard cannot help, because that guards +the emit and this is one emit reaching many receivers. The header for +`MessageSender::finished` states the rule and +`test_messagesender.cpp::aPerSendConnectionMustBeSingleShot` measures it (3 +deliveries for 2 sends without the flag, 2 with it). Noted here because the +plan's code blocks are drafts and this is the line whose absence still +compiles, still runs, and is wrong only on the second send. + ```cpp #include "composewindow.h" @@ -4169,6 +4183,20 @@ void ComposeWindow::send() connect(dialog, &SendDialog::committed, this, [this, dialog, built, account]() { m_sender->send(account.sendCommand, built.bytes); + // Qt::SingleShotConnection IS REQUIRED HERE, and this line is the + // correction of a defect that was in this plan's draft (found while + // building Task 6, 2026-08-21). m_sender is a long-lived member, so a + // bare connect() beside each send() accumulates a permanent receiver + // per send. Send, fail, correct the recipient, send again, and the + // second result runs BOTH lambdas: the first still holds the FIRST + // message's `built` and `account` by value, so it files a sent copy of + // the wrong message and calls accept() on a dialog it already + // deleteLater()'d. MessageSender's m_reported guard cannot prevent + // this: it collapses two QProcess signals into one emit, and this is + // one emit reaching many receivers. Measured in + // test_messagesender.cpp::aPerSendConnectionMustBeSingleShot, where + // the bare shape delivers 3 results for 2 sends and the single-shot + // shape delivers 2. connect(m_sender, &MessageSender::finished, this, [this, dialog, built, account](bool sent, const QString &error) { if (!sent) { diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a4d7c55..eac2fab 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -14,6 +14,7 @@ add_library(qtmaildir_lib STATIC notmuchworker.cpp maildirname.cpp draftstore.cpp + messagesender.cpp tagchip.cpp tagcolors.cpp savequerydialog.cpp diff --git a/src/messagesender.cpp b/src/messagesender.cpp new file mode 100644 index 0000000..f336028 --- /dev/null +++ b/src/messagesender.cpp @@ -0,0 +1,197 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "messagesender.h" + +MessageSender::MessageSender(QObject *parent) + : QObject(parent) +{ + // Separate channels, unlike MailSync's MergedChannels: there is no log + // pane to fill here, and stderr alone is what a failure has to report. + // Merging them would put the command's ordinary chatter into the error + // message shown for a rejected send. + m_process.setProcessChannelMode(QProcess::SeparateChannels); + + connect(&m_process, &QProcess::finished, + this, &MessageSender::handleFinished); + connect(&m_process, &QProcess::errorOccurred, + this, &MessageSender::handleError); +} + +MessageSender::~MessageSender() +{ + if (m_process.state() == QProcess::NotRunning) + return; + + // A send is a live SMTP conversation and abandoning one has a genuinely + // unknown outcome, so give the command a bounded chance to finish rather + // than killing it outright. Measured: without this, a one-second command + // destroyed 100ms in is killed and its work does not complete, announced + // only by a Qt warning on stderr. With it, the same command completes and + // the destructor costs the ~1s the command actually needed. + // + // The write channel is closed first because the command may still be + // reading: a command blocked on stdin would otherwise never reach EOF and + // would burn the whole timeout for no reason. + m_process.closeWriteChannel(); + if (m_process.waitForFinished(kShutdownWaitMs)) + return; + + // Still running. A destructor cannot block a quitting application forever, + // so the process is killed deliberately here rather than by ~QProcess. + // + // NOTHING IS EMITTED. The outcome after a kill is unknown: the message may + // have been fully delivered, partially delivered, or not sent at all, and + // this class reports two outcomes only. Emitting finished(false, ...) would + // report "not sent" for a message that may well have been, which is the + // mailsync.sh mistake pointing the other way. Emitting finished(true, ...) + // would be worse. A caller that must know has to keep this object alive + // until finished() arrives. + // + // Claiming the report BEFORE the kill is what makes that true, and it is + // not optional: kill() makes QProcess deliver finished(CrashExit), which + // reaches handleFinished and would emit exactly the untruthful "not sent" + // this comment forbids. Measured, by a test that failed against the + // version without these two lines. This is also the one place m_reported + // does live work, rather than the defence-in-depth it is on the signal + // paths. + m_reported = true; + m_process.kill(); + m_process.waitForFinished(kShutdownWaitMs); +} + +bool MessageSender::isRunning() const +{ + return m_process.state() != QProcess::NotRunning; +} + +bool MessageSender::send(const QString &command, const QByteArray &bytes) +{ + if (command.trimmed().isEmpty() || isRunning()) + return false; + + // splitCommand gives an argument list; running through a shell would make + // every recipient address, display name and config value a potential + // injection point. QProcess hands the list to execve, so a `;` or a + // `$(...)` in the configured command is a literal argument with nothing to + // interpret it. Note that splitCommand strips DOUBLE quotes only. + // + // Nothing from the message reaches the argument list at all: the command + // reads its recipients from the message's own headers, which is what `-t` + // means in the documented example. + const QStringList parts = QProcess::splitCommand(command); + if (parts.isEmpty()) + return false; + + m_command = command; + m_reported = false; + + m_process.setProgram(parts.first()); + m_process.setArguments(parts.mid(1)); + + // Deliberately no waitForStarted(): this runs on the GUI thread and the + // interface must stay responsive while a send is in flight. A failed + // launch arrives via errorOccurred(FailedToStart) instead, which QProcess + // emits INSTEAD OF finished() rather than before it (measured). + m_process.start(); + + // Written after start() and before the process has necessarily launched, + // which is safe: QProcess buffers and drains as the reader consumes. + // Measured with a 320KB payload against a `cat` stub, which arrived + // byte-identical, so a message with an attachment does not deadlock on the + // 64KB pipe buffer. + m_process.write(bytes); + + // The message goes on stdin and the channel is closed, so a command + // reading to EOF terminates. Without closeWriteChannel() a command like + // `cat` waits forever and the popup never leaves its Sending stage. + m_process.closeWriteChannel(); + + return true; +} + +void MessageSender::handleFinished(int exitCode, QProcess::ExitStatus status) +{ + // errorOccurred may already have reported this failure. Reporting twice + // would close the popup and then act on a second result. + // + // This guard IS load-bearing, on exactly one path: the destructor sets + // m_reported before kill(), because kill() makes QProcess deliver + // finished(CrashExit) and without the flag this handler would emit a + // "not sent" for a message whose fate is genuinely unknown. A test fails + // against its removal. + // + // On the two signal paths it is defence in depth and currently cannot + // fire: handleError is filtered to FailedToStart, and FailedToStart is + // never followed by finished() (measured). An instrumented run of the + // whole suite recorded zero hits there, including on the crash and + // write-error paths that DO emit both signals. It stays because the day + // someone widens handleError to report another error, the double report is + // silent and costs a duplicate sent copy. + if (m_reported) + return; + m_reported = true; + + // The exit status is the only authority. Nothing is inferred from what the + // command printed: mailsync.sh records what a wrong answer here costs, and + // a send reported as succeeding files a sent copy for a message that never + // left the machine. + const bool sent = status == QProcess::NormalExit && exitCode == 0; + if (sent) { + emit finished(true, QString()); + return; + } + + // Exit 75 is deliberately NOT special. See the header. + QString error = QString::fromUtf8(m_process.readAllStandardError()).trimmed(); + if (error.isEmpty()) { + // A failure with a blank explanation gives the user nothing to act on, + // so the status stands in for the reason the command did not give. + error = status == QProcess::CrashExit + ? tr("The send command crashed.") + : tr("The send command exited with status %1 and said nothing.") + .arg(exitCode); + } + emit finished(false, error); +} + +void MessageSender::handleError(QProcess::ProcessError error) +{ + // QProcess emits errorOccurred(FailedToStart) INSTEAD OF finished(), so + // without this the caller waits forever. Measured on Qt 6.11 for both a + // missing binary and a non-executable file: one errorOccurred, no + // finished(). + // + // Every other error IS followed by finished() and is left to it, which is + // not merely tidiness. A command that exits without draining a large stdin + // emits errorOccurred(WriteError) and then finished() with the command's + // real exit code and its real stderr; reporting the write error here would + // replace the server's own rejection message with a plumbing detail, and + // reporting it as well as finished() would deliver two results for one + // message. + if (error != QProcess::FailedToStart) + return; + if (m_reported) + return; + m_reported = true; + + emit finished(false, + tr("The send command '%1' could not be started. Check that " + "the path is correct and the file is executable.") + .arg(m_command)); +} diff --git a/src/messagesender.h b/src/messagesender.h new file mode 100644 index 0000000..86dde68 --- /dev/null +++ b/src/messagesender.h @@ -0,0 +1,164 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include <QObject> +#include <QProcess> +#include <QString> + +/// Runs an account's send_command with the message on stdin. +/// +/// EXACTLY TWO OUTCOMES: sent, or not sent with a reason. Exit code 75 has no +/// special meaning here, unlike in the sync path. Item 125 is open precisely +/// because mailsync.sh treats 75 as neither success nor failure and hangs on +/// it; that exists because the script contends for a lock and there is no lock +/// here. Recorded so the two paths are not later "harmonised". +/// +/// **The exit status is the only authority on whether a message was sent.** +/// This is the same rule assets/mailsync.sh exists to honour, and the same +/// class of bug is available here: a sender that reported success on anything +/// other than exit 0 would file a sent copy and close the composer for a +/// message that never left the machine. Nothing is derived from the command's +/// output, which belongs to whatever the user installed behind send_command. +/// +/// **No shell, ever.** The command is a config value and is split into an +/// argument list with QProcess::splitCommand, then handed to QProcess, which +/// calls execve directly. A `;`, `&&`, `$(...)` or a backtick in the +/// configured string therefore arrives as a literal argument with nothing to +/// interpret it. Note that splitCommand understands DOUBLE quotes only: +/// `-a 'my acct'` splits into three arguments, so a path or an argument +/// containing a space must be written with double quotes. Measured, not +/// assumed. +/// +/// **No message content ever reaches the argument list.** The bytes go on +/// stdin and only on stdin; the command reads its recipients from the +/// message's own headers, which is what `-t` means in the documented example. +/// A recipient address or a display name therefore cannot become an argument +/// however it is spelled. +/// +/// This is the outbox seam. An outbox is built by calling this from a drain +/// loop; nothing in the composer would need to change. +/// +/// Nothing here blocks the GUI thread DURING a send. send() hands the process +/// to the event loop and returns; there is no waitForStarted() and no +/// waitForFinished() on that path, so a command that hangs leaves the +/// interface responsive and the caller waiting on finished(). Timing a hung +/// command out is deliberately NOT this class's job: a timeout here would kill +/// a slow but working send. The one place this class does block is its +/// destructor, and that is the subject of the next paragraph. +/// +/// **Destruction mid-send waits, briefly, and then kills.** A send is a live +/// SMTP conversation, so the outcome of abandoning one is genuinely unknown: +/// the message may be fully delivered, partially delivered, or not sent at +/// all. Measured with a one-second command destroyed 100ms in: plain +/// destruction returns in 100ms, kills the child, and the work does NOT +/// complete, announced by nothing but a `QProcess: Destroyed while process is +/// still running` warning on stderr. That is the mailsync.sh failure in a new +/// place, an unknown real outcome reported as a definite one, and it is +/// reachable by closing the composer with the window manager's X button while +/// a send is in flight. +/// +/// So the destructor waits up to kShutdownWaitMs for the command to finish on +/// its own, which is the outcome that makes the report truthful: the same +/// measurement with a bounded wait completes the child and costs only the +/// ~1s the command actually needed. A command still running after that is +/// killed, because a destructor cannot block a quitting application forever. +/// +/// **No finished() is emitted from the destructor, in either branch, and that +/// is deliberate rather than an omission.** After a kill the outcome is +/// unknown, and this class reports two outcomes only; inventing a third by +/// guessing would be the exact lie the rest of this header is built to avoid. +/// After a successful late finish the emit would reach handlers on a +/// half-destroyed caller. A caller that must know the result has to keep the +/// sender alive until finished() arrives, which is what refusing to close a +/// composer mid-send would express. +/// +/// **There is no cancel(), and the caller does not have one either.** An +/// earlier revision of this comment deferred cancellation to "the caller's +/// popup", which overstated what exists: SendDialog offers an undo BEFORE the +/// send is committed and none after, by an explicit design decision that a +/// post-commit cancel is worse than either clean outcome. If a real cancel is +/// ever wanted it belongs HERE, killing the process and emitting one +/// finished(false, ...) through m_reported, which is the shape that flag +/// already has. It is not built now, and this header does not promise it. +class MessageSender : public QObject +{ + Q_OBJECT + +public: + explicit MessageSender(QObject *parent = nullptr); + + /// Waits briefly for an in-flight send, then kills it. See the class + /// comment: this is the one blocking call in the class, and it emits + /// nothing. + ~MessageSender() override; + + /// How long the destructor gives an in-flight command to finish on its + /// own before killing it. Long enough for a local MTA handing off to a + /// queue, short enough not to hang a quitting application. + static constexpr int kShutdownWaitMs = 5000; + + /// Starts \p command with \p bytes on stdin. + /// + /// Returns false without emitting anything when the command is empty or + /// only whitespace, when it splits to nothing, or when a send is already + /// running. A true return means the process was handed to the event loop, + /// NOT that it launched: a missing or non-executable binary surfaces + /// asynchronously through finished(false, ...), exactly as MailSync + /// documents. + bool send(const QString &command, const QByteArray &bytes); + + bool isRunning() const; + +signals: + /// \p error is empty on success and carries the command's stderr, or a + /// description of why it could not start, on failure. + /// + /// EMITTED exactly once per accepted send, and the distinction between + /// emitted and RECEIVED is the whole of this paragraph. QProcess can report + /// both an error and a finish for one run (measured: a command that exits + /// without draining a large stdin emits errorOccurred(WriteError) and then + /// finished()), and m_reported collapses that to one emit. + /// + /// **m_reported guards the emit, not the receivers, and a caller can still + /// see one result twice.** A MessageSender is normally a long-lived member + /// reused for every send, so a caller that connects INSIDE its send path + /// adds a permanent connection each time: send, fail, correct the + /// recipient, send again, and the second result runs BOTH lambdas. The + /// first still holds the first message's bytes, so it files a sent copy of + /// the wrong message and acts on a dialog it already destroyed. That is + /// precisely the harm this signal's contract exists to prevent, arriving + /// by the one route no guard inside this class can cover. + /// + /// A caller connecting per-send must therefore pass + /// `Qt::SingleShotConnection` (Qt 6.0+; this project is on 6.11), which + /// disconnects the moment the lambda runs. Connecting ONCE in the caller's + /// constructor and keeping the per-send state in members is the other + /// correct shape. What is not correct, and what reads as permitted if this + /// paragraph is skipped, is a bare connect() next to a send() call. + void finished(bool sent, const QString &error); + +private: + void handleFinished(int exitCode, QProcess::ExitStatus status); + void handleError(QProcess::ProcessError error); + + QProcess m_process; + QString m_command; + bool m_reported = false; +}; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 367d23d..e38d764 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -72,6 +72,7 @@ add_qtmaildir_test(markdownrenderer) add_qtmaildir_test(messagebuilder) add_qtmaildir_test(maildirname) add_qtmaildir_test(draftstore) +add_qtmaildir_test(messagesender) add_qtmaildir_test(translations) # Asserts on the tracked .ts rather than the generated .qm: an untranslated # string is dropped by lrelease, so it is invisible in the .qm and shows up diff --git a/tests/test_messagesender.cpp b/tests/test_messagesender.cpp new file mode 100644 index 0000000..89e0fcf --- /dev/null +++ b/tests/test_messagesender.cpp @@ -0,0 +1,532 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include <QtTest> +#include <QTemporaryDir> + +#include "messagesender.h" + +class TestMessageSender : public QObject +{ + Q_OBJECT + +private slots: + void aSuccessfulCommandReportsSent(); + void theMessageArrivesOnStdinIntact(); + void aLargeMessageArrivesWhole(); + void aFailingCommandReportsItsStderr(); + void aCommandThatDoesNotExistReportsAFailure(); + void aCommandThatIsNotExecutableReportsAFailure(); + void anEmptyCommandIsRefusedWithoutRunning(); + void aCommandOfOnlyWhitespaceIsRefusedWithoutRunning(); + void exitCode75IsAnOrdinaryFailure(); + void aSilentFailureStillReportsAReason(); + void aCommandThatNeverReadsStdinIsStillJudgedByItsExitStatus(); + void aCrashedCommandIsAFailureWithAReason(); + void aSecondSendIsRefusedWhileOneIsRunning(); + void shellMetacharactersReachNoShell(); + void nothingIsEverReportedTwice(); + void destroyingTheSenderLetsAnInFlightSendFinish(); + void destroyingTheSenderEmitsNothing(); + void aPerSendConnectionMustBeSingleShot(); + +private: + QString writeStub(const QString &name, const QString &body, + bool executable = true); + + QTemporaryDir m_dir; +}; + +QString TestMessageSender::writeStub(const QString &name, const QString &body, + bool executable) +{ + const QString path = m_dir.filePath(name); + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) + return {}; + file.write(QStringLiteral("#!/bin/sh\n%1\n").arg(body).toUtf8()); + file.close(); + QFile::Permissions permissions = QFile::ReadOwner | QFile::WriteOwner; + if (executable) + permissions |= QFile::ExeOwner; + file.setPermissions(permissions); + return path; +} + +void TestMessageSender::aSuccessfulCommandReportsSent() +{ + const QString stub = writeStub(QStringLiteral("ok.sh"), QStringLiteral("cat >/dev/null")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("From: a@example.org\r\n\r\nbody\r\n"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), true); + QVERIFY2(spy.at(0).at(1).toString().isEmpty(), + "a successful send carried an error message"); + QVERIFY2(!sender.isRunning(), "the sender still reports a run in progress"); +} + +void TestMessageSender::theMessageArrivesOnStdinIntact() +{ + // The property that matters most: the bytes the builder produced are the + // bytes the command receives. A stub that writes stdin to a file is the + // only way to see it, since there is no MTA to ask. + const QString captured = m_dir.filePath(QStringLiteral("captured.eml")); + const QString stub = writeStub(QStringLiteral("capture.sh"), + QStringLiteral("cat > '%1'").arg(captured)); + QVERIFY(!stub.isEmpty()); + + const QByteArray bytes( + "From: a@example.org\r\n" + "Subject: =?UTF-8?B?UGVyY2jDqQ==?=\r\n" + "\r\n" + "Perch=C3=A9 accented body.\r\n"); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, bytes)); + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), true); + + QFile file(captured); + QVERIFY2(file.open(QIODevice::ReadOnly), "the stub captured no stdin at all"); + QCOMPARE(file.readAll(), bytes); +} + +void TestMessageSender::aLargeMessageArrivesWhole() +{ + // A message with an attachment is megabytes, not bytes, and a pipe holds + // 64KB. If the write were not driven by the event loop the process would + // deadlock on a full pipe, or the tail would be silently dropped and a + // truncated message would be reported as sent. Measured: 1.6MB in one + // write() call returns the full count only because QProcess buffers it and + // drains it as the reader consumes; a probe confirmed the payload arrives + // byte-identical. + const QString captured = m_dir.filePath(QStringLiteral("big.eml")); + const QString stub = writeStub(QStringLiteral("bigcapture.sh"), + QStringLiteral("cat > '%1'").arg(captured)); + QVERIFY(!stub.isEmpty()); + + QByteArray bytes("From: a@example.org\r\n\r\n"); + // Well past a pipe buffer, and not a repeating single byte, so a partial + // write cannot accidentally compare equal. + for (int i = 0; i < 60000; ++i) + bytes += QByteArray::number(i) + "\r\n"; + QVERIFY(bytes.size() > 300000); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, bytes)); + QVERIFY(spy.wait(10000)); + QCOMPARE(spy.at(0).at(0).toBool(), true); + + QFile file(captured); + QVERIFY(file.open(QIODevice::ReadOnly)); + const QByteArray got = file.readAll(); + QCOMPARE(got.size(), bytes.size()); + QCOMPARE(got, bytes); +} + +void TestMessageSender::aFailingCommandReportsItsStderr() +{ + // stderr is shown verbatim: network errors, authentication failures and + // server rejections all belong to send_command, and this application + // deliberately does not interpret them. + const QString stub = writeStub( + QStringLiteral("fail.sh"), + QStringLiteral("cat >/dev/null; echo 'auth failed: bad password' >&2; exit 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("auth failed")), + qPrintable(QStringLiteral("stderr was not reported: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCommandThatDoesNotExistReportsAFailure() +{ + // A typo'd path is the likely cause, so the message names the command. + // QProcess emits errorOccurred(FailedToStart) INSTEAD OF finished(), which + // is the trap MailSync already documents: without handling it the signal + // never arrives and the popup waits forever. Measured on Qt 6.11: + // finCount 0, errCount 1. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(QStringLiteral("/nonexistent/msmtp"), QByteArray("body"))); + + QVERIFY2(spy.wait(5000), "no result was ever reported for a missing command"); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("msmtp")), + qPrintable(QStringLiteral("the error does not name the command: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCommandThatIsNotExecutableReportsAFailure() +{ + // A separate case from a missing file and reached by an ordinary mistake: + // a script written by the user and never chmod'd. It also arrives as + // FailedToStart with no finished(), so the same handler covers it, but a + // test asserting only the missing-file case would pass against a handler + // keyed on the errno rather than on the error enum. + const QString stub = writeStub(QStringLiteral("noexec.sh"), + QStringLiteral("cat >/dev/null"), false); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY2(spy.wait(5000), "no result was ever reported for a non-executable command"); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY(!spy.at(0).at(1).toString().isEmpty()); +} + +void TestMessageSender::anEmptyCommandIsRefusedWithoutRunning() +{ + // A receive-only account. The compose actions are disabled on its mail, so + // this should be unreachable; refusing here rather than asserting means a + // future caller cannot accidentally send from an account that cannot. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY2(!sender.send(QString(), QByteArray("body")), + "an empty command was accepted"); + QCOMPARE(spy.count(), 0); + QVERIFY(!sender.isRunning()); +} + +void TestMessageSender::aCommandOfOnlyWhitespaceIsRefusedWithoutRunning() +{ + // A config file with `send_command = ` and a trailing space reaches + // exactly this, and it must not run anything. + // + // MEASURED, and worth stating precisely so this is not mistaken for a + // sharper test than it is: send() has TWO guards that both catch a blank + // command, the trimmed()-empty check and the parts.isEmpty() check after + // QProcess::splitCommand(" ") returns an empty list. Dropping either one + // alone leaves this test green, because the other still refuses. Dropping + // BOTH aborts the run outright: QProcess treats an empty program as fatal, + // and the mutation reports "Received a fatal error" rather than a failed + // comparison. The pair is what is under test here; the redundancy is + // deliberate, since the fatal path is the one thing a send must never + // reach. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY2(!sender.send(QStringLiteral(" \t "), QByteArray("body")), + "a whitespace-only command was accepted"); + QCOMPARE(spy.count(), 0); + QVERIFY(!sender.isRunning()); +} + +void TestMessageSender::exitCode75IsAnOrdinaryFailure() +{ + // Explicitly asserted so the sync path's special handling of 75 is never + // copied here. There is no lock to contend for, so 75 means only what the + // command chose it to mean: not sent. + const QString stub = writeStub(QStringLiteral("busy.sh"), + QStringLiteral("cat >/dev/null; exit 75")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); +} + +void TestMessageSender::aSilentFailureStillReportsAReason() +{ + // The mailsync.sh lesson in the other direction: a command that fails + // without saying anything must not produce an empty error string, because + // the popup would then show a failure with a blank explanation and the + // user would have nothing to act on. + const QString stub = writeStub(QStringLiteral("silent.sh"), + QStringLiteral("cat >/dev/null; exit 3")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); + const QString error = spy.at(0).at(1).toString(); + QVERIFY2(!error.isEmpty(), "a silent failure reported no reason at all"); + QVERIFY2(error.contains(QStringLiteral("3")), + qPrintable(QStringLiteral("the exit status is not named: '%1'").arg(error))); +} + +void TestMessageSender::aCommandThatNeverReadsStdinIsStillJudgedByItsExitStatus() +{ + // Measured on Qt 6.11: a command that exits without draining a large stdin + // emits errorOccurred(WriteError) BEFORE finished(). A handler that treated + // any error as a failure to start would report the write error and swallow + // the real exit status; a handler that reported on every error would report + // twice. The exit status is the only authority, exactly as it is for the + // sync script, so this asserts the reason the command GAVE. + const QString stub = writeStub( + QStringLiteral("nonreading.sh"), + QStringLiteral("echo 'recipient rejected' >&2; exit 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray(1600 * 1024, 'x'))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("recipient rejected")), + qPrintable(QStringLiteral("the command's own reason was lost: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCrashedCommandIsAFailureWithAReason() +{ + // A segfaulting MTA is a real failure mode and reaches a DIFFERENT branch + // from a nonzero exit: status is CrashExit and exitCode carries the signal + // number, so an error message built from the exit code alone would tell the + // user the command "exited with status 11", which is not what happened. + // + // Measured on Qt 6.11: a crash emits errorOccurred(Crashed) and THEN + // finished(11, CrashExit). Only finished() reports, because handleError + // filters to FailedToStart, so the count assertion below also proves that + // filter is doing work on a path that is not the write-error one. + const QString stub = writeStub(QStringLiteral("crash.sh"), + QStringLiteral("cat >/dev/null; kill -SEGV $$")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QTest::qWait(300); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + const QString error = spy.at(0).at(1).toString(); + QVERIFY2(!error.isEmpty(), "a crashed command reported no reason"); + QVERIFY2(error.contains(QStringLiteral("crash")), + qPrintable(QStringLiteral("a crash was reported as an ordinary exit: '%1'") + .arg(error))); +} + +void TestMessageSender::aSecondSendIsRefusedWhileOneIsRunning() +{ + // One QProcess, so a second send would overwrite the first's program and + // arguments mid-flight. Refusing is what makes the popup's Sending stage + // mean one message. + const QString stub = writeStub(QStringLiteral("slow.sh"), + QStringLiteral("cat >/dev/null; sleep 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("first"))); + QVERIFY2(sender.isRunning(), "the sender does not report the run it just started"); + QVERIFY2(!sender.send(stub, QByteArray("second")), + "a second send was accepted while one was running"); + + QVERIFY(spy.wait(10000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), true); +} + +void TestMessageSender::shellMetacharactersReachNoShell() +{ + // The security property, asserted rather than asserted-about-in-a-comment. + // The command is split into an argument list and handed to execve, so a + // `;` in it is a literal argument and there is no shell to act on it. If + // this ever ran through `sh -c` the stub below would be invoked and the + // marker file would exist. + // + // Measured: QProcess::splitCommand("msmtp; rm x") yields ("msmtp;", "rm", + // "x"), so the semicolon does not even separate arguments. + const QString marker = m_dir.filePath(QStringLiteral("shell-ran")); + const QString stub = writeStub(QStringLiteral("args.sh"), + QStringLiteral("cat >/dev/null; exit 0")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(QStringLiteral("%1 ; touch %2").arg(stub, marker), + QByteArray("body"))); + QVERIFY(spy.wait(5000)); + + QVERIFY2(!QFile::exists(marker), + "the send command was interpreted by a shell"); + + // And the same string quoted the way a shell would need it also reaches no + // shell: double quotes are the ONLY quoting splitCommand understands. + // Measured: single quotes are NOT stripped, so `-a 'my acct'` arrives as + // three arguments. Recorded here because the plan's comment claimed + // splitCommand "handles quoted arguments" without that qualification. + QCOMPARE(QProcess::splitCommand(QStringLiteral("m -a \"my acct\" -t")), + QStringList({QStringLiteral("m"), QStringLiteral("-a"), + QStringLiteral("my acct"), QStringLiteral("-t")})); + QCOMPARE(QProcess::splitCommand(QStringLiteral("m -a 'my acct' -t")), + QStringList({QStringLiteral("m"), QStringLiteral("-a"), + QStringLiteral("'my"), QStringLiteral("acct'"), + QStringLiteral("-t")})); +} + +void TestMessageSender::nothingIsEverReportedTwice() +{ + // Reporting twice would close the send popup and then act on a second + // result, which for a caller that files a sent copy on success means two + // copies, or a success followed by a failure. Run every outcome through one + // sender and count. + const QString ok = writeStub(QStringLiteral("dup-ok.sh"), + QStringLiteral("cat >/dev/null")); + const QString bad = writeStub(QStringLiteral("dup-bad.sh"), + QStringLiteral("echo boom >&2; exit 1")); + QVERIFY(!ok.isEmpty() && !bad.isEmpty()); + + for (const QString &command : + {ok, bad, QStringLiteral("/nonexistent/msmtp")}) { + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(command, QByteArray(1600 * 1024, 'x'))); + QVERIFY(spy.wait(10000)); + // Give any second signal a chance to arrive before counting. + QTest::qWait(300); + QVERIFY2(spy.count() == 1, + qPrintable(QStringLiteral("%1 reported %2 times") + .arg(command) + .arg(spy.count()))); + } +} + +void TestMessageSender::destroyingTheSenderLetsAnInFlightSendFinish() +{ + // The composer's X button is reachable mid-send, and abandoning a live + // SMTP conversation has a genuinely unknown outcome. Measured before the + // destructor existed: plain destruction 100ms into a one-second command + // killed the child and the work did NOT complete, announced by nothing but + // a "QProcess: Destroyed while process is still running" warning. + // + // The marker file is the evidence, because it is written by the command + // itself after its work: if the destructor killed the child, it does not + // exist. + const QString marker = m_dir.filePath(QStringLiteral("send-completed")); + const QString stub = writeStub( + QStringLiteral("slowfinish.sh"), + QStringLiteral("cat >/dev/null; sleep 1; touch '%1'").arg(marker)); + QVERIFY(!stub.isEmpty()); + QVERIFY2(!QFile::exists(marker), "the marker existed before the send ran"); + + { + MessageSender sender; + QVERIFY(sender.send(stub, QByteArray("body"))); + // Destroyed well before the command could finish, which is the case + // that matters; without the wait this scope kills it. + QTest::qWait(100); + QVERIFY2(sender.isRunning(), "the command finished before it was abandoned"); + } + + QVERIFY2(QFile::exists(marker), + "destroying the sender killed a send that was in flight"); +} + +void TestMessageSender::destroyingTheSenderEmitsNothing() +{ + // After a kill the outcome is unknown, and this class reports two outcomes + // only. A finished(false, ...) from the destructor would report "not sent" + // for a message that may have been delivered, which is the mailsync.sh + // mistake pointing the other way. + // + // A command that outlasts the shutdown wait is what forces the kill + // branch, so the wait is shortened by pointing the test at a command + // longer than it rather than by changing the constant. + const QString stub = writeStub(QStringLiteral("outlast.sh"), + QStringLiteral("cat >/dev/null; sleep 30")); + QVERIFY(!stub.isEmpty()); + + QSignalSpy *spy = nullptr; + { + MessageSender sender; + spy = new QSignalSpy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + QTest::qWait(100); + QVERIFY(sender.isRunning()); + // The destructor runs as this scope ends: it waits kShutdownWaitMs + // for a command that will not finish, then kills it. + } + // The spy outlives the sender deliberately: a signal emitted during + // destruction would have been recorded before the object went away. + QCOMPARE(spy->count(), 0); + delete spy; +} + +void TestMessageSender::aPerSendConnectionMustBeSingleShot() +{ + // The header's contract, asserted. m_reported collapses two QProcess + // signals into one emit, but it cannot stop a caller from accumulating + // RECEIVERS: a long-lived sender that a caller connects to inside its send + // path runs every previous lambda on the next result, each still holding + // the previous message's bytes. + // + // This is the plan's own Task 11 shape, and it is why that step now + // specifies Qt::SingleShotConnection. + const QString stub = writeStub(QStringLiteral("twice.sh"), + QStringLiteral("cat >/dev/null")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; // long-lived, as a ComposeWindow member is + + // The broken shape: a bare connect() beside each send(). + int bareDeliveries = 0; + for (int i = 0; i < 2; ++i) { + QSignalSpy spy(&sender, &MessageSender::finished); + connect(&sender, &MessageSender::finished, this, + [&bareDeliveries](bool, const QString &) { ++bareDeliveries; }); + QVERIFY(sender.send(stub, QByteArray("body"))); + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); // ONE emit, both times + } + QVERIFY2(bareDeliveries == 3, + qPrintable(QStringLiteral("expected the documented 1+2 accumulation, got %1") + .arg(bareDeliveries))); + + // The prescribed shape: the connection disconnects as it fires, so two + // sends deliver two results rather than three. + MessageSender clean; + int singleShotDeliveries = 0; + for (int i = 0; i < 2; ++i) { + QSignalSpy spy(&clean, &MessageSender::finished); + connect(&clean, &MessageSender::finished, this, + [&singleShotDeliveries](bool, const QString &) { ++singleShotDeliveries; }, + Qt::SingleShotConnection); + QVERIFY(clean.send(stub, QByteArray("body"))); + QVERIFY(spy.wait(5000)); + } + QCOMPARE(singleShotDeliveries, 2); +} + +QTEST_MAIN(TestMessageSender) +#include "test_messagesender.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 76652b8..489c62d 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -999,6 +999,21 @@ </message> </context> <context> + <name>MessageSender</name> + <message> + <source>The send command crashed.</source> + <translation>Il comando di invio si è arrestato in modo anomalo.</translation> + </message> + <message> + <source>The send command exited with status %1 and said nothing.</source> + <translation>Il comando di invio è terminato con stato %1 senza fornire spiegazioni.</translation> + </message> + <message> + <source>The send command '%1' could not be started. Check that the path is correct and the file is executable.</source> + <translation>Impossibile avviare il comando di invio '%1'. Verifica che il percorso sia corretto e che il file sia eseguibile.</translation> + </message> +</context> +<context> <name>MessageView</name> <message> <source>Copied the selected text</source> @@ -1254,6 +1269,18 @@ <source>The message could not be assembled.</source> <translation>Non è stato possibile comporre il messaggio.</translation> </message> + <message> + <source>No folder was configured to write to.</source> + <translation>Nessuna cartella configurata per la scrittura.</translation> + </message> + <message> + <source>Cannot create the folder %1.</source> + <translation>Impossibile creare la cartella %1.</translation> + </message> + <message> + <source>Cannot write to %1: %2</source> + <translation>Impossibile scrivere su %1: %2</translation> + </message> </context> <context> <name>QueryCompleter</name> |
