mirror of
https://github.com/Sea-Haven-Industries/procurement-ingest.git
synced 2026-10-03 17:43:12 +00:00
Harden AR parser per cross-family review
Cross-family (GPT-4.1) review findings: terminate the dkim result token at end-of-clause, whitespace, or a comment so a value like "dkim=pass-fake" can never be read as a pass; normalize trailing dots off allowlist entries so "seahaven.com." matches; make the compat32 parser policy explicit. Adds tests for result-token boundaries, comments after the result, quoted domain values, and folding inside a dkim clause. Refs: INFRA-107
This commit is contained in:
parent
447b0619ad
commit
fa5f60208f
3 changed files with 48 additions and 7 deletions
|
|
@ -30,6 +30,7 @@ Note SES reports the passing DKIM identity as ``header.i=@<domain>``
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import email.parser
|
import email.parser
|
||||||
|
import email.policy
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
|
@ -43,7 +44,9 @@ SES_AUTHSERV_ID = "amazonses.com"
|
||||||
# One resinfo clause of an Authentication-Results value, e.g.
|
# One resinfo clause of an Authentication-Results value, e.g.
|
||||||
# "dkim=pass header.i=@seahaven.com". The clause must START with the
|
# "dkim=pass header.i=@seahaven.com". The clause must START with the
|
||||||
# method=result pair; header.d= / header.i= may appear anywhere after it.
|
# method=result pair; header.d= / header.i= may appear anywhere after it.
|
||||||
_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)", re.IGNORECASE)
|
# The result token must be terminated by end-of-clause, whitespace, or a
|
||||||
|
# comment so "dkim=pass-anything" can never be read as "pass".
|
||||||
|
_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|[\s(])", re.IGNORECASE)
|
||||||
_HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
_HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
_HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
_HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
|
|
||||||
|
|
@ -51,7 +54,9 @@ _HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
def get_allowed_dkim_domains() -> frozenset:
|
def get_allowed_dkim_domains() -> frozenset:
|
||||||
"""Read the DKIM-domain allowlist from the environment (may be empty)."""
|
"""Read the DKIM-domain allowlist from the environment (may be empty)."""
|
||||||
raw = os.environ.get(ALLOWED_DKIM_DOMAINS_ENV, "")
|
raw = os.environ.get(ALLOWED_DKIM_DOMAINS_ENV, "")
|
||||||
return frozenset(d.strip().lower().lstrip("@") for d in raw.split(",") if d.strip())
|
return frozenset(
|
||||||
|
d.strip().lower().lstrip("@").rstrip(".") for d in raw.split(",") if d.strip()
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _unfold(value: str) -> str:
|
def _unfold(value: str) -> str:
|
||||||
|
|
@ -108,7 +113,9 @@ def evaluate_sender_authentication(raw_email: bytes, allowed_domains) -> tuple:
|
||||||
# compat32 keeps header values as raw strings (we unfold ourselves)
|
# compat32 keeps header values as raw strings (we unfold ourselves)
|
||||||
# and never raises on structurally odd headers; headersonly avoids
|
# and never raises on structurally odd headers; headersonly avoids
|
||||||
# parsing the body at all.
|
# parsing the body at all.
|
||||||
msg = email.parser.BytesParser().parsebytes(raw_email, headersonly=True)
|
msg = email.parser.BytesParser(policy=email.policy.compat32).parsebytes(
|
||||||
|
raw_email, headersonly=True
|
||||||
|
)
|
||||||
except Exception:
|
except Exception:
|
||||||
return False, "unparseable_message", {}
|
return False, "unparseable_message", {}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -30,6 +30,7 @@ Note SES reports the passing DKIM identity as ``header.i=@<domain>``
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import email.parser
|
import email.parser
|
||||||
|
import email.policy
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
|
@ -43,7 +44,9 @@ SES_AUTHSERV_ID = "amazonses.com"
|
||||||
# One resinfo clause of an Authentication-Results value, e.g.
|
# One resinfo clause of an Authentication-Results value, e.g.
|
||||||
# "dkim=pass header.i=@seahaven.com". The clause must START with the
|
# "dkim=pass header.i=@seahaven.com". The clause must START with the
|
||||||
# method=result pair; header.d= / header.i= may appear anywhere after it.
|
# method=result pair; header.d= / header.i= may appear anywhere after it.
|
||||||
_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)", re.IGNORECASE)
|
# The result token must be terminated by end-of-clause, whitespace, or a
|
||||||
|
# comment so "dkim=pass-anything" can never be read as "pass".
|
||||||
|
_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|[\s(])", re.IGNORECASE)
|
||||||
_HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
_HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
_HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
_HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
|
|
||||||
|
|
@ -51,7 +54,9 @@ _HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE)
|
||||||
def get_allowed_dkim_domains() -> frozenset:
|
def get_allowed_dkim_domains() -> frozenset:
|
||||||
"""Read the DKIM-domain allowlist from the environment (may be empty)."""
|
"""Read the DKIM-domain allowlist from the environment (may be empty)."""
|
||||||
raw = os.environ.get(ALLOWED_DKIM_DOMAINS_ENV, "")
|
raw = os.environ.get(ALLOWED_DKIM_DOMAINS_ENV, "")
|
||||||
return frozenset(d.strip().lower().lstrip("@") for d in raw.split(",") if d.strip())
|
return frozenset(
|
||||||
|
d.strip().lower().lstrip("@").rstrip(".") for d in raw.split(",") if d.strip()
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _unfold(value: str) -> str:
|
def _unfold(value: str) -> str:
|
||||||
|
|
@ -108,7 +113,9 @@ def evaluate_sender_authentication(raw_email: bytes, allowed_domains) -> tuple:
|
||||||
# compat32 keeps header values as raw strings (we unfold ourselves)
|
# compat32 keeps header values as raw strings (we unfold ourselves)
|
||||||
# and never raises on structurally odd headers; headersonly avoids
|
# and never raises on structurally odd headers; headersonly avoids
|
||||||
# parsing the body at all.
|
# parsing the body at all.
|
||||||
msg = email.parser.BytesParser().parsebytes(raw_email, headersonly=True)
|
msg = email.parser.BytesParser(policy=email.policy.compat32).parsebytes(
|
||||||
|
raw_email, headersonly=True
|
||||||
|
)
|
||||||
except Exception:
|
except Exception:
|
||||||
return False, "unparseable_message", {}
|
return False, "unparseable_message", {}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -82,6 +82,32 @@ class TestParseAuthenticationResults:
|
||||||
assert authserv_id == ""
|
assert authserv_id == ""
|
||||||
assert passing == frozenset()
|
assert passing == frozenset()
|
||||||
|
|
||||||
|
def test_result_token_boundary(self, ses_auth):
|
||||||
|
# "pass-anything" / "passfail" must never be read as "pass".
|
||||||
|
for result in ("pass-fake", "passfail"):
|
||||||
|
_, passing = ses_auth.parse_authentication_results(
|
||||||
|
f"amazonses.com; dkim={result} header.i=@seahaven.com"
|
||||||
|
)
|
||||||
|
assert passing == frozenset()
|
||||||
|
|
||||||
|
def test_result_followed_by_comment(self, ses_auth):
|
||||||
|
_, passing = ses_auth.parse_authentication_results(
|
||||||
|
"amazonses.com; dkim=pass(good signature) header.i=@seahaven.com"
|
||||||
|
)
|
||||||
|
assert passing == frozenset({"seahaven.com"})
|
||||||
|
|
||||||
|
def test_quoted_domain_value(self, ses_auth):
|
||||||
|
_, passing = ses_auth.parse_authentication_results(
|
||||||
|
'amazonses.com; dkim=pass header.i="@seahaven.com"'
|
||||||
|
)
|
||||||
|
assert passing == frozenset({"seahaven.com"})
|
||||||
|
|
||||||
|
def test_folding_inside_dkim_clause(self, ses_auth):
|
||||||
|
_, passing = ses_auth.parse_authentication_results(
|
||||||
|
"amazonses.com;\r\n dkim=pass\r\n header.i=@seahaven.com"
|
||||||
|
)
|
||||||
|
assert passing == frozenset({"seahaven.com"})
|
||||||
|
|
||||||
|
|
||||||
class TestEvaluateSenderAuthentication:
|
class TestEvaluateSenderAuthentication:
|
||||||
def test_ses_stamped_pass_accepted(self, ses_auth):
|
def test_ses_stamped_pass_accepted(self, ses_auth):
|
||||||
|
|
@ -189,7 +215,8 @@ class TestAuthenticateInboundEmail:
|
||||||
assert ses_auth.authenticate_inbound_email(raw(WO_SES_HEADER), self.S3_KEY)
|
assert ses_auth.authenticate_inbound_email(raw(WO_SES_HEADER), self.S3_KEY)
|
||||||
|
|
||||||
def test_allowlist_is_comma_separated_and_normalized(self, ses_auth, monkeypatch):
|
def test_allowlist_is_comma_separated_and_normalized(self, ses_auth, monkeypatch):
|
||||||
monkeypatch.setenv("ALLOWED_DKIM_DOMAINS", " Other.Example , @SEAHAVEN.com ,")
|
# Stray spaces, case, a leading @, and a trailing dot all normalize.
|
||||||
|
monkeypatch.setenv("ALLOWED_DKIM_DOMAINS", " Other.Example , @SEAHAVEN.com. ,")
|
||||||
assert ses_auth.authenticate_inbound_email(raw(WO_SES_HEADER), self.S3_KEY)
|
assert ses_auth.authenticate_inbound_email(raw(WO_SES_HEADER), self.S3_KEY)
|
||||||
|
|
||||||
def test_env_var_unset_fails_closed(self, ses_auth, monkeypatch, caplog):
|
def test_env_var_unset_fails_closed(self, ses_auth, monkeypatch, caplog):
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue