From 83882233f65a6ffbc7de75c25984329507fc618a Mon Sep 17 00:00:00 2001 From: smmariquit Date: Tue, 4 Aug 2026 14:56:02 +0800 Subject: [PATCH 1/2] feat(notifications): handle feedback.submitted from Room TBA Room TBA emits `feedback.submitted` from its in-app feedback box (uplbtools/room-tba#888). The envelope enum did not list the type, so the gateway answered 400 and the submission only reached the server log. - Add `feedbackSubmittedPayloadSchema` matching the emit site exactly: feedbackId, message, contact, screen, appVersion, wasOnline. - Post to #development: message body, contact when given, and the context Room TBA attaches. The handler parses the payload rather than casting it, since `/api/feedback` is public and unauthenticated. - Render untrusted text through `safeUserText`: markdown and masked links escaped, every `@` broken with a zero-width space so no submission can ping @everyone/@here/a role, and truncation with an ellipsis instead of letting Discord reject the message. Raw text is cut before escaping because `escapeMarkdown` is quadratic (22k chars is ~7s of blocked event loop). - Deny mentions on every Discord send, not just this path. Closes #7 --- docs/discord-server-guide.md | 2 +- docs/notifications.md | 21 ++++ src/notifications/delivery/discord.ts | 14 ++- src/notifications/feedback-embed.test.ts | 151 +++++++++++++++++++++++ src/notifications/feedback-embed.ts | 112 +++++++++++++++++ src/notifications/notifications.test.ts | 19 +++ src/notifications/types.ts | 19 +++ src/server.test.ts | 23 ++++ 8 files changed, 359 insertions(+), 2 deletions(-) create mode 100644 src/notifications/feedback-embed.test.ts create mode 100644 src/notifications/feedback-embed.ts diff --git a/docs/discord-server-guide.md b/docs/discord-server-guide.md index 81c14b6..ed79295 100644 --- a/docs/discord-server-guide.md +++ b/docs/discord-server-guide.md @@ -23,7 +23,7 @@ Links: https://uplbtools.me · https://github.com/uplbtools · https://room-tba. Bot commands: `/issue`, `/prs`, `/ci`, `/map`, `/status`. Maintainers: `/triage`. Weekly triage cron posts here (Monday 09:00 UTC). -**This channel receives:** GitHub Actions `workflow_run` failures (except E2E/staging-smoke: those have dedicated handlers), Playwright E2E failure summaries from room-tba CI, and triage cron output. +**This channel receives:** GitHub Actions `workflow_run` failures (except E2E/staging-smoke: those have dedicated handlers), Playwright E2E failure summaries from room-tba CI, triage cron output, and in-app feedback sent from room-tba Settings (`feedback.submitted`). ## #github diff --git a/docs/notifications.md b/docs/notifications.md index 14eae46..79e9fa7 100644 --- a/docs/notifications.md +++ b/docs/notifications.md @@ -8,6 +8,7 @@ type NotificationEvent = { type: | "proposal.submitted" | "proposal.reviewed" + | "feedback.submitted" | "deploy.succeeded" | "deploy.failed" | "release.published" @@ -58,6 +59,26 @@ Posted from room-tba admin approve/reject/request-changes routes. Routes to **`#contributors`** (green approve, red reject, amber needs-changes). +## feedback.submitted payload + +Posted from room-tba `POST /api/feedback` (in-app feedback box, [room-tba#881](https://github.com/uplbtools/room-tba/issues/881)). + +| Field | Type | Notes | +| ---------- | --------------- | ----------------------------------------- | +| feedbackId | number | row id in the room-tba `feedback` table | +| message | string | free text, submitter-written | +| contact | string \| null | optional, whatever the submitter typed | +| screen | string \| null | same-site path only, query string stripped | +| appVersion | string \| null | | +| wasOnline | boolean \| null | | + +Routes to **`#development`**. No IP address is sent. + +`/api/feedback` is public and unauthenticated, so `message` and `contact` are untrusted: +the bot parses the payload with `feedbackSubmittedPayloadSchema` and renders text through +`safeUserText` (markdown and masked links escaped, `@` broken with a zero-width space, +truncated to fit the embed). Mentions are additionally denied on every Discord send. + ## CI E2E payloads (`ci.e2e.*`, `ci.staging-*`) Posted from room-tba `discord-notify-e2e.yml`. Routes to `#development` except `ci.staging-smoke.failed` → `#deploys`. diff --git a/src/notifications/delivery/discord.ts b/src/notifications/delivery/discord.ts index 3464097..d65bf43 100644 --- a/src/notifications/delivery/discord.ts +++ b/src/notifications/delivery/discord.ts @@ -4,6 +4,7 @@ import { config } from "../../config.js"; import { BOT_FOOTER, ROOM_TBA_BASE } from "../../constants.js"; import { log } from "../../log.js"; import { channelIdForCiEvent, ciE2eEmbed } from "../ci-embeds.js"; +import { feedbackSubmittedEmbed } from "../feedback-embed.js"; import { githubActivityEmbed } from "../github-embeds.js"; import { channelIdForGithubRoute, githubDiscordRoute } from "../github-routing.js"; import type { TestInventoryPayload } from "../test-inventory.js"; @@ -14,6 +15,7 @@ import type { ProposalReviewOutcome, ProposalSubmittedPayload, } from "../types.js"; +import { feedbackSubmittedPayloadSchema } from "../types.js"; const seenKeys = new Map(); const IDEMPOTENCY_TTL_MS = 24 * 60 * 60 * 1000; @@ -44,7 +46,9 @@ async function sendToChannel( log("warn", `Channel ${channelId} is not a guild text channel`); return; } - await (channel as TextChannel).send(content); + // No notification we deliver is ever meant to ping, and some carry untrusted + // text (feedback, proposals), so deny mentions for every path at the sender. + await (channel as TextChannel).send({ ...content, allowedMentions: { parse: [] } }); } function proposalSubmittedEmbed(payload: ProposalSubmittedPayload): EmbedBuilder { @@ -127,6 +131,14 @@ export async function deliverToDiscord( }); break; } + case "feedback.submitted": { + // Parsed, not cast: the body came from a public unauthenticated endpoint. + const payload = feedbackSubmittedPayloadSchema.parse(event.payload); + await sendToChannel(client, config.channelDevelopmentId, { + embeds: [feedbackSubmittedEmbed(payload, event.occurredAt)], + }); + break; + } case "deploy.succeeded": case "deploy.failed": { const isProd = diff --git a/src/notifications/feedback-embed.test.ts b/src/notifications/feedback-embed.test.ts new file mode 100644 index 0000000..034506e --- /dev/null +++ b/src/notifications/feedback-embed.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, test } from "bun:test"; +import { + feedbackSubmittedEmbed, + safeInlineText, + safeUserText, +} from "./feedback-embed.js"; +import { feedbackSubmittedPayloadSchema } from "./types.js"; + +const OCCURRED_AT = "2026-08-04T09:00:00.000Z"; + +const validPayload = { + feedbackId: 12, + message: "The 3D buildings do not load on my phone.", + contact: "juan@up.edu.ph", + screen: "/map", + appVersion: "2026.08.01", + wasOnline: true, +}; + +describe("feedbackSubmittedPayloadSchema", () => { + test("accepts the payload room-tba emits", () => { + expect(feedbackSubmittedPayloadSchema.safeParse(validPayload).success).toBe(true); + }); + + test("accepts null optional context", () => { + const result = feedbackSubmittedPayloadSchema.safeParse({ + ...validPayload, + contact: null, + screen: null, + appVersion: null, + wasOnline: null, + }); + expect(result.success).toBe(true); + }); + + test("rejects a missing message", () => { + const { message: _message, ...rest } = validPayload; + expect(feedbackSubmittedPayloadSchema.safeParse(rest).success).toBe(false); + }); + + test("rejects an empty message", () => { + const result = feedbackSubmittedPayloadSchema.safeParse({ + ...validPayload, + message: "", + }); + expect(result.success).toBe(false); + }); + + test("rejects wrong field types", () => { + const result = feedbackSubmittedPayloadSchema.safeParse({ + ...validPayload, + feedbackId: "12", + wasOnline: "yes", + }); + expect(result.success).toBe(false); + }); +}); + +describe("safeUserText", () => { + test("neutralises everyone, here, and role mentions", () => { + const rendered = safeUserText("@everyone @here <@&123> <@456>"); + expect(rendered).not.toContain("@everyone"); + expect(rendered).not.toContain("@here"); + expect(rendered).not.toContain("<@&123>"); + expect(rendered).not.toContain("<@456>"); + // Text is still readable, just un-pingable. + expect(rendered).toContain("everyone"); + }); + + test("escapes markdown and masked links", () => { + const rendered = safeUserText("**bold** [click](https://evil.example)"); + expect(rendered).toContain("\\*\\*bold\\*\\*"); + // Escaped bracket: Discord renders the URL plainly instead of hiding it. + expect(rendered).toContain("\\[click]"); + }); + + test("truncates over-long text instead of throwing", () => { + const rendered = safeUserText("a".repeat(5000), 1024); + expect(rendered.length).toBe(1024); + expect(rendered.endsWith("…")).toBe(true); + }); + + test("does not leave a dangling escape before the ellipsis", () => { + const rendered = safeUserText("*".repeat(5000), 100); + expect(rendered.endsWith("\\…")).toBe(false); + expect(rendered.endsWith("…")).toBe(true); + }); + + test("stays fast on a hostile body (escapeMarkdown is quadratic)", () => { + const started = performance.now(); + safeUserText("@everyone *".repeat(5000), 3900); + expect(performance.now() - started).toBeLessThan(1000); + }); + + test("leaves a readable app version unescaped", () => { + expect(safeInlineText("2026.08.01")).toBe("2026.08.01"); + expect(safeInlineText("/map")).toBe("/map"); + }); + + test("still defuses markdown and mentions in a context value", () => { + expect(safeInlineText("/map\n# @everyone")).not.toContain("@everyone"); + expect(safeInlineText("/map\n# huge")).not.toContain("\n"); + }); +}); + +describe("feedbackSubmittedEmbed", () => { + test("renders message, contact, and context", () => { + const data = feedbackSubmittedEmbed(validPayload, OCCURRED_AT).toJSON(); + expect(data.title).toBe("New in-app feedback"); + expect(data.description).toContain("3D buildings"); + const field = (name: string) => data.fields?.find((f) => f.name === name)?.value; + expect(field("Screen")).toBe("/map"); + expect(field("App version")).toBe("2026.08.01"); + expect(field("Online")).toBe("Yes"); + expect(field("Feedback ID")).toBe("12"); + expect(field("Contact")).toContain("juan"); + }); + + test("omits contact when absent and dashes missing context", () => { + const data = feedbackSubmittedEmbed( + { + ...validPayload, + contact: null, + screen: null, + appVersion: null, + wasOnline: null, + }, + OCCURRED_AT, + ).toJSON(); + expect(data.fields?.some((f) => f.name === "Contact")).toBe(false); + expect(data.fields?.find((f) => f.name === "Online")?.value).toBe("—"); + expect(data.fields?.find((f) => f.name === "Screen")?.value).toBe("—"); + }); + + test("keeps a hostile oversized body inside Discord's limits", () => { + const data = feedbackSubmittedEmbed( + { ...validPayload, message: "@everyone *".repeat(2000) }, + OCCURRED_AT, + ).toJSON(); + expect(data.description?.length).toBeLessThanOrEqual(4096); + expect(data.description).not.toContain("@everyone"); + }); + + test("marks offline submissions", () => { + const data = feedbackSubmittedEmbed( + { ...validPayload, wasOnline: false }, + OCCURRED_AT, + ).toJSON(); + expect(data.fields?.find((f) => f.name === "Online")?.value).toBe("No"); + }); +}); diff --git a/src/notifications/feedback-embed.ts b/src/notifications/feedback-embed.ts new file mode 100644 index 0000000..75e64cb --- /dev/null +++ b/src/notifications/feedback-embed.ts @@ -0,0 +1,112 @@ +import { EmbedBuilder, escapeMarkdown } from "discord.js"; +import { BOT_FOOTER } from "../constants.js"; +import type { FeedbackSubmittedPayload } from "./types.js"; + +/** Discord caps embed descriptions at 4096; leave headroom for escape chars. */ +const MESSAGE_MAX = 3900; +/** Discord caps embed field values at 1024. */ +const FIELD_MAX = 1024; +/** + * Cut raw text before escaping: `escapeMarkdown` is quadratic (22k chars is + * roughly 7s of blocked event loop) and escaping only ever grows text, so a + * longer body loses its tail either way. + * + * ponytail: mirrors room-tba's own 2000-char cap. Raise both together if that + * cap moves, or the tail of a long message quietly disappears here. + */ +const RAW_MAX = 2000; + +function neutralise(escaped: string, max: number, clipped: boolean): string { + // Zero-width space after every `@`: nothing a submitter writes can ping + // @everyone, @here, or a role, and the text still reads normally. + const safe = escaped.replaceAll("@", "@\u200b"); + // `clipped` forces the ellipsis even when escaping did not push the text past + // `max`: the raw body was already cut, and that has to stay visible. + if (!clipped && safe.length <= max) return safe; + // A trailing backslash would escape the ellipsis instead of rendering it. + return `${safe.slice(0, max - 1).replace(/\\+$/, "")}…`; +} + +/** + * Render untrusted free text (the message body, the contact line). + * + * room-tba `/api/feedback` is public and unauthenticated, so anything here can + * be hostile: escape markdown — masked links included, or a submission could + * render a fake link into the team channel — neutralise mentions, and truncate + * rather than let Discord reject the whole message. + */ +export function safeUserText(raw: string, max = FIELD_MAX): string { + const limit = Math.min(max, RAW_MAX); + return neutralise( + escapeMarkdown(raw.slice(0, limit), { + heading: true, + bulletedList: true, + numberedList: true, + maskedLink: true, + }), + max, + raw.length > limit, + ); +} + +/** + * Render a single-token context value (screen path, app version). + * + * Whitespace is collapsed first, so no line start is left for heading or list + * markdown to fire from — which also spares readable values the escaping that + * would otherwise render an app version as `2026\.08.01`. + */ +export function safeInlineText(raw: string): string { + return neutralise( + escapeMarkdown(raw.slice(0, RAW_MAX).replace(/\s+/g, " ").trim(), { + maskedLink: true, + }), + FIELD_MAX, + raw.length > RAW_MAX, + ); +} + +function optionalField(value: string | null): string { + const trimmed = value?.trim(); + return trimmed ? safeInlineText(trimmed) : "—"; +} + +export function feedbackSubmittedEmbed( + payload: FeedbackSubmittedPayload, + occurredAt: string, +): EmbedBuilder { + const embed = new EmbedBuilder() + .setColor(0x0369a1) + .setTitle("New in-app feedback") + .setDescription(safeUserText(payload.message, MESSAGE_MAX)) + .addFields( + { name: "Screen", value: optionalField(payload.screen), inline: true }, + { + name: "App version", + value: optionalField(payload.appVersion), + inline: true, + }, + { + name: "Online", + value: payload.wasOnline === null ? "—" : payload.wasOnline ? "Yes" : "No", + inline: true, + }, + { + name: "Feedback ID", + value: String(payload.feedbackId), + inline: true, + }, + ) + .setTimestamp(new Date(occurredAt)) + .setFooter({ text: BOT_FOOTER }); + + if (payload.contact?.trim()) { + embed.addFields({ + name: "Contact", + value: safeUserText(payload.contact.trim()), + inline: true, + }); + } + + return embed; +} diff --git a/src/notifications/notifications.test.ts b/src/notifications/notifications.test.ts index fddf0d6..0097423 100644 --- a/src/notifications/notifications.test.ts +++ b/src/notifications/notifications.test.ts @@ -36,6 +36,25 @@ describe("notificationEventSchema", () => { expect(result.success).toBe(true); }); + test("accepts feedback.submitted envelope", () => { + const result = notificationEventSchema.safeParse({ + schemaVersion: 1, + type: "feedback.submitted", + source: "room-tba", + occurredAt: new Date().toISOString(), + idempotencyKey: "feedback:12:submitted", + payload: { + feedbackId: 12, + message: "Rooms in CAS are missing.", + contact: null, + screen: "/map", + appVersion: "2026.08.01", + wasOnline: true, + }, + }); + expect(result.success).toBe(true); + }); + test("rejects wrong schema version", () => { const result = notificationEventSchema.safeParse({ schemaVersion: 2, diff --git a/src/notifications/types.ts b/src/notifications/types.ts index 371b63e..df361ce 100644 --- a/src/notifications/types.ts +++ b/src/notifications/types.ts @@ -5,6 +5,7 @@ export const notificationEventSchema = z.object({ type: z.enum([ "proposal.submitted", "proposal.reviewed", + "feedback.submitted", "deploy.succeeded", "deploy.failed", "release.published", @@ -42,6 +43,24 @@ export type ProposalSubmittedPayload = { isAnonymous: boolean; }; +/** + * In-app feedback from room-tba `/api/feedback` (uplbtools/room-tba#881). + * + * That endpoint is public and unauthenticated, so `message` and `contact` are + * attacker-controlled: parse the payload instead of casting it, and render it + * through `safeUserText` in `feedback-embed.ts`. + */ +export const feedbackSubmittedPayloadSchema = z.object({ + feedbackId: z.number(), + message: z.string().min(1), + contact: z.string().nullable(), + screen: z.string().nullable(), + appVersion: z.string().nullable(), + wasOnline: z.boolean().nullable(), +}); + +export type FeedbackSubmittedPayload = z.infer; + export type ProposalReviewOutcome = "approved" | "rejected" | "needs_changes"; export type ProposalReviewedPayload = { diff --git a/src/server.test.ts b/src/server.test.ts index ab737ec..f0906f8 100644 --- a/src/server.test.ts +++ b/src/server.test.ts @@ -41,6 +41,29 @@ describe("createServer", () => { expect(res.body).toEqual({ ok: true }); }); + test("POST /notifications accepts feedback.submitted envelope", async () => { + const app = createServer(mockClient); + const res = await request(app) + .post("/notifications") + .send({ + schemaVersion: 1, + type: "feedback.submitted", + source: "room-tba", + occurredAt: new Date().toISOString(), + idempotencyKey: "feedback:7:submitted", + payload: { + feedbackId: 7, + message: "@everyone the map is blank on iOS", + contact: null, + screen: "/map", + appVersion: "2026.08.01", + wasOnline: false, + }, + }); + expect(res.status).toBe(200); + expect(res.body).toEqual({ ok: true }); + }); + test("POST /notifications accepts proposal.reviewed envelope", async () => { const app = createServer(mockClient); const res = await request(app) From 3e445da6d2aae74ffebadbfd07a043f25061e561 Mon Sep 17 00:00:00 2001 From: smmariquit Date: Mon, 24 Aug 2026 01:35:55 +0800 Subject: [PATCH 2/2] docs: describe central Heroku hosting --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 66291c3..e133462 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,12 @@ Set `NOTIFICATION_GATEWAY_URL` on Room TBA to `https://.herokuapp.co This repo can run **solo on Heroku** (default) or be **imported by a personal multi-bot host** via `@uplbtools/discord-bot/runtime`. See [docs/embeddable-runtime.md](docs/embeddable-runtime.md). +## Central host integration + +The production host is [`smmariquit/discord-bot-host`](https://github.com/smmariquit/discord-bot-host), running the other personal Discord bots on one Heroku web dyno. The host calls `createUplbToolsRuntime({ envPrefix: "UPLB_", listen: false })`, so this bot keeps its cron jobs, Discord event handlers, and webhook routes while the host owns the single Heroku `PORT` and process lifecycle. + +The Heroku app is funded by a GitHub Student Developer Pack credit, planned through May 2028 as an estimate. Verify the actual credit expiry in Heroku. + ## Docs - [docs/embeddable-runtime.md](docs/embeddable-runtime.md): library export + host embedding