From 8eae5632c0624aa875660d3c33f5e30b6483ae25 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:00:19 -0500 Subject: [PATCH 01/17] docs: plan file uploads on the submit page Co-Authored-By: Claude Opus 5.5 --- docs/submit-uploads-plan.md | 242 ++++++++++++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 docs/submit-uploads-plan.md diff --git a/docs/submit-uploads-plan.md b/docs/submit-uploads-plan.md new file mode 100644 index 0000000..157c9ab --- /dev/null +++ b/docs/submit-uploads-plan.md @@ -0,0 +1,242 @@ +# Attach files on the submit page + +**Status: planned (2026-10-16). Not started.** + +## Why + +A submitter can answer every question on `/submit/` except the ones that matter most to a +reader: the screenshots and the slide deck. GitHub prefills an issue form's text fields from a +link but never an `upload` control (`assets/js/submit/handoff.js:10-13`), so the page tells +people to open the issue and drag their files in afterwards (`assets/js/submit/review.js` +~L508-545, `new-entry.yml` ~L432). For the people this catalog is for — health department +staff, most of whom have never used GitHub — that second step is where submissions arrive +without screenshots, or with a SharePoint link to a deck nobody outside the department can +open. A deployment's first real entries had to work around it by hosting +their screenshots in their source repositories and pasting pinned `raw.githubusercontent.com` +links. + +## What it looks like when done + +A deployment that turns this on gets, for a **signed-in** submitter (the existing optional +sign-in, `services/github-signin/`): + +- a file picker on the screenshots question, with an alt-text box beside each image, and one on + each `file` question (the deck); +- each file uploads the moment it is picked, with progress, a remove button, and the image + re-encoded in the browser first (strips EXIF/GPS, caps the long edge); +- the draft remembers what was uploaded, so a reload or a return visit keeps the files; +- **Submit to GitHub** writes the issue body with `upload:` references in place of URLs; the + scaffolder resolves them, copies the files into the entry exactly as it copies attachments + today, and the draft PR shows them. + +Not signed in, or the feature not set up: the page is exactly what it is today. + +A second, independent piece: a **"Using an AI agent?"** link on `/submit/` to an instructions +file and a JSON Schema, both generated from `_data/schema.yml`, and an **Import answers** box +that loads the agent's JSON into the form and runs the form's own validation. + +## Hard constraints + +1. **The upload service stores nothing.** No Vercel Blob, KV or database; it validates a file + and writes it to GitHub in the same request. Files live only in a private GitHub repository + until the scaffolder copies them, then are deleted. +2. **Off by default, per deployment.** Each deployment runs its own service, GitHub App and + private repository (owner decision, 2026-10-16). Blank `submit.uploads_url` = no pickers, no + reference parsing, no new workflow steps doing anything. +3. **Signed-in submitters only, `new-entry` only, v1.** No path for submitters without a GitHub + account (owner decision, 2026-10-16); edit requests and refreshes keep today's flow. +4. **No hand-written manifest for humans.** Agents get generated instructions; people get pickers. + +## Design + +### Why sign-in is required + +Requiring the existing sign-in turns most of the hard problems an anonymous endpoint has into +easy ones: + +| Problem | Anonymous endpoint | Signed-in endpoint | +| --- | --- | --- | +| Abuse, quota exhaustion | CAPTCHA + signed session + IP rate limits | caps per GitHub user, checked against GitHub itself; WAF only as a backstop | +| Whose upload is this? | random id, anyone holding it can reference it | stored under the uploader's numeric GitHub id; the scaffolder only looks under the **issue author's** id | +| Popup blocker vs async upload | `window.open` must stay in the click (`submit.js:393`) | direct mode creates the issue by API, no popup | +| Deck field can't be prefilled | must regenerate `file` questions as textareas | direct mode writes the body itself (`issueFormBody`); the issue form is unchanged | + +The cost: a deployment must run the sign-in service before it can run uploads, and a submitter +signs in once (a popup, then cached for eight hours). + +### Components + +``` +browser (/submit/, signed in) + │ POST bytes + Authorization: Bearer + ▼ +services/uploads/ (own Vercel project; stateless) + │ 1. GET api.github.com/user with the submitter's token → numeric id, login + │ 2. check origin, kind, size, magic bytes, per-user caps + │ 3. with its own GitHub App installation token (uploads repo only, contents:rw): + │ blob → tree → parentless commit → ref refs/heads/u/// + ▼ +/-uploads (private GitHub repo, only ever holds refs under u/) + ▲ + │ scaffold job: read-only token for the uploads repo, minted per run +.github/workflows/new-entry.yml → scripts/lib/uploads.mjs (dedicated fetcher) +``` + +**A second GitHub App, not the sign-in App with more permissions.** Adding `Contents` to the +sign-in App would widen what every submitter token held in the browser can do (the sign-in +README's shared-`github.io`-origin warning). The uploads App has **Contents: read and write** +on the uploads repository only, its private key never leaves the service and the catalog's +Actions secrets, and it has no user-facing authorization. + +### The upload service (`services/uploads/`) + +Same shape as `services/github-signin/`: Node 22, zero runtime dependencies (RS256 JWT via +`node:crypto`), refuses every request when any env var is missing or malformed, `no-store`, +strict CORS from `ALLOWED_ORIGIN`. + +- `POST /api/upload` — raw body (`application/octet-stream`, never base64 JSON), headers + `X-Upload-Set` (client-generated, 128-bit, base32), `X-Upload-Kind` (`image` | `pdf`), + `X-Upload-Name` (display only). Responds `{ ref: "upload:/.", bytes, sha256 }`. +- `DELETE /api/upload?ref=…` — removes one ref, only under the caller's own user id. +- Env: `UPLOADS_APP_ID`, `UPLOADS_APP_PRIVATE_KEY`, `UPLOADS_REPO` (`owner/name`), + `ALLOWED_ORIGIN`, optional `MIN_ACCOUNT_AGE_DAYS` (default 0). + +Checks, in order: origin → bearer token resolves to a user (one `GET /user`; any valid token +identifies a real account, which is all the caps need) → account age → kind/extension/magic +bytes (`%PDF`; PNG/JPEG/WebP signatures; reuse the sniffers from `scripts/lib`) → size +(images ≤ 4 MB each after browser re-encode; PDF ≤ 4 MB; Vercel's 4.5 MB request cap applies +on every plan) → caps, by listing `refs/heads/u//` (≤ 3 open sets, ≤ 9 files per set) → +write. + +**One parentless commit and one ref per file.** Four writes per file (blob, tree, commit, +ref), no races between parallel uploads, and deleting the ref is the whole cleanup. The +alternative — blobs on pick, one commit on submit — saves writes but leaves dangling blobs if a +draft sits for days. GitHub's secondary limit (80 content-creating requests a minute, 500 an +hour, per installation) allows roughly fourteen maximum-size submissions an hour per +deployment; real volume is a few a week. On a GitHub 403/429 the service answers 429 with +`Retry-After` and the page retries. + +### References and the scaffolder + +The issue body carries, in the screenshots section: + +``` +upload:k3j9…/9f2c…e1.png | Landing page with a Get Started button +``` + +and in a `file` section, a bare `upload:k3j9…/77ab…04.pdf`. + +- `scripts/lib/issue_body.mjs`: `parseImageRefs` and `parseAttachmentRef` accept the + `upload:` form (regex-validated: set id and full sha256, extension from an allowlist) and + return `{ upload: { set, sha256, ext } }` instead of `{ url }`. +- `scripts/lib/uploads.mjs` (new, the **only** code that sees the uploads token): builds the + ref name itself — `u///` — so a reference can never + name another user's files or another repository; reads the blob; re-hashes and rejects a + mismatch; hands the bytes to the existing sniff, per-file and total caps. +- `scripts/lib/images.mjs` and `attachments.mjs`: split "fetch" from "check and save bytes" so + both the URL path and the upload path share the checks. The generic fetchers keep sending + only the job's own `GITHUB_TOKEN` (they attach it to any GitHub host a submitter names — + `images.mjs:282`, `attachments.mjs:208` — which is exactly why the uploads token must never + reach them). +- `scripts/new_entry_from_issue.mjs`: route `upload` refs to the new fetcher before + `shouldDownload` / `downloadImages`. A `file` upload is downloaded into the entry, never kept + as a link. +- **The scaffolder never deletes.** Every issue edit rebuilds the draft branch from main + (`new-entry.yml` triggers on `edited`; the scaffold step runs before the branch guard), so + the files must still be there. + +### Workflows + +- `new-entry.yml`, scaffold job: when `vars.UPLOADS_REPO` is set, a step mints a token with + `actions/create-github-app-token` (`UPLOADS_APP_ID` / `UPLOADS_APP_PRIVATE_KEY` secrets, + `repositories:` the uploads repo, `permission-contents: read`) and passes it to the scaffold + step only, as `UPLOADS_TOKEN`, with `UPLOADS_REPO` and `ISSUE_AUTHOR_ID` + (`github.event.issue.user.id`). Unset variable: the step is skipped, `upload:` lines are + reported back to the submitter as unresolvable. +- `uploads-cleanup.yml` (new): on `issues: closed` (the new-entry issue closes when its PR + merges or is declined) delete that author's sets referenced in the body; daily on a schedule, + delete every ref older than 14 days whose set is not referenced by an open `content:new-entry` + issue by the same author. The write token (`permission-contents: write`) is minted in this + job only. Deletion targets always come from the ref listing; issue text can only cause a set + to be **kept**. Follows the `metrics.yml` pattern for a clean "turned off" summary. + +### The submit page + +- `_data/site.yml`: `submit.uploads_url` (blank by default). `scripts/validate.mjs` rejects it + unless `submit.signin_url` is also set, mirroring the `signinUrlProblem` check. +- `submit/index.md`: `data-uploads-url` and a deferred `assets/js/submit/uploads.js`, both + emitted only when set (the `signin_url` pattern, L129-130 / L308-309 / L648-651). +- `uploads.js`: pickers appear only while signed in (`direct.js` exposes the token state); + re-encode images with a canvas (PNG/JPEG/WebP in, long edge ≤ 2400 px, GIF not accepted by + the picker); upload on pick, sequentially, with progress and retry; persist + `{ ref, name, alt, bytes }` per file in the existing draft (`draft.js`, key + `catalog-template:submit-draft:v2:`); write references into the body that + `issueFormBody` produces; remove → `DELETE`. Keyboard and screen-reader complete: native + ``, a live region for progress, alt text required before submit. +- Signing out, or an expired token, hides the pickers but keeps the references in the draft; + signing back in as the same user restores them, as a different user drops them (they would + not resolve). +- Copy changes: `review.js` and `new-entry.yml`'s `draft_ready` details stop telling signed-in + submitters with uploads to drag files in. + +### "Using an AI agent?" + +- `submit/agent-instructions.md` (Liquid, served as plain text) and + `submit/answers.schema.json` (Liquid, the `entries.json` pattern), both built from + `_data/schema.yml` so the allowed options cannot drift. +- The instructions cover: exact option strings; **one item per line** for list fields (the + scaffolder splits on newlines and commas, `issue_body.mjs` `parseList`); alt text limits; + screenshot size and a no-personal-information check; output JSON matching the schema. It + states that the file is reference data. +- An **Import answers** box on `/submit/` (`assets/js/submit/import.js`) parses the JSON, fills + the form, and runs `validateAll`. Files still go through the picker (or GitHub, for people + not signed in), so the page stays the single place where answers and files are checked. + +### Security notes to publish + +- The private repository protects abandoned uploads and stops the service being an anonymous + file host. It does **not** keep screenshots private: once scaffolded they are on a public + `entry/*` branch before anyone reviews them, as GitHub attachments are today. Say so in + `SECURITY.md` and on the picker; keep the no-personal-information attestation. +- The submitter's sign-in token is now sent to a second service the deployment runs, not only + to `api.github.com`. Update the sign-in README's statement and explain why. +- Git keeps deleted content in history; one parentless commit per ref means deleting the ref + leaves it unreachable, but purging something sensitive immediately still needs GitHub + support. Document the procedure in the admin guide. + +## Work plan + +Workstreams own disjoint files. 4 and 5 share `submit/index.md`, so 5 starts after 4. + +| # | Workstream | Owns | Notes | +| --- | --- | --- | --- | +| 1 | Upload service | `services/uploads/**` | Security-critical. Unit tests with a fetch stub: origin, token, caps, magic bytes, size, ref naming, 429 path. | +| 2 | Scaffolder | `scripts/lib/{uploads,images,attachments,issue_body}.mjs`, `scripts/new_entry_from_issue.mjs`, their tests | Security-critical. Tests: another user's ref never resolves, sha mismatch rejected, uploads token never passed to generic fetchers, edit rebuild resolves again, feature off ⇒ unchanged behaviour. | +| 3 | Workflows | `.github/workflows/{new-entry,uploads-cleanup}.yml` | actionlint; `test/helpers/workflow_expr.mjs` tests for the skip conditions; cleanup never deletes a referenced open set. | +| 4 | Submit page | `_data/site.yml`, `scripts/validate.mjs` (+ test), `submit/index.md`, `assets/js/submit/{uploads,direct,draft,review}.js` | JSDOM tests on the `submit-form.html` fixture; a11y flow test. | +| 5 | Agent path | `submit/agent-instructions.md`, `submit/answers.schema.json`, `assets/js/submit/import.js` | Schema JSON validates every shipped preset's sample answers; import runs `validateAll`. | +| 6 | Docs | `services/uploads/README.md`, `services/github-signin/README.md`, `docs/{configuration,admin-guide,decisions}.md`, `SECURITY.md`, `CHANGELOG.md` | Setup steps mirror the sign-in README. Decision entry: why sign-in is required and why a second App. | + +**Gates:** `npm test`, `npm run test:ruby`, `npm run lint`, `npm run validate`, +`npm run test:build`, `npm run test:flows`, the services' own tests, actionlint; an end-to-end +run on a throwaway deployment (real App, real private repo) before release. + +## Rolling out to a deployment + +1. Release in the template; the deployment takes it through its `update-phct` PR. +2. Deploy `services/github-signin` (if not already) and `services/uploads` as two Vercel projects. +3. In the account that owns the catalog: create the private `-uploads` repository and + both GitHub Apps, each owned by that account and installed on its one repository. +4. Catalog repository: secrets `UPLOADS_APP_ID`, `UPLOADS_APP_PRIVATE_KEY`; variable + `UPLOADS_REPO`; `submit.signin_url` and `submit.uploads_url` in `_data/site.yml`. +5. Add a Vercel WAF rate-limit rule on `/api/upload` as a backstop. +6. Submit a test entry with a deck and three screenshots; edit the issue; close it; confirm the + refs are gone. + +## Deferred + +- Decks over 4 MB (chunked upload). Until then the deck question keeps its link answer and the + picker suggests "export as PDF, minimum size". +- Submitting without a GitHub account (the service opening the issue). +- Uploads on edit requests and refreshes. +- Agents calling the upload service directly. From 4e22de354a442e36c37dc3415e39c407a7ff88f0 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:05:28 -0500 Subject: [PATCH 02/17] feat(uploads): add the upload: reference contract Co-Authored-By: Claude Opus 5.5 --- docs/submit-uploads-plan.md | 6 +-- scripts/lib/upload_refs.mjs | 80 +++++++++++++++++++++++++++++++ test/scripts/upload_refs.test.mjs | 56 ++++++++++++++++++++++ 3 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 scripts/lib/upload_refs.mjs create mode 100644 test/scripts/upload_refs.test.mjs diff --git a/docs/submit-uploads-plan.md b/docs/submit-uploads-plan.md index 157c9ab..dc04d8a 100644 --- a/docs/submit-uploads-plan.md +++ b/docs/submit-uploads-plan.md @@ -1,6 +1,6 @@ # Attach files on the submit page -**Status: planned (2026-10-16). Not started.** +**Status: in progress (started 2026-10-16).** ## Why @@ -74,7 +74,7 @@ services/uploads/ (own Vercel project; stateless) │ 1. GET api.github.com/user with the submitter's token → numeric id, login │ 2. check origin, kind, size, magic bytes, per-user caps │ 3. with its own GitHub App installation token (uploads repo only, contents:rw): - │ blob → tree → parentless commit → ref refs/heads/u/// + │ blob → tree → parentless commit → ref refs/heads/u/// ▼ /-uploads (private GitHub repo, only ever holds refs under u/) ▲ @@ -130,7 +130,7 @@ and in a `file` section, a bare `upload:k3j9…/77ab…04.pdf`. `upload:` form (regex-validated: set id and full sha256, extension from an allowlist) and return `{ upload: { set, sha256, ext } }` instead of `{ url }`. - `scripts/lib/uploads.mjs` (new, the **only** code that sees the uploads token): builds the - ref name itself — `u///` — so a reference can never + ref name itself — `u///` (`scripts/lib/upload_refs.mjs`, the shared contract) — so a reference can never name another user's files or another repository; reads the blob; re-hashes and rejects a mismatch; hands the bytes to the existing sniff, per-file and total caps. - `scripts/lib/images.mjs` and `attachments.mjs`: split "fetch" from "check and save bytes" so diff --git a/scripts/lib/upload_refs.mjs b/scripts/lib/upload_refs.mjs new file mode 100644 index 0000000..8a963c1 --- /dev/null +++ b/scripts/lib/upload_refs.mjs @@ -0,0 +1,80 @@ +/** + * The `upload:` reference: how a file attached on /submit/ through the optional + * upload service (services/uploads/) is named in an issue body, and where that + * file lives in the deployment's private uploads repository. See + * docs/submit-uploads-plan.md. + * + * upload:/. + * + * - `set`: 26 characters of lowercase base32 (128 random bits) the submit page + * picks once per draft. + * - `sha256`: the file's full SHA-256, lowercase hex. The scaffolder re-hashes + * the bytes it reads and rejects a mismatch. + * - `ext`: one of UPLOAD_EXTENSIONS. + * + * The reference never names a user or a repository. The ref it resolves to is + * built from the **issue author's** numeric GitHub id, so a reference copied + * into someone else's issue resolves to nothing: + * + * refs/heads/u/// + * + * services/uploads/ keeps its own copy of these rules, because it deploys from + * its own directory; a parity test keeps the two in step. + */ + +/** Extension → the kind the upload service is asked for. */ +export const UPLOAD_EXTENSIONS = Object.freeze({ png: 'image', jpg: 'image', webp: 'image', pdf: 'pdf' }); + +export const SET_ID_RE = /^[a-z2-7]{26}$/; +export const SHA256_RE = /^[0-9a-f]{64}$/; + +const REF_RE = /^upload:([a-z2-7]{26})\/([0-9a-f]{64})\.(png|jpg|webp|pdf)$/; + +/** + * @typedef {{ set: string, sha256: string, ext: 'png'|'jpg'|'webp'|'pdf' }} UploadRef + */ + +/** + * Parse one reference. Surrounding whitespace is ignored; anything else that is + * not exactly the form above is not a reference. + * + * @param {unknown} text + * @returns {UploadRef|null} + */ +export function parseUploadRef(text) { + const match = REF_RE.exec(String(text ?? '').trim()); + if (!match) return null; + return { set: match[1], sha256: match[2], ext: /** @type {UploadRef['ext']} */ (match[3]) }; +} + +/** + * @param {UploadRef} ref + * @returns {string} `upload:/.` + */ +export function formatUploadRef({ set, sha256, ext }) { + return `upload:${set}/${sha256}.${ext}`; +} + +/** + * The branch, without `refs/heads/`, that holds one uploaded file. + * + * @param {number|string} userId the uploader's (and so the issue author's) numeric GitHub id + * @param {UploadRef} ref + * @returns {string} `u///` + */ +export function uploadBranch(userId, { set, sha256 }) { + const id = String(userId ?? '').trim(); + if (!/^[1-9][0-9]*$/.test(id)) throw new Error(`not a GitHub user id: ${JSON.stringify(userId)}`); + if (!SET_ID_RE.test(set) || !SHA256_RE.test(sha256)) throw new Error('not an upload reference'); + return `u/${id}/${set}/${sha256}`; +} + +/** + * The file's name inside that branch's single-file tree. + * + * @param {UploadRef} ref + * @returns {string} `.` + */ +export function uploadFilename({ sha256, ext }) { + return `${sha256}.${ext}`; +} diff --git a/test/scripts/upload_refs.test.mjs b/test/scripts/upload_refs.test.mjs new file mode 100644 index 0000000..2a55d8d --- /dev/null +++ b/test/scripts/upload_refs.test.mjs @@ -0,0 +1,56 @@ +/** + * The `upload:` reference contract (scripts/lib/upload_refs.mjs) that the + * submit page writes, the upload service stores under, and the scaffolder and + * the cleanup workflow resolve. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { + formatUploadRef, + parseUploadRef, + uploadBranch, + uploadFilename, + UPLOAD_EXTENSIONS, +} from '../../scripts/lib/upload_refs.mjs'; + +const SET = 'abcdefghijklmnopqrstuvwxyz'.slice(0, 20) + '234567'; +const SHA = 'a'.repeat(64); + +test('a well-formed reference round-trips', () => { + for (const ext of Object.keys(UPLOAD_EXTENSIONS)) { + const text = `upload:${SET}/${SHA}.${ext}`; + const ref = parseUploadRef(` ${text} `); + assert.deepEqual(ref, { set: SET, sha256: SHA, ext }); + assert.equal(formatUploadRef(ref), text); + } +}); + +test('anything else is not a reference', () => { + for (const text of [ + '', + null, + `upload:${SET}/${SHA}.gif`, + `upload:${SET}/${SHA}.PNG`, + `upload:${SET.toUpperCase()}/${SHA}.png`, + `upload:${SET}/${SHA.slice(1)}.png`, + `upload:${SET}1/${SHA}.png`, + `upload:../${SHA}.png`, + `upload:${SET}/${SHA}.png | alt text`, + `https://example.org/upload:${SET}/${SHA}.png`, + `upload:${SET}/${SHA}.png\nupload:${SET}/${SHA}.png`, + ]) { + assert.equal(parseUploadRef(text), null, JSON.stringify(text)); + } +}); + +test('the branch is built from the user id, never from the reference', () => { + const ref = parseUploadRef(`upload:${SET}/${SHA}.pdf`); + assert.equal(uploadBranch(583231, ref), `u/583231/${SET}/${SHA}`); + assert.equal(uploadBranch('583231', ref), `u/583231/${SET}/${SHA}`); + assert.equal(uploadFilename(ref), `${SHA}.pdf`); + for (const bad of ['', '0', '-1', '12a', 'octocat', '1/2', undefined]) { + assert.throws(() => uploadBranch(bad, ref), /not a GitHub user id/, String(bad)); + } + assert.throws(() => uploadBranch(1, { set: '../x', sha256: SHA }), /not an upload reference/); +}); From 450e047f7bfad3921a1b9ae640ed94a13010b4ff Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:19:51 -0500 Subject: [PATCH 03/17] feat(uploads): add the stateless upload service services/uploads/ validates a signed-in submitter's screenshot or PDF and writes it straight to the deployment's private uploads repository through its own GitHub App, one parentless commit and ref per file. Stores nothing. Co-Authored-By: Claude Opus 5.5 --- scripts/coverage.mjs | 11 + services/uploads/README.md | 267 ++++++++++ services/uploads/api/upload.mjs | 41 ++ services/uploads/lib/config.mjs | 142 +++++ services/uploads/lib/github.mjs | 221 ++++++++ services/uploads/lib/refs.mjs | 67 +++ services/uploads/lib/sniff.mjs | 35 ++ services/uploads/lib/store.mjs | 119 +++++ services/uploads/lib/upload.mjs | 383 ++++++++++++++ services/uploads/package.json | 11 + services/uploads/test/config.test.mjs | 113 ++++ services/uploads/test/github.test.mjs | 165 ++++++ services/uploads/test/helpers.mjs | 140 +++++ services/uploads/test/upload.test.mjs | 644 +++++++++++++++++++++++ test/scripts/coverage.test.mjs | 2 +- test/scripts/upload_refs_parity.test.mjs | 99 ++++ 16 files changed, 2459 insertions(+), 1 deletion(-) create mode 100644 services/uploads/README.md create mode 100644 services/uploads/api/upload.mjs create mode 100644 services/uploads/lib/config.mjs create mode 100644 services/uploads/lib/github.mjs create mode 100644 services/uploads/lib/refs.mjs create mode 100644 services/uploads/lib/sniff.mjs create mode 100644 services/uploads/lib/store.mjs create mode 100644 services/uploads/lib/upload.mjs create mode 100644 services/uploads/package.json create mode 100644 services/uploads/test/config.test.mjs create mode 100644 services/uploads/test/github.test.mjs create mode 100644 services/uploads/test/helpers.mjs create mode 100644 services/uploads/test/upload.test.mjs create mode 100644 test/scripts/upload_refs_parity.test.mjs diff --git a/scripts/coverage.mjs b/scripts/coverage.mjs index 64728d4..1e963da 100644 --- a/scripts/coverage.mjs +++ b/scripts/coverage.mjs @@ -25,6 +25,17 @@ export const COVERAGE_GROUPS = [ includes: ['services/github-signin/api/**/*.mjs', 'services/github-signin/lib/**/*.mjs'], tests: ['services/github-signin/test/signin.test.mjs'], }, + { + id: 'uploads-service', + label: 'The optional upload service, which handles user tokens, the uploads App key and file writes', + thresholds: { lines: 95, branches: 90, functions: 95 }, + includes: ['services/uploads/api/**/*.mjs', 'services/uploads/lib/**/*.mjs'], + tests: [ + 'services/uploads/test/config.test.mjs', + 'services/uploads/test/github.test.mjs', + 'services/uploads/test/upload.test.mjs', + ], + }, { id: 'security-parsers', label: 'Security-sensitive issue, YAML, image, attachment, and advisory parsers', diff --git a/services/uploads/README.md b/services/uploads/README.md new file mode 100644 index 0000000..f002201 --- /dev/null +++ b/services/uploads/README.md @@ -0,0 +1,267 @@ +# Upload service (optional) + +A small service that lets a **signed-in** submitter attach screenshots and a +PDF slide deck **on your catalog's `/submit/` page**, instead of opening the +issue afterwards and dragging the files in. Each file uploads the moment it is +picked, and the issue the page writes refers to it with an `upload:` line. When +the submission is scaffolded into a draft pull request, the automation copies +the files into the entry, exactly as it copies GitHub attachments today. + +You do not need this service. Leave `submit.uploads_url` blank in +`_data/site.yml` and the submit page works exactly as before: submitters add +their files on GitHub after they submit. + +**It needs the sign-in service.** Only signed-in submitters can upload, so set +up [`services/github-signin`](../github-signin/README.md) first and check that +signing in works. + +## How it works + +1. A signed-in submitter picks a screenshot or a PDF on `/submit/`. The page + shrinks the image in the browser (which also strips location data), then + sends the file to this service's `/api/upload`, with the submitter's GitHub + sign-in token. +2. The service asks GitHub who the token belongs to (one `GET /user` call), and + checks the file: it must come from your site, be at most 4 MB, and really be + a PNG, JPEG or WebP image or a PDF, judged by its first bytes rather than its + name. +3. With its **own** GitHub App, the service writes the file to your private + uploads repository, filed under the submitter's numeric GitHub id, and + answers with a reference such as `upload:k3j9…/9f2c…e1.png`. +4. When the submitter presses **Submit to GitHub**, the issue carries those + references. The scaffold workflow looks them up **under the issue author's + id only**, so a reference copied into someone else's issue finds nothing. +5. When the issue closes (the entry is published or declined), a cleanup + workflow deletes that submitter's files; a daily run deletes anything left + over for more than 14 days that no open submission still needs. + +The service stores nothing itself: no database, no file storage, nothing on +disk. Each file goes straight from the request to GitHub. It has no runtime +dependencies. + +- The submitter's token is used for that one `GET /user` call and nothing + else. It is never logged, stored or passed on. +- Each file is its own branch, `u///`, holding a single + commit with just that file, so deleting the branch is the whole cleanup. +- Uploading the same file twice is harmless: the second upload is answered + without writing anything. +- Every response is `Cache-Control: no-store` and `Referrer-Policy: + no-referrer`, and only the origins in `ALLOWED_ORIGIN` may call it. +- If any environment variable is missing or malformed, every upload and every + removal is refused. + +### What the private repository does and does not protect + +The uploads repository is private, so files from a draft that was never +submitted, or a submission that was declined, are never public, and nobody +can use the service to host files for the world to download. + +It does **not** keep screenshots private. Once a submission is scaffolded, its +files are committed to a **public draft branch** in the catalog repository, +before anyone has reviewed them, just as files dragged into a GitHub issue are +public the moment they are attached. Tell submitters not to upload anything +with personal information in it; the submit page says so too. + +Git keeps deleted content in its history. Deleting an upload's branch makes the +file unreachable, but if something sensitive must be gone at once, contact +GitHub Support to have it purged. + +### The sign-in token is shared with this service + +Without uploads, a submitter's sign-in token is sent only to `api.github.com`. +With uploads, the page also sends it to this service, which you run, so the +service can tell who is uploading and hold each person to their own limits. +The service uses the token for one `GET /user` call and nothing else. The +token can only do what the sign-in App allows (read and write issues in the +catalog repository); it cannot touch the uploads repository at all. + +## Set it up + +You need the sign-in service already working, a GitHub account that can create +a repository and a GitHub App for the account (user or organization) that owns +the catalog repository, and a Vercel account. The Vercel CLI is used through +`npx`, so nothing is installed globally. + +### 1. Create the private uploads repository + +In the account that owns the catalog, create a new repository named +`-uploads` (for example `catalog-uploads`): + +- Visibility: **Private**. +- Tick **Add a README file**, so the repository starts with one commit. The + service cannot write to an empty repository. + +Nothing else goes in it. The service and the workflows only ever create and +delete branches under `u/`. + +### 2. Register the uploads GitHub App + +This is a **second** GitHub App, separate from the sign-in App. Giving the +sign-in App access to files would widen what every submitter's token in the +browser can do; this App's key never leaves the service and the catalog's +Actions secrets. + +On GitHub go to **Settings → Developer settings → GitHub Apps → New GitHub +App**. For an organization, go to the organization's settings instead, so the +app belongs to the organization. + +| Setting | Value | +| --- | --- | +| GitHub App name | anything, e.g. " uploads". Submitters never see it. | +| Homepage URL | your catalog's address | +| Callback URL | leave blank | +| Request user authorization (OAuth) during installation | Off | +| Enable Device Flow | Off | +| Webhook | Untick **Active**. The service does not use webhooks. | +| Repository permissions → **Contents** | **Read and write**. Metadata (read-only) is added automatically. | +| Every other permission | No access | +| Where can this GitHub App be installed? | Only on this account | + +Create the app, then: + +1. Note the **App ID** (a number, near the top of the app's page). +2. Under **Private keys**, choose **Generate a private key**. Your browser + downloads a `.pem` file. Keep it safe; you need it twice below, then you can + delete the download. +3. Choose **Install App** and install it on the account that owns the catalog, + for **Only select repositories** → the uploads repository only. Not the + catalog repository. + +### 3. Deploy and configure the service + +From this directory: + +```sh +cd services/uploads +npx vercel link # create (or pick) a Vercel project for this directory +npx vercel env add UPLOADS_APP_ID production # the App ID +npx vercel env add UPLOADS_APP_PRIVATE_KEY production # paste the whole .pem file +npx vercel env add UPLOADS_REPO production # owner/name, e.g. example/catalog-uploads +npx vercel env add ALLOWED_ORIGIN production # your site's origin, e.g. https://catalog.example.org +npx vercel deploy --prod # gives the address, e.g. https://catalog-uploads.vercel.app +``` + +- `UPLOADS_APP_PRIVATE_KEY`: paste the whole file, including the + `-----BEGIN RSA PRIVATE KEY-----` and `-----END RSA PRIVATE KEY-----` lines. + A key pasted on one line with `\n` in place of the line breaks also works. +- `ALLOWED_ORIGIN`: the same value as the sign-in service's: your catalog's + scheme and host only, no path and no trailing slash. Separate several with + commas. Only `https://` origins are accepted, plus `http://localhost` and + other loopback addresses for local testing. +- Optional: `MIN_ACCOUNT_AGE_DAYS`, a whole number of days. GitHub accounts + younger than that cannot upload (they can still submit and add files on + GitHub). Leave it unset to allow any account. + +### 4. Give the catalog's workflows access + +In the **catalog** repository, under **Settings → Secrets and variables → +Actions**: + +- Secrets: `UPLOADS_APP_ID` (the App ID) and `UPLOADS_APP_PRIVATE_KEY` (the + whole `.pem` file). +- Variables: `UPLOADS_REPO`, the same `owner/name` as above. + +The scaffold workflow mints a read-only token for the uploads repository from +these for each run; the cleanup workflow mints one that can delete. Then delete +the downloaded `.pem` file. + +### 5. Turn it on in the catalog + +In `_data/site.yml`: + +```yaml +submit: + signin_url: "https://catalog-signin.vercel.app" + uploads_url: "https://catalog-uploads.vercel.app" +``` + +Use the service's address only, with no path. `npm run validate` checks it, +and refuses `uploads_url` without `signin_url`. + +### 6. Add a rate-limit rule as a backstop + +The service holds each GitHub account to its own limits, but anyone can make +up GitHub accounts. In the Vercel project, under **Firewall**, add a rate-limit +rule on the path `/api/upload`, for example 60 requests per minute per IP +address. Normal use is far below that. + +### 7. Try it + +Publish the change, open `/submit/`, sign in, and submit a test entry with a +deck and three screenshots. Check the draft pull request shows the files, edit +the issue and check they are still there, then close the issue and check the +uploads repository has no branches left under `u/`. + +## Limits + +- **4 MB per file**, for screenshots and decks alike (Vercel accepts at most + 4.5 MB per request on every plan). Screenshots are shrunk in the browser + first, so they rarely come close. A bigger deck has to be exported smaller + ("export as PDF, minimum size") or linked instead. +- **3 drafts per person** with files at once. A fourth is refused with a + message asking the submitter to finish or remove the files of an earlier + draft. +- **9 files per draft.** +- **GitHub's own limit:** an App may make about 80 content-creating requests a + minute and 500 an hour. Each file takes four, so that is roughly fourteen + submissions an hour with a deck and eight screenshots each, for the whole + catalog. When GitHub says to slow down, the service answers `429` with a + `Retry-After` time and the page tries again. + +## For the submit page + +`OPTIONS /api/upload` answers the CORS preflight for the allowed origins. + +`POST /api/upload`, with the file as the raw body: + +| Header | Value | +| --- | --- | +| `Authorization` | `Bearer ` | +| `Content-Type` | `application/octet-stream` | +| `X-Upload-Set` | the draft's id: 26 characters of `a`–`z` and `2`–`7` | +| `X-Upload-Kind` | `image` or `pdf` | +| `X-Upload-Name` | optional, up to 200 characters, for display only; never stored | + +`201` answers `{"ref":"upload:/.","sha256":"…","bytes":n,"ext":"png|jpg|webp|pdf"}`. + +`DELETE /api/upload?ref=upload:/.` removes one of the +caller's own files and answers `204`, also when it was already gone. + +Errors answer `{"error":"","message":""}`: + +| Status | `error` | +| --- | --- | +| 400 | `bad_set`, `bad_kind`, `bad_name`, `bad_length`, `bad_ref`, `empty_file`, `interrupted` | +| 401 | `unauthorized`: sign in again | +| 403 | `origin_not_allowed`, `account_too_new` | +| 405 | `method_not_allowed` | +| 413 | `too_large` | +| 415 | `wrong_content_type`, `not_an_image`, `not_a_pdf` | +| 429 | `rate_limited` (retry after `Retry-After` seconds); `too_many_drafts`, `too_many_files` (a limit, not worth retrying until the submitter removes files) | +| 500 | `not_configured` | +| 503 | `github_unavailable` | + +## Develop and test + +```sh +node --test "services/uploads/test/*.test.mjs" test/scripts/upload_refs_parity.test.mjs +``` + +The tests call the handler directly with a fake GitHub, so they never touch the +network. `lib/refs.mjs` is a copy of `scripts/lib/upload_refs.mjs`, because +this service deploys from its own directory; the parity test fails if the two +drift apart. The repository's `npm test` and `npm run lint` include this +directory. + +## Rotate or remove + +- **Rotate the App's private key:** generate a new key on the app, then update + `UPLOADS_APP_PRIVATE_KEY` in both places: `npx vercel env rm + UPLOADS_APP_PRIVATE_KEY production`, `npx vercel env add + UPLOADS_APP_PRIVATE_KEY production`, `npx vercel deploy --prod`, and the + catalog repository's secret. After that, delete the old key on GitHub. +- **Turn it off:** blank `submit.uploads_url` and publish. Submissions already + open keep their `upload:` lines; leave the App installed until they are + scaffolded and closed, or ask those submitters to attach the files on GitHub. + Then uninstall or delete the uploads App, delete the uploads repository and + the catalog's `UPLOADS_*` secrets and variable, and remove the Vercel project. diff --git a/services/uploads/api/upload.mjs b/services/uploads/api/upload.mjs new file mode 100644 index 0000000..c3fdf67 --- /dev/null +++ b/services/uploads/api/upload.mjs @@ -0,0 +1,41 @@ +/** + * /api/upload — take one screenshot or PDF from a signed-in submitter on a + * PHCT /submit/ page and store it in the private uploads repository; or remove + * one. See lib/upload.mjs for the whole flow. + * + * Every method routes to the same handler, so an unsupported one gets this + * service's own JSON 405 (with CORS headers) rather than the platform's. + */ +import process from 'node:process'; + +import { handleUpload } from '../lib/upload.mjs'; + +/** @param {Request} request @returns {Promise} */ +export function OPTIONS(request) { + return handleUpload(request, process.env); +} + +/** @param {Request} request @returns {Promise} */ +export function POST(request) { + return handleUpload(request, process.env); +} + +/** @param {Request} request @returns {Promise} */ +export function DELETE(request) { + return handleUpload(request, process.env); +} + +/** @param {Request} request @returns {Promise} */ +export function GET(request) { + return handleUpload(request, process.env); +} + +/** @param {Request} request @returns {Promise} */ +export function PUT(request) { + return handleUpload(request, process.env); +} + +/** @param {Request} request @returns {Promise} */ +export function PATCH(request) { + return handleUpload(request, process.env); +} diff --git a/services/uploads/lib/config.mjs b/services/uploads/lib/config.mjs new file mode 100644 index 0000000..66fbf55 --- /dev/null +++ b/services/uploads/lib/config.mjs @@ -0,0 +1,142 @@ +/** + * The upload service's configuration, read from the environment on every + * request. It fails closed: anything missing or malformed is an error, and the + * handlers then refuse every upload and every delete. + * + * UPLOADS_APP_ID the uploads GitHub App's numeric App ID + * UPLOADS_APP_PRIVATE_KEY that App's private key (PEM). Literal `\n` + * escapes are accepted, as Vercel stores them. + * UPLOADS_REPO the private uploads repository, `owner/name` + * ALLOWED_ORIGIN the catalog site origin(s), as for the sign-in + * service + * MIN_ACCOUNT_AGE_DAYS optional; GitHub accounts younger than this many + * days may not upload (default 0: any account) + */ + +import { createPrivateKey } from 'node:crypto'; + +const REQUIRED_ENV = ['UPLOADS_APP_ID', 'UPLOADS_APP_PRIVATE_KEY', 'UPLOADS_REPO', 'ALLOWED_ORIGIN']; + +/** A GitHub App ID is a positive integer. */ +const APP_ID_PATTERN = /^[1-9][0-9]{0,11}$/; +/** GitHub account names: letters, digits and single hyphens, at most 39 characters. */ +const OWNER_PATTERN = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/; +/** Repository names: letters, digits, `.`, `_`, `-`, at most 100 characters. */ +const REPO_NAME_PATTERN = /^[A-Za-z0-9._-]{1,100}$/; +/** Up to ten years, in whole days. */ +const DAYS_PATTERN = /^[0-9]{1,4}$/; + +const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]']); + +/** + * The site origins allowed to call this service: a comma-separated list, each + * an exact origin (`https://example.github.io`, no path and no trailing slash). + * HTTPS only, except a loopback address for local testing. The same rules as + * services/github-signin/lib/signin.mjs, copied because each service deploys + * from its own directory. + * @param {unknown} value + * @returns {{origins: string[], error: string}} + */ +export function parseAllowedOrigins(value) { + const entries = String(value ?? '') + .split(',') + .map((entry) => entry.trim()) + .filter(Boolean); + if (entries.length === 0) return { origins: [], error: 'ALLOWED_ORIGIN is empty.' }; + const origins = []; + for (const entry of entries) { + let url; + try { + url = new URL(entry); + } catch { + return { origins: [], error: `ALLOWED_ORIGIN entry "${entry}" is not a URL.` }; + } + if (url.origin !== entry) { + return { + origins: [], + error: `ALLOWED_ORIGIN entry "${entry}" must be an origin only, like ${url.origin}: no path, query or trailing slash.`, + }; + } + const local = url.protocol === 'http:' && LOOPBACK_HOSTS.has(url.hostname); + if (url.protocol !== 'https:' && !local) { + return { origins: [], error: `ALLOWED_ORIGIN entry "${entry}" must use https://.` }; + } + origins.push(entry); + } + return { origins, error: '' }; +} + +/** + * Turn the stored private key back into PEM text: Vercel (and many copy-paste + * paths) keep it on one line with literal `\n` sequences. + * @param {string} value + * @returns {string} + */ +export function normalizePem(value) { + return String(value).replace(/\\r/g, '').replace(/\\n/g, '\n').replace(/\r/g, '').trim(); +} + +/** + * @typedef {object} UploadsConfig + * @property {string} appId + * @property {import('node:crypto').KeyObject|null} privateKey + * @property {string} owner + * @property {string} repo + * @property {string[]} origins + * @property {number} minAccountAgeDays + * @property {string} error '' when the configuration is usable + */ + +/** + * Read and check the configuration. + * @param {Record} env + * @returns {UploadsConfig} + */ +export function readConfig(env) { + const source = env ?? {}; + const empty = { appId: '', privateKey: null, owner: '', repo: '', origins: [], minAccountAgeDays: 0 }; + const missing = REQUIRED_ENV.filter((name) => !String(source[name] ?? '').trim()); + if (missing.length > 0) { + return { + ...empty, + error: `Missing environment variable${missing.length > 1 ? 's' : ''}: ${missing.join(', ')}.`, + }; + } + + const appId = String(source.UPLOADS_APP_ID).trim(); + if (!APP_ID_PATTERN.test(appId)) { + return { ...empty, error: 'UPLOADS_APP_ID is not a GitHub App ID (a number).' }; + } + + let privateKey; + try { + privateKey = createPrivateKey(normalizePem(source.UPLOADS_APP_PRIVATE_KEY)); + } catch { + // Never echo any part of the key. + return { ...empty, error: 'UPLOADS_APP_PRIVATE_KEY is not a PEM private key.' }; + } + if (privateKey.asymmetricKeyType !== 'rsa') { + return { ...empty, error: 'UPLOADS_APP_PRIVATE_KEY is not an RSA key (GitHub App keys are RSA).' }; + } + + const [owner, repo, extra] = String(source.UPLOADS_REPO).trim().split('/'); + if ( + extra !== undefined || + !OWNER_PATTERN.test(owner ?? '') || + !REPO_NAME_PATTERN.test(repo ?? '') || + repo === '.' || + repo === '..' + ) { + return { ...empty, error: 'UPLOADS_REPO must be owner/name, like example/catalog-uploads.' }; + } + + const { origins, error } = parseAllowedOrigins(source.ALLOWED_ORIGIN); + if (error) return { ...empty, error }; + + const days = String(source.MIN_ACCOUNT_AGE_DAYS ?? '').trim(); + if (days !== '' && !DAYS_PATTERN.test(days)) { + return { ...empty, error: 'MIN_ACCOUNT_AGE_DAYS must be a whole number of days, or blank.' }; + } + + return { appId, privateKey, owner, repo, origins, minAccountAgeDays: Number(days || 0), error: '' }; +} diff --git a/services/uploads/lib/github.mjs b/services/uploads/lib/github.mjs new file mode 100644 index 0000000..0ca01a9 --- /dev/null +++ b/services/uploads/lib/github.mjs @@ -0,0 +1,221 @@ +/** + * The GitHub REST calls the upload service makes, and the uploads GitHub App's + * authentication, with no dependencies. + * + * Two kinds of token pass through here and they never mix: + * + * - the **submitter's** user token, used for exactly one call, `GET /user`, to + * learn who is uploading; + * - the **App installation** token, minted from the App's private key (an + * RS256 JWT signed with node:crypto), scoped to the uploads repository with + * `contents: write`, and kept in module scope until five minutes before it + * expires, so a warm instance mints one about once an hour. + * + * Neither token is ever logged; log lines carry HTTP statuses only. + */ + +import { sign } from 'node:crypto'; + +export const API = 'https://api.github.com'; +export const API_VERSION = '2022-11-28'; +export const USER_AGENT = 'phct-uploads'; + +/** How long to wait for any one GitHub call. */ +export const GITHUB_TIMEOUT_MS = 10000; + +/** Refresh the installation token this long before GitHub says it expires. */ +export const TOKEN_REFRESH_MARGIN_MS = 5 * 60 * 1000; + +/** When GitHub rate-limits us without saying for how long. */ +const DEFAULT_RETRY_AFTER = 60; +const MAX_RETRY_AFTER = 3600; + +/** GitHub user and installation tokens: an opaque `gh?_…`/`github_pat_…` string. */ +export const TOKEN_PATTERN = /^[A-Za-z0-9_]{20,512}$/; + +/** + * A GitHub call that did not succeed. + * - `unauthorized`: 401, the token is not (or no longer) valid + * - `rate_limited`: a primary or secondary rate limit; `retryAfter` seconds + * - `not_found`: 404 + * - `unprocessable`: 422; `message` holds GitHub's own message + * - `failed`: anything else, including a network failure or a timeout + */ +export class GitHubError extends Error { + /** + * @param {'unauthorized'|'rate_limited'|'not_found'|'unprocessable'|'failed'} kind + * @param {number} status 0 when GitHub was not reached + * @param {{retryAfter?: number, message?: string}} [extra] + */ + constructor(kind, status, { retryAfter = 0, message = '' } = {}) { + super(`GitHub ${kind} (HTTP ${status})`); + this.name = 'GitHubError'; + this.kind = kind; + this.status = status; + this.retryAfter = retryAfter; + this.githubMessage = message; + } +} + +/** + * How many seconds GitHub asks us to wait, when a 403/429 is a rate limit; + * 0 when it is not one. GitHub signals a secondary limit with `retry-after` + * or with "secondary rate limit" in the message, and a primary limit with + * `x-ratelimit-remaining: 0` and `x-ratelimit-reset`. + * @param {Response} response + * @param {string} message GitHub's error message, '' if none + * @param {number} nowMs + * @returns {number} + */ +export function rateLimitRetryAfter(response, message, nowMs) { + if (response.status !== 403 && response.status !== 429) return 0; + const clamp = (seconds) => Math.min(MAX_RETRY_AFTER, Math.max(1, Math.ceil(seconds))); + const retryAfter = response.headers.get('retry-after'); + if (retryAfter !== null && /^\s*[0-9]+\s*$/.test(retryAfter)) return clamp(Number(retryAfter)); + const exhausted = response.headers.get('x-ratelimit-remaining') === '0'; + const reset = Number(response.headers.get('x-ratelimit-reset')); + if (exhausted && Number.isFinite(reset) && reset > 0) return clamp(reset - nowMs / 1000); + if (exhausted || response.status === 429 || /rate limit/i.test(message)) return DEFAULT_RETRY_AFTER; + return 0; +} + +/** + * One GitHub REST call. Resolves to the parsed JSON body (or null for an empty + * one); rejects with a GitHubError. + * + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {string} method + * @param {string} path starting with `/` + * @param {{token: string, body?: unknown}} options + * @returns {Promise} + */ +export async function githubRequest(deps, method, path, { token, body }) { + const headers = { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'User-Agent': USER_AGENT, + 'X-GitHub-Api-Version': API_VERSION, + }; + if (body !== undefined) headers['Content-Type'] = 'application/json'; + let response; + let text; + try { + response = await deps.fetch(API + path, { + method, + headers, + body: body === undefined ? undefined : JSON.stringify(body), + redirect: 'error', + signal: AbortSignal.timeout(GITHUB_TIMEOUT_MS), + }); + text = await response.text(); + } catch (error) { + throw new GitHubError('failed', 0, { message: error?.name || 'Error' }); + } + let json; + try { + json = text ? JSON.parse(text) : null; + } catch { + json = null; + } + if (response.ok) return json; + const message = typeof json?.message === 'string' ? json.message.slice(0, 200) : ''; + const retryAfter = rateLimitRetryAfter(response, message, deps.now()); + if (retryAfter > 0) throw new GitHubError('rate_limited', response.status, { retryAfter, message }); + if (response.status === 401) throw new GitHubError('unauthorized', 401, { message }); + if (response.status === 404) throw new GitHubError('not_found', 404, { message }); + if (response.status === 422) throw new GitHubError('unprocessable', 422, { message }); + throw new GitHubError('failed', response.status, { message }); +} + +/** + * @typedef {{id: number, login: string, createdAt: number}} GitHubUser + */ + +/** + * Who owns this user token: `GET /user`, the only call the submitter's token + * is ever used for. + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {string} userToken + * @returns {Promise} + */ +export async function getUser(deps, userToken) { + const user = await githubRequest(deps, 'GET', '/user', { token: userToken }); + const createdAt = Date.parse(user?.created_at ?? ''); + if ( + !Number.isSafeInteger(user?.id) || + user.id <= 0 || + typeof user.login !== 'string' || + !Number.isFinite(createdAt) + ) { + throw new GitHubError('failed', 200, { + message: 'GET /user answered without an id, login or created_at', + }); + } + return { id: user.id, login: user.login, createdAt }; +} + +/** + * The JSON Web Token that authenticates as the App itself, for the two calls + * that mint an installation token. Backdated a minute for clock drift and + * valid for nine, inside GitHub's ten-minute ceiling. + * @param {string} appId + * @param {import('node:crypto').KeyObject} privateKey + * @param {number} nowMs + * @returns {string} + */ +export function createAppJwt(appId, privateKey, nowMs) { + const now = Math.floor(nowMs / 1000); + const encode = (value) => Buffer.from(JSON.stringify(value), 'utf8').toString('base64url'); + const unsigned = `${encode({ alg: 'RS256', typ: 'JWT' })}.${encode({ iat: now - 60, exp: now + 540, iss: Number(appId) })}`; + const signature = sign('RSA-SHA256', Buffer.from(unsigned, 'utf8'), privateKey).toString('base64url'); + return `${unsigned}.${signature}`; +} + +/** @type {{key: string, token: string, expiresAt: number}|null} */ +let cachedToken = null; + +/** Forget the cached installation token (tests; and after GitHub rejects it). */ +export function clearInstallationTokenCache() { + cachedToken = null; +} + +/** + * An installation token for the uploads repository with `contents: write`, + * reused from module scope while it has more than five minutes left. + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {import('./config.mjs').UploadsConfig} config + * @returns {Promise} + */ +export async function installationToken(deps, config) { + const key = `${config.appId}:${config.owner}/${config.repo}`; + if ( + cachedToken && + cachedToken.key === key && + deps.now() < cachedToken.expiresAt - TOKEN_REFRESH_MARGIN_MS + ) { + return cachedToken.token; + } + cachedToken = null; + const jwt = createAppJwt(config.appId, config.privateKey, deps.now()); + const installation = await githubRequest( + deps, + 'GET', + `/repos/${config.owner}/${config.repo}/installation`, + { + token: jwt, + } + ); + if (!Number.isSafeInteger(installation?.id) || installation.id <= 0) { + throw new GitHubError('failed', 200, { message: 'installation lookup answered without an id' }); + } + const minted = await githubRequest(deps, 'POST', `/app/installations/${installation.id}/access_tokens`, { + token: jwt, + body: { repositories: [config.repo], permissions: { contents: 'write' } }, + }); + const expiresAt = Date.parse(minted?.expires_at ?? ''); + if (typeof minted?.token !== 'string' || !TOKEN_PATTERN.test(minted.token) || !Number.isFinite(expiresAt)) { + throw new GitHubError('failed', 201, { message: 'token endpoint answered without a usable token' }); + } + cachedToken = { key, token: minted.token, expiresAt }; + return minted.token; +} diff --git a/services/uploads/lib/refs.mjs b/services/uploads/lib/refs.mjs new file mode 100644 index 0000000..a94cab6 --- /dev/null +++ b/services/uploads/lib/refs.mjs @@ -0,0 +1,67 @@ +/** + * The `upload:` reference rules, copied from scripts/lib/upload_refs.mjs (the + * shared contract; read its header for the whole story). This service deploys + * from its own directory, so it cannot import that file; + * test/scripts/upload_refs_parity.test.mjs fails if the two copies drift. + * + * upload:/. what the submit page puts in the issue + * refs/heads/u/// where the file lives in the + * private uploads repository + */ + +/** Extension → the kind the upload service is asked for. */ +export const UPLOAD_EXTENSIONS = Object.freeze({ png: 'image', jpg: 'image', webp: 'image', pdf: 'pdf' }); + +export const SET_ID_RE = /^[a-z2-7]{26}$/; +export const SHA256_RE = /^[0-9a-f]{64}$/; + +const REF_RE = /^upload:([a-z2-7]{26})\/([0-9a-f]{64})\.(png|jpg|webp|pdf)$/; + +/** + * @typedef {{ set: string, sha256: string, ext: 'png'|'jpg'|'webp'|'pdf' }} UploadRef + */ + +/** + * Parse one reference. Surrounding whitespace is ignored; anything else that is + * not exactly the form above is not a reference. + * + * @param {unknown} text + * @returns {UploadRef|null} + */ +export function parseUploadRef(text) { + const match = REF_RE.exec(String(text ?? '').trim()); + if (!match) return null; + return { set: match[1], sha256: match[2], ext: /** @type {UploadRef['ext']} */ (match[3]) }; +} + +/** + * @param {UploadRef} ref + * @returns {string} `upload:/.` + */ +export function formatUploadRef({ set, sha256, ext }) { + return `upload:${set}/${sha256}.${ext}`; +} + +/** + * The branch, without `refs/heads/`, that holds one uploaded file. + * + * @param {number|string} userId the uploader's (and so the issue author's) numeric GitHub id + * @param {UploadRef} ref + * @returns {string} `u///` + */ +export function uploadBranch(userId, { set, sha256 }) { + const id = String(userId ?? '').trim(); + if (!/^[1-9][0-9]*$/.test(id)) throw new Error(`not a GitHub user id: ${JSON.stringify(userId)}`); + if (!SET_ID_RE.test(set) || !SHA256_RE.test(sha256)) throw new Error('not an upload reference'); + return `u/${id}/${set}/${sha256}`; +} + +/** + * The file's name inside that branch's single-file tree. + * + * @param {UploadRef} ref + * @returns {string} `.` + */ +export function uploadFilename({ sha256, ext }) { + return `${sha256}.${ext}`; +} diff --git a/services/uploads/lib/sniff.mjs b/services/uploads/lib/sniff.mjs new file mode 100644 index 0000000..d80cded --- /dev/null +++ b/services/uploads/lib/sniff.mjs @@ -0,0 +1,35 @@ +/** + * Which file a body is, decided by its first bytes and never by a name or a + * declared type. The signatures match scripts/lib/images.mjs `sniffImageType` + * and scripts/lib/attachments.mjs's `%PDF` check, minus GIF, which the upload + * service does not accept. + */ + +/** @param {Uint8Array} bytes @param {number[]} signature @param {number} [offset] */ +function startsWith(bytes, signature, offset = 0) { + return ( + bytes.length >= offset + signature.length && signature.every((byte, i) => bytes[offset + i] === byte) + ); +} + +const PNG = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; +const JPEG = [0xff, 0xd8, 0xff]; +const RIFF = [0x52, 0x49, 0x46, 0x46]; +const WEBP = [0x57, 0x45, 0x42, 0x50]; +const PDF = [0x25, 0x50, 0x44, 0x46, 0x2d]; // %PDF- + +/** + * The extension a body earns for the kind the page asked to upload, or '' when + * the bytes are not an accepted file of that kind. + * @param {Uint8Array} bytes + * @param {'image'|'pdf'} kind + * @returns {''|'png'|'jpg'|'webp'|'pdf'} + */ +export function detectExtension(bytes, kind) { + if (kind === 'pdf') return startsWith(bytes, PDF) ? 'pdf' : ''; + if (kind !== 'image') return ''; + if (startsWith(bytes, PNG)) return 'png'; + if (startsWith(bytes, JPEG)) return 'jpg'; + if (startsWith(bytes, RIFF) && startsWith(bytes, WEBP, 8)) return 'webp'; + return ''; +} diff --git a/services/uploads/lib/store.mjs b/services/uploads/lib/store.mjs new file mode 100644 index 0000000..c2858fe --- /dev/null +++ b/services/uploads/lib/store.mjs @@ -0,0 +1,119 @@ +/** + * The private uploads repository as a file store: one parentless commit and + * one branch per file, so a write never races another and deleting the branch + * is the whole cleanup. + * + * refs/heads/u/// → commit → tree { . } + * + * Every call here uses the App installation token. Branch names come only from + * lib/refs.mjs `uploadBranch`, from the caller's verified numeric user id. + */ + +import { githubRequest, GitHubError } from './github.mjs'; +import { SET_ID_RE, SHA256_RE, uploadBranch, uploadFilename } from './refs.mjs'; + +/** + * @typedef {{fetch: typeof fetch, now: () => number}} Deps + * @typedef {{owner: string, repo: string}} Repo + */ + +/** @param {Repo} repo */ +const base = (repo) => `/repos/${repo.owner}/${repo.repo}/git`; + +/** + * Every file one user has stored, as `{ set, sha256 }`. Refs under the user's + * prefix that do not have exactly the expected shape are ignored. + * @param {Deps} deps + * @param {Repo} repo + * @param {string} token installation token + * @param {number} userId + * @returns {Promise>} + */ +export async function listUserFiles(deps, repo, token, userId) { + const prefix = `refs/heads/u/${userId}/`; + const refs = await githubRequest(deps, 'GET', `${base(repo)}/matching-refs/heads/u/${userId}/`, { token }); + if (!Array.isArray(refs)) + throw new GitHubError('failed', 200, { message: 'matching-refs answered without a list' }); + const files = []; + for (const entry of refs) { + const name = typeof entry?.ref === 'string' ? entry.ref : ''; + if (!name.startsWith(prefix)) continue; + const [set, sha256, extra] = name.slice(prefix.length).split('/'); + if (extra === undefined && SET_ID_RE.test(set ?? '') && SHA256_RE.test(sha256 ?? '')) + files.push({ set, sha256 }); + } + return files; +} + +/** + * Store one file: blob → tree → parentless commit → branch. A branch that + * already exists (two tabs uploading the same file at once) is success: its + * name is the file's hash, so it holds these same bytes. + * @param {Deps} deps + * @param {Repo} repo + * @param {string} token installation token + * @param {number} userId + * @param {{set: string, sha256: string, ext: string, bytes: Uint8Array}} file + * @returns {Promise} + */ +export async function writeUpload(deps, repo, token, userId, { set, sha256, ext, bytes }) { + const ref = `refs/heads/${uploadBranch(userId, { set, sha256 })}`; + const blob = await githubRequest(deps, 'POST', `${base(repo)}/blobs`, { + token, + body: { content: Buffer.from(bytes).toString('base64'), encoding: 'base64' }, + }); + const tree = await githubRequest(deps, 'POST', `${base(repo)}/trees`, { + token, + body: { tree: [{ path: uploadFilename({ sha256, ext }), mode: '100644', type: 'blob', sha: sha(blob) }] }, + }); + const commit = await githubRequest(deps, 'POST', `${base(repo)}/commits`, { + token, + body: { message: `upload ${ext} for user ${userId}`, tree: sha(tree), parents: [] }, + }); + try { + await githubRequest(deps, 'POST', `${base(repo)}/refs`, { token, body: { ref, sha: sha(commit) } }); + } catch (error) { + if ( + error instanceof GitHubError && + error.kind === 'unprocessable' && + /already exists/i.test(error.githubMessage) + ) { + return; + } + throw error; + } +} + +/** + * Delete one of the caller's files. Already gone is fine. + * @param {Deps} deps + * @param {Repo} repo + * @param {string} token installation token + * @param {number} userId + * @param {{set: string, sha256: string}} file + * @returns {Promise} + */ +export async function deleteUpload(deps, repo, token, userId, { set, sha256 }) { + const branch = uploadBranch(userId, { set, sha256 }); + try { + await githubRequest(deps, 'DELETE', `${base(repo)}/refs/heads/${branch}`, { token }); + } catch (error) { + if (error instanceof GitHubError && error.kind === 'not_found') return; + if ( + error instanceof GitHubError && + error.kind === 'unprocessable' && + /does not exist/i.test(error.githubMessage) + ) { + return; + } + throw error; + } +} + +/** @param {any} object @returns {string} the `sha` GitHub answered with */ +function sha(object) { + if (typeof object?.sha !== 'string' || !/^[0-9a-f]{40,64}$/.test(object.sha)) { + throw new GitHubError('failed', 201, { message: 'GitHub answered a write without a sha' }); + } + return object.sha; +} diff --git a/services/uploads/lib/upload.mjs b/services/uploads/lib/upload.mjs new file mode 100644 index 0000000..e6572db --- /dev/null +++ b/services/uploads/lib/upload.mjs @@ -0,0 +1,383 @@ +/** + * The upload service for a PHCT site's /submit/ page: a signed-in submitter + * sends one screenshot or PDF deck, and this service checks it and writes it + * straight into the deployment's private uploads repository, then answers with + * the `upload:` reference the page puts in the issue body. It stores nothing + * itself. See docs/submit-uploads-plan.md. + * + * OPTIONS /api/upload CORS preflight for the allowed site origins + * POST /api/upload raw file body → 201 {ref, sha256, bytes, ext} + * DELETE /api/upload?ref=upload:/. → 204 + * + * POST, in order: configuration → method → an allowed Origin → the bearer + * token names a GitHub user (one `GET /user`) → account age → upload headers + * → size (4 MiB, read with a hard cap) → magic bytes → sha256 → per-user caps + * (3 drafts, 9 files each; an identical file already stored is answered + * without writing) → blob, tree, parentless commit, branch. + * + * Every handler takes a WHATWG Request and returns a Response; `fetch` and the + * clock are injectable, so the tests never touch the network. + */ + +import { createHash } from 'node:crypto'; + +import { parseAllowedOrigins, readConfig } from './config.mjs'; +import { + GitHubError, + TOKEN_PATTERN, + clearInstallationTokenCache, + getUser, + installationToken, +} from './github.mjs'; +import { SET_ID_RE, formatUploadRef, parseUploadRef } from './refs.mjs'; +import { detectExtension } from './sniff.mjs'; +import { deleteUpload, listUserFiles, writeUpload } from './store.mjs'; + +/** 4 MiB, for images and PDFs alike: under Vercel's 4.5 MB request body limit. */ +export const MAX_BYTES = 4 * 1024 * 1024; +/** Drafts (upload sets) one user may have files in at once. */ +export const MAX_SETS = 3; +/** Files one draft may hold. */ +export const MAX_FILES_PER_SET = 9; +/** Longest display name the page may send. */ +export const MAX_NAME_LENGTH = 200; +/** What a per-user cap 429 suggests: files clear when the user removes them. */ +export const CAP_RETRY_AFTER = 3600; + +export const ALLOWED_METHODS = 'POST, DELETE'; +export const ALLOWED_HEADERS = 'Authorization, Content-Type, X-Upload-Set, X-Upload-Kind, X-Upload-Name'; +export const PREFLIGHT_MAX_AGE = '600'; + +const KINDS = new Set(['image', 'pdf']); +const DAY_MS = 24 * 60 * 60 * 1000; + +/** Every error this service answers with: HTTP status and a sentence a submitter can act on. */ +export const ERRORS = { + not_configured: [ + 500, + 'File uploads are not set up on this server yet. Submit your answers, then add your files on GitHub instead.', + ], + method_not_allowed: [405, 'This address only accepts file uploads and removals.'], + origin_not_allowed: [403, 'This upload service does not accept files from this website.'], + unauthorized: [ + 401, + 'Your GitHub sign-in has expired or is not valid. Sign in again, then add the file again.', + ], + account_too_new: [ + 403, + 'Your GitHub account is too new to upload files here. Submit your answers, then add your files on GitHub instead.', + ], + bad_set: [400, 'The page sent this file without a valid draft id. Reload the page and try again.'], + bad_kind: [ + 400, + 'The page did not say whether this file is an image or a PDF. Reload the page and try again.', + ], + bad_name: [400, 'The file name is too long. Rename the file to something shorter and try again.'], + bad_length: [400, 'The page sent this file with a broken size. Reload the page and try again.'], + bad_ref: [400, 'That file reference is not valid. Reload the page and try again.'], + empty_file: [400, 'This file is empty. Choose the file again.'], + interrupted: [400, 'The upload was interrupted before the whole file arrived. Try again.'], + wrong_content_type: [ + 415, + 'The page sent this file in a form this service does not accept. Reload the page and try again.', + ], + too_large: [ + 413, + 'This file is larger than 4 MB. Make it smaller (for slides, export a PDF at the smallest size) and try again.', + ], + not_an_image: [ + 415, + 'This file is not a PNG, JPEG or WebP image. Save it in one of those formats and try again.', + ], + not_a_pdf: [415, 'This file is not a PDF. Export or save it as a PDF and try again.'], + too_many_drafts: [ + 429, + 'You already have files saved for three other drafts. Finish or remove the files in an earlier draft, then try again.', + ], + too_many_files: [429, 'This draft already has 9 files, the most it can hold. Remove one to add another.'], + rate_limited: [429, 'GitHub is receiving a lot of files right now. Wait a minute and try again.'], + github_unavailable: [503, 'GitHub could not save the file just now. Try again in a minute.'], +}; + +/** Headers on every response: nothing here may be cached or leak a referrer. */ +const BASE_HEADERS = { + 'Cache-Control': 'no-store', + 'Referrer-Policy': 'no-referrer', + 'X-Content-Type-Options': 'nosniff', + Vary: 'Origin', +}; + +/** + * @param {number} status + * @param {unknown} body JSON body, or null for none + * @param {string} corsOrigin the allowed origin to echo, '' for none + * @param {Record} [extra] + * @returns {Response} + */ +function respond(status, body, corsOrigin, extra = {}) { + const headers = new Headers({ ...BASE_HEADERS, ...extra }); + if (corsOrigin) { + headers.set('Access-Control-Allow-Origin', corsOrigin); + headers.set('Access-Control-Expose-Headers', 'Retry-After'); + } + if (body === null) return new Response(null, { status, headers }); + headers.set('Content-Type', 'application/json; charset=utf-8'); + return new Response(JSON.stringify(body), { status, headers }); +} + +/** + * @param {keyof typeof ERRORS} code + * @param {string} corsOrigin + * @param {Record} [extra] + * @returns {Response} + */ +export function errorResponse(code, corsOrigin, extra) { + const [status, message] = ERRORS[code]; + return respond(status, { error: code, message }, corsOrigin, extra); +} + +/** @param {keyof typeof ERRORS} code @param {number} seconds @param {string} corsOrigin */ +function rateLimited(code, seconds, corsOrigin) { + return errorResponse(code, corsOrigin, { 'Retry-After': String(seconds) }); +} + +/** + * The request's Origin, when ALLOWED_ORIGIN parses and lists it; '' otherwise. + * Worked out apart from the rest of the configuration so that even a "not set + * up" answer reaches the page readably. + * @param {Request} request + * @param {Record} env + * @returns {string} + */ +function allowedOrigin(request, env) { + const origin = request.headers.get('origin'); + if (!origin) return ''; + const { origins } = parseAllowedOrigins(env?.ALLOWED_ORIGIN); + return origins.includes(origin) ? origin : ''; +} + +/** + * Map a failed GitHub call to an answer, logging only the kind and status. + * @param {unknown} error + * @param {string} step what was being done, for the log + * @param {string} corsOrigin + * @returns {Response} + */ +function githubFailure(error, step, corsOrigin) { + if (!(error instanceof GitHubError)) { + console.error(`uploads: ${step} failed (${error?.name || 'Error'}).`); + return errorResponse('github_unavailable', corsOrigin); + } + console.error(`uploads: ${step} failed: GitHub ${error.kind}, HTTP ${error.status}.`); + if (error.kind === 'rate_limited') return rateLimited('rate_limited', error.retryAfter, corsOrigin); + if (error.kind === 'unauthorized') clearInstallationTokenCache(); + return errorResponse('github_unavailable', corsOrigin); +} + +/** + * Who is calling: the bearer token resolved to a GitHub user. The token is + * used for this one call and nothing else. + * @param {Request} request + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {string} corsOrigin + * @returns {Promise<{user: import('./github.mjs').GitHubUser}|{response: Response}>} + */ +async function authenticate(request, deps, corsOrigin) { + const match = /^Bearer ([^\s]+)$/.exec(request.headers.get('authorization') || ''); + if (!match || !TOKEN_PATTERN.test(match[1])) return { response: errorResponse('unauthorized', corsOrigin) }; + try { + return { user: await getUser(deps, match[1]) }; + } catch (error) { + if (error instanceof GitHubError && error.kind === 'rate_limited') { + return { response: rateLimited('rate_limited', error.retryAfter, corsOrigin) }; + } + if (error instanceof GitHubError && (error.kind === 'unauthorized' || error.status === 403)) { + return { response: errorResponse('unauthorized', corsOrigin) }; + } + return { response: githubFailure(error, 'GET /user', corsOrigin) }; + } +} + +/** + * The App installation token, or the answer to give when it cannot be minted. + * GitHub refusing the App itself (wrong key, wrong App ID, not installed on + * UPLOADS_REPO) is a setup problem, not an outage. + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {import('./config.mjs').UploadsConfig} config + * @param {string} corsOrigin + * @returns {Promise<{token: string}|{response: Response}>} + */ +async function appToken(deps, config, corsOrigin) { + try { + return { token: await installationToken(deps, config) }; + } catch (error) { + if (error instanceof GitHubError && (error.kind === 'not_found' || error.kind === 'unauthorized')) { + console.error( + `uploads: GitHub refused the uploads App (HTTP ${error.status}): check UPLOADS_APP_ID, UPLOADS_APP_PRIVATE_KEY and that the App is installed on UPLOADS_REPO.` + ); + return { response: errorResponse('not_configured', corsOrigin) }; + } + return { response: githubFailure(error, 'minting the installation token', corsOrigin) }; + } +} + +/** + * Read the body, giving up as soon as it passes `max` bytes. + * @param {Request} request + * @param {number} max + * @returns {Promise} + */ +export async function readCapped(request, max) { + if (!request.body) return new Uint8Array(0); + const reader = request.body.getReader(); + const chunks = []; + let total = 0; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.byteLength; + if (total > max) { + reader.cancel().catch(() => {}); + return 'too_large'; + } + chunks.push(value); + } + } catch { + return 'interrupted'; + } + return new Uint8Array(Buffer.concat(chunks, total)); +} + +/** + * OPTIONS /api/upload: the CORS preflight. It grants nothing by itself, so it + * answers for an allowed origin even while the rest of the configuration is + * broken; the real request then gets the readable "not set up" error. + * @param {string} corsOrigin + * @returns {Response} + */ +function preflight(corsOrigin) { + if (!corsOrigin) return errorResponse('origin_not_allowed', ''); + return respond(204, null, corsOrigin, { + 'Access-Control-Allow-Methods': ALLOWED_METHODS, + 'Access-Control-Allow-Headers': ALLOWED_HEADERS, + 'Access-Control-Max-Age': PREFLIGHT_MAX_AGE, + }); +} + +/** + * POST /api/upload + * @param {Request} request + * @param {import('./config.mjs').UploadsConfig} config + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {string} cors + * @returns {Promise} + */ +async function upload(request, config, deps, cors) { + const auth = await authenticate(request, deps, cors); + if ('response' in auth) return auth.response; + const { user } = auth; + + if (config.minAccountAgeDays > 0 && deps.now() - user.createdAt < config.minAccountAgeDays * DAY_MS) { + return errorResponse('account_too_new', cors); + } + + const set = request.headers.get('x-upload-set') || ''; + if (!SET_ID_RE.test(set)) return errorResponse('bad_set', cors); + const kind = request.headers.get('x-upload-kind') || ''; + if (!KINDS.has(kind)) return errorResponse('bad_kind', cors); + const name = request.headers.get('x-upload-name'); + if (name !== null && name.length > MAX_NAME_LENGTH) return errorResponse('bad_name', cors); + const type = (request.headers.get('content-type') || '').split(';')[0].trim().toLowerCase(); + if (type !== 'application/octet-stream') return errorResponse('wrong_content_type', cors); + const length = request.headers.get('content-length'); + if (length !== null) { + if (!/^[0-9]{1,16}$/.test(length.trim())) return errorResponse('bad_length', cors); + if (Number(length) > MAX_BYTES) return errorResponse('too_large', cors); + } + + const bytes = await readCapped(request, MAX_BYTES); + if (bytes === 'too_large' || bytes === 'interrupted') return errorResponse(bytes, cors); + if (bytes.length === 0) return errorResponse('empty_file', cors); + + const ext = detectExtension(bytes, kind); + if (!ext) return errorResponse(kind === 'pdf' ? 'not_a_pdf' : 'not_an_image', cors); + + const sha256 = createHash('sha256').update(bytes).digest('hex'); + const body = { ref: formatUploadRef({ set, sha256, ext }), sha256, bytes: bytes.length, ext }; + + const app = await appToken(deps, config, cors); + if ('response' in app) return app.response; + + let files; + try { + files = await listUserFiles(deps, config, app.token, user.id); + } catch (error) { + return githubFailure(error, 'listing the uploader’s files', cors); + } + if (files.some((file) => file.set === set && file.sha256 === sha256)) return respond(201, body, cors); + const sets = new Set(files.map((file) => file.set)); + if (!sets.has(set) && sets.size >= MAX_SETS) return rateLimited('too_many_drafts', CAP_RETRY_AFTER, cors); + if (files.filter((file) => file.set === set).length >= MAX_FILES_PER_SET) { + return rateLimited('too_many_files', CAP_RETRY_AFTER, cors); + } + + try { + await writeUpload(deps, config, app.token, user.id, { set, sha256, ext, bytes }); + } catch (error) { + return githubFailure(error, 'storing the file', cors); + } + return respond(201, body, cors); +} + +/** + * DELETE /api/upload?ref=upload:/.. The branch is built from + * the caller's own verified user id, so another user's file cannot be named. + * @param {Request} request + * @param {import('./config.mjs').UploadsConfig} config + * @param {{fetch: typeof fetch, now: () => number}} deps + * @param {string} cors + * @returns {Promise} + */ +async function remove(request, config, deps, cors) { + const auth = await authenticate(request, deps, cors); + if ('response' in auth) return auth.response; + + const ref = parseUploadRef(new URL(request.url).searchParams.get('ref')); + if (!ref) return errorResponse('bad_ref', cors); + + const app = await appToken(deps, config, cors); + if ('response' in app) return app.response; + try { + await deleteUpload(deps, config, app.token, auth.user.id, ref); + } catch (error) { + return githubFailure(error, 'removing a file', cors); + } + return respond(204, null, cors); +} + +/** + * Every method of /api/upload. + * @param {Request} request + * @param {Record} env + * @param {{fetch?: typeof fetch, now?: () => number}} [deps] + * @returns {Promise} + */ +export async function handleUpload(request, env, deps = {}) { + const resolved = { fetch: deps.fetch ?? globalThis.fetch, now: deps.now ?? Date.now }; + const cors = allowedOrigin(request, env); + if (request.method === 'OPTIONS') return preflight(cors); + const config = readConfig(env); + if (config.error) { + console.error(`uploads: refusing every request: ${config.error}`); + return errorResponse('not_configured', cors); + } + if (request.method !== 'POST' && request.method !== 'DELETE') { + return errorResponse('method_not_allowed', cors, { Allow: `OPTIONS, ${ALLOWED_METHODS}` }); + } + // A browser always sends Origin on these requests; anything without an + // allowed one is not the catalog's submit page. + if (!cors) return errorResponse('origin_not_allowed', ''); + if (request.method === 'POST') return upload(request, config, resolved, cors); + return remove(request, config, resolved, cors); +} diff --git a/services/uploads/package.json b/services/uploads/package.json new file mode 100644 index 0000000..63e42ed --- /dev/null +++ b/services/uploads/package.json @@ -0,0 +1,11 @@ +{ + "name": "phct-uploads", + "version": "1.0.0", + "private": true, + "description": "Optional upload service for a PHCT /submit/ page: takes a screenshot or PDF from a signed-in submitter and writes it to the deployment's private GitHub uploads repository. Deployed as its own Vercel project from this directory.", + "license": "MIT", + "type": "module", + "engines": { + "node": "22.x" + } +} diff --git a/services/uploads/test/config.test.mjs b/services/uploads/test/config.test.mjs new file mode 100644 index 0000000..dc94ddc --- /dev/null +++ b/services/uploads/test/config.test.mjs @@ -0,0 +1,113 @@ +/** + * The upload service's configuration: every variable is checked, and while + * any is missing or malformed every upload and delete is refused before + * GitHub is called. + */ +import assert from 'node:assert/strict'; +import { generateKeyPairSync } from 'node:crypto'; +import test from 'node:test'; + +import { normalizePem, parseAllowedOrigins, readConfig } from '../lib/config.mjs'; +import { handleUpload } from '../lib/upload.mjs'; +import { ENV, NOW, PRIVATE_PEM, SITE, fakeGitHub, png, request } from './helpers.mjs'; + +let logged = []; +const realError = console.error; +test.beforeEach(() => { + logged = []; + console.error = (...args) => logged.push(args.join(' ')); +}); +test.afterEach(() => { + console.error = realError; +}); + +const EC_PEM = generateKeyPairSync('ec', { namedCurve: 'P-256' }).privateKey.export({ + type: 'pkcs8', + format: 'pem', +}); + +/** Every broken configuration, and what its error names. */ +const BROKEN = [ + [{}, /UPLOADS_APP_ID, UPLOADS_APP_PRIVATE_KEY, UPLOADS_REPO, ALLOWED_ORIGIN/], + [{ ...ENV, UPLOADS_APP_ID: '' }, /Missing environment variable: UPLOADS_APP_ID/], + [{ ...ENV, UPLOADS_APP_PRIVATE_KEY: ' ' }, /UPLOADS_APP_PRIVATE_KEY/], + [{ ...ENV, UPLOADS_REPO: undefined }, /UPLOADS_REPO/], + [{ ...ENV, ALLOWED_ORIGIN: '' }, /ALLOWED_ORIGIN/], + [{ ...ENV, UPLOADS_APP_ID: 'Iv23liClient' }, /App ID/], + [{ ...ENV, UPLOADS_APP_ID: '0' }, /App ID/], + [{ ...ENV, UPLOADS_APP_PRIVATE_KEY: 'not a key' }, /not a PEM private key/], + [{ ...ENV, UPLOADS_APP_PRIVATE_KEY: EC_PEM }, /not an RSA key/], + [{ ...ENV, UPLOADS_REPO: 'catalog-uploads' }, /owner\/name/], + [{ ...ENV, UPLOADS_REPO: 'acme/catalog/uploads' }, /owner\/name/], + [{ ...ENV, UPLOADS_REPO: 'acme/..' }, /owner\/name/], + [{ ...ENV, UPLOADS_REPO: '-acme/uploads' }, /owner\/name/], + [{ ...ENV, UPLOADS_REPO: 'acme/up loads' }, /owner\/name/], + [{ ...ENV, ALLOWED_ORIGIN: `${SITE}/` }, /origin only/], + [{ ...ENV, ALLOWED_ORIGIN: 'http://acme.github.io' }, /https/], + [{ ...ENV, ALLOWED_ORIGIN: 'acme.github.io' }, /not a URL/], + [{ ...ENV, MIN_ACCOUNT_AGE_DAYS: 'thirty' }, /MIN_ACCOUNT_AGE_DAYS/], + [{ ...ENV, MIN_ACCOUNT_AGE_DAYS: '-1' }, /MIN_ACCOUNT_AGE_DAYS/], + [{ ...ENV, MIN_ACCOUNT_AGE_DAYS: '1.5' }, /MIN_ACCOUNT_AGE_DAYS/], +]; + +test('readConfig fails closed on every missing or malformed setting, without echoing the key', () => { + for (const [env, pattern] of BROKEN) { + const config = readConfig(env); + assert.match(config.error, pattern, JSON.stringify(Object.keys(env))); + assert.equal(config.privateKey, null); + assert.doesNotMatch(config.error, /BEGIN|PRIVATE KEY-----/); + } +}); + +test('readConfig accepts a good configuration, a one-line key with \\n escapes, and a blank account age', () => { + const config = readConfig(ENV); + assert.equal(config.error, ''); + assert.equal(config.appId, '123456'); + assert.equal(config.owner, 'acme'); + assert.equal(config.repo, 'catalog-uploads'); + assert.deepEqual(config.origins, [SITE]); + assert.equal(config.minAccountAgeDays, 0); + + const oneLine = PRIVATE_PEM.trim().replace(/\n/g, '\\n'); + assert.ok(!oneLine.includes('\n')); + assert.equal(normalizePem(oneLine), PRIVATE_PEM.trim()); + const escaped = readConfig({ ...ENV, UPLOADS_APP_PRIVATE_KEY: oneLine, MIN_ACCOUNT_AGE_DAYS: ' ' }); + assert.equal(escaped.error, ''); + assert.equal(escaped.minAccountAgeDays, 0); + assert.equal(readConfig({ ...ENV, MIN_ACCOUNT_AGE_DAYS: '30' }).minAccountAgeDays, 30); + assert.deepEqual(readConfig({ ...ENV, ALLOWED_ORIGIN: ` ${SITE} , http://localhost:4000 ` }).origins, [ + SITE, + 'http://localhost:4000', + ]); +}); + +test('parseAllowedOrigins allows plain http only for a loopback address', () => { + assert.equal(parseAllowedOrigins('http://127.0.0.1:4000').error, ''); + assert.equal(parseAllowedOrigins('http://[::1]:4000').error, ''); + assert.match(parseAllowedOrigins('http://10.0.0.5').error, /https/); + assert.match(parseAllowedOrigins(',').error, /empty/); +}); + +test('a broken configuration refuses every upload and delete before GitHub is called', async () => { + for (const [env] of BROKEN) { + for (const method of ['POST', 'DELETE']) { + const github = fakeGitHub(); + const response = await handleUpload( + request(method, { body: method === 'POST' ? png() : undefined, query: '?ref=x' }), + env, + { fetch: github, now: () => NOW } + ); + assert.equal(response.status, 500); + const body = await response.json(); + assert.equal(body.error, 'not_configured'); + assert.match(body.message, /not set up/); + assert.equal(github.calls.length, 0); + assert.equal(response.headers.get('cache-control'), 'no-store'); + // The page still gets to read the answer when ALLOWED_ORIGIN itself is fine. + const originOk = parseAllowedOrigins(env.ALLOWED_ORIGIN).origins.includes(SITE); + assert.equal(response.headers.get('access-control-allow-origin'), originOk ? SITE : null); + } + } + assert.ok(logged.every((line) => line.startsWith('uploads: refusing every request:'))); + assert.doesNotMatch(logged.join('\n'), /PRIVATE KEY/); +}); diff --git a/services/uploads/test/github.test.mjs b/services/uploads/test/github.test.mjs new file mode 100644 index 0000000..9d3546c --- /dev/null +++ b/services/uploads/test/github.test.mjs @@ -0,0 +1,165 @@ +/** + * The uploads App's authentication (RS256 JWT, installation token and its + * cache) and how GitHub's answers are read, with a stubbed `fetch`. + */ +import assert from 'node:assert/strict'; +import { verify } from 'node:crypto'; +import test from 'node:test'; + +import { readConfig } from '../lib/config.mjs'; +import { + API_VERSION, + GitHubError, + USER_AGENT, + clearInstallationTokenCache, + createAppJwt, + getUser, + githubRequest, + installationToken, + rateLimitRetryAfter, +} from '../lib/github.mjs'; +import { + ENV, + INSTALLATION_ID, + INSTALL_TOKEN, + NOW, + PUBLIC_KEY, + USER_TOKEN, + fakeGitHub, + json, +} from './helpers.mjs'; + +const CONFIG = readConfig(ENV); + +test.beforeEach(() => clearInstallationTokenCache()); + +/** @param {string} jwt */ +function decode(jwt) { + const [header, payload, signature] = jwt.split('.'); + return { + header: JSON.parse(Buffer.from(header, 'base64url').toString()), + payload: JSON.parse(Buffer.from(payload, 'base64url').toString()), + signed: `${header}.${payload}`, + signature: Buffer.from(signature, 'base64url'), + }; +} + +test('the App JWT is RS256 with iat now-60, exp now+540, iss the App ID, and verifies with the public key', () => { + const jwt = createAppJwt(CONFIG.appId, CONFIG.privateKey, NOW + 999); + const { header, payload, signed, signature } = decode(jwt); + const now = Math.floor(NOW / 1000); + assert.deepEqual(header, { alg: 'RS256', typ: 'JWT' }); + assert.deepEqual(payload, { iat: now - 60, exp: now + 540, iss: 123456 }); + assert.equal(verify('RSA-SHA256', Buffer.from(signed), PUBLIC_KEY, signature), true); + assert.equal(verify('RSA-SHA256', Buffer.from(signed + 'x'), PUBLIC_KEY, signature), false); +}); + +test('installationToken looks up the installation with the JWT and asks for contents:write on the one repository', async () => { + const github = fakeGitHub(); + const token = await installationToken({ fetch: github, now: () => NOW }, CONFIG); + assert.equal(token, INSTALL_TOKEN); + assert.deepEqual( + github.calls.map((call) => `${call.method} ${call.path}`), + [ + 'GET /repos/acme/catalog-uploads/installation', + `POST /app/installations/${INSTALLATION_ID}/access_tokens`, + ] + ); + for (const call of github.calls) { + const jwt = call.auth.replace(/^Bearer /, ''); + assert.equal(decode(jwt).payload.iss, 123456); + assert.equal(call.headers['X-GitHub-Api-Version'], API_VERSION); + assert.equal(call.headers['User-Agent'], USER_AGENT); + } + assert.deepEqual(github.calls[1].body, { + repositories: ['catalog-uploads'], + permissions: { contents: 'write' }, + }); +}); + +test('the installation token is reused until five minutes before it expires, then minted again', async () => { + let now = NOW; + const github = fakeGitHub({ now: () => now }); + const deps = { fetch: github, now: () => now }; + await installationToken(deps, CONFIG); + now += 54 * 60 * 1000; // 6 minutes left + await installationToken(deps, CONFIG); + assert.equal(github.calls.length, 2, 'reused'); + now += 2 * 60 * 1000; // 4 minutes left + await installationToken(deps, CONFIG); + assert.equal(github.calls.length, 4, 'refreshed'); + + // A different App never gets the cached token. + await installationToken(deps, { ...CONFIG, appId: '999' }); + assert.equal(github.calls.length, 6); + + clearInstallationTokenCache(); + await installationToken(deps, CONFIG); + assert.equal(github.calls.length, 8); +}); + +test('installationToken refuses an answer without a usable token', async () => { + const github = fakeGitHub({ + routes: { [`POST /app/installations/${INSTALLATION_ID}/access_tokens`]: () => json(201, { token: 'x' }) }, + }); + await assert.rejects(installationToken({ fetch: github, now: () => NOW }, CONFIG), (error) => { + return error instanceof GitHubError && error.kind === 'failed'; + }); + const noId = fakeGitHub({ + routes: { 'GET /repos/acme/catalog-uploads/installation': () => json(200, {}) }, + }); + await assert.rejects(installationToken({ fetch: noId, now: () => NOW }, CONFIG), GitHubError); +}); + +test('rateLimitRetryAfter reads retry-after, the primary-limit reset, and secondary-limit wording', () => { + const at = (status, headers = {}) => new Response(null, { status, headers }); + assert.equal(rateLimitRetryAfter(at(403, { 'retry-after': '30' }), '', NOW), 30); + assert.equal(rateLimitRetryAfter(at(429, { 'retry-after': '0' }), '', NOW), 1); + assert.equal(rateLimitRetryAfter(at(429, { 'retry-after': '99999' }), '', NOW), 3600); + const reset = String(Math.floor(NOW / 1000) + 120); + assert.equal( + rateLimitRetryAfter(at(403, { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': reset }), '', NOW), + 120 + ); + assert.equal(rateLimitRetryAfter(at(403), 'You have exceeded a secondary rate limit.', NOW), 60); + assert.equal(rateLimitRetryAfter(at(429), '', NOW), 60); + assert.equal(rateLimitRetryAfter(at(403), 'Resource not accessible by integration', NOW), 0); + assert.equal(rateLimitRetryAfter(at(500, { 'retry-after': '30' }), '', NOW), 0); +}); + +test('githubRequest maps statuses to kinds and turns a network failure into "failed"', async () => { + const deps = (answer) => ({ fetch: async () => answer(), now: () => NOW }); + const kind = async (answer) => { + try { + await githubRequest(deps(answer), 'GET', '/x', { token: 't' }); + return 'ok'; + } catch (error) { + return error.kind; + } + }; + assert.equal(await kind(() => json(401, { message: 'Bad credentials' })), 'unauthorized'); + assert.equal(await kind(() => json(404, { message: 'Not Found' })), 'not_found'); + assert.equal(await kind(() => json(422, { message: 'Reference already exists' })), 'unprocessable'); + assert.equal(await kind(() => new Response('', { status: 502 })), 'failed'); + assert.equal( + await kind(() => { + throw Object.assign(new Error('timed out'), { name: 'TimeoutError' }); + }), + 'failed' + ); + assert.equal(await kind(() => new Response(null, { status: 204 })), 'ok'); +}); + +test('getUser returns the numeric id, login and creation time, and refuses a malformed answer', async () => { + const github = fakeGitHub(); + const user = await getUser({ fetch: github, now: () => NOW }, USER_TOKEN); + assert.deepEqual(user, { id: 4242, login: 'octo', createdAt: Date.parse('2015-01-01T00:00:00Z') }); + for (const bad of [ + { id: '4242', login: 'octo', created_at: '2015-01-01T00:00:00Z' }, + { id: 1, login: 'x' }, + null, + ]) { + const broken = fakeGitHub({ routes: { 'GET /user': () => json(200, bad) } }); + await assert.rejects(getUser({ fetch: broken, now: () => NOW }, USER_TOKEN), GitHubError); + } +}); diff --git a/services/uploads/test/helpers.mjs b/services/uploads/test/helpers.mjs new file mode 100644 index 0000000..959b37b --- /dev/null +++ b/services/uploads/test/helpers.mjs @@ -0,0 +1,140 @@ +/** + * Shared fixtures for the upload service tests: a throwaway App key, the + * environment, sample files, and a fake GitHub that answers the calls the + * service makes and records every one of them. No network. + */ +import { generateKeyPairSync } from 'node:crypto'; + +export const SERVICE = 'https://uploads.example.app'; +export const SITE = 'https://acme.github.io'; +export const OWNER = 'acme'; +export const REPO = 'catalog-uploads'; +export const USER_TOKEN = 'ghu_' + 'U'.repeat(36); +export const INSTALL_TOKEN = 'ghs_' + 'I'.repeat(36); +export const USER_ID = 4242; +export const INSTALLATION_ID = 77; +export const SET = 'abcdefghijklmnopqrstuvwx23'; +export const NOW = Date.parse('2026-10-16T12:00:00Z'); + +export const { privateKey: PRIVATE_PEM, publicKey: PUBLIC_KEY } = (() => { + const pair = generateKeyPairSync('rsa', { modulusLength: 2048 }); + return { + privateKey: pair.privateKey.export({ type: 'pkcs1', format: 'pem' }), + publicKey: pair.publicKey, + }; +})(); + +export const ENV = Object.freeze({ + UPLOADS_APP_ID: '123456', + UPLOADS_APP_PRIVATE_KEY: PRIVATE_PEM, + UPLOADS_REPO: `${OWNER}/${REPO}`, + ALLOWED_ORIGIN: SITE, +}); + +const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; + +/** + * A body of `size` bytes that starts with `head`. + * @param {number[]|string} head + * @param {number} [size] + * @returns {Uint8Array} + */ +export function file(head, size = 64) { + const start = typeof head === 'string' ? Buffer.from(head, 'latin1') : Buffer.from(head); + const bytes = new Uint8Array(Math.max(size, start.length)).fill(0x41); + bytes.set(start, 0); + return bytes; +} + +export const png = (size) => file(PNG_SIGNATURE, size); + +/** @param {number} status @param {unknown} body @param {Record} [headers] */ +export function json(status, body, headers = {}) { + return new Response(body === null ? null : JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json', ...headers }, + }); +} + +/** + * A request to the service, from the catalog site unless told otherwise. + * @param {string} method + * @param {{body?: BodyInit, headers?: Record, query?: string}} [options] + */ +export function request(method, { body, headers = {}, query = '' } = {}) { + const all = { + Origin: SITE, + Authorization: `Bearer ${USER_TOKEN}`, + 'Content-Type': 'application/octet-stream', + 'X-Upload-Set': SET, + 'X-Upload-Kind': 'image', + ...headers, + }; + for (const [name, value] of Object.entries(all)) if (value === null) delete all[name]; + return new Request(`${SERVICE}/api/upload${query}`, { method, body, headers: all, duplex: 'half' }); +} + +/** + * A fake api.github.com. `routes` override any default by `METHOD /path`; an + * override returning undefined falls through to the default. + * @param {{user?: object, refs?: string[], routes?: Record, now?: () => number}} [options] + */ +export function fakeGitHub({ user, refs = [], routes = {}, now = () => NOW } = {}) { + const state = { refs: new Set(refs), calls: [] }; + const theUser = user ?? { id: USER_ID, login: 'octo', created_at: '2015-01-01T00:00:00Z' }; + const base = `/repos/${OWNER}/${REPO}`; + const impl = async (url, init = {}) => { + const parsed = new URL(url); + const method = init.method ?? 'GET'; + const path = parsed.pathname; + const body = typeof init.body === 'string' ? JSON.parse(init.body) : undefined; + const call = { url: String(url), method, path, body, headers: init.headers ?? {} }; + call.auth = call.headers.Authorization; + state.calls.push(call); + const override = routes[`${method} ${path}`]; + if (override) { + const answer = await override(call, state); + if (answer !== undefined) return answer; + } + if (method === 'GET' && path === '/user') { + return call.auth === `Bearer ${USER_TOKEN}` + ? json(200, theUser) + : json(401, { message: 'Bad credentials' }); + } + if (method === 'GET' && path === `${base}/installation`) return json(200, { id: INSTALLATION_ID }); + if (method === 'POST' && path === `/app/installations/${INSTALLATION_ID}/access_tokens`) { + return json(201, { token: INSTALL_TOKEN, expires_at: new Date(now() + 60 * 60 * 1000).toISOString() }); + } + const matching = `${base}/git/matching-refs/heads/`; + if (method === 'GET' && path.startsWith(matching)) { + const prefix = `refs/heads/${path.slice(matching.length)}`; + return json( + 200, + [...state.refs] + .filter((ref) => ref.startsWith(prefix)) + .map((ref) => ({ ref, object: { sha: 'c'.repeat(40) } })) + ); + } + if (method === 'POST' && path === `${base}/git/blobs`) return json(201, { sha: 'b'.repeat(40) }); + if (method === 'POST' && path === `${base}/git/trees`) return json(201, { sha: 'd'.repeat(40) }); + if (method === 'POST' && path === `${base}/git/commits`) return json(201, { sha: 'c'.repeat(40) }); + if (method === 'POST' && path === `${base}/git/refs`) { + if (state.refs.has(body.ref)) return json(422, { message: 'Reference already exists' }); + state.refs.add(body.ref); + return json(201, { ref: body.ref }); + } + const refPath = `${base}/git/refs/heads/`; + if (method === 'DELETE' && path.startsWith(refPath)) { + const ref = `refs/heads/${path.slice(refPath.length)}`; + if (!state.refs.delete(ref)) return json(422, { message: 'Reference does not exist' }); + return new Response(null, { status: 204 }); + } + return json(404, { message: 'Not Found' }); + }; + impl.state = state; + impl.calls = state.calls; + return impl; +} + +/** The ref name a file is stored under. */ +export const branch = (sha256, set = SET, id = USER_ID) => `refs/heads/u/${id}/${set}/${sha256}`; diff --git a/services/uploads/test/upload.test.mjs b/services/uploads/test/upload.test.mjs new file mode 100644 index 0000000..51159b4 --- /dev/null +++ b/services/uploads/test/upload.test.mjs @@ -0,0 +1,644 @@ +/** + * /api/upload end to end: the handler called directly with WHATWG Requests + * and a fake GitHub — CORS, sign-in, account age, headers, size, magic bytes, + * per-user caps, the exact write sequence, rate limits and deletes. + */ +import assert from 'node:assert/strict'; +import { createHash } from 'node:crypto'; +import process from 'node:process'; +import test from 'node:test'; + +import * as api from '../api/upload.mjs'; +import { clearInstallationTokenCache } from '../lib/github.mjs'; +import { ALLOWED_HEADERS, MAX_BYTES, handleUpload, readCapped } from '../lib/upload.mjs'; +import { + ENV, + INSTALLATION_ID, + INSTALL_TOKEN, + NOW, + SERVICE, + SET, + SITE, + USER_ID, + USER_TOKEN, + branch, + fakeGitHub, + file, + json, + png, + request, +} from './helpers.mjs'; + +// Keep what the handler logs, on every console channel, so the tests can +// prove no token is ever in it. +let logged = []; +const real = { error: console.error, warn: console.warn, log: console.log, info: console.info }; +test.beforeEach(() => { + logged = []; + for (const name of Object.keys(real)) console[name] = (...args) => logged.push(args.map(String).join(' ')); + clearInstallationTokenCache(); +}); +test.afterEach(() => { + Object.assign(console, real); +}); + +const sha = (bytes) => createHash('sha256').update(bytes).digest('hex'); + +/** + * Each call starts without a cached installation token unless `keepToken`, so + * every call exercises the minting routes its fake GitHub defines. + * @param {Request} req @param {object} [options] + */ +async function call(req, { github = fakeGitHub(), env = ENV, now = () => NOW, keepToken = false } = {}) { + if (!keepToken) clearInstallationTokenCache(); + const response = await handleUpload(req, env, { fetch: github, now }); + const text = response.status === 204 ? '' : await response.text(); + return { response, status: response.status, body: text ? JSON.parse(text) : null, github }; +} + +const writes = (github) => github.calls.filter((c) => c.method === 'POST' && c.path.includes('/git/')); + +/** The answer a fake GitHub gives for a secondary rate limit. */ +const secondary = (headers = {}) => + json(403, { message: 'You have exceeded a secondary rate limit. Please wait a few minutes.' }, headers); + +test('the API module routes every method to the handler, configured from process.env', async () => { + const saved = { ...process.env }; + for (const name of [...Object.keys(ENV), 'MIN_ACCOUNT_AGE_DAYS']) delete process.env[name]; + process.env.ALLOWED_ORIGIN = SITE; + try { + const preflight = await api.OPTIONS( + new Request(`${SERVICE}/api/upload`, { method: 'OPTIONS', headers: { Origin: SITE } }) + ); + assert.equal(preflight.status, 204); + for (const method of ['POST', 'DELETE', 'GET', 'PUT', 'PATCH']) { + const response = await api[method](request(method, { body: method === 'POST' ? png() : undefined })); + assert.equal(response.status, 500, method); + assert.equal((await response.json()).error, 'not_configured'); + } + } finally { + for (const name of Object.keys(process.env)) if (!(name in saved)) delete process.env[name]; + Object.assign(process.env, saved); + } +}); + +test('preflight from an allowed origin echoes it with the methods, headers and max-age', async () => { + const { status, response } = await call( + new Request(`${SERVICE}/api/upload`, { method: 'OPTIONS', headers: { Origin: SITE } }) + ); + assert.equal(status, 204); + assert.equal(response.headers.get('access-control-allow-origin'), SITE); + assert.equal(response.headers.get('access-control-allow-methods'), 'POST, DELETE'); + assert.equal(response.headers.get('access-control-allow-headers'), ALLOWED_HEADERS); + assert.equal(ALLOWED_HEADERS, 'Authorization, Content-Type, X-Upload-Set, X-Upload-Kind, X-Upload-Name'); + assert.equal(response.headers.get('access-control-max-age'), '600'); + assert.equal(response.headers.get('vary'), 'Origin'); + assert.equal(response.headers.get('cache-control'), 'no-store'); + assert.equal(response.headers.get('referrer-policy'), 'no-referrer'); +}); + +test('preflight from any other origin, or none, is refused without CORS headers', async () => { + for (const headers of [{ Origin: 'https://evil.example' }, { Origin: 'null' }, {}]) { + const { status, body, response } = await call( + new Request(`${SERVICE}/api/upload`, { method: 'OPTIONS', headers }) + ); + assert.equal(status, 403); + assert.equal(body.error, 'origin_not_allowed'); + assert.equal(response.headers.get('access-control-allow-origin'), null); + } +}); + +test('a POST or DELETE without an allowed Origin is refused before GitHub is called', async () => { + for (const origin of [null, 'https://evil.example', `${SITE}.evil.example`]) { + for (const method of ['POST', 'DELETE']) { + const { status, body, github, response } = await call( + request(method, { body: method === 'POST' ? png() : undefined, headers: { Origin: origin } }) + ); + assert.equal(status, 403); + assert.equal(body.error, 'origin_not_allowed'); + assert.equal(github.calls.length, 0); + assert.equal(response.headers.get('access-control-allow-origin'), null); + } + } +}); + +test('other methods get a JSON 405 with CORS headers', async () => { + for (const method of ['GET', 'PUT', 'PATCH']) { + const { status, body, response, github } = await call( + request(method, { body: method === 'GET' ? undefined : png() }) + ); + assert.equal(status, 405); + assert.equal(body.error, 'method_not_allowed'); + assert.equal(response.headers.get('allow'), 'OPTIONS, POST, DELETE'); + assert.equal(response.headers.get('access-control-allow-origin'), SITE); + assert.equal(github.calls.length, 0); + } +}); + +test('a missing or malformed token is 401 without calling GitHub; a token GitHub rejects is 401', async () => { + for (const authorization of [ + null, + '', + 'token ' + USER_TOKEN, + 'Bearer', + 'Bearer short', + `Bearer ${USER_TOKEN} x`, + ]) { + const { status, body, github, response } = await call( + request('POST', { body: png(), headers: { Authorization: authorization } }) + ); + assert.equal(status, 401, String(authorization)); + assert.equal(body.error, 'unauthorized'); + assert.match(body.message, /Sign in again/); + assert.equal(github.calls.length, 0); + assert.equal(response.headers.get('access-control-allow-origin'), SITE); + } + const revoked = 'ghu_' + 'R'.repeat(36); + for (const method of ['POST', 'DELETE']) { + const { status, github } = await call( + request(method, { + body: method === 'POST' ? png() : undefined, + headers: { Authorization: `Bearer ${revoked}` }, + }) + ); + assert.equal(status, 401); + assert.deepEqual( + github.calls.map((c) => c.path), + ['/user'], + 'nothing after GET /user' + ); + } + const forbidden = fakeGitHub({ routes: { 'GET /user': () => json(403, { message: 'Forbidden' }) } }); + assert.equal((await call(request('POST', { body: png() }), { github: forbidden })).status, 401); + assert.doesNotMatch(logged.join('\n'), /ghu_/); +}); + +test('the submitter token goes only to GET /user and never into a log, even when GitHub fails', async () => { + const failing = fakeGitHub({ + routes: { 'POST /repos/acme/catalog-uploads/git/blobs': () => json(500, { message: 'boom' }) }, + }); + const { status } = await call(request('POST', { body: png() }), { github: failing }); + assert.equal(status, 503); + const down = fakeGitHub({ routes: { 'GET /user': () => new Response('bad gateway', { status: 502 }) } }); + assert.equal((await call(request('POST', { body: png() }), { github: down })).status, 503); + assert.ok(logged.length > 0, 'the failures were logged'); + const all = logged.join('\n'); + assert.doesNotMatch(all, new RegExp(USER_TOKEN)); + assert.doesNotMatch(all, new RegExp(INSTALL_TOKEN)); + + const { github } = await call(request('POST', { body: png() })); + for (const c of github.calls) { + const carriesUserToken = JSON.stringify(c).includes(USER_TOKEN); + assert.equal(carriesUserToken, c.path === '/user', `${c.method} ${c.path}`); + } +}); + +test('GitHub rate-limiting GET /user is a 429 with Retry-After', async () => { + const github = fakeGitHub({ + routes: { 'GET /user': () => json(429, { message: 'slow down' }, { 'retry-after': '17' }) }, + }); + const { status, body, response } = await call(request('POST', { body: png() }), { github }); + assert.equal(status, 429); + assert.equal(body.error, 'rate_limited'); + assert.equal(response.headers.get('retry-after'), '17'); + assert.match(response.headers.get('access-control-expose-headers'), /Retry-After/); +}); + +test('MIN_ACCOUNT_AGE_DAYS refuses accounts younger than that, and accepts older ones', async () => { + const env = { ...ENV, MIN_ACCOUNT_AGE_DAYS: '30' }; + const day = 24 * 60 * 60 * 1000; + const young = fakeGitHub({ + user: { id: USER_ID, login: 'new', created_at: new Date(NOW - 29 * day).toISOString() }, + }); + const refused = await call(request('POST', { body: png() }), { github: young, env }); + assert.equal(refused.status, 403); + assert.equal(refused.body.error, 'account_too_new'); + assert.deepEqual( + young.calls.map((c) => c.path), + ['/user'] + ); + + const old = fakeGitHub({ + user: { id: USER_ID, login: 'old', created_at: new Date(NOW - 31 * day).toISOString() }, + }); + assert.equal((await call(request('POST', { body: png() }), { github: old, env })).status, 201); + // Default 0: a day-old account may upload. + const fresh = fakeGitHub({ + user: { id: USER_ID, login: 'fresh', created_at: new Date(NOW - day).toISOString() }, + }); + assert.equal((await call(request('POST', { body: png() }), { github: fresh })).status, 201); +}); + +test('each malformed upload header is refused before anything is written', async () => { + const cases = [ + [{ 'X-Upload-Set': null }, 400, 'bad_set'], + [{ 'X-Upload-Set': SET.toUpperCase() }, 400, 'bad_set'], + [{ 'X-Upload-Set': SET.slice(1) }, 400, 'bad_set'], + [{ 'X-Upload-Set': SET.slice(0, 25) + '1' }, 400, 'bad_set'], + [{ 'X-Upload-Set': `../${SET}` }, 400, 'bad_set'], + [{ 'X-Upload-Kind': null }, 400, 'bad_kind'], + [{ 'X-Upload-Kind': 'gif' }, 400, 'bad_kind'], + [{ 'X-Upload-Kind': 'IMAGE' }, 400, 'bad_kind'], + [{ 'X-Upload-Name': 'a'.repeat(201) }, 400, 'bad_name'], + [{ 'Content-Type': 'image/png' }, 415, 'wrong_content_type'], + [{ 'Content-Type': null }, 415, 'wrong_content_type'], + [{ 'Content-Length': 'abc' }, 400, 'bad_length'], + ]; + for (const [headers, status, error] of cases) { + const result = await call(request('POST', { body: png(), headers })); + assert.equal(result.status, status, JSON.stringify(headers)); + assert.equal(result.body.error, error); + assert.equal(typeof result.body.message, 'string'); + assert.deepEqual(writes(result.github), []); + } + const named = await call( + request('POST', { + body: png(), + headers: { + 'X-Upload-Name': 'a'.repeat(200), + 'Content-Type': 'application/octet-stream; charset=binary', + }, + }) + ); + assert.equal(named.status, 201); +}); + +test('4 MiB is accepted; one byte more is 413, whatever Content-Length claims', async () => { + const exact = png(MAX_BYTES); + assert.equal(MAX_BYTES, 4 * 1024 * 1024); + const ok = await call(request('POST', { body: exact })); + assert.equal(ok.status, 201); + assert.equal(ok.body.bytes, MAX_BYTES); + + const over = png(MAX_BYTES + 1); + const big = await call(request('POST', { body: over })); + assert.equal(big.status, 413); + assert.equal(big.body.error, 'too_large'); + assert.deepEqual(writes(big.github), []); + + const declared = await call( + request('POST', { body: png(), headers: { 'Content-Length': String(MAX_BYTES + 1) } }) + ); + assert.equal(declared.status, 413); + + const lying = await call(request('POST', { body: over, headers: { 'Content-Length': '64' } })); + assert.equal(lying.status, 413, 'a Content-Length that lies does not get past the read cap'); + assert.deepEqual(writes(lying.github), []); +}); + +test('the body is read with a hard cap: a stream is abandoned as soon as it passes the limit', async () => { + let pulled = 0; + let cancelled = false; + const stream = new ReadableStream({ + pull(controller) { + pulled += 1; + controller.enqueue(new Uint8Array(1024 * 1024)); + if (pulled > 100) controller.close(); + }, + cancel() { + cancelled = true; + }, + }); + const result = await readCapped({ body: stream }, MAX_BYTES); + assert.equal(result, 'too_large'); + assert.ok(pulled <= 6, `pulled ${pulled} MiB`); + assert.equal(cancelled, true); + + const broken = new ReadableStream({ + pull(controller) { + controller.error(new Error('connection reset')); + }, + }); + assert.equal(await readCapped({ body: broken }, MAX_BYTES), 'interrupted'); + assert.equal((await readCapped({ body: null }, MAX_BYTES)).length, 0); +}); + +test('an empty body is refused', async () => { + const { status, body } = await call(request('POST', { body: new Uint8Array(0) })); + assert.equal(status, 400); + assert.equal(body.error, 'empty_file'); +}); + +test('the magic bytes decide the extension, and anything else is 415', async () => { + const RIFF_WEBP = [0x52, 0x49, 0x46, 0x46, 0x10, 0, 0, 0, 0x57, 0x45, 0x42, 0x50]; + const accepted = [ + ['image', png(), 'png'], + ['image', file([0xff, 0xd8, 0xff, 0xe0]), 'jpg'], + ['image', file(RIFF_WEBP), 'webp'], + ['pdf', file('%PDF-1.7\n'), 'pdf'], + ]; + for (const [kind, bytes, ext] of accepted) { + const { status, body } = await call(request('POST', { body: bytes, headers: { 'X-Upload-Kind': kind } })); + assert.equal(status, 201, ext); + assert.equal(body.ext, ext); + assert.equal(body.ref, `upload:${SET}/${sha(bytes)}.${ext}`); + } + const refused = [ + ['image', file('GIF89a'), 'not_an_image'], + ['image', file(''), 'not_an_image'], + ['image', file('…`, `url | alt`, or a bare URL. - * Duplicate URLs are collapsed, keeping the first alt text seen. + * `![alt](url)`, `…`, `url | alt`, or a bare URL — + * and `upload:/.` with or without `| alt`, a file attached + * on /submit/ (scripts/lib/upload_refs.mjs), returned as `{ upload, alt }`. + * Duplicate URLs (and duplicate references) are collapsed, keeping the first + * alt text seen. A line that is neither is dropped. * @param {string} raw - * @returns {Array<{url: string, alt: string}>} + * @returns {Array<{url: string, alt: string}|{upload: import('./upload_refs.mjs').UploadRef, alt: string}>} */ export function parseImageRefs(raw) { const found = new Map(); const add = (url, alt) => { + const upload = parseUploadRef(url); + if (upload) { + const key = formatUploadRef(upload); + if (!found.has(key)) found.set(key, { upload, alt: String(alt ?? '').trim() }); + return; + } const clean = String(url ?? '') .trim() .replace(/[).,]+$/, ''); diff --git a/scripts/lib/uploads.mjs b/scripts/lib/uploads.mjs new file mode 100644 index 0000000..0b88814 --- /dev/null +++ b/scripts/lib/uploads.mjs @@ -0,0 +1,189 @@ +/** + * Read a file a submitter attached on /submit/ through the optional upload + * service (services/uploads/, docs/submit-uploads-plan.md) out of the + * deployment's PRIVATE uploads repository, so the scaffolder can check and save + * it exactly as it saves a GitHub attachment. + * + * This is the only code that ever sees `UPLOADS_TOKEN`, a read-only GitHub App + * installation token for that one repository. It is sent to api.github.com and + * nowhere else — the generic fetchers in images.mjs and attachments.mjs attach + * their `token` to any GitHub host a submitter names, so this token must never + * reach them — and it never appears in a log line, an error or a warning. + * + * The ref is built here, from the ISSUE AUTHOR's numeric id and the reference + * (`uploadBranch`, scripts/lib/upload_refs.mjs), never from anything else in + * the issue: a reference copied into someone else's issue resolves to nothing, + * and no reference can name another repository. + * + * Nothing here deletes or writes. Every edit of the issue rebuilds the draft + * branch and resolves the references again, so the files must still be there; + * clean-up is uploads-cleanup.yml's job, with a token of its own. + */ + +import crypto from 'node:crypto'; +import process from 'node:process'; + +import { TIMEOUT_MS, readCapped, sizeCapMessage } from './images.mjs'; +import { formatUploadRef, uploadBranch, uploadFilename } from './upload_refs.mjs'; + +const API = 'https://api.github.com'; + +/** `owner/name`, in GitHub's own character rules. */ +const REPO_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})\/(?!\.\.?$)[A-Za-z0-9._-]{1,100}$/; +/** A git object id, SHA-1 or SHA-256, as the API returns it. */ +const GIT_SHA_RE = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; +/** A numeric GitHub user id, the same rule `uploadBranch` applies. */ +const USER_ID_RE = /^[1-9][0-9]*$/; + +/** Default cap when a caller names none: GitHub's own document upload ceiling. */ +const DEFAULT_MAX_BYTES = 25 * 1024 * 1024; + +/** + * The warning for `upload:` lines in an issue on a catalog that has no upload + * service configured. Written for the submitter, who never saw a setting. + * @param {string} field the question's label, as the submitter saw it + * @param {number} count how many references that answer held + * @returns {string} + */ +export function notConfiguredWarning(field, count) { + const many = count > 1; + return ( + `${many ? `${count} files` : 'A file'} attached on the submit page for “${field}” could not be collected, ` + + 'because file uploads are not set up on this catalog. ' + + `To include ${many ? 'them' : 'it'}, edit this issue and drag the ${many ? 'files' : 'file'} into the “${field}” answer, ` + + `in place of the ${many ? 'lines that start' : 'line that starts'} with \`upload:\`.` + ); +} + +/** + * The warning for a reference the uploads repository does not hold (expired, + * removed, or never uploaded by this issue's author). + * @param {string} field + * @param {import('./upload_refs.mjs').UploadRef} ref + * @returns {string} + */ +export function notFoundWarning(field, ref) { + return ( + `A file attached on the submit page for “${field}” could not be found — uploads are kept for a limited time, so it may have expired. ` + + `To include it, edit this issue and drag the file into the “${field}” answer, in place of the line \`${formatUploadRef(ref)}\`.` + ); +} + +/** A 404 (or 409, an empty repository) at any step: there is no such file. */ +class NotFound extends Error {} + +/** + * Build the resolver from the environment the workflow sets when the feature + * is on. Any of the three blank or malformed means "not set up": the caller + * skips `upload:` lines with `notConfiguredWarning`. + * + * @param {Record} [env] `UPLOADS_TOKEN`, `UPLOADS_REPO` (`owner/name`), `ISSUE_AUTHOR_ID` + * @param {object} [options] + * @param {typeof fetch} [options.fetchImpl] injected for tests + * @param {number} [options.timeoutMs] covers every request of one resolve, body included + * @returns {{ok: true, resolve: (ref: import('./upload_refs.mjs').UploadRef, opts: {maxBytes?: number, field?: string}) => Promise} + * | {ok: false, problem: string}} + * `problem` names the setting (never its value) for the job log. + */ +export function uploadResolver( + env = process.env, + { fetchImpl = globalThis.fetch, timeoutMs = TIMEOUT_MS } = {} +) { + const token = String(env?.UPLOADS_TOKEN ?? '').trim(); + const repo = String(env?.UPLOADS_REPO ?? '').trim(); + const authorId = String(env?.ISSUE_AUTHOR_ID ?? '').trim(); + + if (!token || !repo || !authorId) { + const missing = [ + !token && 'UPLOADS_TOKEN', + !repo && 'UPLOADS_REPO', + !authorId && 'ISSUE_AUTHOR_ID', + ].filter(Boolean); + return { ok: false, problem: `uploads are not configured (${missing.join(', ')} not set)` }; + } + if (!REPO_RE.test(repo)) return { ok: false, problem: 'UPLOADS_REPO is not in owner/name form' }; + if (!USER_ID_RE.test(authorId)) + return { ok: false, problem: 'ISSUE_AUTHOR_ID is not a numeric GitHub user id' }; + + /** Never let the token out in a message, whatever produced it. */ + const scrub = (text) => + String(text ?? '') + .split(token) + .join('***'); + + /** + * One GET against the uploads repository's git database. + * @param {string} route path under /repos//git/ + * @param {AbortSignal} signal + * @param {string} [accept] + */ + const get = async (route, signal, accept = 'application/vnd.github+json') => { + const response = await fetchImpl(`${API}/repos/${repo}/git/${route}`, { + method: 'GET', + redirect: 'manual', + signal, + headers: { + accept, + authorization: `Bearer ${token}`, + 'x-github-api-version': '2022-11-28', + 'user-agent': 'phct-scaffolder', + }, + }); + if (!response) throw new Error('no response from GitHub'); + const status = Number(response.status); + if (status === 404 || status === 409) throw new NotFound(); + if (!response.ok) throw new Error(`GitHub answered HTTP ${status}`); + return response; + }; + + const sha = (value) => { + const text = String(value ?? ''); + if (!GIT_SHA_RE.test(text)) throw new Error('GitHub returned an unexpected object id'); + return text; + }; + + return { + ok: true, + async resolve(ref, { maxBytes = DEFAULT_MAX_BYTES, field = '' } = {}) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + const { signal } = controller; + try { + const branch = uploadBranch(authorId, ref); + + const head = await (await get(`ref/heads/${branch}`, signal)).json(); + if (head?.object?.type !== 'commit') throw new Error('the upload ref does not point at a commit'); + const commit = await (await get(`commits/${sha(head.object.sha)}`, signal)).json(); + const tree = await (await get(`trees/${sha(commit?.tree?.sha)}`, signal)).json(); + + const name = uploadFilename(ref); + const entry = (Array.isArray(tree?.tree) ? tree.tree : []).find((item) => item?.path === name); + if (!entry) throw new NotFound(); + if (entry.type !== 'blob') throw new Error('the upload is not a single file'); + // Judged before a byte of the file moves. + if (!(Number(entry.size) <= maxBytes)) throw new Error(sizeCapMessage(maxBytes)); + + const response = await get(`blobs/${sha(entry.sha)}`, signal, 'application/vnd.github.raw'); + const declared = Number(response.headers?.get?.('content-length')); + if (Number.isFinite(declared) && declared > maxBytes) throw new Error(sizeCapMessage(maxBytes)); + const streamed = typeof response.body?.[Symbol.asyncIterator] === 'function'; + const bytes = streamed + ? await readCapped(response.body, maxBytes, () => controller.abort()) + : new Uint8Array(await response.arrayBuffer()); + if (bytes.length > maxBytes) throw new Error(sizeCapMessage(maxBytes)); + + const digest = crypto.createHash('sha256').update(bytes).digest('hex'); + if (digest !== ref.sha256) throw new Error('the file does not match its upload reference'); + return bytes; + } catch (error) { + if (error instanceof NotFound) return { warning: notFoundWarning(field, ref) }; + // No `cause`: the original error is dropped on purpose, so nothing that + // might carry the token travels on with the message. + // eslint-disable-next-line preserve-caught-error + throw new Error(scrub(error?.message || 'the upload could not be read')); + } finally { + clearTimeout(timer); + } + }, + }; +} diff --git a/scripts/new_entry_from_issue.mjs b/scripts/new_entry_from_issue.mjs index 9906e12..9b8f699 100644 --- a/scripts/new_entry_from_issue.mjs +++ b/scripts/new_entry_from_issue.mjs @@ -3,6 +3,8 @@ * Scaffold a catalog entry from a GitHub issue-form submission. * * Input (env): ISSUE_BODY, ISSUE_TITLE, ISSUE_NUMBER, GITHUB_TOKEN (optional) + * UPLOADS_TOKEN, UPLOADS_REPO, ISSUE_AUTHOR_ID (optional, all three + * or none: files attached on /submit/, read by scripts/lib/uploads.mjs) * Output: //index.md * //screenshots/NN. (downloaded images) * // (uploaded attachments) @@ -29,8 +31,10 @@ import * as yaml from 'js-yaml'; import { fail, setOutput } from './lib/actions_output.mjs'; import { frontMatter } from './lib/yaml.mjs'; import { escalations, reviewChecklist } from './lib/review.mjs'; -import { attachmentValue, downloadAttachment, shouldDownload } from './lib/attachments.mjs'; -import { downloadImages, MAX_FILES } from './lib/images.mjs'; +import { attachmentValue, downloadAttachment, shouldDownload, storeAttachment } from './lib/attachments.mjs'; +import { downloadImages, MAX_FILES, uploadLabel } from './lib/images.mjs'; +import { formatUploadRef } from './lib/upload_refs.mjs'; +import { notConfiguredWarning, uploadResolver } from './lib/uploads.mjs'; import { NO_RESPONSE, codeSpan, @@ -166,31 +170,76 @@ function insideEntryDir(target) { return resolved; } +// --- uploads --------------------------------------------------------------- +// Files attached on /submit/ arrive as `upload:` lines (docs/submit-uploads-plan.md) +// and are read from the deployment's private uploads repository by +// scripts/lib/uploads.mjs — the only code that touches its token, which is why +// the whole environment is handed over rather than the token picked out here. +// The bytes then go through exactly the checks a downloaded file does. Not set +// up: each answer holding `upload:` lines gets one warning and the rest of the +// submission scaffolds as usual. + +const uploads = uploadResolver(process.env); +let uploadsProblemLogged = false; + +/** + * The warning for an answer whose `upload:` lines cannot be read on this + * catalog; the reason, for a maintainer, goes to the job log once. + * @param {object} field schema field + * @param {number} count + * @returns {string} + */ +function uploadsUnavailable(field, count) { + if (!uploadsProblemLogged) { + console.error(`upload: references skipped: ${uploads.problem}.`); + uploadsProblemLogged = true; + } + return notConfiguredWarning(field.label || field.key, count); +} + // --- images ---------------------------------------------------------------- /** @type {Record>} */ const imageValues = {}; for (const field of fields.filter((f) => f.type === 'images')) { - const refs = parseImageRefs(rawValue(sections, field)); + let refs = parseImageRefs(rawValue(sections, field)); if (refs.length === 0) { imageValues[field.key] = []; continue; } + const uploadRefs = refs.filter((ref) => ref.upload); if (DRY_RUN) { imageValues[field.key] = refs.map((ref, index) => ({ src: `/${entryPath}/${slug}/screenshots/${String(index + 1).padStart(2, '0')}.png`, alt: ref.alt || `${title} — screenshot ${index + 1}`, })); - warnings.push(`Dry run: ${refs.length} image URL(s) were parsed but not downloaded.`); + const urlCount = refs.length - uploadRefs.length; + if (urlCount > 0) warnings.push(`Dry run: ${urlCount} image URL(s) were parsed but not downloaded.`); + if (uploadRefs.length > 0) { + warnings.push( + `Dry run: ${uploadRefs.map((ref) => `\`${formatUploadRef(ref.upload)}\``).join(', ')} parsed but not fetched.` + ); + } continue; } + if (uploadRefs.length > 0 && !uploads.ok) { + warnings.push(uploadsUnavailable(field, uploadRefs.length)); + refs = refs.filter((ref) => !ref.upload); + if (refs.length === 0) { + imageValues[field.key] = []; + continue; + } + } const result = await downloadImages(refs, { destDir: path.join(entryDir, 'screenshots'), publicPrefix: `/${entryPath}/${slug}/screenshots`, altFallback: `${title} — screenshot`, maxFiles: maxImages, token: process.env.GITHUB_TOKEN || '', + resolveUpload: uploads.ok + ? (upload, opts) => uploads.resolve(upload, { ...opts, field: field.label || field.key }) + : undefined, fsImpl: { mkdirSync: (dir, opts) => fs.mkdirSync(insideEntryDir(dir), opts), writeFileSync: (file, data) => fs.writeFileSync(insideEntryDir(file), data), @@ -227,7 +276,14 @@ for (const field of fields.filter((f) => f.type === 'file' || f.type === 'image' attachmentValues[field.key] = ''; continue; } - if (!shouldDownload(field.type, ref.url)) { + // A file attached on /submit/ is always saved into the entry folder, never + // kept as a link (there is no public URL to keep). + if (ref.upload && DRY_RUN) { + attachmentValues[field.key] = publicPath; + warnings.push(`Dry run: \`${formatUploadRef(ref.upload)}\` was parsed but not fetched.`); + continue; + } + if (!ref.upload && !shouldDownload(field.type, ref.url)) { // Stored only in the shape the page renders and the validator accepts; a // link that cannot be one is reported rather than committed as a dead row. const link = siteHttpUrl(ref.url); @@ -245,19 +301,32 @@ for (const field of fields.filter((f) => f.type === 'file' || f.type === 'image' continue; } - const result = await downloadAttachment(ref.url, { + const store = { destDir: entryDir, filename, - token: process.env.GITHUB_TOKEN || '', fsImpl: { mkdirSync: (dir, opts) => fs.mkdirSync(insideEntryDir(dir), opts), writeFileSync: (file, data) => fs.writeFileSync(insideEntryDir(file), data), }, - }); + }; + let result; + if (!ref.upload) { + result = await downloadAttachment(ref.url, { ...store, token: process.env.GITHUB_TOKEN || '' }); + } else if (!uploads.ok) { + result = { saved: false, bytes: 0, warning: uploadsUnavailable(field, 1) }; + } else { + result = await storeAttachment( + uploadLabel(ref.upload), + (maxBytes) => uploads.resolve(ref.upload, { maxBytes, field: field.label || field.key }), + store + ); + } + // An upload that could not be read has no link to keep: the value is what a + // GitHub attachment whose link cannot be kept gets. attachmentValues[field.key] = attachmentValue(field.type, { saved: result.saved, publicPath, - url: ref.url, + url: ref.url ?? '', }); if (result.saved) { savedAttachments.push(publicPath); diff --git a/test/fixtures/scaffold-fetch-stub.mjs b/test/fixtures/scaffold-fetch-stub.mjs new file mode 100644 index 0000000..4663f07 --- /dev/null +++ b/test/fixtures/scaffold-fetch-stub.mjs @@ -0,0 +1,93 @@ +/** + * Preloaded (`node --import`) into a real run of scripts/new_entry_from_issue.mjs + * by test/scripts/new_entry_scaffold.test.mjs, so the scaffolder can be run end + * to end without a network: every hostname resolves to a public address, and + * `fetch` answers from the plan named by `FETCH_STUB_PLAN` (JSON): + * + * { + * "log": "", + * "urls": { "": { "contentType": "image/png", "base64": "…" } }, + * "uploads": { "repo": "owner/name", "branches": { "u/1//": { "path": ".png", "base64": "…" } } } + * } + * + * The uploads repository is served the way the GitHub REST API serves a git + * database: ref → commit → tree → blob. Anything not in the plan is a 404. + * + * Without `FETCH_STUB_PLAN` it does nothing (`npm test` runs every .mjs under + * test/ as a test file, this one included). + */ + +import crypto from 'node:crypto'; +import dns from 'node:dns/promises'; +import fs from 'node:fs'; +import process from 'node:process'; + +if (process.env.FETCH_STUB_PLAN) install(); + +function install() { + const plan = JSON.parse(fs.readFileSync(process.env.FETCH_STUB_PLAN, 'utf8')); + + dns.lookup = async () => [{ address: '93.184.216.34', family: 4 }]; + + const objectId = (text) => crypto.createHash('sha1').update(text).digest('hex'); + + /** Fake git objects for every branch in the plan, keyed by their ids. */ + const commits = new Map(); + const trees = new Map(); + const blobs = new Map(); + const refs = new Map(); + for (const [branch, file] of Object.entries(plan.uploads?.branches ?? {})) { + const bytes = Buffer.from(file.base64, 'base64'); + const blob = objectId(`blob ${branch}`); + const tree = objectId(`tree ${branch}`); + const commit = objectId(`commit ${branch}`); + blobs.set(blob, bytes); + trees.set(tree, [{ path: file.path, mode: '100644', type: 'blob', sha: blob, size: bytes.length }]); + commits.set(commit, tree); + refs.set(branch, commit); + } + + const json = (body) => + new Response(JSON.stringify(body), { status: 200, headers: { 'content-type': 'application/json' } }); + const notFound = () => new Response('{"message":"Not Found"}', { status: 404 }); + + globalThis.fetch = async (input, init = {}) => { + const url = String(input); + const headers = Object.fromEntries( + Object.entries(init.headers ?? {}).map(([k, v]) => [k.toLowerCase(), v]) + ); + fs.appendFileSync( + plan.log, + `${JSON.stringify({ url, method: init.method ?? 'GET', authorization: headers.authorization ?? '' })}\n` + ); + + const api = `https://api.github.com/repos/${plan.uploads?.repo}/git/`; + if (plan.uploads && url.startsWith(api)) { + const route = url.slice(api.length); + let match; + if ((match = /^ref\/heads\/(.+)$/.exec(route)) && refs.has(match[1])) { + return json({ ref: `refs/heads/${match[1]}`, object: { type: 'commit', sha: refs.get(match[1]) } }); + } + if ((match = /^commits\/([0-9a-f]+)$/.exec(route)) && commits.has(match[1])) { + return json({ sha: match[1], tree: { sha: commits.get(match[1]) } }); + } + if ((match = /^trees\/([0-9a-f]+)$/.exec(route)) && trees.has(match[1])) { + return json({ sha: match[1], tree: trees.get(match[1]), truncated: false }); + } + if ((match = /^blobs\/([0-9a-f]+)$/.exec(route)) && blobs.has(match[1])) { + return new Response(blobs.get(match[1]), { + status: 200, + headers: { 'content-type': 'application/octet-stream' }, + }); + } + return notFound(); + } + + const answer = plan.urls?.[url]; + if (!answer) return notFound(); + return new Response(Buffer.from(answer.base64, 'base64'), { + status: 200, + headers: { 'content-type': answer.contentType }, + }); + }; +} diff --git a/test/scripts/attachments.test.mjs b/test/scripts/attachments.test.mjs index 8fd3207..554f9c4 100644 --- a/test/scripts/attachments.test.mjs +++ b/test/scripts/attachments.test.mjs @@ -10,6 +10,7 @@ import { isGitHubAttachmentUrl, matchesExtension, shouldDownload, + storeAttachment, } from '../../scripts/lib/attachments.mjs'; const PDF = new Uint8Array([0x25, 0x50, 0x44, 0x46, 0x2d, 0x31, 0x2e, 0x37]); @@ -252,3 +253,75 @@ test('attachmentValue keeps the link when a file could not be fetched', () => { assert.equal(attachmentValue('image', { saved: true, publicPath, url }), publicPath); assert.equal(attachmentValue('image', { saved: false, publicPath, url }), ''); }); + +// The upload path (scripts/lib/uploads.mjs) and the URL path share one copy of +// the checks (`storeAttachment`). + +test('bytes from an upload meet exactly the checks bytes from a URL do', async () => { + // [case, filename, body, maxBytes, expected warning] + const cases = [ + ['HTML named as a PDF', 'deck.pdf', HTML, undefined, /does not look like PDF/], + ['a PNG named as a PDF', 'deck.pdf', PNG, undefined, /does not look like PDF/], + ['a PNG named as a JPEG', 'photo.jpg', PNG, undefined, /does not look like JPG/], + ['an extension nobody can verify', 'deck.exe', PDF, undefined, /`deck\.exe` is not one of/], + ['a file over the cap', 'deck.pdf', PDF, PDF.length - 1, /larger than the .* size cap/], + ]; + for (const [name, filename, body, maxBytes, expected] of cases) { + const viaUrl = stubFs(); + const fromUrl = await downloadAttachment('https://example.org/x', { + ...base, + filename, + maxBytes, + fsImpl: viaUrl, + fetchImpl: async () => stubResponse(body), + }); + const viaUpload = stubFs(); + const fromUpload = await storeAttachment('the uploaded file `upload:x`', async () => body, { + destDir: base.destDir, + filename, + maxBytes, + fsImpl: viaUpload, + }); + for (const [result, files] of [ + [fromUrl, viaUrl], + [fromUpload, viaUpload], + ]) { + assert.equal(result.saved, false, name); + assert.match(result.warning, expected, name); + assert.equal(files.written.size, 0, name); + } + assert.match(fromUpload.warning, /^Could not attach the uploaded file `upload:x` \(/, name); + } +}); + +test('storeAttachment writes good bytes under the schema’s name and passes the cap to the loader', async () => { + const fsImpl = stubFs(); + const caps = []; + const result = await storeAttachment( + 'the uploaded file', + async (maxBytes) => { + caps.push(maxBytes); + return PDF; + }, + { destDir: base.destDir, filename: 'deck.pdf', fsImpl } + ); + assert.deepEqual(result, { saved: true, bytes: PDF.length, warning: '' }); + assert.deepEqual(caps, [MAX_ATTACHMENT_BYTES]); + assert.deepEqual([...fsImpl.written.keys()], [path.join('/repo/catalog/brief', 'deck.pdf')]); +}); + +test('storeAttachment never calls the loader for a name it cannot verify, and keeps a loader’s own warning', async () => { + const refused = await storeAttachment('x', () => assert.fail('no load'), { + destDir: base.destDir, + filename: 'deck.docx', + fsImpl: stubFs(), + }); + assert.equal(refused.saved, false); + + const missing = await storeAttachment('x', async () => ({ warning: 'It may have expired.' }), { + destDir: base.destDir, + filename: 'deck.pdf', + fsImpl: stubFs(), + }); + assert.deepEqual(missing, { saved: false, bytes: 0, warning: 'It may have expired.' }); +}); diff --git a/test/scripts/images.test.mjs b/test/scripts/images.test.mjs index 013894c..cb58e7d 100644 --- a/test/scripts/images.test.mjs +++ b/test/scripts/images.test.mjs @@ -7,6 +7,7 @@ import { extensionForContentType, ipv6Groups, isBlockedAddress, + saveImages, sniffImageType, } from '../../scripts/lib/images.mjs'; @@ -465,3 +466,154 @@ test('a streamed body under the cap is reassembled byte for byte', async () => { assert.deepEqual(result.warnings, []); assert.deepEqual(new Uint8Array([...files.written.values()][0]), PNG); }); + +// The upload path (scripts/lib/uploads.mjs, through `resolveUpload`) and the +// URL path share one copy of the checks (`saveImages`). + +const SET = 'abcdefghijklmnopqrstuvwxyz'; +const upload = (char, ext) => ({ set: SET, sha256: char.repeat(64), ext }); + +test('bytes from an upload meet exactly the checks bytes from a URL do', async () => { + // [case, URL body + content type, upload body + reference extension, expected warning] + const cases = [ + [ + 'bytes that are not an image', + [TEXT, 'image/png'], + [TEXT, 'png'], + /does not look like a PNG, JPEG, GIF or WebP/, + ], + [ + 'a type that is not an image', + [PNG, 'image/svg+xml'], + [PNG, 'pdf'], + /content type is not PNG, JPEG, GIF or WebP/, + ], + ['an HTML page', [TEXT, 'text/html'], [TEXT, 'pdf'], /content type is not/], + ]; + for (const [name, [urlBytes, contentType], [uploadBytes, ext], expected] of cases) { + const viaUrl = stubFs(); + const fromUrl = await downloadImages([{ url: 'https://e.org/x' }], { + destDir: '/d', + publicPrefix: '/p', + dnsImpl: PUBLIC_DNS, + fsImpl: viaUrl, + fetchImpl: async () => stubResponse(urlBytes, contentType), + }); + const viaUpload = stubFs(); + const fromUpload = await downloadImages([{ upload: upload('a', ext) }], { + destDir: '/d', + publicPrefix: '/p', + fsImpl: viaUpload, + resolveUpload: async () => uploadBytes, + fetchImpl: () => assert.fail('an upload never goes through the URL fetcher'), + }); + for (const [result, files] of [ + [fromUrl, viaUrl], + [fromUpload, viaUpload], + ]) { + assert.deepEqual(result.items, [], name); + assert.equal(result.warnings.length, 1, name); + assert.match(result.warnings[0], expected, name); + assert.equal(files.written.size, 0, name); + } + assert.match(fromUpload.warnings[0], /^Skipped the uploaded file `upload:/, name); + } +}); + +test('an upload is held to the shared size budget, before and after it is read', async () => { + const budgets = []; + const files = stubFs(); + const result = await downloadImages( + [{ url: 'https://e.org/a.png' }, { upload: upload('a', 'png') }, { upload: upload('b', 'png') }], + { + destDir: '/d', + publicPrefix: '/p', + maxTotalBytes: PNG.length * 2 - 1, + dnsImpl: PUBLIC_DNS, + fsImpl: files, + fetchImpl: async () => stubResponse(PNG, 'image/png'), + // Misbehaves: ignores the cap it is given, so the shared total check has to catch it. + resolveUpload: async (_ref, { maxBytes }) => { + budgets.push(maxBytes); + return PNG; + }, + } + ); + assert.deepEqual(budgets, [PNG.length - 1, PNG.length - 1], 'each read is capped at what is left'); + assert.equal(result.items.length, 1); + assert.equal(result.warnings.length, 2); + for (const warning of result.warnings) assert.match(warning, /the images add up to more than/); + assert.equal(files.written.size, 1); +}); + +test('uploads and URLs are numbered in one sequence, and only URLs reach the fetcher', async () => { + const fetched = []; + const resolved = []; + const files = stubFs(); + const result = await downloadImages( + [ + { upload: upload('a', 'png'), alt: 'Landing page' }, + { url: 'https://github.com/user-attachments/assets/b.jpg', alt: 'Queue' }, + { upload: upload('c', 'webp') }, + ], + { + destDir: '/d', + publicPrefix: '/p', + altFallback: 'X — screenshot', + token: 'job-token', + dnsImpl: PUBLIC_DNS, + fsImpl: files, + fetchImpl: async (url, init) => { + fetched.push([url, init.headers.authorization]); + return stubResponse(JPG, 'image/jpeg'); + }, + resolveUpload: async (...args) => { + resolved.push(args); + return args[0].ext === 'png' ? PNG : WEBP; + }, + } + ); + assert.deepEqual(result.items, [ + { src: '/p/01.png', alt: 'Landing page' }, + { src: '/p/02.jpg', alt: 'Queue' }, + { src: '/p/03.webp', alt: 'X — screenshot' }, + ]); + assert.deepEqual(fetched, [['https://github.com/user-attachments/assets/b.jpg', 'Bearer job-token']]); + // The resolver is handed the reference and a cap, and nothing else: no token. + assert.deepEqual( + resolved.map(([ref, opts]) => [ref.sha256[0], Object.keys(opts)]), + [ + ['a', ['maxBytes']], + ['c', ['maxBytes']], + ] + ); +}); + +test('an upload the resolver cannot find is left out with the resolver’s own warning', async () => { + const result = await downloadImages([{ upload: upload('a', 'png') }, { upload: upload('b', 'png') }], { + destDir: '/d', + publicPrefix: '/p', + fsImpl: stubFs(), + resolveUpload: async (ref) => { + if (ref.sha256[0] === 'a') return { warning: 'It may have expired.' }; + throw new Error('the file does not match its upload reference'); + }, + }); + assert.deepEqual(result.items, []); + assert.equal(result.warnings[0], 'It may have expired.'); + assert.match( + result.warnings[1], + /^Could not download the uploaded file `upload:abcdefghijklmnopqrstuvwxyz\/b{64}\.png` \(the file does not match its upload reference\)\. Left it out/ + ); +}); + +test('without a resolver an upload is skipped, not guessed at', async () => { + const result = await downloadImages([{ upload: upload('a', 'png') }], { + destDir: '/d', + publicPrefix: '/p', + fsImpl: stubFs(), + }); + assert.deepEqual(result.items, []); + assert.match(result.warnings[0], /cannot be collected here/); + assert.deepEqual(await saveImages([], { destDir: '/d', publicPrefix: '/p' }), { items: [], warnings: [] }); +}); diff --git a/test/scripts/issue_body.test.mjs b/test/scripts/issue_body.test.mjs index a794523..da74db9 100644 --- a/test/scripts/issue_body.test.mjs +++ b/test/scripts/issue_body.test.mjs @@ -470,3 +470,62 @@ test('coerceChoice strips wrapping quotes, and a long run of quotes stays fast', coerceChoice(field, `a${'"'.repeat(65000)}a`); assert.ok(Date.now() - started < 500, 'quote stripping is linear'); }); + +// `upload:` references: files attached on /submit/ through the optional upload +// service (scripts/lib/upload_refs.mjs). +const UPLOAD_SET = 'abcdefghijklmnopqrstuvwxyz'; +const shaOf = (char) => char.repeat(64); +const uploadLine = (char, ext) => `upload:${UPLOAD_SET}/${shaOf(char)}.${ext}`; +const uploadRef = (char, ext) => ({ set: UPLOAD_SET, sha256: shaOf(char), ext }); + +test('parseImageRefs reads upload: lines with and without alt text, in order, among URLs', () => { + const raw = [ + `${uploadLine('a', 'png')} | Landing page with a Get Started button`, + 'https://example.org/shot2.jpg | The queue view', + `- ${uploadLine('b', 'webp')}`, + `${uploadLine('c', 'jpg')}|Results`, + ].join('\n'); + assert.deepEqual(parseImageRefs(raw), [ + { upload: uploadRef('a', 'png'), alt: 'Landing page with a Get Started button' }, + { url: 'https://example.org/shot2.jpg', alt: 'The queue view' }, + { upload: uploadRef('b', 'webp'), alt: '' }, + { upload: uploadRef('c', 'jpg'), alt: 'Results' }, + ]); +}); + +test('parseImageRefs collapses a repeated reference, keeping the first alt text', () => { + const raw = `${uploadLine('a', 'png')} | First\n${uploadLine('a', 'png')} | Second\n${uploadLine('a', 'png')}`; + assert.deepEqual(parseImageRefs(raw), [{ upload: uploadRef('a', 'png'), alt: 'First' }]); +}); + +test('parseImageRefs drops malformed upload: lines the way it drops malformed URLs', () => { + const malformed = [ + 'upload:', + `upload:${UPLOAD_SET}/${shaOf('a')}`, // no extension + `upload:${UPLOAD_SET}/${shaOf('a')}.gif`, // not an allowed extension + `upload:${UPLOAD_SET}/${'a'.repeat(63)}.png | short`, // truncated hash + `upload:${UPLOAD_SET.toUpperCase()}/${shaOf('a')}.png`, // set id is lower-case base32 + `upload:${UPLOAD_SET}/${shaOf('A')}.png`, // hash is lower-case hex + `upload:../${shaOf('a')}.png`, + `upload:${UPLOAD_SET}/${shaOf('a')}.png/../x.png`, + `see upload:${UPLOAD_SET}/${shaOf('a')}.png`, + 'ftp://example.org/a.png', + ].join('\n'); + assert.deepEqual(parseImageRefs(malformed), []); +}); + +test('parseAttachmentRef reads a bare upload: reference', () => { + assert.deepEqual(parseAttachmentRef(uploadLine('f', 'pdf')), { upload: uploadRef('f', 'pdf'), name: '' }); + assert.deepEqual(parseAttachmentRef(`\n ${uploadLine('f', 'pdf')} \n`), { + upload: uploadRef('f', 'pdf'), + name: '', + }); + // A malformed reference is no attachment at all, like text with no link. + assert.equal(parseAttachmentRef(`upload:${UPLOAD_SET}/${shaOf('f')}.exe`), null); + assert.equal(parseAttachmentRef('upload:nothing'), null); + // A URL answer is unchanged. + assert.deepEqual(parseAttachmentRef('https://example.org/deck.pdf'), { + url: 'https://example.org/deck.pdf', + name: '', + }); +}); diff --git a/test/scripts/new_entry_scaffold.test.mjs b/test/scripts/new_entry_scaffold.test.mjs index 4481cae..a1e2902 100644 --- a/test/scripts/new_entry_scaffold.test.mjs +++ b/test/scripts/new_entry_scaffold.test.mjs @@ -10,15 +10,19 @@ */ import assert from 'node:assert/strict'; +import crypto from 'node:crypto'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import test from 'node:test'; import { spawnSync } from 'node:child_process'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; import * as yaml from 'js-yaml'; +import { formatUploadRef, uploadBranch, uploadFilename } from '../../scripts/lib/upload_refs.mjs'; +import { notConfiguredWarning, notFoundWarning } from '../../scripts/lib/uploads.mjs'; + const ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); const SCRIPT = path.join(ROOT, 'scripts', 'new_entry_from_issue.mjs'); const ISSUE_BODY = fs.readFileSync(path.join(ROOT, 'test', 'fixtures', 'issue-basic.md'), 'utf8'); @@ -312,3 +316,314 @@ test('a file or image question left blank is stored blank, not as a path no file ); assert.equal(dryRun(ROOT, attached)[file.key], `/${schema.entry.path}/${slug}/${file.filename}`); }); + +// --- files attached on /submit/ (`upload:` references) ---------------------- +// The real script, run end to end (not a dry run) in a throwaway checkout, with +// `fetch` and DNS answered by test/fixtures/scaffold-fetch-stub.mjs: GitHub +// attachment URLs, and a private uploads repository served the way the REST API +// serves a git database. Every request is logged, so the tests can see which +// credential went where. + +const STUB = pathToFileURL(path.join(ROOT, 'test', 'fixtures', 'scaffold-fetch-stub.mjs')).href; +const UPLOADS_TOKEN = 'ghs_uploadsTokenThatMustNeverLeak0123456789'; +const JOB_TOKEN = 'ghs_jobToken'; +const UPLOADS_REPO = 'example-org/catalog-uploads'; +const AUTHOR_ID = '4242'; +const UPLOAD_SET = 'abcdefghijklmnopqrstuvwxyz'; +const UPLOADS_ON = { UPLOADS_TOKEN, UPLOADS_REPO, ISSUE_AUTHOR_ID: AUTHOR_ID }; +// What the workflow passes when `vars.UPLOADS_REPO` is unset: the token step is +// skipped, so its output is empty, and the author id is always there. +const UPLOADS_OFF = { UPLOADS_TOKEN: '', UPLOADS_REPO: '', ISSUE_AUTHOR_ID: AUTHOR_ID }; + +const PNG_A = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0x0a]); +const PNG_URL = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0x0b]); +const JPG_B = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0x0b]); +const JPG_URL = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0x0c]); +const PDF = Buffer.from('%PDF-1.7 uploaded deck'); +const PDF_URL = Buffer.from('%PDF-1.7 attached deck'); + +const sha256 = (bytes) => crypto.createHash('sha256').update(bytes).digest('hex'); +const uploadOf = (bytes, ext) => ({ set: UPLOAD_SET, sha256: sha256(bytes), ext }); +const UP_A = uploadOf(PNG_A, 'png'); +const UP_B = uploadOf(JPG_B, 'jpg'); +const UP_DECK = uploadOf(PDF, 'pdf'); + +/** The uploads repository and the attachment URLs every run can reach. */ +const FETCH_PLAN = { + urls: { + 'https://github.com/user-attachments/assets/aaa.png': { + contentType: 'image/png', + base64: PNG_URL.toString('base64'), + }, + 'https://example.org/shot2.jpg': { contentType: 'image/jpeg', base64: JPG_URL.toString('base64') }, + 'https://github.com/user-attachments/files/12345678/deck.pdf': { + contentType: 'application/pdf', + base64: PDF_URL.toString('base64'), + }, + }, + uploads: { + repo: UPLOADS_REPO, + branches: Object.fromEntries( + [ + [UP_A, PNG_A], + [UP_B, JPG_B], + [UP_DECK, PDF], + ].map(([ref, bytes]) => [ + uploadBranch(AUTHOR_ID, ref), + { path: uploadFilename(ref), base64: bytes.toString('base64') }, + ]) + ), + }, +}; + +/** issue-basic.md with its screenshots and deck attached on /submit/ instead. */ +function uploadBody() { + const screenshots = [ + '![Daily brief](https://github.com/user-attachments/assets/aaa.png)\nhttps://example.org/shot2.jpg | The queue view', + [ + `${formatUploadRef(UP_A)} | Landing page with a Get Started button`, + 'https://github.com/user-attachments/assets/aaa.png | Daily brief', + `${formatUploadRef(UP_B)} | Results table`, + ].join('\n'), + ]; + const deck = [ + '[deck.pdf](https://github.com/user-attachments/files/12345678/deck.pdf)', + formatUploadRef(UP_DECK), + ]; + assert.ok( + ISSUE_BODY.includes(screenshots[0]) && ISSUE_BODY.includes(deck[0]), + 'the fixture still has both answers' + ); + return ISSUE_BODY.replace(screenshots[0], screenshots[1]).replace(deck[0], deck[1]); +} + +/** `$GITHUB_OUTPUT`'s heredoc pairs, as a plain object. */ +function readOutputs(file) { + const outputs = {}; + const text = fs.existsSync(file) ? fs.readFileSync(file, 'utf8') : ''; + for (const match of text.matchAll(/^([a-z_]+)<<(GHEOF_\S+)\n([\s\S]*?)\n\2$/gm)) + outputs[match[1]] = match[3]; + return outputs; +} + +/** + * Scaffold for real in a fresh checkout holding this repository's _data. + * @param {string} body issue body + * @param {Record} uploadsEnv + * @param {string} log request log shared across runs (each line one request) + */ +function scaffold( + body, + uploadsEnv, + log = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'scaffold-log-')), 'requests.jsonl') +) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'scaffold-uploads-')); + fs.mkdirSync(path.join(root, '_data')); + for (const name of ['schema.yml', 'governance.yml']) { + fs.copyFileSync(path.join(ROOT, '_data', name), path.join(root, '_data', name)); + } + const planFile = path.join(root, 'fetch-plan.json'); + fs.writeFileSync(planFile, JSON.stringify({ ...FETCH_PLAN, log })); + const outputFile = path.join(root, 'github-output'); + const env = Object.fromEntries( + Object.entries(process.env).filter( + ([key]) => !/^(GITHUB_TOKEN|GITHUB_OUTPUT|GITHUB_STEP_SUMMARY|UPLOADS_.*|ISSUE_AUTHOR_ID)$/.test(key) + ) + ); + const result = spawnSync(process.execPath, ['--import', STUB, SCRIPT], { + cwd: root, + env: { + ...env, + ...uploadsEnv, + ISSUE_BODY: body, + ISSUE_TITLE: '[Use case] Service request routing', + ISSUE_NUMBER: '7', + GITHUB_TOKEN: JOB_TOKEN, + GITHUB_OUTPUT: outputFile, + FETCH_STUB_PLAN: planFile, + }, + encoding: 'utf8', + }); + assert.equal(result.status, 0, result.stderr); + const outputs = readOutputs(outputFile); + const entryDir = path.join(root, outputs.entry_dir); + const page = fs.readFileSync(path.join(entryDir, 'index.md'), 'utf8'); + const files = Object.fromEntries( + fs + .readdirSync(entryDir, { recursive: true }) + .filter((name) => fs.statSync(path.join(entryDir, name)).isFile()) + .sort() + .map((name) => [name, fs.readFileSync(path.join(entryDir, name))]) + ); + const calls = fs.existsSync(log) + ? fs + .readFileSync(log, 'utf8') + .split('\n') + .filter(Boolean) + .map((line) => JSON.parse(line)) + : []; + fs.rmSync(root, { recursive: true, force: true }); + return { + ...result, + outputs, + page, + files, + calls, + frontMatter: yaml.load(/^---\n([\s\S]*?)\n---\n/.exec(page)[1]), + }; +} + +const imagesField = () => repoSchema().fields.find((f) => f.type === 'images'); +const fileField = () => repoSchema().fields.find((f) => f.type === 'file'); +const isUploadsApi = (call) => call.url.startsWith(`https://api.github.com/repos/${UPLOADS_REPO}/git/`); + +test('upload: screenshots and deck are saved into the entry exactly as attachments are', () => { + const images = imagesField(); + const file = fileField(); + const attached = scaffold(ISSUE_BODY, UPLOADS_ON); + const uploaded = scaffold(uploadBody(), UPLOADS_ON); + const slug = uploaded.frontMatter.slug; + const shots = `/${repoSchema().entry.path}/${slug}/screenshots`; + + assert.deepEqual(uploaded.frontMatter[images.key], [ + { src: `${shots}/01.png`, alt: 'Landing page with a Get Started button' }, + { src: `${shots}/02.png`, alt: 'Daily brief' }, + { src: `${shots}/03.jpg`, alt: 'Results table' }, + ]); + assert.deepEqual( + Object.keys(uploaded.frontMatter[images.key][0]), + Object.keys(attached.frontMatter[images.key][0]) + ); + // The deck names the same entry path a downloaded GitHub attachment does. + assert.equal(uploaded.frontMatter[file.key], attached.frontMatter[file.key]); + assert.equal(uploaded.frontMatter[file.key], `/${repoSchema().entry.path}/${slug}/${file.filename}`); + + assert.deepEqual(Object.keys(uploaded.files), [ + file.filename, + 'index.md', + 'screenshots/01.png', + 'screenshots/02.png', + 'screenshots/03.jpg', + ]); + // The same layout the attachment run produced: the deck beside the page, screenshots in their folder. + assert.deepEqual(Object.keys(attached.files), [ + file.filename, + 'index.md', + 'screenshots/01.png', + 'screenshots/02.jpg', + ]); + assert.ok(uploaded.files['screenshots/01.png'].equals(PNG_A)); + assert.ok(uploaded.files['screenshots/02.png'].equals(PNG_URL)); + assert.ok(uploaded.files['screenshots/03.jpg'].equals(JPG_B)); + assert.ok(uploaded.files[file.filename].equals(PDF)); + assert.equal(uploaded.outputs.warnings, ''); + assert.match(uploaded.stdout, /Files written: 5 \(1 page, 3 screenshots, 1 attachment\)/); +}); + +test('UPLOADS_TOKEN reaches the uploads API only, never the generic fetchers or any output', () => { + const run = scaffold(uploadBody(), UPLOADS_ON); + const api = run.calls.filter(isUploadsApi); + const generic = run.calls.filter((call) => !isUploadsApi(call)); + assert.equal(api.length, 12, 'ref, commit, tree and blob for each of the three references'); + for (const call of api) assert.equal(call.authorization, `Bearer ${UPLOADS_TOKEN}`, call.url); + // The generic fetchers attach their token to any GitHub host a submitter names. + assert.deepEqual(generic, [ + { + url: 'https://github.com/user-attachments/assets/aaa.png', + method: 'GET', + authorization: `Bearer ${JOB_TOKEN}`, + }, + ]); + for (const text of [run.stdout, run.stderr, run.page, JSON.stringify(run.outputs)]) { + assert.ok(!text.includes(UPLOADS_TOKEN)); + } +}); + +test('an issue edit scaffolds again: every reference is resolved anew, with reads only', () => { + const log = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'scaffold-log-')), 'requests.jsonl'); + const first = scaffold(uploadBody(), UPLOADS_ON, log); + const firstCount = first.calls.filter(isUploadsApi).length; + // The draft branch is rebuilt from main, so the second run starts from an empty folder. + const second = scaffold(uploadBody(), UPLOADS_ON, log); + assert.equal(firstCount, 12); + assert.equal(second.calls.filter(isUploadsApi).length, 24, 'the second run resolved all three again'); + assert.deepEqual(second.files, first.files); + // Nothing is ever written to or deleted from the uploads repository. + assert.deepEqual([...new Set(second.calls.map((call) => call.method))], ['GET']); +}); + +test('uploads not set up: upload: lines become a warning, and everything else still scaffolds', () => { + const images = imagesField(); + const file = fileField(); + const run = scaffold(uploadBody(), UPLOADS_OFF); + const slug = run.frontMatter.slug; + const entryPath = `/${repoSchema().entry.path}/${slug}`; + + assert.equal(run.calls.filter((call) => call.url.startsWith('https://api.github.com/')).length, 0); + assert.deepEqual(run.frontMatter[images.key], [ + { src: `${entryPath}/screenshots/01.png`, alt: 'Daily brief' }, + ]); + assert.equal(run.frontMatter.title, 'Service request routing assistant'); + // The deck gets what a GitHub attachment whose link cannot be kept gets. + assert.equal(run.frontMatter[file.key], `${entryPath}/${file.filename}`); + assert.deepEqual(Object.keys(run.files), ['index.md', 'screenshots/01.png']); + + const warnings = run.outputs.warnings; + assert.ok(warnings.includes(`- ${notConfiguredWarning(images.label, 2)}`), warnings); + assert.ok( + warnings.includes( + `- ${notConfiguredWarning(file.label, 1)} Commit the file as \`${entryPath}/${file.filename}\` in this pull request if it should be included.` + ), + warnings + ); + assert.match( + run.stderr, + /upload: references skipped: uploads are not configured \(UPLOADS_TOKEN, UPLOADS_REPO not set\)/ + ); +}); + +test('an upload that is gone is a not-found warning, and the rest still scaffolds', () => { + const images = imagesField(); + // A reference the uploads repository does not hold under this author. + const gone = { set: UPLOAD_SET, sha256: 'f'.repeat(64), ext: 'png' }; + const body = uploadBody().replace(formatUploadRef(UP_B), formatUploadRef(gone)); + const run = scaffold(body, UPLOADS_ON); + assert.equal(run.frontMatter[images.key].length, 2); + assert.ok(run.outputs.warnings.includes(`- ${notFoundWarning(images.label, gone)}`), run.outputs.warnings); +}); + +test('no upload: lines: the scaffold is identical whether uploads are set up or not', () => { + const off = scaffold(ISSUE_BODY, UPLOADS_OFF); + const on = scaffold(ISSUE_BODY, UPLOADS_ON); + const none = scaffold(ISSUE_BODY, {}); + for (const run of [off, on, none]) { + assert.equal(run.calls.filter((call) => call.url.startsWith('https://api.github.com/')).length, 0); + } + assert.equal(on.page, off.page); + assert.equal(none.page, off.page); + assert.deepEqual(on.files, off.files); + assert.deepEqual(none.outputs, off.outputs); + assert.deepEqual(on.outputs, off.outputs); + assert.equal(on.stdout, off.stdout); + assert.equal(off.stderr, ''); +}); + +test('a dry run lists upload: references and fetches nothing', () => { + const file = fileField(); + const stdout = dryRunOutput(ROOT, uploadBody()); + const fm = dryRun(ROOT, uploadBody()); + assert.equal(fm[imagesField().key].length, 3); + assert.equal(fm[file.key], `/${repoSchema().entry.path}/${fm.slug}/${file.filename}`); + const warnings = stdout.split('# warnings')[1]; + assert.ok(warnings.includes('- Dry run: 1 image URL(s) were parsed but not downloaded.'), warnings); + assert.ok( + warnings.includes( + `- Dry run: \`${formatUploadRef(UP_A)}\`, \`${formatUploadRef(UP_B)}\` parsed but not fetched.` + ), + warnings + ); + assert.ok( + warnings.includes(`- Dry run: \`${formatUploadRef(UP_DECK)}\` was parsed but not fetched.`), + warnings + ); +}); diff --git a/test/scripts/uploads.test.mjs b/test/scripts/uploads.test.mjs new file mode 100644 index 0000000..e037223 --- /dev/null +++ b/test/scripts/uploads.test.mjs @@ -0,0 +1,230 @@ +/** + * Reading `upload:` references out of the private uploads repository + * (scripts/lib/uploads.mjs). Every request goes to a stub; no network. + */ + +import assert from 'node:assert/strict'; +import crypto from 'node:crypto'; +import test from 'node:test'; + +import { uploadBranch } from '../../scripts/lib/upload_refs.mjs'; +import { notConfiguredWarning, notFoundWarning, uploadResolver } from '../../scripts/lib/uploads.mjs'; + +const TOKEN = 'ghs_uploadsTokenThatMustNeverLeak0123456789'; +const REPO = 'example-org/catalog-uploads'; +const AUTHOR = '123'; +const ENV = { UPLOADS_TOKEN: TOKEN, UPLOADS_REPO: REPO, ISSUE_AUTHOR_ID: AUTHOR }; +const SET = 'abcdefghijklmnopqrstuvwxyz'; + +const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 1, 2, 3, 4]); +const sha256 = (bytes) => crypto.createHash('sha256').update(bytes).digest('hex'); +const REF = { set: SET, sha256: sha256(PNG), ext: 'png' }; + +const COMMIT = 'c'.repeat(40); +const TREE = 'd'.repeat(40); +const BLOB = 'e'.repeat(40); + +function jsonResponse(body, status = 200) { + return { + ok: status >= 200 && status < 300, + status, + headers: { get: () => null }, + json: async () => body, + }; +} + +function bytesResponse(bytes, { stream = false } = {}) { + return { + ok: true, + status: 200, + headers: { get: () => null }, + arrayBuffer: async () => bytes.buffer.slice(bytes.byteOffset, bytes.byteOffset + bytes.byteLength), + ...(stream + ? { + body: (async function* chunks() { + for (let i = 0; i < bytes.length; i += 4) yield bytes.slice(i, i + 4); + })(), + } + : {}), + }; +} + +/** + * A stand-in for the GitHub REST API's git database holding one file on one + * branch. `overrides` replaces the answer to a step (`ref`, `commit`, `tree`, + * `blob`) with a status or a response. + */ +function fakeGitHub({ + branch = uploadBranch(AUTHOR, REF), + file = PNG, + entry = {}, + overrides = {}, + stream, +} = {}) { + const calls = []; + const base = `https://api.github.com/repos/${REPO}/git/`; + const answer = (step, make) => { + const override = overrides[step]; + if (typeof override === 'number') return jsonResponse({ message: 'x' }, override); + if (override) return override; + return make(); + }; + const fetchImpl = async (url, init) => { + calls.push({ url, init }); + const route = url.startsWith(base) ? url.slice(base.length) : ''; + if (route === `ref/heads/${branch}` || (route.startsWith('ref/heads/') && overrides.ref)) { + return answer('ref', () => jsonResponse({ object: { type: 'commit', sha: COMMIT } })); + } + if (route === `commits/${COMMIT}`) return answer('commit', () => jsonResponse({ tree: { sha: TREE } })); + if (route === `trees/${TREE}`) { + return answer('tree', () => + jsonResponse({ + tree: [{ path: `${REF.sha256}.png`, type: 'blob', sha: BLOB, size: file.length, ...entry }], + }) + ); + } + if (route === `blobs/${BLOB}`) return answer('blob', () => bytesResponse(file, { stream })); + return jsonResponse({ message: 'Not Found' }, 404); + }; + return { calls, fetchImpl }; +} + +const resolverFor = (github, env = ENV) => { + const resolver = uploadResolver(env, { fetchImpl: github.fetchImpl }); + assert.equal(resolver.ok, true, resolver.problem); + return resolver; +}; + +test('a reference resolves ref → commit → tree → blob, re-hashed, with the documented headers', async () => { + const github = fakeGitHub(); + const bytes = await resolverFor(github).resolve(REF, { maxBytes: 1024, field: 'Screenshots' }); + assert.deepEqual([...bytes], [...PNG]); + + const base = `https://api.github.com/repos/${REPO}/git/`; + assert.deepEqual( + github.calls.map((call) => call.url), + [ + `${base}ref/heads/u/${AUTHOR}/${SET}/${REF.sha256}`, + `${base}commits/${COMMIT}`, + `${base}trees/${TREE}`, + `${base}blobs/${BLOB}`, + ] + ); + for (const [index, { init }] of github.calls.entries()) { + assert.equal(init.method, 'GET'); + assert.equal(init.headers.authorization, `Bearer ${TOKEN}`); + assert.equal(init.headers['x-github-api-version'], '2022-11-28'); + assert.equal( + init.headers.accept, + index === 3 ? 'application/vnd.github.raw' : 'application/vnd.github+json' + ); + } +}); + +test('the ref is built from ISSUE_AUTHOR_ID alone, so another user’s upload is never found', async () => { + // The file exists, but under user 999. The issue author is 123, and nothing + // in the reference can name a user, so the request goes to 123's ref. + const github = fakeGitHub({ branch: uploadBranch('999', REF) }); + const result = await resolverFor(github).resolve(REF, { maxBytes: 1024, field: 'Screenshots' }); + assert.deepEqual(result, { warning: notFoundWarning('Screenshots', REF) }); + assert.equal(github.calls.length, 1); + assert.ok(github.calls[0].url.includes(`/ref/heads/u/${AUTHOR}/`), github.calls[0].url); + assert.ok(!github.calls[0].url.includes('/u/999/')); +}); + +test('the size cap is judged from the tree entry before the blob is fetched', async () => { + const github = fakeGitHub({ entry: { size: 5000 } }); + await assert.rejects(resolverFor(github).resolve(REF, { maxBytes: 1024 }), /larger than the .* size cap/); + assert.ok(!github.calls.some((call) => call.url.includes('/blobs/')), 'the blob was never requested'); +}); + +test('the size cap is enforced again while the blob streams', async () => { + // The tree claims a small file; the blob is bigger than it said. + const big = new Uint8Array(64).fill(7); + const github = fakeGitHub({ file: big, entry: { size: 8 }, stream: true }); + await assert.rejects(resolverFor(github).resolve(REF, { maxBytes: 16 }), /larger than the .* size cap/); +}); + +test('bytes that do not hash to the reference are rejected', async () => { + const tampered = PNG.slice(); + tampered[11] = 0xff; + const github = fakeGitHub({ file: tampered }); + await assert.rejects( + resolverFor(github).resolve(REF, { maxBytes: 1024 }), + /does not match its upload reference/ + ); +}); + +test('a tree entry that is not a blob is rejected without fetching it', async () => { + const github = fakeGitHub({ entry: { type: 'tree' } }); + await assert.rejects(resolverFor(github).resolve(REF, { maxBytes: 1024 }), /not a single file/); + assert.ok(!github.calls.some((call) => call.url.includes('/blobs/'))); +}); + +test('a 404 at any step, or a tree without the file, is “not found”, never a throw', async () => { + for (const step of ['ref', 'commit', 'tree', 'blob']) { + const github = fakeGitHub({ overrides: { [step]: 404 } }); + const result = await resolverFor(github).resolve(REF, { maxBytes: 1024, field: 'Deck' }); + assert.deepEqual(result, { warning: notFoundWarning('Deck', REF) }, step); + } + const missing = fakeGitHub({ entry: { path: 'other.png' } }); + const result = await resolverFor(missing).resolve(REF, { maxBytes: 1024, field: 'Deck' }); + assert.deepEqual(result, { warning: notFoundWarning('Deck', REF) }); +}); + +test('any other GitHub error is a throw that names the status', async () => { + const github = fakeGitHub({ overrides: { commit: 500 } }); + await assert.rejects(resolverFor(github).resolve(REF, { maxBytes: 1024 }), /GitHub answered HTTP 500/); +}); + +test('the token goes to api.github.com only and never into an error or a warning', async () => { + const hosts = new Set(); + const leaky = async (url) => { + hosts.add(new URL(url).host); + throw new Error(`connect failed with Bearer ${TOKEN}`); + }; + const resolver = uploadResolver(ENV, { fetchImpl: leaky }); + await assert.rejects(resolver.resolve(REF, { maxBytes: 1024 }), (error) => { + assert.ok(!error.message.includes(TOKEN), error.message); + assert.match(error.message, /\*\*\*/); + return true; + }); + assert.deepEqual([...hosts], ['api.github.com']); + + const github = fakeGitHub({ overrides: { ref: 404 } }); + const { warning } = await resolverFor(github).resolve(REF, { maxBytes: 1024, field: 'Screenshots' }); + assert.ok(!warning.includes(TOKEN)); + assert.ok(github.calls.every((call) => new URL(call.url).origin === 'https://api.github.com')); +}); + +test('missing or malformed settings mean “not set up”, and the problem never carries the token', () => { + for (const env of [ + {}, + { ...ENV, UPLOADS_TOKEN: ' ' }, + { ...ENV, UPLOADS_REPO: '' }, + { ...ENV, ISSUE_AUTHOR_ID: '' }, + { ...ENV, UPLOADS_REPO: 'no-slash' }, + { ...ENV, UPLOADS_REPO: 'owner/../x' }, + { ...ENV, UPLOADS_REPO: 'owner/..' }, + { ...ENV, UPLOADS_REPO: 'https://github.com/owner/name' }, + { ...ENV, ISSUE_AUTHOR_ID: 'octocat' }, + { ...ENV, ISSUE_AUTHOR_ID: '0' }, + ]) { + const resolver = uploadResolver(env, { fetchImpl: () => assert.fail('no request without settings') }); + assert.equal(resolver.ok, false, JSON.stringify(env)); + assert.ok(!resolver.problem.includes(TOKEN)); + } +}); + +test('the warnings speak to the submitter, in the singular and the plural', () => { + assert.equal( + notConfiguredWarning('Slide deck', 1), + 'A file attached on the submit page for “Slide deck” could not be collected, because file uploads are not set up on this catalog. To include it, edit this issue and drag the file into the “Slide deck” answer, in place of the line that starts with `upload:`.' + ); + assert.match( + notConfiguredWarning('Screenshots', 2), + /^2 files attached .* drag the files .* lines that start with/ + ); + assert.match(notFoundWarning('Screenshots', REF), /could not be found — .* may have expired/); + assert.ok(notFoundWarning('Screenshots', REF).includes(`upload:${SET}/${REF.sha256}.png`)); +}); From 35a8f0ef07629c6b06547faa9bb790b2e2e43177 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:24:04 -0500 Subject: [PATCH 06/17] feat(uploads): attach files on the submit page With submit.uploads_url set (and sign-in on), signed-in submitters get a picker on the screenshots and file questions. Images are re-encoded in the browser, each file uploads as it is picked, the draft remembers what was uploaded, and the issue body carries upload: references. Blank uploads_url leaves the page exactly as it was; validate.mjs rejects uploads_url without signin_url. Co-Authored-By: Claude Opus 5.5 --- _data/site.yml | 6 + assets/js/configurator/defaults.generated.js | 3 +- assets/js/submit.js | 30 +- assets/js/submit/direct.js | 47 +- assets/js/submit/draft.js | 47 +- assets/js/submit/handoff.js | 27 +- assets/js/submit/review.js | 27 +- assets/js/submit/uploads-core.js | 308 ++++++ assets/js/submit/uploads.js | 892 +++++++++++++++ scripts/lib/uploads_url.mjs | 31 + scripts/validate.mjs | 15 +- submit/index.md | 21 +- test/scripts/submit_uploads.test.mjs | 1036 ++++++++++++++++++ test/scripts/uploads_url.test.mjs | 75 ++ 14 files changed, 2537 insertions(+), 28 deletions(-) create mode 100644 assets/js/submit/uploads-core.js create mode 100644 assets/js/submit/uploads.js create mode 100644 scripts/lib/uploads_url.mjs create mode 100644 test/scripts/submit_uploads.test.mjs create mode 100644 test/scripts/uploads_url.test.mjs diff --git a/_data/site.yml b/_data/site.yml index 03a857e..978d281 100644 --- a/_data/site.yml +++ b/_data/site.yml @@ -123,6 +123,12 @@ submit: # answers as an issue straight from /submit/, however long. Blank keeps the # link hand-off only. See docs/configuration.md. signin_url: "" + # Optional file uploads on /submit/: the address of the upload service you + # deploy from services/uploads/ (e.g. https://catalog-uploads.vercel.app). + # Signed-in submitters can then attach screenshots and a PDF deck right on + # the page. Needs signin_url, because uploads work only for signed-in + # submitters. Blank turns it off. See docs/configuration.md. + uploads_url: "" # "Check your submission" page (module: status). A submitter types the number # GitHub gave their submission and sees its title, date and stage, read from diff --git a/assets/js/configurator/defaults.generated.js b/assets/js/configurator/defaults.generated.js index 2b5335a..e20afbc 100644 --- a/assets/js/configurator/defaults.generated.js +++ b/assets/js/configurator/defaults.generated.js @@ -96,7 +96,8 @@ export const SITE = { "turnaround": "Intake checks it within about five business days and the Governance Committee reviews it within about ten more; you keep ownership of anything you share.", "review_note": "Please do not include protected health information, credentials or non-public data. Link out to repositories and documents rather than pasting sensitive content.", "fallback_email": "catalog@example.org", - "signin_url": "" + "signin_url": "", + "uploads_url": "" }, "status": { "heading": "Check your submission", diff --git a/assets/js/submit.js b/assets/js/submit.js index a1ce4f0..283994b 100644 --- a/assets/js/submit.js +++ b/assets/js/submit.js @@ -1,7 +1,8 @@ /** * Submit form — entry point. Wires the pieces in assets/js/submit/*.js: * fields (registry) → validate → repeatable → preview → draft → handoff → - * review, and direct (Sign in with GitHub) when the site has it on. + * review, and direct (Sign in with GitHub) and uploads (file pickers for + * signed-in submitters) when the site has them on. * * Loaded last by the `scripts:` list in submit/index.md front matter, so every * window.SubmitForm helper it calls is already defined. @@ -241,10 +242,15 @@ draft.save(); } + // File uploads (assets/js/submit/uploads.js), set up below beside the + // sign-in they depend on; a restored draft hands its record of them over. + let uploads = null; + draft = ns.initDraft( form, fields, - function afterRestore(ui) { + function afterRestore(ui, uploaded) { + if (uploads) uploads.restore(uploaded); fields.filter((field) => field.type === 'images').forEach(ns.renderImagePreviews); // Visibility before position: the short-form recompute decides which // sections still have anything to show before the stepper lands on one. @@ -426,6 +432,13 @@ // it can — the link hand-off is the way. const direct = titleField && typeof ns.initDirect === 'function' ? ns.initDirect(form) : null; + // Only with sign-in, and the service's address: submit/index.md renders + // data-uploads-url and uploads.js together, and only with signin_url set. + uploads = + direct && form.dataset.uploadsUrl && typeof ns.initUploads === 'function' + ? ns.initUploads({ form: form, fields: fields, direct: direct, draft: draft, onChange: refresh }) + : null; + /** * Send the answers straight to GitHub as the signed-in submitter. The * draft is deleted only once GitHub answers 201 — every other outcome @@ -433,6 +446,12 @@ * @param {HTMLButtonElement} button held while the request is out */ function sendDirect(button) { + // Counted before the draft is cleared: the panel says where to drag the + // file for each file question that has no upload. + const uploaded = uploads ? uploads.count() : 0; + const fileLabels = fields + .filter((field) => field.type === 'file' && !(uploads && uploads.hasFiles(field))) + .map((field) => field.label); direct.submit({ button: button, status: form.querySelector('[data-review-status]'), @@ -442,11 +461,8 @@ draft.clear(); countEvent('Send to GitHub'); say(status, ''); - ns.renderSubmitted( - form, - issue, - fields.filter((field) => field.type === 'file').map((field) => field.label) - ); + if (uploads) uploads.reset(); + ns.renderSubmitted(form, issue, fileLabels, uploaded); }, onFallback: send, }); diff --git a/assets/js/submit/direct.js b/assets/js/submit/direct.js index 68b8acd..ebfb5cd 100644 --- a/assets/js/submit/direct.js +++ b/assets/js/submit/direct.js @@ -29,7 +29,13 @@ * [data-direct-copy] (the intro's two versions of the last step, one shown); * [data-review-status] (rendered by review.js when `handlers.direct` is set). * - * Exposes: window.SubmitForm.initDirect + * The object initDirect returns also lets assets/js/submit/uploads.js (file + * uploads, only when `submit.uploads_url` is set) use the sign-in: `token()` + * hands out the bearer token for a request to the upload service (never to be + * stored or logged), `userId()` / `login()` say whose it is, `onChange(fn)` + * reports every sign-in state change, and `expire()` treats the token as dead. + * + * Exposes: window.SubmitForm.initDirect, .serviceOrigin */ (function (ns) { 'use strict'; @@ -65,10 +71,14 @@ return url.protocol === 'https:' || local ? url.origin : ''; } + ns.serviceOrigin = serviceOrigin; + /** * Wire "Sign in with GitHub" into the page. * @param {HTMLFormElement} form - * @returns {null|{signedIn: () => boolean, login: () => string, submit: (options: object) => void}} + * @returns {null|{signedIn: () => boolean, login: () => string, userId: () => number|null, + * token: () => string|null, onChange: (fn: () => void) => void, expire: () => void, + * submit: (options: object) => void}} * null when the site has no sign-in service (or no repository to send to) */ ns.initDirect = function initDirect(form) { @@ -86,6 +96,10 @@ /** Fallback when sessionStorage is refused: the token lives as long as the page. */ let memory = null; let login = ''; + /** GitHub's numeric id for the signed-in account, once GitHub has been asked. */ + let userId = null; + /** Callbacks told whenever the sign-in state is repainted. */ + const listeners = []; /** The sign-in window in progress: {popup, resolve, timer}. */ let pending = null; let inFlight = false; @@ -123,6 +137,7 @@ function clearToken() { memory = null; login = ''; + userId = null; try { window.sessionStorage.removeItem(storeKey); } catch { @@ -160,6 +175,18 @@ /** @param {'out'|'checking'|'in'} state */ function paint(state) { + paintControls(state); + listeners.forEach((listener) => { + try { + listener(); + } catch { + // A listener's trouble is not the sign-in's. + } + }); + } + + /** @param {'out'|'checking'|'in'} state */ + function paintControls(state) { // The intro's "what happens" steps, outside any template: say which // route a press of the last button will take. const signed = state !== 'out'; @@ -229,12 +256,14 @@ } return response.json().then((user) => { login = user && LOGIN_PATTERN.test(String(user.login)) ? String(user.login) : ''; + userId = user && Number.isInteger(user.id) && user.id > 0 ? user.id : null; paint('in'); }); }) .catch(() => { // GitHub out of reach: keep the token, and say only that it is there. login = ''; + userId = null; paint(readToken() ? 'in' : 'out'); }); } @@ -561,6 +590,20 @@ return { signedIn: () => readToken() !== null, login: () => login, + userId: () => (readToken() !== null ? userId : null), + token: () => { + const saved = readToken(); + return saved ? saved.token : null; + }, + onChange: (listener) => { + listeners.push(listener); + }, + // A service that took the token said it is no longer good. + expire: () => { + clearToken(); + paint('out'); + sayNote('Your GitHub sign-in has expired. Sign in again to submit from this page.'); + }, submit: submit, }; }; diff --git a/assets/js/submit/draft.js b/assets/js/submit/draft.js index f47a048..1ba2181 100644 --- a/assets/js/submit/draft.js +++ b/assets/js/submit/draft.js @@ -27,7 +27,10 @@ * Bump the version segment when the stored shape changes *incompatibly*. * The optional `ui` key (stepper position, short-form mode) rode in without * a bump on purpose: readers on either side ignore keys they don't know, so - * a bump would only have thrown away everyone's saved answers. + * a bump would only have thrown away everyone's saved answers. So did the + * optional `uploads` key (files attached through assets/js/submit/uploads.js): + * `{set, user: {id, login}, files: {: [{ref, name, alt, bytes, kind}]}}`. + * It holds references only — never a sign-in token, never file contents. */ const KEY_PREFIX = 'catalog-template:submit-draft:v2'; const DEBOUNCE_MS = 500; @@ -87,11 +90,16 @@ * typed answers are still not in storage — the one case worth a warning. * @param {HTMLFormElement} form * @param {object[]} fields descriptors from readFields - * @param {(ui: object|null) => void} onRestore called after a draft is - * written into the form, with the draft's saved UI state (or null) + * @param {(ui: object|null, uploads: object|null) => void} onRestore called + * after a draft is written into the form, with the draft's saved UI state + * and uploaded-files record (or null) * @param {(() => object|null)|undefined} uiState reads the UI state (stepper * position, short-form mode) to store beside the answers - * @returns {{save: Function, clear: Function, flush: Function, unsaved: () => boolean}} + * @returns {{save: Function, clear: Function, flush: Function, unsaved: () => boolean, + * trackUploads: (read: () => object|null) => void}} + * `trackUploads` registers the reader of the uploaded-files record stored + * beside the answers (null for none); a draft restored later hands the + * record it holds to `onRestore` as its second argument. */ ns.initDraft = function initDraft(form, fields, onRestore, uiState) { const key = storageKey(form); @@ -124,6 +132,16 @@ // True after the kept draft is restored and until its answers are saved // under `key`: until then `:previous` is still the only stored copy. let dropKept = false; + // Reads the uploaded-files record to store beside the answers; see trackUploads. + let uploadsState = null; + + /** @returns {object|null} the uploaded-files record, when it names any file */ + function uploadsRecord() { + const record = uploadsState ? uploadsState() : null; + if (!record || !record.files) return null; + const any = Object.keys(record.files).some((name) => record.files[name].length > 0); + return any ? record : null; + } /** @returns {object} the current answers, keyed by schema key */ function snapshot() { @@ -183,7 +201,8 @@ timer = null; if (held) return; const data = snapshot(); - const empty = Object.keys(data).length === 0; + const uploads = uploadsRecord(); + const empty = Object.keys(data).length === 0 && !uploads; if (!memory) { dirty = !empty; return; @@ -206,6 +225,7 @@ const payload = { saved: stamp, fields: data }; const ui = uiState ? uiState() : null; if (ui) payload.ui = ui; + if (uploads) payload.uploads = uploads; // A failed setItem leaves the key as it was, so the offered draft stays // under `key` until the new answers have actually replaced it; the copy // under `:previous` is undone if they have not. @@ -282,7 +302,7 @@ /** * @param {string} from storage key - * @returns {{saved: string, fields: object, ui: object|null}|null} the stored draft, if any + * @returns {{saved: string, fields: object, ui: object|null, uploads: object|null}|null} the stored draft, if any */ function read(from) { if (!memory) return null; @@ -293,6 +313,7 @@ saved: String(parsed.saved || ''), fields: parsed.fields, ui: parsed.ui && typeof parsed.ui === 'object' ? parsed.ui : null, + uploads: parsed.uploads && typeof parsed.uploads === 'object' ? parsed.uploads : null, }; } catch (error) { return null; @@ -326,7 +347,7 @@ offered = ''; if (bar) bar.hidden = true; setStatus(message); - onRestore(draft.ui); + onRestore(draft.ui, draft.uploads); savedAt = ''; save(); } @@ -398,7 +419,7 @@ // After the caller has finished starting up: the answers are in place // for its controllers to read, but the stepper and short-form mode the // saved UI state moves do not exist until initDraft has returned. - Promise.resolve().then(() => onRestore(existing.ui)); + Promise.resolve().then(() => onRestore(existing.ui, existing.uploads)); } else if (existing) offerRestore(existing, key); else if (kept) offerRestore(kept, previousKey); } @@ -475,6 +496,14 @@ } }); - return { save: save, clear: clear, flush: flush, unsaved: unsaved }; + return { + save: save, + clear: clear, + flush: flush, + unsaved: unsaved, + trackUploads: function trackUploads(read) { + uploadsState = read; + }, + }; }; })((window.SubmitForm = window.SubmitForm || {})); diff --git a/assets/js/submit/handoff.js b/assets/js/submit/handoff.js index 6188a73..6f3283b 100644 --- a/assets/js/submit/handoff.js +++ b/assets/js/submit/handoff.js @@ -17,6 +17,13 @@ * assets/js/configurator/issue-form-ids.js, the same helper the generator * names the form's elements with. * + * Files attached through assets/js/submit/uploads.js (a signed-in submitter, + * `submit.uploads_url` set) are written into the two bodies — never into the + * prefilled URL, which is the route for submitters who are not signed in — + * as `upload:/. | ` lines in a screenshots + * question and a bare `upload:…` reference in a `file` question, where they + * replace any typed link. The scaffolder resolves them (scripts/lib/upload_refs.mjs). + * * Exposes: window.SubmitForm.issueUrl, .markdownBody, .issueFormBody, .yamlFrontMatter, * .mailtoUrl, .unprefillable */ @@ -34,6 +41,20 @@ .filter((entry) => entry.text !== ''); } + /** + * A field's answer as the issue body carries it: its attached files first + * (see the header), then what was typed. An attached file replaces a typed + * answer in a `file` question. + * @param {object} field + * @returns {string} '' when there is nothing to say + */ + function bodyText(field) { + const lines = typeof ns.uploadLines === 'function' ? ns.uploadLines(field) : []; + if (lines.length === 0) return ns.serialize(field); + if (field.type === 'file') return lines[0]; + return lines.concat(ns.serialize(field) === '' ? [] : [ns.serialize(field)]).join('\n'); + } + /** * Fields the submitter will have to re-answer on GitHub. * @param {object[]} fields @@ -69,7 +90,9 @@ * @returns {string} */ ns.markdownBody = function markdownBody(fields) { - return answered(fields) + return fields + .map((field) => ({ field: field, text: bodyText(field) })) + .filter((entry) => entry.text !== '') .map((entry) => '### ' + entry.field.label + '\n\n' + entry.text + '\n') .join('\n'); }; @@ -87,7 +110,7 @@ */ ns.issueFormBody = function issueFormBody(fields) { return fields - .map((field) => '### ' + field.label + '\n\n' + (ns.serialize(field) || '_No response_')) + .map((field) => '### ' + field.label + '\n\n' + (bodyText(field) || '_No response_')) .join('\n\n'); }; diff --git a/assets/js/submit/review.js b/assets/js/submit/review.js index 3df3300..805e0c1 100644 --- a/assets/js/submit/review.js +++ b/assets/js/submit/review.js @@ -160,7 +160,10 @@ * @returns {HTMLElement[]} a
/
pair */ function answerRow(field, change) { - const text = ns.serialize(field); + // Files attached on this page (assets/js/submit/uploads.js) read back by + // name and description, ahead of anything typed. + const files = typeof ns.uploadSummary === 'function' ? ns.uploadSummary(field) : []; + const text = files.concat(ns.serialize(field) || []).join('\n'); const term = el('dt', { class: 'text-sm font-medium text-brand-ink sm:col-span-4' }, [ document.createTextNode(field.question || field.label), ]); @@ -504,10 +507,12 @@ * @param {{number: number|null, url: string}} issue as GitHub returned it * (no number when GitHub's answer lacked one: the link is then the * submitter's own issues) - * @param {string[]} [fileLabels] labels of the form's `file` questions: - * nothing can be attached from this page, so the panel says where to + * @param {string[]} [fileLabels] labels of the form's `file` questions that + * have no uploaded file: nothing else can be attached from this page, so + * the panel says where to. A question with an upload is left out. + * @param {number} [uploaded] how many files were sent from this page */ - ns.renderSubmitted = function renderSubmitted(form, issue, fileLabels) { + ns.renderSubmitted = function renderSubmitted(form, issue, fileLabels, uploaded) { if (!panelHost(form)) return; const named = issue.number ? 'issue #' + issue.number : 'an issue'; const heading = el('h2', { @@ -536,6 +541,20 @@ text: issue.number ? 'View #' + issue.number + ' on GitHub' : 'View your issues on GitHub', }), ]), + ...(uploaded > 0 + ? [ + el('div', { class: 'border-t border-brand-line px-6 py-4' }, [ + el('p', { + class: 'text-sm text-brand-ink', + text: + (uploaded === 1 + ? 'Your uploaded file went with it' + : 'Your ' + uploaded + ' uploaded files went with it') + + '. A reviewer checks every file before anything is published.', + }), + ]), + ] + : []), ...(fileLabels && fileLabels.length ? [ el('div', { class: 'border-t border-brand-line px-6 py-4' }, [ diff --git a/assets/js/submit/uploads-core.js b/assets/js/submit/uploads-core.js new file mode 100644 index 0000000..5672d7a --- /dev/null +++ b/assets/js/submit/uploads-core.js @@ -0,0 +1,308 @@ +/** + * Submit form — file uploads, the parts that need no page: the limits, the + * decisions about an image's size and format, the shape of a stored record, + * and the two things that touch the browser (re-encoding a picture, talking to + * the upload service). assets/js/submit/uploads.js builds the pickers on top. + * + * Only loaded when `submit.uploads_url` is set (submit/index.md), just before + * uploads.js. The rules here mirror scripts/lib/upload_refs.mjs (the shared + * `upload:/.` reference) and services/uploads/ (the limits); + * these scripts are classic ` +{%- if uploads_url != '' %} +{%- comment -%} Also before the front-matter scripts: it only defines window.SubmitForm.initUploads, which assets/js/submit.js calls once sign-in is wired. {%- endcomment %} + + +{%- endif -%} {%- endif -%} {%- endif -%} diff --git a/test/scripts/submit_uploads.test.mjs b/test/scripts/submit_uploads.test.mjs new file mode 100644 index 0000000..dbf3cfa --- /dev/null +++ b/test/scripts/submit_uploads.test.mjs @@ -0,0 +1,1036 @@ +/** + * File uploads on the submit page (assets/js/submit/uploads.js and + * uploads-core.js), driven through a real DOM. + * + * Same harness as submit_direct.test.mjs: the Liquid snapshot of /submit/ with + * the markup submit/index.md adds when `submit.signin_url` and + * `submit.uploads_url` are set laid over it (`data-signin-url`, + * `data-uploads-url`), and the shipped scripts in the order the page loads + * them. GitHub's API and the upload service are stand-ins; so are the canvas + * re-encode and the sleep between retries, which JSDOM cannot do (injected + * through `window.SubmitForm.uploadHooks`, the seam uploads.js documents). + */ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { JSDOM } from 'jsdom'; +import * as jsYaml from 'js-yaml'; + +const ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); +const read = (rel) => fs.readFileSync(path.join(ROOT, rel), 'utf8'); +const FIXTURE = read('test/fixtures/submit-form.html'); +const INDEX = read('submit/index.md'); +const SCRIPTS = jsYaml.load(INDEX.split(/^---$/m)[1]).scripts.map((src) => src.replace(/^\//, '')); +const DIRECT = read('assets/js/submit/direct.js'); +const CORE = read('assets/js/submit/uploads-core.js'); +const UPLOADS = read('assets/js/submit/uploads.js'); + +const SIGNIN = 'https://signin.example'; +const UPLOADS_URL = 'https://uploads.example'; +const PAGE = 'https://example.org/submit/'; +const REPO = 'crypticpy/phct'; +const TOKEN = 'ghu_' + 'a'.repeat(36); +const STORE = 'phct:github-signin:v1:' + REPO + '@' + SIGNIN; +const ISSUES_URL = 'https://api.github.com/repos/' + REPO + '/issues'; +const DRAFT_KEY = 'catalog-template:submit-draft:v2:' + /data-draft-key="([^"]+)"/.exec(FIXTURE)[1]; +const SET_RE = /^[a-z2-7]{26}$/; +const REF_RE = /^upload:[a-z2-7]{26}\/[0-9a-f]{64}\.(png|jpg|webp|pdf)$/; + +const NOTICE = + '

Sign in first.

' + + '
  1. Press Submit new issue on GitHub
'; +const HANDOFF_STEP = + '
  • 1. Sending opens a GitHub issue with your answers already filled in. Nothing is submitted until you press Submit new issue there.
    • '; + +/** The fixture as a site with the given switches renders it. */ +function page({ signin = true, uploads = true } = {}) { + assert.ok(FIXTURE.includes(HANDOFF_STEP), 'the fixture no longer has the hand-off step'); + const attrs = + (signin ? ' data-signin-url="' + SIGNIN + '"' : '') + + (uploads ? ' data-uploads-url="' + UPLOADS_URL + '"' : ''); + return ( + NOTICE + + FIXTURE.replace(' data-submit-form=""', ' data-submit-form=""' + attrs).replace( + HANDOFF_STEP, + '
  • 1. Sending opens a GitHub issue.
    • ' + ) + ); +} + +const booted = []; +test.after(() => booted.forEach((dom) => dom.window.close())); + +const flush = () => + new Promise((resolve) => setTimeout(resolve, 0)).then(() => new Promise((r) => setTimeout(r, 0))); +/** Let chains of awaited stand-ins (re-encode, send, sleep) run to the end. */ +async function settle() { + for (let i = 0; i < 8; i += 1) await flush(); +} +const settleDraft = () => new Promise((resolve) => setTimeout(resolve, 700)); + +function reply(status, data, retryAfter = null) { + return { status, retryAfter, data }; +} + +/** A fixed 64-hex digest per upload, so refs are predictable. */ +const digest = (n) => + String(n) + .padStart(64, '0') + .replace(/0/g, (_, i) => (i % 2 ? 'a' : 'b')); + +/** + * A booted submit page. + * @param {object} [options] + * @param {boolean} [options.signin=true] data-signin-url on the form + * @param {boolean} [options.uploads=true] data-uploads-url and the upload scripts + * @param {boolean} [options.signedIn=true] a token in sessionStorage + * @param {object} [options.user] what GET /user answers + * @param {string} [options.draft] raw localStorage draft to start from + * @param {(request: object, ctx: object) => any} [options.send] the upload service + * @param {(file: File) => any} [options.reencode] the canvas + */ +async function boot(options = {}) { + const dom = new JSDOM('' + page(options) + '', { + url: PAGE, + runScripts: 'outside-only', + }); + booted.push(dom); + const { window } = dom; + const ctx = { + window, + document: window.document, + requests: [], + uploadsSent: [], + sleeps: [], + reencoded: [], + count: 0, + }; + + const send = async (request) => { + ctx.uploadsSent.push(request); + if (options.send) return options.send(request, ctx); + if (request.method === 'DELETE') return reply(204, null); + ctx.count += 1; + const kind = request.headers['X-Upload-Kind']; + const ext = kind === 'pdf' ? 'pdf' : 'png'; + const sha = digest(ctx.count); + return reply(201, { + ref: 'upload:' + request.headers['X-Upload-Set'] + '/' + sha + '.' + ext, + sha256: sha, + bytes: request.body.size, + ext, + }); + }; + window.SubmitForm = { + uploadHooks: { + send, + sleep: async (ms) => { + ctx.sleeps.push(ms); + }, + reencode: async (file) => { + if (options.reencode) return options.reencode(file); + ctx.reencoded.push(file); + return { blob: new window.Blob(['re-encoded ' + file.name], { type: file.type }), type: file.type }; + }, + }, + }; + + window.open = () => null; + window.fetch = (url, init = {}) => { + ctx.requests.push({ url, init }); + if (url === 'https://api.github.com/user') + return Promise.resolve({ + status: 200, + ok: true, + json: () => Promise.resolve(options.user || { login: 'octo-cat', id: 1001 }), + }); + if (url === ISSUES_URL && init.method === 'POST') + return Promise.resolve({ + status: 201, + ok: true, + json: () => Promise.resolve({ number: 42, html_url: 'https://github.com/' + REPO + '/issues/42' }), + }); + return Promise.reject(new TypeError('unexpected request to ' + url)); + }; + if (options.signedIn !== false) + window.sessionStorage.setItem(STORE, JSON.stringify({ token: TOKEN, expiresAt: Date.now() + 3600000 })); + if (options.draft) window.localStorage.setItem(DRAFT_KEY, options.draft); + + await new Promise((resolve) => { + if (window.document.readyState === 'complete') resolve(); + else window.addEventListener('load', resolve); + }); + ctx.form = window.document.querySelector('[data-submit-form]'); + ctx.rawDraft = () => window.localStorage.getItem(DRAFT_KEY); + ctx.storedDraft = () => JSON.parse(ctx.rawDraft() || 'null'); + ctx.live = () => ctx.form.querySelector('[data-uploads-live]'); + + if (options.signin !== false) window.eval(DIRECT); + if (options.uploads !== false) { + window.eval(CORE); + window.eval(UPLOADS); + } + SCRIPTS.forEach((rel) => window.eval(read(rel))); + await settle(); + ctx.fields = window.SubmitForm.readFields(ctx.form); + return ctx; +} + +const wrapOf = (ctx, key) => ctx.form.querySelector('[data-field="' + key + '"]'); +const pickerOf = (ctx, key) => wrapOf(ctx, key).querySelector('input[type="file"]'); +const rowsOf = (ctx, key) => Array.from(wrapOf(ctx, key).querySelectorAll('[data-upload-item]')); + +/** Pick files the way the browser reports them, then let the uploads run. */ +async function pick(ctx, key, files) { + const input = pickerOf(ctx, key); + assert.ok(input, 'no file picker for ' + key); + Object.defineProperty(input, 'files', { value: files, configurable: true }); + input.dispatchEvent(new ctx.window.Event('change', { bubbles: true })); + await settle(); +} + +const file = (ctx, name, type, size = 10) => new ctx.window.File([new Uint8Array(size)], name, { type }); +const png = (ctx, name = 'shot.png') => file(ctx, name, 'image/png'); +const pdf = (ctx, name = 'deck.pdf', size = 10) => file(ctx, name, 'application/pdf', size); + +function describe(ctx, key, index, alt) { + const input = rowsOf(ctx, key)[index].querySelector('[data-upload-alt]'); + input.value = alt; + input.dispatchEvent(new ctx.window.Event('input', { bubbles: true })); +} + +function answer(ctx, key, value) { + // The screenshots question’s typed addresses live in its textarea; its file + // picker is an input too. + const control = wrapOf(ctx, key).querySelector(key === 'screenshots' ? 'textarea' : 'input, textarea'); + control.value = value; + control.dispatchEvent(new ctx.window.Event('input', { bubbles: true })); +} + +function fillRequired(ctx) { + ctx.form.querySelectorAll('[data-required="true"]').forEach((wrap) => { + const type = wrap.dataset.type; + const key = wrap.dataset.field; + if (type === 'select' || type === 'multiselect') { + const input = wrap.querySelector('input[value]:not([data-clear])'); + if (input) { + input.checked = true; + input.dispatchEvent(new ctx.window.Event('change', { bubbles: true })); + } else { + const select = wrap.querySelector('select'); + select.value = Array.from(select.options).find((o) => o.value !== '').value; + select.dispatchEvent(new ctx.window.Event('change', { bubbles: true })); + } + } else if (type === 'boolean') { + const box = wrap.querySelector('input[type="checkbox"]'); + box.checked = true; + box.dispatchEvent(new ctx.window.Event('change', { bubbles: true })); + } else if (type === 'email') answer(ctx, key, 'someone@example.org'); + else if (type === 'url') answer(ctx, key, 'https://example.org'); + else if (type !== 'file' && type !== 'images') answer(ctx, key, 'Value for ' + key); + }); + answer(ctx, 'title', 'Water routing'); +} + +/** Press through the steps; true when the review panel opened. */ +function tryReview(ctx) { + const cap = ctx.form.querySelectorAll('[data-section]').length + 1; + for (let i = 0; i < cap; i += 1) { + ctx.form.dispatchEvent(new ctx.window.Event('submit', { bubbles: true, cancelable: true })); + if (!ctx.form.querySelector('[data-review]').hidden) return true; + } + return false; +} + +const summaryText = (ctx) => ctx.form.querySelector('[data-error-summary]').textContent; + +/** Drive the review panel's send button. */ +async function submitToGitHub(ctx) { + const button = Array.from(ctx.form.querySelectorAll('[data-review] button')).find( + (b) => b.textContent.trim() === 'Submit to GitHub' + ); + assert.ok(button, 'no Submit to GitHub button'); + button.click(); + await settle(); +} + +/* ------------------------------------------------------------- off */ + +test('feature off: no pickers, no script, no data attribute — in the markup and at runtime', async () => { + // The Liquid: the attribute and both scripts exist only inside the guard, + // and the guard needs a sign-in service first (the signin_url pattern). + assert.match( + INDEX, + /\{%- assign uploads_url = '' -%\}\n\{%- if signin_url != '' -%\}\{%- assign uploads_url = cfg\.submit\.uploads_url \| default: '' \| strip -%\}\{%- endif -%\}/ + ); + assert.match( + INDEX, + /\{% if uploads_url != '' %\}\s*data-uploads-url="\{\{ uploads_url \| escape \}\}"\{% endif %\}/ + ); + const scripts = INDEX.split('\n').filter( + (line) => /uploads(-core)?\.js/.test(line) && line.includes(' block.includes(line)), + 'an uploads script sits outside the uploads_url guard' + ); + assert.ok( + !SCRIPTS.some((src) => /uploads/.test(src)), + 'the uploads scripts must not be in the always-on list' + ); + assert.match(read('_data/site.yml'), /^ {2}uploads_url: ""/m, 'uploads must be off by default'); + assert.doesNotMatch(FIXTURE, /uploads/i, 'the off-by-default snapshot mentions uploads'); + + // At runtime: signed in, but no uploads script and no attribute. + const ctx = await boot({ uploads: false }); + assert.equal(ctx.form.dataset.uploadsUrl, undefined); + assert.equal(typeof ctx.window.SubmitForm.initUploads, 'undefined'); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); + assert.equal(ctx.form.querySelector('[data-uploads]'), null); + assert.equal(ctx.form.querySelector('[data-uploads-live]'), null); +}); + +test('data-uploads-url without a sign-in service does nothing (initUploads is only called with direct)', async () => { + const ctx = await boot({ signin: false }); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); +}); + +test('signed out: no pickers at all', async () => { + const ctx = await boot({ signedIn: false }); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); + assert.equal(ctx.form.querySelector('[data-uploads]:not([hidden])'), null); + assert.equal(ctx.uploadsSent.length, 0); +}); + +test('signed in: a picker for every screenshots question and every file question', async () => { + const ctx = await boot(); + const images = ctx.fields.filter((f) => f.type === 'images'); + const files = ctx.fields.filter((f) => f.type === 'file'); + assert.ok(images.length > 0 && files.length > 0); + assert.equal(ctx.form.querySelectorAll('input[type="file"]').length, images.length + files.length); + images.forEach((f) => { + const input = pickerOf(ctx, f.key); + assert.equal(input.accept, 'image/png,image/jpeg,image/webp'); + assert.equal(input.multiple, true); + assert.doesNotMatch(input.accept, /gif/); + }); + files.forEach((f) => { + const input = pickerOf(ctx, f.key); + assert.equal(input.accept, 'application/pdf'); + assert.equal(input.multiple, false); + // Native, labelled, and the page’s "drag it into GitHub" note is hidden. + assert.ok(wrapOf(ctx, f.key).querySelector('label[for="' + input.id + '"]')); + assert.equal(wrapOf(ctx, f.key).querySelector('.field-note:not([data-upload-signed-out])').hidden, true); + }); + assert.match(wrapOf(ctx, 'screenshots').textContent, /A reviewer checks every file/); + assert.match(wrapOf(ctx, 'screenshots').textContent, /do not show personal information/); + assert.match(wrapOf(ctx, 'deck_pdf').textContent, /used instead of any link/); + const live = ctx.live(); + assert.equal(live.getAttribute('aria-live'), 'polite'); + assert.equal(live.getAttribute('role'), 'status'); +}); + +test('signing out hides the pickers; signing back in shows them', async () => { + const ctx = await boot(); + assert.ok(pickerOf(ctx, 'screenshots')); + ctx.document.querySelector('[data-direct-action="signout"]').click(); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); + assert.equal( + wrapOf(ctx, 'deck_pdf').querySelector('.field-note:not([data-upload-signed-out])').hidden, + false + ); +}); + +/* ---------------------------------------------------------- uploading */ + +test('an upload request carries exactly the contract headers, the re-encoded bytes, and never reaches the draft', async () => { + const ctx = await boot(); + await pick(ctx, 'screenshots', [png(ctx, 'My Screenshot é.png')]); + assert.equal(ctx.uploadsSent.length, 1); + const request = ctx.uploadsSent[0]; + assert.equal(request.method, 'POST'); + assert.equal(request.url, UPLOADS_URL + '/api/upload'); + assert.deepEqual(Object.keys(request.headers).sort(), [ + 'Authorization', + 'Content-Type', + 'X-Upload-Kind', + 'X-Upload-Name', + 'X-Upload-Set', + ]); + assert.equal(request.headers.Authorization, 'Bearer ' + TOKEN); + assert.equal(request.headers['Content-Type'], 'application/octet-stream'); + assert.equal(request.headers['X-Upload-Kind'], 'image'); + assert.match(request.headers['X-Upload-Set'], SET_RE); + assert.equal(request.headers['X-Upload-Name'], 'My Screenshot _.png'); + assert.equal(await request.body.text(), 're-encoded My Screenshot é.png'); + assert.equal(ctx.reencoded.length, 1); + + describe(ctx, 'screenshots', 0, 'The queue'); + await settleDraft(); + const raw = ctx.rawDraft(); + assert.doesNotMatch(raw, new RegExp(TOKEN)); + assert.doesNotMatch(raw, /Bearer|Authorization/); + assert.doesNotMatch(ctx.window.localStorage.getItem(STORE) || '', /./); + const stored = ctx.storedDraft().uploads; + assert.equal(stored.set, request.headers['X-Upload-Set']); + assert.deepEqual(stored.user, { id: 1001, login: 'octo-cat' }); + assert.equal(stored.files.screenshots.length, 1); + assert.deepEqual(Object.keys(stored.files.screenshots[0]).sort(), ['alt', 'bytes', 'kind', 'name', 'ref']); + assert.match(stored.files.screenshots[0].ref, REF_RE); + assert.equal(stored.files.screenshots[0].alt, 'The queue'); + assert.equal(stored.files.screenshots[0].kind, 'image'); +}); + +test('a PDF is sent as it is, as kind pdf; a second one waits for the first to be removed', async () => { + const ctx = await boot(); + const deck = pdf(ctx); + await pick(ctx, 'deck_pdf', [deck]); + assert.equal(ctx.reencoded.length, 0, 'a PDF must not be re-encoded'); + const request = ctx.uploadsSent[0]; + assert.equal(request.headers['X-Upload-Kind'], 'pdf'); + assert.equal(request.body, deck); + assert.equal(pickerOf(ctx, 'deck_pdf'), null, 'one deck: the picker steps aside'); + assert.equal(rowsOf(ctx, 'deck_pdf').length, 1); +}); + +test('uploads go one at a time, in order, and share one set', async () => { + let active = 0; + let overlap = 0; + const ctx = await boot({ + send: async (request, c) => { + active += 1; + overlap = Math.max(overlap, active); + await flush(); + active -= 1; + c.count += 1; + const sha = digest(c.count); + return reply(201, { + ref: 'upload:' + request.headers['X-Upload-Set'] + '/' + sha + '.png', + sha256: sha, + bytes: 1, + ext: 'png', + }); + }, + }); + await pick(ctx, 'screenshots', [png(ctx, 'a.png'), png(ctx, 'b.png'), png(ctx, 'c.png')]); + assert.equal(overlap, 1); + const sets = new Set(ctx.uploadsSent.map((r) => r.headers['X-Upload-Set'])); + assert.equal(sets.size, 1); + assert.deepEqual( + ctx.uploadsSent.map((r) => r.headers['X-Upload-Name']), + ['a.png', 'b.png', 'c.png'] + ); + assert.equal(rowsOf(ctx, 'screenshots').length, 3); +}); + +test('the set id is made once per draft and survives a reload', async () => { + const first = await boot(); + await pick(first, 'screenshots', [png(first, 'a.png')]); + describe(first, 'screenshots', 0, 'First'); + await settleDraft(); + const set = first.storedDraft().uploads.set; + assert.match(set, SET_RE); + + const second = await boot({ draft: first.rawDraft() }); + second.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + assert.equal(rowsOf(second, 'screenshots').length, 1); + await pick(second, 'screenshots', [png(second, 'b.png')]); + assert.equal(second.uploadsSent[0].headers['X-Upload-Set'], set, 'a restored draft kept its set'); +}); + +test('a 401 means the sign-in is over: signed-out state, pickers gone, files kept as failed', async () => { + const ctx = await boot({ + send: async () => reply(401, { error: 'unauthorized', message: 'Token expired' }), + }); + await pick(ctx, 'screenshots', [png(ctx)]); + assert.equal(ctx.window.sessionStorage.getItem(STORE), null, 'the dead token was kept'); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); + assert.ok(ctx.document.querySelector('[data-direct-action="signin"]'), 'the sign-in button is back'); + assert.match(ctx.document.querySelector('[data-signin-notice]').textContent, /sign-in has expired/); + assert.match(ctx.live().textContent, /did not upload/); + assert.equal(ctx.uploadsSent.length, 1, 'a 401 must not be retried'); +}); + +test('a 429 waits as long as Retry-After says, then retries, up to three times', async () => { + const answers = [ + reply(429, { error: 'rate_limited', message: 'Busy' }, '3'), + reply(429, { error: 'rate_limited', message: 'Busy' }, '7'), + ]; + const ctx = await boot({ + send: async (request, c) => { + const next = answers.shift(); + if (next) return next; + c.count += 1; + const sha = digest(c.count); + return reply(201, { + ref: 'upload:' + request.headers['X-Upload-Set'] + '/' + sha + '.png', + sha256: sha, + bytes: 1, + ext: 'png', + }); + }, + }); + await pick(ctx, 'screenshots', [png(ctx)]); + assert.deepEqual(ctx.sleeps, [3000, 7000]); + assert.equal(ctx.uploadsSent.length, 3); + assert.match(rowsOf(ctx, 'screenshots')[0].textContent, /Uploaded\./); + + const never = await boot({ + send: async () => + reply(429, { error: 'rate_limited', message: 'Too many uploads. Try again in a minute.' }, '1'), + }); + await pick(never, 'screenshots', [png(never)]); + assert.equal(never.uploadsSent.length, 4, 'the first try and three retries'); + assert.deepEqual(never.sleeps, [1000, 1000, 1000]); + const row = rowsOf(never, 'screenshots')[0]; + assert.match(row.textContent, /Not uploaded: Too many uploads\. Try again in a minute\./); + assert.equal(row.querySelector('[data-upload-action="retry"]').hidden, false); +}); + +test('a 429 for a per-user cap (too_many_files, too_many_drafts) is not retried; its message is shown', async () => { + for (const code of ['too_many_files', 'too_many_drafts']) { + const ctx = await boot({ + send: async () => reply(429, { error: code, message: 'Cap reached: ' + code }, '5'), + }); + await pick(ctx, 'screenshots', [png(ctx)]); + assert.equal(ctx.uploadsSent.length, 1, code + ' was retried'); + assert.deepEqual(ctx.sleeps, []); + assert.match(rowsOf(ctx, 'screenshots')[0].textContent, new RegExp('Not uploaded: Cap reached: ' + code)); + } +}); + +test('a refusal shows the service’s message; a network failure offers Try again, which uploads', async () => { + let fail = true; + const ctx = await boot({ + send: async (request, c) => { + if (fail) throw new Error('network'); + c.count += 1; + const sha = digest(c.count); + return reply(201, { + ref: 'upload:' + request.headers['X-Upload-Set'] + '/' + sha + '.png', + sha256: sha, + bytes: 1, + ext: 'png', + }); + }, + }); + await pick(ctx, 'screenshots', [png(ctx)]); + const row = () => rowsOf(ctx, 'screenshots')[0]; + assert.match(row().textContent, /could not reach the upload service/); + fail = false; + row().querySelector('[data-upload-action="retry"]').click(); + await settle(); + assert.match(row().textContent, /Uploaded\./); + assert.equal(ctx.reencoded.length, 1, 'a retry must not re-encode'); + + const refused = await boot({ + send: async () => reply(413, { error: 'too_large', message: 'That file is over 4 MB.' }), + }); + await pick(refused, 'screenshots', [png(refused)]); + assert.match(rowsOf(refused, 'screenshots')[0].textContent, /That file is over 4 MB\./); +}); + +test('remove: DELETE with the ref, then the row is gone, focus moves on, the draft forgets it', async () => { + const ctx = await boot(); + await pick(ctx, 'screenshots', [png(ctx, 'a.png'), png(ctx, 'b.png')]); + const ref = rowsOf(ctx, 'screenshots').length && ctx.uploadsSent[0].headers['X-Upload-Set']; + assert.ok(ref); + const first = rowsOf(ctx, 'screenshots')[0]; + const button = first.querySelector('[data-upload-action="remove"]'); + assert.match(button.getAttribute('aria-label'), /a\.png/); + button.click(); + await settle(); + const del = ctx.uploadsSent.find((r) => r.method === 'DELETE'); + assert.ok(del, 'no DELETE was sent'); + assert.match( + del.url, + /^https:\/\/uploads\.example\/api\/upload\?ref=upload%3A[a-z2-7]{26}%2F[0-9a-f]{64}\.png$/ + ); + assert.deepEqual(Object.keys(del.headers), ['Authorization']); + assert.equal(del.headers.Authorization, 'Bearer ' + TOKEN); + assert.equal(rowsOf(ctx, 'screenshots').length, 1); + assert.match(rowsOf(ctx, 'screenshots')[0].textContent, /b\.png/); + assert.equal( + ctx.document.activeElement, + rowsOf(ctx, 'screenshots')[0].querySelector('[data-upload-action="remove"]') + ); + assert.match(ctx.live().textContent, /Removed a\.png/); + + await settleDraft(); + assert.equal(ctx.storedDraft().uploads.files.screenshots.length, 1); + + // Last one out: focus lands on the picker. + rowsOf(ctx, 'screenshots')[0].querySelector('[data-upload-action="remove"]').click(); + await settle(); + assert.equal(rowsOf(ctx, 'screenshots').length, 0); + assert.equal(ctx.document.activeElement, pickerOf(ctx, 'screenshots')); +}); + +test('a deck removed brings its picker back; a failed DELETE keeps the row and says so', async () => { + const ctx = await boot({ + send: async (request, c) => + request.method === 'DELETE' + ? reply(500, { error: 'x', message: 'GitHub is down.' }) + : defaultCreated(request, c), + }); + await pick(ctx, 'deck_pdf', [pdf(ctx)]); + rowsOf(ctx, 'deck_pdf')[0].querySelector('[data-upload-action="remove"]').click(); + await settle(); + assert.equal(rowsOf(ctx, 'deck_pdf').length, 1); + assert.match( + wrapOf(ctx, 'deck_pdf').querySelector('[data-upload-message]').textContent, + /could not be removed.*GitHub is down\./ + ); + + const ok = await boot(); + await pick(ok, 'deck_pdf', [pdf(ok)]); + rowsOf(ok, 'deck_pdf')[0].querySelector('[data-upload-action="remove"]').click(); + await settle(); + assert.equal(rowsOf(ok, 'deck_pdf').length, 0); + assert.ok(pickerOf(ok, 'deck_pdf')); + assert.equal(ok.document.activeElement, pickerOf(ok, 'deck_pdf')); +}); + +function defaultCreated(request, c) { + c.count += 1; + const sha = digest(c.count); + const ext = request.headers['X-Upload-Kind'] === 'pdf' ? 'pdf' : 'png'; + return reply(201, { + ref: 'upload:' + request.headers['X-Upload-Set'] + '/' + sha + '.' + ext, + sha256: sha, + bytes: 1, + ext, + }); +} + +/* ------------------------------------------------------- refusals */ + +test('refusals before any upload: a big PDF, a GIF, a non-PDF, too many pictures', async () => { + const ctx = await boot(); + await pick(ctx, 'deck_pdf', [pdf(ctx, 'big.pdf', 4 * 1024 * 1024 + 1)]); + const message = wrapOf(ctx, 'deck_pdf').querySelector('[data-upload-message]'); + assert.equal(message.hidden, false); + assert.match(message.textContent, /big\.pdf.*over the 4 MB limit/); + assert.match(message.textContent, /export it again as PDF, minimum size/); + assert.match(message.textContent, /paste a link/); + assert.equal(ctx.uploadsSent.length, 0); + assert.match(ctx.live().textContent, /big\.pdf/); + + await pick(ctx, 'deck_pdf', [file(ctx, 'notes.txt', 'text/plain')]); + assert.match(message.textContent, /is not a PDF/); + await pick(ctx, 'screenshots', [file(ctx, 'move.gif', 'image/gif')]); + assert.match( + wrapOf(ctx, 'screenshots').querySelector('[data-upload-message]').textContent, + /not a PNG, JPEG or WebP/ + ); + assert.equal(ctx.uploadsSent.length, 0); + + await pick( + ctx, + 'screenshots', + Array.from({ length: 9 }, (_, i) => png(ctx, 'p' + i + '.png')) + ); + assert.equal(rowsOf(ctx, 'screenshots').length, 8); + assert.match( + wrapOf(ctx, 'screenshots').querySelector('[data-upload-message]').textContent, + /Up to 8 screenshots/ + ); +}); + +test('a picture that cannot be shrunk under 4 MB, or read, is reported with a way to remove it', async () => { + const fails = (code) => async () => { + const error = new Error(code); + error.code = code; + throw error; + }; + const ctx = await boot({ reencode: fails('toolarge') }); + await pick(ctx, 'screenshots', [png(ctx)]); + const row = rowsOf(ctx, 'screenshots')[0]; + assert.match(row.textContent, /over 4 MB/); + assert.equal(row.querySelector('[data-upload-action="remove"]').hidden, false); + assert.equal(ctx.uploadsSent.length, 0); + + const unreadable = await boot({ reencode: fails('decode') }); + await pick(unreadable, 'screenshots', [png(unreadable)]); + assert.match(rowsOf(unreadable, 'screenshots')[0].textContent, /could not be read as a picture/); +}); + +/* ----------------------------------------------------------- the body */ + +test('the issue body carries upload lines for screenshots (with alt, then typed URLs) and a bare ref for the deck', async () => { + const ctx = await boot(); + await pick(ctx, 'screenshots', [png(ctx, 'a.png'), png(ctx, 'b.png')]); + await pick(ctx, 'deck_pdf', [pdf(ctx)]); + describe(ctx, 'screenshots', 0, 'The intake queue'); + describe(ctx, 'screenshots', 1, 'Routing result'); + answer(ctx, 'screenshots', 'https://example.org/extra.png | Extra view'); + const set = ctx.uploadsSent[0].headers['X-Upload-Set']; + const refs = ctx.uploadsSent.map( + (r, i) => 'upload:' + set + '/' + digest(i + 1) + (r.headers['X-Upload-Kind'] === 'pdf' ? '.pdf' : '.png') + ); + const { issueFormBody, markdownBody, issueUrl } = ctx.window.SubmitForm; + const expectedShots = + refs[0] + + ' | The intake queue\n' + + refs[1] + + ' | Routing result\nhttps://example.org/extra.png | Extra view'; + + const body = issueFormBody(ctx.fields); + assert.ok(body.includes('### Screenshots\n\n' + expectedShots), body); + assert.ok( + body.includes('### Slide deck or one-pager (PDF)\n\n' + refs[2] + '\n\n') || + body.endsWith('### Slide deck or one-pager (PDF)\n\n' + refs[2]), + body + ); + const markdown = markdownBody(ctx.fields); + assert.ok(markdown.includes('### Screenshots\n\n' + expectedShots + '\n')); + assert.ok(markdown.includes('### Slide deck or one-pager (PDF)\n\n' + refs[2] + '\n')); + for (const line of body.split('\n').filter((l) => l.startsWith('upload:'))) + assert.match(line.split(' | ')[0], REF_RE); + + // The prefilled link is for signed-out submitters: never an upload in it. + const url = issueUrl(ctx.form, ctx.fields, 'Water routing'); + assert.doesNotMatch(decodeURIComponent(url), /upload:/); + assert.match(decodeURIComponent(url), /extra\.png/); +}); + +test('with no uploads the body is exactly what it was', async () => { + const ctx = await boot(); + answer(ctx, 'screenshots', 'https://example.org/a.png | A'); + const plain = await boot({ uploads: false }); + answer(plain, 'screenshots', 'https://example.org/a.png | A'); + assert.equal( + ctx.window.SubmitForm.issueFormBody(ctx.fields), + plain.window.SubmitForm.issueFormBody(plain.fields) + ); + assert.equal( + ctx.window.SubmitForm.markdownBody(ctx.fields), + plain.window.SubmitForm.markdownBody(plain.fields) + ); +}); + +test('signed out, hidden uploads are not written into the body', async () => { + const ctx = await boot(); + await pick(ctx, 'screenshots', [png(ctx)]); + describe(ctx, 'screenshots', 0, 'Alt'); + ctx.document.querySelector('[data-direct-action="signout"]').click(); + assert.doesNotMatch(ctx.window.SubmitForm.issueFormBody(ctx.fields), /upload:/); + assert.doesNotMatch(ctx.window.SubmitForm.markdownBody(ctx.fields), /upload:/); +}); + +/* ------------------------------------------------------ draft restore */ + +/** A draft with one uploaded screenshot, made by `user`. */ +async function draftWithUpload(user = { login: 'octo-cat', id: 1001 }) { + const ctx = await boot({ user }); + await pick(ctx, 'screenshots', [png(ctx, 'kept.png')]); + describe(ctx, 'screenshots', 0, 'Kept alt'); + await pick(ctx, 'deck_pdf', [pdf(ctx, 'kept.pdf')]); + await settleDraft(); + return ctx.rawDraft(); +} + +test('restoring a draft as the same user shows the files again, with their descriptions', async () => { + const draft = await draftWithUpload(); + const ctx = await boot({ draft }); + assert.equal(rowsOf(ctx, 'screenshots').length, 0, 'files must wait for Restore, like the answers'); + ctx.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + const shots = rowsOf(ctx, 'screenshots'); + assert.equal(shots.length, 1); + assert.match(shots[0].textContent, /kept\.png/); + assert.equal(shots[0].querySelector('[data-upload-alt]').value, 'Kept alt'); + assert.equal(rowsOf(ctx, 'deck_pdf').length, 1); + assert.match( + ctx.window.SubmitForm.issueFormBody(ctx.fields), + /upload:[a-z2-7]{26}\/[0-9a-f]{64}\.png \| Kept alt/ + ); + assert.equal(ctx.uploadsSent.length, 0, 'restoring must not upload anything'); +}); + +test('restoring as a different user drops the files with a notice', async () => { + const draft = await draftWithUpload(); + const ctx = await boot({ draft, user: { login: 'someone-else', id: 2002 } }); + ctx.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + assert.equal(rowsOf(ctx, 'screenshots').length, 0); + assert.equal(rowsOf(ctx, 'deck_pdf').length, 0); + const note = wrapOf(ctx, 'screenshots').querySelector('[data-upload-message]'); + assert.equal(note.hidden, false); + assert.match(note.textContent, /uploaded by @octo-cat.*signed in as @someone-else.*removed from the draft/); + assert.match(ctx.live().textContent, /removed from the draft/); + assert.doesNotMatch(ctx.window.SubmitForm.issueFormBody(ctx.fields), /upload:/); + await settleDraft(); + assert.equal((ctx.storedDraft() || {}).uploads, undefined, 'the other user’s refs stayed in the draft'); +}); + +test('restoring while signed out keeps the files in the draft, hides the pickers, and says to sign in', async () => { + const draft = await draftWithUpload(); + const ctx = await boot({ draft, signedIn: false }); + ctx.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + assert.equal(ctx.form.querySelector('input[type="file"]'), null); + const note = wrapOf(ctx, 'screenshots').querySelector('[data-upload-signed-out]'); + assert.equal(note.hidden, false); + assert.match(note.textContent, /Sign in with GitHub again/); + assert.doesNotMatch(ctx.window.SubmitForm.issueFormBody(ctx.fields), /upload:/); + await settleDraft(); + assert.equal(ctx.storedDraft().uploads.files.screenshots.length, 1, 'the files were lost from the draft'); + assert.equal(ctx.storedDraft().uploads.user.id, 1001); + + // Signing in as the same user brings them back. + ctx.window.sessionStorage.setItem(STORE, JSON.stringify({ token: TOKEN, expiresAt: null })); + ctx.window.dispatchEvent( + Object.assign( + new ctx.window.MessageEvent('message', { + data: { type: 'phct-github-token', token: TOKEN }, + origin: SIGNIN, + }) + ) + ); +}); + +test('an old draft, from before uploads, loads exactly as it did', async () => { + const draft = JSON.stringify({ saved: new Date().toISOString(), fields: { title: 'Old draft' } }); + const ctx = await boot({ draft }); + ctx.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + assert.equal(wrapOf(ctx, 'title').querySelector('input').value, 'Old draft'); + assert.equal(rowsOf(ctx, 'screenshots').length, 0); +}); + +test('a hand-edited uploads record is checked: wrong set, wrong ref, wrong kind are dropped', async () => { + const set = 'a'.repeat(26); + const good = 'upload:' + set + '/' + 'b'.repeat(64) + '.png'; + const draft = JSON.stringify({ + saved: new Date().toISOString(), + fields: {}, + uploads: { + set, + user: { id: 1001, login: 'octo-cat' }, + files: { + screenshots: [ + { ref: good, name: 'ok.png', alt: 'ok', bytes: 5, kind: 'image' }, + { + ref: 'upload:' + 'c'.repeat(26) + '/' + 'b'.repeat(64) + '.png', + name: 'other-set.png', + alt: '', + bytes: 5, + kind: 'image', + }, + { ref: 'https://evil.example/x.png', name: 'url.png', alt: '', bytes: 5, kind: 'image' }, + { + ref: 'upload:' + set + '/' + 'd'.repeat(64) + '.pdf', + name: 'pdf-in-images.pdf', + alt: '', + bytes: 5, + kind: 'pdf', + }, + ], + }, + }, + }); + const ctx = await boot({ draft }); + ctx.document.querySelector('[data-draft-action="restore"]').click(); + await settle(); + assert.deepEqual( + rowsOf(ctx, 'screenshots').map( + (row) => /ok\.png|other-set|url\.png|pdf-in-images/.exec(row.textContent)[0] + ), + ['ok.png'] + ); +}); + +/* -------------------------------------------------- blocking submit */ + +test('Submit is blocked while a file uploads, after one fails, and while a description is missing', async () => { + let release; + const ctx = await boot({ + send: (request, c) => + new Promise((resolve) => { + release = () => resolve(defaultCreated(request, c)); + }), + }); + fillRequired(ctx); + await pick(ctx, 'screenshots', [png(ctx)]); + // Still uploading. + assert.equal(tryReview(ctx), false); + assert.match(summaryText(ctx), /still uploading/); + assert.equal(rowsOf(ctx, 'screenshots')[0].querySelector('progress').hidden, false); + + release(); + await settle(); + // Uploaded, but no description yet. + assert.equal(tryReview(ctx), false); + assert.match(summaryText(ctx), /Describe every screenshot/); + describe(ctx, 'screenshots', 0, 'The queue'); + assert.equal(tryReview(ctx), true, 'a described, uploaded screenshot should let the form through'); + + // A failed upload blocks until it is dealt with. + const failed = await boot({ send: async () => reply(500, { error: 'x', message: 'Broke.' }) }); + fillRequired(failed); + await pick(failed, 'screenshots', [png(failed)]); + assert.equal(tryReview(failed), false); + assert.match(summaryText(failed), /did not upload/); + rowsOf(failed, 'screenshots')[0].querySelector('[data-upload-action="remove"]').click(); + await settle(); + assert.equal(tryReview(failed), true); +}); + +test('a required question is satisfied by an upload alone', async () => { + const ctx = await boot(); + const field = ctx.fields.find((f) => f.key === 'deck_pdf'); + field.required = true; + assert.match(ctx.window.SubmitForm.checkField(field), /Enter an answer/); + await pick(ctx, 'deck_pdf', [pdf(ctx)]); + assert.equal(ctx.window.SubmitForm.checkField(field), ''); +}); + +test('the review reads the files back by name and description', async () => { + const ctx = await boot(); + fillRequired(ctx); + await pick(ctx, 'screenshots', [png(ctx, 'queue.png')]); + describe(ctx, 'screenshots', 0, 'The queue'); + assert.equal(tryReview(ctx), true); + assert.match(ctx.form.querySelector('[data-review]').textContent, /queue\.png — The queue/); +}); + +/* ------------------------------------------------------ sending, copy */ + +test('Submit to GitHub sends the upload lines; the confirmation no longer says to drag the deck in', async () => { + const ctx = await boot(); + fillRequired(ctx); + await pick(ctx, 'screenshots', [png(ctx)]); + describe(ctx, 'screenshots', 0, 'The queue'); + await pick(ctx, 'deck_pdf', [pdf(ctx)]); + assert.equal(tryReview(ctx), true); + await submitToGitHub(ctx); + const post = ctx.requests.find((r) => r.url === ISSUES_URL); + assert.ok(post, 'no issue was created'); + const body = JSON.parse(post.init.body).body; + assert.match(body, /### Screenshots\n\nupload:[a-z2-7]{26}\/[0-9a-f]{64}\.png \| The queue/); + assert.match(body, /### Slide deck or one-pager \(PDF\)\n\nupload:[a-z2-7]{26}\/[0-9a-f]{64}\.pdf/); + const done = ctx.form.querySelector('[data-review]').textContent; + assert.match(done, /Submitted — thank you/); + assert.doesNotMatch(done, /Files cannot be sent from this page/); + assert.match(done, /Your 2 uploaded files went with it/); + assert.equal(ctx.rawDraft(), null, 'the draft survived the send'); +}); + +test('without an upload the confirmation still says to drag the deck into the issue', async () => { + const ctx = await boot(); + fillRequired(ctx); + assert.equal(tryReview(ctx), true); + await submitToGitHub(ctx); + const done = ctx.form.querySelector('[data-review]').textContent; + assert.match(done, /Files cannot be sent from this page.*Slide deck or one-pager \(PDF\)/); + assert.doesNotMatch(done, /uploaded file/); +}); + +/* ------------------------------------------- the core's decision logic */ + +function core() { + const dom = new JSDOM('', { runScripts: 'outside-only' }); + booted.push(dom); + dom.window.eval(CORE); + return dom.window.SubmitForm.uploadsCore; +} + +test('encodeUnderLimit keeps the format at quality 0.9 when that fits', async () => { + const c = core(); + const calls = []; + const result = await c.encodeUnderLimit(async (type, quality) => { + calls.push([type, quality]); + return { size: 1000, type }; + }, 'image/png'); + assert.deepEqual(calls, [['image/png', 0.9]]); + assert.equal(result.type, 'image/png'); +}); + +test('encodeUnderLimit falls back to JPEG at falling quality until it fits', async () => { + const c = core(); + const limit = 4 * 1024 * 1024; + const calls = []; + const sizes = { + 'image/webp@0.9': limit + 1, + 'image/jpeg@0.85': limit + 1, + 'image/jpeg@0.75': limit + 1, + 'image/jpeg@0.65': limit, + }; + const result = await c.encodeUnderLimit(async (type, quality) => { + calls.push(type + '@' + quality); + return { size: sizes[type + '@' + quality] ?? 1, type }; + }, 'image/webp'); + assert.deepEqual(calls, ['image/webp@0.9', 'image/jpeg@0.85', 'image/jpeg@0.75', 'image/jpeg@0.65']); + assert.equal(result.type, 'image/jpeg'); + assert.equal(result.blob.size, limit, 'exactly 4 MiB is allowed'); +}); + +test('encodeUnderLimit gives up (null) when no quality fits, after trying every step', async () => { + const c = core(); + const calls = []; + const result = await c.encodeUnderLimit(async (type, quality) => { + calls.push(quality); + return { size: 10 * 1024 * 1024, type }; + }, 'image/jpeg'); + assert.equal(result, null); + assert.deepEqual(calls, [0.9, ...c.FALLBACK_QUALITIES]); + assert.ok( + c.FALLBACK_QUALITIES.every((q, i, all) => i === 0 || q < all[i - 1]), + 'qualities must decrease' + ); +}); + +test('fitWithin caps the long edge at 2400 px, keeps the ratio, never scales up', () => { + const c = core(); + const fit = (core, w, h) => ({ ...core.fitWithin(w, h) }); + assert.deepEqual(fit(c, 4800, 2400), { width: 2400, height: 1200 }); + assert.deepEqual(fit(c, 1000, 6000), { width: 400, height: 2400 }); + assert.deepEqual(fit(c, 2400, 2400), { width: 2400, height: 2400 }); + assert.deepEqual(fit(c, 800, 600), { width: 800, height: 600 }); + assert.deepEqual(fit(c, 10000, 1), { width: 2400, height: 1 }); +}); + +test('outputType keeps PNG, JPEG and WebP and refuses GIF; kindOf reads type then extension', () => { + const c = core(); + assert.equal(c.outputType('image/png'), 'image/png'); + assert.equal(c.outputType('image/jpeg'), 'image/jpeg'); + assert.equal(c.outputType('image/webp'), 'image/webp'); + assert.equal(c.outputType('image/gif'), ''); + assert.equal(c.kindOf({ name: 'a.PDF', type: '' }), 'pdf'); + assert.equal(c.kindOf({ name: 'a.jpeg', type: '' }), 'image'); + assert.equal(c.kindOf({ name: 'a.gif', type: 'image/gif' }), ''); + assert.equal(c.kindOf({ name: 'a.png', type: 'application/pdf' }), 'pdf'); +}); + +test('newSetId is 26 base32 characters from the secure source, 5 bits per byte', () => { + const c = core(); + const id = c.newSetId({ getRandomValues: (bytes) => bytes.fill(0xff) }); + assert.equal(id, '7'.repeat(26)); + assert.equal(c.newSetId({ getRandomValues: (bytes) => bytes.fill(0) }), 'a'.repeat(26)); + assert.match(c.newSetId(), SET_RE); + assert.notEqual(c.newSetId(), c.newSetId()); + assert.throws(() => c.newSetId({}), /secure random/); +}); + +test('retryDelay honours Retry-After seconds within reason', () => { + const c = core(); + assert.equal(c.retryDelay('3'), 3000); + assert.equal(c.retryDelay(0), 0); + assert.equal(c.retryDelay('1.5'), 1500); + assert.equal(c.retryDelay('9999'), 60000); + for (const bad of [null, undefined, '', 'soon', '-4']) assert.equal(c.retryDelay(bad), 2000, String(bad)); +}); + +test('headerName is printable ASCII, at most 200 characters', () => { + const c = core(); + assert.equal(c.headerName('Résumé.pdf'), 'R_sum_.pdf'); + assert.equal(c.headerName('a'.repeat(300)).length, 200); + assert.equal(c.headerName(''), 'file'); +}); diff --git a/test/scripts/uploads_url.test.mjs b/test/scripts/uploads_url.test.mjs new file mode 100644 index 0000000..95be6a6 --- /dev/null +++ b/test/scripts/uploads_url.test.mjs @@ -0,0 +1,75 @@ +/** + * `submit.uploads_url`, the switch for file uploads on /submit/: the address + * check `npm run validate` applies (scripts/lib/uploads_url.mjs), its + * dependency on `submit.signin_url`, and the key surviving a run of either + * configurator, which has no question for it. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import * as jsYaml from 'js-yaml'; + +import { uploadsUrlProblem } from '../../scripts/lib/uploads_url.mjs'; +import { applyAnswers, answersFromConfig } from '../../assets/js/configurator/answers.js'; +import { defaultConfig } from '../../assets/js/configurator/default-config.js'; +import { renderFiles } from '../../assets/js/configurator/render-files.js'; + +const SIGNIN = 'https://catalog-signin.vercel.app'; + +test('blank, missing or off is fine, with or without a sign-in service', () => { + for (const value of [undefined, null, '', ' ', false]) { + assert.equal(uploadsUrlProblem(value, ''), '', String(value)); + assert.equal(uploadsUrlProblem(value, SIGNIN), '', String(value)); + } +}); + +test('the upload service’s https origin is accepted next to a sign-in service', () => { + for (const value of [ + 'https://catalog-uploads.vercel.app', + 'https://catalog-uploads.vercel.app/', + ' https://uploads.example.org:8443 ', + 'http://localhost:3001', + 'http://127.0.0.1:3001', + ]) { + assert.equal(uploadsUrlProblem(value, SIGNIN), '', value); + } +}); + +test('an address without a sign-in service is a problem: only signed-in submitters can upload', () => { + for (const signin of [undefined, null, '', ' ', false]) + assert.match( + uploadsUrlProblem('https://catalog-uploads.vercel.app', signin), + /needs submit\.signin_url: uploads work only for signed-in submitters/, + String(signin) + ); +}); + +test('the same address rules as signin_url apply, worded for the upload service', () => { + const cases = { + 'http://catalog-uploads.vercel.app': /must start with https/, + 'javascript:alert(1)': /must start with https/, + 'catalog-uploads.vercel.app': /not a web address.*upload service's address/, + 'https://catalog-uploads.vercel.app/api/upload': + /no path or query — use "https:\/\/catalog-uploads\.vercel\.app"/, + 'https://catalog-uploads.vercel.app/?x=1': /no path or query/, + 'https://user:pass@catalog-uploads.vercel.app': /no path or query/, + }; + for (const [value, pattern] of Object.entries(cases)) + assert.match(uploadsUrlProblem(value, SIGNIN), pattern, value); + assert.match(uploadsUrlProblem(42, SIGNIN), /in quotes/); + assert.match(uploadsUrlProblem(['https://a.example'], SIGNIN), /in quotes/); + // The address problem is reported before the missing sign-in one. + assert.match(uploadsUrlProblem('http://x.example', ''), /must start with https/); +}); + +test('a hand-set uploads_url survives the configurators, which never ask about it', () => { + const base = defaultConfig(); + base.site.submit = { + ...base.site.submit, + signin_url: SIGNIN, + uploads_url: 'https://catalog-uploads.vercel.app', + }; + const config = applyAnswers(base, { ...answersFromConfig(base), siteName: 'Renamed' }); + const site = jsYaml.load(renderFiles(config)['_data/site.yml']); + assert.equal(site.submit.uploads_url, 'https://catalog-uploads.vercel.app'); + assert.equal(site.submit.signin_url, SIGNIN); +}); From 9b51b9f6dacafcb41ebf04650ffd909da05dfae7 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:31:01 -0500 Subject: [PATCH 07/17] fix(uploads): clean up when a draft pull request closes A declined submission's issue is closed with the built-in GITHUB_TOKEN, which starts no issues: closed run, so its files waited for the daily sweep. Also run on pull_request: closed for same-repository entry/ drafts, resolve the issue from the branch name, and keep any set another open submission by the same author still names. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/uploads-cleanup.yml | 28 +++++- scripts/uploads_cleanup.mjs | 124 ++++++++++++++++++------ test/scripts/uploads_cleanup.test.mjs | 76 ++++++++++++++- test/scripts/uploads_workflows.test.mjs | 47 ++++++++- 4 files changed, 239 insertions(+), 36 deletions(-) diff --git a/.github/workflows/uploads-cleanup.yml b/.github/workflows/uploads-cleanup.yml index b2db76a..82c13e5 100644 --- a/.github/workflows/uploads-cleanup.yml +++ b/.github/workflows/uploads-cleanup.yml @@ -4,8 +4,11 @@ name: Uploads cleanup # branch each (`u///`) in a PRIVATE uploads repository # (docs/submit-uploads-plan.md). This deletes them once they are not needed: # -# * when a `content:new-entry` issue closes (its pull request merged or was -# declined), the author's sets named in the issue body; +# * when a `content:new-entry` issue closes, or its draft pull request +# (branch `entry/-`) closes merged or declined, the author's +# sets named in the issue body. The pull request trigger exists because a +# close made with the built-in GITHUB_TOKEN (a decline, a "Closes #N" on +# merge) starts no `issues: closed` run; # * every day, any set older than 14 days that no open `content:new-entry` # issue by the same author still references. # @@ -21,6 +24,12 @@ on: issues: types: - closed + # Same-repository drafts only (the job's `if`); the branch name is read from + # the event and used through `env:` only. The checkout below names the default + # branch because a declined, conflicting draft has no merge ref to check out. + pull_request: + types: + - closed schedule: # 04:47 UTC daily, off the hour-zero peak where scheduled runs queue for ages. - cron: "47 4 * * *" @@ -42,7 +51,9 @@ jobs: # A job whose `if` is false shows up as a bare grey "skipped" with no reason # (see metrics.yml). This one runs only when the feature is off and says so. turned-off: - if: vars.UPLOADS_REPO == '' && (github.event_name != 'issues' || contains(github.event.issue.labels.*.name, 'content:new-entry')) + if: >- + vars.UPLOADS_REPO == '' && (github.event_name != 'pull_request' || (contains(github.event.pull_request.labels.*.name, 'content:new-entry') && github.event.pull_request.head.repo.full_name == github.repository && startsWith(github.event.pull_request.head.ref, 'entry/'))) + && (github.event_name != 'issues' || contains(github.event.issue.labels.*.name, 'content:new-entry')) runs-on: ubuntu-latest steps: - name: Say why nothing ran @@ -50,7 +61,9 @@ jobs: echo "Uploads cleanup is turned off: the repository variable UPLOADS_REPO is not set, so there is no uploads repository to clean. Set it in Settings -> Secrets and variables -> Actions -> Variables to turn uploads on." >> "$GITHUB_STEP_SUMMARY" cleanup: - if: vars.UPLOADS_REPO != '' && (github.event_name != 'issues' || contains(github.event.issue.labels.*.name, 'content:new-entry')) + if: >- + vars.UPLOADS_REPO != '' && (github.event_name != 'pull_request' || (contains(github.event.pull_request.labels.*.name, 'content:new-entry') && github.event.pull_request.head.repo.full_name == github.repository && startsWith(github.event.pull_request.head.ref, 'entry/'))) + && (github.event_name != 'issues' || contains(github.event.issue.labels.*.name, 'content:new-entry')) runs-on: ubuntu-latest permissions: # Checkout, and listing this repository's open issues for the sweep. @@ -61,6 +74,7 @@ jobs: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: ${{ github.event.repository.default_branch }} persist-credentials: false - name: Setup Node @@ -99,7 +113,11 @@ jobs: - name: Delete unneeded uploads run: node scripts/uploads_cleanup.mjs env: - MODE: ${{ github.event_name == 'issues' && 'closed' || 'sweep' }} + MODE: ${{ github.event_name == 'issues' && 'closed' || github.event_name == 'pull_request' && 'pr-closed' || 'sweep' }} + # pr-closed: the issue is found from the draft branch name, which the + # script re-checks; the fork check is in the job's `if`. + PR_BRANCH: ${{ github.event.pull_request.head.ref }} + ISSUE_NUMBER: ${{ github.event.issue.number }} UPLOADS_TOKEN: ${{ steps.uploads-token.outputs.token }} UPLOADS_REPO: ${{ vars.UPLOADS_REPO }} CATALOG_REPO: ${{ github.repository }} diff --git a/scripts/uploads_cleanup.mjs b/scripts/uploads_cleanup.mjs index 410ce55..eadfaba 100644 --- a/scripts/uploads_cleanup.mjs +++ b/scripts/uploads_cleanup.mjs @@ -8,6 +8,10 @@ * * closed A new-entry issue was closed: delete the author's sets that its * body references. + * pr-closed A draft pull request closed (merged or declined). Neither + * closing path is guaranteed to start `issues: closed` (a close made + * with GITHUB_TOKEN starts nothing), so the issue is found from the + * draft branch `entry/-` and handled as `closed`. * sweep Daily: delete every set older than MAX_AGE_DAYS that no OPEN * `content:new-entry` issue by the same author still references. * @@ -18,12 +22,13 @@ * `refs/heads/u///` is left alone. * * Environment: UPLOADS_TOKEN (write, uploads repo only), UPLOADS_REPO, - * CATALOG_REPO, GITHUB_TOKEN (issues: read), MODE, ISSUE_BODY and - * ISSUE_AUTHOR_ID (closed), MAX_AGE_DAYS (default 14), DRY_RUN. + * CATALOG_REPO, GITHUB_TOKEN (issues: read), MODE, ISSUE_BODY, ISSUE_AUTHOR_ID + * and ISSUE_NUMBER (closed), PR_BRANCH (pr-closed), MAX_AGE_DAYS (default 14), DRY_RUN. */ import fs from 'node:fs'; import { pathToFileURL } from 'node:url'; +import { draftBranchPattern } from './lib/drafts.mjs'; import { parseUploadRef, SET_ID_RE, SHA256_RE } from './lib/upload_refs.mjs'; const API = 'https://api.github.com'; @@ -53,14 +58,28 @@ function setsFromAnywhere(text) { return sets; } +/** + * The issue number a draft branch `entry/-` was made for, or null. + * The shape is checked by the same pattern the draft finder uses. + * + * @param {unknown} branch + * @returns {number|null} + */ +export function issueFromDraftBranch(branch) { + const m = /-([1-9][0-9]{0,9})$/.exec(String(branch ?? '')); + if (!m) return null; + const issue = Number(m[1]); + return draftBranchPattern('entry/', issue, { numbered: true }).test(String(branch)) ? issue : null; +} + /** * @param {{ env: Record, fetch?: typeof fetch, now?: () => number, log?: (line: string) => void }} io * @returns {Promise<{ mode: string, dryRun: boolean, listed: number, ignored: number, deleted: string[], gone: number, kept: number, failed: number, lines: string[] }>} */ export async function cleanup({ env, fetch: fetchImpl = globalThis.fetch, now = Date.now, log = () => {} }) { const mode = env.MODE; - if (mode !== 'closed' && mode !== 'sweep') - throw new Error(`MODE must be "closed" or "sweep", got ${JSON.stringify(mode)}`); + if (!['closed', 'pr-closed', 'sweep'].includes(mode)) + throw new Error(`MODE must be "closed", "pr-closed" or "sweep", got ${JSON.stringify(mode)}`); const uploadsRepo = env.UPLOADS_REPO; if (!/^[\w.-]+\/[\w.-]+$/.test(uploadsRepo ?? '')) throw new Error('UPLOADS_REPO must be owner/name'); if (!env.UPLOADS_TOKEN) throw new Error('UPLOADS_TOKEN is not set'); @@ -95,6 +114,38 @@ export async function cleanup({ env, fetch: fetchImpl = globalThis.fetch, now = } } + function requireCatalog() { + if (!env.GITHUB_TOKEN) throw new Error('GITHUB_TOKEN is not set'); + if (!/^[\w.-]+\/[\w.-]+$/.test(env.CATALOG_REPO ?? '')) + throw new Error('CATALOG_REPO must be owner/name'); + } + + function maxAgeDays() { + const days = Number(env.MAX_AGE_DAYS || 14); + if (!Number.isFinite(days) || days < 0) throw new Error('MAX_AGE_DAYS must be a non-negative number'); + return days; + } + + /** + * `/` for every set an OPEN entry issue (other than `except`) still + * names. A failure throws before anything is deleted: not knowing what is + * still referenced must never look like "nothing is". + */ + async function openReferences(except) { + requireCatalog(); + const open = await pages( + `/repos/${env.CATALOG_REPO}/issues?state=open&labels=${encodeURIComponent(ENTRY_LABEL)}`, + env.GITHUB_TOKEN + ); + const referenced = new Set(); + for (const issue of open) { + if (issue.pull_request || String(issue.number) === String(except ?? '')) continue; + const author = String(issue.user?.id ?? ''); + for (const set of setsFromAnywhere(issue.body)) referenced.add(`${author}/${set}`); + } + return referenced; + } + // The ref listing: the only source of deletion targets. const listing = await pages(`/repos/${uploadsRepo}/git/matching-refs/heads/u/`, env.UPLOADS_TOKEN); const refs = []; @@ -110,31 +161,48 @@ export async function cleanup({ env, fetch: fetchImpl = globalThis.fetch, now = let kept = 0; const lines = []; - if (mode === 'closed') { - const author = String(env.ISSUE_AUTHOR_ID ?? '').trim(); + if (mode !== 'sweep') { + let { ISSUE_AUTHOR_ID: authorId, ISSUE_BODY: body, ISSUE_NUMBER: number } = env; + if (mode === 'pr-closed') { + number = String(issueFromDraftBranch(env.PR_BRANCH) ?? ''); + if (!number) { + lines.push(`Uploads cleanup (pr-closed): branch is not a draft branch; nothing to do`); + for (const line of lines) log(line); + return { + mode, + dryRun, + listed: listing.length, + ignored, + deleted: [], + gone: 0, + kept: 0, + failed: 0, + lines, + }; + } + requireCatalog(); + const res = await call('GET', `/repos/${env.CATALOG_REPO}/issues/${number}`, env.GITHUB_TOKEN); + if (!res.ok) throw new Error(`GET issue ${number} failed: ${res.status}`); + const issue = await res.json(); + if (issue.pull_request) throw new Error(`#${number} is a pull request, not an issue`); + authorId = String(issue.user?.id ?? ''); + body = issue.body; + } + const author = String(authorId ?? '').trim(); if (!/^[1-9][0-9]*$/.test(author)) throw new Error('ISSUE_AUTHOR_ID is not a GitHub user id'); - const named = setsFromLines(env.ISSUE_BODY); - targets = refs.filter((r) => r.user === author && named.has(r.set)); - } else { - if (!env.GITHUB_TOKEN) throw new Error('GITHUB_TOKEN is not set'); - const catalog = env.CATALOG_REPO; - if (!/^[\w.-]+\/[\w.-]+$/.test(catalog ?? '')) throw new Error('CATALOG_REPO must be owner/name'); - const days = Number(env.MAX_AGE_DAYS || 14); - if (!Number.isFinite(days) || days < 0) throw new Error('MAX_AGE_DAYS must be a non-negative number'); - const cutoff = now() - days * DAY_MS; - - // A failure here throws before anything is deleted: not knowing what is - // still referenced must never look like "nothing is". - const open = await pages( - `/repos/${catalog}/issues?state=open&labels=${encodeURIComponent(ENTRY_LABEL)}`, - env.GITHUB_TOKEN + const named = setsFromLines(body); + // Another open submission by the same author that still names a set keeps + // it, so a copied `upload:` line cannot take that submission's files. + const referenced = await openReferences(number); + targets = refs.filter( + (r) => r.user === author && named.has(r.set) && !referenced.has(`${r.user}/${r.set}`) ); - const referenced = new Set(); // `/` - for (const issue of open) { - if (issue.pull_request) continue; - const author = String(issue.user?.id ?? ''); - for (const set of setsFromAnywhere(issue.body)) referenced.add(`${author}/${set}`); - } + kept = + new Set(refs.filter((r) => r.user === author && named.has(r.set)).map((r) => r.set)).size - + new Set(targets.map((r) => r.set)).size; + } else { + const cutoff = now() - maxAgeDays() * DAY_MS; + const referenced = await openReferences(); const groups = new Map(); for (const r of refs) { @@ -189,7 +257,7 @@ export async function cleanup({ env, fetch: fetchImpl = globalThis.fetch, now = `- refs listed: ${listing.length} (ignored, not an upload branch: ${ignored})`, `- ${dryRun ? 'would delete' : 'deleted'}: ${deleted.length}`, `- already gone: ${gone}`, - ...(mode === 'sweep' ? [`- sets kept (referenced or too young): ${kept}`] : []), + `- sets kept (referenced elsewhere${mode === 'sweep' ? ' or too young' : ''}): ${kept}`, `- failed: ${failed}` ); for (const line of lines) log(line); diff --git a/test/scripts/uploads_cleanup.test.mjs b/test/scripts/uploads_cleanup.test.mjs index 3a7f0bf..039d41d 100644 --- a/test/scripts/uploads_cleanup.test.mjs +++ b/test/scripts/uploads_cleanup.test.mjs @@ -1,7 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { cleanup } from '../../scripts/uploads_cleanup.mjs'; +import { cleanup, issueFromDraftBranch } from '../../scripts/uploads_cleanup.mjs'; const SET_B = 'b'.repeat(26); const SET_C = 'c'.repeat(26); @@ -49,6 +49,11 @@ function fake({ refs = [], issues = [], deleteStatus = 204 } = {}) { return json({ committer: { date: new Date(NOW - r.object.ageDays * DAY).toISOString() } }); } if (u.pathname.endsWith('/issues')) return json(slice(issues, url)); + const one = /\/issues\/(\d+)$/.exec(u.pathname); + if (one) { + const found = issues.find((i) => String(i.number) === one[1]); + return found ? json(found) : json({}, 404); + } return json({}, 404); }; return { @@ -238,3 +243,72 @@ test('a missing token or bad mode is refused', async () => { ); await assert.rejects(run(gh, { MODE: 'nuke' }), /MODE/); }); + +test('closed: a set another open issue by the same author still references is kept', async () => { + const gh = fake({ + refs: [entry(7, SET_B, H1), entry(7, SET_C, H1)], + issues: [ + { number: 5, user: { id: 7 }, body: refLine(SET_B, H1) }, + { number: 6, user: { id: 8 }, body: refLine(SET_C, H1) }, // another author: no protection + ], + }); + const body = `${refLine(SET_B, H1)}\n${refLine(SET_C, H1)}`; + const result = await run(gh, { MODE: 'closed', ISSUE_AUTHOR_ID: '7', ISSUE_NUMBER: '9', ISSUE_BODY: body }); + assert.deepEqual(gh.deletes(), [`u/7/${SET_C}/${H1}`]); + assert.equal(result.kept, 1); +}); + +test('issueFromDraftBranch: good, malformed and fork-like branch names', () => { + assert.equal(issueFromDraftBranch('entry/water-routing-41'), 41); + assert.equal(issueFromDraftBranch('entry/a-1'), 1); + for (const bad of [ + 'entry/water', + 'entry/-41', + 'entry/Water-41', + 'entry/a/b-41', + 'feature/water-41', + 'refs/heads/entry/a-41', + 'entry/a-0', + 'entry/a-41 ', + 'entry/a-41\nx', + '', + null, + undefined, + 'entry/../x-41', + 'entry/a-99999999999', + ]) { + assert.equal(issueFromDraftBranch(bad), null, String(bad)); + } +}); + +test('pr-closed: the issue is found from the branch, fetched, and handled as closed (even if still open)', async () => { + const gh = fake({ + refs: [entry(7, SET_B, H1), entry(8, SET_B, H1), entry(7, SET_C, H1)], + issues: [ + { number: 41, user: { id: 7 }, body: refLine(SET_B, H1), state: 'open' }, + { number: 42, user: { id: 7 }, body: refLine(SET_C, H1) }, + ], + }); + const result = await run(gh, { MODE: 'pr-closed', PR_BRANCH: 'entry/water-41' }); + assert.deepEqual(gh.deletes(), [`u/7/${SET_B}/${H1}`]); + assert.equal(result.mode, 'pr-closed'); + assert.ok(gh.calls.some((c) => c.url.endsWith('/org/catalog/issues/41'))); +}); + +test('pr-closed: a branch that is not a draft branch does nothing, and a PR number is refused', async () => { + const gh = fake({ + refs: [entry(7, SET_B, H1)], + issues: [{ number: 41, user: { id: 7 }, body: refLine(SET_B, H1) }], + }); + const result = await run(gh, { MODE: 'pr-closed', PR_BRANCH: 'main' }); + assert.deepEqual(gh.deletes(), []); + assert.deepEqual(result.deleted, []); + assert.ok(!gh.calls.some((c) => c.url.includes('/issues/'))); + + const pr = fake({ + refs: [entry(7, SET_B, H1)], + issues: [{ number: 41, pull_request: {}, user: { id: 7 }, body: refLine(SET_B, H1) }], + }); + await assert.rejects(run(pr, { MODE: 'pr-closed', PR_BRANCH: 'entry/x-41' }), /pull request/); + assert.deepEqual(pr.deletes(), []); +}); diff --git a/test/scripts/uploads_workflows.test.mjs b/test/scripts/uploads_workflows.test.mjs index 13295bf..056ba57 100644 --- a/test/scripts/uploads_workflows.test.mjs +++ b/test/scripts/uploads_workflows.test.mjs @@ -52,8 +52,14 @@ test('new-entry: UPLOADS_TOKEN reaches exactly one step, the scaffolder, just be }); test('cleanup: triggers, empty top-level permissions, concurrency', () => { - assert.deepEqual(Object.keys(cleanup.on).sort(), ['issues', 'schedule', 'workflow_dispatch']); + assert.deepEqual(Object.keys(cleanup.on).sort(), [ + 'issues', + 'pull_request', + 'schedule', + 'workflow_dispatch', + ]); assert.deepEqual(cleanup.on.issues.types, ['closed']); + assert.deepEqual(cleanup.on.pull_request.types, ['closed']); assert.equal(cleanup.on.schedule.length, 1); const [minute, hour] = cleanup.on.schedule[0].cron.split(' '); assert.notEqual(minute, '0'); @@ -99,7 +105,10 @@ test('cleanup: the write token is minted here, scoped to contents write, and rea const holders = job.steps.filter((s) => s.env && 'UPLOADS_TOKEN' in s.env); assert.equal(holders.length, 1); assert.equal(holders[0].run, 'node scripts/uploads_cleanup.mjs'); - assert.equal(holders[0].env.MODE, "${{ github.event_name == 'issues' && 'closed' || 'sweep' }}"); + assert.equal( + holders[0].env.MODE, + "${{ github.event_name == 'issues' && 'closed' || github.event_name == 'pull_request' && 'pr-closed' || 'sweep' }}" + ); // Issue text only ever reaches a step through env:. for (const s of job.steps) assert.ok(!/github\.event\.issue/.test(s.run ?? ''), s.name); }); @@ -108,3 +117,37 @@ test('both workflows pin the app-token action to the same commit', () => { const pins = [entry.jobs.scaffold, cleanup.jobs.cleanup].map((j) => step(j, 'uploads-token').uses); assert.equal(pins[0], pins[1]); }); + +test('cleanup: a closed draft pull request runs only for a labelled same-repo entry/ branch', () => { + const job = cleanup.jobs.cleanup; + const off = cleanup.jobs['turned-off']; + const pr = ({ label = 'content:new-entry', head = 'org/catalog', ref = 'entry/water-41' } = {}) => ({ + github: { + event_name: 'pull_request', + repository: 'org/catalog', + event: { pull_request: { labels: [{ name: label }], head: { ref, repo: { full_name: head } } } }, + }, + vars: { UPLOADS_REPO: 'o/r' }, + }); + assert.equal(runs(job, pr()), true); + assert.equal(runs(job, pr({ label: 'bug' })), false); + assert.equal(runs(job, pr({ head: 'stranger/catalog' })), false); + assert.equal(runs(job, pr({ ref: 'feature/x' })), false); + const unset = { ...pr(), vars: { UPLOADS_REPO: '' } }; + assert.equal(runs(job, unset), false); + assert.equal(runs(off, unset), true); + assert.equal( + runs(off, { ...unset, github: { ...unset.github, event: pr({ head: 'x/y' }).github.event } }), + false + ); +}); + +test('cleanup: the pull request branch reaches the script through env only', () => { + const run = cleanup.jobs.cleanup.steps.find((s) => s.run === 'node scripts/uploads_cleanup.mjs'); + assert.equal(run.env.PR_BRANCH, '${{ github.event.pull_request.head.ref }}'); + assert.match(run.env.MODE, /pull_request' && 'pr-closed'/); + for (const s of cleanup.jobs.cleanup.steps) + assert.ok(!/github\.event\.pull_request/.test(s.run ?? ''), s.name); + for (const s of cleanup.jobs.cleanup.steps) + assert.ok(!/head\.(sha|ref)/.test(JSON.stringify(s.with ?? {})), s.name); +}); From 8bcfb61ec09107f3de0f905acf9919844e7dc849 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Fri, 16 Oct 2026 18:31:01 -0500 Subject: [PATCH 08/17] docs(uploads): document file uploads on the submit page Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 39 +++++++++++ SECURITY.md | 36 ++++++++++ docs/admin-guide.md | 111 ++++++++++++++++++++++++++++++- docs/configuration.md | 23 ++++++- docs/decisions.md | 39 +++++++++++ docs/submit-uploads-plan.md | 36 +++++++++- services/github-signin/README.md | 5 +- services/uploads/README.md | 26 ++++++-- 8 files changed, 304 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d40127a..2470dc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,45 @@ major version, and each entry says so when it happens. ## [Unreleased] +### Added + +- Optional **attach files on the submit page**. Set `submit.uploads_url` in + `_data/site.yml` (it needs `submit.signin_url`; `npm run validate` checks + both) and a signed-in submitter gets a file picker on the screenshots question, + with alt text beside each picture, and on each file question (the slide deck). + Each file uploads as soon as it is picked, pictures are re-encoded in the browser + first (EXIF and location data stripped, longest side 2,400 px), the draft remembers + the files, and the issue carries `upload:` references instead of asking the + submitter to drag files onto GitHub afterwards. Off by default; blank keeps the page + exactly as it was. Files are limited to 4 MB each, 3 drafts and 9 files per draft per + person. +- `services/uploads/`, the **upload service** behind it: a stateless Vercel project with + no runtime dependencies. It checks the submitter's GitHub token, the origin, the + file's real type and size and the per-person caps, then writes each file as its own + parentless commit and branch in a private uploads repository, with a second GitHub App + of its own. Setup is in `services/uploads/README.md`. +- The scaffolder resolves `upload:` references. `new-entry.yml` mints a read-only token for + the uploads repository (repository variable `UPLOADS_REPO`, secrets `UPLOADS_APP_ID` and + `UPLOADS_APP_PRIVATE_KEY`) and hands it to the scaffold step only; the files are read as + the issue's author, re-hashed, checked like any attachment and committed into the entry. + An unset or broken setup, or a file that has expired, becomes a warning on the draft, + and dragging files onto the issue keeps working. +- **Uploads cleanup** workflow (`uploads-cleanup.yml`): deletes a submission's files when + its draft pull request is merged or closed (or its issue is closed by hand), and + daily sweeps any file older than 14 days that no open submission names. A manual run with + **Preview only** lists what would go. Admin guide: *Files attached on the submit page*, + including how to purge something sensitive. +- "Using an AI agent?" on `/submit/`: an instructions file and a JSON Schema for answers, + both generated from `_data/schema.yml` by `npm run generate`, and an **Import answers** + box that loads an agent's JSON into the form and runs the form's own checks. + +### Security + +- `SECURITY.md` and the sign-in README now say that, on a deployment that also runs the + upload service, the submitter's sign-in token is sent to that service as well as to + `api.github.com` (one `GET /user` call), and that the private uploads repository does + not keep screenshots private once a submission is scaffolded. + ## [1.11.0-rc.10] — 2026-10-13 Tenth candidate. rc.9 failed closed in BCHC on three new tests that read the diff --git a/SECURITY.md b/SECURITY.md index 10152e6..eecd55b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -108,6 +108,42 @@ See [docs/admin-guide.md](docs/admin-guide.md#checks-on-a-generated-pull-request Every `uses:` in `.github/workflows/` names a full commit SHA with the release tag in a trailing comment. Dependabot proposes updates weekly. +## File uploads on the submit page (optional) + +Off unless a deployment sets `submit.uploads_url`. Then a signed-in submitter can attach screenshots +and a PDF deck on `/submit/` through a small service the deployment runs +([`services/uploads/`](services/uploads/README.md)). What that changes: + +- **The service stores nothing.** No database, no file storage, nothing on disk. It checks each file + (origin, a real PNG, JPEG, WebP or PDF judged by its bytes, at most 4 MB, per-person caps) and writes + it to a **private** GitHub repository in the same request. It has no runtime dependencies. +- **A private repository does not keep screenshots private.** It protects drafts that were never + submitted and stops the service being an anonymous file host. Once a submission is scaffolded, its + files are committed to a public `entry/*` draft branch and shown in the draft pull request before + anyone has reviewed them, just as files dragged into an issue are public the moment they are attached. + The page tells submitters not to show personal information; reviewers still open every picture at + full size ([docs/incidents.md](docs/incidents.md)). +- **The submitter's sign-in token goes to the upload service too.** Without uploads it is sent only to + `api.github.com`. With uploads the page also sends it to the service the deployment runs, which uses + it for one `GET /user` call to learn who is uploading and never logs, stores or forwards it. It can + do only what the sign-in App allows, and cannot touch the uploads repository. +- **A reference cannot name someone else's files.** The scaffolder builds the branch name itself from + the issue author's numeric id, so a reference copied into another person's issue finds nothing, and + no reference can name another repository. It re-hashes every file and rejects a mismatch. +- **The uploads token is read-only and reaches one step.** The scaffold job mints a token that can + read contents of the uploads repository only, hands it to the scaffold step alone, and the generic + image and attachment fetchers (which send their token to any GitHub host a submitter names) never + see it. Deleting uses a separate write token minted only in `uploads-cleanup.yml`, whose deletion + targets always come from the repository's own branch listing; issue text can only cause a set to be + kept. +- **Deleted is not erased.** One parentless commit per file means deleting a branch leaves the file + unreachable, but git keeps it until GitHub garbage-collects. To purge something sensitive at once, + contact GitHub Support. The procedure is in + [docs/admin-guide.md](docs/admin-guide.md#files-attached-on-the-submit-page). + +Report a problem with the upload service the same way as any other: see +[Reporting a vulnerability](#reporting-a-vulnerability) below. + ## Who can submit Open by default: the catalog is meant to collect work from people without diff --git a/docs/admin-guide.md b/docs/admin-guide.md index 750cb67..a78568a 100644 --- a/docs/admin-guide.md +++ b/docs/admin-guide.md @@ -75,7 +75,7 @@ Optional, and off by default. With `submit.signin_url` set in `_data/site.yml` What changes for you: - **The issue opens without a label.** GitHub drops the labels of an API request from anyone without push access. The `claim` job in `new-entry.yml` (on `opened`, and on `edited` in case an early edit cancelled the opened run before it labelled the issue) recognises an entry submission by its shape — the form's `[] ` title prefix and a `###