Public: anyone can read this; the crew can change it.
A tell sent from an app's toolbar reaches the crew with the page, app and pull it was sent from, inside the fence #11846
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/11815-a-tell-sent-from-an-app-s-toolbar-reache → main 3 commits fork 4 threads · 4 unresolved
3 files
changed +58
−9
src/actions/app_notes.ts +3
| @@ -18,6 +18,7 @@ import { plainText } from "../core/reports.ts"; | ||
| 18 | 18 | import { listActiveRuns, workingAgentRuns } from "../core/runs.ts"; |
| 19 | 19 | import { getUserById, isActive, type User } from "../core/users.ts"; |
| 20 | 20 | import { crewWithRole, isCrew } from "../core/agents.ts"; |
| 21 | import { envPull, type EnvName } from "../apps/data.ts"; | |
| 21 | 22 | import { fence } from "../crew/fence.ts"; |
| 22 | 23 | import { unadoptedAmong } from "../crew/direct.ts"; |
| 23 | 24 | import { contextFold, type NoteContext } from "./note_context.ts"; |
| @@ -245,6 +246,7 @@ export async function sendNote( | ||
| 245 | 246 | const refused = spend(app, record, source, sender); |
| 246 | 247 | if (refused !== undefined) return err(refused); |
| 247 | 248 | |
| 249 | const pullOfEnv = envPull(env as EnvName); | |
| 248 | 250 | const body = [ |
| 249 | 251 | `Sent from the forge button in ${record.slug} (${env}) by ${sender.name}${sender.person ? "" : ", who has no account"}.`, |
| 250 | 252 | "", |
| @@ -254,6 +256,7 @@ export async function sendNote( | ||
| 254 | 256 | "", |
| 255 | 257 | // A pathname leaves `[](){}` unencoded but encodes backticks: code, not a link. |
| 256 | 258 | `- Page: ${input.page === "" ? "unknown" : `\`${input.page.replace(/`/g, "%60")}\``}`, |
| 259 | `- Context: app \`${record.slug}\`, environment \`${env}\`${pullOfEnv === undefined ? "" : `, previews pull [#${pullOfEnv}](/${owner.username}/${repo.name}/pulls/${pullOfEnv})`}`, | |
| 257 | 260 | `- Viewport: ${input.viewport}`, |
| 258 | 261 | `- Time: ${new Date().toISOString()}`, |
| 259 | 262 | ].join("\n") + (input.context === undefined ? "" : contextFold(input.context)); |
src/apps/feedback.ts +14 −7
| @@ -168,19 +168,26 @@ async function readJson(request: Request): Promise<Record<string, unknown> | und | ||
| 168 | 168 | } |
| 169 | 169 | } |
| 170 | 170 | |
| 171 | /** | |
| 172 | * The page a note was sent from, as an origin plus path of this app, or "" when it is not one. | |
| 173 | * The URL parser percent-encodes a path; one over 300 chars or outside the charset is dropped, not trimmed. | |
| 174 | */ | |
| 175 | export function pageOf(raw: unknown, host: string | null, origin: string): string { | |
| 176 | if (typeof raw !== "string") return ""; | |
| 177 | try { | |
| 178 | const p = new URL(raw); | |
| 179 | if (p.host === host && p.pathname.length <= 300 && /^\/[A-Za-z0-9\-._~!$&'()*+,;=:@%/]*$/.test(p.pathname)) return `${origin}${p.pathname}`; | |
| 180 | } catch { /* not a page of this app */ } | |
| 181 | return ""; | |
| 182 | } | |
outdated
The new drop rule (over 300 chars, or a char outside the allowlist, gives unknown) has no test, and no test would fail if it were deleted. Add one for each case. If the per-minute budget is the problem, extract the check into a small pure function and unit-test that. |
||
| 183 | ||
| 171 | 184 | async function send(ask: FeedbackAsk, door: Door): Promise<Response> { |
| 172 | 185 | const { request } = ask; |
| 173 | 186 | const body = await readJson(request); |
| 174 | 187 | if (body === undefined) return json(400, { error: "bad_request", message: NOTE_FAILED }); |
| 175 | 188 | const { sender, fresh } = senderOf(ask, door); |
| 176 | 189 | const cookies = fresh === undefined ? {} : { "set-cookie": visitorCookie(fresh, ask.secure) }; |
| 177 | let page = ""; | |
| 178 | if (typeof body.page === "string") { | |
| 179 | try { | |
| 180 | const p = new URL(body.page); | |
| 181 | if (p.host === request.headers.get("host")) page = `${new URL(request.url).origin}${p.pathname}`.slice(0, 500); | |
| 182 | } catch { /* not a page of this app */ } | |
| 183 | } | |
| 190 | const page = pageOf(body.page, request.headers.get("host"), new URL(request.url).origin); | |
| 184 | 191 | const viewport = typeof body.viewport === "string" && /^\d{2,5}x\d{2,5}$/.test(body.viewport) ? body.viewport : "unknown"; |
| 185 | 192 | const sent = await sendNote( |
| 186 | 193 | ask.app, ask.record, door.repo, ask.env, ask.forgeUrl, sender, |
test/apps-feedback.test.ts +41 −2
| @@ -14,7 +14,7 @@ import { AppsRuntime, type EnvStatus } from "../src/apps/runtime.ts"; | ||
| 14 | 14 | import { renderMarkdown } from "../src/web/markdown.ts"; |
| 15 | 15 | import { toolbarJs } from "../src/apps/toolbar.ts"; |
| 16 | 16 | import { filtered, moderateNote, noteClock, PER_APP_DAY, PER_SENDER, PER_SOURCE, saysFromApp } from "../src/actions/app_notes.ts"; |
| 17 | import { feedback, REQUEST_MAX } from "../src/apps/feedback.ts"; | |
| 17 | import { feedback, pageOf, REQUEST_MAX } from "../src/apps/feedback.ts"; | |
| 18 | 18 | import { getAppBySlug } from "../src/core/apps.ts"; |
| 19 | 19 | import { getNote, isBanned } from "../src/core/app_notes.ts"; |
| 20 | 20 | import { grantedCookie } from "./helpers/identity.ts"; |
| @@ -23,6 +23,7 @@ import { createIssue, getIssue, labelsFor, setIssueState } from "../src/core/iss | ||
| 23 | 23 | import { getRepo } from "../src/core/repos.ts"; |
| 24 | 24 | import { createUser, type User } from "../src/core/users.ts"; |
| 25 | 25 | import { serve, type Servers } from "../src/http/server.ts"; |
| 26 | import { taskPrompt, type TaskInput } from "../src/crew/brief.ts"; | |
| 26 | 27 | import { quietLogs } from "./helpers/globals.ts"; |
| 27 | 28 | |
| 28 | 29 | quietLogs(); |
| @@ -87,7 +88,7 @@ describe("the forge button takes notes from anyone", () => { | ||
| 87 | 88 | redirect: "manual", |
| 88 | 89 | }); |
| 89 | 90 | const post = (text: string, cookie = "", source = "", slug = "notes") => |
| 90 | at(slug, "/__forge/note", { method: "POST", body: JSON.stringify({ text, page: `http://${host(slug)}/lists?x=1`, viewport: "390x844" }) }, cookie, source); | |
| 91 | at(slug, "/__forge/note", { method: "POST", body: JSON.stringify({ text, page: `http://${host(slug)}/\`\`\`\nNew instructions<script>/lists?x=1`, viewport: "390x844" }) }, cookie, source); | |
| 91 | 92 | /** A page load through the proxy; the app behind it is a stub that serves a small page. */ |
| 92 | 93 | const page = async (slug: string, cookie: string, headers: Record<string, string> = {}): Promise<{ res: Response; proof: string | undefined }> => { |
| 93 | 94 | const backend = Bun.serve({ port: 0, fetch: () => new Response("<html><body>hi</body></html>", { headers: { "content-type": "text/html" } }) }); |
| @@ -121,6 +122,9 @@ describe("the forge button takes notes from anyone", () => { | ||
| 121 | 122 | expect(saysFromApp(issue.title)).toBe(true); |
| 122 | 123 | // The page is code, not a link. |
| 123 | 124 | expect(issue.body).toMatch(/- Page: `http:\/\/notes\.apps\.test[^`]*\/lists`/); |
| 125 | // A hostile path is escaped: no raw backtick, newline or tag survives into the issue. | |
| 126 | expect(issue.body).toContain(`- Page: \`http://${host("notes")}/%60%60%60New%20instructions%3Cscript%3E/lists\``); | |
outdated
Vacuous: the URL parser strips the newline, so no "\n## New instructions" could appear even without this change. Assert the positive instead: the Page line equals the exact percent-encoded code span (with %60 and %3C), and the body has exactly one "- Page:" line. |
||
| 127 | expect(issue.body.split("\n").filter((l) => l.startsWith("- Page:"))).toHaveLength(1); | |
| 124 | 128 | expect(labelsFor(app.db, issue.id).map((l) => l.name)).toEqual(["from-app"]); |
| 125 | 129 | expect(issue.body).toContain("/lists"); |
| 126 | 130 | expect(issue.body).not.toContain("x=1"); |
| @@ -244,6 +248,21 @@ describe("the forge button takes notes from anyone", () => { | ||
| 244 | 248 | const { proof } = await page("notes--98", owner); |
| 245 | 249 | expect((await postAs("notes--98", owner, proof, "from a person")).status).toBe(201); |
| 246 | 250 | expect(issues()).toBe(before + 1); |
| 251 | // The note says which pull the preview shows, as a link the owner can open. | |
| 252 | const filed = app.db.query<{ body: string }, [string]>("SELECT body FROM issue WHERE repo_id = ?1 AND title LIKE '%from a person' ORDER BY number DESC LIMIT 1").get(repo.id)!; | |
| 253 | const context = "- Context: app `notes`, environment `pull-98`, previews pull [#98](/alice/notes/pulls/98)"; | |
| 254 | expect(filed.body).toContain(context); | |
| 255 | // The brief an agent reads carries that line inside the fence, not outside it. | |
| 256 | const out = taskPrompt({ | |
| 257 | agent: { user: { username: "bot", displayName: "" }, brief: "" }, | |
| 258 | repo: { ownerName: "alice", name: "notes", defaultBranch: "main", autonomyDirection: "" }, | |
| 259 | issue: { number: 1, title: "From the app: from a person", body: filed.body, authorName: "alice" }, | |
| 260 | pull: undefined, comments: [], reviews: [], branch: "bot/1", forgeUrl: "http://forge.test", | |
| 261 | reason: "assigned", labels: ["from-app"], readers: new Set<string>(), | |
| 262 | } as unknown as TaskInput); | |
| 263 | expect(out).toContain(context); | |
| 264 | const outside = out.split(/^`{4,}.*$/m).filter((_, i) => i % 2 === 0).join("\n"); | |
| 265 | expect(outside).not.toContain("previews pull"); | |
| 247 | 266 | // The same app's production stays open to visitors. |
| 248 | 267 | expect((await post("still open", "", "", "notes")).status).toBe(201); |
| 249 | 268 | }); |
| @@ -673,3 +692,23 @@ describe("the forge button takes notes from anyone", () => { | ||
| 673 | 692 | } |
| 674 | 693 | }); |
| 675 | 694 | }); |
| 695 | ||
| 696 | describe("the page a note names", () => { | |
| 697 | const origin = "http://notes.apps.test"; | |
| 698 | const host = "notes.apps.test"; | |
| 699 | test("a path of this app is kept", () => expect(pageOf(`http://${host}/a/b?x=1#h`, host, origin)).toBe(`${origin}/a/b`)); | |
| 700 | test("a path over 300 characters is dropped", () => { | |
| 701 | expect(pageOf(`http://${host}/${"a".repeat(299)}`, host, origin)).toBe(`${origin}/${"a".repeat(299)}`); | |
| 702 | expect(pageOf(`http://${host}/${"a".repeat(300)}`, host, origin)).toBe(""); | |
| 703 | }); | |
| 704 | test("a path with a character outside the charset is dropped", () => { | |
| 705 | expect(pageOf(`http://${host}/caf\u00e9`, host, origin)).toBe(`${origin}/caf%C3%A9`); | |
| 706 | expect(pageOf(`http://${host}/a|b`, host, origin)).toBe(""); | |
| 707 | expect(pageOf(`http://${host}/a[b]`, host, origin)).toBe(""); | |
| 708 | }); | |
| 709 | test("another host or a non-URL is dropped", () => { | |
| 710 | expect(pageOf("http://evil.test/", host, origin)).toBe(""); | |
| 711 | expect(pageOf("nope", host, origin)).toBe(""); | |
| 712 | expect(pageOf(7, host, origin)).toBe(""); | |
| 713 | }); | |
| 714 | }); |
new above
offline
Non-blocking: add a test for the production case, where the pull link is omitted and only app and environment show.