Add "-i" to sendmail's pipe

[FedericoHeichou]

• Resolves: [Bug]: Double dots in links in emails #39594
• Fix First email notification link wrong #33754
• Fix Email Logo - two dots in url #16379

Summary

Symfony Mailer replaces "\n." with "\n..".
If the email to be sent contains links when they are breaklined a line may start with a ., thus can generating a URL like https://www..example.com/[...] or https://www.example.com/index..php/[...] .
With "-i" this replacement is not performed.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[FedericoHeichou]

Hello, do I need to do something?

[tanganellilore]

Hi team,
I'm also interested to this features... @solracsf @icewind1991 can you help us to address this and include in 28?

Thanks

[joshtrichards]

TL;DR I agree that -i should probably be added, but in theory it's not supposed to matter given this bit of code. But, even so, it should be added because doing so is probably the Right Thing(tm) to do for the broadest compatibility. It's also how I read Symfony's inline recommendation.

I'm wondering if what's happening is a one (or both) of:

• some third-party command-line option "sendmail compatibility layers" are accepting -i as a parameter but not actually handling mail in "-i mode" (thus creating a problem with .
• some third-party command-line option "sendmail compatibility layers" are operating in -i mode by default even when the option hasn't been specified (thus triggering Symfony's code above unintentionally and undesirably because Symfony merely looks for the presence or lack there of of -i on the command-line to decide whether to adjust message content).

My money is on the second being more common.

The balance - probably the bulk since this would be a much more widely reported otherwise I'd imagine - are handling things just fine without -i being present by operating how an official sendmail binary would when not provided with that option (and thus the activation of the Symfony code above is both desirable and functioning).

Bottom line: It's completely reasonable to run -t mode with or without the -i flag when using the Symfony Mailer. (Well, as long as that bit of code linked above is working).

IMO: Since our aim should be compatibility (and as deterministic as possible outcomes) we should:

• explicitly specify -i when operating in -t mode (what Symfony calls "pipe" mode)

Ideally, documentation also gets a minor addition in the Server Requirements or Email Configuration section: e.g. If using pipe mode we'll be invoking the installed sendmail binary (genuine or compatible). We expect the called sendmail binary to fully support the following conventional sendmail compatible options: -i, -t, -f, and -bs (only the last is not needed in a pipe only configuration). These are generally supported universally since they're central to sendmail compatibility, but occasionally there have been differences in behavior. These differences can cause problems ranging from inability to send mail to the mangling of periods in message bodies. If you run into issues such as inability to send email or unusual email mangling, please try to troubleshoot your sendmail installation or try switching to smtp mode.

All that said: There may yet be further reports of issues like this (but hopefully fewer). My recollection from my email server days is that the only dot . that matters with sendmail/smtp is the one that is the very first character of a line. A cursory look at #33754, #39594, and #16379 doesn't make it immediately apparent to me why these particular message lines were identified for adjustment by this code regardless of the presence (or not) of -i. Of course it's also possible they were mangled downstream - by the server's own mailer for whatever reason. 🤦‍♂️

Still same conclusion: we should send -i.

I offered you the TL;DR. You didn't have to read this far. 😇

Related: symfony/symfony#50875 (comment)

[joshtrichards]

LGTM

[joshtrichards]

/update-3rdparty

[kesselb]

Thank you 👍

[joshtrichards]

No idea why the cypress tests keep failing for this PR.

 1) Versions expiration
       Expire all versions:
     CypressError: `cy.exec('docker exec --user www-data nextcloud-cypress-tests-server php ./occ config:system:set versions_retention_obligation --value "0, 0"')` failed because the command exited with a non-zero code.

Pass `{failOnNonZeroExit: false}` to ignore exit code failures.

Information about the failure:
Code: 1

Stdout:
An unhandled exception has been thrown:
Error: Call to undefined method OC\Server::getEventDispatcher() in /var/www/html/console.php:94
Stack trace:
#0 /var/www/html/occ(11): require_once()
#1 {main}

https://on.cypress.io/exec
      at <unknown> (http://172.17.0.2/__cypress/runner/cypress_runner.js:119190:77)
      at tryCatcher (http://172.17.0.2/__cypress/runner/cypress_runner.js:1807:23)
      at Promise._settlePromiseFromHandler (http://172.17.0.2/__cypress/runner/cypress_runner.js:1519:31)
      at Promise._settlePromise (http://172.17.0.2/__cypress/runner/cypress_runner.js:1576:18)
      at Promise._settlePromise0 (http://172.17.0.2/__cypress/runner/cypress_runner.js:1621:10)
      at Promise._settlePromises (http://172.17.0.2/__cypress/runner/cypress_runner.js:1701:18)
      at _drainQueueStep (http://172.17.0.2/__cypress/runner/cypress_runner.js:2407:12)
      at _drainQueue (http://172.17.0.2/__cypress/runner/cypress_runner.js:2400:9)
      at Async._drainQueues (http://172.17.0.2/__cypress/runner/cypress_runner.js:2416:5)
      at Async.drainQueues (http://172.17.0.2/__cypress/runner/cypress_runner.js:2286:14)
  From Your Spec Code:
      at Context.eval (webpack://nextcloud/./cypress/support/commands.ts:160:7)

  2) Versions expiration
       Expire versions v2:
     CypressError: `cy.exec('docker exec --user www-data nextcloud-cypress-tests-server php ./occ config:system:set versions_retention_obligation --value "0, 0"')` failed because the command exited with a non-zero code.

Pass `{failOnNonZeroExit: false}` to ignore exit code failures.

Information about the failure:
Code: 1

Stdout:
An unhandled exception has been thrown:
Error: Call to undefined method OC\Server::getEventDispatcher() in /var/www/html/console.php:94
Stack trace:
#0 /var/www/html/occ(11): require_once()
#1 {main}

[kesselb]

No idea why the cypress tests keep failing for this PR.

#40181

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[joshtrichards]

/backport to stable28

[joshtrichards]

/backport to stable27

[joshtrichards]

/backport to stable26