Deleting a domain relayed its mail off-site, and seven more subsystems audited

Eight areas taken row by row against the roadmap, then the defects that exposed. The
first one is a mail-confidentiality defect, not an untidiness defect, and it had been
there as long as the cascade had.

DELETING OR RENAMING A DOMAIN SENT ITS MAIL TO A STRANGER

An address at a deleted domain stops being local. RecipientParser then finds no
account, no alias and no list for it, falls through to the route/external branch, and
adds it as a NON-LOCAL recipient - so the message is relayed to whatever MX now
answers for that name on the internet. On the alias path bTreatSecurityAsLocal is
already set at recursion level 1, so the relay restriction does not stop it either.
Anyone who registers a domain an installation used to host receives its mail.

The delete cascade removed accounts, aliases, lists, domain aliases, the hm_domains
row and the directory tree - and left every reference that OTHER domains held into the
deleted one. Each is now handled in the direction that fails closed and stays visible:
an alias is switched off with its target left in the row so the administrator can see
what it was, forwarding likewise, and a list membership - which has no active flag -
is removed and logged with the address and the list so it can be put back. Every name
the domain answered to is covered, including its domain aliases, collected before
those rows are deleted. The sweep runs only after "delete from hm_domains" succeeds,
so a failed delete cannot disable references into a domain that still exists.

Rename had the same hole. NameChanger::RenameDomain iterates only the renamed domain's
own accounts, aliases and lists, so a second local domain holding an alias target, a
list membership or a forward address at the renamed domain kept the old name - and the
old name is no longer hosted, so the same off-site relay applied. PersistentDomain::
SaveObject now sweeps the three columns that can name an address in another domain.

The row claiming "account rename rewrites dependent data" was wrong in both halves.
The domain half now works as described. The account half is declared instead of
claimed: NameChanger::RenameAccount renames the mailbox directory and nothing else.
Its exposure is materially lower - a stale reference to a renamed account is still at
a local domain, so mail bounces at 550 or hits the catch-all rather than leaving the
building - and fixing it means a policy call about deleted accounts that was not worth
making unilaterally.

Deleting a group never deleted its membership rows: there was no DeleteByGroup
anywhere, only DeleteByAccount, and with no foreign key in the schema those rows
survive for the life of the database. Dead weight while the id is unused, and worse if
it is ever reissued, because a group is an ACL principal.

And the distribution-list limit tested the NUMBER where the account and alias limits
test their enable FLAG. The Control Panel draws it as a checkbox beside a number and
leaves the number alone when the box is cleared, so clearing that checkbox did nothing
at all - the stale number went on being enforced - while ticking it with the number at
0 enforced nothing.

A CRASH REPORTER THAT HID CRASHES

Four defects, all in the part that runs when things are already going wrong. The report
did not say which fault it was: exception_code was a parameter of ExceptionLogger::Log
that the file never referenced anywhere, so an access violation, a stack overflow and
a divide by zero were announced with the identical sentence - and an unreferenced
parameter is C4100, a /W4 warning, which is why /W3 /WX never caught it. A crash that
could not be dumped was not reported AT ALL: the refusal went to LOG_DEBUG, which is
off by default, and the function returned before the error-log entry, so the eleventh
fault in a four-hour window and every one after it was invisible - and the eleventh is
often the one that finally makes somebody look. The reporter could also kill the
service while describing a crash, because ExceptionHandler::Run calls it from inside
the __except FILTER EXPRESSION and three calls in it can throw, including a
windows_shared_memory create whose segment name is the fixed, unqualified
"hMailServerMiniDumpMemory" - a second hMailServer process beside the service was
enough. And the wait on the dump writer was INFINITE, on a pool thread holding the
reporter's mutex.

The AWStats delivery journal sanitised the two addresses and not the other fields. The
one that most needed it was %host_r, filled from the MX target hostname on the
outbound path - a value served by whoever runs the recipient's domain. A CR or LF in
it forges a complete, plausible delivery record in a stream AWStats parses by field
position.

OnError did not fire for every error, and the ones it skipped were the ones worth
forwarding. The event is invoked by building script source text, and ReportError
flattened newlines for the log line it writes while pasting the RAW description into
the generated call - so any error whose text contains a newline produced a call that
neither VBScript nor JScript can parse. The handler never ran. Script compile reports
are literally "File: <path><CRLF>Script Error: ...", so this is not an obscure class.
Reporting an error from inside the handler can no longer recurse to stack exhaustion
either.

