Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for mailer to override mail headers #27860

Merged
merged 26 commits into from
Jun 3, 2024

Conversation

6543
Copy link
Member

@6543 6543 commented Nov 1, 2023

Add option to override headers of mails, gitea send out


Sponsored by Kithara Software GmbH

@6543 6543 added the type/enhancement An improvement of existing functionality label Nov 1, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 1, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 1, 2023
@lunny lunny added this to the 1.22.0 milestone Nov 1, 2023
@silverwind
Copy link
Member

If I understand these headers correctly:

  • Return-Path is for bounced error e-mails only, e.g. sent by smtp servers, not by users
  • Reply-To is what the clients will reply to

So maybe you are really looking for Reply-To?

@6543 6543 added the pr/wip This PR is not ready for review label Nov 1, 2023
@6543
Copy link
Member Author

6543 commented Nov 1, 2023

I'll test how mailclients do react ...

@silverwind
Copy link
Member

silverwind commented Nov 1, 2023

We should only set Reply-To when it's different than From because:

Some email clients issue a warning message when Reply-To: is set

I do see we already set this header on line 54, so maybe just improve it in this regard.

@techknowlogick
Copy link
Member

This seems like it would conflict with the reply to issues via email functionality as that relies upon a specific address?

@6543
Copy link
Member Author

6543 commented Nov 1, 2023

Well yes i'll add a check so it does not conflict.

The usecase i try to solve: have a mail in from header witch is not "noreplay@..." but let mail clients still not replay to it (or show at least a warning)

@silverwind
Copy link
Member

I guess in any case, having this Return-Path option will be good to have, even thought it does likely not solve your use case.

@silverwind
Copy link
Member

silverwind commented Nov 1, 2023

Happy to merge this as-is for the Return-Path option. Could add check to compare it to FROM and skip setting it if equal but it would need to perform rfc5322 format-splitting (John Doe <[email protected]>) to make them comparable.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 1, 2023
@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 29, 2024
@6543 6543 requested a review from delvh May 27, 2024 08:29
@wxiaoguang
Copy link
Contributor

Is it really changeable from sender side?

https://www.rfc-editor.org/rfc/rfc5321

   When the delivery SMTP server makes the "final delivery" of a
   message, it inserts a return-path line at the beginning of the mail
   data.  This use of return-path is required; mail systems MUST support
   it.  The return-path line preserves the information in the <reverse-
   path> from the MAIL command.  Here, final delivery means the message
   has left the SMTP environment.  Normally, this would mean it had been
   delivered to the destination user or an associated mail drop, but in
   some cases it may be further processed and transmitted by another
   mail system.

https://stackoverflow.com/questions/1235534/what-is-the-behavior-difference-between-return-path-reply-to-and-from

Only the recipient's mail server is supposed to add a Return-Path header to the top of the email. 
This records the actual Return-Path sender during the SMTP session.
If a Return-Path header already exists in the message,
then that header is removed and replaced by the recipient's mail server.

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 28, 2024
@silverwind
Copy link
Member

BTW I just checked: Multiple headers values split on , and sub-values split on ; and I think this is true for both mail and http headers.

@6543
Copy link
Member Author

6543 commented May 28, 2024

BTW I just checked: Multiple headers values split on , and sub-values split on ; and I think this is true for both mail and http headers.

well we now just make a pass-through more or less ...

@silverwind
Copy link
Member

silverwind commented May 28, 2024

Interesting block syntax, where's it from? At least GFM can't seem to render it.

@6543
Copy link
Member Author

6543 commented May 28, 2024

Interesting block syntax, where's it from? At least GFM can't seem to render it.

docusaurus buildin syntax

https://docusaurus.io/docs/markdown-features/admonitions

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 28, 2024
@silverwind
Copy link
Member

Interesting block syntax, where's it from? At least GFM can't seem to render it.

docusaurus buildin syntax

https://docusaurus.io/docs/markdown-features/admonitions

Thanks, I see facebook/docusaurus#7471 is related.

@lunny
Copy link
Member

lunny commented May 29, 2024

It's better to have a test.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2024
@6543
Copy link
Member Author

6543 commented Jun 3, 2024

It's better to have a test.

added

@6543 6543 requested a review from lunny June 3, 2024 12:27
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 3, 2024
@6543 6543 changed the title mailer: add option to override mail headers Add option for mailer to override mail headers Jun 3, 2024
@6543 6543 merged commit aace3bc into go-gitea:main Jun 3, 2024
26 checks passed
@6543 6543 deleted the mail_set-return-path branch June 3, 2024 18:42
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 4, 2024
* giteaofficial/main:
  Add option for mailer to override mail headers (go-gitea#27860)
  Move custom `tw-` helpers to tailwind plugin (go-gitea#31184)
  Remove unnecessary inline style for tab-size (go-gitea#31224)
  Document possible action types for the user activity feed API (go-gitea#31196)
  Remove sqlite-viewer and using database client (go-gitea#31223)
  Update golangci-lint to v1.59.0 (go-gitea#31221)
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 6, 2024
* origin/main: (231 commits)
  Allow including `Reviewed-on`/`Reviewed-by` lines for custom merge messages (go-gitea#31211)
  Add `MAX_ROWS` option for CSV rendering (go-gitea#30268)
  Update `golang.org/x/net` (go-gitea#31260)
  Add replacement module for `mholt/archiver` (go-gitea#31267)
  Fix Activity Page Contributors dropdown (go-gitea#31264)
  Optimize runner-tags layout to enhance visual experience (go-gitea#31258)
  fix: allow actions artifacts storage migration to complete succesfully (go-gitea#31251)
  Add `lint-go-gopls` (go-gitea#30729)
  Make blockquote attention recognize more syntaxes (go-gitea#31240)
  Fix admin oauth2 custom URL settings (go-gitea#31246)
  Replace `gt-word-break` with `tw-break-anywhere` (go-gitea#31183)
  Make pasted "img" tag has the same behavior as markdown image (go-gitea#31235)
  Remove .segment from .project-column (go-gitea#31204)
  Fix overflow on push notification (go-gitea#31179)
  Fix NuGet Package API for $filter with Id equality  (go-gitea#31188)
  Fix overflow on notifications (go-gitea#31178)
  Update chroma to v2.14.0 (go-gitea#31177)
  Update air package path (go-gitea#31233)
  Bump `@github/relative-time-element` to v4.4.1 (go-gitea#31232)
  Add option for mailer to override mail headers (go-gitea#27860)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/docs modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants