From f1ddc982fe4aada7354a4b0549a336aeb5d19424 Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Sun, 17 May 2026 13:44:08 +0200 Subject: [PATCH] docs(audits): record PR-1B merge commit hash in D-2 + D-3 closures PR-1B (#43) merged to main as squash commit ff16664. Update the closure footnotes in docs/audits/PHASE-1-SECURITY.md (D-2 + D-3) and the corresponding rows in docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md, replacing the branch-name placeholders with PR number + commit hash. Mirrors the citation style applied to D-1 in the PR-1A closure trail. Co-Authored-By: Claude Opus 4.7 --- docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md | 4 ++-- docs/audits/PHASE-1-SECURITY.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md index 2465066..8a8025c 100644 --- a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md +++ b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md @@ -144,8 +144,8 @@ If you do all 8: you have layered defenses against D-1 / D-2 / D-3 / D-4 / D-7 u | ID | Audit | Title | Wave | |---|---|---|---| | **D-1** | Security | Skill registry lazy-load = RCE for anyone who can drop a `.py` file | **W1 — CLOSED ([#42](https://github.com/AVADSA25/codec/pull/42), `48ec5d5`)** | -| **D-2** | Security | `/api/forge` fetches arbitrary URL → LLM → writes skill, no review gate | **W1 — CLOSED (PR-1B)** | -| **D-3** | Security | `/api/save_skill` writes directly to skills/ with only substring check | **W1 — CLOSED (PR-1B)** | +| **D-2** | Security | `/api/forge` fetches arbitrary URL → LLM → writes skill, no review gate | **W1 — CLOSED ([#43](https://github.com/AVADSA25/codec/pull/43), `ff16664`)** | +| **D-3** | Security | `/api/save_skill` writes directly to skills/ with only substring check | **W1 — CLOSED ([#43](https://github.com/AVADSA25/codec/pull/43), `ff16664`)** | | **D-4** | Security | `file_write` skill (MCP-exposed) can write to `~/.codec/skills/` | **W1** | | **D-5** | Security | `permission_gate` accepts path-traversal via `fnmatch` (no realpath) | **W1** | | C-1 | Reliability | `codec.py` daemon ignores SIGINT/SIGTERM; leaks sox + tkinter on every restart | W4 | diff --git a/docs/audits/PHASE-1-SECURITY.md b/docs/audits/PHASE-1-SECURITY.md index 0674f1d..7976982 100644 --- a/docs/audits/PHASE-1-SECURITY.md +++ b/docs/audits/PHASE-1-SECURITY.md @@ -66,7 +66,7 @@ No exploits were executed. All bypasses were analyzed against source as text-onl ### D-2 — `/api/forge` endpoint fetches arbitrary URL → LLM → writes skill, no review gate [CRITICAL] **Location:** `routes/skills.py:71-201` (`forge_skill`) -> **Closed by PR-1B** (branch `fix/pr1b-remove-save-skill-and-forge`). The `/api/forge` endpoint and its handler `forge_skill` were removed entirely from `routes/skills.py`. Skill creation now routes exclusively through `/api/skill/review` → `/api/skill/approve` (the human-review-and-approve flow). The URL-fetch capability is intentionally dropped per Mickael decision Q1 — users wanting to import code from a URL paste the source into the editor and go through the review-and-approve flow. The Skill Forge modal + toolbar button + JS handlers in `codec_vibe.html` were removed alongside. 9 tests in `tests/test_skill_routes.py` verify the endpoint returns 404 and that the replacement flow remains functional. AGENTS.md §7 documents the contributor / operator workflow. +> **Closed by PR-1B** ([#43](https://github.com/AVADSA25/codec/pull/43), merged as `ff16664`). The `/api/forge` endpoint and its handler `forge_skill` were removed entirely from `routes/skills.py`. Skill creation now routes exclusively through `/api/skill/review` → `/api/skill/approve` (the human-review-and-approve flow). The URL-fetch capability is intentionally dropped per Mickael decision Q1 — users wanting to import code from a URL paste the source into the editor and go through the review-and-approve flow. The Skill Forge modal + toolbar button + JS handlers in `codec_vibe.html` were removed alongside. 9 tests in `tests/test_skill_routes.py` verify the endpoint returns 404 and that the replacement flow remains functional. AGENTS.md §7 documents the contributor / operator workflow. **CWE / OWASP:** CWE-94 Code Injection / CWE-918 SSRF / OWASP A10 SSRF / Agentic A01 Memory Poisoning, A02 Tool Misuse **Description:** Endpoint accepts `code` field; if it starts with `http://` or `https://`, fetches the URL via `requests.get(code, timeout=15)` with no localhost / internal IP filtering, no domain allowlist, no MIME check. The fetched body is then passed to the LLM. The LLM's output is filtered through a 10-pattern substring blocklist (line 170-173: `os.system(`, `subprocess.`, `eval(`, `exec(`, `__import__`, `importlib`, `shutil.rmtree`, `open('/etc`, `open('/dev`, `ctypes`) — bypassable in dozens of ways: split string concatenation (`'os.sys' + 'tem('`), getattr indirection (`getattr(__builtins__, chr(95)*2+'import'+chr(95)*2)`), or simply double-quote variants of the `open('/...` patterns (the blocker only matches single-quote `open('/etc`). After substring check + AST compile (which only catches syntax errors), the code is **written directly to `/skills/.py` AND `~/.codec/skills/.py`** (line 186-190). No `/api/skill/review` staging. @@ -84,7 +84,7 @@ After substring check + AST compile (which only catches syntax errors), the code ### D-3 — `/api/save_skill` writes directly to `skills/` with only substring check [CRITICAL] **Location:** `routes/skills.py:14-28` (`save_skill`) -> **Closed by PR-1B** (branch `fix/pr1b-remove-save-skill-and-forge`). The `/api/save_skill` endpoint and its handler `save_skill` were removed entirely from `routes/skills.py`. Skill creation now routes exclusively through `/api/skill/review` → `/api/skill/approve` (the human-review-and-approve flow), which already runs `is_dangerous_skill_code` at the approve gate. The "Skill" toolbar button in `codec_vibe.html` and the `doSkill()` JS function were removed alongside. Defense-in-depth with PR-1A: even if a malicious file reaches `~/.codec/skills/` via some other path, `SkillRegistry.load` refuses it at load time. Same 9 tests in `tests/test_skill_routes.py` cover D-2 + D-3. +> **Closed by PR-1B** ([#43](https://github.com/AVADSA25/codec/pull/43), merged as `ff16664`). The `/api/save_skill` endpoint and its handler `save_skill` were removed entirely from `routes/skills.py`. Skill creation now routes exclusively through `/api/skill/review` → `/api/skill/approve` (the human-review-and-approve flow), which already runs `is_dangerous_skill_code` at the approve gate. The "Skill" toolbar button in `codec_vibe.html` and the `doSkill()` JS function were removed alongside. Defense-in-depth with PR-1A: even if a malicious file reaches `~/.codec/skills/` via some other path, `SkillRegistry.load` refuses it at load time. Same 9 tests in `tests/test_skill_routes.py` cover D-2 + D-3. **CWE / OWASP:** CWE-94 Code Injection / OWASP A04 Insecure Design **Description:** `/api/save_skill` writes the request body's `content` field directly to `/` after only: 1. Checking that "SKILL_DESCRIPTION" and "def run(" are in the content (presence check, not security).