TLS: AN EXPIRED CERTIFICATE WAS SERVED IN SILENCE

OpenSSL checks the validity window of certificates it VERIFIES, never of the one it is
told to SERVE. An expired certificate loaded without complaint: the listener bound,
TLS was offered, and every client that checks the date refused - with no error, no
application-log line, and only an ordinary aborted session in the TCP/IP log. Both
ends of the window are now reported, naming the file, the date and the listener.
Deliberately a report and not a refusal: mail ports disappearing the moment a
certificate expires turns a degraded server into an unreachable one.

Clearing all four TLS version bits bound every listener and then failed every handshake
with "no protocols available", silently; it now reports and falls back to TLS 1.2+1.3.
ACME renewal threw away RequestCertificate's result and nine failure paths returned
without logging, so a failing renewal left one hopeful line an hour in the log right up
to expiry; and the new pair was installed by overwriting privkey.pem first, which could
leave a new key beside the previous certificate - a mismatch InitServer refuses, taking
every port using that certificate off the air at the next restart. It now writes
.pem.new and renames twice.

And the post-quantum key exchange shipped yesterday did not reach outbound delivery at
all. InitClient called SetContextOptions_ and SetCipherList_ and never
SetKeyExchangeGroups_, so every outbound SMTP session, the external POP3 fetcher and
the ACME client offered OpenSSL's default groups while TlsKeyExchangeGroups was
ignored. The release note said otherwise. One line, and the roadmap row is corrected
rather than quietly widened.

THE POP3 FETCHER TRUSTED A MACHINE THE ADMINISTRATOR DOES NOT RUN

Six defects, five of them losing or duplicating mail. A -ERR to RETR abandoned the
whole session, and because the mailbox is walked in ascending message order, one
message the server listed and would not hand over stopped everything behind it from
being collected - on that check and every check afterwards, silently. A UIDL line with
no space was not rejected but guessed at, becoming the unique-id of "message 0", the
lowest key in the map and therefore the first thing requested. A unique-id longer than
the 255-character column could not be recorded at all, so the message was
re-downloaded and re-delivered on every check for ever. MessageCleanup_ dereferenced
GetUID's result unchecked - an access violation, so the whole service - reachable from
a server that gives one unique-id to two message numbers. The UIDL listing had no size
ceiling whatsoever.

The duplicate-delivery fix is the one with teeth. The local record of a downloaded
message was deliberately NOT written when the account deletes immediately, and was
discarded the instant DELE was handed to the socket with the response never examined.
Anything interrupting the session between delivery and confirmed deletion left the
message on the server with no local trace that it had already been delivered.

RESTORE, THE MESSAGE STORE, COM, AND SCOPED API KEYS

A restore used to empty a category the archive does not contain and report success,
because Collection::XMLLoad cannot tell "empty at backup time" from "absent from this
archive"; it now refuses, with the server untouched, and refuses an archive written by
a later version - which is what the index's Version attribute has been there for since
2010. The live-store copy no longer uses FileUtilities::CopyDirectory, whose throwing
copy_file escaped the work-queue thread and terminated the service, which with
CompressDestinationFiles off happened on the second backup an installation took.

PersistentMessage::GetPartialFilename's public-folder branch compared the wrong
substring and could never match, so nothing under #Public was recognised as part of the
store. MessageData::Write now refuses to serialise a MimeBody whose load gave up -
the load leaves the body EMPTY, and writing it replaced the message with a stub - and
the missing-file placeholder no longer stores a size of 0 that AddObject then refuses,
which made that failure invisible twice over.

A MessageHeader object outlived the header it named: MimeHeader keeps fields in a
vector BY VALUE and the COM object cached the raw MimeField*, so adding a header
(which may reallocate) or deleting one (which move-assigns the rest down a slot) left
a script writing its value onto a header it had never asked for, in a message then
saved to disk. Scripting.Language accepted any string with S_OK, and ScriptServer
compares it case-sensitively against two literals - so "vbscript" switched every
handler off in silence, OnClientLogon and OnClientValidatePassword included.
AntiVirus.Action ran a two-case switch with no break on the second and no default over
an uninitialised local. Four getters returned S_OK without ever writing their out
parameter.

