[guerricv]

Description

Reusing the email service for a backlog or an administrator loop could carry recipients from one message into the next. Background notifications also ignored SMTP failures and could mark a rejected email as completed.

This fixes those delivery problems and prevents SMTP diagnostics and task logs from exposing credentials or message contents. The existing SMTP settings and return contract are preserved; no reconfiguration or database migration is required.

The changes are split into three commits:

1. 50200b002: isolate recipient lists between deliveries and close the connection after a failure.
2. 98ef52a8b: report failed background deliveries, skip empty credential decryption, and refuse delivery when a credential cannot be decrypted.
3. ce4697c55: retain only safe SMTP stages, library reasons and validated numeric codes; replace task argument dumps with task metadata; add troubleshooting documentation.

Related issue

No existing issue. This is a standalone prerequisite for the email multi-transport work in #5454, kept in draft while this prerequisite is integrated and acceptance checks are completed. This corrective PR adds no new transport or automatic retry.

Type of change

• ☑ Bug fix (non-breaking change)
• ☑ Documentation
• ☐ New feature
• ☐ Breaking change
• ☐ Translation
• ☐ Refactor / maintenance

How has this been tested?

Rebase follow-up (8 November 2026): rebased the three separate commits onto current develop at 2e3db0edde1b4bfb1a93c84f5944e964c278b548 (TeamPass 3.2.4.2), without conflicts or patch changes. Current head: ce4697c5509205cfe9cf83874bfe5380397bb4ad. Local email/background-task tests passed (129 tests, 1,289 assertions; five environment skips). Fresh Quality CI passed on this rebased head: PHP 8.2/8.3 each passed 3,538 tests and 83,901 assertions; all 311 JavaScript tests, PHPStan level 4, MariaDB and repository guards passed. CodeQL and external Scrutinizer also passed on this rebased head; all 11 checks are green. The results below are historical checks of the earlier head. No manual role or real-provider acceptance is claimed.

• PHP 8.2.34: 116 focused tests, 1,167 assertions, no failures.
• Linux / PHP 8.5.4: the complete suite passed on 6fb4a6c3c: 3,504 tests, 83,394 assertions, no errors or failures.
• The reference develop commit 2783652a2 was tested in the same Linux environment: 3,494 tests, 83,304 assertions. Both runs reported 14 deprecations and 5 skipped tests.
• PHPStan level 4 passed locally and on Linux. The email package, excluded by the current global configuration, was also analysed explicitly.
• PHP lint, the SQL table-prefix guard (86 table names), dual-copy checks and git diff --check passed.
• Ten new regression tests exercise real PHPMailer message preparation with a simulated transport, and the production worker class with simulated persistence and logging boundaries.

These checks use no installation credentials or real SMTP relay. No manual UI or user-role validation is claimed; access-control code is unchanged. The complete Windows run encountered CRLF/OpenSSL/POSIX environment differences; the complete Linux comparison used LF archives and the required test extensions. GitHub Actions checks passed on 6fb4a6c3c, including PHP 8.2/8.3, PHPStan level 4, JavaScript, MariaDB integration tests, CodeQL and the repository guards. The external Scrutinizer check also passed on this commit.

Checklist

• ☑ PHPStan level 4 passes locally and on Linux
• ☑ The complete test suite passes on Linux
• ☑ New public functions have docblocks
• ☑ Variable names and comments are in English
• ☑ No new var_dump() or console.log()
• ☑ No new query controller requiring a public proxy
• ☑ Both copies of the TeamPass email service were updated
• ☑ The committed Composer autoloader remains production-only and unchanged
• ☑ GitHub Actions checks verified on the published commit
• ☑ External Scrutinizer check completed

Impact on install / upgrade

• ☑ No schema change
• ☐ Schema change

Existing SMTP authentication, encryption and certificate verification settings are retained.

Release note

The test-email dialog now shows a filtered SMTP transcript: connection stages, numeric replies, failed command names and local connection/TLS/OpenSSL errors remain visible, while authentication data, message contents and raw server replies are omitted. Socket error numbers are no longer displayed as SMTP reply codes.

Screenshots

Not applicable: no interface layout changes.

[nilsteampassnet]

Hi @guerricv, thanks for this PR. Before merging, I'd like the following points addressed.

1. Failed send_email tasks keep their full arguments forever (blocking)

