mirror of
https://github.com/openclaw/openclaw.git
synced 2026-10-03 17:53:39 +00:00
fix(ci): clarify security review comments (#152578)
* fix(ci): clarify security review warning comments * fix(ci): simplify security approval comments * fix(ci): remove hyphen from maintainer security warning * fix(ci): list dependency changes in maintainer warnings * fix(ci): simplify dependency graph filename lists
This commit is contained in:
Josh Avant
GitHub
parent
90ab8619b3
commit
5a2bc22ce9
4 changed files with 110 additions and 68 deletions
|
|
@ -157,24 +157,44 @@ export function isDependencyGuardMarkerComment(comment, marker, trustedAuthors)
|
|||
return Boolean(login && trustedAuthors.has(login) && comment.body?.includes(marker));
|
||||
}
|
||||
|
||||
function renderApprovedDependencyComment(approval) {
|
||||
function renderDependencyChangeLines({
|
||||
lockfileChanges,
|
||||
dependencyFiles = [],
|
||||
dependencyManifestChanges,
|
||||
}) {
|
||||
const files = new Set([...lockfileChanges, ...dependencyFiles]);
|
||||
for (const change of dependencyManifestChanges) {
|
||||
if (change.previousPath) {
|
||||
files.add(change.previousPath);
|
||||
}
|
||||
files.add(change.path);
|
||||
}
|
||||
return [...files].map((path) => `- ${markdownCode(path)}`);
|
||||
}
|
||||
|
||||
function renderApprovedDependencyComment(approval, changes) {
|
||||
return [
|
||||
dependencyGraphGuardMarker,
|
||||
"",
|
||||
approval.kind === "author"
|
||||
? "### Dependency graph changes noted"
|
||||
: "### Dependency graph changes approved",
|
||||
? "### ⚠️ Dependency graph changes"
|
||||
: "### ✅ Dependency graph changes approved",
|
||||
"",
|
||||
approval.kind === "author"
|
||||
? "This PR changes dependency resolution. The guard is informational because the PR author has repository Maintain or Admin access."
|
||||
: "A maintainer approved this revision with an explicit dependency approval comment. SecOps approval is not required.",
|
||||
? "This PR makes dependency graph changes. This comment is informational because the PR author has repository Maintain or Admin access."
|
||||
: "A maintainer approved this revision with an explicit dependency approval comment.",
|
||||
"",
|
||||
`- Current SHA: ${markdownCode(approval.sha)}`,
|
||||
`- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`,
|
||||
`- Repository role: ${markdownCode(approval.role)}`,
|
||||
...(approval.kind === "comment" ? [`- Approval comment: ${approval.url}`] : []),
|
||||
"",
|
||||
"Review resolved package changes and dependency policy before merging. A later push requires a fresh approval comment for an external contributor's PR.",
|
||||
...(approval.kind === "author"
|
||||
? ["These dependency graph changes were made:", ...renderDependencyChangeLines(changes), ""]
|
||||
: []),
|
||||
approval.kind === "author"
|
||||
? "Carefully review these changes before merging."
|
||||
: "A later push requires a fresh approval comment for an external contributor's PR.",
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
|
|
@ -254,13 +274,6 @@ export function renderBlockedDependencyComment({
|
|||
}) {
|
||||
const safeBranch = sanitizeGuardDisplayValue(baseBranch ?? "main");
|
||||
const baseRef = shellQuote(`origin/${safeBranch}`);
|
||||
const reasons = [];
|
||||
for (const path of new Set([...lockfileChanges, ...dependencyFiles])) {
|
||||
reasons.push(`- ${markdownCode(path)} changed.`);
|
||||
}
|
||||
for (const change of dependencyManifestChanges) {
|
||||
reasons.push(renderManifestChangeLine(change));
|
||||
}
|
||||
const autoscrubLines = renderAutoscrubStatusLines(autoscrubStatus);
|
||||
const removalSteps =
|
||||
lockfileChanges.length > 0
|
||||
|
|
@ -279,12 +292,14 @@ export function renderBlockedDependencyComment({
|
|||
return [
|
||||
dependencyGraphGuardMarker,
|
||||
"",
|
||||
"### Maintainer dependency review required",
|
||||
"### ⚠️ Maintainer dependency review required",
|
||||
"",
|
||||
"This external contributor PR changes dependency resolution. A maintainer with repository Maintain or Admin access must review the resolved packages and dependency policy before merging.",
|
||||
"This external contributor PR changes the dependency graph. A maintainer must review these changes before merging.",
|
||||
"",
|
||||
"Detected dependency graph changes:",
|
||||
...reasons,
|
||||
`Current SHA: ${markdownCode(headSha ?? "<head-sha>")}`,
|
||||
"",
|
||||
"These dependency graph changes were made:",
|
||||
...renderDependencyChangeLines({ lockfileChanges, dependencyFiles, dependencyManifestChanges }),
|
||||
...autoscrubLines,
|
||||
...removalSteps,
|
||||
"",
|
||||
|
|
@ -294,9 +309,7 @@ export function renderBlockedDependencyComment({
|
|||
dependencyApprovalCommand,
|
||||
"```",
|
||||
"",
|
||||
"Post the comment after this guard notice identifies the current head SHA below. Do not edit an earlier comment. A normal GitHub Approve review does not satisfy this check. SecOps approval is not required; this check updates automatically.",
|
||||
"",
|
||||
`Current head SHA: ${markdownCode(headSha ?? "<head-sha>")}. A later push requires a fresh approval comment.`,
|
||||
"A later push requires a fresh approval comment.",
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
|
|
@ -314,7 +327,10 @@ function renderAutoscrubStatusLines(status) {
|
|||
return [
|
||||
"",
|
||||
"Auto-scrub was not attempted because this PR changes package manifest dependency graph fields:",
|
||||
...status.changes.map(renderManifestChangeLine),
|
||||
...renderDependencyChangeLines({
|
||||
lockfileChanges: [],
|
||||
dependencyManifestChanges: status.changes,
|
||||
}),
|
||||
"",
|
||||
"Dependency graph changes require maintainer review. Please remove lockfile changes manually if they are not needed.",
|
||||
];
|
||||
|
|
@ -337,15 +353,6 @@ function renderAutoscrubStatusLines(status) {
|
|||
return [];
|
||||
}
|
||||
|
||||
function renderManifestChangeLine(change) {
|
||||
const location = change.previousPath
|
||||
? `${markdownCode(change.previousPath)} moved to ${markdownCode(change.path)}`
|
||||
: markdownCode(change.path);
|
||||
const fields =
|
||||
change.fields.length > 0 ? ` changed ${change.fields.map(markdownCode).join(", ")}` : "";
|
||||
return `- ${location}${fields}.`;
|
||||
}
|
||||
|
||||
export function githubApi(token, options = {}) {
|
||||
const api = createGitHubApi(token, { ...options, userAgent: "openclaw-dependency-guard" });
|
||||
return {
|
||||
|
|
@ -707,7 +714,14 @@ export async function reviewDependencyChanges(
|
|||
dependencyGraphChanges,
|
||||
headSha: pullRequest.head.sha,
|
||||
})
|
||||
: withApprovalRequest(guard, renderApprovedDependencyComment(guard.approval));
|
||||
: withApprovalRequest(
|
||||
guard,
|
||||
renderApprovedDependencyComment(guard.approval, {
|
||||
lockfileChanges,
|
||||
dependencyFiles,
|
||||
dependencyManifestChanges,
|
||||
}),
|
||||
);
|
||||
await upsertComment(existingGuardComment, body);
|
||||
await writeSummary(body);
|
||||
return;
|
||||
|
|
|
|||
|
|
@ -14,25 +14,56 @@ function code(value) {
|
|||
}
|
||||
|
||||
function renderComment({ changes, pullRequest, approval }) {
|
||||
if (changes.length > 0 && approval?.kind === "comment") {
|
||||
return [
|
||||
marker,
|
||||
"",
|
||||
"### ✅ Maintainer security changes approved",
|
||||
"",
|
||||
"A maintainer approved this revision with an explicit security approval comment.",
|
||||
"",
|
||||
`- Current SHA: ${code(approval.sha)}`,
|
||||
`- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`,
|
||||
`- Repository role: ${code(approval.role)}`,
|
||||
`- Approval comment: ${approval.url}`,
|
||||
"",
|
||||
"A later push requires a fresh approval comment for an external contributor's PR.",
|
||||
].join("\n");
|
||||
}
|
||||
const heading =
|
||||
changes.length === 0
|
||||
? "Security-sensitive guard cleared"
|
||||
: approval?.kind === "author"
|
||||
? "Security-sensitive changes noted"
|
||||
: approval
|
||||
? "Maintainer security review complete"
|
||||
: "Maintainer security review required";
|
||||
const lines = [
|
||||
marker,
|
||||
"",
|
||||
`### ${heading}`,
|
||||
"",
|
||||
`Current revision: ${code(pullRequest.head.sha)}`,
|
||||
];
|
||||
? "⚠️ Security sensitive changes"
|
||||
: "⚠️ Maintainer security review required";
|
||||
const lines = [marker, "", `### ${heading}`, ""];
|
||||
if (changes.length > 0 && approval?.kind === "author") {
|
||||
lines.push(
|
||||
"This PR makes security sensitive changes. This comment is informational because the PR author has repository Maintain or Admin access.",
|
||||
"",
|
||||
`- Current SHA: ${code(pullRequest.head.sha)}`,
|
||||
`- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`,
|
||||
`- Repository role: ${code(approval.role)}`,
|
||||
);
|
||||
} else if (changes.length > 0 && !approval) {
|
||||
lines.push(
|
||||
"This external contributor PR changes sensitive security components. A maintainer must review these changes before merging.",
|
||||
"",
|
||||
`Current SHA: ${code(pullRequest.head.sha)}`,
|
||||
);
|
||||
} else {
|
||||
lines.push(`Current revision: ${code(pullRequest.head.sha)}`);
|
||||
}
|
||||
if (changes.length === 0) {
|
||||
lines.push("", "This PR no longer changes files in the maintainer security-review tier.");
|
||||
} else {
|
||||
lines.push("", "Review these security responsibilities:", "");
|
||||
lines.push(
|
||||
"",
|
||||
approval?.kind === "author"
|
||||
? "These security sensitive changes were made:"
|
||||
: "These sensitive security changes were made:",
|
||||
"",
|
||||
);
|
||||
for (const change of changes.slice(0, 25)) {
|
||||
lines.push(`- ${code(change.path)}: ${change.reason}`);
|
||||
}
|
||||
|
|
@ -41,28 +72,25 @@ function renderComment({ changes, pullRequest, approval }) {
|
|||
}
|
||||
lines.push("");
|
||||
if (approval?.kind === "author") {
|
||||
lines.push(
|
||||
`Informational: author @${approval.login} has repository ${code(approval.role)} access.`,
|
||||
);
|
||||
} else if (approval) {
|
||||
lines.push(
|
||||
`@${approval.login} approved this revision with ${code("/allow-security-sensitive-change")} and repository ${code(approval.role)} access.`,
|
||||
);
|
||||
lines.push("Carefully review these changes before merging.");
|
||||
} else {
|
||||
lines.push(
|
||||
"A GitHub user account with repository `maintain` or `admin` access must post a new comment containing `/allow-security-sensitive-change` after this notice names the current revision. SecOps approval is not required for this tier.",
|
||||
"Use only the command, or include `/allow-dependencies-change` on a separate line if both guards need approval. A normal GitHub Approve review does not satisfy this check.",
|
||||
"After reviewing the changes, post a new PR comment containing only approval commands, each on its own line:",
|
||||
"",
|
||||
"```text",
|
||||
"/allow-security-sensitive-change",
|
||||
"```",
|
||||
"",
|
||||
"A later push requires a fresh approval comment.",
|
||||
);
|
||||
}
|
||||
}
|
||||
if (changes.length === 0) {
|
||||
lines.push(
|
||||
"",
|
||||
"A later push requires a new approval comment after the notice updates. Editing an old comment does not renew approval; deleting the command removes its approval.",
|
||||
"Separate CODEOWNERS requirements still apply to security policy and enforcement files.",
|
||||
);
|
||||
}
|
||||
lines.push(
|
||||
"",
|
||||
"Separate CODEOWNERS requirements still apply to security policy and enforcement files.",
|
||||
);
|
||||
return lines.join("\n");
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -143,6 +143,7 @@ describe("dependency guard script", () => {
|
|||
expect(result.status, result.stderr).toBe(0);
|
||||
expect(result.statuses.map((call) => call.body?.state)).toEqual(["failure", "success"]);
|
||||
expect(result.stdout).toContain("informational");
|
||||
expect(result.stdout).toContain("- `pnpm-workspace.yaml`\n");
|
||||
});
|
||||
|
||||
it("does not transfer command approval to a duplicate PR with the same head", () => {
|
||||
|
|
@ -263,7 +264,7 @@ describe("dependency guard script", () => {
|
|||
expect(result.status).toBe(1);
|
||||
expect(result.statuses.at(-1)?.body?.state).toBe("failure");
|
||||
expect(result.stdout).toContain(
|
||||
"`extensions/old/package.json` moved to `extensions/new/package.json`",
|
||||
"- `extensions/old/package.json`\n- `extensions/new/package.json`\n",
|
||||
);
|
||||
});
|
||||
|
||||
|
|
@ -503,17 +504,14 @@ describe("dependency guard script", () => {
|
|||
|
||||
expect(body).toContain("<!-- openclaw:dependency-graph-guard -->");
|
||||
expect(body).toContain("Maintainer dependency review required");
|
||||
expect(body).toContain("`pnpm-lock.yaml` changed.");
|
||||
expect(body).toContain("`tools/nested/pnpm-lock.yaml` changed.");
|
||||
expect(body).toContain("`package.json` changed `dependencies`.");
|
||||
expect(body).toContain("- `pnpm-lock.yaml`\n");
|
||||
expect(body).toContain("- `tools/nested/pnpm-lock.yaml`\n");
|
||||
expect(body).toContain("- `package.json`\n");
|
||||
expect(body).toContain(
|
||||
"git checkout 'origin/main' -- 'pnpm-lock.yaml' 'tools/nested/pnpm-lock.yaml'",
|
||||
);
|
||||
expect(body).toContain("```text\n/allow-dependencies-change\n```");
|
||||
expect(body).toContain("Post the comment after this guard notice identifies the current head");
|
||||
expect(body).toContain("A normal GitHub Approve review does not satisfy this check");
|
||||
expect(body).toContain("SecOps approval is not required");
|
||||
expect(body).toContain(`Current head SHA: \`${headSha}\``);
|
||||
expect(body).toContain(`Current SHA: \`${headSha}\``);
|
||||
expect(body).toContain("A later push requires a fresh approval comment.");
|
||||
});
|
||||
|
||||
|
|
@ -674,7 +672,7 @@ describe("dependency guard script", () => {
|
|||
"only push deterministic cleanup commits to PR branches that maintainers can modify",
|
||||
);
|
||||
expect(unsafeBody).toContain("changes package manifest dependency graph fields");
|
||||
expect(unsafeBody).toContain("`package.json` changed `dependencies`");
|
||||
expect(unsafeBody).toContain("- `package.json`\n");
|
||||
expect(unsafeBody).toContain("Dependency graph changes require maintainer review");
|
||||
expect(mixedBody).toContain("also changes dependency-related files");
|
||||
expect(mixedBody).toContain("`patches/example.patch`");
|
||||
|
|
|
|||
|
|
@ -38,6 +38,7 @@ const notice = {
|
|||
const approval = {
|
||||
id: 11,
|
||||
user: approver,
|
||||
html_url: "https://github.com/openclaw/openclaw/pull/7#issuecomment-11",
|
||||
body: "/allow-security-sensitive-change",
|
||||
created_at: "2026-01-01T00:01:00Z",
|
||||
updated_at: "2026-01-01T00:01:00Z",
|
||||
|
|
@ -168,7 +169,7 @@ describe("security-sensitive guard entry point", () => {
|
|||
const result = runGuard({ authorRole });
|
||||
expect(result.status, result.stderr).toBe(0);
|
||||
expect(result.statuses).toEqual(["failure", "success"]);
|
||||
expect(result.comment).toContain("Informational");
|
||||
expect(result.comment).toContain("informational");
|
||||
});
|
||||
|
||||
it.each([
|
||||
|
|
@ -330,7 +331,8 @@ describe("security-sensitive guard entry point", () => {
|
|||
const result = runGuard({ comments: [notice, approval], approverRole, event: commentEvent });
|
||||
expect(result.status, result.stderr).toBe(0);
|
||||
expect(result.statuses).toEqual(["failure", "success"]);
|
||||
expect(result.comment).toContain("@maintainer approved");
|
||||
expect(result.comment).toContain("- Maintainer: @maintainer");
|
||||
expect(result.comment).toContain(`- Approval comment: ${approval.html_url}`);
|
||||
expect(
|
||||
result.requests
|
||||
.filter((request) => request.path.includes("/statuses/"))
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue