diff --git a/README.md b/README.md index b89689b..b226147 100644 --- a/README.md +++ b/README.md @@ -60,8 +60,10 @@ Defined in [spec 003](docs/specs/003-edits-and-suggestions/spec.md) — the cont Writes need a signed-in session, an IPL origin and the CSRF token. Before any GitHub call, each identity is checked against a deny-list and two caps (20 contributions a day, 5 open at once → `429` with `Retry-After`); bodies are capped -(edits 272 KB, suggestions 16 KB → `413`); a per-IP limit protects sign-in and writes. The open-PR slot is freed by the -webhook when the pull request closes. To block an account (maintainers): +(edits 272 KB, suggestions 16 KB → `413`); a per-IP limit protects sign-in and writes, and link checks for suggestions are capped at 50 a day per identity. The +daily slot is reserved before GitHub is called and given back if the contribution fails; the open-PR slot is freed by +the webhook when the pull request closes. KV has no atomic counters, so parallel requests can exceed a cap by a few +(bounded by the per-IP limit). To block an account (maintainers): ```bash npx wrangler kv key put --binding CONTRIBUTE_KV --env production "deny:github:" "reason" # or deny:orcid: diff --git a/docs/specs/003-edits-and-suggestions/spec.md b/docs/specs/003-edits-and-suggestions/spec.md index c3f4a4d..be303dd 100644 --- a/docs/specs/003-edits-and-suggestions/spec.md +++ b/docs/specs/003-edits-and-suggestions/spec.md @@ -15,7 +15,7 @@ All routes require a session (002) and an IPL origin; responses are JSON. | `GET /v1/source?url=` | — | `{ repository, path, branch, content, sha }` of the page's source file | | `POST /v1/edits` | `{ url, content, sha, summary }` | opens a PR replacing the file; `{ pullRequest: { number, url } }` | | `POST /v1/suggestions` | `{ lang, topicPath, type, name, url, description, access? }` | opens a PR appending to `_data////.yaml`; `{ pullRequest }` | -| `GET /v1/contributions` | — | the signed-in author's open/merged PRs | +| `GET /v1/contributions` | — | the signed-in author's PRs (`open`, `closed` or `merged`) | Rules: 1. Page → source mapping: `en-US`/`pt-BR` content repositories by URL prefix; generated pages map to diff --git a/src/contrib/attribution.ts b/src/contrib/attribution.ts index 496ed38..05922e1 100644 --- a/src/contrib/attribution.ts +++ b/src/contrib/attribution.ts @@ -1,5 +1,5 @@ import type { Identity } from '../auth/identity'; -import { singleLine } from '../text'; +import { neutralizeGitHubText, singleLine } from '../text'; /** * Credit built only from the verified session identity (issue #16, ADR 0003). The commit author is the App; people @@ -27,7 +27,7 @@ export function coAuthorTrailer(identity: Identity): string { /** Commit message: a one-line subject (≤ 72 characters), a blank line and exactly one trailer. */ export function commitMessage(subject: string, identity: Identity): string { - return `${singleLine(subject, 72)}\n\n${coAuthorTrailer(identity)}\n`; + return `${neutralizeGitHubText(singleLine(subject, 72))}\n\n${coAuthorTrailer(identity)}\n`; } /** Markdown credit for the PR body: name and profile link; GitHub logins are linked, not @mentioned. */ diff --git a/src/contrib/labels.ts b/src/contrib/labels.ts index 2b479f7..fd25ca1 100644 --- a/src/contrib/labels.ts +++ b/src/contrib/labels.ts @@ -1,4 +1,4 @@ -import { singleLine } from '../text'; +import { neutralizeGitHubText, singleLine } from '../text'; import type { Language } from './topics'; /** Titles and labels of contribution pull requests (issue #9). */ @@ -6,7 +6,7 @@ export const MAX_TITLE = 120; export function pullRequestTitle(kind: 'edit' | 'suggestion', subject: string): string { const prefix = kind === 'edit' ? 'Edit: ' : 'Resource suggestion: '; - return singleLine(`${prefix}${singleLine(subject, MAX_TITLE)}`, MAX_TITLE); + return singleLine(`${prefix}${neutralizeGitHubText(singleLine(subject, MAX_TITLE))}`, MAX_TITLE); } export interface LabelInput { diff --git a/src/contrib/limits.test.ts b/src/contrib/limits.test.ts index 12c06f9..f6f3619 100644 --- a/src/contrib/limits.test.ts +++ b/src/contrib/limits.test.ts @@ -49,7 +49,7 @@ describe('per-identity limits (before any GitHub call)', () => { it('answers a 6th concurrently open contribution with 429', async () => { const identity = reader(); const session = await signedIn(identity); - for (let n = 1; n <= 5; n += 1) await recordContribution(env.CONTRIBUTE_KV, identity, `o/r#${n}`, new Date(Date.UTC(2026, 0, 1))); + for (let n = 1; n <= 5; n += 1) await recordContribution(env.CONTRIBUTE_KV, identity, `o/r#${n}`); const fake = new FakeGitHub(); const response = await postEdit(app(fake), session.headers); diff --git a/src/contrib/limits.ts b/src/contrib/limits.ts index 4c3cde7..d7ce592 100644 --- a/src/contrib/limits.ts +++ b/src/contrib/limits.ts @@ -5,26 +5,36 @@ import { AppError } from '../errors'; /** * Abuse limits (issue #6). Every write is tied to a signed-in identity and checked here **before** any GitHub call: - * a maintainer deny-list, a daily cap and a cap on pull requests open at once. KV counters are eventually - * consistent, so caps are approximate across locations; the per-IP Rate Limiting binding adds a coarse guard. + * a maintainer deny-list, a daily cap (reserved up front and given back if the contribution fails) and a cap on + * pull requests open at once. Workers KV has no atomic increment, so parallel requests from one identity can still + * slip past a cap by a few; the per-IP Rate Limiting binding bounds that, and a Durable Object is the upgrade path if + * exact counts are ever needed. */ export const DEFAULT_MAX_PRS_PER_DAY = 20; export const DEFAULT_MAX_OPEN_PRS = 5; +export const MAX_LINK_CHECKS_PER_DAY = 50; const DAY_TTL_S = 2 * 24 * 60 * 60; const OPEN_TTL_S = 30 * 24 * 60 * 60; const OPEN_RETRY_S = 60 * 60; type AppContext = Context<{ Bindings: Env }>; -const who = (identity: Pick) => `${identity.provider}:${identity.id}`; +type Who = Pick; +const who = (identity: Who) => `${identity.provider}:${identity.id}`; const today = (now: Date) => now.toISOString().slice(0, 10); -const dayKey = (identity: Pick, now: Date) => `quota:day:${who(identity)}:${today(now)}`; -const openKey = (identity: Pick) => `quota:open:${who(identity)}`; +const dayKey = (identity: Who, now: Date) => `quota:day:${who(identity)}:${today(now)}`; +const probeKey = (identity: Who, now: Date) => `quota:probe:${who(identity)}:${today(now)}`; +const openKey = (identity: Who) => `quota:open:${who(identity)}`; +/** Repository names are case-insensitive on GitHub; keys must be too. */ +const pullKey = (pullRequest: string) => pullRequest.toLowerCase(); function limit(value: string | undefined, fallback: number): number { const parsed = Number(value); return Number.isInteger(parsed) && parsed > 0 ? parsed : fallback; } +const secondsToMidnight = (now: Date) => + Math.ceil((Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate() + 1) - now.getTime()) / 1000); + /** Coarse per-IP limit (Workers Rate Limiting binding, configured for staging and production). */ export async function assertIpAllowed(c: AppContext): Promise { const limiter = c.env.IP_RATE_LIMITER; @@ -34,40 +44,60 @@ export async function assertIpAllowed(c: AppContext): Promise { if (!success) throw new AppError('RATE_LIMITED', 'Too many requests. Please wait a minute.', undefined, 60); } -/** Deny-list and per-identity caps; 403 or 429 with Retry-After, before any GitHub call. */ -export async function assertContributionAllowed(c: AppContext, identity: Identity, now = new Date()): Promise { +export interface Reservation { + /** Give the daily slot back (the contribution failed before a pull request existed). */ + rollback(): Promise; +} + +/** Deny-list and caps; reserves today's slot. 403 or 429 with Retry-After, before any GitHub call. */ +export async function reserveContribution(c: AppContext, identity: Identity, now = new Date()): Promise { const kv = c.env.CONTRIBUTE_KV; if (await kv.get(`deny:${who(identity)}`)) { throw new AppError('FORBIDDEN', 'This account cannot send contributions. Contact the maintainers if you think this is a mistake.'); } - const perDay = limit(c.env.MAX_PRS_PER_DAY, DEFAULT_MAX_PRS_PER_DAY); - const sentToday = Number((await kv.get(dayKey(identity, now))) ?? 0); - if (sentToday >= perDay) { - const midnight = Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate() + 1); - throw new AppError('RATE_LIMITED', `You can send up to ${perDay} contributions a day.`, { limit: 'daily', max: perDay }, Math.ceil((midnight - now.getTime()) / 1000)); - } const maxOpen = limit(c.env.MAX_OPEN_PRS, DEFAULT_MAX_OPEN_PRS); const open = (await kv.get(openKey(identity), 'json')) ?? []; if (open.length >= maxOpen) { throw new AppError('RATE_LIMITED', `You already have ${open.length} contributions waiting for review.`, { limit: 'open', max: maxOpen }, OPEN_RETRY_S); } + const perDay = limit(c.env.MAX_PRS_PER_DAY, DEFAULT_MAX_PRS_PER_DAY); + const key = dayKey(identity, now); + const sentToday = Number((await kv.get(key)) ?? 0); + if (sentToday >= perDay) { + throw new AppError('RATE_LIMITED', `You can send up to ${perDay} contributions a day.`, { limit: 'daily', max: perDay }, secondsToMidnight(now)); + } + await kv.put(key, String(sentToday + 1), { expirationTtl: DAY_TTL_S }); + return { + async rollback() { + const current = Number((await kv.get(key)) ?? 0); + await kv.put(key, String(Math.max(0, current - 1)), { expirationTtl: DAY_TTL_S }); + }, + }; +} + +/** Link checks reach other servers on the reader's behalf, so they are capped per identity too. */ +export async function countLinkCheck(kv: KVNamespace, identity: Identity, now = new Date()): Promise { + const key = probeKey(identity, now); + const used = Number((await kv.get(key)) ?? 0); + if (used >= MAX_LINK_CHECKS_PER_DAY) { + throw new AppError('RATE_LIMITED', 'Too many link checks today.', { limit: 'link-checks', max: MAX_LINK_CHECKS_PER_DAY }, secondsToMidnight(now)); + } + await kv.put(key, String(used + 1), { expirationTtl: DAY_TTL_S }); } -/** Count a pull request that was opened. */ -export async function recordContribution(kv: KVNamespace, identity: Identity, pullRequest: string, now = new Date()): Promise { - const sentToday = Number((await kv.get(dayKey(identity, now))) ?? 0); - await kv.put(dayKey(identity, now), String(sentToday + 1), { expirationTtl: DAY_TTL_S }); +/** Count a pull request that was opened and is waiting for review. */ +export async function recordContribution(kv: KVNamespace, identity: Who, pullRequest: string): Promise { const open = (await kv.get(openKey(identity), 'json')) ?? []; - await kv.put(openKey(identity), JSON.stringify([...new Set([...open, pullRequest])]), { expirationTtl: OPEN_TTL_S }); + await kv.put(openKey(identity), JSON.stringify([...new Set([...open, pullKey(pullRequest)])]), { expirationTtl: OPEN_TTL_S }); } /** Free an open slot when a contribution pull request closes (merged or not), using the body's identity marker. */ export async function releaseContribution(kv: KVNamespace, body: unknown, pullRequest: string): Promise { - const match = typeof body === 'string' ? //.exec(body) : null; + const match = typeof body === 'string' ? /\s*$/.exec(body) : null; if (!match?.[1] || !match[2]) return false; const identity = { provider: match[1] as Identity['provider'], id: match[2] }; const open = (await kv.get(openKey(identity), 'json')) ?? []; - if (!open.includes(pullRequest)) return false; - await kv.put(openKey(identity), JSON.stringify(open.filter((item) => item !== pullRequest)), { expirationTtl: OPEN_TTL_S }); + if (!open.includes(pullKey(pullRequest))) return false; + await kv.put(openKey(identity), JSON.stringify(open.filter((item) => item !== pullKey(pullRequest))), { expirationTtl: OPEN_TTL_S }); return true; } diff --git a/src/contrib/reachability.ts b/src/contrib/reachability.ts index 8e90413..131c9b1 100644 --- a/src/contrib/reachability.ts +++ b/src/contrib/reachability.ts @@ -1,20 +1,41 @@ import { AppError } from '../errors'; +import { assertResourceUrl } from './safety'; + +const MAX_REDIRECTS = 3; /** * Spec 003 rule 3: a suggested link must answer. Sites that refuse bots (401/403/429) still exist and pass; 404, 410, - * 5xx, timeouts and network errors do not. + * 5xx, timeouts and network errors do not. Redirects are followed by hand and every hop must again be a public https + * address, so a link cannot bounce the probe to http, localhost or an IP address. */ export async function assertReachable(url: string, fetcher: typeof fetch, timeoutMs = 5000): Promise { - const attempt = (method: 'HEAD' | 'GET') => - fetcher(url, { method, redirect: 'follow', headers: { 'user-agent': 'ipl-contribute (+https://github.com/InternetPublicLibrary/contribute)' }, signal: AbortSignal.timeout(timeoutMs) }); - let status: number; - try { - let response = await attempt('HEAD'); - if (response.status === 405 || response.status === 501) response = await attempt('GET'); - status = response.status; - } catch { - status = 0; + const unreachable = (status: number) => + new AppError('VALIDATION_FAILED', 'The link does not answer. Check the address and try again.', { fields: ['url'], status }); + const attempt = (target: string, method: 'HEAD' | 'GET') => + fetcher(target, { + method, + redirect: 'manual', + headers: { 'user-agent': 'ipl-contribute (+https://github.com/InternetPublicLibrary/contribute)' }, + signal: AbortSignal.timeout(timeoutMs), + }); + + let target = url; + for (let hop = 0; hop <= MAX_REDIRECTS; hop += 1) { + assertResourceUrl(target); + let response: Response; + try { + response = await attempt(target, 'HEAD'); + if (response.status === 405 || response.status === 501) response = await attempt(target, 'GET'); + } catch { + throw unreachable(0); + } + const status = response.status; + if (status >= 300 && status < 400 && response.headers.has('location')) { + target = new URL(response.headers.get('location') ?? '', target).href; + continue; + } + if ((status >= 200 && status < 300) || [401, 403, 429].includes(status)) return; + throw unreachable(status); } - if ((status >= 200 && status < 400) || [401, 403, 429].includes(status)) return; - throw new AppError('VALIDATION_FAILED', 'The link does not answer. Check the address and try again.', { fields: ['url'], status }); + throw unreachable(310); } diff --git a/src/contrib/review-regressions.test.ts b/src/contrib/review-regressions.test.ts new file mode 100644 index 0000000..2656f3b --- /dev/null +++ b/src/contrib/review-regressions.test.ts @@ -0,0 +1,194 @@ +import { env } from 'cloudflare:workers'; +import { Hono } from 'hono'; +import { beforeAll, describe, expect, it } from 'vitest'; +import type { Env } from '../env'; +import { handleError, readJsonObject } from '../errors'; +import { GitHubApp } from '../github/client'; +import { utf8ToBase64 } from '../github/encoding'; +import { proposeChange } from '../github/pull-requests'; +import { FakeGitHub, generateAppKey } from '../test/fake-github'; +import { GITHUB_READER, signedIn } from '../test/session'; +import { commitMessage } from './attribution'; +import { pullRequestTitle } from './labels'; +import { recordContribution, releaseContribution } from './limits'; +import { assertReachable } from './reachability'; +import { contributionRoutes } from './routes'; + +const MENTION = /(^|[^\w])@[A-Za-z0-9]/; +const REFERENCE = /(^|[^\w])#\d|[\w.-]+\/[\w.-]+#\d|\bGH-\d/i; +const CLOSING = /\b(close[sd]?|fix(e[sd])?|resolve[sd]?)\b/i; +const SHA = 'a'.repeat(40); +const PAGE = '---\ntitle: Sobre\n---\n

texto

\n'; +let pem: string; +beforeAll(async () => { + ({ pem } = await generateAppKey()); +}); + +let counter = 0; +const reader = () => ({ ...GITHUB_READER, id: String(800000 + counter++) }); +const file = (text: string, sha = SHA) => ({ type: 'file', sha, encoding: 'base64', content: utf8ToBase64(text) }); +const change = (repo: string) => ({ repository: { owner: 'InternetPublicLibrary', repo }, base: 'main', branch: 'contribute/x', path: 'a.md', content: 'x', message: 'm', title: 't', body: 'b', labels: ['contribution'] }); + +describe('titles and commit subjects cannot close, reference or mention', () => { + const nasty = 'fixes #12 closes InternetPublicLibrary/pt-BR#3 resolves GH-4 @octocat https://evil.example'; + + it('neutralises the PR title', () => { + const title = pullRequestTitle('suggestion', nasty); + + expect(title).not.toMatch(MENTION); + expect(title).not.toMatch(REFERENCE); + expect(title).not.toMatch(CLOSING); + expect(title).not.toMatch(/https:\/\//); + }); + + it('neutralises the commit subject', () => { + const subject = commitMessage(`Suggest ${nasty}`, GITHUB_READER).split('\n')[0] ?? ''; + + expect(subject).not.toMatch(MENTION); + expect(subject).not.toMatch(REFERENCE); + expect(subject).not.toMatch(CLOSING); + }); +}); + +describe('proposeChange failure handling', () => { + const base = (fake: FakeGitHub) => + fake.withInstallation().on('GET', /\/git\/ref\/heads\/main$/, { object: { sha: 'b' } }).on('POST', /\/git\/refs$/, {}).on('DELETE', /\/git\/refs\/heads\//, new Response(null, { status: 204 })); + + it('deletes the new branch when the commit fails, leaving no orphan', async () => { + const fake = base(new FakeGitHub()).on('PUT', /\/contents\//, new Response('{"message":"conflict"}', { status: 409 })); + const github = new GitHubApp({ appId: '1', privateKey: pem, kv: env.CONTRIBUTE_KV, fetch: fake.fetch }); + + await expect(proposeChange(github, change(`orphan-${Date.now()}`))).rejects.toMatchObject({ code: 'GITHUB_ERROR' }); + expect(fake.calls.some((call) => call.method === 'DELETE' && call.path.endsWith('/git/refs/heads/contribute/x'))).toBe(true); + }); + + it('keeps an opened pull request when labelling fails', async () => { + const fake = base(new FakeGitHub()) + .on('PUT', /\/contents\//, {}) + .on('POST', /\/pulls$/, { number: 3, html_url: 'https://github.com/o/r/pull/3' }) + .on('POST', /\/labels$/, new Response('boom', { status: 500 })); + const github = new GitHubApp({ appId: '1', privateKey: pem, kv: env.CONTRIBUTE_KV, fetch: fake.fetch }); + + await expect(proposeChange(github, change(`labels-${Date.now()}`))).resolves.toEqual({ number: 3, url: 'https://github.com/o/r/pull/3', labelled: false }); + expect(fake.calls.some((call) => call.method === 'DELETE')).toBe(false); + }); +}); + +describe('reachability probe', () => { + const hops = (responses: Record) => { + const seen: string[] = []; + const fetcher: typeof fetch = async (input) => { + const url = String(input); + seen.push(url); + return responses[url]?.clone() ?? new Response(null, { status: 404 }); + }; + return { seen, fetcher }; + }; + const redirect = (location: string) => new Response(null, { status: 302, headers: { location } }); + + it.each(['http://10.0.0.1/', 'http://localhost/', 'https://127.0.0.1/admin'])('refuses a redirect to %s without requesting it', async (target) => { + const { seen, fetcher } = hops({ 'https://attacker.example/': redirect(target) }); + + await expect(assertReachable('https://attacker.example/', fetcher)).rejects.toMatchObject({ code: 'VALIDATION_FAILED' }); + expect(seen).toEqual(['https://attacker.example/']); + }); + + it('follows up to three public https redirects', async () => { + const { fetcher } = hops({ 'https://a.example/': redirect('https://b.example/x'), 'https://b.example/x': redirect('/y'), 'https://b.example/y': new Response(null, { status: 200 }) }); + + await expect(assertReachable('https://a.example/', fetcher)).resolves.toBeUndefined(); + }); + + it('gives up after too many redirects', async () => { + const { fetcher } = hops({ 'https://a.example/': redirect('https://a.example/1'), 'https://a.example/1': redirect('https://a.example/2'), 'https://a.example/2': redirect('https://a.example/3'), 'https://a.example/3': redirect('https://a.example/4') }); + + await expect(assertReachable('https://a.example/', fetcher)).rejects.toMatchObject({ code: 'VALIDATION_FAILED' }); + }); +}); + +describe('quota bookkeeping', () => { + it('frees open slots whatever the case of the repository name', async () => { + const identity = reader(); + await recordContribution(env.CONTRIBUTE_KV, identity, 'internetpubliclibrary/pt-br#5'); + + await expect(releaseContribution(env.CONTRIBUTE_KV, `x\n`, 'InternetPublicLibrary/pt-BR#5')).resolves.toBe(true); + }); + + it('ignores a marker that is not the last line of the body', async () => { + const identity = reader(); + await recordContribution(env.CONTRIBUTE_KV, identity, 'o/r#1'); + + await expect(releaseContribution(env.CONTRIBUTE_KV, `\nmore`, 'o/r#1')).resolves.toBe(false); + }); +}); + +describe('routes after review', () => { + const IPL = 'http://localhost:4000'; + const fakeSite = () => + new FakeGitHub() + .withInstallation() + .on('GET', /^\/repos\/[^/]+\/[^/]+$/, { default_branch: 'main' }) + .on('GET', /\/contents\/_data\/pt-BR\/menu\.yaml$/, file('- title: Computação\n path: /pt-BR/computacao/\n')) + .on('GET', /\/contents\/sobre\/index\.(html|md)$/, new Response('{}', { status: 404 })) + .on('GET', /\/contents\/_data\/pt-BR\/computacao\/learn\.yaml$/, new Response('{}', { status: 404 })); + const call = (fake: FakeGitHub, path: string, body: unknown, headers: Record, probe: typeof fetch = async () => new Response(null)) => { + const app = new Hono<{ Bindings: Env }>(); + app.onError(handleError); + app.route('/v1', contributionRoutes({ fetch: fake.fetch, probe })); + return app.request(path, { method: 'POST', body: JSON.stringify(body), headers: { 'content-type': 'application/json', ...headers } }, { ...env, GITHUB_APP_ID: '1', GITHUB_APP_PRIVATE_KEY: pem }); + }; + const today = () => new Date().toISOString().slice(0, 10); + + it('returns the current content when the page changes between read and commit', async () => { + let reads = 0; + const fake = fakeSite() + .on('GET', /\/contents\/sobre\.html$/, () => Response.json(reads++ === 0 ? file(PAGE) : file(`${PAGE}

novo

\n`, 'c'.repeat(40)))) + .on('GET', /\/git\/ref\/heads\/main$/, { object: { sha: 'b' } }) + .on('POST', /\/git\/refs$/, {}) + .on('PUT', /\/contents\//, new Response('{"message":"sha mismatch"}', { status: 409 })) + .on('DELETE', /\/git\/refs\//, new Response(null, { status: 204 })); + const session = await signedIn(reader()); + const response = await call(fake, '/v1/edits', { url: `${IPL}/pt-BR/sobre/`, content: PAGE.replace('texto', 'texto novo'), sha: SHA, summary: 's' }, session.headers); + + expect(response.status).toBe(409); + expect(await response.json()).toMatchObject({ error: { code: 'STALE_SHA', details: { content: `${PAGE}

novo

\n`, sha: 'c'.repeat(40) } } }); + }); + + it('gives the daily slot back when a contribution fails', async () => { + const identity = reader(); + const session = await signedIn(identity); + const suggestion = { lang: 'pt-BR', topicPath: '/pt-BR/computacao/', type: 'learn', name: 'X', url: 'https://x.example/', description: 'y' }; + const response = await call(fakeSite(), '/v1/suggestions', suggestion, session.headers, async () => new Response(null, { status: 404 })); + + expect(response.status).toBe(422); + expect(await env.CONTRIBUTE_KV.get(`quota:day:github:${identity.id}:${today()}`)).toBe('0'); + }); + + it('limits link checks per identity', async () => { + const identity = reader(); + const session = await signedIn(identity); + await env.CONTRIBUTE_KV.put(`quota:probe:github:${identity.id}:${today()}`, '50'); + const suggestion = { lang: 'pt-BR', topicPath: '/pt-BR/computacao/', type: 'learn', name: 'X', url: 'https://x.example/', description: 'y' }; + const response = await call(fakeSite(), '/v1/suggestions', suggestion, session.headers); + + expect(response.status).toBe(429); + expect(await response.json()).toMatchObject({ error: { details: { limit: 'link-checks' } } }); + }); +}); + +describe('streamed bodies', () => { + it('stops reading a body without Content-Length once it passes the limit', async () => { + let pulled = 0; + const stream = new ReadableStream({ + pull(controller) { + pulled += 1; + controller.enqueue(new Uint8Array(64 * 1024)); + if (pulled > 1000) controller.close(); + }, + }); + const request = new Request('https://x/', { method: 'POST', body: stream, headers: { 'content-type': 'application/json' }, duplex: 'half' } as RequestInit); + + await expect(readJsonObject(request, 256 * 1024)).rejects.toMatchObject({ code: 'PAYLOAD_TOO_LARGE' }); + expect(pulled).toBeLessThan(20); + }); +}); diff --git a/src/contrib/routes.test.ts b/src/contrib/routes.test.ts index 53f5887..24ee12a 100644 --- a/src/contrib/routes.test.ts +++ b/src/contrib/routes.test.ts @@ -115,7 +115,7 @@ describe('POST /v1/edits', () => { const response = await post(fake, '/v1/edits', edit(proposed), session); expect(response.status).toBe(201); - expect(await response.json()).toEqual({ pullRequest: { number: 12, url: `https://github.com/${SANDBOX}/pull/12` } }); + expect(await response.json()).toEqual({ pullRequest: { number: 12, url: `https://github.com/${SANDBOX}/pull/12`, labelled: true } }); expect(base64ToUtf8(put(fake)?.content ?? '')).toBe(proposed); expect(put(fake)).toMatchObject({ sha: SHA, branch: expect.stringMatching(/^contribute\//) }); expect(put(fake)?.message).toBe('Edit sobre.html\n\nCo-authored-by: The Octocat <583231+octocat@users.noreply.github.com>\n'); @@ -250,8 +250,11 @@ describe('GET /v1/contributions', () => { const session = await signedIn(); const fake = github().on('GET', /^\/search\/issues$/, { items: [ - { number: 12, title: 'Edit: Sobre', html_url: 'https://github.com/x/pull/12', state: 'closed', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: '2026-10-11T00:00:00Z' } }, - { number: 13, title: 'Resource suggestion: K', html_url: 'https://github.com/x/pull/13', state: 'open', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: null } }, + { number: 12, title: 'Edit: Sobre', body: 'a\n', html_url: 'https://github.com/x/pull/12', state: 'closed', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: '2026-10-11T00:00:00Z' } }, + { number: 13, title: 'Resource suggestion: K', body: 'b\n\n', html_url: 'https://github.com/x/pull/13', state: 'open', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: null } }, + // Someone else's PR whose summary quotes the marker words: GitHub search finds it, the route must not list it. + { number: 14, title: 'Spoof', body: '> contribute-identity:github:583231\n', html_url: 'https://github.com/x/pull/14', state: 'open', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: null } }, + { number: 15, title: 'Spoof 2', body: '> <!-- contribute-identity:github:583231 -->', html_url: 'https://github.com/x/pull/15', state: 'open', repository_url: `https://api.github.com/repos/${SANDBOX}`, pull_request: { merged_at: null } }, ], }); const response = await request(fake, '/v1/contributions', { headers: session.headers }); diff --git a/src/contrib/routes.ts b/src/contrib/routes.ts index 7e12020..e0fead0 100644 --- a/src/contrib/routes.ts +++ b/src/contrib/routes.ts @@ -4,7 +4,7 @@ import { assertWriteAllowed, requireSession, type Session } from '../auth/sessio import { loadSettings, type Settings } from '../config/environment'; import type { Env } from '../env'; import { AppError, readJsonObject } from '../errors'; -import { assertContributionAllowed, assertIpAllowed, recordContribution } from './limits'; +import { assertIpAllowed, countLinkCheck, recordContribution, type Reservation, reserveContribution } from './limits'; import { type GitHubApp, gitHubAppFromEnv, type RepositoryRef } from '../github/client'; import { proposeChange } from '../github/pull-requests'; import { defaultBranch, readFile, type RepositoryFile } from '../github/repository'; @@ -70,12 +70,21 @@ export function matchLineEndings(original: string, proposed: string): string { } /** Writes: a session, an IPL origin with the CSRF token, the per-IP limit, then the identity's deny-list and caps. */ -async function writeSession(c: AppContext, settings: Settings): Promise { +async function writeSession(c: AppContext, settings: Settings): Promise<{ session: Session; reservation: Reservation }> { const session = await requireSession(c); assertWriteAllowed(c, session, settings.allowedOrigins); await assertIpAllowed(c); - await assertContributionAllowed(c, session.identity); - return session; + return { session, reservation: await reserveContribution(c, session.identity) }; +} + +/** Run a write; if it fails before a pull request exists, the reserved daily slot is given back. */ +async function withReservation(reservation: Reservation, work: () => Promise): Promise { + try { + return await work(); + } catch (error) { + await reservation.rollback(); + throw error; + } } export function contributionRoutes(deps: ContributionDeps = {}) { @@ -106,94 +115,100 @@ export function contributionRoutes(deps: ContributionDeps = {}) { routes.post('/edits', async (c) => { const settings = loadSettings(c.env); - const session = await writeSession(c, settings); - const body = await readJsonObject(c.req.raw, MAX_EDIT_BODY); - strictKeys(body, ['url', 'content', 'sha', 'summary']); - const { url, content, sha, summary } = body; - const invalid = [ - typeof url !== 'string' && 'url', - (typeof content !== 'string' || content === '') && 'content', - (typeof sha !== 'string' || !SHA.test(sha)) && 'sha', - (typeof summary !== 'string' || summary.trim() === '') && 'summary', - ].filter((field): field is string => Boolean(field)); - if (invalid.length) throw new AppError('VALIDATION_FAILED', 'Some fields are missing or invalid.', { fields: invalid }); - if (new TextEncoder().encode(content as string).length > MAX_EDIT_BYTES) throw new AppError('PAYLOAD_TOO_LARGE', 'The page is too large to edit here.'); - - const page = pageSource(url, settings); - const app = github(c); - const { file: current, branch } = await findSource(app, c.env.CONTRIBUTE_KV, page); - if (!current) throw await missingSource(app, c.env.CONTRIBUTE_KV, settings, page); - // Optimistic concurrency: the reader edited the version with this sha; anything newer wins and is returned. - if (current.sha !== sha) { - throw new AppError('STALE_SHA', 'The page changed while you were editing it.', { content: current.content, sha: current.sha }); - } - - const proposed = matchLineEndings(current.content, content as string); - if (proposed === current.content) throw new AppError('VALIDATION_FAILED', 'The edit does not change the page.', { fields: ['content'] }); - const scan = scanPageEdit(current.content, proposed, current.path.endsWith('.md') ? 'markdown' : 'html'); - - const identity = session.identity; - const pageUrl = `${new URL(url as string).origin}${page.pagePath}`; - try { - const pullRequest = await proposeChange(app, { - repository: page.repository, - base: branch, - branch: contributionBranch(newContributionId()), - path: current.path, - content: proposed, - sha: current.sha, - message: commitMessage(`Edit ${current.path}`, identity), - title: pullRequestTitle('edit', pageTitle(current.content, current.path)), - body: renderPullRequestBody({ kind: 'edit', identity, pageUrl, path: current.path, summary: summary as string, containsHtml: scan.containsHtml }), - labels: contributionLabels({ kind: 'edit', lang: page.lang, containsHtml: scan.containsHtml }), - }); - await recordContribution(c.env.CONTRIBUTE_KV, identity, `${repoName(page.repository)}#${pullRequest.number}`); - return c.json({ pullRequest }, 201); - } catch (error) { - // The default branch moved between our read and the commit. - if (error instanceof AppError && error.details?.githubStatus === 409) { - throw new AppError('STALE_SHA', 'The page changed while you were editing it. Reload and try again.'); + const { session, reservation } = await writeSession(c, settings); + return withReservation(reservation, async () => { + const body = await readJsonObject(c.req.raw, MAX_EDIT_BODY); + strictKeys(body, ['url', 'content', 'sha', 'summary']); + const { url, content, sha, summary } = body; + const invalid = [ + typeof url !== 'string' && 'url', + (typeof content !== 'string' || content === '') && 'content', + (typeof sha !== 'string' || !SHA.test(sha)) && 'sha', + (typeof summary !== 'string' || summary.trim() === '') && 'summary', + ].filter((field): field is string => Boolean(field)); + if (invalid.length) throw new AppError('VALIDATION_FAILED', 'Some fields are missing or invalid.', { fields: invalid }); + if (new TextEncoder().encode(content as string).length > MAX_EDIT_BYTES) throw new AppError('PAYLOAD_TOO_LARGE', 'The page is too large to edit here.'); + + const page = pageSource(url, settings); + const app = github(c); + const { file: current, branch } = await findSource(app, c.env.CONTRIBUTE_KV, page); + if (!current) throw await missingSource(app, c.env.CONTRIBUTE_KV, settings, page); + // Optimistic concurrency: the reader edited the version with this sha; anything newer wins and is returned. + if (current.sha !== sha) { + throw new AppError('STALE_SHA', 'The page changed while you were editing it.', { content: current.content, sha: current.sha }); + } + + const proposed = matchLineEndings(current.content, content as string); + if (proposed === current.content) throw new AppError('VALIDATION_FAILED', 'The edit does not change the page.', { fields: ['content'] }); + const scan = scanPageEdit(current.content, proposed, current.path.endsWith('.md') ? 'markdown' : 'html'); + + const identity = session.identity; + const pageUrl = `${new URL(url as string).origin}${page.pagePath}`; + try { + const pullRequest = await proposeChange(app, { + repository: page.repository, + base: branch, + branch: contributionBranch(newContributionId()), + path: current.path, + content: proposed, + sha: current.sha, + message: commitMessage(`Edit ${current.path}`, identity), + title: pullRequestTitle('edit', pageTitle(current.content, current.path)), + body: renderPullRequestBody({ kind: 'edit', identity, pageUrl, path: current.path, summary: summary as string, containsHtml: scan.containsHtml }), + labels: contributionLabels({ kind: 'edit', lang: page.lang, containsHtml: scan.containsHtml }), + }); + await recordContribution(c.env.CONTRIBUTE_KV, identity, `${repoName(page.repository)}#${pullRequest.number}`); + return c.json({ pullRequest }, 201); + } catch (error) { + // The default branch moved between our read and the commit. + if (error instanceof AppError && error.details?.githubStatus === 409) { + const { file: latest } = await findSource(app, c.env.CONTRIBUTE_KV, page); + throw new AppError('STALE_SHA', 'The page changed while you were editing it.', latest ? { content: latest.content, sha: latest.sha } : undefined); + } + throw error; } - throw error; - } + }); }); routes.post('/suggestions', async (c) => { const settings = loadSettings(c.env); - const session = await writeSession(c, settings); - const suggestion = parseSuggestion(await readJsonObject(c.req.raw, MAX_SUGGESTION_BODY)); - const website = settings.repositories.website; - if (!website) throw new AppError('NOT_CONFIGURED', 'No website repository is configured.'); - - const app = github(c); - const topic = resolveTopic(await loadMenu(app, c.env.CONTRIBUTE_KV, website, suggestion.lang), suggestion.lang, suggestion.topicPath); - const path = resourceFilePath(topic, suggestion.type); - await assertReachable(suggestion.url, probe); - - const branch = await defaultBranch(app, c.env.CONTRIBUTE_KV, website); - const existing = await readFile(app, website, path, branch); - const content = appendEntry(existing?.content ?? null, toEntry(suggestion)); - const identity = session.identity; - const pullRequest = await proposeChange(app, { - repository: website, - base: branch, - branch: contributionBranch(newContributionId()), - path, - content, - ...(existing ? { sha: existing.sha } : {}), - message: commitMessage(`Suggest ${suggestion.name} for ${topic.title}`, identity), - title: pullRequestTitle('suggestion', suggestion.name), - body: renderPullRequestBody({ - kind: 'suggestion', - identity, - pageUrl: `${c.req.header('origin') ?? ''}${topic.path}`, + const { session, reservation } = await writeSession(c, settings); + return withReservation(reservation, async () => { + const suggestion = parseSuggestion(await readJsonObject(c.req.raw, MAX_SUGGESTION_BODY)); + const website = settings.repositories.website; + if (!website) throw new AppError('NOT_CONFIGURED', 'No website repository is configured.'); + + const app = github(c); + const topic = resolveTopic(await loadMenu(app, c.env.CONTRIBUTE_KV, website, suggestion.lang), suggestion.lang, suggestion.topicPath); + const path = resourceFilePath(topic, suggestion.type); + await countLinkCheck(c.env.CONTRIBUTE_KV, session.identity); + await assertReachable(suggestion.url, probe); + + const branch = await defaultBranch(app, c.env.CONTRIBUTE_KV, website); + const existing = await readFile(app, website, path, branch); + const content = appendEntry(existing?.content ?? null, toEntry(suggestion)); + const identity = session.identity; + const pullRequest = await proposeChange(app, { + repository: website, + base: branch, + branch: contributionBranch(newContributionId()), path, - fields: { Name: suggestion.name, Link: suggestion.url, Description: suggestion.description, Access: suggestion.access, Organization: suggestion.organization }, - }), - labels: contributionLabels({ kind: 'suggestion', lang: suggestion.lang, topic: topic.categorySegment }), + content, + ...(existing ? { sha: existing.sha } : {}), + message: commitMessage(`Suggest ${suggestion.name} for ${topic.title}`, identity), + title: pullRequestTitle('suggestion', suggestion.name), + body: renderPullRequestBody({ + kind: 'suggestion', + identity, + pageUrl: `${c.req.header('origin') ?? ''}${topic.path}`, + path, + fields: { Name: suggestion.name, Link: suggestion.url, Description: suggestion.description, Access: suggestion.access, Organization: suggestion.organization }, + }), + labels: contributionLabels({ kind: 'suggestion', lang: suggestion.lang, topic: topic.categorySegment }), }); await recordContribution(c.env.CONTRIBUTE_KV, identity, `${repoName(website)}#${pullRequest.number}`); return c.json({ pullRequest }, 201); + }); }); routes.get('/contributions', async (c) => { @@ -202,16 +217,20 @@ export function contributionRoutes(deps: ContributionDeps = {}) { const repositories = Object.values(settings.repositories); const first = repositories[0]; if (!first) return c.json({ contributions: [] }); - const marker = identityMarker(session.identity).replace(/^$/g, ''); + const exact = identityMarker(session.identity); + const marker = exact.replace(/^$/g, ''); const query = [`"${marker}"`, 'in:body', 'is:pr', ...new Set(repositories.map((r) => `repo:${repoName(r)}`))].join(' '); - const result = await github(c).request<{ items: { number: number; title: string; html_url: string; state: string; repository_url: string; pull_request?: { merged_at: string | null } }[] }>( + const result = await github(c).request<{ items: { number: number; title: string; body?: string | null; html_url: string; state: string; repository_url: string; pull_request?: { merged_at: string | null } }[] }>( first, 'GET', `/search/issues?q=${encodeURIComponent(query)}&sort=created&order=desc&per_page=50`, 'search contributions', ); + // Search matches words, and reader text can contain the marker's words: keep only bodies whose last line is the + // exact marker. Reader text cannot produce it, because '<' is escaped in the body template. + const own = result.items.filter((item) => (item.body ?? '').trimEnd().endsWith(exact)); return c.json({ - contributions: result.items.map((item) => ({ + contributions: own.map((item) => ({ repository: item.repository_url.split('/repos/')[1] ?? '', number: item.number, title: item.title, diff --git a/src/errors.ts b/src/errors.ts index 11278af..f8f13cc 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -91,8 +91,28 @@ export async function readJsonObject(request: Request, maxBytes = MAX_BODY_BYTES } const tooLarge = () => new AppError('PAYLOAD_TOO_LARGE', `The request body is larger than ${Math.round(maxBytes / 1024)} KB.`); if (Number(request.headers.get('content-length') ?? 0) > maxBytes) throw tooLarge(); - const bytes = new Uint8Array(await request.arrayBuffer()); - if (bytes.length > maxBytes) throw tooLarge(); + // Read as a stream and stop as soon as the limit is passed, whatever Content-Length claimed. + const chunks: Uint8Array[] = []; + let size = 0; + if (request.body) { + const reader = request.body.getReader(); + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + size += value.byteLength; + if (size > maxBytes) { + await reader.cancel().catch(() => undefined); + throw tooLarge(); + } + chunks.push(value); + } + } + const bytes = new Uint8Array(size); + let offset = 0; + for (const chunk of chunks) { + bytes.set(chunk, offset); + offset += chunk.byteLength; + } let value: unknown; try { value = JSON.parse(new TextDecoder().decode(bytes)); diff --git a/src/github/client.test.ts b/src/github/client.test.ts index 4bdb70c..00df7b0 100644 --- a/src/github/client.test.ts +++ b/src/github/client.test.ts @@ -83,7 +83,7 @@ describe('GitHubApp', () => { const repository = freshRepo(); const cold = app(fake); - await expect(proposeChange(cold, change(repository))).resolves.toEqual({ number: 7, url: 'https://github.com/InternetPublicLibrary/test/pull/7' }); + await expect(proposeChange(cold, change(repository))).resolves.toEqual({ number: 7, url: 'https://github.com/InternetPublicLibrary/test/pull/7', labelled: true }); expect(cold.calls).toBe(6); const warm = app(fake); diff --git a/src/github/pull-requests.ts b/src/github/pull-requests.ts index 5e0f327..b866f3d 100644 --- a/src/github/pull-requests.ts +++ b/src/github/pull-requests.ts @@ -21,6 +21,8 @@ export interface ProposedChange { export interface OpenedPullRequest { number: number; url: string; + /** False when the labels could not be added; the pull request itself is open. */ + labelled?: boolean; } /** @@ -39,21 +41,34 @@ export async function proposeChange(github: GitHubApp, change: ProposedChange): ref: `refs/heads/${change.branch}`, sha: base.object.sha, }); - await github.request(change.repository, 'PUT', `${repo}/contents/${filePath}`, 'write file', { - message: change.message, - content: utf8ToBase64(change.content), - branch: change.branch, - ...(change.sha ? { sha: change.sha } : {}), - }); - const pull = await github.request<{ number: number; html_url: string }>(change.repository, 'POST', `${repo}/pulls`, 'open pull request', { - title: change.title, - body: change.body, - head: change.branch, - base: change.base, - maintainer_can_modify: true, - }); + let pull: { number: number; html_url: string }; + try { + await github.request(change.repository, 'PUT', `${repo}/contents/${filePath}`, 'write file', { + message: change.message, + content: utf8ToBase64(change.content), + branch: change.branch, + ...(change.sha ? { sha: change.sha } : {}), + }); + pull = await github.request<{ number: number; html_url: string }>(change.repository, 'POST', `${repo}/pulls`, 'open pull request', { + title: change.title, + body: change.body, + head: change.branch, + base: change.base, + maintainer_can_modify: true, + }); + } catch (error) { + // No pull request will point at this branch: remove it (best effort) so no orphan stays behind. + await github + .request(change.repository, 'DELETE', `${repo}/git/refs/heads/${change.branch.split('/').map(encodeURIComponent).join('/')}`, 'delete branch') + .catch(() => undefined); + throw error; + } + // The pull request exists from here on: a labelling failure must not turn it into an error (and a retried duplicate). + let labelled = true; if (change.labels?.length) { - await github.request(change.repository, 'POST', `${repo}/issues/${pull.number}/labels`, 'add labels', { labels: change.labels }); + labelled = await github + .request(change.repository, 'POST', `${repo}/issues/${pull.number}/labels`, 'add labels', { labels: change.labels }) + .then(() => true, () => false); } - return { number: pull.number, url: pull.html_url }; + return { number: pull.number, url: pull.html_url, labelled }; } diff --git a/src/text.ts b/src/text.ts index afd6cd7..96adcf7 100644 --- a/src/text.ts +++ b/src/text.ts @@ -24,6 +24,22 @@ export function singleLine(value: string, maxLength = 200): string { return cleaned.length > maxLength ? `${cleaned.slice(0, maxLength - 1)}…` : cleaned; } +const JOINER = '\u2060'; + +/** + * Text that GitHub reads in titles and commit messages: a word joiner breaks @mentions, #123 / GH-123 / owner/repo#123 + * references, closing keywords and URLs, so reader text can neither notify people nor close or cross-link issues. + */ +export function neutralizeGitHubText(value: string): string { + return value + .replace(/@/g, `@${JOINER}`) + .replace(/#(?=\d)/g, `#${JOINER}`) + .replace(/\b(GH)-(?=\d)/gi, `$1-${JOINER}`) + .replace(/:\/\//g, `:${JOINER}//`) + .replace(/\bwww\./gi, (match) => `${match.slice(0, 3)}${JOINER}.`) + .replace(/\b(close[sd]?|fix(?:e[sd])?|resolve[sd]?)\b/gi, (word) => `${word.slice(0, 1)}${JOINER}${word.slice(1)}`); +} + const pad = (value: number, length = 2) => String(value).padStart(length, '0'); /**