From b00ab2a7f246adaf5da9474b551267dac34c156a Mon Sep 17 00:00:00 2001 From: ankurmans <12939357+ankurmans@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:22:38 +0100 Subject: [PATCH 1/3] Briefings: reserve the briefing: retry key and send only the draft the run wrote A drafts-level key could create a campaign under a briefing's retry key, and an auto-sending briefing then sent it to the whole list. create_campaign now refuses the briefing: prefix for anyone but a briefing, and settleSend keeps a draft that is not the run's own, or was edited since, as a draft. --- lib/actions/campaigns.js | 2 ++ lib/briefings-delivery.js | 4 +++- tests/briefings-auto-send.test.mjs | 35 +++++++++++++++++++++++++++++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/actions/campaigns.js b/lib/actions/campaigns.js index ccb3138..8deff02 100644 --- a/lib/actions/campaigns.js +++ b/lib/actions/campaigns.js @@ -872,6 +872,8 @@ const createCampaign = defineAction({ const newsletter = await findNewsletter(account, input.newsletter, { _id: 1, paidTier: 1, paidTiers: 1, defaultStyle: 1 }); const isNote = input.kind === "note"; if (isNote) assertShortPostInput(input); + // A briefing that sends by itself sends whatever sits under its retry key (lib/briefings-delivery.js), so nobody else claims it. + if (input.externalId?.startsWith("briefing:") && via !== "briefing") throw actionError("validation_failed", "externalId: the briefing: prefix is reserved for briefings"); if (input.externalId) { const existing = await Campaign.findOne(scoped(account, { externalId: input.externalId })).lean(); if (existing) { diff --git a/lib/briefings-delivery.js b/lib/briefings-delivery.js index faef8af..e275298 100644 --- a/lib/briefings-delivery.js +++ b/lib/briefings-delivery.js @@ -197,8 +197,10 @@ async function settleSend({ account, campaign, payload }) { if (!briefing || briefing.reviewMode !== "send_if_checks_pass" || briefing.status !== "active") return held("review_mode", "Sending by itself was turned off, or the briefing paused, while this run was going."); if (isOnFreeTier(account)) return held("review_mode", "The Free plan keeps a briefing's issues as drafts: a paid plan sends them by itself."); - const current = await Campaign.findOne(scoped(account, { _id: campaign.id })).select({ status: 1 }).lean(); + const current = await Campaign.findOne(scoped(account, { _id: campaign.id })).select({ status: 1, source: 1, markdown: 1 }).lean(); if (current && current.status !== "draft") return { status: "sent", alreadySent: true }; + // What goes out is what the engine wrote and its checks judged: not a draft someone else made under the retry key, or edited since. + if (current?.source !== "briefing" || String(current.markdown ?? "") !== String(payload.markdown ?? "")) return held("checks", "The draft is not the one this run wrote: it was made or changed by someone else."); const cap = Math.min(AUTO_SEND.maxDailyCap, Math.max(1, Number(briefing.autoSendDailyCap) || AUTO_SEND.dailyCap)); const sentToday = await BriefingRun.countDocuments(scoped(account, { briefingId: briefing._id, autoSentAt: { $gte: startOfUtcDay(new Date()) } })); diff --git a/tests/briefings-auto-send.test.mjs b/tests/briefings-auto-send.test.mjs index fe83c96..bf89485 100644 --- a/tests/briefings-auto-send.test.mjs +++ b/tests/briefings-auto-send.test.mjs @@ -3,7 +3,7 @@ import crypto from "node:crypto"; import { after, before, describe, test } from "node:test"; import { renderToStaticMarkup } from "react-dom/server"; import { anonymous, as, catchRedirect, form, platformEmails, startTestApp } from "./helpers/app.mjs"; -import { QSTASH_KEYS, loadSendingModules, setupSender } from "./helpers/sending.mjs"; +import { QSTASH_KEYS, act, loadSendingModules, setupSender } from "./helpers/sending.mjs"; // Briefings sending by itself (docs/BRIEFINGS-DESIGN.md, Auto-send): lib/briefings-delivery.js sends a run's issue through // send_campaign only when every condition holds, and leaves a draft with the reason (heldReason) and a note to the owner when one @@ -77,6 +77,39 @@ before(async () => { after(() => app?.stop()); +describe("a draft that is not the run's own", () => { + test("the briefing: retry key is reserved, so a key or seat cannot claim a briefing's slot", async () => { + const setup = await scenario(); + await assert.rejects( + act(m, owner, "create_campaign", { newsletter: owner.newsletterId, externalId: `briefing:${setup.briefingId}:2026-11-02`, subject: "Mine", format: "markdown", content: "Mine." }), + (error) => error.code === "validation_failed" && /reserved for briefings/.test(error.message) + ); + assert.equal(await m.Campaign.countDocuments({ accountId: owner.accountId, externalId: `briefing:${setup.briefingId}:2026-11-02` }), 0); + }); + + test("a draft already under the retry key from another source is kept as a draft, never sent", async () => { + const setup = await scenario(); + // Made as a key makes a draft, then moved under the retry key as a draft from before the prefix was reserved would sit. + const seeded = await act(m, owner, "create_campaign", { newsletter: owner.newsletterId, subject: "Attacker chosen", previewText: "A short note", format: "markdown", content: "Attacker chosen body." }); + await m.Campaign.updateOne({ accountId: owner.accountId, _id: seeded.id }, { $set: { externalId: `briefing:${setup.briefingId}:2026-11-02` } }); + const { result, campaign, run } = await deliver(setup); + assert.equal(result.status, "drafted"); + assert.equal(result.heldReason, "checks"); + assert.equal(campaign.status, "draft"); + assert.equal(campaign.subject, "Attacker chosen"); + assert.equal(run.autoSentAt, undefined); + }); + + test("the run's own draft edited before the send is kept as a draft", async () => { + const setup = await scenario(); + const first = await delivery.deliverIssue({ ...setup.payload, heldReason: "thin" }); + assert.equal(first.status, "drafted"); + await m.Campaign.updateOne({ accountId: owner.accountId, _id: first.campaignId }, { $set: { markdown: "Edited after the checks." } }); + const { result, campaign } = await deliver(setup); + assert.deepEqual([result.status, result.heldReason, campaign.status], ["drafted", "checks", "draft"]); + }); +}); + describe("a run that passes everything", () => { test("is queued for sending to the audience through send_campaign, counted against the daily cap, and the owner is told with a pause link", async () => { const setup = await scenario(); From 1f357f1f10fd36633b0c3b716f764ac0b454322f Mon Sep 17 00:00:00 2001 From: ankurmans <12939357+ankurmans@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:22:38 +0100 Subject: [PATCH 2/3] Segments: a key or app can't change the rules a scheduled send or active automation uses update_segment is a draft-level action, but a scheduled campaign and an active automation resolve the segment's rules when they run, so changing them moved an approved audience. Through the API and MCP it now answers conflict while a scheduled send or active automation uses the segment; the owner changes it in the dashboard. --- lib/actions/segments.js | 21 +++++++++++++++++++-- tests/segments.test.mjs | 25 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/lib/actions/segments.js b/lib/actions/segments.js index 4563ced..c40c59a 100644 --- a/lib/actions/segments.js +++ b/lib/actions/segments.js @@ -179,13 +179,29 @@ const createSegment = defineAction({ }, }); +// A scheduled send was approved for the audience its segments had then, and an active automation for the one it started with: a key +// or app that can't send or schedule must not move either by changing the rules. The owner does it in the dashboard. +async function assertNoPendingSend(account, segment) { + const scheduled = await Campaign.exists( + scoped(account, { + status: { $in: ["scheduled", "held"] }, + $or: [{ segmentId: segment._id }, { segmentIds: segment._id }, { excludeSegmentIds: segment._id }], + }) + ); + if (scheduled) throw actionError("conflict", "A scheduled send goes to this segment: change its rules in the dashboard, or unschedule the send first"); + const id = String(segment._id); + const automations = await Automation.find(scoped(account, { status: "active" })).select({ name: 1, trigger: 1, steps: 1 }).lean(); + const using = automations.find((automation) => automation.trigger?.segmentId === id || findStepSegmentIds(automation.steps).includes(id)); + if (using) throw actionError("conflict", `The automation ${using.name} is active and uses this segment: change its rules in the dashboard, or pause the automation first`); +} + const updateSegment = defineAction({ name: "update_segment", tryWithoutPlan: true, title: "Update a segment", group: GROUP, description: - "Changes a segment's name, description or rules. Campaigns not yet sent and automations that use it follow the change; a campaign already sending keeps the audience it started with.", + "Changes a segment's name, description or rules. Campaigns not yet sent and automations that use it follow the change; a campaign already sending keeps the audience it started with. Changing the rules of a segment that a scheduled send or an active automation uses answers conflict here: the owner does that in the dashboard.", http: { method: "PATCH", path: "/segments/{id}" }, idempotent: true, example: { name: "Engaged this month" }, @@ -196,9 +212,10 @@ const updateSegment = defineAction({ rules: SegmentRulesSchema.optional().describe("The new rules, which replace the old ones"), }), output: SegmentSchema.extend(CountsSchema.shape), - handler: async ({ account, input }) => { + handler: async ({ account, input, via }) => { const segment = await findSegment(account, input.id); if (input.rules && segment.kind === "static") throw actionError("conflict", "A saved list's contacts can't change: save a new list instead"); + if (input.rules && via !== "dashboard") await assertNoPendingSend(account, segment); const rules = input.rules ? await validateRules(account, segment.newsletterId, input.rules) : null; const set = { ...(input.name !== undefined && { name: input.name }), diff --git a/tests/segments.test.mjs b/tests/segments.test.mjs index ec94a88..cf51a41 100644 --- a/tests/segments.test.mjs +++ b/tests/segments.test.mjs @@ -311,6 +311,31 @@ describe("a campaign sent to a segment", () => { assert.equal(more.body.error.code, "no_recipients"); }); + test("a key or app can't change the rules of a segment a scheduled send or an active automation uses, and can once they don't", async () => { + const user = await setup("segment-pending-send@example.com"); + await t.importReaders(user, 4, { tags: (index) => (index === 0 ? ["vip"] : []) }); + const segment = (await t.api(user.key, "POST", "/segments", { name: "VIPs", rules: vipRules })).body; + const nobody = { match: "all", conditions: [{ type: "tag", op: "has_not", value: "nobody-has-this" }] }; + const id = await t.draft(user, { segment: segment.id }); + assert.equal((await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: vipRules })).status, 200, "a draft follows the segment"); + const at = new Date(Date.now() + 2 * 3_600_000); + assert.equal((await t.api(user.key, "POST", `/campaigns/${id}/schedule`, { scheduledAt: at.toISOString() })).status, 200); + + assert.match(t.assertError(await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: nobody }), 409, "conflict"), /scheduled send goes to this segment/); + assert.equal((await t.api(user.key, "GET", `/campaigns/${id}/recipients`)).body.emails, 1, "the audience is the one that was approved"); + assert.equal((await t.api(user.key, "PATCH", `/segments/${segment.id}`, { name: "Renamed VIPs" })).status, 200, "a name is not an audience"); + assert.equal((await t.api(user.key, "POST", `/campaigns/${id}/unschedule`)).status, 200); + assert.equal((await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: nobody })).status, 200); + await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: vipRules }); + + const created = await t.api(user.key, "POST", "/automations", { name: "VIP series", trigger: { type: "manual" }, steps: [{ type: "branch", segment: "Renamed VIPs", paths: [{ steps: [{ type: "add_tag", tag: "yes" }] }, { steps: [] }] }] }); + assert.equal(created.status, 201, JSON.stringify(created.body)); + assert.equal((await t.api(user.key, "POST", `/automations/${created.body.id}/activate`)).status, 200); + assert.match(t.assertError(await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: nobody }), 409, "conflict"), /VIP series is active and uses this segment/); + assert.equal((await t.api(user.key, "POST", `/automations/${created.body.id}/pause`)).status, 200); + assert.equal((await t.api(user.key, "PATCH", `/segments/${segment.id}`, { rules: nobody })).status, 200); + }); + test("narrows within the audience tier: a paid issue to a segment reaches its paid readers only", async () => { const user = await setup("segment-and-tier@example.com"); await t.m.Newsletter.updateOne({ accountId: user.accountId, _id: user.newsletterId }, { $set: { "paidTier.enabled": true } }); From 5602524cca0c5ec2d7f67fba15e83a03bb69215c Mon Sep 17 00:00:00 2001 From: ankurmans <12939357+ankurmans@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:22:38 +0100 Subject: [PATCH 3/3] OAuth consent: answer for the signed-in person's own account, not a team workspace A team seat of any role could answer a consent request with the owner's workspace selected, and saveConsentAccess then rewrote the owner's connected app level. The consent page and action now use the person's own account (getPersonalAccount). --- app/oauth/consent/actions.js | 5 +++-- app/oauth/consent/page.js | 4 ++-- lib/auth.js | 13 ++++++++++--- tests/oauth.test.mjs | 23 +++++++++++++++++++++++ 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/app/oauth/consent/actions.js b/app/oauth/consent/actions.js index 7bd6fc1..e3dd3a7 100644 --- a/app/oauth/consent/actions.js +++ b/app/oauth/consent/actions.js @@ -4,7 +4,7 @@ import { headers } from "next/headers"; import { redirect } from "next/navigation"; import { isDeletionPending } from "@/lib/account-state"; import { notifyActivity } from "@/lib/alerts"; -import { getAuth, getCurrentAccount } from "@/lib/auth"; +import { getAuth, getPersonalAccount } from "@/lib/auth"; import { answerConsent, saveConsentAccess } from "@/lib/oauth"; import { isExpiredOAuthQuery, readOAuthQuery, readOAuthQueryField } from "@/lib/oauth-rules"; @@ -14,7 +14,8 @@ const EXPIRED_MESSAGE = "This request expired. Go back to the app and connect Fl export async function answerConsentRequest(_state, formData) { const query = readOAuthQueryField(formData.get("oauth_query")); if (!query) return { error: EXPIRED_MESSAGE }; - const current = await getCurrentAccount(); + // The app is connected to the signed-in person's own account: a seat on a team never answers for the owner's. + const current = await getPersonalAccount(); if (!current) redirect(`/signin?${query}`); if (isDeletionPending(current.account)) redirect("/restore"); const accept = formData.get("decision") === "allow"; diff --git a/app/oauth/consent/page.js b/app/oauth/consent/page.js index 2d6579f..09164d3 100644 --- a/app/oauth/consent/page.js +++ b/app/oauth/consent/page.js @@ -6,7 +6,7 @@ import FormConsent from "@/components/FormConsent"; import LogoFlockstack from "@/components/LogoFlockstack"; import { PRESET_INFO, getPresetOfScopes, getPresetsUpTo } from "@/lib/action-scopes"; import { isDeletionPending } from "@/lib/account-state"; -import { getCurrentAccount } from "@/lib/auth"; +import { getPersonalAccount } from "@/lib/auth"; import { readOAuthRequest } from "@/lib/oauth"; import { isLoopbackRedirect } from "@/lib/oauth-rules"; @@ -20,7 +20,7 @@ const SUGGESTED = "send_with_approval"; export default async function ConsentPage({ searchParams }) { const oauthRequest = await readOAuthRequest(await searchParams); if (!oauthRequest || oauthRequest.isExpired) return ; - const current = await getCurrentAccount(); + const current = await getPersonalAccount(); if (!current) redirect(`/signin?${oauthRequest.query}`); if (isDeletionPending(current.account)) redirect("/restore"); diff --git a/lib/auth.js b/lib/auth.js index 5da48f4..e89070a 100644 --- a/lib/auth.js +++ b/lib/auth.js @@ -523,14 +523,21 @@ export async function requireAccount() { return current; } -export async function requirePersonalAccount() { +// The signed-in person's own account, never the workspace of a team they joined. Null when nobody is signed in. +export async function getPersonalAccount() { const current = await getCurrentAccount(); - if (!current) redirect("/signin"); + if (!current) return null; const account = String(current.account.ownerId) === String(current.user.id) ? current.account : await Account.findOne({ ownerId: current.user.id }).lean() ?? await ensureAccount(current.user.id); - if (isDeletionPending(account)) redirect("/restore"); return { user: current.user, account }; } +export async function requirePersonalAccount() { + const current = await getPersonalAccount(); + if (!current) redirect("/signin"); + if (isDeletionPending(current.account)) redirect("/restore"); + return current; +} + export async function requireAdmin() { const session = await getSession(); if (!isAdmin(session?.user)) notFound(); diff --git a/tests/oauth.test.mjs b/tests/oauth.test.mjs index d30e078..5f05488 100644 --- a/tests/oauth.test.mjs +++ b/tests/oauth.test.mjs @@ -908,6 +908,29 @@ describe("connected apps", () => { }); }); +describe("a team seat", () => { + test("never answers a consent request for the owner's account: the owner's connected app keeps the level the owner gave", async () => { + const owner = await customer("oauth-team-owner@example.com", { slug: "oauth-team-owner" }); + const viewer = await customer("oauth-team-viewer@example.com"); + const account = await m.Account.findById(owner.accountId).lean(); + const person = (who) => ({ id: String(who.userId), email: who.email, emailVerified: true }); + const { runAction } = await import("../lib/actions/run-action.js"); + const team = await import("../lib/team.js"); + const invite = await as(owner, () => runAction("invite_team_member", { account, user: person(owner), input: { email: viewer.email, role: "viewer" }, via: "dashboard" })); + await team.acceptTeamInvitation(person(viewer), invite.id); + const consents = () => mongoose.connection.db.collection("oauthConsent"); + const urlFor = () => authorizationUrl(AGENT, { challenge: pkce().challenge, prompt: "consent", scope: `${MCP_SCOPE} offline_access` }); + + assert.ok((await authorizeInBrowser(owner, urlFor(), { access: "drafts" })).searchParams.get("code")); + assert.equal((await consents().findOne({ userId: owner.userId, clientId: AGENT.clientId })).flockstackLevel, "drafts"); + + viewer.jar.set(team.TEAM_COOKIE, String(account._id)); + assert.ok((await authorizeInBrowser(viewer, urlFor(), { access: "full" })).searchParams.get("code"), "the seat can still connect an app of its own"); + assert.equal((await consents().findOne({ userId: owner.userId, clientId: AGENT.clientId })).flockstackLevel, "drafts", "the owner's choice is the owner's"); + assert.equal((await consents().findOne({ userId: viewer.userId, clientId: AGENT.clientId })).flockstackLevel, "full", "the seat's own connection holds the seat's answer"); + }); +}); + describe("API keys", () => { test("keep working beside OAuth, with the same tools and checks", async () => { const { key } = await as(users.owner, () => m.apiKeys.createApiKey(null, form({ name: "Claude Code", level: "full" })));