The REST API's "scoped" keys were the administrator password with a different spelling:
one minted for a monitoring probe could DELETE any account in any domain and empty the
delivery queue, while the header comment called them scoped. Scope=readonly is now the
default for a create request that names none, Domains= confines a key to named
domains, and no key of any scope can create or revoke keys - otherwise a narrow key
escalates to an unscoped one in a single request.

TESTS AND WIRING

17 new fixtures, 74 tests, all passing. Every new file had to be registered by hand:
RegressionTests.csproj lists all its sources explicitly, and three new C++ files needed
hMailServer.vcxproj - both are central files the parallel work was not allowed to
touch, which is exactly why they were the integration's job. Three defects were fixed
by hand from the reports rather than by an agent, for the same reason: the
distribution-list enable flag lives in PreSaveLimitationsCheck.cpp, outside the
ownership of the developer who found it, and its test was red until it was applied.

One build fix of my own: SslContextInitializer used X509_cmp_current_time, which
OpenSSL 4.0 deprecates and /WX rejects. It now uses ASN1_TIME_cmp_time_t, the idiom
already in AcmeClient and MetricsServer, and tests the exact return value rather than
its sign - because -2 means "could not parse" and "< 0" would report an unparseable
notAfter as an expired certificate.

ONE EXISTING TEST NOW EXPECTS THE OPPOSITE, DELIBERATELY

POP3/Fetching/ServerBehaviors.cs TestDisconnectAfterRetrCommand asserted that two
fetches of one message produce TWO messages in the inbox. That 2 was the duplicate
delivery, not a requirement: the first session delivers the message and loses the
connection before the DELE is acknowledged, so the message is still on the remote
server. It now asserts 1, and the reason is written at the assertion.

Changing a test to match new behaviour is how a regression gets waved through, so:
the message is still collected exactly once, the remote copy is still deleted - the
already-downloaded branch adds it to downloaded_messages_, so the second session issues
the DELE - and the UID row is removed when that DELE is acknowledged. One copy in the
inbox, nothing left behind on the server. The developer who made the change predicted
this exact test would fail, in its report, having cross-checked every existing fixture
that drives a fetch and written the replacement assertion out in full. It was a
prediction, not a discovery during integration, which is the part worth trusting.

AND THE TEST RUNNER STOPPED HIDING FAILURES

build\run-tests.ps1 hard-coded NUnit's /stoponerror, which abandons the entire run at
the first error. A single failure in an early fixture therefore reported "Test Count:
240" out of about 1,275 and said nothing at all about the other thousand tests - twice
this afternoon, on the same integration. It is now a -StopOnError switch, off by
default, so a gate run reports every failure at once; and a -Where passthrough, so a
subset can be run without hand-building an nunit3-console command line.

HOW THIS WAS VERIFIED, PRECISELY

Release build clean, and the full regression suite green on the integrated tree - this
commit together with the one before it. The split is presentational, so each
subsystem's roadmap rows sit with the code that earned them; the two touch disjoint
source files and each carries the project-file and Roadmap.md changes it needs, but
neither was built or tested in isolation.

The parallel work itself built nothing and ran nothing, by instruction - which is why
integration found three things it could not: an OpenSSL deprecation under /WX, three
source files absent from the project, and one C# namespace collision. That division is
working, but it means "the developer says it is done" and "it compiles" are separate
claims, and only the second one is checkable here.

ROADMAP

43 rows in this commit: 30 replaced, 13 added, and four ticks moved. Rows are corrected
in both directions - capabilities claimed that did not hold (post-quantum reaching
outbound delivery, "individually toggleable" domain limits, OnError firing for every
error) and rows marked open whose code ships. 766 items, 571 shipped;
build\check-roadmap.ps1 green.

Author: chrisholloway5

Roadmap.md

