#25: deliver mail addressed to this server without going out to the internet
Every message was resolved through MX and delivered over SMTP, including one addressed to a mailbox on this very host. Two users on the same server could not write to each other without a working outbound port 25 - which the provider blocks, so on this deployment they could not write to each other at all. It also made internal mail take a pointless round trip through the public internet. The worker now checks whether the recipient resolves to a local mailbox and, if so, hands the message to the inbound path directly. The check runs ahead of the relay branch on purpose: sending local mail out to a smarthost only to have it delivered back is slower and has more ways to fail. The shortcut is narrow. It relies on ResolveDeliveryTargets, which only resolves addresses on an active local domain, and a test covers that a remote recipient still goes out over SMTP - otherwise local delivery would swallow mail meant for the internet. A resolver error is treated as retry rather than as not-local: falling through to SMTP on an unknown answer would send to the internet a message that may belong here. Local delivery is still content-scanned; a virus between two accounts on one host is no less a virus. This also surfaced a permissive test double: fakeAliasFanOutRepository claimed every address was local, which was harmless while nothing consulted it while routing and wrong the moment something did. It now hosts one address, as the real repository does, and the DSN fan-out test names it explicitly. Verified: the local-delivery test fails without the change (the recipient triggers an MX lookup) and passes with it; 22 Go packages green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
0fcdd15b58
commit
c83f2f75b2
5 files changed
+198
-3
No files matched your search
@@ -139,7 +139,9 @@ func TestOutboundWorker_DsnToFanOutSenderDeliversToEveryTarget(t *testing.T) {
|
||||
repo := &fakeOutboundRepo{}
|
||||
repo.Enqueue(context.Background(), newTestJob())
|
||||
|
||||
mailboxes := &fakeAliasFanOutRepository{targets: []port.MailboxRecord{
|
||||
// The sender is the local, aliased address; the recipient is remote, which
|
||||
// is what makes this a delivery that goes out and then bounces back.
|
||||
mailboxes := &fakeAliasFanOutRepository{localAddress: "user@domain.test", targets: []port.MailboxRecord{
|
||||
{ID: uuid.New(), QuotaBytes: 1000},
|
||||
{ID: uuid.New(), QuotaBytes: 1000},
|
||||
}}
|
||||
|
||||
@@ -0,0 +1,114 @@
|
||||
package usecase
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
"lambdamail/protocols/internal/application/port"
|
||||
"lambdamail/protocols/internal/domain/entity"
|
||||
)
|
||||
|
||||
// countingMXResolver fails the test if delivery ever tries to leave the host.
|
||||
type countingMXResolver struct{ lookups int }
|
||||
|
||||
func (c *countingMXResolver) LookupMX(_ context.Context, _ string) ([]string, error) {
|
||||
c.lookups++
|
||||
// An address that cannot be dialled, so a test that does reach here fails
|
||||
// on the assertion rather than on a timeout.
|
||||
return []string{"127.0.0.1:9"}, nil
|
||||
}
|
||||
|
||||
func newLocalDeliveryJob(to string) *entity.OutboundJob {
|
||||
mbID := uuid.New()
|
||||
return &entity.OutboundJob{
|
||||
ID: uuid.New(),
|
||||
MailboxID: &mbID,
|
||||
BlobID: uuid.New(),
|
||||
EnvelopeFrom: "me@example.test",
|
||||
EnvelopeTo: to,
|
||||
DestinationDomain: "example.test",
|
||||
Status: entity.OutboundJobStatusQueued,
|
||||
NextAttemptAt: time.Now(),
|
||||
ExpiresAt: time.Now().Add(time.Hour),
|
||||
}
|
||||
}
|
||||
|
||||
// Mail between two accounts on this server must not depend on outbound port
|
||||
// 25. Every message used to be resolved through MX and delivered over SMTP,
|
||||
// including one addressed to a mailbox on this very host - so on a provider
|
||||
// that blocks port 25, users could not write to each other on their own
|
||||
// server.
|
||||
func TestLocalRecipientIsDeliveredWithoutLeavingTheHost(t *testing.T) {
|
||||
recipient := "colleague@example.test"
|
||||
mailboxes := &fakeMailboxRepository{byAddress: map[string]port.MailboxRecord{
|
||||
recipient: {ID: uuid.New(), MaxMessageBytes: 1_000_000},
|
||||
}}
|
||||
messages := &recordingMessageRepository{}
|
||||
inbound := NewProcessInboundEmailUseCase(
|
||||
mailboxes, &stubBlobStorage{ref: port.BlobRef{ID: uuid.New(), SHA256: "s", SizeBytes: 4}}, messages,
|
||||
)
|
||||
|
||||
mx := &countingMXResolver{}
|
||||
repo := &fakeOutboundRepo{}
|
||||
job := newLocalDeliveryJob(recipient)
|
||||
repo.jobs = append(repo.jobs, job)
|
||||
|
||||
worker := NewOutboundWorkerUseCase(
|
||||
repo, mx,
|
||||
&fakeBlobReader{payload: []byte("From: me@example.test\r\nTo: " + recipient + "\r\n\r\nhello\r\n")},
|
||||
inbound, mailboxes, "mail.example.test",
|
||||
)
|
||||
|
||||
if _, err := worker.ProcessBatch(context.Background(), "w1", 10); err != nil {
|
||||
t.Fatalf("ProcessBatch: %v", err)
|
||||
}
|
||||
|
||||
if mx.lookups != 0 {
|
||||
t.Errorf("a local recipient triggered %d MX lookups; delivery left the host", mx.lookups)
|
||||
}
|
||||
if len(messages.persisted) != 1 {
|
||||
t.Fatalf("want the message stored once for the local recipient, got %d", len(messages.persisted))
|
||||
}
|
||||
if job.Status != entity.OutboundJobStatusDelivered {
|
||||
t.Errorf("job status = %q, want delivered", job.Status)
|
||||
}
|
||||
if job.TlsPolicyUsed != entity.TLSModeLocal {
|
||||
t.Errorf("tls policy = %q, want %q - nothing crossed a network",
|
||||
job.TlsPolicyUsed, entity.TLSModeLocal)
|
||||
}
|
||||
}
|
||||
|
||||
// The shortcut must be narrow: an address this server does not host still goes
|
||||
// out over SMTP, or local delivery would swallow the internet's mail.
|
||||
func TestRemoteRecipientStillGoesOutOverSmtp(t *testing.T) {
|
||||
mailboxes := &fakeMailboxRepository{byAddress: map[string]port.MailboxRecord{}}
|
||||
messages := &recordingMessageRepository{}
|
||||
inbound := NewProcessInboundEmailUseCase(
|
||||
mailboxes, &stubBlobStorage{ref: port.BlobRef{ID: uuid.New()}}, messages,
|
||||
)
|
||||
|
||||
mx := &countingMXResolver{}
|
||||
repo := &fakeOutboundRepo{}
|
||||
job := newLocalDeliveryJob("stranger@remote.test")
|
||||
job.DestinationDomain = "remote.test"
|
||||
repo.jobs = append(repo.jobs, job)
|
||||
|
||||
worker := NewOutboundWorkerUseCase(
|
||||
repo, mx, &fakeBlobReader{payload: []byte("From: me\r\n\r\nhi\r\n")},
|
||||
inbound, mailboxes, "mail.example.test",
|
||||
)
|
||||
|
||||
if _, err := worker.ProcessBatch(context.Background(), "w1", 10); err != nil {
|
||||
t.Fatalf("ProcessBatch: %v", err)
|
||||
}
|
||||
|
||||
if mx.lookups == 0 {
|
||||
t.Error("a remote recipient was not resolved through MX")
|
||||
}
|
||||
if len(messages.persisted) != 0 {
|
||||
t.Errorf("a remote message was filed into a local mailbox: %+v", messages.persisted)
|
||||
}
|
||||
}
|
||||
@@ -98,6 +98,29 @@ func (w *OutboundWorkerUseCase) processSingleJob(ctx context.Context, job *entit
|
||||
return
|
||||
}
|
||||
|
||||
// Mail addressed to a mailbox on this server is delivered here, without
|
||||
// going out to the internet and back.
|
||||
//
|
||||
// Nothing did this before: every message was resolved through MX and
|
||||
// delivered over SMTP, including one user writing to another on the same
|
||||
// host. That makes internal mail depend on outbound port 25 - so on a
|
||||
// provider that blocks it, which is most of them, colleagues could not
|
||||
// write to each other on their own server. It also fails ahead of the
|
||||
// relay check on purpose: sending local mail out to a smarthost only to
|
||||
// have it delivered back is a slower path with more ways to break.
|
||||
if delivered, err := w.deliverLocally(ctx, job, payload); delivered {
|
||||
if err != nil {
|
||||
w.handleFailure(ctx, job, fmt.Sprintf("local delivery: %v", err), true)
|
||||
return
|
||||
}
|
||||
job.Status = entity.OutboundJobStatusDelivered
|
||||
job.LastError = ""
|
||||
job.LastSmtpCode = ""
|
||||
job.TlsPolicyUsed = entity.TLSModeLocal
|
||||
_ = w.outboundRepo.UpdateJob(ctx, job)
|
||||
return
|
||||
}
|
||||
|
||||
// With a smarthost every message goes to the relay regardless of
|
||||
// destination, and the relay owns transport security from there on.
|
||||
if w.relay.Configured() {
|
||||
@@ -379,3 +402,44 @@ func (w *OutboundWorkerUseCase) deliverToMX(ctx context.Context, mxHost string,
|
||||
|
||||
return client.Quit()
|
||||
}
|
||||
|
||||
// deliverLocally hands a message to a mailbox on this server.
|
||||
//
|
||||
// Returns (handled, err): handled is false when the recipient is not local, so
|
||||
// the caller falls through to relay or MX delivery. A local recipient whose
|
||||
// delivery then fails returns (true, err) and is retried like any other job -
|
||||
// the queue row stays the durable record either way.
|
||||
//
|
||||
// The message is still content-scanned. It arrives DKIM-signed by this server,
|
||||
// so it authenticates cleanly, and a virus mailed between two accounts on the
|
||||
// same host is no less a virus.
|
||||
func (w *OutboundWorkerUseCase) deliverLocally(
|
||||
ctx context.Context, job *entity.OutboundJob, payload []byte,
|
||||
) (bool, error) {
|
||||
if w.inboundUC == nil || w.mailboxes == nil {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
targets, err := w.mailboxes.ResolveDeliveryTargets(ctx, job.EnvelopeTo)
|
||||
if err != nil {
|
||||
// Not knowing whether the address is local is not the same as knowing
|
||||
// it is not: falling through to SMTP would deliver to the internet a
|
||||
// message that may belong here. Retrying is the safe answer.
|
||||
return true, err
|
||||
}
|
||||
if len(targets) == 0 {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
addresses := make([]string, len(targets))
|
||||
for i := range targets {
|
||||
addresses[i] = job.EnvelopeTo
|
||||
}
|
||||
|
||||
return true, w.inboundUC.Handle(ctx, ProcessInboundEmailInput{
|
||||
Sender: job.EnvelopeFrom,
|
||||
Recipients: targets,
|
||||
RecipientAddresses: addresses,
|
||||
Body: bytes.NewReader(payload),
|
||||
})
|
||||
}
|
||||
@@ -83,15 +83,25 @@ func TestUseCase_ResolveRecipient_ReturnsErrMailboxQuotaExceededWhenAnyFanOutTar
|
||||
}
|
||||
|
||||
// fakeAliasFanOutRepository simulates an alias resolving to multiple mailboxes.
|
||||
//
|
||||
// localAddress is the one address it hosts. It used to answer for any address
|
||||
// at all, which made every recipient look local - harmless while nothing
|
||||
// consulted it on the delivery path, and wrong the moment local delivery
|
||||
// started asking. The real repository only ever resolves addresses on an
|
||||
// active local domain.
|
||||
type fakeAliasFanOutRepository struct {
|
||||
targets []port.MailboxRecord
|
||||
localAddress string
|
||||
targets []port.MailboxRecord
|
||||
}
|
||||
|
||||
func (f *fakeAliasFanOutRepository) FindActiveByAddress(_ context.Context, _ string) (*port.MailboxRecord, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (f *fakeAliasFanOutRepository) ResolveDeliveryTargets(_ context.Context, _ string) ([]port.MailboxRecord, error) {
|
||||
func (f *fakeAliasFanOutRepository) ResolveDeliveryTargets(_ context.Context, address string) ([]port.MailboxRecord, error) {
|
||||
if f.localAddress != "" && address != f.localAddress {
|
||||
return []port.MailboxRecord{}, nil
|
||||
}
|
||||
return f.targets, nil
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user