completeTask() anonymizes a send_email task through buildCompletionArguments(), which keeps only the receivers and the login. With this PR, a relay refusal goes through handleTaskFailure() instead. That method writes the status but leaves arguments untouched, so the full body and encryptedUserPassword stay in background_tasks. A body can hold a secret in clear text: for example, the Google Authenticator code sent by email (#2FACode#, app/sources/main.queries.php:1744).

These rows are never cleaned up. The purge of old failed tasks (app/scripts/background_tasks___handler.php:1037-1046) uses an INNER JOIN on background_subtasks, and a send_email task has no subtask. Before this PR, a refused send was marked completed, so it was anonymized and later purged. With a misconfigured relay, every notification body would now pile up with no limit.

Requested:

• in handleTaskFailure(), apply the same anonymization to send_email:

  if ($this->processType === 'send_email') {
      // Same anonymization as completeTask(): the body and the encrypted credential must not outlive the attempt.
      $updateData['arguments'] = $this->buildCompletionArguments();
  }

• change the purge's INNER JOIN to a LEFT JOIN, so that failed tasks without subtasks are removed too. The bug predates this PR, but it is what makes the retention permanent;
• extend testWorkerPersistsDeliveryFailuresAndLogsOnlyMetadata to assert that the failed update stores the anonymized arguments, without the body or the ciphertext.

2. The test email no longer shows TLS and connection errors

safeDebugLine() drops the lines that tell an administrator why the connection failed. I measured it with PHPMailer 6.12 against 127.0.0.1, at debug level 3:

Scenario	develop	this PR
Refused port	Connection failed. Error #2: stream_socket_client(): Unable to connect to 127.0.0.1:1 (Connection refused) [...] SMTP ERROR: Failed to connect to server: Connection refused (111)	Connection: opening
Certificate verification on, invalid certificate	Connection failed. Error #2: stream_socket_client(): SSL operation failed with code 1. OpenSSL Error messages: error:0A000086:SSL routines::certificate verify failed [...]	Connection: opening

The second case is exactly what the test email is for since email_smtp_verify_certificate was added: the administrator has to know whether to fix the CA chain or turn verification off. These lines come from the local stream layer and OpenSSL, not from the provider. They contain no credentials, since PHPMailer already hides AUTH below level 4.

Requested:

• keep Connection failed. Error #N: <message>, without the trailing [file line N]. The OpenSSL message spans several lines, so the pattern needs the /s modifier;
• keep SMTP ERROR: Failed to connect to server: <reason> (<errno>);
• keep SMTP ERROR: <COMMAND> command failed, without the reply text;
• keep Connection: closing due to error;
• drop the failed alternative from Connection: (opening|opened|closed|failed): PHPMailer 6.12 never emits Connection: failed;
• cover these lines with the exact strings PHPMailer emits (the ones above), not with made-up ones.

3. Decryption failures are reported as delivery failures

EmailTrait throws Email credential could not be decrypted., but handleTaskFailure() replaces every send_email message with Email delivery failed.. An administrator then cannot tell a SECUREFILE problem from a relay refusal. Meanwhile, the documentation tells them to diagnose with the test email, and in that case the test email succeeds. Both messages thrown by the trait are fixed strings, so please keep them: use an allowlist of the trait's own messages in handleTaskFailure(), and fall back to the generic text for anything else.

4. A socket errno is shown as an SMTP code

When the connection fails, PHPMailer stores the socket errno in smtp_code. A three-digit errno passes the [1-5][0-9]{2} check, so a refused connection shows as SMTP Error: Could not connect to SMTP host. [111], which reads like an SMTP reply. Please only append codes when the SMTP error is not a connection failure (Failed to connect to server / Connection failed.), or label the value as an errno. In that case smtp_code_ex holds the local socket message (Connection refused), which is safe to show as well: [errno 111: Connection refused].

5. Documentation

• add docs/manage/email-troubleshooting.md to docs/_sidebar.md, next to Email templates (line 47). Otherwise the page cannot be reached from the docs navigation;
• after point 1, rewrite the paragraph "Failed task arguments remain in the database for investigation…": only the recipient and the login will remain;
• update the debug-level paragraph to list the connection and TLS errors kept by point 2.

6. Release note

Administrators will see a different debug output for the test email (a filtered transcript instead of the raw one). Please add a short Release note section to the PR description that describes this change, so I can carry it into the changelog.

Thanks again! I'll take another look as soon as these changes are pushed.

[guerricv]

Hi Nils, thanks for the precise checks and repro cases. I've rebased onto develop after #5452 (945069257) and addressed the six points in separate commits:

• e530e6589: failed email attempts now use the same argument anonymization as completed ones. The purge uses LEFT JOIN, including failures without subtasks. The worker regression checks the persisted arguments, and a new MariaDB test runs the actual handler cleanup against disposable tables.
• 918aa042a: the two fixed messages from EmailTrait survive in the task status; unknown exceptions, including a known message with extra text appended, fall back to the generic delivery error.
• e10931bc5: local connection and multiline OpenSSL errors remain visible without the trailing source location, command failures keep only their labels, and socket errnos are no longer shown as SMTP reply codes. Tests use PHPMailer's own stream-error handler and a real refused loopback connection. The troubleshooting page and sidebar are updated.
• 004760823: clarified that argument anonymization happens when the worker records completion or failure, rather than implying that it runs after an interrupted process.

I've also added the requested Release note section to the description, leaving the existing review context intact.

Final validation on 004760823d502149d3717bb730dd2bc45b383838: Quality CI passed on PHP 8.2 and 8.3 (3,554 tests / 84,024 assertions each), all 319 JavaScript tests passed, and PHPStan level 4, MariaDB (including the new purge regression), repository guards, CodeQL and Scrutinizer all passed. The email package, excluded by the main PHPStan configuration, was also checked explicitly.

In an isolated Linux copy, PHP 8.5.4 also passed 3,554 tests / 83,974 assertions (14 deprecations and five skips reported). The MariaDB cleanup test passed locally too. An additional loopback test with a real untrusted certificate confirmed that verification rejects it and the OpenSSL cause remains visible without source paths or false SMTP codes. The simulator was stopped and its temporary certificate/key removed; the installed application was not changed.

Scrutinizer is green and reports one non-blocking finding. I also checked the file-write taint trace at sendMailToUser(): it leads to PHPMailer's optional S/MIME temporary-file write. The path comes from tempnam(); the message body is file content, not a filename. EmailService does not configure S/MIME signing, so I have left that vendor code unchanged.