feat: fix the six self-host defects reported in issue #439 (#456)

* feat: fix the six defects reported in issue #439 by mapping the IMAP UNAVAILABLE, INUSE and NONEXISTENT response codes to retry-level errors instead of a critical reconnect prompt, synthesising a stable no-msgid key so one message with no Message-ID header can no longer 400 the internal map endpoint and wedge every later sync pass with its cursors held, adding mailhtml.FromText and HasContent so an API or agent-created step with a plain body stops shipping the composer's empty div placeholder as its text/html part (derived on create and plain-only update, exposed as body_html on update_campaign_step, dropped at send and preview time, and refused at campaign start with empty_step_body), honouring sender_strategy='explicit' in ResolveCampaignSenderPool and ValidateCampaignReady so an emptied explicit pool parks the campaign instead of widening it to every mailbox in the workspace, making the paused_no_accounts auto-pause loud with an error log line, an error-level activity-feed entry and an org-scoped CAMPAIGN_PAUSED realtime pulse, gating the admin sign-in's Turnstile widget on GET /v1/auth/config so a self-host with CAPTCHA_PROVIDER=none is not locked out, and parsing NATS_URL down to its host:port so a credentialed bus URL no longer reports NATS down

* feat: act on the self-review of the issue #439 fixes by dropping the campaign wizard's own escapeHtml body_html builder, which entity-escaped the quotes in a conditional and made the template fail to parse at send time, and letting the backend's FromText render that part instead so wizard-written steps also get their bare URLs linked for click tracking, correcting the docs and openapi description that claimed an explicit sender pool never falls back when it still unions its tags as migration 000013 designed, extracting the duplicated blank-HTML-part guard into dropBlankHTMLPart shared by the send path and the preview, and recording why the no-msgid key keeps the folder name despite a RENAME changing it

