Website feedback: attribution first, escape <, wait out the fill timer; maintainers only in the approval queue

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

.github/workflows/approve-contributor.yml

@@ -9,9 +9,12 @@ on:
 
 jobs:
   approve:
-    # Only comments that might approve someone join the queue, and they run one
-    # at a time so two lgtm replies can't race on APPROVED_CONTRIBUTORS.
-    if: contains(github.event.comment.body, 'lgtm')  # contains() ignores case
+    # Only maintainers' comments that might approve someone join the queue, and
+    # they run one at a time so two lgtm replies can't race on APPROVED_CONTRIBUTORS.
+    # (The script below still checks for write access.)
+    if: >-
+      contains(github.event.comment.body, 'lgtm') &&
+      contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)
     concurrency:
       group: approve-contributor
       cancel-in-progress: false

site/lib/feedback.js

@@ -20,11 +20,13 @@ const oneLine = (value, max) => String(value ?? "").replace(/\s+/g, " ").trim().
 // Mentions in someone else's text would ping strangers, and issue references
 // (#1, owner/repo#1, GH-1, github.com links) would add backlinks to other
 // people's issues, so break them all with a zero-width space. Escaping & first
-// stops @ and # from turning back into @ and # when GitHub renders.
+// stops @ and # from turning back into @ and # when GitHub renders,
+// and escaping < keeps out raw HTML such as an unclosed <!-- comment.
 const ZWSP = "\u200b";
 export function defang(text) {
   return text
     .replace(/&/g, "&amp;")
+    .replace(/</g, "&lt;")
     .replace(/@(?=[A-Za-z0-9])/g, `@${ZWSP}`)
     .replace(/#(?=\d)/g, `#${ZWSP}`)
     .replace(/\b(GH)-(?=\d)/gi, `$1${ZWSP}-`)
@@ -37,7 +39,9 @@ export function validate(input) {
   if (oneLine(input.website, 200)) return { spam: true };
   // Measured in the browser with a monotonic clock, so clock skew doesn't matter.
   const elapsed = Number(input.elapsed);
-  if (!Number.isFinite(elapsed) || elapsed < MIN_FILL_MS) return { spam: true };
+  if (!Number.isFinite(elapsed)) return { spam: true };
+  // The page waits this long before sending, so only scripts get here; say so anyway.
+  if (elapsed < MIN_FILL_MS) return { error: "That was quick. Send it again in a moment." };
 
   const kind = Object.hasOwn(KINDS, input.kind) ? input.kind : "other";
   const title = oneLine(input.title, LIMITS.title[1]);
@@ -70,16 +74,17 @@ export function buildIssue(value) {
     ["SteamOS build", value.steamos],
   ].filter(([, v]) => v);
 
-  const lines = [defang(value.message), ""];
+  // Our own lines go first, so nothing in the sender's text can hide them.
+  const lines = [
+    value.github
+      ? `> Sent from the website feedback form by @${value.github}.`
+      : "> Sent from the website feedback form. The sender left no GitHub username, so they won't see replies here.",
+    "",
+  ];
   if (details.length) {
     lines.push("| | |", "|---|---|", ...details.map(([k, v]) => `| ${k} | ${defang(v).replace(/\|/g, "\\|")} |`), "");
   }
-  lines.push(
-    "---",
-    value.github
-      ? `Sent from the website feedback form by @${value.github}.`
-      : "Sent from the website feedback form. The sender left no GitHub username, so they won't see replies here.",
-  );
+  lines.push(defang(value.message));
 
   return {
     title: `${kind.title}: ${value.title}`,

site/public/js/feedback.js

@@ -50,7 +50,6 @@ form.addEventListener("submit", async (e) => {
   e.preventDefault();
   errorBox.hidden = true;
   const data = Object.fromEntries(new FormData(form));
-  data.elapsed = Math.round(performance.now() - started);
 
   if (data.title.trim().length < 5) {
     form.elements.title.focus();
@@ -64,10 +63,13 @@ form.addEventListener("submit", async (e) => {
   send.disabled = true;
   send.textContent = "Sending…";
   try {
+    // The server treats anything sent sooner as a script (site/lib/feedback.js MIN_FILL_MS).
+    const wait = 3100 - (performance.now() - started);
+    if (wait > 0) await new Promise((resolve) => setTimeout(resolve, wait));
     const res = await fetch("/api/feedback", {
       method: "POST",
       headers: { "content-type": "application/json" },
-      body: JSON.stringify(data),
+      body: JSON.stringify({ ...data, elapsed: Math.round(performance.now() - started) }),
     });
     const reply = await res.json().catch(() => ({}));
     if (!res.ok) throw new Error(reply.error || "Something went wrong sending that.");

site/test/feedback.test.mjs

@@ -19,9 +19,9 @@ test("accepts a normal report and trims it", () => {
   assert.equal(value.kind, "bug");
 });
 
-test("flags the honeypot and too-fast submissions as spam", () => {
+test("flags the honeypot as spam and asks fast senders to retry", () => {
   assert.deepEqual(validate(form({ website: "http://spam" })), { spam: true });
-  assert.deepEqual(validate(form({ elapsed: 500 })), { spam: true });
+  assert.match(validate(form({ elapsed: 500 })).error, /again/);
   assert.deepEqual(validate(form({ elapsed: undefined })), { spam: true });
 });
 
@@ -43,7 +43,8 @@ test("builds a labelled issue that credits a GitHub user", () => {
   assert.equal(issue.title, "Bug report: Live view freezes");
   assert.deepEqual(issue.labels, ["feedback", "bug"]);
   assert.match(issue.body, /\| Frame Control version \| 0\.3\.1 \|/);
-  assert.match(issue.body, /by @octocat\.$/);
+  assert.match(issue.body, /^> Sent from the website feedback form by @octocat\./);
+  assert.ok(issue.body.endsWith(value.message));
 });
 
 test("anonymous feedback says replies won't reach the sender", () => {
@@ -57,6 +58,7 @@ test("breaks mentions, issue refs and table cells in user text", () => {
   assert.equal(defang("email me@example.com"), "email me@\u200bexample.com");
   assert.equal(defang("see valve/steam#7 and GH-8"), "see valve/steam#\u200b7 and GH\u200b-8");
   assert.equal(defang("@valve @valve #3"), "@valve &#\u200b64;valve #3");
+  assert.equal(defang("end <!--"), "end &lt;!--");
   assert.equal(defang("https://github.com/a/b/issues/1"), "https://github\u200b.com/a/b/issues/1");
   const issue = buildIssue(validate(form({ os: "a | b" })).value);
   assert.match(issue.body, /\| a \\\| b \|/);