@@ -1,3 +1,17 @@
+Param(
+	# NUnit's /stoponerror was hard-coded here, and it abandons the whole run at the first
+	# error - so a single failure in an early fixture reported "Test Count: 240" out of
+	# ~1275 and said nothing whatsoever about the other thousand tests. That is the wrong
+	# default for a regression gate: after a large integration you need every failure in
+	# one run, not the alphabetically first one, and it cost two full runs to learn that
+	# twice in one afternoon. It stays available for the case it is actually good for -
+	# bisecting a crash that poisons everything after it.
+	[switch]$StopOnError,
+	# Passed to --where, so a subset can be run without hand-building an nunit3-console
+	# command line: -Where "class =~ /RegressionTests.IMAP/".
+	[string]$Where
+)
+
 # Determine repository root relative to this script (script is in the `build` folder)
 $repoRoot = (Get-Item $PSScriptRoot).Parent.FullName
 
@@ -25,9 +39,10 @@ Write-Host "  Assembly: $testAssembly"
 # Add helpful NUnit arguments to show test names as they run
 $nunitArgs = @(
     $testAssembly,
-    '--labels=All',
-    '/stoponerror'
+    '--labels=All'
 )
+if ($StopOnError) { $nunitArgs += '/stoponerror' }
+if ($Where)       { $nunitArgs += "--where=$Where" }
 
 Write-Host "Starting NUnit runner (streaming output)..." -ForegroundColor Cyan
 

build/run-tests.ps1

@@ -261,10 +261,18 @@ STDMETHODIMP InterfaceAntiVirus::put_Action(eAntivirusAction newVal)
          break;
       case hDeleteAttachments:
          iAction = HM::AntiVirusConfiguration::ActionStripAttachments;
+         break;
+      default:
+         // Refused rather than stored. There was no default here and iAction was
+         // uninitialised, so a caller passing anything outside the enum - and a
+         // scripting language hands this property an ordinary integer, with no
+         // enum to constrain it - wrote whatever happened to be in that stack slot
+         // into the persisted action taken on a virus-bearing message.
+         return COMError::GenerateError("Antivirus action must be 0 (delete the message) or 1 (strip the attachments).");
       }
-   
+
       antiVirusConfiguration_.AVAction (iAction);
-   
+
       return S_OK;
    }
    catch (...)

hmailserver/source/Server/COM/InterfaceAntiVirus.cpp

@@ -37,8 +37,21 @@ STDMETHODIMP InterfaceBlockedAttachments::get_Count(long *pVal)
 {
    try
    {
+      if (!pVal)
+         return E_POINTER;
+
+      // Guarded, as Refresh already was. BlockedAttachments is a registered coclass, so
+      // one can be created directly rather than obtained from AntiVirus, and one created
+      // that way has no collection attached for this to count.
+      //
+      // Refused rather than answered S_FALSE, which Refresh uses: S_FALSE is a success
+      // code, so a caller would go on to read an out parameter nobody had written -
+      // which is the same defect as the one this guard is here to close.
+      if (!blocked_attachments_)
+         return COMError::GenerateError("This BlockedAttachments collection is not attached to anything. Obtain it from Settings.AntiVirus.BlockedAttachments rather than creating it directly.");
+
       *pVal = blocked_attachments_->GetCount();
-   
+
       return S_OK;
    }
    catch (...)
@@ -47,19 +60,30 @@ STDMETHODIMP InterfaceBlockedAttachments::get_Count(long *pVal)
    }
 }
 
