Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,15 @@ gate:
size:
mode: off

# Lockfile-tamper-risk gate. Scans a changed package-lock.json diff for a
# resolved/integrity value that changed WITHOUT the same package's version
# changing in a changed package.json, or a resolved URL outside
# registry.npmjs.org — the classic supply-chain hand-edit tell. Distinct
# from the OSV.dev known-CVE dependency scan (a different threat model).
# off | advisory | block. Default: off. Config-as-code only — no DB column
# or dashboard toggle; this can only be set here.
lockfileIntegrity: off

# Composite merge-readiness gate (no min score).
# off | advisory | block. Default: off.
mergeReadiness: off
Expand Down
8 changes: 8 additions & 0 deletions apps/gittensory-ui/public/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -8720,6 +8720,14 @@
"type": "integer",
"minimum": 0,
"exclusiveMinimum": true
},
"lockfileIntegrityGateMode": {
"type": "string",
"enum": [
"off",
"advisory",
"block"
]
}
},
"required": [
Expand Down
1 change: 1 addition & 0 deletions src/openapi/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,7 @@ export const RepositorySettingsSchema = z
qualityGateMinScore: z.number().nullable().optional(),
slopGateMode: z.enum(["off", "advisory", "block"]),
sizeGateMode: z.enum(["off", "advisory", "block"]).optional(),
lockfileIntegrityGateMode: z.enum(["off", "advisory", "block"]).optional(),
gateDryRun: z.boolean().optional(),
premergeContentRecheck: z.boolean().optional(),
requireFreshRebaseWindowMinutes: z.number().int().positive().nullable().optional(),
Expand Down
53 changes: 53 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ import {
} from "../review/inline-comments";
import { evaluatePreMergeChecks } from "../review/pre-merge-checks";
import { secretLeakFinding } from "../review/safety";
import { lockfileTamperRiskFinding } from "../review/lockfile-tamper";
import {
buildIssuePlanComment,
classifyPlanCommandRequest,
Expand Down Expand Up @@ -430,6 +431,7 @@ import type {
ContributorEvidenceRecord,
ContributorRepoStatRecord,
DetectedNotificationEvent,
GateRuleMode,
GitHubWebhookPayload,
IssueRecord,
JobMessage,
Expand Down Expand Up @@ -4895,6 +4897,7 @@ export function gateCheckPolicy(
// thresholds default to 10 files / 1000 lines (advisory.ts constants); the live counts + guardrail-hit come from
// the per-PR sizeContext threaded by the caller.
sizeGateMode: settings.sizeGateMode,
lockfileIntegrityGateMode: settings.lockfileIntegrityGateMode,
changedFileCount: sizeContext?.changedFileCount ?? null,
changedLineCount: sizeContext?.changedLineCount ?? null,
guardrailHit: sizeContext?.guardrailHit ?? false,
Expand Down Expand Up @@ -5587,6 +5590,46 @@ export async function maybeAddSecretLeakFinding(
}
}

/**
* Lockfile-tamper-risk scan (#2563, opt-in via `lockfileIntegrityGateMode`). Scans a changed
* `package-lock.json`'s diff for a `resolved`/`integrity` value that changed without the corresponding
* `package.json` dependency version changing, or a `resolved` URL outside `registry.npmjs.org`, and on a hit
* appends ONE warning-severity `lockfile_tamper_risk` finding to the advisory BEFORE evaluateGateCheck runs —
* the gate treats that code as a blocker only when the repo has set `lockfileIntegrityGateMode: block`
* (rules/advisory.ts). Mode `off` (the default) skips the scan entirely so the advisory/gate stays
* byte-identical to today. Fail-safe: a file-load error is swallowed so it can never destabilize the gate.
*/
export async function maybeAddLockfileTamperFinding(
env: Env,
args: {
advisory: Awaited<ReturnType<typeof buildPullRequestAdvisory>>;
repoFullName: string;
pullNumber: number;
lockfileIntegrityGateMode: GateRuleMode | undefined;
files: Awaited<ReturnType<typeof listPullRequestFiles>> | null;
},
): Promise<void> {
if (!args.lockfileIntegrityGateMode || args.lockfileIntegrityGateMode === "off") return;
try {
const files =
args.files ??
(await listPullRequestFiles(env, args.repoFullName, args.pullNumber));
const finding = lockfileTamperRiskFinding(files);
if (finding) args.advisory.findings.push(finding);
} catch (error) {
/* v8 ignore next -- fail-safe: a file-load error never destabilizes the gate. */
console.error(
JSON.stringify({
level: "error",
event: "lockfile_tamper_scan_failed",
repository: args.repoFullName,
pullNumber: args.pullNumber,
error: errorMessage(error),
}),
);
}
}

/**
* AI-assisted slop advisory (opt-in `slopAiAdvisory`). Appends at most one ADVISORY-only `ai_slop_advisory`
* finding to the advisory; NEVER touches slopRisk or the gate (only the deterministic core can block). The
Expand Down Expand Up @@ -6677,6 +6720,16 @@ async function maybePublishPrPublicSurface(
files: await getReviewFiles(),
});

// Lockfile-tamper-risk scan (#2563): opt-in via `lockfileIntegrityGateMode` (default off — the scan is
// skipped entirely). getReviewFiles() is memoized, so this reuses the already-loaded diff when present.
await maybeAddLockfileTamperFinding(env, {
advisory,
repoFullName,
pullNumber: pr.number,
lockfileIntegrityGateMode: settings.lockfileIntegrityGateMode,
files: await getReviewFiles(),
});

// Unresolved GitHub review threads (for example external security scanner inline findings) are blocking
// review facts. Fetch them before gate evaluation so the normal blocker path drives the check-run, comment,
// and disposition consistently. Fail-open on GitHub/GraphQL errors: a transient thread-read failure should not
Expand Down
182 changes: 182 additions & 0 deletions src/review/lockfile-tamper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
// Lockfile-tamper-risk gate check (#2563). Deterministic scan of a changed `package-lock.json` (or another
// `*.lock` file) diff for the classic supply-chain tell: a `resolved`/`integrity` value changed WITHOUT the
// corresponding `package.json` dependency version changing, or a `resolved` URL that points outside the public
// npm registry. Distinct from the OSV.dev CVE analyzer (review-enrichment/src/analyzers/lockfile-drift.ts) —
// that flags KNOWN-CVE versions; this flags tamper/integrity-substitution regardless of whether the substituted
// version has a published CVE. Config-driven, off by default (see rules/advisory.ts isConfiguredGateBlocker +
// signals/focus-manifest.ts gate.lockfileIntegrity) — this module only PRODUCES the finding; it never decides
// whether the finding blocks.

import type { AdvisoryFinding, PullRequestFileRecord } from "../types";

const NPM_REGISTRY_HOST_RE = /^https:\/\/registry\.npmjs\.org\//i;

// Package-lock "packages" entries are keyed either `"node_modules/<pkg>"` (lockfileVersion 2/3) or a bare
// `"<pkg>"` (lockfileVersion 1 "dependencies" tree, and yarn/pnpm equivalents keep a similar bare-name header).
// Root ("": {...}) and pure container headers ("packages": {...}, "dependencies": {...}) are never package
// entries themselves.
const CONTAINER_KEYS = new Set(["", "packages", "dependencies", "devDependencies", "optionalDependencies"]);

function npmPackageFromNodeModulesPath(path: string): string | null {
const marker = "node_modules/";
const i = path.lastIndexOf(marker);
if (i < 0) return null;
const rest = path.slice(i + marker.length);
if (rest.startsWith("@")) {
const parts = rest.split("/");
return parts.length >= 2 ? `${parts[0]}/${parts[1]}` : null;
}
return rest.split("/")[0] || null;
}

/** True when `path`'s basename is `package-lock.json` — the only lockfile format this check parses today
* (npm/lockfileVersion 2-3 JSON shape). Matches ANY directory depth (root, `review-enrichment/`,
* `apps/gittensory-ui/`, or a future workspace) rather than a hardcoded path list, so a new workspace package
* is covered without a code change. */
export function isNpmLockfilePath(path: string): boolean {
const normalized = path.replace(/\\/g, "/").toLowerCase();
const slash = normalized.lastIndexOf("/");
const basename = slash >= 0 ? normalized.slice(slash + 1) : normalized;
return basename === "package-lock.json";
}

type PatchLine = { sign: "+" | "-" | " "; content: string };

function* patchLines(patch: string): Generator<PatchLine> {
for (const raw of patch.split("\n")) {
if (raw.startsWith("+++ ") || raw.startsWith("--- ") || raw.startsWith("@@")) continue;
const first = raw[0];
if (first === "+") yield { sign: "+", content: raw.slice(1) };
else if (first === "-") yield { sign: "-", content: raw.slice(1) };
else yield { sign: " ", content: raw.slice(1) };
}
}

type LockfileTamperCandidate = {
file: string;
package: string;
/** True when a `resolved`/`integrity` value changed for this package block in the diff. */
resolvedOrIntegrityChanged: boolean;
/** A `+resolved` URL seen for this package block that does not point at registry.npmjs.org, or null. */
offRegistryResolvedUrl: string | null;
};

/** Parse one `package-lock.json` unified-diff patch for per-package resolved/integrity changes. Heuristic
* line-based scan (mirrors review-enrichment's lockfile-drift parser), not a full JSON parse — good enough to
* flag suspicious hunks without needing the complete (potentially huge) lockfile tree in memory. */
function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandidate[] {
const byPackage = new Map<string, LockfileTamperCandidate>();
let currentPackage: string | null = null;
let sawPackagesEntry = false;
for (const line of patchLines(patch)) {
const body = line.content.trim();
const objectHeader = /^"([^"]+)"\s*:\s*\{/.exec(body);
if (objectHeader) {
const key = objectHeader[1]!;
const nodeModulesPackage = npmPackageFromNodeModulesPath(key);
if (nodeModulesPackage) {
currentPackage = nodeModulesPackage;
sawPackagesEntry = true;
} else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) {
currentPackage = key;
} else {
currentPackage = null;
}
continue;
}
if (body === "}" || body.startsWith("},")) currentPackage = null;
if (!currentPackage || line.sign === " ") continue;

const resolvedMatch = /^"resolved"\s*:\s*"([^"]*)"/.exec(body);
const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body);
if (!resolvedMatch && !integrityMatch) continue;

