From 20eac2acd3f52eddf1421184125120db613039f8 Mon Sep 17 00:00:00 2001 From: daniel63194 Date: Mon, 12 Oct 2026 12:57:23 +0200 Subject: [PATCH 1/2] Fix two CodeQL findings Signed-off-by: daniel63194 --- CHANGELOG.md | 7 +++++++ src/report.ts | 7 +++++-- src/rules.ts | 16 ++++++++++++++-- test/fuzz.test.ts | 26 ++++++++++++++++++++++++++ test/report.test.ts | 6 ++++++ test/rules.test.ts | 6 ++++++ 6 files changed, 64 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96e8574..6df2257 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project are documented here. This project follows [S ## Unreleased +## 0.1.3 + +### Fixed + +- A crafted publisher name could make the publisher comparison very slow. Found by CodeQL. +- A backslash before `|` in the evidence could break the findings table in the Markdown report. Found by CodeQL. + ## 0.1.2 ### Fixed diff --git a/src/report.ts b/src/report.ts index 80033e3..60ba864 100644 --- a/src/report.ts +++ b/src/report.ts @@ -86,8 +86,11 @@ export function renderText(r: Review, options: TextOptions = {}): string { const escapeMd = (s: string): string => displaySafe(s).replace(/[\\`*_[\]<>|#!~]/g, (ch) => `\\${ch}`); const code = (s: string): string => `\`${displaySafe(s).replace(/`/g, "'")}\``; -/** A code span inside a table cell, where `|` must be escaped even in code. */ -const cellCode = (s: string): string => code(s).replace(/\|/g, '\\|'); +/** + * A code span inside a table cell. GitHub splits table rows on any `|` that is + * not escaped, even inside code, and `\\` escapes a backslash, so both are escaped. + */ +const cellCode = (s: string): string => code(s).replace(/[\\|]/g, (ch) => `\\${ch}`); /** A report for GitHub (job summaries, issues, pull request comments). */ export function renderMarkdown(r: Review, extra: { approveCommand?: string } = {}): string { diff --git a/src/rules.ts b/src/rules.ts index 52563b2..6f702db 100644 --- a/src/rules.ts +++ b/src/rules.ts @@ -172,6 +172,20 @@ const DEPENDENCY_LABEL: Record<(typeof DEPENDENCY_FIELDS)[number], [string, stri const NON_REGISTRY_SPEC = /^(?:git\+|git:|github:|gitlab:|bitbucket:|gist:|https?:|file:|link:)|^[\w.-]+\/[\w.-]+(?:#.*)?$/i; +/** + * A publisher name for comparison: "github-actions (oidc)" and "GitHub Actions" + * are the same publisher. Written without a regular expression for the trailing + * "(…)" so that long, crafted names cannot make it slow. + */ +function who(name: string): string { + let s = name.trimEnd(); + if (s.endsWith(')')) { + const open = s.lastIndexOf('('); + if (open !== -1) s = s.slice(0, open); + } + return s.toLowerCase().replace(/[^a-z0-9]/g, ''); +} + function parseSemver(v: string): number[] | null { const m = /^(\d+)\.(\d+)\.(\d+)/.exec(v); return m ? [Number(m[1]), Number(m[2]), Number(m[3])] : null; @@ -253,8 +267,6 @@ export function runRules(ctx: RuleContext): Finding[] { } } - // "github-actions (oidc)" and "GitHub Actions" are the same publisher. - const who = (s: string): string => s.replace(/\s*\([^)]*\)\s*$/, '').toLowerCase().replace(/[^a-z0-9]/g, ''); if (ctx.stagedBy && ctx.previousPublisher && who(ctx.stagedBy) !== who(ctx.previousPublisher)) { add({ rule: 'publisher-change', severity: 'medium', message: `staged by ${ctx.stagedBy}, but the previous version was published by ${ctx.previousPublisher}` }); } diff --git a/test/fuzz.test.ts b/test/fuzz.test.ts index cde5020..4809963 100644 --- a/test/fuzz.test.ts +++ b/test/fuzz.test.ts @@ -166,6 +166,32 @@ describe('fuzz: review', () => { }); }); +/** Table cell delimiters in a GitHub Markdown row: `|` not escaped by a backslash. */ +function cellDelimiters(row: string): number { + let n = 0; + for (let i = 0; i < row.length; i++) { + if (row[i] === '\\') i++; + else if (row[i] === '|') n++; + } + return n; +} + +describe('fuzz: Markdown report', () => { + it('keeps every findings row at three cells, whatever the evidence contains', () => { + const tricky = fc.string({ unit: fc.constantFrom('\\', '|', '`', '*', '<', 'a', ' ', '\n'), maxLength: 40 }); + fc.assert( + fc.property(tricky, tricky, (a, b) => { + const after = readTarball(makeTarball({ 'package.json': '{"name":"demo","version":"1.0.1"}', [`lib/${a.replace(/[^a-z]/g, '') || 'x'}.js`]: `fetch('https://webhook.site/x') // ${a}\nrequire('child_process') // ${b}\n` })); + const md = renderMarkdown(reviewFiles(null, after)); + const rows = md.split('\n').filter((l) => /^\| (?:🔴|🟠|⚪) /.test(l)); + expect(rows.length).toBeGreaterThan(0); + for (const row of rows) expect(cellDelimiters(row)).toBe(4); + }), + RUNS, + ); + }); +}); + describe('fuzz: helpers', () => { it('addedLines returns exactly the lines a change introduces', () => { const lines = fc.array(fc.constantFrom('a', 'b', 'c', '', 'd e'), { maxLength: 12 }); diff --git a/test/report.test.ts b/test/report.test.ts index 5cc1958..07c0053 100644 --- a/test/report.test.ts +++ b/test/report.test.ts @@ -39,6 +39,12 @@ describe('renderMarkdown', () => { expect(md).toContain('```sh\nnpm stage approve x\n```'); }); + it('escapes backslashes too, so a backslash before a pipe cannot end the cell', () => { + const r = reviewFiles(null, files({ 'package.json': '{"name":"demo","version":"1.0.0"}', 'a.js': "fetch('https://webhook.site/x') // a\\| b | c\n" })); + const row = renderMarkdown(r).split('\n').find((l) => l.includes('exfiltrate')) as string; + expect(row).toContain('a\\\\\\| b \\| c'); + }); + it('keeps table rows intact when evidence contains pipes', () => { const md = renderMarkdown(risky); const row = md.split('\n').find((l) => l.includes('downloads and executes a script')) as string; diff --git a/test/rules.test.ts b/test/rules.test.ts index 64c9f7b..6c76ae3 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -78,6 +78,12 @@ describe('package.json rules', () => { expect(rules(r)).toEqual(expect.arrayContaining(['low:entry-points', 'low:version-order', 'medium:publisher-change'])); }); + it('compares publisher names in linear time, even when crafted', () => { + const start = Date.now(); + review(cleanPackage(), cleanPackage('1.0.1'), { stagedBy: `${'('.repeat(100_000)}x`, previousPublisher: `${' '.repeat(100_000)}x` }); + expect(Date.now() - start).toBeLessThan(1000); + }); + it('treats the same publisher written differently as unchanged', () => { const r = review(cleanPackage(), cleanPackage('1.0.1'), { stagedBy: 'github-actions (oidc)', previousPublisher: 'GitHub Actions' }); expect(rules(r)).not.toContain('medium:publisher-change'); From 6dbefc3e5402d2b0c16be95a75ff47c3c712cb99 Mon Sep 17 00:00:00 2001 From: daniel63194 Date: Mon, 12 Oct 2026 12:57:27 +0200 Subject: [PATCH 2/2] 0.1.3 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1c05b0c..9140562 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "stagecheck", - "version": "0.1.2", + "version": "0.1.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "stagecheck", - "version": "0.1.2", + "version": "0.1.3", "license": "MIT", "bin": { "stagecheck": "dist/cli.js" diff --git a/package.json b/package.json index 2d1fcaa..58ecf27 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "stagecheck", - "version": "0.1.2", + "version": "0.1.3", "description": "Review npm releases before they go live: diffs a staged (or any) package version against the last one and flags supply-chain red flags such as new install scripts, credential access, obfuscated code and exfiltration endpoints. Zero dependencies.", "keywords": [ "npm",