security: narrow skill grants, ship ask/deny rules, guard skill frontmatter, GET-only 403 retry (#642)

* security(skills): drop bare Bash and the portal CLI glob from skill grants

A skill's allowed-tools pre-approves the listed tools for the turn that
invokes it, even in an untrusted folder. job-application-assistant granted
bare Bash, Edit and Write; job-scraper granted bun run on any
.agents/skills/*/cli/src/cli.ts. Every Bash command the skills document is
already allowlisted per command in .claude/settings.json, so the bare and
glob grants bought nothing but exposure. The per-portal grants stay in
settings.json, which is where a portal added by /add-portal registers too
(Step 5 now says so).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* security(settings): deny exfil-shaped shell and credential reads, always ask on permission-defining files

Deny rules block in every permission mode including bypassPermissions and
override any allow rule or skill grant; ask rules force a prompt even when
a skill's allowed-tools would pre-approve the call. settings.json now
ships both: deny for shell that uploads or opens a remote shell (curl
data/form/upload flags, wget post, scp, sftp, ssh, nc) and for reads of
.env and ~/.ssh, ~/.aws, ~/.gnupg; ask for the files whose content is the
permission surface (settings, skill frontmatter, agents, CI, this guard)
and for git push. security_guards.py requires each rule to stay present,
the same way it requires the personal-data gitignore rules. Prefix rules
are tripwires, not walls; SECURITY.md will say so.

Also allowlists tools/robots_check.py and the upcoming tools/fetch_page.py
so the 403 retry no longer needs a curl grant.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* security(guards): hold skill and command allowed-tools to a reviewed allowlist

The guard allowlisted settings.json permissions but never read skill
frontmatter, so the narrow settings allowlist sat next to a bare Bash
grant it could not see. Check 4 parses every .claude/skills/*/SKILL.md,
.agents/skills/*/SKILL.md and .claude/commands/*.md frontmatter with a
stdlib parser (scalar, folded, and list forms) and holds each
allowed-tools entry to ALLOWED_SKILL_TOOLS; a portal skill may
additionally grant exactly its own cli/src/cli.ts, matched by path so no
portal name is hardcoded. Bare Bash and mid-pattern wildcards get their
own message. On master's frontmatter this check reports two failures.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* feat(tools): add GET-only fetch_page.py for the browser-header retry

09-web-research.md documented the 403 retry as a curl command with browser
headers plus an inline python3 -c tag-stripper, which meant the workflow
wanted a Bash(curl:*) grant - and curl with a free tail is an upload
channel. fetch_page.py is the same retry as one command with a fixed GET
argv (URL after --, bounded to 45 s / 5 redirects / 10 MB), http(s) only,
robots_check.gate() enforced with no bypass flag, --out refused inside
the repo, and --text for the stripped page. urllib fallback when curl is
absent. The settings.json grant names the script.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(security): describe the permission surface as a table; route the 403 retry through fetch_page.py

SECURITY.md now lists every shipped grant with its location, lifetime
and guard, and says plainly what the permission layer does not do: no
sandbox on native Windows, prefix rules are tripwires, WebFetch is
unrestricted by domain, subagents run with the parent's tools (next
change), and Edit/Write stay granted because tailoring a CV is editing
files. 09-web-research.md's retry section is the one fetch_page.py
command (framework_version 1.1.3 -> 1.1.4); apply, rank and scrape point
at it instead of curl. SETUP.md and the README tools tree follow.
CHANGELOG gains a Security section under [Unreleased].

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* security(review): ask on command frontmatter, portal skills grant only their CLI, bound the urllib fallback

Findings from a fresh-context review of the branch:

- A command's frontmatter is a grant too (check 4 already parses it), so
  Edit(.claude/commands/*.md) joins the required ask rules; without it a
  steered session could write `allowed-tools: Bash` into apply.md with no
  prompt and the next /apply would be pre-approved for bare Bash.
- SECURITY.md promised a portal skill may grant "its own CLI and nothing
  else"; the guard now enforces exactly that instead of own-CLI plus the
  core allowlist. All six shipped portal skills already comply.
- fetch_page.py's urllib fallback now matches curl's 5-redirect cap and
  applies the 10 MB cap after decompression, so the docstring's bounds
  hold on both paths.
- Frontmatter parser: a BOM no longer reads as "missing frontmatter",
  YAML flow lists split correctly, and a non-UTF-8 file fails with a
  message instead of a traceback. Module docstring lists check 4.
- CHANGELOG no longer claims "no new prompt": compile and aux cleanup,
  which the removed bare Bash covered in a skill-invoking turn, now
  prompt, deliberately. "Once per session" wording corrected to per call.
- Three upload-shaped deny siblings added (curl -XPOST, curl --request
  POST, wget --method=POST); /add-portal recon points at fetch_page.py.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

tests/test_security_guards.py

@@ -47,8 +47,44 @@ class GuardRepoFixture(unittest.TestCase):
         self.manifest.parent.mkdir(parents=True)
         self.write_manifest({"name": "example-cli", "scripts": {"start": "bun run src/cli.ts"}})
 
-    def write_settings(self, allow):
-        self.settings.write_text(json.dumps({"permissions": {"allow": list(allow)}}))
+        # Frontmatter grants: one core skill, one portal skill granting its own
+        # CLI, one command with no frontmatter at all. All pass as written.
+        self.core_skill = self.root / ".claude" / "skills" / "example" / "SKILL.md"
+        self.core_skill.parent.mkdir(parents=True)
+        self.write_skill(self.core_skill, "Read, Glob, Grep, WebFetch, Edit, Write")
+        self.portal_skill = self.root / ".agents" / "skills" / "example-search" / "SKILL.md"
+        self.write_skill(
+            self.portal_skill, "Bash(bun run .agents/skills/example-search/cli/src/cli.ts *)"
+        )
+        self.command = self.root / ".claude" / "commands" / "example.md"
+        self.command.parent.mkdir(parents=True)
+        self.command.write_text("# /example - A command with no frontmatter\n", encoding="utf-8")
+
+    def write_skill(self, path, allowed_tools, extra_frontmatter=""):
+        allowed = (
+            f"allowed-tools: {allowed_tools}\n"
+            if isinstance(allowed_tools, str)
+            else "allowed-tools:\n" + "".join(f"  - {t}\n" for t in allowed_tools)
+        )
+        path.write_text(
+            "---\n"
+            f"name: {path.parent.name}\n"
+            "description: >\n"
+            "  Folded description line one,\n"
+            "  line two mentions Bash, Edit, Write on purpose.\n"
+            f"{extra_frontmatter}"
+            f"{allowed}"
+            "---\n\n# Body\n",
+            encoding="utf-8",
+        )
+
+    def write_settings(self, allow, ask=None, deny=None):
+        permissions = {
+            "allow": list(allow),
+            "ask": list(security_guards.REQUIRED_ASK_RULES if ask is None else ask),
+            "deny": list(security_guards.REQUIRED_DENY_RULES if deny is None else deny),
+        }
+        self.settings.write_text(json.dumps({"permissions": permissions}))
 
     def write_gitignore(self, rules):
         self.gitignore.write_text("\n".join(rules) + "\n")
@@ -107,6 +143,200 @@ class PermissionGuardTests(GuardRepoFixture):
                 self.assertNotIn("Traceback", result.stderr)
 
 
+class AskDenyRuleTests(GuardRepoFixture):
+    """settings.json ships deny rules (block in every mode, bypass included) and
+    ask rules (always prompt, even over a skill's allowed-tools grant). Removing
+    one silently re-opens the hole it closed, so each is required, like the
+    personal-data gitignore rules."""
+
+    def test_missing_required_deny_rule_fails(self):
+        deny = list(security_guards.REQUIRED_DENY_RULES)
+        dropped = deny.pop()
+        self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS), deny=deny)
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("required deny rule missing", result.stdout)
+        self.assertIn(dropped, result.stdout)
+
+    def test_missing_required_ask_rule_fails(self):
+        ask = list(security_guards.REQUIRED_ASK_RULES)
+        dropped = ask.pop(0)
+        self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS), ask=ask)
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("required ask rule missing", result.stdout)
+        self.assertIn(dropped, result.stdout)
+
+    def test_extra_deny_and_ask_rules_pass(self):
+        # Narrowing further is always fine; only removal is guarded.
+        self.write_settings(
+            sorted(security_guards.ALLOWED_PERMISSIONS),
+            ask=list(security_guards.REQUIRED_ASK_RULES) + ["Bash(git commit:*)"],
+            deny=list(security_guards.REQUIRED_DENY_RULES) + ["WebFetch(domain:pastebin.com)"],
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_required_rules_are_the_shipped_shapes(self):
+        # The constants are the contract SECURITY.md describes; pin the two
+        # groups that matter most so a rename cannot quietly drop them.
+        self.assertIn("Read(.env)", security_guards.REQUIRED_DENY_RULES)
+        self.assertIn("Read(~/.ssh/**)", security_guards.REQUIRED_DENY_RULES)
+        self.assertIn("Bash(ssh:*)", security_guards.REQUIRED_DENY_RULES)
+        # The obvious siblings of the listed upload shapes.
+        for sibling in ("Bash(curl -XPOST:*)", "Bash(curl --request POST:*)", "Bash(wget --method=POST:*)"):
+            self.assertIn(sibling, security_guards.REQUIRED_DENY_RULES)
+        self.assertIn("Edit(.claude/settings.json)", security_guards.REQUIRED_ASK_RULES)
+        self.assertIn("Edit(.claude/skills/*/SKILL.md)", security_guards.REQUIRED_ASK_RULES)
+        # A command's frontmatter is a grant too (check 4 parses it), so writing
+        # one must prompt - otherwise a steered session could add
+        # `allowed-tools: Bash` to apply.md with no prompt and the next /apply
+        # would be pre-approved for bare Bash.
+        self.assertIn("Edit(.claude/commands/*.md)", security_guards.REQUIRED_ASK_RULES)
+        self.assertIn("Bash(git push:*)", security_guards.REQUIRED_ASK_RULES)
+
+    def test_malformed_ask_or_deny_shape_fails_cleanly(self):
+        for permissions, message in [
+            ({"allow": [], "deny": "Bash(ssh:*)"}, "permissions.deny must be a list of strings"),
+            ({"allow": [], "ask": [1]}, "permissions.ask must be a list of strings"),
+            ({"allow": []}, "required deny rule missing"),
+        ]:
+            with self.subTest(permissions=permissions):
+                self.settings.write_text(json.dumps({"permissions": permissions}))
+                result = run_guards(self.root)
+                self.assertEqual(result.returncode, 1)
+                self.assertIn(message, result.stdout)
+                self.assertNotIn("Traceback", result.stderr)
+
+
+class SkillGrantGuardTests(GuardRepoFixture):
+    """A skill's or command's allowed-tools pre-approves those tools for the turn
+    that invokes it, with no prompt, in untrusted folders too. settings.json is
+    allowlisted; this holds the frontmatter to the same bar."""
+
+    def test_bare_bash_in_skill_fails(self):
+        self.write_skill(self.core_skill, "Read, Bash, Edit")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("allowed-tools entry not in the reviewed allowlist", result.stdout)
+        self.assertIn("'Bash'", result.stdout)
+        self.assertIn(".claude/skills/example/SKILL.md", result.stdout.replace("\\", "/"))
+
+    def test_glob_cli_grant_in_skill_fails(self):
+        self.write_skill(self.core_skill, "Read, Bash(bun run .agents/skills/*/cli/src/cli.ts *)")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("wildcard", result.stdout)
+        self.assertIn(".agents/skills/*/cli/src/cli.ts", result.stdout)
+
+    def test_portal_skill_may_grant_only_its_own_cli(self):
+        for own in (
+            "Bash(bun run .agents/skills/example-search/cli/src/cli.ts *)",
+            "Bash(bun run .agents/skills/example-search/cli/src/cli.ts:*)",
+        ):
+            with self.subTest(own=own):
+                self.write_skill(self.portal_skill, own)
+                result = run_guards(self.root)
+                self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+        self.write_skill(
+            self.portal_skill, "Bash(bun run .agents/skills/other-search/cli/src/cli.ts *)"
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("other-search", result.stdout)
+
+    def test_portal_skill_may_not_grant_core_tools(self):
+        # SECURITY.md promises "exactly that portal's own cli/src/cli.ts and
+        # nothing else"; the guard has to make that true.
+        self.write_skill(
+            self.portal_skill,
+            "Read, Edit, Bash(bun run .agents/skills/example-search/cli/src/cli.ts *)",
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("'Edit'", result.stdout)
+
+    def test_bom_prefixed_frontmatter_is_parsed(self):
+        # Windows editors prepend a BOM; the file must parse, not fail as
+        # "missing frontmatter".
+        self.write_skill(self.core_skill, "Read")
+        self.core_skill.write_bytes(b"\xef\xbb\xbf" + self.core_skill.read_bytes())
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_non_utf8_skill_fails_cleanly(self):
+        self.core_skill.write_bytes(b"---\nname: x\nallowed-tools: Read\ndescription: caf\xe9\n---\n")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("unreadable", result.stdout)
+        self.assertNotIn("Traceback", result.stderr)
+
+    def test_flow_list_allowed_tools_is_parsed(self):
+        self.write_skill(self.core_skill, "[Read, Grep, Bash]")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("'Bash'", result.stdout)
+        self.assertNotIn("'[Read'", result.stdout)
+
+    def test_core_skill_may_not_grant_a_portal_cli(self):
+        # Per-portal grants live in settings.json; a core skill naming one would
+        # hardcode a portal and bypass the settings allowlist.
+        self.write_skill(
+            self.core_skill, "Bash(bun run .agents/skills/example-search/cli/src/cli.ts *)"
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("allowed-tools entry not in the reviewed allowlist", result.stdout)
+
+    def test_command_frontmatter_is_checked(self):
+        self.command.write_text(
+            "---\ndescription: Example\nallowed-tools: Bash\n---\n# /example\n", encoding="utf-8"
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("'Bash'", result.stdout)
+        self.assertIn("commands/example.md", result.stdout.replace("\\", "/"))
+
+    def test_command_without_frontmatter_passes(self):
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_list_form_allowed_tools_is_parsed(self):
+        self.write_skill(self.core_skill, ["Read", "Grep", "Bash"])
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("'Bash'", result.stdout)
+
+    def test_folded_description_does_not_confuse_parser(self):
+        # The fixture's description folds across lines that mention Bash, Edit,
+        # Write. Only the allowed-tools line counts.
+        self.write_skill(self.core_skill, "Read")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_entry_with_mid_pattern_wildcard_fails(self):
+        self.write_skill(self.core_skill, "Read, Bash(python tools/*.py:*)")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("wildcard", result.stdout)
+
+    def test_allowlisted_shipped_entries_pass(self):
+        self.write_skill(
+            self.core_skill,
+            "Read, Write, Edit, Glob, Grep, Bash(bun --version), Bash(python tools/job_key.py:*), "
+            "Bash(python3 tools/job_key.py:*), WebFetch, WebSearch, Agent, AskUserQuestion",
+        )
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_skill_without_frontmatter_fails_cleanly(self):
+        self.core_skill.write_text("# No frontmatter\n", encoding="utf-8")
+        result = run_guards(self.root)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("frontmatter", result.stdout)
+        self.assertNotIn("Traceback", result.stderr)
+
+
 class HookGuardTests(GuardRepoFixture):
     """A hook in .claude/settings.json runs with no prompt when its event fires.
 
@@ -119,7 +349,11 @@ class HookGuardTests(GuardRepoFixture):
         self.settings.write_text(
             json.dumps(
                 {
-                    "permissions": {"allow": sorted(security_guards.ALLOWED_PERMISSIONS)},
+                    "permissions": {
+                        "allow": sorted(security_guards.ALLOWED_PERMISSIONS),
+                        "ask": list(security_guards.REQUIRED_ASK_RULES),
+                        "deny": list(security_guards.REQUIRED_DENY_RULES),
+                    },
                     "hooks": hooks,
                 }
             )