const entry =
byPackage.get(currentPackage) ??
({ file: path, package: currentPackage, resolvedOrIntegrityChanged: false, offRegistryResolvedUrl: null } satisfies LockfileTamperCandidate);
entry.resolvedOrIntegrityChanged = true;
if (resolvedMatch && line.sign === "+" && resolvedMatch[1] && !NPM_REGISTRY_HOST_RE.test(resolvedMatch[1])) {
entry.offRegistryResolvedUrl = resolvedMatch[1];
}
byPackage.set(currentPackage, entry);
}
return [...byPackage.values()];
}

// `"<name>": "<range>"` inside a package.json dependency block, e.g. `"lodash": "^4.17.21",`. Line-based, not a
// full JSON parse — the same heuristic review-enrichment's dependency-scan.ts uses for the same shape.
const PACKAGE_JSON_DEP_RE = /^"([^"]+)"\s*:\s*"([^"]+)"/;

/** Package names whose declared `package.json` version range CHANGED somewhere in this PR's diff (across every
* changed `package.json`, any dependency block) — a `+`/`-` pair with different range strings for the same key
* counts as changed; a line present on only one side (add/remove of the dependency entirely) also counts. */
function packagesWithManifestVersionChange(files: PullRequestFileRecord[]): Set<string> {
const changed = new Set<string>();
for (const file of files) {
if (file.path.replace(/\\/g, "/").toLowerCase().split("/").pop() !== "package.json") continue;
const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
if (!patch) continue;
const removedVersions = new Map<string, string>();
const addedVersions = new Map<string, string>();
for (const line of patchLines(patch)) {
if (line.sign === " ") continue;
const match = PACKAGE_JSON_DEP_RE.exec(line.content.trim());
if (!match) continue;
const [, name, range] = match as unknown as [string, string, string];
(line.sign === "+" ? addedVersions : removedVersions).set(name, range);
}
for (const [name, addedRange] of addedVersions) {
const removedRange = removedVersions.get(name);
if (removedRange === undefined || removedRange !== addedRange) changed.add(name);
}
for (const name of removedVersions.keys()) {
if (!addedVersions.has(name)) changed.add(name);
}
}
return changed;
}

const MAX_FLAGGED_PACKAGES_IN_TITLE = 3;

/**
* Scan every changed `package-lock.json` in the PR for a tamper-risk hunk: a `resolved`/`integrity` value
* changed WITHOUT the same package's version changing in a changed `package.json`, or a `resolved` URL outside
* `registry.npmjs.org`. Returns ONE `lockfile_tamper_risk` advisory finding on any hit, else null. Callers gate
* this on the repo's `lockfileIntegrityGateMode` (default `off` — see rules/advisory.ts) before invoking it.
*/
export function lockfileTamperRiskFinding(files: PullRequestFileRecord[]): AdvisoryFinding | null {
const lockfiles = files.filter((file) => isNpmLockfilePath(file.path));
if (lockfiles.length === 0) return null;
const bumpedPackages = packagesWithManifestVersionChange(files);

const flagged: { file: string; package: string; reason: "off_registry" | "unbumped_resolved" }[] = [];
for (const file of lockfiles) {
const patch = typeof file.payload?.patch === "string" ? file.payload.patch : "";
if (!patch) continue;
for (const candidate of scanPackageLockPatch(file.path, patch)) {
if (candidate.offRegistryResolvedUrl) {
flagged.push({ file: candidate.file, package: candidate.package, reason: "off_registry" });
} else if (candidate.resolvedOrIntegrityChanged && !bumpedPackages.has(candidate.package)) {
flagged.push({ file: candidate.file, package: candidate.package, reason: "unbumped_resolved" });
}
}
}
if (flagged.length === 0) return null;

const names = [...new Set(flagged.map((f) => f.package))];
const shownNames = names.slice(0, MAX_FLAGGED_PACKAGES_IN_TITLE).join(", ");
const moreSuffix = names.length > MAX_FLAGGED_PACKAGES_IN_TITLE ? ` +${names.length - MAX_FLAGGED_PACKAGES_IN_TITLE} more` : "";
const hasOffRegistry = flagged.some((f) => f.reason === "off_registry");
const hasUnbumped = flagged.some((f) => f.reason === "unbumped_resolved");
const detailParts: string[] = [];
if (hasOffRegistry) detailParts.push("a resolved URL points outside registry.npmjs.org");
if (hasUnbumped) detailParts.push("a resolved/integrity value changed without a matching package.json version bump");

return {
code: "lockfile_tamper_risk",
severity: "warning",
title: `Possible lockfile tamper risk (${shownNames}${moreSuffix})`,
detail: `The lockfile diff for ${[...new Set(flagged.map((f) => f.file))].join(", ")} is suspicious: ${detailParts.join("; ")}. Affected package(s): ${names.join(", ")}.`,
action: "Re-run the package manager's install/lock command to regenerate the lockfile from package.json rather than hand-editing resolved/integrity entries, and confirm every resolved URL is on the public npm registry.",
};
}
9 changes: 9 additions & 0 deletions src/rules/advisory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ export type GateCheckPolicy = {
* neutral gate → "manual" verdict, never auto-merged and never a hard failure. Defaults off; thresholds default
* to 10 files / 1000 lines. This is a HOLD (advisory dry-run friendly), not a close. */
sizeGateMode?: GateRuleMode | undefined;
/** Lockfile-tamper-risk gate (#2563). When `block`, a `lockfile_tamper_risk` finding (produced by
* review/lockfile-tamper.ts when a changed package-lock.json's resolved/integrity value changed without a
* matching package.json version bump, or points off the npm registry) becomes a hard blocker. Defaults to
* `off` — the finding is never produced when off, and never blocks under `advisory`. */
lockfileIntegrityGateMode?: GateRuleMode | undefined;
/** Aggregate change size, threaded from the resolved file list (changedLineCount = additions + deletions). */
changedFileCount?: number | null | undefined;
changedLineCount?: number | null | undefined;
Expand Down Expand Up @@ -874,6 +879,10 @@ function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPoli
// Self-authored linked-issue gate: blocks only when the maintainer opts in with `block`. Defaults to
// advisory — the finding surfaces in the panel without ever closing the PR unless explicitly configured.
if (code === "self_authored_linked_issue") return gateMode(policy.selfAuthoredLinkedIssueGateMode ?? "advisory") === "block";
// Lockfile-tamper-risk gate (#2563): blocks only when the maintainer opts in with `block`. Defaults to `off`
// (the finding is never even produced — see maybeAddLockfileTamperFinding's mode gate in queue/processors.ts),
// so this branch only matters once a repo has explicitly turned the scan on.
if (code === "lockfile_tamper_risk") return gateMode(policy.lockfileIntegrityGateMode ?? "off") === "block";
return false;
}

Expand Down
Loading
Loading