diff --git a/pkg/safety/classify.go b/pkg/safety/classify.go index f8738ea66f..d2d3ecbf53 100644 --- a/pkg/safety/classify.go +++ b/pkg/safety/classify.go @@ -204,6 +204,20 @@ func carriesDenyFlag(normalized string, flags []string) bool { return false } +// isShellMetacharAnchor reports whether c can start a pattern that +// only makes sense as a shell operator (redirect, pipe, chain, +// separator). Anchoring such patterns on `\b` is too weak — a word +// boundary fires between any word char and the operator, so a +// letter-adjacent `>` (e.g. inside `` or a literal `"a>b"`) +// would spuriously match `> `. +func isShellMetacharAnchor(c byte) bool { + switch c { + case '>', '<', '|', '&', ';': + return true + } + return false +} + // containsShellMetacharacter returns true when the command contains a // character that can chain (`;`, `&`), pipe (`|`), redirect (`<`, `>`), // or substitute (backticks, `$(`) commands — with or without @@ -300,9 +314,24 @@ func stringSlice(value any) []string { // matches anywhere in the normalised command. Destructive intent is // the priority — a destructive pattern hidden inside a larger // command (e.g. `cd /tmp && rm -rf foo`) should still match. +// +// A pattern that begins with a shell metacharacter (`>`, `<`, `|`, +// `&`, `;`) is anchored on whitespace rather than a word boundary: +// `>` between two word characters isn't a shell redirect, so a +// placeholder like `` or an inline `"a > b"` inside a quoted +// argument must not match the `> ` truncate pattern. Trade-off: +// fd-number redirects (`1> file`, `2> file`) no longer match either, +// which drops the `2> /dev/null` false positive as a bonus. Gating +// treats destructive and unknown identically, so this only affects +// the label attached to the confirmation dialog. func patternToRegexp(pattern string) string { var b strings.Builder - b.WriteString(`(?i)(?:^|.*\b)`) + b.WriteString(`(?i)`) + if pattern != "" && isShellMetacharAnchor(pattern[0]) { + b.WriteString(`(?:^|.*\s)`) + } else { + b.WriteString(`(?:^|.*\b)`) + } for i := 0; i < len(pattern); { switch pattern[i] { case '<': diff --git a/pkg/safety/safety_patterns.json b/pkg/safety/safety_patterns.json index 42d44db6ce..b674e5b126 100644 --- a/pkg/safety/safety_patterns.json +++ b/pkg/safety/safety_patterns.json @@ -37,6 +37,12 @@ { "pattern": "chmod -R 777 ", "blast_radius": "LOW", "category": "fs-permissions", "notes": "security regression, reversible" }, { "pattern": "sed -i", "blast_radius": "MEDIUM", "category": "fs-modify", "notes": "in-place file edit; -i.bak variants also match" } ], + "powershell": [ + { "pattern": "Remove-Item ...-Recurse", "blast_radius": "HIGH", "category": "fs-delete", "notes": "wildcard spans quoted paths with spaces (`C:\\Program Files\\…`) that `` (=\\S+) could not" }, + { "pattern": "Remove-Item ...-Force", "blast_radius": "MEDIUM", "category": "fs-delete", "notes": "when -Recurse is also present, the HIGH entry above wins on severity" }, + { "pattern": "Remove-Item ", "blast_radius": "LOW", "category": "fs-delete", "notes": "prompts on tty" }, + { "pattern": "Clear-Content ...", "blast_radius": "MEDIUM", "category": "fs-overwrite", "notes": "empties file contents in place, positional or -Path form" } + ], "git": [ { "pattern": "git reset --hard", "blast_radius": "HIGH", "category": "git-discard", "notes": "discards uncommitted work; unrecoverable" }, { "pattern": "git checkout -- ", "blast_radius": "MEDIUM-HIGH", "category": "git-discard", "notes": "overwrites local modifications" }, @@ -93,8 +99,8 @@ { "pattern": "docker system prune", "blast_radius": "MEDIUM", "category": "dk-multi-del", "notes": "containers + images + networks, NO volumes" }, { "pattern": "docker system prune -a", "blast_radius": "MEDIUM-HIGH", "category": "dk-multi-del", "notes": "adds all unused images" }, { "pattern": "docker system prune --all", "blast_radius": "MEDIUM-HIGH", "category": "dk-multi-del", "notes": "long form of -a" }, - { "pattern": "docker system prune --volumes", "blast_radius": "HIGH", "category": "dk-multi-del", "notes": "adds named-volume deletion" }, - { "pattern": "docker system prune -af --volumes", "blast_radius": "HIGH", "category": "dk-multi-del", "notes": "the YOLO combination" }, + { "pattern": "docker system prune --volumes", "blast_radius": "HIGH", "category": "dk-multi-del", "notes": "adds named-volume deletion; --volumes trailed by other flags inherits via the compiled `\\b.*` suffix" }, + { "pattern": "docker system prune ...--volumes", "blast_radius": "HIGH", "category": "dk-multi-del", "notes": "covers -f/-fa (or future flags) between `prune` and `--volumes`" }, { "pattern": "docker buildx prune", "blast_radius": "MEDIUM", "category": "dk-build-cache", "notes": "rebuild cost only" }, { "pattern": "docker buildx prune --all", "blast_radius": "MEDIUM-HIGH", "category": "dk-build-cache", "notes": "every cached layer" }, { "pattern": "docker buildx prune -a", "blast_radius": "MEDIUM-HIGH", "category": "dk-build-cache", "notes": "short form of --all" }, @@ -105,6 +111,8 @@ { "pattern": "docker compose down -v", "blast_radius": "HIGH", "category": "dk-compose", "notes": "drops named volumes" }, { "pattern": "docker compose down --volumes", "blast_radius": "HIGH", "category": "dk-compose", "notes": "long form of -v" }, { "pattern": "docker compose down --volumes --remove-orphans", "blast_radius": "HIGH", "category": "dk-compose", "notes": "aggressive cleanup" }, + { "pattern": "docker compose down ...-v", "blast_radius": "HIGH", "category": "dk-compose", "notes": "covers --remove-orphans (or future flags) between `down` and `-v`" }, + { "pattern": "docker compose down ...--volumes", "blast_radius": "HIGH", "category": "dk-compose", "notes": "long-flag variant" }, { "pattern": "docker compose rm -v ", "blast_radius": "MEDIUM-HIGH", "category": "dk-compose", "notes": "volume removal per service" }, { "pattern": "docker compose rm -f", "blast_radius": "MEDIUM", "category": "dk-compose", "notes": "bypasses removal prompt" }, { "pattern": "docker context rm ", "blast_radius": "LOW", "category": "dk-context", "notes": "reversible if endpoint known" }, @@ -115,6 +123,44 @@ { "pattern": "docker exec rm -rf ", "blast_radius": "HIGH", "category": "dk-exec-host", "notes": "filesystem op via bind mount hits host" }, { "pattern": "docker run --rm -v $(pwd):/x rm -rf /x", "blast_radius": "HIGH", "category": "dk-bind-mount", "notes": "host-bind YOLO pattern" }, { "pattern": "docker run --rm -v /:/host ", "blast_radius": "HIGH", "category": "dk-bind-mount", "notes": "mounts host root — almost always wrong" } + ], + "sql": [ + { "pattern": "mysql ...drop database", "blast_radius": "HIGH", "category": "sql-drop", "notes": "anchored on the client so `grep 'drop database' -r .` stays safe; docker/kubectl exec wrappers still match via the leading `.*\\b` anchor" }, + { "pattern": "mysql ...drop schema", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "mysql ...drop table", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "mysql ...truncate table", "blast_radius": "MEDIUM-HIGH", "category": "sql-truncate" }, + { "pattern": "mysql ...delete from", "blast_radius": "MEDIUM", "category": "sql-delete" }, + { "pattern": "mariadb ...drop database", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "mariadb ...drop schema", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "mariadb ...drop table", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "mariadb ...truncate table", "blast_radius": "MEDIUM-HIGH", "category": "sql-truncate" }, + { "pattern": "mariadb ...delete from", "blast_radius": "MEDIUM", "category": "sql-delete" }, + { "pattern": "psql ...drop database", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "psql ...drop schema", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "psql ...drop table", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "psql ...truncate table", "blast_radius": "MEDIUM-HIGH", "category": "sql-truncate" }, + { "pattern": "psql ...delete from", "blast_radius": "MEDIUM", "category": "sql-delete" }, + { "pattern": "sqlite3 ...drop database", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "sqlite3 ...drop schema", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "sqlite3 ...drop table", "blast_radius": "HIGH", "category": "sql-drop" }, + { "pattern": "sqlite3 ...truncate table", "blast_radius": "MEDIUM-HIGH", "category": "sql-truncate" }, + { "pattern": "sqlite3 ...delete from", "blast_radius": "MEDIUM", "category": "sql-delete" } + ], + "app-cli": [ + { "pattern": "n8n db:reset", "blast_radius": "HIGH", "category": "app-reset", "notes": "wipes the n8n instance database" }, + { "pattern": "n8n user-management:reset", "blast_radius": "MEDIUM", "category": "app-reset", "notes": "resets the n8n owner account and API keys" }, + { "pattern": "bench new-site ...--force", "blast_radius": "HIGH", "category": "app-reset", "notes": "Frappe/ERPNext: --force overwrites an existing site's database; wildcard matches either flag ordering" }, + { "pattern": "bench drop-site", "blast_radius": "HIGH", "category": "app-reset", "notes": "Frappe/ERPNext: drops the site's database and archive" }, + { "pattern": "rails db:drop", "blast_radius": "HIGH", "category": "app-reset", "notes": "Rails: drops the database for the current env" }, + { "pattern": "rails db:reset", "blast_radius": "HIGH", "category": "app-reset", "notes": "Rails: drop + create + migrate + seed" }, + { "pattern": "rails db:schema:load", "blast_radius": "HIGH", "category": "app-reset", "notes": "Rails: replaces the schema, wiping rows" }, + { "pattern": "manage.py flush", "blast_radius": "HIGH", "category": "app-reset", "notes": "Django: removes all data from the database" }, + { "pattern": "prisma migrate reset", "blast_radius": "HIGH", "category": "app-reset", "notes": "Prisma: drops schema and reruns migrations" }, + { "pattern": "sequelize db:drop", "blast_radius": "HIGH", "category": "app-reset", "notes": "Sequelize CLI: drops the database" } + ], + "key-value-store": [ + { "pattern": "redis-cli ...flushall", "blast_radius": "HIGH", "category": "kv-flush", "notes": "wildcard spans connection flags (-h/-p/-n); removes every key from every Redis database" }, + { "pattern": "redis-cli ...flushdb", "blast_radius": "HIGH", "category": "kv-flush", "notes": "as above, current database only" } ] }, "safe": { diff --git a/pkg/safety/safety_test.go b/pkg/safety/safety_test.go index d063ae3b63..3be3c1c128 100644 --- a/pkg/safety/safety_test.go +++ b/pkg/safety/safety_test.go @@ -21,6 +21,13 @@ func TestClassifyCommand_DestructivePatterns(t *testing.T) { {"docker rm -f web", "high"}, {"docker rm -vf web", "high"}, {"docker stop web", "low"}, + {"docker system prune -f --volumes", "high"}, + {"docker system prune --volumes -f", "high"}, + {"docker system prune -a --volumes", "high"}, + {"docker system prune -fa --volumes", "high"}, + {"docker compose down --remove-orphans -v", "high"}, + {"docker compose down -v --remove-orphans", "high"}, + {"docker compose down --remove-orphans --volumes", "high"}, {"docker volume prune --all", "high"}, {"docker volume prune -af", "high"}, {"docker builder prune", "medium"}, @@ -34,6 +41,35 @@ func TestClassifyCommand_DestructivePatterns(t *testing.T) { {"git push --force origin main", "medium"}, {"git push --force-with-lease origin main", "medium"}, {"git stash clear", "high"}, + {`Remove-Item -Path ".\config\cache\*" -Recurse -Force`, "high"}, + {`Remove-Item -Path ".\config\cache\*" -Force -Recurse`, "high"}, + {`Remove-Item "C:\tmp\foo" -Recurse -Force`, "high"}, + {`Remove-Item -Recurse -Force C:\tmp\foo`, "high"}, + {`Remove-Item "C:\Program Files\MyApp\cache" -Recurse -Force`, "high"}, + {`Remove-Item -Path "C:\Users\Foo\OneDrive - Corp\cache" -Recurse -Force`, "high"}, + {`Remove-Item -Force -Recurse "C:\Program Files\MyApp"`, "high"}, + {`Remove-Item "C:\tmp\foo" -Recurse`, "high"}, + {`Remove-Item -Path ".\file.txt" -Force`, "medium"}, + {`Remove-Item .\file.txt -Force`, "medium"}, + {`Remove-Item -Path ".\file.txt"`, "low"}, + {`Clear-Content -Path .\log.txt`, "medium"}, + {`docker exec mariadb-1 mariadb -uroot -proot -e "DROP DATABASE IF EXISTS azfulfillment_db;"`, "high"}, + {`psql -c "DROP TABLE users"`, "high"}, + {`sqlite3 /tmp/x.db "DROP SCHEMA public CASCADE"`, "high"}, + {`mysql -e "TRUNCATE TABLE sessions"`, "high"}, + {`docker exec pg psql -c "DELETE FROM audit_log"`, "medium"}, + {"docker exec n8n n8n db:reset", "high"}, + {"docker exec n8n n8n user-management:reset", "medium"}, + {"docker exec frappe bench new-site --force site1.local", "high"}, + {"docker exec redis redis-cli FLUSHALL", "high"}, + {"docker exec redis redis-cli FLUSHDB", "high"}, + {"redis-cli -h prod.example.com -p 6379 FLUSHALL", "high"}, + {"redis-cli -n 2 flushdb", "high"}, + {"bench new-site site1.local --force", "high"}, + {"bench new-site --force site1.local", "high"}, + {"docker exec app rails db:reset", "high"}, + {"docker exec app python manage.py flush --noinput", "high"}, + {"docker exec app npx prisma migrate reset --force", "high"}, } for _, tt := range tests { t.Run(tt.command, func(t *testing.T) { @@ -124,6 +160,55 @@ func TestClassifyCommand_DenyFlagsAreNeverSafe(t *testing.T) { assert.Equal(t, ClassSafe, ClassifyCommand("git log --oneline -5").Class) } +// SQL patterns are client-anchored so text searches for SQL verbs stay +// safe; drift here silently flips ALLOW→DENY under `restricted`. +func TestClassifyCommand_SqlPatternsDoNotShadowTextSearch(t *testing.T) { + for _, command := range []string{ + `grep "drop table users" -r .`, + `grep -rn "delete from cart" .`, + `rg "DROP DATABASE"`, + `rg "DELETE FROM users"`, + `git log --grep "delete from cart"`, + } { + t.Run(command, func(t *testing.T) { + assert.Equal(t, ClassSafe, ClassifyCommand(command).Class) + }) + } + + // Accepted trade: destructive wins when the client name itself + // appears in the search argument. + label := ClassifyCommand(`grep "mysql -e 'drop database foo'" file`) + assert.Equal(t, ClassDestructive, label.Class) +} + +// The truncate-redirect pattern `> ` must not fire when `>` sits +// between two word characters — it isn't a shell redirect there, just a +// placeholder or comparison operator. Anchoring on `\b` alone was too +// permissive. +func TestClassifyCommand_TruncateRedirectRequiresWhitespaceAnchor(t *testing.T) { + notDestructive := []string{ + `docker exec n8n n8n user:create --email --firstName Admin`, + `git commit -m "feat: 1>0 check"`, + `echo "a>b"`, + } + for _, command := range notDestructive { + t.Run(command, func(t *testing.T) { + assert.NotEqual(t, ClassDestructive, ClassifyCommand(command).Class, + "letter-adjacent > must not match the > truncate pattern") + }) + } + + // Real redirects — `>` preceded by whitespace — must still match. + for _, command := range []string{ + `echo hi > /etc/passwd`, + `cat data > /tmp/out.txt`, + } { + t.Run(command, func(t *testing.T) { + assert.Equal(t, ClassDestructive, ClassifyCommand(command).Class) + }) + } +} + func TestClassifyCommand_UnknownCommand(t *testing.T) { label := ClassifyCommand("./deploy.sh --prod") assert.Equal(t, ClassUnknown, label.Class)