From 5066e39fb0cc97110d388a05d00c081a53d0699b Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 05:47:35 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9Asee?= =?UTF-8?q?=20a=20description=20as=20a=20reader=20would?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL had a second alert open on my code that I had not seen: stripping HTML comments with a single replacement leaves what surrounded one able to form another. `-- sneaky -->` came out as ``, which a reader still cannot see -- so a pull request with no description at all satisfied the check for having one. Comments are now removed by scanning, repeatedly until nothing changes, capped so that a description written to be awkward cannot turn that into many passes. Each pass is linear and removes at least one comment. This is a heuristic for whether anything was written, not a sanitizer, but the pattern is worth not having in the codebase at all. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/shared/pull-request.mts | 68 +++++++++++++++++++++++++++++ build/shared/pull-request.test.mts | 63 ++++++++++++++++++++++++++ build/tasks/verify-pull-request.mts | 3 +- package.json | 1 + 4 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 build/shared/pull-request.mts create mode 100644 build/shared/pull-request.test.mts diff --git a/build/shared/pull-request.mts b/build/shared/pull-request.mts new file mode 100644 index 000000000..41edfb6a6 --- /dev/null +++ b/build/shared/pull-request.mts @@ -0,0 +1,68 @@ +/** + * @file What a reader would see of a pull request description. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/pull-request + */ + +/** + * Strips HTML comments the way a renderer does: left to right, in one pass, + * each comment ending at the first `-->` after it. Removing them with a + * repeated replacement instead lets what is left over form a new comment -- + * `-- sneaky -->` becomes ``, which a reader still + * cannot see, so a description made of nothing would have counted as one. + * @param {string} text The description as written. + * @returns {string} What is left once the comments are gone. + */ +const OPEN = ''; + +/** + * Removes every comment in one left-to-right pass, each ending at the first + * `-->` after it. + * @param {string} text What to read. + * @returns {string} The same, with its comments gone. + */ +const withoutComments = (text: string) => { + let kept = ''; + let index = 0; + + while (index < text.length) { + const start = text.indexOf(OPEN, index); + + if (start === -1) { + kept += text.slice(index); + break; + } + + kept += text.slice(index, start); + + const end = text.indexOf(CLOSE, start + OPEN.length); + + // An unterminated comment runs to the end, which is what a renderer does + // with one too. + if (end === -1) break; + + index = end + CLOSE.length; + } + + return kept; +}; + +export function visibleText(text: string) { + let visible = text; + + // Removing a comment can leave what surrounded it forming another, so this + // repeats until nothing changes. Each pass is a scan and each removes at + // least one comment, and the cap keeps a pathological description from + // turning that into a great many passes. + for (let pass = 0; pass < 8; pass += 1) { + const shorter = withoutComments(visible); + + if (shorter === visible) break; + + visible = shorter; + } + + return visible.trim(); +} diff --git a/build/shared/pull-request.test.mts b/build/shared/pull-request.test.mts new file mode 100644 index 000000000..df4dacb11 --- /dev/null +++ b/build/shared/pull-request.test.mts @@ -0,0 +1,63 @@ +/** + * @file Tests for what a reader would see of a pull request description. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/pull-request.test + */ + +import { deepStrictEqual, ok } from 'node:assert/strict'; +import { readFile } from 'node:fs/promises'; +import { describe, test } from 'node:test'; +import { visibleText } from '@openinf/portal/build/pull-request'; + +describe('visibleText', () => { + test('keeps what a reader would see', () => { + deepStrictEqual( + visibleText('A real description.'), + 'A real description.' + ); + }); + + test('finds nothing in a description that is only a comment', () => { + deepStrictEqual(visibleText(''), ''); + }); + + test('leaves nothing behind that could form another comment', () => { + // A repeated replacement turns this into ``, which renders + // as nothing -- so a description made of it would have counted as one. + deepStrictEqual(visibleText('-- sneaky -->'), ''); + }); + + test('treats an unterminated comment as running to the end', () => { + deepStrictEqual(visibleText('shownonetwo'), + 'one\ntwo'.replace('\n', '') + ); + }); + + test('the template on its own leaves nothing', async () => { + // The template is one long comment, so a pull request opened without a + // word written renders as empty. That is the case this exists to catch. + const template = await readFile( + new URL('../../.github/PULL_REQUEST_TEMPLATE.md', import.meta.url), + 'utf8' + ); + + deepStrictEqual(visibleText(template), ''); + }); + + test('reads a long description in linear time', () => { + const text = `${'x'.repeat(100_000)}end`; + const started = performance.now(); + + ok(visibleText(text).endsWith('end')); + + const spent = performance.now() - started; + + ok(spent < 3000, `took ${spent.toFixed(0)}ms`); + }); +}); diff --git a/build/tasks/verify-pull-request.mts b/build/tasks/verify-pull-request.mts index b917aafcf..2003f3cd4 100644 --- a/build/tasks/verify-pull-request.mts +++ b/build/tasks/verify-pull-request.mts @@ -6,6 +6,7 @@ */ import { validateCommitMessage } from '@openinf/portal/build/commit-message'; +import { visibleText } from '@openinf/portal/build/pull-request'; // Read from the environment rather than interpolated into a command by the // workflow: a title is written by whoever opened the pull request, and pasting @@ -32,7 +33,7 @@ if (process.env.PR_TITLE === undefined) { // The template is one long HTML comment, so a pull request opened without a // word written renders as nothing at all. That is what this looks for -- // not what the description says, only that there is one. - if (body.replaceAll(//g, '').trim() === '') { + if (visibleText(body) === '') { problems.push( 'description: say why the change is needed and what it does. The ' + 'template is a comment, so a description that is only the template ' + diff --git a/package.json b/package.json index 0227bb74a..659804139 100644 --- a/package.json +++ b/package.json @@ -12,6 +12,7 @@ "./build/commit-message": "./build/shared/commit-message.mts", "./build/constants": "./build/shared/constants.mts", "./build/landing": "./build/shared/landing.mts", + "./build/pull-request": "./build/shared/pull-request.mts", "./build/utils": "./build/utils.mts" }, "homepage": "open.inf.is",