fix(uq-publish): address review blockers + high-severity items

Addresses CodeQL polynomial-regex (HIGH), token-gate ordering, distinct
401/403/404/422 response branches, fetch timeout, expanded test coverage,
tightened owner/repo validation, and non-GitHub remote rejection.

See response thread on PR #1425 for the per-finding rationale.

Signed-off-by: amacsmith <alex.mac@looptech.ai>
This commit is contained in:
amacsmith 2026-05-08 04:38:27 -04:00
parent d6ccc186f0
commit 750025e80c
4 changed files with 349 additions and 36 deletions

View file

@ -151,6 +151,7 @@ export {
buildUqDispatchPayload,
isValidOwnerRepo,
parseOwnerRepoFromRemote,
stripGitSuffix,
} from './integrations/understand-quickly.js';
export type { UqDispatchPayload } from './integrations/understand-quickly.js';

View file

@ -64,9 +64,37 @@ export function buildUqDispatchPayload(id: string): UqDispatchPayload {
* `owner/repo` validation. Conservative on purpose: GitHub's actual
* naming rules are looser, but we want to catch local paths
* (`/Users/...`), bare slugs (`my-repo`), and accidental whitespace.
*
* Tightened (LOW 8) to match GitHub's published slug rules:
* owner: starts with alnum, then alnum/hyphen only — no underscore,
* no dot. Length cap 39.
* repo: any of alnum/dot/hyphen/underscore. Length cap 100.
*/
export function isValidOwnerRepo(id: string): boolean {
return /^[A-Za-z0-9][A-Za-z0-9._-]*\/[A-Za-z0-9._-]+$/.test(id);
return /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})\/[A-Za-z0-9._-]{1,100}$/.test(id);
}
/**
* Strip a single trailing `.git` (case-insensitive) and any trailing
* slashes from a URL-ish string. Bounded linear: each character is
* visited at most twice, no backtracking.
*
* Replaces `s.replace(/\.git\/*$/i, '').replace(/\/+$/, '')` which
* CodeQL's polynomial-regex check (codeql/js/polynomial-redos) flags as
* a worst-case O(n²) on adversarial input like "////.../x".
*/
export function stripGitSuffix(input: string): string {
let end = input.length;
// Trim trailing '/'.
while (end > 0 && input.charCodeAt(end - 1) === 0x2f) end--;
// Drop one trailing '.git' (case-insensitive).
if (end >= 4) {
const tail = input.slice(end - 4, end).toLowerCase();
if (tail === '.git') end -= 4;
}
// Trim trailing '/' that may have sat between '.git' and the rest.
while (end > 0 && input.charCodeAt(end - 1) === 0x2f) end--;
return input.slice(0, end);
}
/**
@ -86,16 +114,32 @@ export function parseOwnerRepoFromRemote(url: string | null | undefined): string
if (!trimmed) return null;
// Strip a trailing `.git` (case-insensitive) and any trailing slashes
// so https://h/o/r and https://h/o/r.git collapse to the same id.
const stripped = trimmed.replace(/\.git\/*$/i, '').replace(/\/+$/, '');
// Bounded-linear helper avoids the polynomial-regex CodeQL alert.
const stripped = stripGitSuffix(trimmed);
// SCP-form SSH (`git@host:owner/repo`).
const ssh = stripped.match(/^[^@]+@[^:]+:([^/]+)\/([^/]+)$/);
if (ssh) return `${ssh[1]}/${ssh[2]}`;
// SCP-form SSH (`git@host:owner/repo`). Capture host so we can reject
// non-GitHub remotes — a GitLab origin like
// `https://gitlab.example.com/group/sub/project.git` would otherwise
// silently dispatch the wrong id (LOW 9).
const ssh = stripped.match(/^[^@]+@([^:]+):([^/]+)\/([^/]+)$/);
if (ssh) {
const host = ssh[1].toLowerCase();
if (host !== 'github.com' && host !== 'www.github.com') return null;
return `${ssh[2]}/${ssh[3]}`;
}
// URL forms (https://, ssh://, git://, file://) — last two path segments.
const url2 = stripped.match(/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\/[^/]+\/(.+)$/);
const url2 = stripped.match(/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\/([^/]+)\/(.+)$/);
if (url2) {
const segments = url2[1].split('/').filter(Boolean);
// Strip optional `userinfo@` (e.g. `ssh://git@github.com/...`).
const authority = url2[1];
const atIdx = authority.lastIndexOf('@');
const hostAndPort = atIdx >= 0 ? authority.slice(atIdx + 1) : authority;
// Strip `:port` suffix if present.
const colonIdx = hostAndPort.indexOf(':');
const host = (colonIdx >= 0 ? hostAndPort.slice(0, colonIdx) : hostAndPort).toLowerCase();
if (host !== 'github.com' && host !== 'www.github.com') return null;
const segments = url2[2].split('/').filter(Boolean);
if (segments.length >= 2) {
const [owner, repo] = segments.slice(-2);
return `${owner}/${repo}`;

View file

@ -44,10 +44,34 @@ const REGISTER_HINT =
'Register your repo once with: npx @understand-quickly/cli add\n' +
'Or use the wizard: https://looptech-ai.github.io/understand-quickly/add.html';
/**
* Hard cap on the dispatch fetch to keep CI publish steps from stalling
* for the OS TCP timeout (~2 min) when api.github.com is unreachable.
* Matches the pattern used in `src/core/embeddings/http-client.ts`.
*/
const DISPATCH_TIMEOUT_MS = 15_000;
export const publishCommand = async (
inputPath?: string,
options: PublishOptions = {},
): Promise<void> => {
// ── 0. Token gate FIRST — guarantees true no-op without the token. ──
// The README, CLI --help, and PR body all promise "exit 0 without
// UNDERSTAND_QUICKLY_TOKEN". Doing the index/repo-root checks before
// the token gate would make those promises false for users who haven't
// run `gitnexus analyze` yet but want to verify the command is wired.
const token = process.env[UNDERSTAND_QUICKLY_TOKEN_ENV];
if (!token) {
cliInfo(
`[understand-quickly] ${UNDERSTAND_QUICKLY_TOKEN_ENV} is not set — skipping dispatch.\n` +
`Set it to a fine-grained PAT with "Repository dispatches: write" on ` +
`looptech-ai/understand-quickly to enable instant resync.\n` +
`(Without the token, the registry's nightly sync still picks up your entry.)`,
{ skipped: 'no-token' },
);
return;
}
// ── 1. Resolve the repo root (same precedence as `analyze`) ──────────
let repoPath: string;
if (inputPath) {
@ -93,22 +117,8 @@ export const publishCommand = async (
return;
}
// ── 4. Token gate: no token → informational no-op (exit 0) ───────────
const token = process.env[UNDERSTAND_QUICKLY_TOKEN_ENV];
if (!token) {
cliInfo(
`[understand-quickly] ${UNDERSTAND_QUICKLY_TOKEN_ENV} is not set — skipping dispatch.\n` +
`Set it to a fine-grained PAT with "Repository dispatches: write" on ` +
`looptech-ai/understand-quickly to enable instant resync.\n` +
`(Without the token, the registry's nightly sync still picks up ${id}.)`,
{ id, skipped: 'no-token' },
);
return;
}
// ── 5. Fire the dispatch ─────────────────────────────────────────────
// ── 4. Fire the dispatch ─────────────────────────────────────────────
const payload = buildUqDispatchPayload(id);
const commit = getCurrentCommit(repoPath);
let response: Response;
try {
response = await fetch(UNDERSTAND_QUICKLY_DISPATCH_URL, {
@ -118,43 +128,94 @@ export const publishCommand = async (
Authorization: `Bearer ${token}`,
'X-GitHub-Api-Version': '2022-11-28',
'Content-Type': 'application/json',
'User-Agent': 'gitnexus-cli',
},
body: JSON.stringify(payload),
signal: AbortSignal.timeout(DISPATCH_TIMEOUT_MS),
});
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
cliError(`[understand-quickly] dispatch network error: ${msg}`, { id });
if (err instanceof Error && err.name === 'AbortError') {
cliError(
`[understand-quickly] dispatch timed out after ${DISPATCH_TIMEOUT_MS}ms. ` +
`Check network access to api.github.com and retry.`,
{ id },
);
} else {
const msg = err instanceof Error ? err.message : String(err);
cliError(`[understand-quickly] dispatch network error: ${msg}`, { id });
}
process.exitCode = 1;
return;
}
// GitHub returns 204 on success, 404 when the token can't reach the
// registry repo, 401 when the token is invalid. Surface these
// distinctly so users debug without checking the docs.
// GitHub returns 204 on success. Distinct branches for 401/403/404/422
// so users debug without checking the docs.
if (response.status === 204) {
await response.body?.cancel().catch(() => {});
// `getCurrentCommit` is only meaningful in the success path — moving
// it inside this branch removes a wasted child-process spawn on every
// error response (LOW 7).
const commit = getCurrentCommit(repoPath);
cliInfo(
`[understand-quickly] dispatched sync-entry for ${id}` +
(commit ? ` @ ${commit.slice(0, 7)}` : '') +
'.\n' +
`View the workflow run: ` +
`Note: a 204 only confirms GitHub accepted the dispatch. Whether the ` +
`registry workflow finds an entry for "${id}" is logged at ` +
`https://github.com/looptech-ai/understand-quickly/actions/workflows/sync.yml`,
{ id, commit, status: response.status },
);
return;
}
if (response.status === 404) {
if (response.status === 401) {
cliError(
`[understand-quickly] dispatch returned 404 — the token cannot reach ` +
`looptech-ai/understand-quickly. Verify the PAT has Repository access ` +
`to that repo and the "Repository dispatches: write" permission.`,
`[understand-quickly] dispatch returned 401 — the ${UNDERSTAND_QUICKLY_TOKEN_ENV} value is invalid or expired.\n` +
`Regenerate a fine-grained PAT at https://github.com/settings/personal-access-tokens ` +
`with Repository access scoped to looptech-ai/understand-quickly and the ` +
`"Repository dispatches: write" permission, then retry.`,
{ id, status: response.status },
);
process.exitCode = 1;
return;
}
// 401, 403, 422, 5xx → bubble up the body so the user can act.
if (response.status === 403) {
cliError(
`[understand-quickly] dispatch returned 403 — the token authenticated but ` +
`lacks the "Repository dispatches: write" permission on ` +
`looptech-ai/understand-quickly. Edit the PAT scopes and retry.`,
{ id, status: response.status },
);
process.exitCode = 1;
return;
}
if (response.status === 404) {
cliError(
`[understand-quickly] dispatch returned 404 — the token cannot reach ` +
`looptech-ai/understand-quickly. Verify the PAT has Repository access to ` +
`that exact repo (not just your own org).`,
{ id, status: response.status },
);
process.exitCode = 1;
return;
}
if (response.status === 422) {
// Malformed event_type / client_payload — a code bug in this CLI,
// not a user mistake. Surface so we get bug reports.
const body422 = await response.text().catch(() => '');
cliError(
`[understand-quickly] dispatch returned 422 (this is a CLI bug; please report).\n` +
`Body: ${body422 || '(empty)'}`,
{ id, status: response.status },
);
process.exitCode = 1;
return;
}
// 5xx and anything else → bubble the body so the user has something to act on.
const body = await response.text().catch(() => '');
cliError(
`[understand-quickly] dispatch failed with HTTP ${response.status}: ${body || '(empty body)'}`,

View file

@ -1,11 +1,13 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { afterEach, beforeEach, describe, expect, it, test, vi } from 'vitest';
import fs from 'fs/promises';
import os from 'os';
import path from 'path';
import { performance } from 'node:perf_hooks';
import {
buildUqDispatchPayload,
isValidOwnerRepo,
parseOwnerRepoFromRemote,
stripGitSuffix,
UNDERSTAND_QUICKLY_TOKEN_ENV,
} from 'gitnexus-shared';
@ -14,28 +16,77 @@ describe('understand-quickly helpers (gitnexus-shared)', () => {
it.each([
['looptech-ai/understand-quickly', true],
['abhigyanpatwari/GitNexus', true],
['Some_Org/Some.Repo-2', true],
// LOW 8: GitHub user/org slugs are alnum/hyphen only — no underscore.
['Some_Org/Some.Repo-2', false],
['', false],
['just-a-name', false],
['/Users/me/code/repo', false],
['org/with spaces', false],
['org//double', false],
// LOW 8 additions:
['some_org/repo', false], // underscore in owner — invalid
['-org/repo', false], // leading hyphen — invalid
['org-/repo', true], // trailing hyphen — GitHub actually allows this
['org/repo_with_underscore', true],
['org/.dotfile', true], // repos may start with dot
])('returns %s for %j', (id, expected) => {
expect(isValidOwnerRepo(id as string)).toBe(expected);
});
});
describe('stripGitSuffix (BLOCKER 1 — ReDoS-safe)', () => {
it.each([
['https://github.com/o/r.git', 'https://github.com/o/r'],
['https://github.com/o/r.git/', 'https://github.com/o/r'],
['https://github.com/o/r/', 'https://github.com/o/r'],
['https://github.com/o/r', 'https://github.com/o/r'],
['https://github.com/o/r.GIT', 'https://github.com/o/r'],
['https://github.com/o/r//', 'https://github.com/o/r'],
['', ''],
['/', ''],
])('strips %j -> %j', (input, expected) => {
expect(stripGitSuffix(input)).toBe(expected);
});
test('linear time on adversarial trailing slashes (regression for ReDoS)', () => {
const adversarial = 'https://github.com/o/r' + '/'.repeat(10_000);
const start = performance.now();
const result = stripGitSuffix(adversarial);
const elapsed = performance.now() - start;
expect(result).toBe('https://github.com/o/r');
expect(elapsed).toBeLessThan(50); // generous; should be sub-millisecond
});
test('parseOwnerRepoFromRemote terminates quickly on adversarial input', () => {
const adversarial = 'https://github.com/o/r.git' + '/'.repeat(10_000);
const start = performance.now();
const result = parseOwnerRepoFromRemote(adversarial);
const elapsed = performance.now() - start;
expect(result).toBe('o/r');
expect(elapsed).toBeLessThan(50);
});
});
describe('parseOwnerRepoFromRemote', () => {
it.each([
['git@github.com:looptech-ai/understand-quickly.git', 'looptech-ai/understand-quickly'],
['https://github.com/looptech-ai/understand-quickly', 'looptech-ai/understand-quickly'],
['https://github.com/looptech-ai/understand-quickly.git', 'looptech-ai/understand-quickly'],
['ssh://git@github.com/abhigyanpatwari/GitNexus.git', 'abhigyanpatwari/GitNexus'],
['https://gitlab.example.com/group/sub/project.git', 'sub/project'],
])('parses %s -> %s', (url, expected) => {
expect(parseOwnerRepoFromRemote(url)).toBe(expected);
});
// LOW 9: non-GitHub remotes must be rejected — a wrong id is worse
// than no id, since the user can always pass --id explicitly.
it.each([
['https://gitlab.example.com/group/sub/project.git'],
['git@gitlab.example.com:group/sub/project.git'],
['https://bitbucket.org/team/repo.git'],
])('returns null for non-GitHub host %j', (input) => {
expect(parseOwnerRepoFromRemote(input)).toBeNull();
});
it.each([null, undefined, '', ' ', 'not-a-url', 'https://github.com/'])(
'returns null for %j',
(input) => {
@ -102,4 +153,160 @@ describe('publishCommand (no-token no-op)', () => {
expect(process.exitCode ?? 0).toBe(0);
fetchSpy.mockRestore();
});
it('exits 0 with no token even when no index/repo exists (BLOCKER 2)', async () => {
// Per the README, CLI --help, and PR body: without a token, the
// command must be a no-op even if the repo lacks `.gitnexus/`.
const noIndexDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-publish-noidx-'));
try {
const fetchSpy = vi.spyOn(globalThis, 'fetch').mockImplementation(() => {
throw new Error('publishCommand should NOT call fetch when the token is missing');
});
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(noIndexDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(fetchSpy).not.toHaveBeenCalled();
expect(process.exitCode ?? 0).toBe(0);
fetchSpy.mockRestore();
} finally {
await fs.rm(noIndexDir, { recursive: true, force: true });
}
});
});
describe('publishCommand response branches (MEDIUM 5)', () => {
let tempDir: string;
let originalToken: string | undefined;
let exitCodeBefore: number | undefined;
let fetchSpy: ReturnType<typeof vi.spyOn>;
beforeEach(async () => {
vi.resetModules();
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-publish-resp-'));
await fs.mkdir(path.join(tempDir, '.gitnexus'), { recursive: true });
await fs.writeFile(
path.join(tempDir, '.gitnexus', 'meta.json'),
JSON.stringify({ repoPath: tempDir, lastCommit: '', indexedAt: '' }),
'utf-8',
);
originalToken = process.env[UNDERSTAND_QUICKLY_TOKEN_ENV];
process.env[UNDERSTAND_QUICKLY_TOKEN_ENV] = 'pat_test';
exitCodeBefore = process.exitCode;
process.exitCode = 0;
fetchSpy = vi.spyOn(globalThis, 'fetch');
});
afterEach(async () => {
if (originalToken !== undefined) {
process.env[UNDERSTAND_QUICKLY_TOKEN_ENV] = originalToken;
} else {
delete process.env[UNDERSTAND_QUICKLY_TOKEN_ENV];
}
process.exitCode = exitCodeBefore;
vi.restoreAllMocks();
await fs.rm(tempDir, { recursive: true, force: true });
});
function mockResponse(status: number, body = '') {
fetchSpy.mockResolvedValueOnce({
status,
ok: status >= 200 && status < 300,
text: async () => body,
body: { cancel: async () => {} },
headers: new Headers(),
} as unknown as Response);
}
it('204 → exit 0 with success message', async () => {
mockResponse(204);
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(fetchSpy).toHaveBeenCalledTimes(1);
expect(process.exitCode ?? 0).toBe(0);
});
it('401 → exit 1 with PAT-invalid hint', async () => {
mockResponse(401, '{"message":"Bad credentials"}');
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
});
it('403 → exit 1 with scope-missing hint', async () => {
mockResponse(403, '{"message":"Resource not accessible"}');
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
});
it('404 → exit 1 with repo-access hint', async () => {
mockResponse(404, '{"message":"Not Found"}');
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
});
it('5xx → exit 1 with raw body', async () => {
mockResponse(503, 'gateway timeout');
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
});
it('network throw → exit 1', async () => {
fetchSpy.mockRejectedValueOnce(new Error('ECONNRESET'));
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
});
it('AbortError (HIGH 4 — fetch timeout) → exit 1 with timed-out message', async () => {
const abort = new Error('aborted');
abort.name = 'AbortError';
fetchSpy.mockRejectedValueOnce(abort);
const errSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true);
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
expect(process.exitCode).toBe(1);
const written = errSpy.mock.calls.map((c) => String(c[0])).join('');
expect(written).toMatch(/timed out/i);
errSpy.mockRestore();
});
it('token never appears in any logged output', async () => {
process.env[UNDERSTAND_QUICKLY_TOKEN_ENV] = 'pat_secret_value';
mockResponse(401, '');
const errSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true);
const { publishCommand } = await import('../../src/cli/publish.js');
await publishCommand(tempDir, {
id: 'looptech-ai/understand-quickly',
skipGit: true,
});
const written = errSpy.mock.calls.map((c) => String(c[0])).join('');
expect(written).not.toContain('pat_secret_value');
errSpy.mockRestore();
});
});