fe2746198c
* fix: 6 unaddressed log-audit / run-audit findings - #836 + #818 (clerk middleware SyntaxError on action-runtime endpoints): narrow proxy.ts matcher to exclude /api/repo/<owner>/<repo>/run-context, /api/runtime/*, /api/proxy-token. these are server-to-server with their own auth and have no Clerk session to evaluate, so clerkMiddleware's decodeJwt throws turn into 500s on every request. - #837 (npx EBADDEVENGINES on customer's package.json): change runCli's bootstrap cwd from $GITHUB_WORKSPACE to os.tmpdir() so npm v11+ doesn't enforce devEngines.packageManager from the customer's tree before our bootstrap can install pullfrog. CLI process.chdir's to payload.cwd internally, so the runtime work still happens in $GITHUB_WORKSPACE. - #838 (createWorkflowDispatch silently dropped 39 user runs on a 5xx spike): add bounded retry (3 attempts, ~750ms total) on Octokit 5xx and network errors inside dispatchReservedRun. preserves the existing 422 "Unexpected inputs" path. retry budget stays well under GitHub's 10s webhook redelivery window. - #833 (bail() redirect("/signout") propagated NEXT_REDIRECT into webhook handlers, 40+ 500s/24h): drop the redirect side-effect; bail now just classifies bad-credentials as non-retryable and propagates. UI flows that wanted auto-signout on revoked tokens can detect it themselves; the side-effect was wrong for any non-page caller. - #835 (BYOK provider billing-exhausted fell through to raw error renderer, 37 review-mode runs/24h with no PR-side signal): - extend providerErrors patterns with Anthropic "credit balance is too low" + extract isProviderBillingExhausted / extractProviderId helpers - add a renderer branch in runErrorRenderer.ts that names the provider (parsed from providerID=) and links to its billing dashboard - route handleAgentResult's !result.success path through the renderer + reportErrorToComment with createIfMissing, so review-mode and silent triggers get an actionable PR comment regardless of mode - #834 (post-hook ERR_MODULE_NOT_FOUND already fixed on main, hardening): - add a vitest invariant that walks the entryPost.ts import graph and refuses any non-relative / non-node: specifier — catches the next `@actions/core` slip-up before publish - add an analyze-logs classifier so future entryPost crashes surface as failure:post-hook-module-not-found instead of hiding inside failure:unknown / failure:git-lock-file Co-authored-by: Cursor <cursoragent@cursor.com> * anneal: fix dead-code matcher, 404 url, silent-trigger gate, and grammar regression Round-1 anneal pass on the audit-fixes PR surfaced two critical issues + several majors that the original fixes shipped with: - proxy.ts matcher 1 (`(?!_next|[^?]*\.(...))`) still caught every /api/... route because matcher arrays are OR'd. The narrowing in matcher 2 was dead code → middleware still 500'd on /api/runtime/*, /api/proxy-token, /api/repo/.../run-context. Carve-out now lives in BOTH matchers. - opencode.ai/billing returns 404; canonical top-up surface is /zen. deepseek /usage is the consumption page, not the top-up flow (/top_up is correct). google /apikey is the keys list, not billing (/usage is the spend dashboard). All three URL strings updated. - handleAgentResult gated reportErrorToComment behind `if (!ctx.silent)`, contradicting the createIfMissing intent — silent IncrementalReview / pull_request_synchronize / auto-label still got zero PR signal on BYOK billing exhaustion, the exact failure mode #835 was meant to fix. Moved createIfMissing into finalizeSuccessRun's existing render-and- post block (single source of truth), reverted handleAgentResult to its prior shape. Side benefit: drops the double-PATCH that fired on every non-silent !success path with an existing progress comment. - Anthropic-direct error rendered "**Your your provider account is out of credit.**" because Anthropic SDK has no providerID= tag, so extractProviderId returned null and the headline composed "Your " + "your provider". Added detectProviderId Anthropic fallback (matches "Anthropic API" / "credit balance is too low") so the link is reachable AND made the headline conditional on whether a provider id was detected. - Reordered renderRunError classifier: BYOK billing-exhausted now runs BEFORE api-key auth detection. Providers commonly return 401 for billing exhaustion (DeepSeek, Gemini), and the OpenCode harness logs often include "API Error: 401" in the raw error body, which isApiKeyAuthError would otherwise match — surfacing "rotate your key" when the actual fix is "top up credits". - isTransientUpstreamError missed ENOTFOUND / ENETUNREACH / EHOSTUNREACH (undici DNS-class failures Octokit doesn't wrap with a status). Added to the prefix alternation. - Tightened "10s webhook redelivery budget" / "GitHub redelivers" wording in triggerWorkflow.ts and bail.ts JSDoc — GitHub's 10s is the response timeout (it doesn't auto-redeliver); upstream webhook proxy retries are what multiplied the failure. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal r2: unbreak proxy.ts matcher 2, extend carve-out, mkdtemp bootstrap Round-2 anneal (security + cross-cutting lenses) on top of the round-1 audit-fixes commit caught a critical regression + several majors: - proxy.ts matcher 2 from r1 (`/(api|trpc)(?!...)(.*)`) does NOT compile. path-to-regexp rejects a top-level `(?!` after `)` as "Pattern cannot start with '?'", and Next.js's SourceSchema runs the same validator at build time and aborts via `process.exit(1)`. PR #840's Vercel + preview deployments have been failing since 978eca26 for exactly this reason. Fixed by nesting the lookahead inside an outer parameter group: `/(api|trpc)((?!...).*)`. Same shape matcher 1 already uses, which is why m1 always compiled. Verified `pnpm next build` succeeds end-to-end. - proxy.ts carve-out was incomplete relative to its own justification. The "no Clerk session, decodeJwt 500" failure mode applies to ALL server-to-server action-runtime endpoints — five more share the exact shape: /api/repo/.../learnings, /api/repo/.../pr/.../summary-comment, /api/repo/.../issue/.../plan-comment, /api/workflow-run/, and /api/github/installation-token. Extended both matchers. /api/upload/ signed-url stays in (dual auth: Clerk session OR bearer JWT — needs middleware for the user path). - proxy.ts carve-outs were unanchored: a future /api/proxy-token-info, /api/proxy-tokens, or /api/repo/X/Y/run-context-foo would silently bypass Clerk. Added `(?:$|/)` for path-prefix carve-outs (allow exact match or sub-path), `$` for routes with browser-callable siblings (e.g. `learnings/history` is browser-Clerk, `learnings$` is action- bearer-JWT). Verified 30/31 routes via path-to-regexp test harness. - runCli.ts cwd flipped from $GITHUB_WORKSPACE to os.tmpdir() in r1 (#837 fix for npm v11 devEngines.packageManager EBADDEVENGINES). But $TMPDIR is overridable from a prior $GITHUB_ENV step — a customer- authored or compromised prior step can plant /atk/node_modules/ pullfrog/ and `echo "TMPDIR=/atk" >> $GITHUB_ENV`, and our npx --yes pullfrog@<v> bootstrap resolves the local install first, executing attacker code with full action env (provider keys, OIDC, installation token, CODEX_AUTH_JSON). Switched to mkdtempSync(join (tmpdir(), "pullfrog-bootstrap-")) — fresh per-invocation 0700 dir, not pre-writable by anything earlier in the job. - runLifecycle.ts: writeRunErrorOutputs (catch-path) didn't pass createIfMissing: true, contradicting the symmetric intent of the r1 finalizeSuccessRun fix. Silent triggers (IncrementalReview / pull_request_synchronize / auto-label) that throw past the success path still got zero PR signal — exact failure mode #835 was meant to close. Now both paths pass createIfMissing: true. - runErrorRenderer.ts detectProviderId regex `/Anthropic API|credit balance is too low/i` could mis-tag a non-Anthropic billing-exhausted error that mentioned "Anthropic API" in passing (fallback-chain agent prompt text, OpenCode harness logs). Tightened to /credit balance is too low/i — Anthropic-specific phrasing, sufficient for the direct- Anthropic SDK case the fallback exists to handle. - runErrorRenderer.ts JSDoc: r1's classifier reorder put hang at #6 in code but the JSDoc still listed it at #2. Reordered the doc to match dispatch order, with explicit note that hang is a sub-source for the api-key check (which is why hangBody is precomputed early). - analyze-logs.ts: ERR_MODULE_NOT_FOUND.*entryPost regex needs `s` flag so it survives Node v23+ stack-trace reformatting onto multiple lines. One-char fix to defend the #834 classification bucket. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
258 lines
9.0 KiB
TypeScript
258 lines
9.0 KiB
TypeScript
import { execFileSync } from "node:child_process";
|
|
import { accessSync, constants, existsSync, mkdtempSync } from "node:fs";
|
|
import { tmpdir } from "node:os";
|
|
import { delimiter, dirname, isAbsolute, join, resolve, sep } from "node:path";
|
|
import { fileURLToPath } from "node:url";
|
|
import actionPackageJson from "./package.json" with { type: "json" };
|
|
|
|
interface RunPullfrogCliParams {
|
|
cliArgs: string[];
|
|
swallowErrors?: boolean;
|
|
}
|
|
|
|
interface RuntimeContext {
|
|
actionRef: string | undefined;
|
|
actionRepository: string | undefined;
|
|
actionRoot: string;
|
|
nodeBinDir: string;
|
|
env: NodeJS.ProcessEnv;
|
|
}
|
|
|
|
const NPM_REGISTRY = "https://registry.npmjs.org";
|
|
const FALLBACK_PACKAGE_SPEC = `pullfrog@^${actionPackageJson.version}`;
|
|
|
|
function getErrorMessage(error: unknown): string {
|
|
return error instanceof Error ? error.message : String(error);
|
|
}
|
|
|
|
function canAccessExecutable(path: string): boolean {
|
|
try {
|
|
accessSync(path, constants.X_OK);
|
|
return true;
|
|
} catch {
|
|
if (process.platform !== "win32") {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
try {
|
|
accessSync(path, constants.F_OK);
|
|
return true;
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
// reject PATH entries that an attacker can plausibly write to before pullfrog
|
|
// runs. specifically: relative entries (., bin, etc., which resolve against
|
|
// cwd), and anything inside the customer's checkout. an attacker who can land
|
|
// a malicious `npx` in the repo and prepend `$GITHUB_WORKSPACE/bin` to
|
|
// `GITHUB_PATH` from a prior workflow step would otherwise get full code
|
|
// execution under our action token.
|
|
//
|
|
// on Windows the filesystem is case-insensitive but `resolve()` preserves
|
|
// input case, so we lowercase both sides before comparing — otherwise an
|
|
// attacker can bypass the filter by varying the case of GITHUB_WORKSPACE in
|
|
// their injected PATH entry (`d:\a\repo` vs `D:\a\repo`).
|
|
function normalizePathForCompare(path: string): string {
|
|
return process.platform === "win32" ? resolve(path).toLowerCase() : resolve(path);
|
|
}
|
|
|
|
function isUntrustedPathEntry(entry: string, untrustedRoots: string[]): boolean {
|
|
if (!isAbsolute(entry)) return true;
|
|
const normalized = normalizePathForCompare(entry);
|
|
for (const root of untrustedRoots) {
|
|
if (normalized === root) return true;
|
|
if (normalized.startsWith(root + sep)) return true;
|
|
}
|
|
return false;
|
|
}
|
|
|
|
function getUntrustedPathRoots(env: NodeJS.ProcessEnv): string[] {
|
|
const roots: string[] = [];
|
|
const workspace = env.GITHUB_WORKSPACE;
|
|
if (workspace && isAbsolute(workspace)) roots.push(normalizePathForCompare(workspace));
|
|
return roots;
|
|
}
|
|
|
|
function resolveExecutable(params: { command: string; env: NodeJS.ProcessEnv }): string | null {
|
|
const pathValue = params.env.PATH ?? "";
|
|
const untrustedRoots = getUntrustedPathRoots(params.env);
|
|
const pathEntries = pathValue
|
|
.split(delimiter)
|
|
.filter(Boolean)
|
|
.filter((entry) => !isUntrustedPathEntry(entry, untrustedRoots));
|
|
const extensions =
|
|
process.platform === "win32"
|
|
? (params.env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD").split(";").filter(Boolean)
|
|
: [""];
|
|
|
|
for (const pathEntry of pathEntries) {
|
|
for (const extension of extensions) {
|
|
const candidate = join(pathEntry, `${params.command}${extension.toLowerCase()}`);
|
|
if (canAccessExecutable(candidate)) {
|
|
return candidate;
|
|
}
|
|
}
|
|
}
|
|
|
|
return null;
|
|
}
|
|
|
|
function createRuntimeContext(): RuntimeContext {
|
|
const actionRoot = dirname(fileURLToPath(import.meta.url));
|
|
const nodeBinDir = dirname(process.execPath);
|
|
const env: NodeJS.ProcessEnv = { ...process.env };
|
|
env.npm_config_registry = NPM_REGISTRY;
|
|
env.COREPACK_NPM_REGISTRY = NPM_REGISTRY;
|
|
// bypass customer-side release-age gates (npm's `min-release-age`, pnpm's
|
|
// `minimumReleaseAge`) so our bootstrap can resolve the latest publish.
|
|
// pullfrog's npm version is server-stamped from a SHA-pinned action ref the
|
|
// customer already vets at the action layer — not a customer-vetted dep, so
|
|
// the gate is the wrong affordance here. env beats .npmrc in both tools.
|
|
// npm uses `npm_config_*`; pnpm v11+ requires `pnpm_config_*` (the v10→v11
|
|
// migration renamed the prefix). tracked: #713
|
|
env.npm_config_min_release_age = "0";
|
|
env.pnpm_config_minimum_release_age = "0";
|
|
const currentPath = process.env.PATH ?? "";
|
|
env.PATH = currentPath ? `${nodeBinDir}${delimiter}${currentPath}` : nodeBinDir;
|
|
|
|
return {
|
|
actionRef: process.env.GITHUB_ACTION_REF,
|
|
actionRepository: process.env.GITHUB_ACTION_REPOSITORY,
|
|
actionRoot,
|
|
nodeBinDir,
|
|
env,
|
|
};
|
|
}
|
|
|
|
// $GITHUB_WORKSPACE is the customer's repo. running `npx --yes pullfrog@…`
|
|
// there makes npm read THEIR `package.json` first, which on npm v11+ enforces
|
|
// `devEngines.packageManager` and aborts the bootstrap with EBADDEVENGINES
|
|
// before the agent ever boots. our bootstrap doesn't need anything from the
|
|
// customer's tree — a freshly-created tmpdir is package.json-free and
|
|
// parent-less, so npm walks up to `/` finding nothing. see #837.
|
|
//
|
|
// `mkdtempSync` (vs raw `tmpdir()`): `$TMPDIR` is overridable from a prior
|
|
// `$GITHUB_ENV` step, and a customer-authored or compromised prior step
|
|
// could plant `node_modules/pullfrog/` in the resolved tmpdir to hijack
|
|
// `npx --yes pullfrog@<version>` resolution. a fresh per-invocation
|
|
// subdirectory is mode 0700 and not pre-writable by anything earlier in
|
|
// the job.
|
|
function runCommand(params: { context: RuntimeContext; command: string; args: string[] }): void {
|
|
execFileSync(params.command, params.args, {
|
|
cwd: mkdtempSync(join(tmpdir(), "pullfrog-bootstrap-")),
|
|
stdio: "inherit",
|
|
env: params.context.env,
|
|
});
|
|
}
|
|
|
|
// resolve a launcher binary by walking PATH (which already has the action
|
|
// runtime's nodeBinDir prepended). some hosted Node 24 runner pools ship
|
|
// `node` at `externals/node24/bin/node` without the sibling `npx`/`corepack`,
|
|
// so a hardcoded sibling path can't be relied on — fall back to whatever the
|
|
// runner image provides on PATH.
|
|
function requireExecutable(params: {
|
|
context: RuntimeContext;
|
|
command: string;
|
|
purpose: string;
|
|
}): string {
|
|
const resolved = resolveExecutable({ command: params.command, env: params.context.env });
|
|
if (!resolved) {
|
|
throw new Error(
|
|
`could not find ${params.command} on PATH (needed to ${params.purpose}); ` +
|
|
`runtime PATH was: ${params.context.env.PATH ?? "<empty>"}`
|
|
);
|
|
}
|
|
return resolved;
|
|
}
|
|
|
|
function runPackageCli(context: RuntimeContext, packageSpec: string, cliArgs: string[]): void {
|
|
const npxPath = resolveExecutable({ command: "npx", env: context.env });
|
|
if (npxPath) {
|
|
runCommand({ context, command: npxPath, args: ["--yes", packageSpec, ...cliArgs] });
|
|
return;
|
|
}
|
|
|
|
const corepackPath = resolveExecutable({ command: "corepack", env: context.env });
|
|
if (corepackPath) {
|
|
console.warn("» npx not found, using corepack pnpm dlx");
|
|
runCommand({ context, command: corepackPath, args: ["pnpm", "dlx", packageSpec, ...cliArgs] });
|
|
return;
|
|
}
|
|
|
|
throw new Error(
|
|
`could not find npx or corepack on PATH to run ${packageSpec}; ` +
|
|
`runtime PATH was: ${context.env.PATH ?? "<empty>"}`
|
|
);
|
|
}
|
|
|
|
function ensureActionDependencies(context: RuntimeContext): void {
|
|
const nodeModulesPath = join(context.actionRoot, "node_modules");
|
|
if (existsSync(nodeModulesPath)) {
|
|
return;
|
|
}
|
|
|
|
const corepackPath = requireExecutable({
|
|
context,
|
|
command: "corepack",
|
|
purpose: "install action dependencies via pnpm",
|
|
});
|
|
const adjacentCorepack = join(
|
|
context.nodeBinDir,
|
|
process.platform === "win32" ? "corepack.cmd" : "corepack"
|
|
);
|
|
if (corepackPath !== adjacentCorepack) {
|
|
// bad-runner case: GitHub's externals/node24/bin/ is missing the corepack
|
|
// sibling, so we resolved via PATH instead. logging this lets us correlate
|
|
// bootstrap path to runner pool when validating the fix.
|
|
console.warn(
|
|
`» nodeBinDir corepack missing (${adjacentCorepack}); using PATH-resolved ${corepackPath}`
|
|
);
|
|
}
|
|
execFileSync(corepackPath, ["pnpm", "install", "--frozen-lockfile", "--ignore-scripts"], {
|
|
cwd: context.actionRoot,
|
|
stdio: "inherit",
|
|
env: context.env,
|
|
});
|
|
}
|
|
|
|
function runLocalCli(context: RuntimeContext, cliArgs: string[]): void {
|
|
ensureActionDependencies(context);
|
|
execFileSync(process.execPath, ["cli.ts", ...cliArgs], {
|
|
cwd: context.actionRoot,
|
|
stdio: "inherit",
|
|
env: context.env,
|
|
});
|
|
}
|
|
|
|
function runPullfrogCliInner(context: RuntimeContext, cliArgs: string[]): void {
|
|
if (process.env.PULLFROG_FORCE_LOCAL_CLI === "1") {
|
|
runLocalCli(context, cliArgs);
|
|
return;
|
|
}
|
|
|
|
if (context.actionRef === "main" && context.actionRepository === "pullfrog/pullfrog") {
|
|
runLocalCli(context, cliArgs);
|
|
return;
|
|
}
|
|
|
|
runPackageCli(context, FALLBACK_PACKAGE_SPEC, cliArgs);
|
|
}
|
|
|
|
export function runPullfrogCli(params: RunPullfrogCliParams): void {
|
|
const context = createRuntimeContext();
|
|
|
|
if (params.swallowErrors) {
|
|
try {
|
|
runPullfrogCliInner(context, params.cliArgs);
|
|
} catch (error) {
|
|
console.warn(`» pullfrog cleanup bootstrap failed: ${getErrorMessage(error)}`);
|
|
// best-effort cleanup
|
|
}
|
|
return;
|
|
}
|
|
|
|
runPullfrogCliInner(context, params.cliArgs);
|
|
}
|