Skip to content

fix: block-env-files and protect-env-vars false positives on quoted content #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yashexosphere wants to merge 1 commit into from
Closed

fix: block-env-files and protect-env-vars false positives on quoted content #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 188 additions & 1 deletion __tests__/hooks/builtin-policies.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ describe("hooks/builtin-policies", () => {
describe("protect-env-vars", () => {
const policy = BUILTIN_POLICIES.find((p) => p.name === "protect-env-vars")!;

// ── true positives — must remain blocked ─────────────────────────────────

it("blocks env command", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
Expand All @@ -417,16 +419,70 @@ describe("hooks/builtin-policies", () => {
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks printenv with no args", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "printenv" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks echo $VAR", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "echo $SECRET_KEY" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks export VAR=", async () => {
it("blocks echo $HOME (reads env var)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "echo $HOME" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks export VAR=value", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "export API_KEY=abc" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks export PATH=$PATH:/usr/local/bin (reads $PATH)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "export PATH=$PATH:/usr/local/bin" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks export NODE_ENV=production", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "export NODE_ENV=production" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks env chained with other command", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "env | grep SECRET" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

// ── false positives fixed — quoted string content must be ignored ─────────

it("allows git commit -m with 'export' in message", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "git commit -m \"export PATH changes in readme\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows git commit -m with 'export' in single-quoted message", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "git commit -m 'export PATH changes'" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows gh pr create with 'export' in body", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "gh pr create --body \"export the config using env vars\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows echo of a plain string with no $VAR", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "echo \"use export to set vars\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows echo with $VAR inside single quotes (not expanded by shell)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "echo '$HOME is not expanded'" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

// ── baseline allows ───────────────────────────────────────────────────────

it("allows normal commands", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "ls -la" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
Expand All @@ -436,11 +492,23 @@ describe("hooks/builtin-policies", () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { command: "env" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows npm install (reads env vars internally but not via bash)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "npm install express" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows git push (gh tools that use GITHUB_TOKEN internally)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "git push origin main" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});
});

describe("block-env-files", () => {
const policy = BUILTIN_POLICIES.find((p) => p.name === "block-env-files")!;

// ── true positives — must remain blocked ─────────────────────────────────

it("blocks Read of .env file", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/home/user/.env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
Expand All @@ -451,15 +519,134 @@ describe("hooks/builtin-policies", () => {
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Read of .env.production file", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.env.production" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Read of nested .env file", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/home/user/project/.env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Write to .env file", async () => {
const ctx = makeCtx({ toolName: "Write", toolInput: { file_path: "/app/.env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash cat .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cat .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash source .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "source .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash dot-source .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: ". .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash cp .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cp .env /tmp/backup" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash less .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "less .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash head .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "head .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash tail .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "tail .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash nano .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "nano .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash vim .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "vim .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash rm .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "rm .env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

it("blocks Bash cat path/.env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cat /home/user/.env" } });
expect((await policy.fn(ctx)).decision).toBe("deny");
});

// ── false positives fixed — .env in quoted strings must be ignored ────────

it("allows gh pr create with .env in body", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "gh pr create --body \"don't commit .env files\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows git commit -m with .env in message", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "git commit -m \"add .env to .gitignore\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows echo with .env in quoted string", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "echo \"copy .env.example to .env\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows python -c with .env in comment string", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "python -c \"# load from .env file\nimport os\"" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows git commit with single-quoted message mentioning .env", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "git commit -m 'document .env usage'" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

// ── baseline allows ───────────────────────────────────────────────────────

it("allows non-.env files", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/src/main.ts" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows .envrc (different suffix)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cat .envrc" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows cat .env.example (safe template file, no real secrets)", async () => {
const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cat .env.example" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows Read of .env.example (safe template file)", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.env.example" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows Read of .env.template", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.env.template" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});

it("allows Read of .env.sample", async () => {
const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.env.sample" } });
expect((await policy.fn(ctx)).decision).toBe("allow");
});
});

describe("block-sudo", () => {
Expand Down
43 changes: 35 additions & 8 deletions src/hooks/builtin-policies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,26 @@ const DOTNET_GETENV_RE = /\[Environment\]::GetEnvironment/i;
const CMD_ECHO_ENV_RE = /echo\s+%[A-Za-z_]/i;

// blockEnvFiles
// Allow .env.example / .env.template / .env.sample / .env.dist — these are
// committed template files with no real secrets. Block everything else.
const ENV_SAFE_SUFFIX_RE = /\.(?:example|template|sample|dist|test)(?:[./\\]|$)/i;
const ENV_FILE_PATH_RE = /(?:^|[\\/])\.env(?:\.|$)/;
const ENV_CMD_RE = /\.env(?:\b|\s|$|\.)/;
const ENV_CMD_RE = /\.env(?!\.(?:example|template|sample|dist|test)\b)(?:\b|\s|$|\.)/i;
Comment on lines +97 to +101

@coderabbitai coderabbitai Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Match the exact safe .env basename instead of regexing the whole path.

Right now a safe-looking substring anywhere in the path suppresses the block, so /tmp/.env.example/.env is allowed, and extra-extension names like .env.example.local also slip through. ENV_FILE_PATH_RE is also case-sensitive, so .ENV/.Env bypass Read/Write on case-insensitive filesystems. Please normalize to the matched basename and compare it against the exact allowed template names.

Also applies to: 367-373

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/builtin-policies.ts` around lines 97 - 101, ENV_SAFE_SUFFIX_RE /
ENV_FILE_PATH_RE / ENV_CMD_RE currently allow safe substrings anywhere in the
path and miss case-insensitive matches; change the logic to extract the file
basename (last path segment), normalize it to lowercase, and compare it exactly
against the whitelist of allowed template basenames (e.g. ".env",
".env.example", ".env.template", ".env.sample", ".env.dist", ".env.test")
instead of using the existing regexes to match anywhere; update the checks that
use ENV_SAFE_SUFFIX_RE, ENV_FILE_PATH_RE and ENV_CMD_RE (including the other
occurrence around the 367-373 area) to use this basename-based, case-insensitive
exact comparison so paths like "/tmp/.env.example/.env" or ".env.example.local"
no longer bypass the policy.


/**
* Strip quoted string content from a shell command before regex matching.
*
* Removes the text INSIDE double quotes, single quotes, and backticks so that
* policy regexes don't false-positive on argument values like:
* gh pr create --body "don't commit .env files"
* git commit -m "export PATH changes"
*
* Real threats like `cat .env` have no quotes to strip so they still match.
* Known limitation: heredoc bodies are not quoted so they are not stripped.
*/
function stripQuotedStrings(cmd: string): string {
return cmd.replace(/"[^"]*"|'[^']*'|`[^`]*`/g, '""');
Comment on lines +114 to +115

@coderabbitai coderabbitai Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

This quote stripping creates a security bypass, not just a false-positive fix.

Because the stripped string is what both policies inspect, semantically active shell syntax disappears before matching. cat ".env", source '.env', echo "$HOME", and bash -lc "cat .env" all become invisible here, so real .env reads and env-var disclosure are now allowed with ordinary quoting. Please make the suppression context-aware instead of blanking every quoted span, and add regression tests for quoted true-positive cases.

Also applies to: 331-339, 373-373

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/builtin-policies.ts` around lines 114 - 115, stripQuotedStrings
currently replaces entire quoted spans with empty quotes which removes
semantically active shell syntax and permits real violations to bypass policy
checks; change stripQuotedStrings to perform context-aware masking (e.g.,
preserve or mark shell-expanding constructs like $VAR, command substitutions
$(...), backticks, and prefixes like bash -c inside quoted strings) rather than
blanking every quoted span so that downstream matchers still see dangerous
patterns, and update any other quote-stripping logic in this file to the same
approach; add regression tests that include quoted true-positive cases such as
cat ".env", source '.env', echo "$HOME", and bash -lc "cat .env" to ensure these
are detected.

}

// blockSudo
const SUDO_RE = /(?:^|;|&&|\|\|)\s*sudo\s/;
Expand Down Expand Up @@ -305,14 +323,20 @@ function warnPackagePublish(ctx: PolicyContext): PolicyResult {
function protectEnvVars(ctx: PolicyContext): PolicyResult {
if (ctx.toolName !== "Bash") return allow();
const cmd = getCommand(ctx);
// Strip quoted strings before checking so that policy regexes don't
// false-positive on argument values like:
// git commit -m "export PATH changes in readme"
// echo "use export to configure vars"
// Real threats like `export API_KEY=abc` have no quotes to strip.
const stripped = stripQuotedStrings(cmd);
// Block: env, printenv, echo $VAR, export VAR=
if (ENV_PRINTENV_RE.test(cmd)) {
if (ENV_PRINTENV_RE.test(stripped)) {
return deny("Command reads environment variables");
}
if (ECHO_ENV_RE.test(cmd)) {
if (ECHO_ENV_RE.test(stripped)) {
return deny("Command echoes environment variable");
}
if (EXPORT_RE.test(cmd)) {
if (EXPORT_RE.test(stripped)) {
return deny("Command exports environment variable");
}
// PowerShell: $env:VAR
Expand All @@ -338,12 +362,15 @@ function blockEnvFiles(ctx: PolicyContext): PolicyResult {
const cmd = getCommand(ctx);
const filePath = getFilePath(ctx);

// Check file_path for Read/Write tools (match both / and \ path separators)
if (filePath && ENV_FILE_PATH_RE.test(filePath)) {
// Check file_path for Read/Write tools (match both / and \ path separators).
// Allow .env.example / .env.template / .env.sample — safe committed templates.
if (filePath && ENV_FILE_PATH_RE.test(filePath) && !ENV_SAFE_SUFFIX_RE.test(filePath)) {
return deny("Access to .env file blocked");
}
// Check Bash commands referencing .env files
if (ctx.toolName === "Bash" && ENV_CMD_RE.test(cmd)) {
// Check Bash commands referencing .env files.
// Strip quoted strings first so that .env appearing inside argument values
// like --body "don't commit .env" or -m "add .env to gitignore" is ignored.
if (ctx.toolName === "Bash" && ENV_CMD_RE.test(stripQuotedStrings(cmd))) {
return deny("Command references .env file");
}
return allow();
Expand Down