From b822e6e425b1851323ee1403ebf52c6630db9662 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 14 Aug 2026 13:29:21 -0700 Subject: [PATCH] refactor(github): consolidate guard display sanitization (#123620) --- .github/CODEOWNERS | 1 + scripts/github/dependency-guard.mjs | 25 ++++++------- scripts/github/guard-shared.mjs | 6 ++++ scripts/github/security-sensitive-guard.mjs | 36 ++++++------------- test/scripts/dependency-guard-script.test.ts | 6 ---- .../security-sensitive-guard-script.test.ts | 30 ++-------------- .../security-sensitive-guard-workflow.test.ts | 1 + 7 files changed, 31 insertions(+), 74 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 25f7a2a16701..b6b03c8ff462 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -18,6 +18,7 @@ /test/scripts/security-sensitive-guard-workflow.test.ts @openclaw/openclaw-secops /test/scripts/security-sensitive-guard-script.test.ts @openclaw/openclaw-secops /scripts/github/dependency-guard.mjs @openclaw/openclaw-secops +/scripts/github/guard-shared.mjs @openclaw/openclaw-secops /scripts/github/security-sensitive-guard.mjs @openclaw/openclaw-secops /.gitignore @openclaw/openclaw-secops /package-lock.json @openclaw/openclaw-secops diff --git a/scripts/github/dependency-guard.mjs b/scripts/github/dependency-guard.mjs index 14980ecc0919..dcd65557b567 100644 --- a/scripts/github/dependency-guard.mjs +++ b/scripts/github/dependency-guard.mjs @@ -15,6 +15,7 @@ import { normalizeGuardLoginSet, readBoundedGitHubErrorText, readBoundedGitHubJson, + sanitizeGuardDisplayValue, } from "./guard-shared.mjs"; /** Marker used to identify dependency guard comments. */ @@ -159,18 +160,12 @@ function stableJson(value) { return JSON.stringify(sorted); } -export function sanitizeDisplayValue(value) { - return String(value) - .replace(/[\p{Cc}]/gu, "?") - .slice(0, 240); -} - export function markdownCode(value) { - return `\`${sanitizeDisplayValue(value).replaceAll("`", "\\`")}\``; + return `\`${sanitizeGuardDisplayValue(value).replaceAll("`", "\\`")}\``; } function shellQuote(value) { - return `'${sanitizeDisplayValue(value).replaceAll("'", "'\\''")}'`; + return `'${sanitizeGuardDisplayValue(value).replaceAll("'", "'\\''")}'`; } function* dependencyOverrideCandidates({ comments, expectedSha, newerThan }) { @@ -188,7 +183,7 @@ function* dependencyOverrideCandidates({ comments, expectedSha, newerThan }) { } yield { login, - reason: reason ? sanitizeDisplayValue(reason) : null, + reason: reason ? sanitizeGuardDisplayValue(reason) : null, sha: expectedSha, url: comment.html_url, }; @@ -313,7 +308,7 @@ export function renderAuthorizedDependencyComment(override) { "This PR includes dependency graph changes. A repository admin or member of `@openclaw/openclaw-secops` authorized this exact head SHA with `/allow-dependencies-change`.", "", `- Approved SHA: ${markdownCode(override.sha)}`, - `- Approved by: @${sanitizeDisplayValue(override.login)}`, + `- Approved by: @${sanitizeGuardDisplayValue(override.login)}`, ]; if (override.reason) { lines.push(`- Reason: ${markdownCode(override.reason)}`); @@ -332,7 +327,7 @@ export function renderTrustedDependencyComment({ actor, headSha }) { "This PR includes dependency graph changes. The dependency guard is informational because the PR author is a repository admin or a member of `@openclaw/openclaw-secops`.", "", `- Current SHA: ${markdownCode(headSha ?? "")}`, - `- Trusted actor: @${sanitizeDisplayValue(actor.login)}`, + `- Trusted actor: @${sanitizeGuardDisplayValue(actor.login)}`, `- Trusted role: ${markdownCode(actor.reason)}`, "", "Security review is still recommended before merge when the dependency graph change is intentional.", @@ -360,7 +355,7 @@ export function renderRemovalOnlyDependencyComment({ dependencyGraphChanges, hea } export function renderAutoscrubbedDependencyComment({ baseBranch, lockfileChanges, commitSha }) { - const safeBranch = sanitizeDisplayValue(baseBranch ?? "main"); + const safeBranch = sanitizeGuardDisplayValue(baseBranch ?? "main"); const fileLines = lockfileChanges.map((path) => `- ${markdownCode(path)}`); return `${dependencyGraphGuardMarker} @@ -411,7 +406,7 @@ export function renderBlockedDependencyComment({ dependencyManifestChanges, autoscrubStatus, }) { - const safeBranch = sanitizeDisplayValue(baseBranch ?? "main"); + const safeBranch = sanitizeGuardDisplayValue(baseBranch ?? "main"); const baseRef = shellQuote(`origin/${safeBranch}`); const reasons = []; for (const path of lockfileChanges) { @@ -839,7 +834,7 @@ async function main() { [ "## Dependency Guard", "", - `Dependency graph change noted for trusted actor @${sanitizeDisplayValue(trustedActor.login)} and allowed to continue.`, + `Dependency graph change noted for trusted actor @${sanitizeGuardDisplayValue(trustedActor.login)} and allowed to continue.`, ].join("\n"), ); console.log("Dependency graph change noted for trusted actor; guard is informational."); @@ -987,7 +982,7 @@ async function main() { [ "## Dependency Guard", "", - `Dependency graph change authorized by @${sanitizeDisplayValue(override.login)} for ${markdownCode(override.sha)}.`, + `Dependency graph change authorized by @${sanitizeGuardDisplayValue(override.login)} for ${markdownCode(override.sha)}.`, ].join("\n"), ); console.log("Dependency graph change authorized by trusted override."); diff --git a/scripts/github/guard-shared.mjs b/scripts/github/guard-shared.mjs index d52961ebc46e..0c5cb94b36bd 100644 --- a/scripts/github/guard-shared.mjs +++ b/scripts/github/guard-shared.mjs @@ -8,6 +8,12 @@ export const GITHUB_API_REQUEST_TIMEOUT_MS = 30_000; const githubApiRetryStatuses = new Set([502, 503, 504]); const githubApiRetryDelaysMs = [1_000, 2_000, 4_000]; +export function sanitizeGuardDisplayValue(value) { + return String(value) + .replace(/[\p{Cc}]/gu, "?") + .slice(0, 240); +} + /** * @param {string | null | undefined} value * @param {string} [fallback] diff --git a/scripts/github/security-sensitive-guard.mjs b/scripts/github/security-sensitive-guard.mjs index 6f193d53102a..6c40c281d28f 100644 --- a/scripts/github/security-sensitive-guard.mjs +++ b/scripts/github/security-sensitive-guard.mjs @@ -16,6 +16,7 @@ import { normalizeGuardLoginSet, readBoundedGitHubErrorText, readBoundedGitHubJson, + sanitizeGuardDisplayValue, } from "./guard-shared.mjs"; /** Marker used to identify security-sensitive guard comments. */ @@ -62,14 +63,8 @@ export function isSecuritySensitiveFile(filename) { return securitySensitiveFileDefinition(filename) !== null; } -export function sanitizeDisplayValue(value) { - return String(value) - .replace(/[\p{Cc}]/gu, "?") - .slice(0, 240); -} - export function markdownCode(value) { - return `\`${sanitizeDisplayValue(value).replaceAll("`", "\\`")}\``; + return `\`${sanitizeGuardDisplayValue(value).replaceAll("`", "\\`")}\``; } function* securitySensitiveOverrideCandidates({ comments, expectedSha, newerThan }) { @@ -87,7 +82,7 @@ function* securitySensitiveOverrideCandidates({ comments, expectedSha, newerThan } yield { login, - reason: reason ? sanitizeDisplayValue(reason) : null, + reason: reason ? sanitizeGuardDisplayValue(reason) : null, sha: expectedSha, url: comment.html_url, }; @@ -170,10 +165,6 @@ export function isSecuritySensitiveGuardTrustedForHead(comment, currentHeadSha) ); } -export function securityApproverSet(value) { - return normalizeGuardLoginSet(value); -} - export function securitySensitiveGuardCommentAuthors(value) { return normalizeGuardLoginSet(value, "github-actions[bot]"); } @@ -217,7 +208,7 @@ function renderChangedFileLines(changes) { const listedFiles = changes.slice(0, maxListedFiles); const omittedCount = changes.length - listedFiles.length; const lines = listedFiles.map( - (change) => `- ${markdownCode(change.path)}: ${sanitizeDisplayValue(change.reason)}`, + (change) => `- ${markdownCode(change.path)}: ${sanitizeGuardDisplayValue(change.reason)}`, ); if (omittedCount > 0) { lines.push(`- ${omittedCount} additional security-sensitive files not shown`); @@ -252,7 +243,7 @@ export function renderAuthorizedSecuritySensitiveComment(override) { "This PR includes security-sensitive file changes. A repository admin or member of `@openclaw/openclaw-secops` authorized this exact head SHA with `/allow-security-sensitive-change`.", "", `- Approved SHA: ${markdownCode(override.sha)}`, - `- Approved by: @${sanitizeDisplayValue(override.login)}`, + `- Approved by: @${sanitizeGuardDisplayValue(override.login)}`, ]; if (override.reason) { lines.push(`- Reason: ${markdownCode(override.reason)}`); @@ -270,7 +261,7 @@ export function renderTrustedSecuritySensitiveComment({ actor, headSha, changes "This PR includes security-sensitive file changes. The guard is informational because the PR author is a repository admin or a member of `@openclaw/openclaw-secops`.", "", `- Current SHA: ${markdownCode(headSha ?? "")}`, - `- Trusted actor: @${sanitizeDisplayValue(actor.login)}`, + `- Trusted actor: @${sanitizeGuardDisplayValue(actor.login)}`, `- Trusted role: ${markdownCode(actor.reason)}`, "", "Changed files:", @@ -343,13 +334,6 @@ export async function findTrustedSecuritySensitiveGuardActor({ return null; } -export function githubApi(token, options = {}) { - return createGitHubApi(token, { - ...options, - userAgent: "openclaw-security-sensitive-guard", - }); -} - async function writeSummary(markdown) { const summaryPath = process.env.GITHUB_STEP_SUMMARY; if (!summaryPath) { @@ -374,8 +358,8 @@ async function main() { return; } - const api = githubApi(token); - const explicitSecurityApprovers = securityApproverSet(process.env.OPENCLAW_SECURITY_APPROVERS); + const api = createGitHubApi(token, { userAgent: "openclaw-security-sensitive-guard" }); + const explicitSecurityApprovers = normalizeGuardLoginSet(process.env.OPENCLAW_SECURITY_APPROVERS); const trustedCommentAuthors = securitySensitiveGuardCommentAuthors( process.env.OPENCLAW_SECURITY_SENSITIVE_GUARD_COMMENT_BOTS, ); @@ -479,7 +463,7 @@ async function main() { [ "## Security Sensitive Guard", "", - `Security-sensitive changes noted for trusted actor @${sanitizeDisplayValue(trustedActor.login)} and allowed to continue.`, + `Security-sensitive changes noted for trusted actor @${sanitizeGuardDisplayValue(trustedActor.login)} and allowed to continue.`, ].join("\n"), ); console.log("Security-sensitive changes noted for trusted actor; guard is informational."); @@ -508,7 +492,7 @@ async function main() { [ "## Security Sensitive Guard", "", - `Security-sensitive changes authorized by @${sanitizeDisplayValue(override.login)} for ${markdownCode(override.sha)}.`, + `Security-sensitive changes authorized by @${sanitizeGuardDisplayValue(override.login)} for ${markdownCode(override.sha)}.`, ].join("\n"), ); console.log("Security-sensitive changes authorized by trusted override."); diff --git a/test/scripts/dependency-guard-script.test.ts b/test/scripts/dependency-guard-script.test.ts index 0368bb7f0020..f85149381041 100644 --- a/test/scripts/dependency-guard-script.test.ts +++ b/test/scripts/dependency-guard-script.test.ts @@ -28,7 +28,6 @@ import { renderClearedDependencyGuardComment, renderRemovalOnlyDependencyComment, renderTrustedDependencyComment, - sanitizeDisplayValue, securityApproverSet, shouldAutoscrubDependencyLockfiles, } from "../../scripts/github/dependency-guard.mjs"; @@ -681,11 +680,6 @@ describe("dependency guard script", () => { ); }); - it("sanitizes display values", () => { - expect(sanitizeDisplayValue("abc\u0000def")).toBe("abc?def"); - expect(sanitizeDisplayValue("x".repeat(300))).toHaveLength(240); - }); - it("bounds GitHub error bodies by content-length", async () => { const response = new Response("ignored", { headers: { "content-length": String(GITHUB_ERROR_BODY_MAX_BYTES + 1) }, diff --git a/test/scripts/security-sensitive-guard-script.test.ts b/test/scripts/security-sensitive-guard-script.test.ts index ab6b2af5e979..e2719c2950fc 100644 --- a/test/scripts/security-sensitive-guard-script.test.ts +++ b/test/scripts/security-sensitive-guard-script.test.ts @@ -1,13 +1,12 @@ // Security Sensitive Guard Script tests cover sensitive file guard behavior. import { describe, expect, it } from "vitest"; +import { sanitizeGuardDisplayValue } from "../../scripts/github/guard-shared.mjs"; import { - GITHUB_RESPONSE_BODY_MAX_BYTES, allowSecuritySensitiveCommand, collectSecuritySensitiveChanges, findSecuritySensitiveOverrideCommand, findSecuritySensitiveOverrideCommandAsync, findTrustedSecuritySensitiveGuardActor, - githubApi, isSecuritySensitiveFile, isSecuritySensitiveGuardAuthorizedForHead, isSecuritySensitiveGuardMarkerComment, @@ -18,8 +17,6 @@ import { renderClearedSecuritySensitiveGuardComment, renderSecuritySensitiveAwarenessComment, renderTrustedSecuritySensitiveComment, - sanitizeDisplayValue, - securityApproverSet, securitySensitiveFileDefinition, securitySensitiveFileDefinitions, securitySensitiveGuardCommentAuthors, @@ -240,29 +237,8 @@ describe("security-sensitive guard script", () => { }); it("sanitizes display values and markdown code", () => { - expect(sanitizeDisplayValue("abc\u0000def")).toBe("abc?def"); - expect(sanitizeDisplayValue("x".repeat(300))).toHaveLength(240); + expect(sanitizeGuardDisplayValue("abc\u0000def")).toBe("abc?def"); + expect(sanitizeGuardDisplayValue("x".repeat(300))).toHaveLength(240); expect(markdownCode("`quoted`")).toBe("`\\`quoted\\``"); }); - - it("parses explicit security approver allowlists", () => { - expect(securityApproverSet("vincentkoc, steipete\njoshavant")).toEqual( - new Set(["vincentkoc", "steipete", "joshavant"]), - ); - }); - - it("bounds successful GitHub API response bodies", async () => { - const request = githubApi("token", { - responseMaxBodyBytes: 64, - fetchImpl: (() => - Promise.resolve( - new Response("x".repeat(65), { - headers: { "content-length": "65" }, - }), - )) as typeof fetch, - }).request("/repos/openclaw/openclaw"); - - await expect(request).rejects.toThrow("GitHub response body exceeded 64 bytes"); - expect(GITHUB_RESPONSE_BODY_MAX_BYTES).toBeGreaterThan(64); - }); }); diff --git a/test/scripts/security-sensitive-guard-workflow.test.ts b/test/scripts/security-sensitive-guard-workflow.test.ts index f5b26c42f777..2d0261959f96 100644 --- a/test/scripts/security-sensitive-guard-workflow.test.ts +++ b/test/scripts/security-sensitive-guard-workflow.test.ts @@ -137,6 +137,7 @@ describe("security-sensitive guard workflow", () => { expect(codeowners).toContain( "/scripts/github/security-sensitive-guard.mjs @openclaw/openclaw-secops", ); + expect(codeowners).toContain("/scripts/github/guard-shared.mjs @openclaw/openclaw-secops"); expect(codeowners).toContain("/.gitignore @openclaw/openclaw-secops"); }); });