more review fix

src/backend/core/services/dns/check.py

@@ -42,13 +42,8 @@ SPF_TERM_NAME_RE = re.compile(r"[^:=/]*")
 # Ordered from most permissive to strictest (RFC 7208 4.6.2).
 SPF_ALL_STRICTNESS = {"+all": 0, "?all": 1, "~all": 2, "-all": 3}
 SPF_ALL_MECHANISMS = frozenset(SPF_ALL_STRICTNESS)
-
-# RFC 7208 3.3 and RFC 6376 3.6.2: the strings of a single TXT record are
-# concatenated with no separator, which is how records over 255 octets are
-# published. Some local resolvers (e.g. systemd-resolved) instead merge
-# separate TXT records into one RR; those show up as a later string opening
-# its own record, and have to stay apart.
-TXT_RECORD_START_RE = re.compile(r"v=(spf1( |$)|DMARC1\b)", re.IGNORECASE)
+# RFC 7208 4.7: a record with no "all" ends in an implicit "?all".
+SPF_IMPLICIT_ALL = "?all"
 
 
 def normalize_txt_value(value: str) -> str:
@@ -244,13 +239,16 @@ def _check_spf(expected_value: str, found_values: List[str]) -> Dict[str, any]:
 def _all_is_acceptable(expected_all: Optional[str], found_all: Optional[str]) -> bool:
     """Whether a found "all" mechanism is at least as strict as expected.
 
-    "~all" also passes for an expected "-all": softfail is where most domains
-    start, and it does not keep us from sending.
+    A record with no "all" is read as the implicit "?all" it ends in, on
+    either side: without it, an expected value carrying no "all" of its own
+    could never be matched. "~all" also passes for an expected "-all":
+    softfail is where most domains start, and it does not keep us from
+    sending.
     """
+    expected_all = expected_all or SPF_IMPLICIT_ALL
+    found_all = found_all or SPF_IMPLICIT_ALL
     if expected_all == found_all:
         return True
-    if expected_all is None or found_all is None:
-        return False
     if expected_all == "-all" and found_all == "~all":
         return True
     return SPF_ALL_STRICTNESS[found_all] > SPF_ALL_STRICTNESS[expected_all]
@@ -334,8 +332,7 @@ def _resolve_spf_includes(
             answers = dns.resolver.resolve(include_domain, "TXT")
             spf_records = [
                 value
-                for rr in answers.rrset
-                for value in _txt_record_values(rr)
+                for value in (_txt_record_value(rr) for rr in answers.rrset)
                 if is_spf_record(value)
             ]
         except (dns.resolver.NXDOMAIN, dns.resolver.NoAnswer):
@@ -368,22 +365,20 @@ def _resolve_spf_includes(
     return resolved, visited, transient, None
 
 
-def _txt_record_values(rr) -> List[str]:
-    """Normalized TXT values carried by a single resource record.
+def _txt_record_value(rr) -> str:
+    """Normalized value of a single TXT resource record.
 
-    Its strings belong to one record and are concatenated, unless a later one
-    opens a record of its own — the sign of a resolver having merged separate
-    records into one RR. SPF records are US-ASCII (RFC 7208 3.1), but an
-    unrelated TXT record at the same name may hold anything, and that is no
-    reason to fail the whole check.
+    RFC 7208 3.3 and RFC 6376 3.6.2: the strings of one record are
+    concatenated with no separator, which is how a value over the 255-octet
+    limit on a character-string is published. Separate TXT records arrive as
+    separate resource records, so several strings here always belong to the
+    same record. SPF records are US-ASCII (RFC 7208 3.1), but an unrelated
+    TXT record may hold anything, and that is no reason to fail the check.
     """
-    strings = [s.decode(errors="replace") for s in rr.strings]
-    if any(TXT_RECORD_START_RE.match(s) for s in strings[1:]):
-        return [normalize_txt_value(s) for s in strings]
-    return [normalize_txt_value("".join(strings))]
+    return normalize_txt_value(b"".join(rr.strings).decode(errors="replace"))
 
 
-def _resolve_dns_values(record_type, target, query_name):
+def _resolve_dns_values(record_type, query_name):
     """Resolve DNS and return found values and normalized expected value flag."""
     if record_type.upper() == "MX":
         answers = dns.resolver.resolve(query_name, "MX")
@@ -391,16 +386,7 @@ def _resolve_dns_values(record_type, target, query_name):
 
     if record_type.upper() == "TXT":
         answers = dns.resolver.resolve(query_name, "TXT")
-        values = []
-        for rr in answers.rrset:
-            if target.endswith("._domainkey"):
-                # DKIM: concatenate strings (long key split across strings)
-                values.append(
-                    normalize_txt_value(b"".join(rr.strings).decode(errors="replace"))
-                )
-            else:
-                values.extend(_txt_record_values(rr))
-        return values
+        return [_txt_record_value(rr) for rr in answers.rrset]
 
     answers = dns.resolver.resolve(query_name, record_type)
     return [answer.to_text() for answer in answers]
@@ -449,7 +435,7 @@ def check_single_record(
     query_name = f"{target}.{maildomain.name}" if target else maildomain.name
 
     try:
-        found_values = _resolve_dns_values(record_type, target, query_name)
+        found_values = _resolve_dns_values(record_type, query_name)
         if record_type.upper() == "TXT":
             expected_value = normalize_txt_value(expected_value)
 

src/backend/core/tests/dns/test_check.py

@@ -407,12 +407,16 @@ class TestDNSChecking:  # pylint: disable=too-many-public-methods
 
             assert result["status"] == "correct"
 
-    def test_check_single_record_spf_found_when_resolver_merges_txt_records(
+    def test_check_single_record_spf_found_among_other_txt_records(
         self, maildomain_factory
     ):
-        """Regression: some local resolvers (e.g. systemd-resolved) merge
-        separate TXT records into a single RR with multiple strings. SPF must
-        still be found by iterating individual strings."""
+        """SPF must be found when the name carries other TXT records too.
+
+        Separate TXT records arrive as separate resource records, each with
+        its own strings: the RDATA boundary is part of the record framing, so
+        no resolver can merge two records into one. Several strings on one
+        record therefore always belong together (RFC 7208 3.3).
+        """
         maildomain = maildomain_factory(name="example.com")
         expected_record = {
             "type": "TXT",
@@ -421,15 +425,10 @@ class TestDNSChecking:  # pylint: disable=too-many-public-methods
         }
 
         with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
-            # Single RR with two strings (merged by local resolver)
-            merged_rr = MagicMock()
-            merged_rr.strings = (
-                b"google-site-verification=abc123",
-                b"v=spf1 include:_spf.example.com -all",
+            mock_resolve.return_value = _txt_answer(
+                "google-site-verification=abc123",
+                "v=spf1 include:_spf.example.com -all",
             )
-            answer = MagicMock()
-            answer.rrset = [merged_rr]
-            mock_resolve.return_value = answer
 
             result = check_single_record(maildomain, expected_record)
             assert result["status"] == "correct"
@@ -1422,7 +1421,9 @@ class TestSPFValidRecordsAreNotFlagged:
 
     def test_record_split_across_several_strings(self, maildomain_factory):
         """RFC 7208 3.3: the strings of one TXT record are concatenated with no
-        separator, which is how records over 255 octets are published."""
+        separator, which is how a value over the 255-octet character-string
+        limit is published. Publishers split where they like, not only at
+        exactly 255 octets, so the split point carries no meaning."""
         maildomain = maildomain_factory(name="example.com")
         expected_record = {
             "type": "TXT",
@@ -1555,6 +1556,50 @@ class TestSPFValidRecordsAreNotFlagged:
             result = check_single_record(maildomain, expected_record)
             assert result["status"] == "correct"
 
+    def test_expected_value_without_an_all_mechanism(self, maildomain_factory):
+        """A configured expected value carrying no "all" ends in the implicit
+        "?all" (RFC 7208 4.7); a stricter published one satisfies it. Reading
+        it as "no acceptable all exists" would make the check unsatisfiable."""
+        maildomain = maildomain_factory(name="example.com")
+        expected_record = {
+            "type": "TXT",
+            "target": "",
+            "value": "v=spf1 include:_spf.example.com",
+        }
+
+        with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
+            mock_resolve.side_effect = self._resolver(
+                {
+                    "example.com": _txt_answer("v=spf1 include:_spf.example.com -all"),
+                    "_spf.example.com": _txt_answer("v=spf1 ip4:1.2.3.4 -all"),
+                }
+            )
+
+            result = check_single_record(maildomain, expected_record)
+            assert result["status"] == "correct"
+
+    def test_expected_value_without_an_all_still_rejects_plus_all(
+        self, maildomain_factory
+    ):
+        """The implicit "?all" is still stricter than a published "+all"."""
+        maildomain = maildomain_factory(name="example.com")
+        expected_record = {
+            "type": "TXT",
+            "target": "",
+            "value": "v=spf1 include:_spf.example.com",
+        }
+
+        with patch("core.services.dns.check.dns.resolver.resolve") as mock_resolve:
+            mock_resolve.side_effect = self._resolver(
+                {
+                    "example.com": _txt_answer("v=spf1 include:_spf.example.com +all"),
+                    "_spf.example.com": _txt_answer("v=spf1 ip4:1.2.3.4 -all"),
+                }
+            )
+
+            result = check_single_record(maildomain, expected_record)
+            assert result["status"] == "insecure"
+
     def test_unrelated_non_ascii_txt_record(self, maildomain_factory):
         """SPF records are US-ASCII (RFC 7208 3.1), but another TXT record at
         the same name may hold anything, and decoding it must not fail the