diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index 48f6bf7b625..c45d8f6a408 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -215,12 +215,9 @@ async function audit( }); } -async function runAuditCases< - T extends { - run: () => Promise; - assert: (res: SecurityAuditReport) => void; - }, ->(cases: readonly T[]) { +async function runAuditCases( + cases: readonly { run: () => Promise; assert: (result: T) => void }[], +) { await Promise.all( cases.map(async ({ run, assert }) => { assert(await run()); @@ -228,6 +225,21 @@ async function runAuditCases< ); } +async function runConfigAuditCases( + cases: readonly T[], + assert: (res: SecurityAuditReport, testCase: T) => void, + options?: ( + testCase: T, + ) => Omit & { preserveExecApprovals?: boolean }, +) { + await runAuditCases( + cases.map((testCase) => ({ + run: () => audit(testCase.cfg, options?.(testCase)), + assert: (res: SecurityAuditReport) => assert(res, testCase), + })), + ); +} + function hasFinding(res: SecurityAuditReport, checkId: string, severity?: string): boolean { return res.findings.some( (f) => f.checkId === checkId && (severity == null || f.severity === severity), @@ -242,6 +254,36 @@ function expectNoFinding(res: SecurityAuditReport, checkId: string): void { expect(hasFinding(res, checkId)).toBe(false); } +function expectFindingSet(params: { + res: SecurityAuditReport; + name: string; + expectedPresent?: readonly string[]; + expectedAbsent?: readonly string[]; + severity?: string; +}) { + const severity = params.severity ?? "warn"; + for (const checkId of params.expectedPresent ?? []) { + expect(hasFinding(params.res, checkId, severity), `${params.name}:${checkId}`).toBe(true); + } + for (const checkId of params.expectedAbsent ?? []) { + expect(hasFinding(params.res, checkId), `${params.name}:${checkId}`).toBe(false); + } +} + +function expectDetailText(params: { + detail: string | null | undefined; + name: string; + includes?: readonly string[]; + excludes?: readonly string[]; +}) { + for (const text of params.includes ?? []) { + expect(params.detail, `${params.name}:${text}`).toContain(text); + } + for (const text of params.excludes ?? []) { + expect(params.detail, `${params.name}:${text}`).not.toContain(text); + } +} + async function expectSeverityByExposureCases(params: { checkId: string; cases: Array<{ @@ -325,6 +367,17 @@ describe("security audit", () => { ); }; + const runChannelSecurityStateCases = async ( + cases: readonly T[], + run: (testCase: T, tmp: string) => Promise, + ) => { + await Promise.all( + cases.map(async (testCase) => { + await withChannelSecurityStateDir(async (tmp) => run(testCase, tmp)); + }), + ); + }; + const runSharedExtensionsAudit = async (config: OpenClawConfig) => { return runSecurityAudit({ config, @@ -598,14 +651,15 @@ description: test skill expectedSeverity: "critical", }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg, { env: {} }); + await runConfigAuditCases( + cases, + (res, testCase) => { expect( hasFinding(res, "gateway.tools_invoke_http.dangerous_allow", testCase.expectedSeverity), testCase.name, ).toBe(true); - }), + }, + () => ({ env: {} }), ); }); @@ -664,12 +718,9 @@ description: test skill checkId: "tools.exec.host_sandbox_no_sandbox_agents", }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - expect(hasFinding(res, testCase.checkId, "warn"), testCase.name).toBe(true); - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + expect(hasFinding(res, testCase.checkId, "warn"), testCase.name).toBe(true); + }); }); it("warns for interpreter safeBins only when explicit profiles are missing", async () => { @@ -735,15 +786,12 @@ description: test skill expected: false, }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - expect( - hasFinding(res, "tools.exec.safe_bins_interpreter_unprofiled", "warn"), - testCase.name, - ).toBe(testCase.expected); - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + expect( + hasFinding(res, "tools.exec.safe_bins_interpreter_unprofiled", "warn"), + testCase.name, + ).toBe(testCase.expected); + }); }); it("warns when risky broad-behavior bins are explicitly added to safeBins", async () => { @@ -775,14 +823,11 @@ description: test skill expected: false, }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - expect(hasFinding(res, "tools.exec.safe_bins_broad_behavior", "warn"), testCase.name).toBe( - testCase.expected, - ); - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + expect(hasFinding(res, "tools.exec.safe_bins_broad_behavior", "warn"), testCase.name).toBe( + testCase.expected, + ); + }); }); it("evaluates safeBinTrustedDirs risk findings", async () => { @@ -837,12 +882,9 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - testCase.assert(res); - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + testCase.assert(res); + }); }); it("warns when exec approvals enable autoAllowSkills", async () => { @@ -988,11 +1030,12 @@ description: test skill severity: "warn", }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg, testCase.opts); + await runConfigAuditCases( + cases, + (res, testCase) => { expect(hasFinding(res, testCase.checkId, testCase.severity), testCase.name).toBe(true); - }), + }, + (testCase) => testCase.opts ?? {}, ); }); @@ -1048,28 +1091,29 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const tmp = await makeTmpDir(testCase.label); - const stateDir = path.join(tmp, "state"); - await fs.mkdir(stateDir, { recursive: true }); - const configPath = path.join(stateDir, "openclaw.json"); - await fs.writeFile(configPath, "{}\n", "utf-8"); + await runAuditCases( + cases.map((testCase) => ({ + run: async () => { + const tmp = await makeTmpDir(testCase.label); + const stateDir = path.join(tmp, "state"); + await fs.mkdir(stateDir, { recursive: true }); + const configPath = path.join(stateDir, "openclaw.json"); + await fs.writeFile(configPath, "{}\n", "utf-8"); - const res = await runSecurityAudit({ - config: {}, - includeFilesystem: true, - includeChannelSecurity: false, - stateDir, - configPath, - platform: "win32", - env: windowsAuditEnv, - execIcacls: testCase.execIcacls, - execDockerRawFn: execDockerRawUnavailable, - }); - - testCase.assert(res); - }), + return runSecurityAudit({ + config: {}, + includeFilesystem: true, + includeChannelSecurity: false, + stateDir, + configPath, + platform: "win32", + env: windowsAuditEnv, + execIcacls: testCase.execIcacls, + execDockerRawFn: execDockerRawUnavailable, + }); + }, + assert: testCase.assert, + })), ); }); @@ -1327,28 +1371,39 @@ description: test skill }, ] as const; - await Promise.all( + await runAuditCases( cases .filter((testCase) => testCase.supported) - .map(async (testCase) => { - const fixture = await testCase.setup(); - const configPath = path.join(fixture.stateDir, "openclaw.json"); - await fs.writeFile(configPath, "{}\n", "utf-8"); - if (!isWindows) { - await fs.chmod(configPath, 0o600); - } + .map((testCase) => ({ + run: async () => { + const fixture = await testCase.setup(); + const configPath = path.join(fixture.stateDir, "openclaw.json"); + await fs.writeFile(configPath, "{}\n", "utf-8"); + if (!isWindows) { + await fs.chmod(configPath, 0o600); + } - const res = await runSecurityAudit({ - config: { agents: { defaults: { workspace: fixture.workspaceDir } } }, - includeFilesystem: true, - includeChannelSecurity: false, - stateDir: fixture.stateDir, - configPath, - execDockerRawFn: execDockerRawUnavailable, - }); + const res = await runSecurityAudit({ + config: { agents: { defaults: { workspace: fixture.workspaceDir } } }, + includeFilesystem: true, + includeChannelSecurity: false, + stateDir: fixture.stateDir, + configPath, + execDockerRawFn: execDockerRawUnavailable, + }); - testCase.assert(res, fixture); - }), + return { fixture, res }; + }, + assert: ({ + fixture, + res, + }: { + fixture: Awaited>; + res: SecurityAuditReport; + }) => { + testCase.assert(res, fixture); + }, + })), ); }); @@ -1382,16 +1437,15 @@ description: test skill detailIncludes: ["mistral-8b", "sandbox=all"], }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - const finding = res.findings.find((f) => f.checkId === "models.small_params"); - expect(finding?.severity, testCase.name).toBe(testCase.expectedSeverity); - for (const text of testCase.detailIncludes) { - expect(finding?.detail, `${testCase.name}:${text}`).toContain(text); - } - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + const finding = res.findings.find((f) => f.checkId === "models.small_params"); + expect(finding?.severity, testCase.name).toBe(testCase.expectedSeverity); + expectDetailText({ + detail: finding?.detail, + name: testCase.name, + includes: testCase.detailIncludes, + }); + }); }); it("evaluates sandbox docker config findings", async () => { @@ -1474,24 +1528,20 @@ description: test skill }, ] as const; - await runAuditCases( - cases.map((testCase) => ({ - run: () => audit(testCase.cfg), - assert: (res: SecurityAuditReport) => { - if (testCase.expectedFindings.length > 0) { - expect(res.findings, testCase.name).toEqual( - expect.arrayContaining( - testCase.expectedFindings.map((finding) => expect.objectContaining(finding)), - ), - ); - } - const expectedAbsent = "expectedAbsent" in testCase ? testCase.expectedAbsent : []; - for (const checkId of expectedAbsent) { - expect(hasFinding(res, checkId), `${testCase.name}:${checkId}`).toBe(false); - } - }, - })), - ); + await runConfigAuditCases(cases, (res, testCase) => { + if (testCase.expectedFindings.length > 0) { + expect(res.findings, testCase.name).toEqual( + expect.arrayContaining( + testCase.expectedFindings.map((finding) => expect.objectContaining(finding)), + ), + ); + } + expectFindingSet({ + res, + name: testCase.name, + expectedAbsent: "expectedAbsent" in testCase ? testCase.expectedAbsent : [], + }); + }); }); it("evaluates ineffective gateway.nodes.denyCommands entries", async () => { @@ -1532,24 +1582,18 @@ description: test skill }, ] as const; - await runAuditCases( - cases.map((testCase) => ({ - run: () => audit(testCase.cfg), - assert: (res: SecurityAuditReport) => { - const finding = res.findings.find( - (f) => f.checkId === "gateway.nodes.deny_commands_ineffective", - ); - expect(finding?.severity, testCase.name).toBe("warn"); - for (const text of testCase.detailIncludes) { - expect(finding?.detail, `${testCase.name}:${text}`).toContain(text); - } - const detailExcludes = "detailExcludes" in testCase ? testCase.detailExcludes : []; - for (const text of detailExcludes) { - expect(finding?.detail, `${testCase.name}:${text}`).not.toContain(text); - } - }, - })), - ); + await runConfigAuditCases(cases, (res, testCase) => { + const finding = res.findings.find( + (f) => f.checkId === "gateway.nodes.deny_commands_ineffective", + ); + expect(finding?.severity, testCase.name).toBe("warn"); + expectDetailText({ + detail: finding?.detail, + name: testCase.name, + includes: testCase.detailIncludes, + excludes: "detailExcludes" in testCase ? testCase.detailExcludes : [], + }); + }); }); it("evaluates dangerous gateway.nodes.allowCommands findings", async () => { @@ -1588,29 +1632,27 @@ description: test skill }, ] as const; - await runAuditCases( - cases.map((testCase) => ({ - run: () => audit(testCase.cfg), - assert: (res: SecurityAuditReport) => { - if ("expectedAbsent" in testCase && testCase.expectedAbsent) { - expectNoFinding(res, "gateway.nodes.allow_commands_dangerous"); - return; - } - const expectedSeverity = - "expectedSeverity" in testCase ? testCase.expectedSeverity : undefined; - if (!expectedSeverity) { - return; - } + await runConfigAuditCases(cases, (res, testCase) => { + if ("expectedAbsent" in testCase && testCase.expectedAbsent) { + expectNoFinding(res, "gateway.nodes.allow_commands_dangerous"); + return; + } + const expectedSeverity = + "expectedSeverity" in testCase ? testCase.expectedSeverity : undefined; + if (!expectedSeverity) { + return; + } - const finding = res.findings.find( - (f) => f.checkId === "gateway.nodes.allow_commands_dangerous", - ); - expect(finding?.severity, testCase.name).toBe(expectedSeverity); - expect(finding?.detail, testCase.name).toContain("camera.snap"); - expect(finding?.detail, testCase.name).toContain("screen.record"); - }, - })), - ); + const finding = res.findings.find( + (f) => f.checkId === "gateway.nodes.allow_commands_dangerous", + ); + expect(finding?.severity, testCase.name).toBe(expectedSeverity); + expectDetailText({ + detail: finding?.detail, + name: testCase.name, + includes: ["camera.snap", "screen.record"], + }); + }); }); it("flags agent profile overrides when global tools.profile is minimal", async () => { @@ -1787,24 +1829,21 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - if ("expectedFinding" in testCase) { - expect(res.findings, testCase.name).toEqual( - expect.arrayContaining([expect.objectContaining(testCase.expectedFinding)]), - ); - } - const finding = res.findings.find( - (f) => f.checkId === "config.insecure_or_dangerous_flags", + await runConfigAuditCases(cases, (res, testCase) => { + if ("expectedFinding" in testCase) { + expect(res.findings, testCase.name).toEqual( + expect.arrayContaining([expect.objectContaining(testCase.expectedFinding)]), ); - expect(finding, testCase.name).toBeTruthy(); - expect(finding?.severity, testCase.name).toBe("warn"); - for (const detail of testCase.expectedDangerousDetails) { - expect(finding?.detail, `${testCase.name}:${detail}`).toContain(detail); - } - }), - ); + } + const finding = res.findings.find((f) => f.checkId === "config.insecure_or_dangerous_flags"); + expect(finding, testCase.name).toBeTruthy(); + expect(finding?.severity, testCase.name).toBe("warn"); + expectDetailText({ + detail: finding?.detail, + name: testCase.name, + includes: testCase.expectedDangerousDetails, + }); + }); }); it.each([ @@ -2129,19 +2168,19 @@ description: test skill }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - expect( - hasFinding(res, testCase.expectedCheckId, testCase.expectedSeverity), - testCase.name, - ).toBe(true); - if (testCase.suppressesGenericSharedSecretFindings) { - expect(hasFinding(res, "gateway.bind_no_auth"), testCase.name).toBe(false); - expect(hasFinding(res, "gateway.auth_no_rate_limit"), testCase.name).toBe(false); - } - }), - ); + await runConfigAuditCases(cases, (res, testCase) => { + expect( + hasFinding(res, testCase.expectedCheckId, testCase.expectedSeverity), + testCase.name, + ).toBe(true); + if (testCase.suppressesGenericSharedSecretFindings) { + expectFindingSet({ + res, + name: testCase.name, + expectedAbsent: ["gateway.bind_no_auth", "gateway.auth_no_rate_limit"], + }); + } + }); }); it("warns when multiple DM senders share the main session", async () => { @@ -2241,25 +2280,21 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - await withChannelSecurityStateDir(async () => { - const res = await runSecurityAudit({ - config: testCase.cfg, - includeFilesystem: false, - includeChannelSecurity: true, - plugins: [discordPlugin], - }); + await runChannelSecurityStateCases(cases, async (testCase) => { + const res = await runSecurityAudit({ + config: testCase.cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [discordPlugin], + }); - expect( - res.findings.some( - (finding) => finding.checkId === "channels.discord.commands.native.no_allowlists", - ), - testCase.name, - ).toBe(testCase.expectFinding); - }); - }), - ); + expect( + res.findings.some( + (finding) => finding.checkId === "channels.discord.commands.native.no_allowlists", + ), + testCase.name, + ).toBe(testCase.expectFinding); + }); }); it("keeps source-configured channel security findings when resolved inspection is incomplete", async () => { @@ -2453,28 +2488,24 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - await withChannelSecurityStateDir(async () => { - const res = await runSecurityAudit({ - config: testCase.resolvedConfig, - sourceConfig: testCase.sourceConfig, - includeFilesystem: false, - includeChannelSecurity: true, - plugins: [testCase.plugin(testCase.sourceConfig)], - }); + await runChannelSecurityStateCases(cases, async (testCase) => { + const res = await runSecurityAudit({ + config: testCase.resolvedConfig, + sourceConfig: testCase.sourceConfig, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [testCase.plugin(testCase.sourceConfig)], + }); - expect(res.findings, testCase.name).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - checkId: testCase.expectedCheckId, - severity: "warn", - }), - ]), - ); - }); - }), - ); + expect(res.findings, testCase.name).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: testCase.expectedCheckId, + severity: "warn", + }), + ]), + ); + }); }); it("adds a read-only resolution warning when channel account resolveAccount throws", async () => { @@ -2987,16 +3018,19 @@ description: test skill }, }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(cfg, { - deep: true, - deepTimeoutMs: 50, - probeGatewayFn: testCase.probeGatewayFn, - }); - testCase.assertDeep?.(res); - expect(hasFinding(res, "gateway.probe_failed", "warn"), testCase.name).toBe(true); - }), + await runAuditCases( + cases.map((testCase) => ({ + run: () => + audit(cfg, { + deep: true, + deepTimeoutMs: 50, + probeGatewayFn: testCase.probeGatewayFn, + }), + assert: (res: SecurityAuditReport) => { + testCase.assertDeep?.(res); + expect(hasFinding(res, "gateway.probe_failed", "warn"), testCase.name).toBe(true); + }, + })), ); }); @@ -3024,18 +3058,21 @@ description: test skill expectedAbsentCheckId: "models.weak_tier", }, ]; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit({ + await runConfigAuditCases( + cases.map((testCase) => ({ + ...testCase, + cfg: { agents: { defaults: { model: { primary: testCase.model } } }, - }); + } satisfies OpenClawConfig, + })), + (res, testCase) => { for (const expected of testCase.expectedFindings ?? []) { expect(hasFinding(res, expected.checkId, expected.severity), testCase.name).toBe(true); } if (testCase.expectedAbsentCheckId) { expect(hasFinding(res, testCase.expectedAbsentCheckId), testCase.name).toBe(false); } - }), + }, ); }); @@ -3123,10 +3160,9 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const env = "env" in testCase ? testCase.env : undefined; - const res = await audit(testCase.cfg, env ? { env } : undefined); + await runConfigAuditCases( + cases, + (res, testCase) => { expectFinding(res, testCase.expectedFinding, testCase.expectedSeverity); if ("expectedExtraFinding" in testCase) { expectFinding( @@ -3135,7 +3171,11 @@ description: test skill testCase.expectedExtraFinding.severity, ); } - }), + }, + (testCase) => { + const env = "env" in testCase ? testCase.env : undefined; + return env ? { env } : {}; + }, ); }); @@ -3411,18 +3451,18 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const res = await testCase.run(); - const expectedPresent = "expectedPresent" in testCase ? testCase.expectedPresent : []; - for (const checkId of expectedPresent) { - expect(hasFinding(res, checkId, "warn"), `${testCase.name}:${checkId}`).toBe(true); - } - const expectedAbsent = "expectedAbsent" in testCase ? testCase.expectedAbsent : []; - for (const checkId of expectedAbsent) { - expect(hasFinding(res, checkId), `${testCase.name}:${checkId}`).toBe(false); - } - }), + await runAuditCases( + cases.map((testCase) => ({ + run: () => testCase.run(), + assert: (res: SecurityAuditReport) => { + expectFindingSet({ + res, + name: testCase.name, + expectedPresent: "expectedPresent" in testCase ? testCase.expectedPresent : [], + expectedAbsent: "expectedAbsent" in testCase ? testCase.expectedAbsent : [], + }); + }, + })), ); }); @@ -3523,11 +3563,11 @@ description: test skill SLACK_APP_TOKEN: undefined, }, async () => { - await Promise.all( - cases.map(async (testCase) => { - const res = await runSharedExtensionsAudit(testCase.cfg); - testCase.assert(res); - }), + await runAuditCases( + cases.map((testCase) => ({ + run: () => runSharedExtensionsAudit(testCase.cfg), + assert: testCase.assert, + })), ); }, ); @@ -3638,8 +3678,7 @@ description: test skill await Promise.all( cases.slice(0, -1).map(async (testCase) => { - const result = await testCase.run(); - testCase.assert(result as never); + testCase.assert((await testCase.run()) as never); }), ); @@ -3772,11 +3811,11 @@ description: test skill }, ] as const; - await Promise.all( - cases.map(async (testCase) => { - const res = await audit(testCase.cfg); - testCase.assert(res); - }), + await runAuditCases( + cases.map((testCase) => ({ + run: () => audit(testCase.cfg), + assert: testCase.assert, + })), ); }); @@ -3886,17 +3925,22 @@ description: test skill }, ]; - await Promise.all( - cases.map(async (testCase) => { - const { probeGatewayFn, getAuth } = makeProbeCapture(); - await audit(testCase.cfg, { - deep: true, - deepTimeoutMs: 50, - probeGatewayFn, - env: makeProbeEnv(testCase.env), - }); - expect(getAuth(), testCase.name).toEqual(testCase.expectedAuth); - }), + await runAuditCases( + cases.map((testCase) => ({ + run: async () => { + const probe = makeProbeCapture(); + await audit(testCase.cfg, { + deep: true, + deepTimeoutMs: 50, + probeGatewayFn: probe.probeGatewayFn, + env: makeProbeEnv(testCase.env), + }); + return probe.getAuth(); + }, + assert: (capturedAuth: { token?: string; password?: string } | undefined) => { + expect(capturedAuth, testCase.name).toEqual(testCase.expectedAuth); + }, + })), ); }); diff --git a/src/security/windows-acl.test.ts b/src/security/windows-acl.test.ts index 8b058149a6c..36e0b3b5e0d 100644 --- a/src/security/windows-acl.test.ts +++ b/src/security/windows-acl.test.ts @@ -62,6 +62,16 @@ function expectSinglePrincipal(entries: WindowsAclEntry[], principal: string): v expect(entries[0].principal).toBe(principal); } +function expectAccessRights( + rights: string, + expected: { canWrite: boolean; canRead: boolean }, +): void { + const output = `C:\\test\\file.txt BUILTIN\\Users:${rights}`; + const entries = parseIcaclsOutput(output, "C:\\test\\file.txt"); + expect(entries[0].canWrite, rights).toBe(expected.canWrite); + expect(entries[0].canRead, rights).toBe(expected.canRead); +} + function expectTrustedOnly( entries: WindowsAclEntry[], options?: { env?: NodeJS.ProcessEnv; expectedTrusted?: number }, @@ -80,6 +90,17 @@ function expectInspectSuccess( expect(result.entries).toHaveLength(expectedEntries); } +function expectSummaryCounts( + entries: readonly WindowsAclEntry[], + expected: { trusted?: number; untrustedWorld?: number; untrustedGroup?: number }, + env?: NodeJS.ProcessEnv, +): void { + const summary = summarizeWindowsAcl([...entries], env); + expect(summary.trusted).toHaveLength(expected.trusted ?? 0); + expect(summary.untrustedWorld).toHaveLength(expected.untrustedWorld ?? 0); + expect(summary.untrustedGroup).toHaveLength(expected.untrustedGroup ?? 0); +} + describe("windows-acl", () => { describe("resolveWindowsUserPrincipal", () => { it("returns DOMAIN\\USERNAME when both are present", () => { @@ -182,27 +203,20 @@ Successfully processed 1 files`; expect(entries).toHaveLength(1); }); - it("detects write permissions correctly", () => { + it.each([ + { rights: "(F)", canWrite: true, canRead: true }, + { rights: "(M)", canWrite: true, canRead: true }, + { rights: "(W)", canWrite: true, canRead: false }, + { rights: "(D)", canWrite: true, canRead: false }, + { rights: "(R)", canWrite: false, canRead: true }, + { rights: "(RX)", canWrite: false, canRead: true }, + ] as const)("detects write permissions correctly for %s", ({ rights, canWrite, canRead }) => { // F = Full control (read + write) // M = Modify (read + write) // W = Write // D = Delete (considered write) // R = Read only - const testCases = [ - { rights: "(F)", canWrite: true, canRead: true }, - { rights: "(M)", canWrite: true, canRead: true }, - { rights: "(W)", canWrite: true, canRead: false }, - { rights: "(D)", canWrite: true, canRead: false }, - { rights: "(R)", canWrite: false, canRead: true }, - { rights: "(RX)", canWrite: false, canRead: true }, - ]; - - for (const tc of testCases) { - const output = `C:\\test\\file.txt BUILTIN\\Users:${tc.rights}`; - const entries = parseIcaclsOutput(output, "C:\\test\\file.txt"); - expect(entries[0].canWrite).toBe(tc.canWrite); - expect(entries[0].canRead).toBe(tc.canRead); - } + expectAccessRights(rights, { canWrite, canRead }); }); }); @@ -263,121 +277,108 @@ Successfully processed 1 files`; }); describe("summarizeWindowsAcl — SID-based classification", () => { - it("classifies SYSTEM SID (S-1-5-18) as trusted", () => { - expectTrustedOnly([aclEntry({ principal: "S-1-5-18" })]); - }); - - it("classifies *S-1-5-18 (icacls /sid prefix form of SYSTEM) as trusted (refs #35834)", () => { - // icacls /sid output prefixes SIDs with *, e.g. *S-1-5-18 instead of - // S-1-5-18. Without this fix the asterisk caused SID_RE to not match - // and the SYSTEM entry was misclassified as "group" (untrusted). - expectTrustedOnly([aclEntry({ principal: "*S-1-5-18" })]); - }); - - it("classifies *S-1-5-32-544 (icacls /sid Administrators) as trusted", () => { - const entries: WindowsAclEntry[] = [aclEntry({ principal: "*S-1-5-32-544" })]; - const summary = summarizeWindowsAcl(entries); - expect(summary.trusted).toHaveLength(1); - expect(summary.untrustedGroup).toHaveLength(0); - }); - - it("classifies BUILTIN\\Administrators SID (S-1-5-32-544) as trusted", () => { - const entries: WindowsAclEntry[] = [aclEntry({ principal: "S-1-5-32-544" })]; - const summary = summarizeWindowsAcl(entries); - expect(summary.trusted).toHaveLength(1); - expect(summary.untrustedGroup).toHaveLength(0); - }); - - it("classifies caller SID from USERSID env var as trusted", () => { - const callerSid = "S-1-5-21-1824257776-4070701511-781240313-1001"; - expectTrustedOnly([aclEntry({ principal: callerSid })], { - env: { USERSID: callerSid }, - }); - }); - - it("matches SIDs case-insensitively and trims USERSID", () => { - expectTrustedOnly( - [aclEntry({ principal: "s-1-5-21-1824257776-4070701511-781240313-1001" })], - { env: { USERSID: " S-1-5-21-1824257776-4070701511-781240313-1001 " } }, - ); - }); - - it("does not trust *-prefixed Everyone via USERSID", () => { - const entries: WindowsAclEntry[] = [ - { - principal: "*S-1-1-0", - rights: ["R"], - rawRights: "(R)", - canRead: true, - canWrite: false, - }, - ]; - const summary = summarizeWindowsAcl(entries, { USERSID: "*S-1-1-0" }); - expect(summary.untrustedWorld).toHaveLength(1); - expect(summary.trusted).toHaveLength(0); - }); - - it("classifies unknown SID as group (not world)", () => { - const entries: WindowsAclEntry[] = [ - { - principal: "S-1-5-21-9999-9999-9999-500", - rights: ["R"], - rawRights: "(R)", - canRead: true, - canWrite: false, - }, - ]; - const summary = summarizeWindowsAcl(entries); - expect(summary.untrustedGroup).toHaveLength(1); - expect(summary.untrustedWorld).toHaveLength(0); - expect(summary.trusted).toHaveLength(0); - }); - - it("classifies Everyone SID (S-1-1-0) as world, not group", () => { - // When icacls is run with /sid, "Everyone" becomes *S-1-1-0. - // It must be classified as "world" to preserve security-audit severity. - const entries: WindowsAclEntry[] = [ - { - principal: "*S-1-1-0", - rights: ["R"], - rawRights: "(R)", - canRead: true, - canWrite: false, - }, - ]; - const summary = summarizeWindowsAcl(entries); - expect(summary.untrustedWorld).toHaveLength(1); - expect(summary.untrustedGroup).toHaveLength(0); - }); - - it("classifies Authenticated Users SID (S-1-5-11) as world, not group", () => { - const entries: WindowsAclEntry[] = [ - { - principal: "*S-1-5-11", - rights: ["R"], - rawRights: "(R)", - canRead: true, - canWrite: false, - }, - ]; - const summary = summarizeWindowsAcl(entries); - expect(summary.untrustedWorld).toHaveLength(1); - expect(summary.untrustedGroup).toHaveLength(0); - }); - - it("classifies BUILTIN\\Users SID (S-1-5-32-545) as world, not group", () => { - const entries: WindowsAclEntry[] = [ - { - principal: "*S-1-5-32-545", - rights: ["R"], - rawRights: "(R)", - canRead: true, - canWrite: false, - }, - ]; - const summary = summarizeWindowsAcl(entries); - expect(summary.untrustedWorld).toHaveLength(1); - expect(summary.untrustedGroup).toHaveLength(0); + it.each([ + { + name: "SYSTEM SID (S-1-5-18) is trusted", + entries: [aclEntry({ principal: "S-1-5-18" })], + expected: { trusted: 1 }, + }, + { + name: "*S-1-5-18 (icacls /sid SYSTEM) is trusted", + // icacls /sid output prefixes SIDs with *. + entries: [aclEntry({ principal: "*S-1-5-18" })], + expected: { trusted: 1 }, + }, + { + name: "*S-1-5-32-544 (icacls /sid Administrators) is trusted", + entries: [aclEntry({ principal: "*S-1-5-32-544" })], + expected: { trusted: 1 }, + }, + { + name: "BUILTIN\\\\Administrators SID (S-1-5-32-544) is trusted", + entries: [aclEntry({ principal: "S-1-5-32-544" })], + expected: { trusted: 1 }, + }, + { + name: "caller SID from USERSID env var is trusted", + entries: [aclEntry({ principal: "S-1-5-21-1824257776-4070701511-781240313-1001" })], + env: { USERSID: "S-1-5-21-1824257776-4070701511-781240313-1001" }, + expected: { trusted: 1 }, + }, + { + name: "SIDs match case-insensitively and trim USERSID", + entries: [aclEntry({ principal: "s-1-5-21-1824257776-4070701511-781240313-1001" })], + env: { USERSID: " S-1-5-21-1824257776-4070701511-781240313-1001 " }, + expected: { trusted: 1 }, + }, + { + name: "does not trust *-prefixed Everyone via USERSID", + entries: [ + aclEntry({ + principal: "*S-1-1-0", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }), + ], + env: { USERSID: "*S-1-1-0" }, + expected: { untrustedWorld: 1 }, + }, + { + name: "unknown SID is group, not world", + entries: [ + aclEntry({ + principal: "S-1-5-21-9999-9999-9999-500", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }), + ], + expected: { untrustedGroup: 1 }, + }, + { + name: "Everyone SID (S-1-1-0) is world, not group", + entries: [ + aclEntry({ + principal: "*S-1-1-0", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }), + ], + expected: { untrustedWorld: 1 }, + }, + { + name: "Authenticated Users SID (S-1-5-11) is world, not group", + entries: [ + aclEntry({ + principal: "*S-1-5-11", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }), + ], + expected: { untrustedWorld: 1 }, + }, + { + name: "BUILTIN\\\\Users SID (S-1-5-32-545) is world, not group", + entries: [ + aclEntry({ + principal: "*S-1-5-32-545", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }), + ], + expected: { untrustedWorld: 1 }, + }, + ] as const)("$name", ({ entries, env, expected }) => { + expectSummaryCounts(entries, expected, env); }); it("full scenario: SYSTEM SID + owner SID only → no findings", () => {