Skip to content

Commit 9f940c8

Browse files
committed
chore: apply AI code review suggestions
- Fix gcloud IAM regex to match multi-segment commands (e.g., gcloud alpha projects add-iam-policy-binding) - Fix localhost allow-rule to add boundary after hostname/port to prevent bypasses like localhost.evil.com - Fix path traversal in classifyWriteEdit by using path.resolve before checking project membership - Fix URL domain patterns for paste/file services to anchor to authority section - Fix raw.githubusercontent.com regex to handle query parameters (not just $ anchor) - Broaden chmod 777 detection to match flags before the mode (e.g., chmod -R 777)
1 parent 39b27c0 commit 9f940c8

2 files changed

Lines changed: 19 additions & 15 deletions

File tree

plugins/gatekeeper/dist/pre-tool-use.js

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// src/pre-tool-use.ts
2+
import path from "node:path";
23
import process from "node:process";
34

45
// src/chain-parser.ts
@@ -165,11 +166,11 @@ var SOFT_DENY_RULES = [
165166
{ pattern: /(?:^|\s)\.claude\/settings/i, reason: "Agent self-modification needs user intent verification" },
166167
{ pattern: /\bCLAUDE\.md\b/i, reason: "Agent self-modification needs user intent verification" },
167168
{ pattern: /^git\s+commit(?:\s+\S+)*\s--no-verify\b/i, reason: "Skipping commit verification needs user intent verification" },
168-
{ pattern: /\bchmod\s+777\b/i, reason: "Broad permission change needs user intent verification" },
169+
{ pattern: /\bchmod\s+\S*777\b/i, reason: "Broad permission change needs user intent verification" },
169170
{ pattern: /\b(nc|ncat|socat)\s+-l/i, reason: "Exposing local service needs user intent verification" },
170171
{ pattern: /\bpython3?\s+-m\s+http\.server/i, reason: "Exposing HTTP server needs user intent verification" },
171172
{ pattern: /\b(crontab|systemctl\s+enable|ssh-keygen|ssh-copy-id)\b/i, reason: "Unauthorized persistence needs user intent verification" },
172-
{ pattern: /\b(?:gcloud\s+\S+\s+add-iam|aws\s+iam|az\s+role\s+assignment)\b/i, reason: "Permission grant needs user intent verification" },
173+
{ pattern: /\b(?:gcloud(?:\s+\S+)*\s+add-iam|aws\s+iam|az\s+role\s+assignment)\b/i, reason: "Permission grant needs user intent verification" },
173174
{ pattern: /\bsystemctl\s+stop\s+\S*log/i, reason: "Logging tampering needs user intent verification" }
174175
];
175176
var ALLOW_RULES = [
@@ -216,16 +217,17 @@ function classifyWriteEdit(filePath) {
216217
return { decision: "soft_deny", reason: rule.reason };
217218
}
218219
}
219-
if (!filePath.startsWith("/") || filePath.includes("/node_modules/")) {
220+
const resolvedPath = path.resolve(filePath);
221+
if (!resolvedPath.startsWith("/") || resolvedPath.startsWith(process.cwd()) || resolvedPath.includes("/node_modules/")) {
220222
return { decision: "allow", reason: "Safe project file write" };
221223
}
222224
return null;
223225
}
224226
var WEBFETCH_SOFT_DENY_PATTERNS = [
225-
{ pattern: /\b(pastebin\.com|paste\.ee|hastebin\.com|dpaste\.org|ghostbin\.com|rentry\.co)\b/i, reason: "Paste service needs user intent verification" },
226-
{ pattern: /\b(transfer\.sh|file\.io|0x0\.st|tmpfiles\.org)\b/i, reason: "File sharing service needs user intent verification" },
227+
{ pattern: /^https?:\/\/(?:[^/]+\.)?(pastebin\.com|paste\.ee|hastebin\.com|dpaste\.org|ghostbin\.com|rentry\.co)(?:\/|$)/i, reason: "Paste service needs user intent verification" },
228+
{ pattern: /^https?:\/\/(?:[^/]+\.)?(transfer\.sh|file\.io|0x0\.st|tmpfiles\.org)(?:\/|$)/i, reason: "File sharing service needs user intent verification" },
227229
{ pattern: /\.(sh|bash|ps1|bat|cmd)(\?|$)/i, reason: "Script download needs user intent verification" },
228-
{ pattern: /\braw\.githubusercontent\.com\/.*\.(sh|py|rb|js)$/i, reason: "Raw script download needs user intent verification" }
230+
{ pattern: /\braw\.githubusercontent\.com\/.*\.(sh|py|rb|js)(?:\?|$)/i, reason: "Raw script download needs user intent verification" }
229231
];
230232
function classifyWebFetch(url) {
231233
if (!url) {
@@ -236,7 +238,7 @@ function classifyWebFetch(url) {
236238
return { decision: "soft_deny", reason: rule.reason };
237239
}
238240
}
239-
if (/^https?:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0)(?::\d+)?/i.test(url)) {
241+
if (/^https?:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0)(?::\d+)?(?:[/?#]|$)/i.test(url)) {
240242
return { decision: "allow", reason: "Safe localhost request" };
241243
}
242244
return null;

plugins/gatekeeper/src/pre-tool-use.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type {
33
PreToolUseHookSpecificOutput,
44
SyncHookJSONOutput,
55
} from '@anthropic-ai/claude-agent-sdk'
6+
import path from 'node:path'
67
import process from 'node:process'
78
import { parseChainedCommand } from './chain-parser'
89

@@ -71,7 +72,7 @@ export const SOFT_DENY_RULES: Rule[] = [
7172

7273
// Security weakening — only match --no-verify on commit (not push, which just skips pre-push hook)
7374
{ pattern: /^git\s+commit(?:\s+\S+)*\s--no-verify\b/i, reason: 'Skipping commit verification needs user intent verification' },
74-
{ pattern: /\bchmod\s+777\b/i, reason: 'Broad permission change needs user intent verification' },
75+
{ pattern: /\bchmod\s+\S*777\b/i, reason: 'Broad permission change needs user intent verification' },
7576

7677
// Expose local services
7778
{ pattern: /\b(nc|ncat|socat)\s+-l/i, reason: 'Exposing local service needs user intent verification' },
@@ -81,7 +82,7 @@ export const SOFT_DENY_RULES: Rule[] = [
8182
{ pattern: /\b(crontab|systemctl\s+enable|ssh-keygen|ssh-copy-id)\b/i, reason: 'Unauthorized persistence needs user intent verification' },
8283

8384
// Permission grants (IAM/RBAC)
84-
{ pattern: /\b(?:gcloud\s+\S+\s+add-iam|aws\s+iam|az\s+role\s+assignment)\b/i, reason: 'Permission grant needs user intent verification' },
85+
{ pattern: /\b(?:gcloud(?:\s+\S+)*\s+add-iam|aws\s+iam|az\s+role\s+assignment)\b/i, reason: 'Permission grant needs user intent verification' },
8586

8687
// Logging/audit tampering
8788
{ pattern: /\bsystemctl\s+stop\s+\S*log/i, reason: 'Logging tampering needs user intent verification' },
@@ -150,8 +151,9 @@ export function classifyWriteEdit(filePath: string): { decision: Decision, reaso
150151
}
151152
}
152153

153-
// Project-relative paths are generally safe
154-
if (!filePath.startsWith('/') || filePath.includes('/node_modules/')) {
154+
// Project-relative paths are generally safe; resolve to absolute path first to prevent path traversal
155+
const resolvedPath = path.resolve(filePath)
156+
if (!resolvedPath.startsWith('/') || resolvedPath.startsWith(process.cwd()) || resolvedPath.includes('/node_modules/')) {
155157
return { decision: 'allow', reason: 'Safe project file write' }
156158
}
157159

@@ -162,10 +164,10 @@ export function classifyWriteEdit(filePath: string): { decision: Decision, reaso
162164
// ─── WebFetch: URL-based classification ─────────────────────────────────────
163165

164166
const WEBFETCH_SOFT_DENY_PATTERNS: Rule[] = [
165-
{ pattern: /\b(pastebin\.com|paste\.ee|hastebin\.com|dpaste\.org|ghostbin\.com|rentry\.co)\b/i, reason: 'Paste service needs user intent verification' },
166-
{ pattern: /\b(transfer\.sh|file\.io|0x0\.st|tmpfiles\.org)\b/i, reason: 'File sharing service needs user intent verification' },
167+
{ pattern: /^https?:\/\/(?:[^/]+\.)?(pastebin\.com|paste\.ee|hastebin\.com|dpaste\.org|ghostbin\.com|rentry\.co)(?:\/|$)/i, reason: 'Paste service needs user intent verification' },
168+
{ pattern: /^https?:\/\/(?:[^/]+\.)?(transfer\.sh|file\.io|0x0\.st|tmpfiles\.org)(?:\/|$)/i, reason: 'File sharing service needs user intent verification' },
167169
{ pattern: /\.(sh|bash|ps1|bat|cmd)(\?|$)/i, reason: 'Script download needs user intent verification' },
168-
{ pattern: /\braw\.githubusercontent\.com\/.*\.(sh|py|rb|js)$/i, reason: 'Raw script download needs user intent verification' },
170+
{ pattern: /\braw\.githubusercontent\.com\/.*\.(sh|py|rb|js)(?:\?|$)/i, reason: 'Raw script download needs user intent verification' },
169171
]
170172

171173
export function classifyWebFetch(url: string): { decision: Decision, reason: string } | null {
@@ -180,7 +182,7 @@ export function classifyWebFetch(url: string): { decision: Decision, reason: str
180182
}
181183

182184
// Localhost and known dev services are safe
183-
if (/^https?:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0)(?::\d+)?/i.test(url)) {
185+
if (/^https?:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0)(?::\d+)?(?:[/?#]|$)/i.test(url)) {
184186
return { decision: 'allow', reason: 'Safe localhost request' }
185187
}
186188

0 commit comments

Comments
 (0)