-STDMETHODIMP 
+STDMETHODIMP
 InterfaceBlockedAttachments::get_Item(long Index, IInterfaceBlockedAttachment **pVal)
 {
    try
    {
-      CComObject<InterfaceBlockedAttachment>* pInterfaceBlockedAttachment = new CComObject<InterfaceBlockedAttachment>();
-      pInterfaceBlockedAttachment->SetAuthentication(authentication_);
-   
+      if (!pVal)
+         return E_POINTER;
+
+      *pVal = nullptr;
+
+      if (!blocked_attachments_)
+         return COMError::GenerateError("This BlockedAttachments collection is not attached to anything. Obtain it from Settings.AntiVirus.BlockedAttachments rather than creating it directly.");
+
+      // Looked up before the wrapper is created, not after: the wrapper used to be
+      // constructed first and abandoned on the bad-index path, and a CComObject that
+      // has never been AddRef'd is never freed.
       std::shared_ptr<HM::BlockedAttachment> pBA = blocked_attachments_->GetItem(Index);
-   
+
       if (!pBA)
          return DISP_E_BADINDEX;
-   
+
+      CComObject<InterfaceBlockedAttachment>* pInterfaceBlockedAttachment = new CComObject<InterfaceBlockedAttachment>();
+      pInterfaceBlockedAttachment->SetAuthentication(authentication_);
+
       pInterfaceBlockedAttachment->AttachItem(pBA);
       pInterfaceBlockedAttachment->AttachParent(blocked_attachments_, true);
       pInterfaceBlockedAttachment->AddRef();

hmailserver/source/Server/COM/InterfaceBlockedAttachments.cpp

@@ -8,6 +8,19 @@
 
 #include "../Common/Scripting/ClientInfo.h"
 
+namespace
+{
+   // Client is a registered coclass with a ProgID of its own, so it can be created
+   // directly rather than received from the script engine - and one created that way
+   // has no session attached. Every accessor below dereferenced the empty shared_ptr.
+   // See the same comment in InterfaceResult.cpp: /EHa is the only reason that read
+   // turned into a COM error rather than the end of the service process.
+   HRESULT NotAttached_()
+   {
+      return COMError::GenerateError("This Client object is not attached to a session. The Client object is handed to a script event handler by the server; it cannot be created directly.");
+   }
+}
+
 // InterfaceClient
 
 void 
@@ -20,6 +33,12 @@ STDMETHODIMP InterfaceClient::get_Port(long *pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetPort();
       return S_OK;
    }
@@ -33,6 +52,12 @@ STDMETHODIMP InterfaceClient::get_IPAddress(BSTR *pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetIPAddress().AllocSysString();
       return S_OK;
    }
@@ -46,6 +71,12 @@ STDMETHODIMP InterfaceClient::get_Username(BSTR *pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetUsername().AllocSysString();
       return S_OK;
    }
@@ -59,6 +90,12 @@ STDMETHODIMP InterfaceClient::get_HELO(BSTR *pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetHELO().AllocSysString();
       return S_OK;
    }
@@ -72,6 +109,12 @@ STDMETHODIMP InterfaceClient::get_Authenticated(VARIANT_BOOL *pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetIsAuthenticated() ? VARIANT_TRUE : VARIANT_FALSE;
       return S_OK;
    }
@@ -85,6 +128,12 @@ STDMETHODIMP InterfaceClient::get_EncryptedConnection(VARIANT_BOOL* pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetIsEncryptedConnection() ? VARIANT_TRUE : VARIANT_FALSE;
       return S_OK;
    }
@@ -98,6 +147,12 @@ STDMETHODIMP InterfaceClient::get_CipherVersion(BSTR* pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetCipherVersion().AllocSysString();
       return S_OK;
    }
@@ -111,6 +166,12 @@ STDMETHODIMP InterfaceClient::get_CipherName(BSTR* pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetCipherName().AllocSysString();
       return S_OK;
    }
@@ -124,6 +185,12 @@ STDMETHODIMP InterfaceClient::get_CipherBits(long* pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetCipherBits();
       return S_OK;
    }
@@ -137,11 +204,17 @@ STDMETHODIMP InterfaceClient::get_SessionID(long* pVal)
 {
    try
    {
+      if (!client_info_)
+         return NotAttached_();
+
+      if (!pVal)
+         return E_POINTER;
+
       *pVal = client_info_->GetSessionID();
       return S_OK;
    }
    catch (...)
    {
       return COMError::GenerateGenericMessage();
    }
-}
+}

hmailserver/source/Server/COM/InterfaceClient.cpp

@@ -550,7 +550,16 @@ STDMETHODIMP InterfaceLogging::get_MaskPasswordsInLog(VARIANT_BOOL *pVal)
       if (!config_)
          return GetAccessDenied();
 
-      // OBSOLETE
+      if (!pVal)
+         return E_POINTER;
+
+      // OBSOLETE: masking is unconditional, which is what the IDL help string says.
+      // The value is written all the same. This returned S_OK without ever touching
+      // the out parameter, so a caller read back whatever was in its own stack slot
+      // and treated it as the server's answer - a property that reports a setting it
+      // never looked at is worse than one that is not there.
+      *pVal = VARIANT_TRUE;
+
       return S_OK;
    }
    catch (...)