* feat: address the CodeRabbit review on the issue #439 fixes by holding the admin sign-in's Turnstile widget unmounted until /v1/auth/config resolves so an instance with no route to Cloudflare cannot raise a widget error on a screen nobody submitted, failing StartCampaign closed when the sequence read errors rather than skipping both the malformed-template and empty-body refusals, giving TCPCheck the default port its protocol assumes so a portless NATS_URL is no longer reported down, leaving a URL that carries a merge field unanchored because the send path renders bodies with text/template and a quoted contact value would break out of the href, and correcting the sequences guide and the Campaign and CampaignUpdate openapi descriptions that named the wrong tag field
This commit is contained in:
Matthew Meszaros
2026-09-12 03:13:38 -07:00
committed by GitHub
parent dc9ce403de
commit 47defafa09
30 changed files with 973 additions and 58 deletions
+31 -2
View File
@@ -21,7 +21,7 @@ import { Label } from "@/components/ui/label";
import { InputOTP, InputOTPGroup, InputOTPSlot } from "@/components/ui/input-otp";
import { Logo } from "@/components/Logo";
import { TurnstileModal } from "@/components/captcha/TurnstileModal";
import { login, loginConfirm, verifyTwoFA } from "@/lib/api/client/auth";
import { getAuthConfig, login, loginConfirm, verifyTwoFA } from "@/lib/api/client/auth";
import type { LoginResponse } from "@/lib/api/models/auth";
import { setToken } from "@/lib/auth/storage";
import { APIError } from "@/lib/api/client";
@@ -51,6 +51,12 @@ export default function LoginPage() {
const [session, setSession] = useState("");
const [code, setCode] = useState("");
const [captcha, setCaptcha] = useState(false);
// Whether this deployment verifies a captcha token at all. null until the
// answer arrives: nothing is mounted before then, so an instance with no
// route to Cloudflare does not raise a widget error on a screen nobody has
// submitted. A failed fetch resolves to true rather than leaving it
// pending, so the check is never skipped on an instance that enforces it.
const [captchaRequired, setCaptchaRequired] = useState<boolean | null>(null);
const [submitting, setSubmitting] = useState(false);
const [error, setError] = useState<string | null>(null);
const [resendIn, setResendIn] = useState(0);
@@ -59,6 +65,24 @@ export default function LoginPage() {
const busy = submitting || captcha;
// What this deployment can do, read before anything mounts a widget. A
// self-host with CAPTCHA_PROVIDER=none cannot reach Cloudflare, so an
// invisible Turnstile there can only time out and lock the operator out.
useEffect(() => {
let cancelled = false;
getAuthConfig()
.then((cfg) => {
if (!cancelled) setCaptchaRequired(cfg.captcha);
})
.catch(() => {
// Fail safe: assume the check is enforced.
if (!cancelled) setCaptchaRequired(true);
});
return () => {
cancelled = true;
};
}, []);
// Resend cooldown tick.
useEffect(() => {
if (resendIn <= 0) return;
@@ -311,7 +335,12 @@ export default function LoginPage() {
)}
</Button>
<TurnstileModal visible={captcha} onToken={onToken} onError={onCaptchaError} />
<TurnstileModal
visible={captcha}
required={captchaRequired}
onToken={onToken}
onError={onCaptchaError}
/>
</form>
</motion.div>
) : (
@@ -5,6 +5,20 @@
// the backend's TURNSTILE_BYPASS_TOKEN accepts; in prod it renders the
// invisible Turnstile widget and delivers a real token. Either way the parent
// gets a token via onToken and sends it as `turnstile` on login.
//
// `required` is the deployment's own answer, read from /v1/auth/config. With
// CAPTCHA_PROVIDER=none the backend verifies no token, and mounting the widget
// anyway meant an air-gapped or self-hosted instance could only sit on
// challenges.cloudflare.com until it timed out: the operator was locked out of
// their own admin panel.
//
// null means the answer has not arrived. Nothing is mounted and no token is
// delivered while it is pending, because mounting optimistically loads the
// Cloudflare script on an instance that may have no route to it, and its load
// failure raises an error on a screen the operator has not even submitted yet.
// A submit made in that window is held by the effect and resolves as soon as
// the config lands. A fetch that fails resolves to true, so the check is never
// skipped on an instance that does enforce it.
import { useCallback, useEffect, useRef, type ComponentProps } from "react";
import Turnstile, { type BoundTurnstileObject } from "react-turnstile";
@@ -12,15 +26,22 @@ import { TURNSTILE_KEY } from "@/lib/env";
interface Props {
visible: boolean;
required: boolean | null;
onToken: (token: string) => void;
onError?: (message?: string) => void;
}
export function TurnstileModal({ visible, onToken, onError }: Props) {
export function TurnstileModal({ visible, required, onToken, onError }: Props) {
const defaultDevBypassToken = "warmbly-local-turnstile-bypass";
const bypassToken = import.meta.env.DEV
const devBypassToken = import.meta.env.DEV
? import.meta.env.VITE_TURNSTILE_BYPASS_TOKEN?.trim() || defaultDevBypassToken
: "";
// The dev bypass is its own answer and needs no deployment config.
const pending = required === null && devBypassToken === "";
// No widget, and the token the parent gets is whatever the backend will
// accept: the dev bypass string, or "" when nothing is verified at all.
const skipWidget = required === false || devBypassToken !== "";
const bypassToken = devBypassToken;
const tokenRef = useRef("");
const waitingRef = useRef(false);
@@ -77,7 +98,10 @@ export function TurnstileModal({ visible, onToken, onError }: Props) {
}, [fail]);
useEffect(() => {
if (visible && bypassToken) {
// Hold a submit made before the deployment answered; this effect runs
// again the moment it does, with visible still true.
if (pending) return;
if (visible && skipWidget) {
onTokenRef.current(bypassToken);
return;
}
@@ -96,9 +120,9 @@ export function TurnstileModal({ visible, onToken, onError }: Props) {
}
waitingRef.current = false;
}
}, [visible, bypassToken, deliver, execute]);
}, [visible, pending, skipWidget, bypassToken, deliver, execute]);
if (bypassToken) return null;
if (pending || skipWidget) return null;
const turnstileProps = {
ref: turnstileRef,
+12
View File
@@ -4,6 +4,7 @@
import { Request } from "@/lib/api/client";
import type {
DeploymentAuthConfig,
LoginRequest,
LoginStartResponse,
LoginConfirmRequest,
@@ -13,6 +14,17 @@ import type {
AdminProfile,
} from "@/lib/api/models/auth";
// What this deployment's auth can do. Public and unauthenticated: it is the
// first request the sign-in screen makes, and the answer decides whether a
// Turnstile widget is mounted at all.
export function getAuthConfig(): Promise<DeploymentAuthConfig> {
return Request<DeploymentAuthConfig>({
method: "GET",
url: "/v1/auth/config",
timeout: 10_000,
});
}
// Step 1: verify password + captcha. Emails a one-time code, returns a session.
export function login(input: LoginRequest): Promise<LoginStartResponse> {
return Request<LoginStartResponse>({
+11
View File
@@ -2,6 +2,17 @@
import type { AdminToken } from "@/lib/auth/storage";
// The slice of GET /v1/auth/config the admin sign-in needs.
//
// Only `captcha` today, and it exists because the login screen used to guess.
// A self-host with CAPTCHA_PROVIDER=none verifies no token, but the screen
// mounted the invisible Turnstile widget anyway and waited on a challenge it
// could not reach, so the operator was locked out of their own instance with
// "Verification timed out".
export interface DeploymentAuthConfig {
captcha: boolean;
}
export interface LoginRequest {
email: string;
password: string;
+2 -2
View File
@@ -1810,9 +1810,9 @@ func main() {
systemChecker.Add("redis", func(ctx context.Context) error { return cache.Ping(ctx).Err() })
switch bus.Name() {
case "kafka":
systemChecker.Add("kafka", sysstatus.TCPCheck(kafkaBootstrapServers))
systemChecker.Add("kafka", sysstatus.TCPCheck(kafkaBootstrapServers, "9092"))
case "nats":
systemChecker.Add("nats", sysstatus.TCPCheck(strings.TrimPrefix(getenvDefault("NATS_URL", "nats://localhost:4222"), "nats://")))
systemChecker.Add("nats", sysstatus.TCPCheck(getenvDefault("NATS_URL", "nats://localhost:4222"), "4222"))
}
if sr := os.Getenv("SCHEMA_REGISTRY_URL"); sr != "" {
systemChecker.Add("schema-registry", sysstatus.HTTPCheck(strings.TrimRight(sr, "/")+"/subjects"))
+1
View File
@@ -84,6 +84,7 @@ Returned when the request cannot be processed due to invalid syntax.
| `no_contacts` | `POST /contacts/verification` selected no contacts: neither `contacts` nor a `campaign_id` with refused leads |
| `list_bounce_risk` | `POST /campaigns/:id/start` refused the launch on the list's projected bounce rate. Clean or verify the list, or repeat the request with `acknowledge_list_risk: true` |
| `leads_undeliverable` | `POST /campaigns/:id/start` found nothing to send because address verification refused every remaining lead; the campaign is parked at `paused_undeliverable` until they are re-verified or marked deliverable |
| `empty_step_body` | `POST /campaigns/:id/start` found an email step with nothing in either body, so it would send a blank message to every lead it reached. Write the step's body and start again |
| `no_leads` | `POST /campaigns/:id/start` on a campaign that has never had a lead, with `continuous` off. Add contacts, or set `continuous` so it starts empty and waits for them. A campaign whose leads have all finished is a different case: it starts and waits |
| `no_remaining_leads` | A platform-initiated restart of a campaign with nothing left to send and `continuous` off found nothing to do; the campaign is `completed` again. A start you request never answers this: it turns `continuous` on and waits |
| `no_organization` | The request needs a workspace and the caller has none selected. Every entitlement, limit and suppression rule is scoped to a workspace, so a write that would run unscoped is refused rather than run without those checks. API keys always carry their workspace; a dashboard session picks one at sign-in, so this normally means the session predates the workspace being chosen. Select a workspace and retry |
@@ -177,7 +177,7 @@ Create a campaign. Only `name` is required, every other field is optional and ap
| `schedule_windows` | array | no | Per-day sending windows, 7 arrays indexed by weekday (Sunday = 0) of `{start, end}` minute-of-day intervals. When non-empty it supersedes `days`, `start_time` and `end_time`. |
| `email_tag_ids` | string[] | no | Mailbox tag ids that resolve the sender pool (tags strategy). |
| `folder_ids` | string[] | no | Folder ids to file the campaign under. |
| `sender_strategy` | string | no | `tags` (default) or `explicit`. |
| `sender_strategy` | string | no | `tags` (default) or `explicit`. `tags` resolves the pool from `email_tag_ids`, and falls back to every active mailbox in the workspace when no tag and no explicit sender is set. `explicit` sends from the mailboxes in `senders`, plus any `email_tag_ids` set alongside them. It never falls back to every active mailbox, so a pool that empties out parks the campaign at `paused_no_accounts` instead of widening to the whole workspace. |
| `rotation_mode` | string | no | How volume spreads across the chosen mailboxes. |
| `senders` | object[] | no | Explicit-strategy mailbox pool (see sender input below). |
| `ramp_enabled` | boolean | no | Enable per-campaign daily ramp-up. |
@@ -732,6 +732,8 @@ Start (activate) the campaign so it begins sending real mail. Works from `draft`
A start refused because every remaining lead was refused by address verification answers `leads_undeliverable` and parks the campaign at `paused_undeliverable`; re-verify the leads or mark them deliverable with [`POST /contacts/verification`](/api/reference/contacts/#verify-or-override-contacts), which resumes it.
A campaign whose email step has nothing in either body is refused with `empty_step_body` rather than started, because it would send a blank message to every lead it reached.
### Response
```json
@@ -832,7 +834,7 @@ Atomically replace the campaign's explicit sender pool with the supplied list. *
| Field | Type | Required | Description |
| --- | --- | --- | --- |
| `senders` | object[] | yes | The full new sender pool. Each item: `email_account_id` (uuid, required), `weight` (integer, optional), `enabled` (boolean, optional). |
| `senders` | object[] | yes | The full new sender pool. Each item: `email_account_id` (uuid, required), `weight` (integer, optional), `enabled` (boolean, optional). An empty array clears the pool. What that means depends on the campaign's `sender_strategy`: a `tags` campaign falls back to its tags or to every active mailbox, while an `explicit` one falls back to its tags only, and with none it is left with no mailbox to send from and parks itself at `paused_no_accounts`. |
```json
{
@@ -1020,7 +1022,7 @@ All fields optional.
| `name` | string | no | Step label. |
| `subject` | string | no | Subject template. |
| `body_plain` | string | no | Plain-text body template. Leave it empty and the send path renders one from `body_html`, keeping list bullets, table rows and link destinations, and leaving out the stylesheet. |
| `body_html` | string | no | HTML body template. Sent as written, including a whole document with its own `<head>`. Any `<style>` block is inlined onto the elements it matches at send time; see [the sequences guide](/guides/sequences/). |
| `body_html` | string | no | HTML body template. Sent as written, including a whole document with its own `<head>`. Any `<style>` block is inlined onto the elements it matches at send time; see [the sequences guide](/guides/sequences/). Send `body_plain` on its own and the HTML part is rendered from it, because a step with a plain body and an empty HTML one would otherwise arrive blank: every modern client prefers the HTML alternative. A step that already has an HTML body is never overwritten. |
| `body_sync` | boolean | no | Keep plain and HTML bodies in sync. |
| `body_code` | boolean | no | The body is authored as raw HTML. The dashboard editor opens it as markup instead of parsing it into the visual editor, which keeps only what its schema can represent. It does not change what is sent. |
| `wait_after` | integer | no | Days to wait before this step, counted from the contact's previous step (`0` to `60`). Spacing belongs to the target step, so there is no standalone wait node for email steps. |
+4
View File
@@ -37,6 +37,8 @@ Pick mailboxes in **Sending accounts** three ways, and the first two combine:
- **Individually**: specific mailboxes by hand.
- **All active mailboxes**: the default when you pick neither.
A campaign set through the API to `sender_strategy: "explicit"` is the exception to the third line: it never falls back to every active mailbox. If its list empties out, because the mailboxes were disconnected or the list was replaced with nothing, it uses any tags it also has, and with none it pauses itself rather than widening to every mailbox in the workspace. See [replace senders](/api/reference/campaigns/#replace-senders).
Sending accounts always resolve inside the campaign's own workspace. If you belong to more than one workspace, a tag you reuse across them still only picks up mailboxes belonging to the workspace that owns the campaign, so one workspace's sending reputation, daily caps, and warmup are never spent on another's traffic.
<Callout type="info" title="Follow-ups stay on the same mailbox">
@@ -186,6 +188,8 @@ The play and pause buttons work from the list row or the detail view. Starting m
A campaign can pause itself: **paused, no accounts** when it loses every sender or no sender can send under its settings (a sending behaviour profile with no working days), **paused, trial expired** when a trial ends, **auto-paused** when a guardrail trips, **needs verification** when address verification has refused every remaining lead (the campaign offers to re-verify them or send anyway; see [address verification](/guides/deliverability/#address-verification)), and plain **paused** if it ever loses its workspace, because unsubscribes, bounces and complaints are checked per workspace and cannot be honoured without one. It moves to **finished** once every contact completes the sequence or its end date passes, unless **Keep running for new leads** is on: then it stays active and shows **waiting for leads** until the next lead arrives, and only its end date finishes it. Configured to do so, it also stops following up with a contact the moment they reply.
An auto-pause is never silent. It writes a red entry to the campaign's activity log saying which of the several possible causes it was, and every teammate's campaign list moves to the paused status live, so a campaign that stops sending overnight is visible the moment anyone looks at it rather than hours later.
A finished campaign starts again on its own when a lead is added to it, whether by a linked segment, the API, a form, an integration or an automation, through the same launch checks as pressing play. If a check refuses the restart (the list is a bounce risk, the plan cannot send, too many campaigns are active), the activity log says so and the new leads wait until you fix the cause and press play. Pressing play also works after extending or clearing the end date. If there is genuinely nothing left to send, pressing play turns on **Keep running for new leads** and the campaign stays active, **waiting for leads**, so it never dead-ends on a campaign that only ran out of people to write to; the activity log notes the switch.
## Duplicate and delete
+4
View File
@@ -107,6 +107,10 @@ A step written in the visual editor gets its plain-text half as you type, and it
This only applies when there is no plain-text body to send. Set `body_plain` through the API and that text is sent as written; leave it empty and the send path renders one from the HTML.
The reverse holds too. A step written through the API or an agent tool usually carries `body_plain` and nothing else, and the HTML half is rendered from that text: the lines are kept and bare URLs become links, so click tracking has something to wrap. The step is stored with the composer's empty HTML document until then, and that empty document, not the absence of one, is what used to arrive as a blank email: clients prefer the HTML alternative and would show an empty one in place of the text. A step that already has an HTML body is never overwritten.
A step with nothing in either body is refused at start rather than sent: a blank email to every lead is not a campaign anyone meant to launch.
### Preview and test
The Preview tab renders through the real send engine, so merge fields, conditionals, and spintax resolve exactly as they will at send time. It draws the body the way a mail client would, in its own frame, rather than the way the editor does. **Preview as** picks who it renders for: a built-in sample contact, one of the campaign's leads, or any contact you search for, so a custom field your list does not actually have shows up as an unresolved token instead of looking fine. The mailbox picker next to it adds that sender's signature and shows the From name recipients will see. When the campaign is known the preview also appends the opt-out footer and lists the files attached to the campaign. Malformed templates (an `{{if}}` with no `{{end}}`) are flagged before you start the campaign. See [Personalization](/guides/expressions/) for everything you can put in copy.
+3
View File
@@ -20215,6 +20215,7 @@
},
"sender_strategy": {
"type": "string",
"description": "How the campaign resolves its sending mailboxes. \"tags\" reads email_tags and falls back to every active mailbox in the workspace when no tag and no explicit sender is set. \"explicit\" sends from the campaign_senders pool and any email_tags set alongside it, and never falls back to every active mailbox: an empty pool with no tags parks the campaign at paused_no_accounts instead of widening to the whole workspace.",
"enum": [
"tags",
"explicit"
@@ -20390,6 +20391,7 @@
},
"sender_strategy": {
"type": "string",
"description": "How the campaign resolves its sending mailboxes. \"tags\" reads email_tag_ids and falls back to every active mailbox in the workspace when no tag and no explicit sender is set. \"explicit\" sends from the campaign_senders pool and any email_tag_ids set alongside it, and never falls back to every active mailbox: an empty pool with no tags parks the campaign at paused_no_accounts instead of widening to the whole workspace.",
"enum": [
"tags",
"explicit"
@@ -20556,6 +20558,7 @@
},
"sender_strategy": {
"type": "string",
"description": "How the campaign resolves its sending mailboxes. \"tags\" reads email_tags and falls back to every active mailbox in the workspace when no tag and no explicit sender is set. \"explicit\" sends from the campaign_senders pool and any email_tags set alongside it, and never falls back to every active mailbox: an empty pool with no tags parks the campaign at paused_no_accounts instead of widening to the whole workspace.",
"enum": [
"tags",
"explicit"
+4 -1
View File
@@ -49,7 +49,8 @@ func (d Deps) registerSequenceTools(r *Registry) {
"step_id": strProp("The step (sequence) UUID."),
"name": strProp("New step name."),
"subject": strProp("New email subject."),
"body": strProp("New email body text."),
"body": strProp("New email body text. Sent as the plain-text part; when the step has no HTML of its own, the HTML part is rendered from it."),
"body_html": strProp("New email body as HTML, for a designed email. Optional: omit it and the HTML part is rendered from body."),
"wait_days": intProp("Days to wait before this step runs."),
}, "campaign_id", "step_id"),
Risk: generation.RiskWrite,
@@ -114,6 +115,7 @@ func (d Deps) updateCampaignStep(ctx context.Context, inv Invocation, args json.
Name *string `json:"name"`
Subject *string `json:"subject"`
Body *string `json:"body"`
BodyHTML *string `json:"body_html"`
WaitDays *int `json:"wait_days"`
}](args)
if err != nil {
@@ -130,6 +132,7 @@ func (d Deps) updateCampaignStep(ctx context.Context, inv Invocation, args json.
Name: in.Name,
Subject: in.Subject,
BodyPlain: in.Body,
BodyHTML: in.BodyHTML,
WaitAfter: in.WaitDays,
}
step, xerr := d.Sequences.Update(ctx, inv.UserID.String(), in.CampaignID, in.StepID, upd)
+30 -11
View File
@@ -21,6 +21,7 @@ import (
"github.com/warmbly/warmbly/internal/infrastructure/pubsub"
"github.com/warmbly/warmbly/internal/models"
"github.com/warmbly/warmbly/internal/observability/errs"
"github.com/warmbly/warmbly/internal/pkg/mailhtml"
"github.com/warmbly/warmbly/internal/pkg/trackdns"
"github.com/warmbly/warmbly/internal/repository"
"github.com/warmbly/warmbly/internal/scheduler"
@@ -464,19 +465,37 @@ func (s *campaignService) StartCampaign(ctx context.Context, orgID uuid.UUID, ca
// conditional (e.g. an {{if}} with no {{end}}) silently degrades to literal
// template text in the sent email — better to catch it here with a clear,
// step-scoped error than to ship {{if ...}} to recipients.
if seqs, serr := s.campaignRepository.GetSequencesByCampaignID(ctx, cID); serr == nil {
for i, seq := range seqs {
for _, f := range []struct {
name, val string
}{{"subject", seq.Subject}, {"body", seq.BodyHTML}, {"plain-text body", seq.BodyPlain}} {
if terr := tasks.TemplateError(f.val); terr != nil {
return errx.New(errx.BadRequest, fmt.Sprintf(
"Step %d's %s has a template error — fix the {{if}}/{{end}} or {{eq}} syntax before starting.",
i+1, f.name,
))
}
// Fail closed. This read backs two refusals (a malformed template, and a
// step with no body at all), so skipping it on a query error would start a
// campaign that sends {{if}} literals or blank mail to every lead.
seqs, serr := s.campaignRepository.GetSequencesByCampaignID(ctx, cID)
if serr != nil {
errs.CaptureException(serr)
return errx.InternalError()
}
for i, seq := range seqs {
for _, f := range []struct {
name, val string
}{{"subject", seq.Subject}, {"body", seq.BodyHTML}, {"plain-text body", seq.BodyPlain}} {
if terr := tasks.TemplateError(f.val); terr != nil {
return errx.New(errx.BadRequest, fmt.Sprintf(
"Step %d's %s has a template error — fix the {{if}}/{{end}} or {{eq}} syntax before starting.",
i+1, f.name,
))
}
}
// An email step with nothing in either body sends a blank message
// to every lead it reaches. A step created through the API carries
// the composer's empty placeholder, which is not an empty string,
// so this asks whether the body would RENDER anything.
if seq.Kind == "email" &&
!mailhtml.HasContent(seq.BodyHTML) &&
strings.TrimSpace(seq.BodyPlain) == "" {
return errx.NewWithIdentifier(errx.BadRequest, "empty_step_body", fmt.Sprintf(
"Step %d has no email body, so it would send a blank message. Write the body before starting.",
i+1,
))
}
}
// Refuse a launch whose list is known to be largely undeliverable. Only
+37
View File
@@ -3,12 +3,14 @@ package sequence
import (
"context"
"fmt"
"strings"
"github.com/google/uuid"
"github.com/warmbly/warmbly/internal/observability/errs"
"github.com/warmbly/warmbly/internal/errx"
"github.com/warmbly/warmbly/internal/models"
"github.com/warmbly/warmbly/internal/pkg/mailhtml"
)
func (s *sequenceService) Create(ctx context.Context, userID, campaignID string) (*models.Sequence, *errx.Error) {
@@ -20,6 +22,7 @@ func (s *sequenceService) Get(ctx context.Context, userID, campaignID string) ([
}
func (s *sequenceService) Update(ctx context.Context, userID, campaignID, sequenceID string, data *models.UpdateSequence) (*models.Sequence, *errx.Error) {
s.deriveBodyHTML(ctx, userID, campaignID, sequenceID, data)
// Branch routing is resolved (and made safe against deleted/dangling targets
// and loops) at schedule time in the repository's finder; the repository also
// validates branch shape before persisting. No cross-step write validation is
@@ -27,6 +30,40 @@ func (s *sequenceService) Update(ctx context.Context, userID, campaignID, sequen
return s.sequenceRepository.Update(ctx, userID, campaignID, sequenceID, data)
}
// deriveBodyHTML fills in the HTML part for a plain-only write.
//
// The API and the agent tools set body_plain and nothing else, while the send
// path puts body_html on the wire as the text/html alternative that every
// modern client prefers. A step left on the composer's empty placeholder
// therefore arrived blank, with the real copy only in the text fallback.
//
// Only when the stored HTML has nothing in it: an author who wrote both parts
// keeps the HTML they designed, and a caller that sends body_html explicitly
// is never second-guessed.
func (s *sequenceService) deriveBodyHTML(ctx context.Context, userID, campaignID, sequenceID string, data *models.UpdateSequence) {
if data == nil || data.BodyPlain == nil || data.BodyHTML != nil {
return
}
if strings.TrimSpace(*data.BodyPlain) == "" {
return
}
steps, xerr := s.sequenceRepository.Get(ctx, userID, campaignID)
if xerr != nil {
return
}
for _, step := range steps {
if step.ID.String() != sequenceID {
continue
}
if mailhtml.HasContent(step.BodyHTML) {
return
}
derived := mailhtml.FromText(*data.BodyPlain)
data.BodyHTML = &derived
return
}
}
// UpdateLayout persists only step canvas coordinates (drag-to-stick). Cosmetic
// and high-churn, so it stays out of the audited content-update path.
func (s *sequenceService) UpdateLayout(ctx context.Context, userID, campaignID string, positions []models.SequencePosition) *errx.Error {
+51 -3
View File
@@ -94,9 +94,17 @@ func HTTPCheck(url string) func(ctx context.Context) error {
}
}
// TCPCheck probes the first address of a comma-separated host:port list.
func TCPCheck(addrs string) func(ctx context.Context) error {
addr := strings.TrimSpace(strings.Split(addrs, ",")[0])
// TCPCheck probes the first address of a comma-separated list. Each entry may
// be a bare host:port or a full URL, because the variables these come from
// (NATS_URL, the broker list) are connection strings, not dial addresses.
//
// defaultPort is the port the client library assumes when the URL names none,
// and it has to be supplied because that is protocol knowledge this package
// does not have. Without it a perfectly good NATS_URL of "nats://host" is
// reported down: the client connects on 4222 and net.Dial refuses an address
// with no port at all.
func TCPCheck(addrs, defaultPort string) func(ctx context.Context) error {
addr := withPort(DialAddr(strings.Split(addrs, ",")[0]), defaultPort)
return func(ctx context.Context) error {
var d net.Dialer
conn, err := d.DialContext(ctx, "tcp", addr)
@@ -106,3 +114,43 @@ func TCPCheck(addrs string) func(ctx context.Context) error {
return conn.Close()
}
}
// DialAddr reduces a connection string to the host:port net.Dial accepts.
// Credentials in the authority are the reason this exists: net.Dial reads
// "user:pass@host:4222" as an address with too many colons, so a credentialed
// NATS_URL reported the bus down while everything worked.
func DialAddr(raw string) string {
addr := strings.TrimSpace(raw)
if i := strings.Index(addr, "://"); i >= 0 {
addr = addr[i+3:]
}
// Last "@": a password may legitimately contain one.
if i := strings.LastIndex(addr, "@"); i >= 0 {
addr = addr[i+1:]
}
// Anything after the authority (path, query, fragment) is not dialled.
if i := strings.IndexAny(addr, "/?#"); i >= 0 {
addr = addr[:i]
}
return addr
}
// withPort appends the caller's default port to an address that names none.
// IPv6 literals are left alone unless they are bracketed, since "::1" is all
// colons and guessing where the port would go is how this gets worse.
func withPort(addr, defaultPort string) string {
if addr == "" || defaultPort == "" {
return addr
}
if strings.HasPrefix(addr, "[") {
// [::1]:4222 has a port, [::1] does not.
if strings.Contains(addr[strings.Index(addr, "]"):], ":") {
return addr
}
return addr + ":" + defaultPort
}
if strings.Contains(addr, ":") {
return addr
}
return addr + ":" + defaultPort
}
+55
View File
@@ -0,0 +1,55 @@
package sysstatus
import "testing"
func TestDialAddr(t *testing.T) {
cases := []struct {
name string
in string
want string
}{
{"bare", "localhost:4222", "localhost:4222"},
{"scheme", "nats://localhost:4222", "localhost:4222"},
{"credentials", "nats://user:pass@nats.internal:4222", "nats.internal:4222"},
{"password with at", "nats://user:p@ss@nats.internal:4222", "nats.internal:4222"},
{"tls scheme", "tls://nats.internal:4222", "nats.internal:4222"},
{"path", "nats://nats.internal:4222/cluster", "nats.internal:4222"},
{"padded", " broker:9092 ", "broker:9092"},
}
for _, c := range cases {
if got := DialAddr(c.in); got != c.want {
t.Errorf("%s: DialAddr(%q) = %q, want %q", c.name, c.in, got, c.want)
}
}
}
// nats.go defaults a portless URL to 4222, so a probe that refuses to dial one
// reports the bus down while the client is happily connected.
func TestWithPort(t *testing.T) {
cases := []struct {
name string
addr, port string
want string
}{
{"portless gets the default", "nats.internal", "4222", "nats.internal:4222"},
{"explicit port wins", "nats.internal:5222", "4222", "nats.internal:5222"},
{"no default supplied", "nats.internal", "", "nats.internal"},
{"empty address", "", "4222", ""},
{"bracketed ipv6 with a port", "[::1]:4222", "4222", "[::1]:4222"},
{"bracketed ipv6 without one", "[::1]", "4222", "[::1]:4222"},
{"bare ipv6 is left alone", "::1", "4222", "::1"},
}
for _, c := range cases {
if got := withPort(c.addr, c.port); got != c.want {
t.Errorf("%s: withPort(%q, %q) = %q, want %q", c.name, c.addr, c.port, got, c.want)
}
}
}
// The whole path, as TCPCheck runs it.
func TestDialAddrWithPortEndToEnd(t *testing.T) {
got := withPort(DialAddr("nats://user:pass@nats.internal"), "4222")
if got != "nats.internal:4222" {
t.Errorf("got %q, want nats.internal:4222", got)
}
}
+41 -2
View File
@@ -2,8 +2,10 @@ package wmail
import (
"context"
"fmt"
"slices"
"sort"
"strings"
"time"
goimap "github.com/emersion/go-imap/v2"
@@ -255,6 +257,7 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill
var fresh []*imap.Fetched
for _, f := range fetched {
w.ensureMessageKey(f.Email)
internal, err := w.EmailMessageMapRepository.Get(ctx, w.UserID, w.ID, f.Email.MessageID)
if err != nil {
return false, w.controlPlaneError(err, stats)
@@ -326,6 +329,33 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill
return all, nil
}
// ensureMessageKey gives a message without a Message-ID header one that is
// stable for this mailbox, because the empty string is not a key: the map
// endpoint refuses it with 400 and the failed lookup ends the whole sync pass
// with its cursors held, so ONE legacy or malformed sender parked every later
// message on the account for good.
//
// Folder name, UIDVALIDITY and UID: RFC 9051 makes that triple the identity of
// a message on a server, which is what is left when the sender gave it none of
// its own. It re-derives to the same string on the next pass, so the message is
// recognised as known rather than stored again, and it cannot collide with a
// real Message-ID.
//
// The folder name is in it deliberately, even though a RENAME keeps UIDVALIDITY
// and would therefore change the key. Dropping it would key on a pair two
// folders can in principle share, and the failure there is a message silently
// treated as already stored. A rename re-importing the handful of messages that
// carried no Message-ID is the cheaper of the two.
//
// Threading is unaffected: a message with no Message-ID roots its own thread
// on this key, and nothing can ever reply to an id that was never on the wire.
func (w *WMail) ensureMessageKey(msg *models.EmailMessageData) {
if msg == nil || strings.TrimSpace(msg.MessageID) != "" {
return
}
msg.MessageID = fmt.Sprintf("no-msgid/%s/%d/%d", w.SmtpImapData.folderPath, w.SmtpImapData.mailbox, msg.UID)
}
// threadParentID is the message this one answers, and the key its thread is
// built on. Only In-Reply-To carries that.
//
@@ -339,11 +369,20 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill
//
// A message that answers nothing has no parent, and the caller roots its
// thread on its own Message-ID.
//
// A blank entry is skipped rather than returned: an empty parent id is not a
// key either, and the map lookup it would cause ends the pass exactly as a
// missing Message-ID used to (see ensureMessageKey).
func threadParentID(msg *models.EmailMessageData) string {
if msg == nil || len(msg.InReplyTo) == 0 {
if msg == nil {
return ""
}
return msg.InReplyTo[len(msg.InReplyTo)-1]
for i := len(msg.InReplyTo) - 1; i >= 0; i-- {
if id := strings.TrimSpace(msg.InReplyTo[i]); id != "" {
return id
}
}
return ""
}
// imapStore threads a new message and hands it to storeNew.
@@ -0,0 +1,64 @@
package wmail
import (
"testing"
"github.com/warmbly/warmbly/internal/models"
)
// One message with no Message-ID header used to end every sync pass: the empty
// string is not a map key, the internal endpoint refuses it with 400, and
// controlPlaneError holds the cursors, so the same batch came back forever.
func TestEnsureMessageKeySynthesizesAStableKey(t *testing.T) {
w := &WMail{SmtpImapData: &SmtpImapData{folderPath: "INBOX", mailbox: 42}}
msg := &models.EmailMessageData{UID: 17}
w.ensureMessageKey(msg)
if msg.MessageID == "" {
t.Fatal("a message with no Message-ID still has no key")
}
// Stable: the next pass has to recognise the message as already stored.
again := &models.EmailMessageData{UID: 17}
w.ensureMessageKey(again)
if again.MessageID != msg.MessageID {
t.Errorf("key is not stable across passes: %q then %q", msg.MessageID, again.MessageID)
}
other := &models.EmailMessageData{UID: 18}
w.ensureMessageKey(other)
if other.MessageID == msg.MessageID {
t.Errorf("two UIDs share the key %q", other.MessageID)
}
}
func TestEnsureMessageKeyLeavesARealIDAlone(t *testing.T) {
w := &WMail{SmtpImapData: &SmtpImapData{folderPath: "INBOX"}}
msg := &models.EmailMessageData{MessageID: "<real@example.test>", UID: 3}
w.ensureMessageKey(msg)
if msg.MessageID != "<real@example.test>" {
t.Errorf("MessageID = %q, want the header's own value", msg.MessageID)
}
// A header that is present but blank is no more of a key than a missing one.
blank := &models.EmailMessageData{MessageID: " ", UID: 4}
w.ensureMessageKey(blank)
if blank.MessageID == " " {
t.Error("a whitespace-only Message-ID was kept as the map key")
}
}
// An In-Reply-To whose entries are blank is the same wedge one step along: the
// empty parent id would go to the map endpoint as a key.
func TestThreadParentIDSkipsBlankEntries(t *testing.T) {
if got := threadParentID(&models.EmailMessageData{InReplyTo: []string{"<a@x>", " "}}); got != "<a@x>" {
t.Errorf("threadParentID = %q, want the last non-blank id", got)
}
if got := threadParentID(&models.EmailMessageData{InReplyTo: []string{"", " "}}); got != "" {
t.Errorf("threadParentID = %q, want no parent", got)
}
if got := threadParentID(nil); got != "" {
t.Errorf("threadParentID(nil) = %q, want no parent", got)
}
}
+11
View File
@@ -22,6 +22,17 @@ func (c *Client) handleError(err error) *errx.MailError {
}
case imap.ResponseCodeAuthorizationFailed:
return errx.ErrMailAuthorizationFailed
case imap.ResponseCodeUnavailable, imap.ResponseCodeInUse:
// RFC 5530's "try again later" pair: the server is up and the
// credentials are fine, it just refused this command for a few
// minutes. Falling through to the unknown-IMAP error told every
// mailbox on the provider to reconnect (resolve method RELOAD) over
// a blip the next pass clears on its own.
return errx.ErrMailServerUnreachable
case imap.ResponseCodeNonExistent:
// The folder or message asked for is gone, which the folder walk
// recovers from by re-listing. Not a reason to reconnect a mailbox.
return errx.ErrMailResourceNotFound
default:
return errx.ErrMailUnknownImapError(imapErrDetail(imapErr))
}
+33
View File
@@ -29,6 +29,39 @@ func TestHandleErrorTransportIsNotNil(t *testing.T) {
}
}
// A "try again later" response code is not a broken mailbox. UNAVAILABLE used
// to fall through to the unknown-IMAP error, which is CRITICAL with resolve
// method RELOAD, so a few minutes of provider maintenance told every mailbox
// on that provider to reconnect.
func TestHandleErrorTransientResponseCodes(t *testing.T) {
c := &Client{}
for _, tc := range []struct {
code goimap.ResponseCode
want errx.MailErrorCode
}{
{goimap.ResponseCodeUnavailable, errx.MailErrorCodeServerUnreachable},
{goimap.ResponseCodeInUse, errx.MailErrorCodeServerUnreachable},
{goimap.ResponseCodeNonExistent, errx.MailErrorCodeNotFound},
} {
t.Run(string(tc.code), func(t *testing.T) {
got := c.handleError(&goimap.Error{Type: goimap.StatusResponseTypeNo, Code: tc.code, Text: "try later"})
if got == nil {
t.Fatalf("handleError(%s) = nil, want a mail error", tc.code)
}
if got.Code != tc.want {
t.Errorf("Code = %q, want %q", got.Code, tc.want)
}
if got.Type == errx.MailErrorCritical {
t.Errorf("%s is a transient refusal; Type = CRITICAL parks a mailbox error the user has to clear", tc.code)
}
if got.ResolveMethod != errx.MailErrorResolveMethodRetry {
t.Errorf("ResolveMethod = %q, want %q", got.ResolveMethod, errx.MailErrorResolveMethodRetry)
}
})
}
}
// A NO/BAD carries a response code only when the server chooses to send one.
// A codeless one used to render as "Something went wrong: " with nothing after
// the colon (issue #405, IONOS), which tells the customer nothing and leaves a
+132
View File
@@ -0,0 +1,132 @@
package mailhtml
import (
"regexp"
"strings"
nethtml "golang.org/x/net/html"
"golang.org/x/net/html/atom"
)
// bareURL matches a link a plain-text author typed with no markup around it.
// Trailing sentence punctuation is deliberately outside the match: "see
// https://example.com." ends in a full stop, not in a URL.
var bareURL = regexp.MustCompile(`https?://[^\s<>"']+[^\s<>"'.,;:!?)\]]`)
// FromText renders a plain-text body as the HTML part that ships beside it.
//
// A step written through the API or an agent tool has a plain body and no
// HTML, and the send path puts body_html on the wire as the text/html
// alternative. Every modern client prefers that part, so a step whose HTML was
// still the composer's empty <div></div> placeholder arrived as a blank
// message with the real copy only in the fallback nobody reads.
//
// The output is the shape the dashboard composer produces (one <div> per
// line), so a body derived here opens in the editor unchanged rather than as
// something the author did not write. Bare URLs become anchors because click
// tracking rewrites hrefs: a link left as text is a link that is never
// tracked and never gets a UTM tag.
func FromText(plain string) string {
// Normalise the line endings first: a CRLF body would otherwise leave a
// stray carriage return inside every div.
text := strings.ReplaceAll(strings.ReplaceAll(plain, "\r\n", "\n"), "\r", "\n")
if strings.TrimSpace(text) == "" {
return ""
}
var b strings.Builder
for _, line := range strings.Split(text, "\n") {
if strings.TrimSpace(line) == "" {
// Gmail's own shape for a blank line. An empty <div> collapses to
// nothing in several clients, which loses the author's spacing.
b.WriteString("<div><br></div>")
continue
}
b.WriteString("<div>")
b.WriteString(linkifyEscaped(line))
b.WriteString("</div>")
}
return b.String()
}
// textEscaper escapes the three characters that are markup in element content.
//
// Quotes are deliberately NOT escaped, which is why html.EscapeString is not
// used here: a step body is a Go template, and turning `{{if eq .Company
// "Acme"}}` into `&#34;Acme&#34;` makes it fail to parse, which drops the send
// onto the naive renderer and ships the literal template text to the
// recipient. Nothing this writes can land in an attribute: the URL pattern
// excludes both quote characters, so the href below is always quote-free.
var textEscaper = strings.NewReplacer("&", "&amp;", "<", "&lt;", ">", "&gt;")
// linkifyEscaped escapes one line of plain text and wraps its bare URLs in
// anchors. Escaping happens first and the anchor markup is written around the
// escaped text, so nothing the author typed can become markup.
func linkifyEscaped(line string) string {
escaped := textEscaper.Replace(line)
return bareURL.ReplaceAllStringFunc(escaped, func(match string) string {
// A URL carrying a merge field is left as text. The send path renders
// the body with text/template, which by design performs no escaping
// (see internal/tasks/template.go), so a contact value containing a
// quote would break out of the href this would otherwise build. Plain
// bodies had no anchors at all before, so declining to add one here
// costs nothing that existed.
if strings.Contains(match, "{{") {
return match
}
// Otherwise the href is literal text the author typed, already through
// textEscaper and holding no quote, so it is attribute-safe as it is.
return `<a href="` + match + `">` + match + `</a>`
})
}
// contentTags are elements that are content in themselves: an email may
// legitimately be one image, a horizontal rule between two blocks, or a table
// of them, and none of those carry text.
var contentTags = map[atom.Atom]bool{
atom.Img: true, atom.Video: true, atom.Audio: true, atom.Hr: true,
}
// HasContent reports whether an HTML body would render anything a recipient
// can see: visible text, or an image or rule standing in for it.
//
// It exists because "" is not the only empty body. A step created through the
// API carries the composer's <div></div> placeholder, which is non-empty as a
// string, passes every len() check, and ships as a completely blank email.
func HasContent(bodyHTML string) bool {
if strings.TrimSpace(bodyHTML) == "" {
return false
}
if strings.TrimSpace(ToPlainText(bodyHTML)) != "" {
return true
}
doc, err := nethtml.Parse(strings.NewReader(bodyHTML))
if err != nil {
return false
}
root := findElement(doc, atom.Body)
if root == nil {
root = doc
}
return hasContentNode(root)
}
func hasContentNode(n *nethtml.Node) bool {
if n.Type == nethtml.ElementNode {
if textSkip[n.DataAtom] {
return false
}
if contentTags[n.DataAtom] && !isHidden(n) {
// An <img> with no source renders as nothing (or as a broken-image
// placeholder), which is not content either.
if n.DataAtom != atom.Img || strings.TrimSpace(attrOf(n, "src")) != "" {
return true
}
}
}
for c := n.FirstChild; c != nil; c = c.NextSibling {
if hasContentNode(c) {
return true
}
}
return false
}
+108
View File
@@ -0,0 +1,108 @@
package mailhtml
import (
"strings"
"testing"
)
func TestFromTextKeepsLineStructure(t *testing.T) {
got := FromText("Hi Ana,\n\nQuick question about your team.")
want := "<div>Hi Ana,</div><div><br></div><div>Quick question about your team.</div>"
if got != want {
t.Errorf("FromText =\n%q\nwant\n%q", got, want)
}
}
func TestFromTextIsEmptyForEmptyInput(t *testing.T) {
for _, in := range []string{"", " ", "\n\n", "\r\n"} {
if got := FromText(in); got != "" {
t.Errorf("FromText(%q) = %q, want an empty body so nothing ships an HTML part", in, got)
}
}
}
// The plain body is untrusted template text: it may contain angle brackets,
// ampersands, or a merge field someone pasted markup into.
func TestFromTextEscapes(t *testing.T) {
got := FromText(`Tom & Jerry <script>alert(1)</script>`)
if strings.Contains(got, "<script>") {
t.Errorf("FromText did not escape markup: %q", got)
}
if !strings.Contains(got, "&amp;") {
t.Errorf("FromText did not escape the ampersand: %q", got)
}
}
// Click tracking rewrites hrefs, so a URL left as bare text is never tracked
// and never gets a UTM tag.
func TestFromTextLinkifiesBareURLs(t *testing.T) {
got := FromText("Book here: https://cal.example.com/ana?ref=a&b=c")
if !strings.Contains(got, `<a href="https://cal.example.com/ana?ref=a&amp;b=c">`) {
t.Errorf("FromText did not anchor the URL: %q", got)
}
}
func TestFromTextLeavesSentencePunctuationOutOfTheLink(t *testing.T) {
got := FromText("See https://example.com.")
if !strings.Contains(got, `<a href="https://example.com">https://example.com</a>.`) {
t.Errorf("FromText swallowed the full stop into the href: %q", got)
}
}
// A step body is a Go template. Escaping the quotes in a conditional makes it
// fail to parse, which drops the send onto the naive renderer and ships the
// literal {{if ...}} text to the recipient.
func TestFromTextKeepsTemplateSyntaxIntact(t *testing.T) {
tmpl := `{{if eq .Company "Acme"}}Hi {{.FirstName}}{{end}}`
got := FromText(tmpl)
if !strings.Contains(got, tmpl) {
t.Errorf("FromText mangled the template:\n%q\nwant it to contain\n%q", got, tmpl)
}
}
func TestHasContent(t *testing.T) {
cases := []struct {
name string
body string
want bool
}{
{"empty string", "", false},
{"composer placeholder", "<div></div>", false},
{"nested empty blocks", "<div><p></p><p><br></p></div>", false},
{"whitespace only", "<div> </div>", false},
{"style block only", "<style>.a{color:red}</style>", false},
{"real copy", "<div>Hi Ana</div>", true},
{"image only", `<div><img src="https://example.com/a.png"></div>`, true},
{"image with no source or alt", "<div><img></div>", false},
{"rule", "<div><hr></div>", true},
}
for _, c := range cases {
if got := HasContent(c.body); got != c.want {
t.Errorf("%s: HasContent(%q) = %v, want %v", c.name, c.body, got, c.want)
}
}
}
// The round trip the derivation relies on: whatever FromText writes for a
// non-empty body must read back as content, or the send-time guard would drop
// the part it just derived.
func TestFromTextOutputHasContent(t *testing.T) {
for _, in := range []string{"Hello", "a\n\nb", "https://example.com"} {
if !HasContent(FromText(in)) {
t.Errorf("FromText(%q) produced a body HasContent calls empty", in)
}
}
}
// The send path renders the body with text/template, which escapes nothing by
// design, so an anchor built around a templated URL would let a contact value
// containing a quote break out of the href. Such a URL stays plain text.
func TestFromTextDoesNotAnchorATemplatedURL(t *testing.T) {
got := FromText("Book here: https://cal.example.com/?ref={{.Company}}")
if strings.Contains(got, "<a href") {
t.Errorf("FromText anchored a URL carrying a merge field: %q", got)
}
if !strings.Contains(got, "https://cal.example.com/?ref={{.Company}}") {
t.Errorf("FromText mangled the URL: %q", got)
}
}
+36 -2
View File
@@ -3,6 +3,7 @@ package repository
import (
"context"
"github.com/google/uuid"
"github.com/warmbly/warmbly/internal/errx"
"github.com/warmbly/warmbly/internal/models"
)
@@ -18,6 +19,15 @@ type CampaignSenderPool struct {
Explicit []CampaignSenderAccount
}
// CampaignSenderSource is the slice of EmailRepository the pool resolver needs.
// Narrow on purpose: the resolver is the one place three callers must agree on,
// and a three-method dependency is one a test can stand up.
type CampaignSenderSource interface {
GetByCampaignSenders(ctx context.Context, scope AccountScope, campaignID uuid.UUID) ([]CampaignSenderAccount, *errx.Error)
GetByTags(ctx context.Context, scope AccountScope, tags []string) ([]models.Email, *errx.Error)
GetAllActiveInScope(ctx context.Context, scope AccountScope) ([]models.Email, *errx.Error)
}
// ResolveCampaignSenderPool resolves a campaign's mailboxes. The scheduler and
// the pre-send checks both go through it, so a check can never refuse a pool
// the scheduler would happily send from (issue #340: a campaign on the "all"
@@ -26,7 +36,7 @@ type CampaignSenderPool struct {
// Tenancy is the campaign's organization, never its owner: a user in two
// organizations must not have A's campaign pick up B's mailbox. A campaign
// with no organization resolves to no mailboxes.
func ResolveCampaignSenderPool(ctx context.Context, repo EmailRepository, campaign *models.Campaign) (CampaignSenderPool, *errx.Error) {
func ResolveCampaignSenderPool(ctx context.Context, repo CampaignSenderSource, campaign *models.Campaign) (CampaignSenderPool, *errx.Error) {
pool := CampaignSenderPool{Accounts: []models.Email{}}
scope := NewAccountScope(campaign.OrganizationID)
explicit, err := repo.GetByCampaignSenders(ctx, scope, campaign.ID)
@@ -51,7 +61,7 @@ func ResolveCampaignSenderPool(ctx context.Context, repo EmailRepository, campai
}
}
}
if len(explicit) == 0 && len(campaign.EmailTags) == 0 {
if len(explicit) == 0 && len(campaign.EmailTags) == 0 && !ExplicitSenderPool(campaign) {
all, err := repo.GetAllActiveInScope(ctx, scope)
if err != nil {
return pool, err
@@ -60,3 +70,27 @@ func ResolveCampaignSenderPool(ctx context.Context, repo EmailRepository, campai
}
return pool, nil
}
// CampaignSenderStrategyExplicit is the campaigns.sender_strategy value that
// means "these mailboxes and no others".
const CampaignSenderStrategyExplicit = "explicit"
// ExplicitSenderPool reports whether a campaign named its mailboxes by hand.
//
// It is what stops the "all active mailboxes" fallback from widening a
// campaign that asked for three mailboxes into one sending from every mailbox
// in the workspace. An explicit pool can empty out on its own (the mailboxes
// are disconnected, or the pool is replaced with nothing) and before this the
// campaign silently carried on from every address the tenant owns.
//
// The tag union above still applies, which is what migration 000013 designed:
// an explicit campaign that also carries tags falls back to those. What it can
// no longer do is fall back to the whole workspace. With neither it resolves to
// no mailboxes, which parks it as paused_no_accounts with a reason in its
// activity log.
//
// Only 'explicit' is special-cased: 'tags' is the default and the value the
// dashboard writes, so nothing about the existing tag or "all" behaviour moves.
func ExplicitSenderPool(campaign *models.Campaign) bool {
return campaign != nil && campaign.SenderStrategy == CampaignSenderStrategyExplicit
}
@@ -0,0 +1,112 @@
package repository
import (
"context"
"testing"
"github.com/google/uuid"
"github.com/warmbly/warmbly/internal/errx"
"github.com/warmbly/warmbly/internal/models"
)
type stubSenderSource struct {
explicit []CampaignSenderAccount
tagged []models.Email
all []models.Email
allCalls int
}
func (s *stubSenderSource) GetByCampaignSenders(context.Context, AccountScope, uuid.UUID) ([]CampaignSenderAccount, *errx.Error) {
return s.explicit, nil
}
func (s *stubSenderSource) GetByTags(context.Context, AccountScope, []string) ([]models.Email, *errx.Error) {
return s.tagged, nil
}
func (s *stubSenderSource) GetAllActiveInScope(context.Context, AccountScope) ([]models.Email, *errx.Error) {
s.allCalls++
return s.all, nil
}
func testCampaign(strategy string) *models.Campaign {
org := uuid.New()
return &models.Campaign{ID: uuid.New(), OrganizationID: &org, SenderStrategy: strategy}
}
// The incident this guards: a campaign that named three mailboxes by hand lost
// them (revoked, or the pool replaced with nothing) and silently carried on
// sending from every mailbox in the workspace.
func TestResolveCampaignSenderPoolExplicitDoesNotWidenToEveryMailbox(t *testing.T) {
src := &stubSenderSource{all: []models.Email{{ID: uuid.New()}, {ID: uuid.New()}}}
pool, err := ResolveCampaignSenderPool(context.Background(), src, testCampaign("explicit"))
if err != nil {
t.Fatalf("ResolveCampaignSenderPool: %v", err)
}
if len(pool.Accounts) != 0 {
t.Errorf("an empty explicit pool resolved to %d mailboxes, want none", len(pool.Accounts))
}
if src.allCalls != 0 {
t.Error("the explicit strategy fell back to every active mailbox in the workspace")
}
}
// Everything the dashboard writes is sender_strategy='tags', so the fallback
// that campaigns have always relied on has to be untouched.
func TestResolveCampaignSenderPoolTagsStrategyStillFallsBackToAll(t *testing.T) {
all := []models.Email{{ID: uuid.New()}, {ID: uuid.New()}}
src := &stubSenderSource{all: all}
pool, err := ResolveCampaignSenderPool(context.Background(), src, testCampaign("tags"))
if err != nil {
t.Fatalf("ResolveCampaignSenderPool: %v", err)
}
if len(pool.Accounts) != len(all) {
t.Errorf("pool has %d mailboxes, want the %d active ones", len(pool.Accounts), len(all))
}
}
// An explicit pool that still has its mailboxes resolves to exactly those.
func TestResolveCampaignSenderPoolExplicitUsesItsOwnMailboxes(t *testing.T) {
picked := models.Email{ID: uuid.New()}
src := &stubSenderSource{
explicit: []CampaignSenderAccount{{Account: picked}},
all: []models.Email{{ID: uuid.New()}, {ID: uuid.New()}},
}
pool, err := ResolveCampaignSenderPool(context.Background(), src, testCampaign("explicit"))
if err != nil {
t.Fatalf("ResolveCampaignSenderPool: %v", err)
}
if len(pool.Accounts) != 1 || pool.Accounts[0].ID != picked.ID {
t.Errorf("pool = %+v, want only the picked mailbox", pool.Accounts)
}
if src.allCalls != 0 {
t.Error("a populated explicit pool still consulted every active mailbox")
}
}
// An explicit campaign that also carries tags falls back to those, which is
// what migration 000013 designed. Only the whole-workspace fallback is gone.
func TestResolveCampaignSenderPoolExplicitStillUnionsItsTags(t *testing.T) {
tagged := models.Email{ID: uuid.New()}
src := &stubSenderSource{
tagged: []models.Email{tagged},
all: []models.Email{{ID: uuid.New()}, {ID: uuid.New()}},
}
campaign := testCampaign("explicit")
campaign.EmailTags = []string{"founders"}
pool, err := ResolveCampaignSenderPool(context.Background(), src, campaign)
if err != nil {
t.Fatalf("ResolveCampaignSenderPool: %v", err)
}
if len(pool.Accounts) != 1 || pool.Accounts[0].ID != tagged.ID {
t.Errorf("pool = %+v, want the tagged mailbox", pool.Accounts)
}
if src.allCalls != 0 {
t.Error("the explicit strategy still reached for every active mailbox")
}
}
+31 -9
View File
@@ -16,6 +16,7 @@ import (
"github.com/warmbly/warmbly/internal/errx"
"github.com/warmbly/warmbly/internal/infrastructure/db"
"github.com/warmbly/warmbly/internal/models"
"github.com/warmbly/warmbly/internal/pkg/mailhtml"
"github.com/warmbly/warmbly/internal/utils/paging"
"github.com/warmbly/warmbly/internal/utils/validate"
)
@@ -620,9 +621,17 @@ func (r *campaignRepository) Create(ctx context.Context, userID string, orgID *u
if seq.BodyCode != nil {
bodyCode = *seq.BodyCode
}
// A step given a plain body and no HTML is the API/agent shape.
// The placeholder below is what the composer stores for an empty
// step, and the send path puts body_html on the wire as the
// text/html alternative, so leaving it would ship a blank email
// with the real copy only in the fallback part.
bodyHTML := seq.BodyHTML
if !mailhtml.HasContent(bodyHTML) {
bodyHTML = mailhtml.FromText(seq.BodyPlain)
}
if bodyHTML == "" {
bodyHTML = "<div></div>"
bodyHTML = emptyBodyHTML
}
seqInsert := `
INSERT INTO sequences (
@@ -1609,6 +1618,19 @@ func (r *campaignRepository) ValidateCampaignReady(ctx context.Context, campaign
if senderCount > 0 || tagCount > 0 {
return nil
}
// A campaign that named its mailboxes by hand does NOT fall back to every
// active mailbox (see ExplicitSenderPool), so the check must not either:
// a check that passes a pool the scheduler would find empty is how a
// campaign starts and then parks itself on its first tick (issue #340 is
// the same mistake in the other direction).
var strategy string
if err := r.DB.QueryRow(ctx, `SELECT sender_strategy FROM campaigns WHERE id = $1`, campaignID).Scan(&strategy); err != nil {
return err
}
if strategy == CampaignSenderStrategyExplicit {
return errx.New(errx.BadRequest,
"this campaign sends from mailboxes picked by hand and none are left; pick its sending accounts again, or switch it back to selecting by tag")
}
var activeMailboxes int
if err := r.DB.QueryRow(ctx, `
SELECT COUNT(*) FROM email_accounts
@@ -1979,15 +2001,15 @@ func (r *campaignRepository) GetCampaignSenders(ctx context.Context, campaignID
return senders, nil
}
// ReplaceCampaignSenders atomically swaps the explicit sender pool. An empty
// list is rejected — clearing senders should be done by switching the campaign
// back to sender_strategy='tags'.
// ReplaceCampaignSenders atomically swaps the explicit sender pool.
//
// An empty list is allowed and clears the pool: it is how the dashboard's
// sending-accounts picker deselects everything. What that then means depends
// on the campaign's sender_strategy, which is the safety boundary — a 'tags'
// campaign falls back to its tags or to every active mailbox, and an
// 'explicit' one resolves to no mailboxes at all rather than widening to the
// whole workspace (see ExplicitSenderPool).
func (r *campaignRepository) ReplaceCampaignSenders(ctx context.Context, campaignID uuid.UUID, in []models.CampaignSenderInput) ([]models.CampaignSender, *errx.Error) {
// An empty list is allowed: it clears the explicit sender pool, so the
// campaign falls back to its email tags or, with neither, to every active
// mailbox of the owner. syncCampaignSendersTx handles the empty set safely
// (it deletes all current rows and inserts none).
// Resolve the campaign owner + organization so we can validate mailbox
// ownership against the org (the senders route is org-scoped).
var userID string
+7 -1
View File
@@ -16,6 +16,12 @@ import (
"github.com/warmbly/warmbly/internal/pkg/encrypt"
)
// emptyBodyHTML is what a step with no body carries: the dashboard composer's
// own empty document, so opening a blank step in the editor shows an empty
// canvas rather than nothing at all. It is NOT an empty string, which is why
// every "does this step have a body" check goes through mailhtml.HasContent.
const emptyBodyHTML = "<div></div>"
type SequenceRepository interface {
Create(ctx context.Context, userID, campaignID string) (*models.Sequence, *errx.Error)
Get(ctx context.Context, userID, campaignID string) ([]models.Sequence, *errx.Error)
@@ -194,7 +200,7 @@ func (r *sequenceRepository) Create(ctx context.Context, userID string, campaign
config.SequenceDefaultName,
"",
"",
"<div></div>",
emptyBodyHTML,
nextPos,
}
+51 -7
View File
@@ -567,6 +567,10 @@ func (s *tasksService) HandleCampaignTask(task *proto.ProcessTask) *errx.Error {
bodyHTML = ""
}
// A blank HTML alternative never ships as the part the client prefers.
// Shared with the preview and the test send (see dropBlankHTMLPart).
bodyHTML = dropBlankHTMLPart(bodyHTML, bodyPlain)
// STEP 10.7: A hand-placed {{.UnsubscribeLink}} resolved to the bare signed
// URL; give it an anchor so the recipient reads "Unsubscribe" and not the
// API address (issue #341). After the plain part was derived, so plain text
@@ -1066,16 +1070,56 @@ func (s *tasksService) clearIdle(ctx context.Context, campaign *models.Campaign)
// reason is carried through to the activity log because "paused_no_accounts"
// covers several very different fixes (connect a mailbox, widen a sending
// window, repair DNS) and the status alone cannot tell them apart.
//
// It is deliberately LOUD. This runs unattended, hours after anyone touched
// the campaign, and a pause nobody is told about is a campaign that quietly
// stops sending: an error-level line in the operator's log, an error-level
// entry in the activity feed so the dashboard tints it red, and an org-scoped
// realtime pulse so every teammate's campaign list moves to "paused — no
// accounts" without a refresh.
func (s *tasksService) autoPauseCampaign(ctx context.Context, campaignID, taskID uuid.UUID, reason string) {
s.campaignRepo.UpdateStatusWithLock(ctx, campaignID, "paused_no_accounts")
s.taskRepo.UpdateTaskStatus(ctx, taskID, "completed")
if s.campaignLogRepo != nil {
s.campaignLogRepo.CreateLog(ctx, &repository.CampaignLogEntry{
CampaignID: campaignID,
EventType: "auto_paused",
Message: reason,
})
if taskID != uuid.Nil {
s.taskRepo.UpdateTaskStatus(ctx, taskID, "completed")
}
log.Error().Str("campaign_id", campaignID.String()).Str("reason", reason).
Msg("campaign auto-paused: no mailbox can send for it")
// CreateLogOnce, not CreateLog: the reconciler re-checks paused campaigns
// too, and a repeat of the SAME reason must not fill the feed or re-pulse
// the dashboard every pass. Keyed on the reason rather than the code, so a
// pause whose cause changed (DNS, then no mailbox at all) still says so.
// The write also reports whether this pause is news, which is what gates
// the announcement below.
if s.campaignLogRepo == nil {
return
}
entry := &repository.CampaignLogEntry{
CampaignID: campaignID,
EventType: "auto_paused",
Message: reason,
Metadata: map[string]interface{}{
"level": "error",
"code": "no_accounts",
"reason": reason,
},
}
written, err := s.campaignLogRepo.CreateLogOnce(ctx, entry, "reason", reason, time.Now().Add(-time.Hour))
if err != nil || !written || s.streamingPublisher == nil {
return
}
campaign, gerr := s.campaignRepo.GetByID(ctx, campaignID)
if gerr != nil || campaign == nil {
return
}
s.streamingPublisher.PublishCampaignEvent(ctx, &pubsub.CampaignEvent{
BaseEvent: pubsub.BaseEvent{EventType: pubsub.EventCampaignPaused, UserID: campaign.UserID},
OrgID: campaignOrgID(campaign),
CampaignID: campaignID.String(),
Name: campaign.Name,
Status: "paused_no_accounts",
})
}
// haltOrglessCampaign stops a campaign that reached the send path with no
+23
View File
@@ -92,6 +92,26 @@ func (s *tasksService) PreviewEmail(ctx context.Context, orgID uuid.UUID, in Ema
return out
}
// dropBlankHTMLPart removes an HTML alternative that would render nothing.
//
// An empty HTML part is worse than no HTML part: every modern client prefers
// text/html, so the recipient reads a blank message while the real copy sits
// unread in the text alternative. It catches whatever produced the row, which
// is the composer's <div></div> placeholder on an API-created step, but also a
// variant whose HTML-only spintax resolved away.
//
// With no plain part either there is nothing to fall back to, so the body is
// left alone; a campaign in that state is refused at start instead.
func dropBlankHTMLPart(bodyHTML, bodyPlain string) string {
if bodyHTML == "" || strings.TrimSpace(bodyPlain) == "" {
return bodyHTML
}
if mailhtml.HasContent(bodyHTML) {
return bodyHTML
}
return ""
}
// finishBody applies what the send path adds after rendering, in its order:
// derive the plain part, drop HTML for a plain-text campaign, turn a
// hand-placed unsubscribe link into an anchor, add the mailbox signature,
@@ -104,6 +124,9 @@ func finishBody(bodyHTML, bodyPlain string, textOnly bool, account *models.Email
if textOnly {
bodyHTML = ""
}
// Shared with the send path, so the preview and the test send show the
// same message a recipient gets.
bodyHTML = dropBlankHTMLPart(bodyHTML, bodyPlain)
// After the plain part is derived, so plain text keeps the URL it needs.
linkText := ""
if optOut != nil {
+35
View File
@@ -53,3 +53,38 @@ func TestPreviewTemplatesWithUsesTheGivenLink(t *testing.T) {
t.Fatalf("default preview link changed: %q", q.BodyHTML)
}
}
// A step created through the API or an agent tool carries the composer's empty
// <div></div> under body_html. It is not an empty string, so it shipped as the
// text/html alternative and the recipient opened a blank message with the real
// copy only in the fallback part.
func TestFinishBodyDropsATextlessHTMLPart(t *testing.T) {
h, p := finishBody("<div></div>", "Hi Ana, quick question.", false, nil, nil, "")
if h != "" {
t.Errorf("HTML part = %q, want it dropped so the plain part is what ships", h)
}
if p != "Hi Ana, quick question." {
t.Errorf("plain part = %q, want the step's own text", p)
}
}
// The guard must not touch a body that renders something, including one whose
// only content is an image.
func TestFinishBodyKeepsABodyThatRenders(t *testing.T) {
if h, _ := finishBody("<div>Hi</div>", "Hi", false, nil, nil, ""); h != "<div>Hi</div>" {
t.Errorf("HTML part = %q, want it kept", h)
}
img := `<div><img src="https://example.com/a.png"></div>`
if h, _ := finishBody(img, "see the image", false, nil, nil, ""); h == "" {
t.Error("an image-only body was dropped; it renders for the recipient")
}
}
// With no plain part either there is nothing to fall back to, so dropping the
// HTML would send an empty message. The start-time guard is what stops that
// campaign existing; here the body is left alone.
func TestFinishBodyKeepsAnEmptyHTMLPartWhenThereIsNoPlainText(t *testing.T) {
if h, _ := finishBody("<div></div>", "", false, nil, nil, ""); h == "" {
t.Error("dropped the only part the message had")
}
}
@@ -354,6 +354,12 @@ export function NewCampaignDialog({ open, onClose }: Props) {
return () => document.removeEventListener("keydown", onKey);
}, [open, requestClose]);
// The wizard writes plain text, so it sends plain text. The backend renders
// the HTML part from it: it used to be built here with an escapeHtml that
// turned the quotes in a conditional ({{if eq .Company "Acme"}}) into
// entities, which makes the template fail to parse at send time and ships
// the literal {{if}} to the recipient. The server's version also links bare
// URLs, so a wizard-written step gets click tracking like any other.
function buildSteps() {
return draft.sequences
.filter((s) => s.subject.trim().length > 0 || s.body_plain.trim().length > 0)
@@ -361,7 +367,6 @@ export function NewCampaignDialog({ open, onClose }: Props) {
name: draft.kind === "one_time" ? "Email" : `Step ${i + 1}`,
subject: s.subject.trim(),
body_plain: s.body_plain,
body_html: `<div>${escapeHtml(s.body_plain).replace(/\n/g, "<br/>")}</div>`,
wait_after: i === 0 ? 0 : Math.max(0, s.wait_after),
}));
}
@@ -1476,11 +1481,3 @@ function EstimatePanel({
);
}
function escapeHtml(s: string): string {
return s
.replace(/&/g, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/"/g, "&quot;")
.replace(/'/g, "&#39;");
}
@@ -59,7 +59,10 @@ export interface CreateCampaignInput {
name: string;
subject: string;
body_plain: string;
body_html: string;
// Optional: leave it off for a plain-text step and the backend renders
// the HTML part from body_plain, keeping the line structure and turning
// bare URLs into links so click tracking has hrefs to wrap.
body_html?: string;
body_sync?: boolean;
body_code?: boolean;
wait_after?: number;