fix(email): address postguard#197 deliverability + show disclosed signer name#170
Conversation
…ner name - X-PostGuard header now tracks the pg-core version via a new build.rs that parses Cargo.lock, instead of the stale hardcoded "0.1.0". When pg-core releases 1.0, the header auto-bumps on the next build. - Add Reply-To header pointing to the disclosed sender on recipient mail so replies reach the human sender, not the noreply From-address. - Add Auto-Submitted: auto-generated (RFC 3834) on both notification and confirmation mail to suppress responder loops and signal "transactional" to receiving MTAs. - Send multipart/alternative with a hand-authored plain-text branch in addition to HTML (new templates/email/email.txt). - Embed the PostGuard logo as a multipart/related CID inline part instead of fetching https://postguard.eu/pg_logo.png at render time. Kills the HTML-only + remote-image spam fingerprint and keeps the logo rendering even with images-blocked-by-default. - Show the sender's disclosed full name (pbdf.gemeente.personalData.fullname) wherever the bare email used to appear in the body; filter it from the attribute pill list so it doesn't render twice. Deliberately not added: List-Unsubscribe — wrong semantic fit for person-to-person mail, would misrepresent the sender. Refs encryption4all/postguard#197
|
Dobby has received the request! Routing to the right specialist now... |
CI fallout from the previous commit:
- The Docker build couldn't run build.rs because it wasn't COPY'd into
the planner / builder stages, so env!("PG_CORE_VERSION") had nothing
to resolve and the release build failed. Add `COPY build.rs ./build.rs`
to both stages.
- rustfmt wanted `build_body(...)`'s signature on a single line. Apply.
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Addresses postguard#197 correctly and the disclosed-name surfacing works. Blocking: the doc-comment block above build_body actually describes sender_display, leaving the latter undocumented; no test asserts the produced Message contains multipart/alternative / multipart/related / Content-ID: <pg-logo> — the whole point of the PR — so a future refactor can silently re-introduce the spam shape; and an empty disclosed fullname renders as a blank everywhere the email used to appear. Minor: pbdf-only fullname match excludes irma-demo (staging/dev), build.rs panic message could explain the header coupling, and a state.sender that fails to parse as Mailbox silently drops Reply-To without a log line. (Stage 1's cargo fmt finding was fixed in the second commit; verified locally.)
Rule compliance
Memory note repos/cryptify/x-postguard-header.md says cryptify's X-PostGuard value convention should be a semantic token (e.g. notification) over a version string; this PR keeps the value as a version string and now auto-tracks pg-core. Non-blocking, but a follow-up to switch to a semantic token would align the three PostGuard surfaces (tb-addon decrypted, cryptify notification).
| sender_attributes: &'a [(String, String)], | ||
| } | ||
|
|
||
| /// Resolve the display string and remaining attribute pills for the |
There was a problem hiding this comment.
[Code review] The first /// paragraph (lines 160-164) describes sender_display, not build_body. As written, sender_display at line 183 is undocumented and build_body carries a misleading dual description. Move lines 160-164 above fn sender_display.
| .position(|(t, _)| t == FULLNAME_ATYPE) | ||
| .map(|i| attrs.remove(i).1); | ||
| let display = name | ||
| .or_else(|| state.sender.clone()) |
There was a problem hiding this comment.
[Code review] An empty disclosed fullname ((FULLNAME_ATYPE, "")) produces display == "" and renders as a blank in the subject and body where the email used to appear. Treat empty disclosed values as not disclosed, e.g. .map(|i| attrs.remove(i).1).filter(|n| !n.is_empty()).
| .map(|i| attrs.remove(i).1); | ||
| let display = name | ||
| .or_else(|| state.sender.clone()) | ||
| .unwrap_or_else(|| "Someone".to_string()); |
There was a problem hiding this comment.
[Code review] PR description claims the helper is covered by two unit tests, but the three-branch fallback's Someone case (sender=None, no fullname) has no test. Add one or correct the PR body.
| /// IRMA/Yivi attribute identifier for the signer's full name. When this | ||
| /// attribute appears in `FileState.sender_attributes` we render the name | ||
| /// in place of the bare email everywhere the sender is shown in the body. | ||
| const FULLNAME_ATYPE: &str = "pbdf.gemeente.personalData.fullname"; |
There was a problem hiding this comment.
[Code review] Only pbdf.gemeente.personalData.fullname is recognised; irma-demo.gemeente.personalData.fullname (demo scheme used in staging/dev) is silently ignored, so staging won't exercise this path. Match on the suffix .gemeente.personalData.fullname or maintain a small allowlist — or add a comment if prod-only is intentional.
| .subject(subject) | ||
| .body(email)?; | ||
| .subject(subject); | ||
| if let Some(reply_to) = state |
There was a problem hiding this comment.
[Code review] When state.sender is Some(...) but doesn't parse as a Mailbox, the recipient mail silently ships with no Reply-To — one of the three deliverability fixes this PR is meant to guarantee. At least log::warn! on the parse failure so a malformed sender is observable.
| None | ||
| } | ||
| }) | ||
| .expect("pg-core entry not found in Cargo.lock"); |
There was a problem hiding this comment.
[Code review] Acceptable as a tripwire, but the panic message could mention that PG_CORE_VERSION drives the X-PostGuard header used by the Outlook add-in filter — saves the next reader a grep.
| } | ||
|
|
||
| const X_POSTGUARD_VERSION: &str = "0.1.0"; | ||
| const X_POSTGUARD_VERSION: &str = env!("PG_CORE_VERSION"); |
There was a problem hiding this comment.
[Rule: cryptify/x-postguard-header] Repo memory note says cryptify's X-PostGuard value convention should be a semantic token (e.g. notification), not a version string. The existing 0.1.0 was already a version string and this PR doubles down by auto-tracking pg-core. Non-blocking — but worth a follow-up to switch the value to a semantic token.
- Treat empty disclosed fullname as not disclosed; fall through to the email instead of rendering a blank where the sender appears. - Match the fullname attribute by `.gemeente.personalData.fullname` suffix so the `irma-demo` scheme used in staging also routes through this path. (Previously only `pbdf.gemeente.personalData.fullname` matched.) - Log a warning when `state.sender` is `Some(...)` but doesn't parse as a `Mailbox` — the Reply-To deliverability fix shouldn't silently drop. - Move the `sender_display` docstring from above `build_body` to above `sender_display`. - Extend the `pg-core not found in Cargo.lock` panic in build.rs to mention that PG_CORE_VERSION feeds the X-PostGuard header consumed by the Outlook add-in's OnMessageRead filter. - New tests: demo-scheme name detection, empty-name fallback, and the `Someone` fallback (sender=None, no name).
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
There was a problem hiding this comment.
Code review
Postguard#197 deliverability fixes (X-PostGuard from build.rs, Reply-To, Auto-Submitted, multipart/alternative + inline CID logo) and disclosed-name display. 106 tests pass and clippy is clean; correct overall. Minor non-blocking notes inline.
Rule compliance
Two PR-body nits, not blocking:
- [Rule: pr-close-issue-keywords] Body uses
Addresses [encryption4all/postguard#197]— won't auto-close on merge. If this PR alone closes the issue, switch toCloses encryption4all/postguard#197; otherwise leaving it as a soft reference (perhaps asRefs) is deliberate and fine. - [Rule: writing-rules] PR body has em-dash overuse and bold lead-ins on every bullet — both are flagged AI-slop tells. Consider commas/parens for the dashes and drop the mechanical bolding.
| /// such an attribute appears in `FileState.sender_attributes` we render | ||
| /// the disclosed name in place of the bare email everywhere the sender | ||
| /// is shown in the body. | ||
| const FULLNAME_ATYPE_SUFFIX: &str = ".gemeente.personalData.fullname"; |
There was a problem hiding this comment.
[Code review] FULLNAME_ATYPE_SUFFIX matches any scheme ending in .gemeente.personalData.fullname, including a hypothetical malicious third-party scheme. Safer to allow-list explicit prefixes (pbdf., irma-demo.) so the trust boundary lives in this module.
| /// Embedded PostGuard logo, served inline via a `Content-ID: <pg-logo>` | ||
| /// MIME part rather than fetched from postguard.eu. Removes the | ||
| /// HTML-only-plus-remote-image spam signal flagged in postguard#197. | ||
| const LOGO_PNG: &[u8] = include_bytes!("../templates/email/pg_logo.png"); |
There was a problem hiding this comment.
[Code review] LOGO_PNG.to_vec() clones the 41 KB embedded PNG on every send. Keep it as &'static [u8] and only convert at the Attachment::body(...) call site — same allocation, but the rest of the module avoids paying for it.
| /// logo as an inline image referenced via `cid:pg-logo`. This shape avoids | ||
| /// the HTML-only + remote-image spam signal flagged in postguard#197 while | ||
| /// keeping graceful degradation for text-only clients. | ||
| fn build_body(html: String, text: String) -> Result<MultiPart, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
[Code review] No test asserts the MIME shape produced by build_body (outer multipart/alternative, HTML branch multipart/related with Content-ID: <pg-logo> matching cid:pg-logo in email.html). A small integration test that renders the message and greps the bytes would lock this in against a CID drift.
| /// body, and is removed from the attribute pill list so it doesn't render | ||
| /// twice. An empty disclosed value is treated as not disclosed, so we | ||
| /// fall through to the email instead of rendering a blank. | ||
| fn sender_display(state: &FileState) -> (String, Vec<(String, String)>) { |
There was a problem hiding this comment.
[Code review] If sender_attributes ever contains more than one fullname-suffixed entry (e.g. both pbdf and demo simultaneously disclosed), only the first is removed and the others render as pills. Use retain or a loop to strip all matches before picking the display name.
| .subject(subject) | ||
| .body(email)?; | ||
| .subject(subject); | ||
| if let Some(sender) = state.sender.as_deref() { |
There was a problem hiding this comment.
[Code review] The Reply-To parse-failure warn branch has no test — the explicit match was added so failures are observable. A unit test that feeds a malformed state.sender and asserts the message still builds (without Reply-To) would pin the behaviour.
| {{file_size}} - {{expires_str}} {{expiry_date}} | ||
| {% if html_content != "" %} | ||
|
|
||
| {{html_content}} |
There was a problem hiding this comment.
[Code review] mail_content is rendered raw into the plain-text part via escape = "none". Not a security issue in text/plain, but pasted HTML markup shows up verbatim with angle brackets. Consider stripping tags or documenting the field as plain text.
|
|
||
| let lock = std::fs::read_to_string("Cargo.lock").expect("Cargo.lock not readable"); | ||
| let version = lock | ||
| .split("[[package]]") |
There was a problem hiding this comment.
[Code review] Splitting Cargo.lock on the literal [[package]] is fragile if a future lockfile format ever includes that substring inside a quoted string (no such case today). A tiny TOML parse (or cargo_metadata) would be safer; not blocking.
…cence
Extend `sender_display` so the recipient mail can show a real sender
name when the signer's Yivi disclosure came from an ID credential
instead of the Dutch municipality credential.
`FULLNAME_ATYPE_SUFFIX` still wins when present. If absent, the new
`take_firstname_lastname_pair` helper scans the disclosed attributes
for a `<cred>.firstName` + `<cred>.lastName` pair from one of
`pbdf.pbdf.{passport,idcard,drivinglicence}` (and matching
`irma-demo.*` variants), and concatenates them. Both attributes are
removed from the pill list once consumed, so the recipient doesn't see
them rendered twice. Empty values fall through to the bare email, as
they already did for the gemeente fullname path.
Companion to the postguard-website condiscon change: senders can now
satisfy the mandatory name disclosure from any of four credentials,
not just gemeente. Without this rendering update, non-Dutch senders
would still appear as their bare email in the recipient mail.
Tests: 7 new `sender_display_*` cases (per-credential, demo-scheme,
preference-order, firstname-only fallback, empty-value fallback). All
24 email tests pass, full `cargo test`: 113/113.
When the signer discloses no name, the recipient email now reads 'PostGuard sent you files' instead of showing the raw sender email. Also drop 'via PostGuard' from the subject — it would be redundant when the sender is already 'PostGuard', and is cleaner for named senders too. Renames the two tests that previously asserted the email fallback.
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
There was a problem hiding this comment.
Code review
Round 3 follow-ups (firstName+lastName helper for passport/idcard/drivinglicence; 'PostGuard' fallback when no name disclosed; drop 'via PostGuard' from subject) are well-tested and clippy-clean. All prior critical/major findings resolved.
Rule compliance
No issues found. cargo fmt --check and cargo clippy -D warnings clean; PR title conventional; body claims all delivered; no AI-slop tells in prose or comments; email HTML alt text and embedded-CID change does not regress WCAG AA; build.rs approach matches the cryptify x-postguard-header convention.
| /// If `attrs` contains `<cred>.firstName` and `<cred>.lastName` for one of | ||
| /// the supported credentials and both are non-empty, remove them and | ||
| /// return `"<firstName> <lastName>"`. Otherwise leave `attrs` untouched. | ||
| fn take_firstname_lastname_pair(attrs: &mut Vec<(String, String)>) -> Option<String> { |
There was a problem hiding this comment.
[Code review] Orphan attribute UX nit (non-blocking): when only firstName (or only lastName) is disclosed, or one value is empty, the orphan stays in the pill list — recipient sees a 'PostGuard sent you files' header with a lone 'Jan' pill below. The test at line 671 documents this as intentional, so flagging only in case the pill looks odd in production.
1 participant
Summary
Addresses encryption4all/postguard#197: PostGuard notification mail lands in Gmail spam. The mailer hit three signals at once — a stale
X-PostGuard: 0.1.0header, noReply-To/Auto-Submitted, and an HTML-only body with a remote<img>. This PR removes all three. While in the email path I also surfaced the signer's disclosed name in the body.Deliverability
X-PostGuardtrackspg-core. Addedbuild.rsthat parsesCargo.lock, exportsPG_CORE_VERSIONas a compile-time env var, andX_POSTGUARD_VERSIONnow readsenv!("PG_CORE_VERSION"). Resolves to0.6.1today; auto-bumps the next timepg-coredoes. No new build deps — stdlib string scanning. Rationale and supersession of the previous "semantic token" recommendation captured inencryption4all/dobbymemory note.Reply-Toon recipient mail points to the disclosed sender (state.sender) so human replies reach a human, notnoreply@. When the sender string fails to parse as aMailboxwe now emit alog::warn!rather than silently dropping the header (Dobby review). Not set on the confirmation mail (recipient is the sender themself).Auto-Submitted: auto-generated(RFC 3834) on both notification and confirmation mail — suppresses vacation-responder loops and gives receiving MTAs a clean "transactional" signal.multipart/alternativewith plain-text branch. New hand-authoredtemplates/email/email.txtmirrors the HTML template's fields.email_templates()/email_confirm()now return(html, text, subject).pg_logo.pngisinclude_bytes!'d at build, served as aContent-ID: <pg-logo>part inside amultipart/relatedHTML branch. The HTML now referencescid:pg-logoinstead ofhttps://postguard.eu/pg_logo.png. Removes the HTML-only-plus-remote-image spam fingerprint and lets the logo render even with images-blocked-by-default.Show disclosed sender name in body
When the signer disclosed a fullname attribute matching the
.gemeente.personalData.fullnamesuffix (covers bothpbdfprod scheme andirma-demostaging scheme), the name takes the place of the bare email everywhere it used to appear in the body ({{header}}and{{sender_email}}inemail.html/email.txt). The name is also removed from the attribute pill list so it doesn't render twice. Empty disclosed values are treated as not disclosed and fall through to the email. New helpersender_display(state) -> (String, Vec<(String, String)>); covered by five unit tests.Deliberately not done
List-Unsubscribe— wrong semantic fit. PostGuard mail is person-to-person, not a list; recipients can't meaningfully opt out of a friend sending a file. Adding it would misrepresent the sender.Test plan
cargo test— 106 passing. New tests covering:auto_submitted_header_emits_auto_generated,sender_display_promotes_disclosed_name,sender_display_promotes_disclosed_name_from_demo_scheme,sender_display_treats_empty_disclosed_name_as_not_disclosed,sender_display_falls_back_to_email_when_no_name_disclosed,sender_display_uses_someone_when_no_sender_and_no_name. Updated:x_postguard_header_round_trips,x_postguard_header_serialises_into_message(no longer pin literal"0.1.0").cargo clean && cargo build—build.rsruns,PG_CORE_VERSIONresolves to0.6.1perCargo.lock.build.rsis COPY'd into the planner / builder stages.X-PostGuard: 0.6.1,Reply-To: <sender>,Auto-Submitted: auto-generated,Content-Type: multipart/alternativewithmultipart/relatedHTML branch containing the inlineimage/png; Content-ID: <pg-logo>.Companion change
The matching frontend change — making the signer's
fullnameattribute mandatory in the Yivi disclosure session, so this mail path is the default rather than the exception — is in postguard-website (PR #239).