From 4805f1042a40115de60be5b784a2850d2ab1352a Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 05:31:50 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9Aread?= =?UTF-8?q?=20a=20commit=20message=20in=20linear=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL was right, and the input really is uncontrolled: a commit message comes from whoever opened the pull request, and the queue reads it while holding credentials that can write here. `/[\r\n]+$/` takes time proportional to the square of the run of newlines it is asked about -- 80,000 of them took 3.7 seconds, so a million would hang the job for minutes. Sweeping the rest of what I had written found a second one CodeQL did not report, and worse: `\S` matches a colon, so `^\S+:\S+( \S+)*$` let the engine try every colon in an Assisted-by value as the split point. Same verdicts either way, 8000 times faster. A third of the time was going on building an Intl.Segmenter for every line rather than once. Both are held to a budget by tests that fail in forty-odd seconds if either spelling comes back. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/shared/commit-message.mts | 40 +++++++++++++++++++++++----- build/shared/commit-message.test.mts | 35 ++++++++++++++++++++++++ build/shared/landing.mts | 6 ++--- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index bb0f04f1e..9a18ad157 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -96,8 +96,13 @@ const describeNearMiss = (cluster: string) => { return `“${cluster}” is not the emoji for ${VOCABULARY[intended]}; copy “${intended}” from ${HANDBOOK_URL}`; }; -const countGraphemes = (text: string) => - [...new Intl.Segmenter().segment(text)].length; +/** + * One segmenter, not one per line. Building a new one for every line of a + * message is most of the time spent reading a long one. + */ +const SEGMENTER = new Intl.Segmenter(); + +const countGraphemes = (text: string) => [...SEGMENTER.segment(text)].length; /** * A trailer is `Token: value` with no whitespace in the token. Written out @@ -112,11 +117,34 @@ const TRAILER_LINE = /^(?[A-Za-z][\w-]*):[ \t]*(?.*)$/; * by any specialised analysis tools, and nodejs/node lands it that way. Basic * development tools are left out. */ -const ASSISTED_BY_VALUE = /^\S+:\S+( \S+)*$/; +const ASSISTED_BY_VALUE = /^[^\s:]+:\S+( \S+)*$/; /** git folds a trailer whose value runs onto an indented line beneath it. */ const CONTINUATION_LINE = /^\s/; +/** + * Splits a commit message into its lines, without the blank ones git leaves + * at the end. Written as a scan rather than as `/[\r\n]+$/`, which takes time + * proportional to the square of the run of newlines it is asked about: a + * message is written by whoever opened the pull request, so a million of them + * is a thing somebody can send. + * @param {string} message The whole commit message. + * @returns {string[]} Its lines, however they were ended. + */ +export function linesOf(message: string) { + let end = message.length; + + while (end > 0) { + const last = message[end - 1]; + + if (last !== '\n' && last !== '\r') break; + + end -= 1; + } + + return message.slice(0, end).split(/\r?\n/); +} + /** * Splits a message body into paragraphs of non-empty lines. * @param {string[]} lines Every line after the subject. @@ -139,7 +167,7 @@ const paragraphsOf = (lines: string[]) => * @returns {string[]} The trailer lines, one per trailer, empty if there is no block. */ export function readTrailers(message: string) { - const [, ...rest] = message.replace(/[\r\n]+$/, '').split(/\r?\n/); + const [, ...rest] = linesOf(message); const last = paragraphsOf(rest).at(-1) ?? []; const isBlock = last.length > 0 && @@ -169,7 +197,7 @@ const checkSubject = (subject: string) => { const description = subject.slice(colon + IDEOGRAPHIC_COLON.length); // The variation selector belongs to the character before it, so the // prefix has to be read as grapheme clusters and not code points. - const clusters = [...new Intl.Segmenter().segment(prefix)].map( + const clusters = [...SEGMENTER.segment(prefix)].map( (entry) => entry.segment ); @@ -355,7 +383,7 @@ export function validateCommitMessage(message: string) { // about the message itself. Carriage returns say nothing either: git reads // trailers through them, so a message written on Windows must not be judged // differently from the same message written anywhere else. - const lines = message.replace(/[\r\n]+$/, '').split(/\r?\n/); + const lines = linesOf(message); const [subject = '', ...rest] = lines; const problems = checkSubject(subject); diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts index 462403697..7c928c391 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -365,3 +365,38 @@ describe('the vocabulary', () => { ); }); }); + +describe('a message written to be slow', () => { + // A commit message comes from whoever opened the pull request, and the + // commit queue reads it holding credentials that can write here. Taking + // time proportional to the square of its length is a way to stop the queue + // working, so these hold the rules to reading it in linear time. + // Generous, because the work itself is linear and CI is slow. What it + // separates is linear from quadratic: at this size the regexes these + // replaced took twenty seconds and ninety seconds respectively. + const budget = 3000; + + test('reads a long run of newlines quickly', () => { + const message = `🏗️🔧：fix it\n\nA body.${'\n'.repeat(200_000)}x`; + const started = performance.now(); + + validateCommitMessage(message); + + const spent = performance.now() - started; + + ok(spent < budget, `took ${spent.toFixed(0)}ms, budget ${budget}ms`); + }); + + test('reads a long Assisted-by value quickly', () => { + // `\S` matches a colon, so the obvious spelling of agent:model lets the + // engine try every colon as the split point. + const message = `🏗️🔧：fix it\n\nAssisted-by: ${'a:'.repeat(100_000)} `; + const started = performance.now(); + + validateCommitMessage(message); + + const spent = performance.now() - started; + + ok(spent < budget, `took ${spent.toFixed(0)}ms, budget ${budget}ms`); + }); +}); diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 0de64f7c7..dcb3dd5cc 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -11,7 +11,7 @@ * the repositories in this organization do not agree on that yet. */ -import { TRAILER_ORDER } from '@openinf/portal/build/commit-message'; +import { linesOf, TRAILER_ORDER } from '@openinf/portal/build/commit-message'; /** One commit's message, split into the parts a landed message reuses. */ export type CommitParts = { @@ -30,9 +30,7 @@ const tokenOf = (line: string) => line.match(/^([A-Za-z][\w-]*):/)?.[1] ?? ''; * @returns {CommitParts} Its subject, its body, and the trailers it carried. */ export function partsOfMessage(message: string): CommitParts { - const [subject = '', ...rest] = message - .replace(/[\r\n]+$/, '') - .split(/\r?\n/); + const [subject = '', ...rest] = linesOf(message); const body: string[] = []; const trailers: string[] = [];