diff --git a/.github/workflows/approve-contributor.yml b/.github/workflows/approve-contributor.yml index 1f8a8c7..38be871 100644 --- a/.github/workflows/approve-contributor.yml +++ b/.github/workflows/approve-contributor.yml @@ -7,6 +7,11 @@ on: issue_comment: types: [created] +# One approval at a time, so two lgtm replies can't race on the same commit. +concurrency: + group: approve-contributor + cancel-in-progress: false + jobs: approve: runs-on: ubuntu-latest @@ -183,6 +188,8 @@ jobs: git config user.email "github-actions[bot]@users.noreply.github.com" git add .github/APPROVED_CONTRIBUTORS git diff --staged --quiet || git commit -m "chore: approve contributors from issue #${{ github.event.issue.number }}" + # main may have moved since checkout; replay the approval on top of it. + git pull --rebase origin "${{ github.event.repository.default_branch }}" git push - name: Comment on issue diff --git a/site/functions/api/feedback.js b/site/functions/api/feedback.js index 20ab7fc..67a29f7 100644 --- a/site/functions/api/feedback.js +++ b/site/functions/api/feedback.js @@ -45,7 +45,9 @@ export async function onRequestPost({ request, env }) { if (checked.spam) return json(200, { ok: true }); if (checked.error) return json(400, { error: checked.error }); - if (env.FEEDBACK_RL) { + // Best effort: KV is eventually consistent, so bursts can slip past, and a + // storage error lets the feedback through rather than losing it. + if (env.FEEDBACK_RL) try { const ip = request.headers.get("cf-connecting-ip") || "unknown"; const hour = Math.floor(Date.now() / 3600e3); const day = Math.floor(Date.now() / 86400e3); @@ -57,6 +59,8 @@ export async function onRequestPost({ request, env }) { if (await overLimit(env.FEEDBACK_RL, `day:${day}`, TOTAL_PER_DAY, 90000)) { return json(429, { error: "The form has had a busy day. Try again tomorrow, or use GitHub." }); } + } catch (err) { + console.log(`Rate limit check failed: ${err}`); } const res = await fetch(`https://api.github.com/repos/${env.GITHUB_REPO}/issues`, { diff --git a/site/lib/feedback.js b/site/lib/feedback.js index 9b089df..6c04045 100644 --- a/site/lib/feedback.js +++ b/site/lib/feedback.js @@ -17,18 +17,25 @@ const GITHUB_LOGIN = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/; const oneLine = (value, max) => String(value ?? "").replace(/\s+/g, " ").trim().slice(0, max); -// Mentions in someone else's text would ping strangers, and issue refs would -// cross-link, so break both with a zero-width space. +// 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. +const ZWSP = "\u200b"; export function defang(text) { - return text.replace(/@(?=[A-Za-z0-9])/g, "@​").replace(/(^|\s)#(?=\d)/g, "$1#​"); + return text + .replace(/@(?=[A-Za-z0-9])/g, `@${ZWSP}`) + .replace(/#(?=\d)/g, `#${ZWSP}`) + .replace(/\b(GH)-(?=\d)/gi, `$1${ZWSP}-`) + .replace(/\b(github)\.com/gi, `$1${ZWSP}.com`); } // Returns { error } or { value } with every field trimmed and bounded. -export function validate(input, now = Date.now()) { +export function validate(input) { if (!input || typeof input !== "object") return { error: "Send the form as JSON." }; if (oneLine(input.website, 200)) return { spam: true }; - const started = Number(input.started); - if (!Number.isFinite(started) || now - started < MIN_FILL_MS) 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 }; const kind = Object.hasOwn(KINDS, input.kind) ? input.kind : "other"; const title = oneLine(input.title, LIMITS.title[1]); diff --git a/site/public/js/feedback.js b/site/public/js/feedback.js index 663f661..385e1df 100644 --- a/site/public/js/feedback.js +++ b/site/public/js/feedback.js @@ -2,7 +2,7 @@ const form = document.getElementById("feedback"); const errorBox = document.getElementById("error"); const send = document.getElementById("send"); -const started = Date.now(); +const started = performance.now(); const HINTS = { bug: "What you tried, what happened, and what you expected.", @@ -50,7 +50,7 @@ form.addEventListener("submit", async (e) => { e.preventDefault(); errorBox.hidden = true; const data = Object.fromEntries(new FormData(form)); - data.started = started; + data.elapsed = Math.round(performance.now() - started); if (data.title.trim().length < 5) { form.elements.title.focus(); diff --git a/site/test/feedback.test.mjs b/site/test/feedback.test.mjs index c0a18ad..6ce9ed6 100644 --- a/site/test/feedback.test.mjs +++ b/site/test/feedback.test.mjs @@ -3,17 +3,16 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import { buildIssue, defang, hashIp, MIN_FILL_MS, validate } from "../lib/feedback.js"; -const now = 1_800_000_000_000; const form = (over = {}) => ({ kind: "bug", title: "Live view freezes", message: "After about a minute the live view stops updating.", - started: now - MIN_FILL_MS - 1, + elapsed: MIN_FILL_MS + 1, ...over, }); test("accepts a normal report and trims it", () => { - const { value, error } = validate(form({ title: " Live view freezes ", os: " macOS 26 " }), now); + const { value, error } = validate(form({ title: " Live view freezes ", os: " macOS 26 " })); assert.equal(error, undefined); assert.equal(value.title, "Live view freezes"); assert.equal(value.os, "macOS 26"); @@ -21,25 +20,25 @@ test("accepts a normal report and trims it", () => { }); test("flags the honeypot and too-fast submissions as spam", () => { - assert.deepEqual(validate(form({ website: "http://spam" }), now), { spam: true }); - assert.deepEqual(validate(form({ started: now - 500 }), now), { spam: true }); - assert.deepEqual(validate(form({ started: undefined }), now), { spam: true }); + assert.deepEqual(validate(form({ website: "http://spam" })), { spam: true }); + assert.deepEqual(validate(form({ elapsed: 500 })), { spam: true }); + assert.deepEqual(validate(form({ elapsed: undefined })), { spam: true }); }); test("rejects short, long and malformed input", () => { - assert.match(validate(form({ title: "hi" }), now).error, /title/); - assert.match(validate(form({ message: "short" }), now).error, /more/); - assert.match(validate(form({ message: "x".repeat(5001) }), now).error, /under/); - assert.match(validate(form({ github: "not a user!" }), now).error, /GitHub/); - assert.match(validate(null, now).error, /JSON/); + assert.match(validate(form({ title: "hi" })).error, /title/); + assert.match(validate(form({ message: "short" })).error, /more/); + assert.match(validate(form({ message: "x".repeat(5001) })).error, /under/); + assert.match(validate(form({ github: "not a user!" })).error, /GitHub/); + assert.match(validate(null).error, /JSON/); }); test("unknown kinds become other feedback", () => { - assert.equal(validate(form({ kind: "__proto__" }), now).value.kind, "other"); + assert.equal(validate(form({ kind: "__proto__" })).value.kind, "other"); }); test("builds a labelled issue that credits a GitHub user", () => { - const { value } = validate(form({ github: "@octocat", version: "0.3.1", steamos: "20260922" }), now); + const { value } = validate(form({ github: "@octocat", version: "0.3.1", steamos: "20260922" })); const issue = buildIssue(value); assert.equal(issue.title, "Bug report: Live view freezes"); assert.deepEqual(issue.labels, ["feedback", "bug"]); @@ -48,15 +47,17 @@ test("builds a labelled issue that credits a GitHub user", () => { }); test("anonymous feedback says replies won't reach the sender", () => { - const issue = buildIssue(validate(form({ kind: "other" }), now).value); + const issue = buildIssue(validate(form({ kind: "other" })).value); assert.deepEqual(issue.labels, ["feedback"]); assert.match(issue.body, /won't see replies/); }); test("breaks mentions, issue refs and table cells in user text", () => { - assert.equal(defang("ping @valve about #12"), "ping @​valve about #​12"); - assert.equal(defang("email me@example.com"), "email me@​example.com"); - const issue = buildIssue(validate(form({ os: "a | b" }), now).value); + assert.equal(defang("ping @valve about #12"), "ping @\u200bvalve about #\u200b12"); + 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("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 \|/); });