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

Feat: Add message token to return path #2855

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jmdunsing
Copy link

@jmdunsing jmdunsing commented Mar 6, 2024

This PR incorporates a minor change to the way the return path is generated to allow for additional bounce processing and message forwarding. Those using postal strictly as a transactional email solution should notice no difference in performance. Those who wish to be able to capture all hard bounces and those who use postal as a marketing solution will have another method besides the X-Postal-MsgID header to identify bounces and replies, since the message token will be included in the return path.

Background:
Some mail servers return a bounce but omit identifiable information and strip the X-Postal-MsgID header from the email, leaving postal users unable to prevent future hard bounces. This change will provide the ability for a subsequent coding solution to tie bounces and replies to emails to their original sender.

This design change is consistent with the current bounce design, which splits the domain path on "@" then subsequently on "+", and checks the uname to see if the server was a bounce (specifically line 328), see

def rcpt_to(data)
unless in_state(:mail_from_received, :rcpt_to_received)
increment_error_count("rcpt-to-out-of-order")
return "503 EHLO/HELO and MAIL FROM first please"
end
rcpt_to = data.gsub(/RCPT TO\s*:\s*/i, "").gsub(/.*</, "").gsub(/>.*/, "").strip
if rcpt_to.blank?
increment_error_count("empty-rcpt-to")
return "501 RCPT TO should not be empty"
end
uname, domain = rcpt_to.split("@", 2)
if domain.blank?
increment_error_count("invalid-rcpt-to")
return "501 Invalid RCPT TO"
end
uname, tag = uname.split("+", 2)
if domain == Postal::Config.dns.return_path_domain || domain =~ /\A#{Regexp.escape(Postal::Config.dns.custom_return_path_prefix)}\./
# This is a return path
@state = :rcpt_to_received
if server = ::Server.where(token: uname).first
if server.suspended?
increment_error_count("server-suspended")
"535 Mail server has been suspended"
else
logger&.debug "Added bounce on server #{server.id}"
@recipients << [:bounce, rcpt_to, server]
"250 OK"
end
else
increment_error_count("invalid-server-token")
"550 Invalid server token"
end
:

These bounces will still not be identified by postal as bounces, and the incoming messages will still hardfail, because the current code still searches exclusively for the X-Postal-MsgID header

#
# Return a message object that this message is a reply to
#
def original_messages
return nil unless bounce
other_message_ids = raw_message.scan(/\X-Postal-MsgID:\s*([a-z0-9]+)/i).flatten
if other_message_ids.empty?
[]
else
database.messages(where: { token: other_message_ids })
end
end
:

* Fix: allow build for jmdunsing version of postal

Signed-off-by: jmdunsing <113212353+jmdunsing@users.noreply.github.com>

* Fix: Revise version number for jmdunsing version of postal

Signed-off-by: jmdunsing <113212353+jmdunsing@users.noreply.github.com>

---------

Signed-off-by: jmdunsing <113212353+jmdunsing@users.noreply.github.com>
Adding the message token to the return path would be beneficial for users to link bounces to email addresses when X-Postal-MsgID was not included in the bounce message's headers.

Signed-off-by: jmdunsing <113212353+jmdunsing@users.noreply.github.com>
Signed-off-by: jmdunsing <113212353+jmdunsing@users.noreply.github.com>
@jmdunsing
Copy link
Author

I created an image in my fork of postal and emails are being sent correctly using the new method.

Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@pedroluccasc
Copy link

i like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants