mirror of
https://github.com/alirezarezvani/claude-skills.git
synced 2026-08-28 04:24:58 +00:00
fix(human-gate): sanitize reviewed HTML; fix 3 gate-integrity defects
Second PR-review round on #948. All four reproduced before fixing. 1. HIGH — reviewed HTML executed in the review page. BlockTagger re-emitted attributes verbatim, escaping values but never filtering attribute names or URL schemes. The Markdown path has had _safe_href scheme-allowlisting all along; the HTML path had nothing. Reproduced: a draft.html containing `<img src=x onerror=...>`, `<a href="javascript:alert(1)">` and an <iframe> passed straight into the page a reviewer opens — and reviewing a landing-page draft is a documented use of this skill. sanitize_attrs() drops on* handlers, srcdoc and srcset, and runs href/src/ action/formaction/poster/cite/background through the scheme allowlist. _safe_href now strips control characters before reading the scheme (so `java\tscript:` cannot smuggle one) and allows data:image only for image attributes. DROP_TAGS removes iframe/object/embed/frame/base/applet as well as script/style/head/link/meta. Verified: handlers, javascript: (plain and tab-smuggled), and iframes all gone; https links and relative images kept. 2. MEDIUM — verify_quotes compared rendered text against raw markup. A quote comes from window.getSelection(), which is what the browser rendered, so selecting a sentence containing **bold**, `code` or a link never matched the raw source. G7 had just made that blocking, so this refused legitimate closes. Now matched against raw OR a rendered-text projection (inline markup stripped for Markdown, tags stripped and entities unescaped for HTML). A fabricated quote is still caught — verified both directions. 3. MEDIUM — state["waiver"] was never cleared, so after waived-close → reopen → a clean round, close still printed the old waiver reason. For a tool whose premise is an honest record of what was actually reviewed and waived, that is its own integrity bug. Cleared whenever a close passes with zero refusals. 4. LOW — status returned 4 for both "no sidecar yet" and "collected, blockers open". The blocked case now returns 2, matching close, so an agent can branch on the exit code alone: 0 clear, 2 blocked, 3 collect, 4 nothing yet. Also corrected an over-broad claim of my own: the page makes no network request of its own, but a reviewed HTML artifact's own https: assets do load, as they must for the review to be faithful. README and SKILL.md now say that precisely. Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6 PASS, description validator PASS, all three scripts --help/--sample green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
This commit is contained in:
Claude
parent
daa4dde7d1
commit
70c908a65a
7 changed files with 139 additions and 23 deletions
19
CHANGELOG.md
19
CHANGELOG.md
|
|
@ -55,7 +55,24 @@ three stdlib-only Python scripts, no server, no socket, no network fetch.
|
|||
waiver without a recorded reason, or a round carrying unresolved integrity problems.
|
||||
Waivers store both the reason and every refusal they overrode.
|
||||
|
||||
**G7 came out of PR review** and closes a real hole: a mistyped severity heading
|
||||
**Four more fixes came out of a second PR review round**, each reproduced before fixing:
|
||||
**(a)** the HTML artifact path re-emitted attributes verbatim, so a reviewed draft containing
|
||||
`<img src=x onerror=...>`, `<a href="javascript:...">` or an `<iframe>` executed inside the
|
||||
review page — the Markdown path had `_safe_href` scheme-allowlisting all along and the HTML
|
||||
path had nothing. `sanitize_attrs()` now drops `on*`/`srcdoc`/`srcset`, runs every URL
|
||||
attribute through the same allowlist (control characters stripped first, so `java\tscript:`
|
||||
cannot smuggle a scheme), and `DROP_TAGS` removes `iframe`/`object`/`embed`/`base`. Legitimate
|
||||
`https:` links and relative images survive. **(b)** `verify_quotes()` compared a browser
|
||||
selection (rendered text) against raw markup, so quoting a sentence containing `**bold**` or a
|
||||
link failed — and since G7 made that blocking, it refused a legitimate close. It now matches
|
||||
against raw *or* a rendered-text projection, while a genuinely fabricated quote is still
|
||||
caught. **(c)** `state["waiver"]` was never cleared, so a clean unwaived round N+1 still
|
||||
printed round N's waiver reason — in a tool whose premise is an honest record, that is its own
|
||||
integrity bug. **(d)** `status` returned 4 for both "no sidecar yet" and "collected, blockers
|
||||
open"; the blocked case now returns 2, matching `close`, so an agent can branch on the exit
|
||||
code alone (0 clear · 2 blocked · 3 collect · 4 nothing yet).
|
||||
|
||||
**G7 came out of the first PR review round** and closes a real hole: a mistyped severity heading
|
||||
(`## BLOKCER`) silently downgrades to `NIT`, so before this a reviewer's genuine blocker
|
||||
could be lost to a typo and `close` would still exit 0. Reproduced, then fixed — the
|
||||
parser's integrity problems (unknown severity, EDIT with no replacement text, a quote
|
||||
|
|
|
|||
|
|
@ -33,7 +33,7 @@ exactly what is still open.
|
|||
```sh
|
||||
python3 $S/human_gate.py open plan.md --launch # build review page, start round N
|
||||
# → hand over path, END YOUR TURN
|
||||
python3 $S/human_gate.py status plan.md # exit 3 = feedback waiting (non-blocking)
|
||||
python3 $S/human_gate.py status plan.md # non-blocking: 0 clear · 2 blocked · 3 collect · 4 none
|
||||
python3 $S/human_gate.py collect plan.md --output json # batch.v1 — apply every item
|
||||
python3 $S/human_gate.py close plan.md # exit 2 = you are NOT done
|
||||
```
|
||||
|
|
@ -42,7 +42,7 @@ python3 $S/human_gate.py close plan.md # exit 2 = you are NOT done
|
|||
|
||||
| File | Purpose |
|
||||
|---|---|
|
||||
| `scripts/review_page_builder.py` | Markdown/HTML → single-file review page, every block anchored. **Zero network requests** — no CDN, no fonts, no server, no socket. ~11 KB, opens over `file://`. |
|
||||
| `scripts/review_page_builder.py` | Markdown/HTML → single-file review page, every block anchored. The page itself makes **zero network requests** — no CDN, no fonts, no server, no socket; ~11 KB, opens over `file://`. Reviewed HTML is sanitized first: `on*` handlers, `javascript:` URLs, and `iframe`/`object`/`embed` are dropped, so a draft cannot execute inside the page. A reviewed HTML artifact's own `https:` images still load, as they must for the review to be faithful. |
|
||||
| `scripts/feedback_parser.py` | Review sidecar → `batch.v1` JSON, with quote verification against the real file. |
|
||||
| `scripts/human_gate.py` | State machine + the gate. `open`/`status`/`collect`/`close`/`reset`. |
|
||||
| `references/human_in_the_loop_canon.md` | Bainbridge, Fagan, Wiegers, Weinberg, Google SWE ch.9, Klein pre-mortem. |
|
||||
|
|
@ -90,7 +90,7 @@ Severities are **BLOCKER / MAJOR / MINOR / NIT** — the same ladder
|
|||
| **G4** | the sidecar changed after the last collect |
|
||||
| **G5** | round cap exhausted → **escalate**, never pass |
|
||||
| **G6** | a waiver is used without a recorded reason |
|
||||
| **G7** | the round carries unresolved integrity problems — a mistyped severity (`## BLOKCER`) silently downgrades to NIT, an EDIT has no replacement text, or a quote is not in the file |
|
||||
| **G7** | the round carries unresolved integrity problems — a mistyped severity (`## BLOKCER`) silently downgrades to NIT, an EDIT has no replacement text, or a quote is in neither the raw source nor its rendered text |
|
||||
|
||||
Overrides are legitimate and must be explicit:
|
||||
|
||||
|
|
|
|||
|
|
@ -62,9 +62,12 @@ python3 $S/human_gate.py status "$ARTIFACT"
|
|||
|
||||
| Exit | Meaning |
|
||||
|---|---|
|
||||
| 3 | feedback waiting — collect it |
|
||||
| 4 | nothing yet — end the turn again |
|
||||
| 0 | last round collected, nothing blocking |
|
||||
| 2 | collected, but blocking items are open — same code `close` uses |
|
||||
| 3 | feedback waiting — collect it |
|
||||
| 4 | nothing on disk yet — end the turn again |
|
||||
|
||||
Branch on the code alone: 0 clear · 2 blocked · 3 collect me · 4 nothing yet.
|
||||
|
||||
### 3. `collect` — read the batch
|
||||
|
||||
|
|
|
|||
|
|
@ -26,15 +26,15 @@ is available now. Read `human-gate-context.md` first if it exists.
|
|||
S=engineering/human-gate/skills/human-gate/scripts
|
||||
|
||||
python3 $S/human_gate.py open plan.md --launch # build page, start round N → END YOUR TURN
|
||||
python3 $S/human_gate.py status plan.md # exit 3 = feedback waiting (non-blocking)
|
||||
python3 $S/human_gate.py status plan.md # non-blocking: 0 clear·2 blocked·3 collect·4 none
|
||||
python3 $S/human_gate.py collect plan.md --output json # batch.v1 — apply every item
|
||||
python3 $S/human_gate.py close plan.md # exit 2 = NOT done
|
||||
```
|
||||
|
||||
`human_gate.py --sample` runs the whole loop, refusals included, in ~1s. It drives
|
||||
`review_page_builder.py` (Markdown/HTML → single-file anchored page, zero network requests,
|
||||
opens over `file://`) and `feedback_parser.py` (sidecar → `batch.v1`, verifying every quote
|
||||
against the real file); both also run standalone with `--help`/`--sample`.
|
||||
`review_page_builder.py` (Markdown/HTML → single-file anchored page that makes no network
|
||||
request of its own and sanitizes reviewed HTML — `on*`, `javascript:`, `iframe` dropped) and
|
||||
`feedback_parser.py` (sidecar → `batch.v1`, quotes checked against raw *and* rendered text).
|
||||
|
||||
## The sidecar
|
||||
|
||||
|
|
|
|||
|
|
@ -43,6 +43,7 @@ Exit codes
|
|||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import html
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
|
|
@ -316,20 +317,53 @@ def parse(text, target_hint=None):
|
|||
return batch, problems
|
||||
|
||||
|
||||
def plain_text(source, is_html=False):
|
||||
"""Approximate what the browser renders, so quotes can be matched fairly.
|
||||
|
||||
A reviewer selects text in the *rendered* page, so `window.getSelection()`
|
||||
hands back "We expect a 40% lift" for source that reads
|
||||
"We expect a **40%** lift". Comparing that against raw markup would flag a
|
||||
perfectly good quote — and since integrity problems now block the gate
|
||||
(G7), a false positive here refuses a legitimate close.
|
||||
"""
|
||||
text = source
|
||||
if is_html:
|
||||
text = re.sub(r"(?is)<(script|style)\b.*?</\1>", " ", text)
|
||||
text = re.sub(r"(?s)<!--.*?-->", " ", text)
|
||||
text = re.sub(r"(?s)<[^>]+>", " ", text)
|
||||
return html.unescape(text)
|
||||
|
||||
text = re.sub(r"!\[([^\]]*)\]\([^)]*\)", r"\1", text) # images -> alt
|
||||
text = re.sub(r"\[([^\]]+)\]\([^)]*\)", r"\1", text) # links -> label
|
||||
text = re.sub(r"`([^`]+)`", r"\1", text) # inline code
|
||||
text = re.sub(r"\*\*([^*]+)\*\*", r"\1", text) # bold
|
||||
text = re.sub(r"__([^_]+)__", r"\1", text)
|
||||
text = re.sub(r"(?<!\*)\*([^*\n]+)\*(?!\*)", r"\1", text) # italic
|
||||
text = re.sub(r"(?<!_)_([^_\n]+)_(?!_)", r"\1", text)
|
||||
text = re.sub(r"(?m)^\s{0,3}#{1,6}\s+", "", text) # heading markers
|
||||
text = re.sub(r"(?m)^\s{0,3}>\s?", "", text) # blockquote markers
|
||||
return text
|
||||
|
||||
|
||||
def verify_quotes(batch, target_path):
|
||||
"""Report quotes that do not literally appear in the reviewed file."""
|
||||
"""Report quotes that appear in neither the raw source nor its rendered text."""
|
||||
problems = []
|
||||
try:
|
||||
with open(target_path, "r", encoding="utf-8") as handle:
|
||||
source = handle.read()
|
||||
except OSError as err:
|
||||
return ["could not read target %s to verify quotes: %s" % (target_path, err)]
|
||||
normalized = re.sub(r"\s+", " ", source)
|
||||
|
||||
squash = lambda s: re.sub(r"\s+", " ", s)
|
||||
is_html = os.path.splitext(target_path)[1].lower() in (".html", ".htm")
|
||||
haystacks = (squash(source), squash(plain_text(source, is_html=is_html)))
|
||||
|
||||
for item in batch["items"]:
|
||||
quote = item.get("quote", "").strip()
|
||||
if not quote:
|
||||
continue
|
||||
if re.sub(r"\s+", " ", quote) not in normalized:
|
||||
needle = squash(quote)
|
||||
if not any(needle in hay for hay in haystacks):
|
||||
problems.append(
|
||||
"item %s quotes text not found in %s: %r"
|
||||
% (item["id"], os.path.basename(target_path), quote[:60])
|
||||
|
|
|
|||
|
|
@ -38,8 +38,12 @@ Exit codes
|
|||
1 usage error
|
||||
2 gate refuses to close
|
||||
3 feedback is waiting to be collected
|
||||
4 no feedback yet
|
||||
4 no feedback yet (status: no sidecar on disk)
|
||||
5 round cap exhausted — escalate to a human
|
||||
|
||||
`status` uses the same 2 as `close` when the collected round still has open
|
||||
blocking items, so an agent can branch on the exit code alone: 0 clear,
|
||||
2 blocked, 3 collect me, 4 nothing yet.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -250,7 +254,9 @@ def cmd_status(args):
|
|||
elif rounds and rounds[-1].get("sidecar_sha") == current["sha"]:
|
||||
payload["status"] = "collected"
|
||||
payload["blocking_open"] = rounds[-1].get("blocking_open", 0)
|
||||
code = 0 if rounds[-1].get("blocking_open", 0) == 0 else 4
|
||||
# 2 mirrors close's "gate would refuse" so the two agree, and keeps 4
|
||||
# meaning exactly one thing: nothing to collect.
|
||||
code = 0 if rounds[-1].get("blocking_open", 0) == 0 else 2
|
||||
else:
|
||||
payload["status"] = "feedback-waiting"
|
||||
code = 3
|
||||
|
|
@ -364,6 +370,10 @@ def cmd_close(args):
|
|||
"refusals": list(refusals),
|
||||
}
|
||||
refusals = []
|
||||
elif not refusals:
|
||||
# A pass that needed no waiver must not inherit an earlier one, or a
|
||||
# genuinely clean round N+1 reports round N's waiver as if it applied.
|
||||
state["waiver"] = None
|
||||
|
||||
if refusals:
|
||||
print("GATE REFUSED — %s" % os.path.basename(artifact))
|
||||
|
|
|
|||
|
|
@ -33,6 +33,19 @@ BLOCK_TAGS = {
|
|||
"table", "hr", "section", "figure", "div",
|
||||
}
|
||||
|
||||
# Dropped entirely from a reviewed HTML artifact. script/style/head/link/meta
|
||||
# are chrome the review page supplies itself; iframe/object/embed/frame would
|
||||
# execute or fetch third-party content inside a page the reviewer trusts.
|
||||
DROP_TAGS = {
|
||||
"script", "style", "head", "link", "meta",
|
||||
"iframe", "object", "embed", "frame", "frameset", "base", "applet",
|
||||
}
|
||||
|
||||
# Attributes carrying a URL, which must clear the same scheme allowlist the
|
||||
# Markdown path uses. Everything named on* is an event handler and is dropped.
|
||||
URL_ATTRS = {"href", "src", "action", "formaction", "poster", "cite", "background"}
|
||||
DROP_ATTRS = {"srcdoc", "srcset"}
|
||||
|
||||
SAMPLE_MD = """# Quarterly plan
|
||||
|
||||
We expect a 40% lift in activation.
|
||||
|
|
@ -56,12 +69,24 @@ ITALIC = re.compile(r"(?<!\*)\*([^*]+)\*(?!\*)")
|
|||
LINK = re.compile(r"\[([^\]]+)\]\(([^)\s]+)\)")
|
||||
|
||||
|
||||
def _safe_href(url):
|
||||
def _safe_href(url, image=False):
|
||||
"""Allow relative URLs and http/https/mailto; data: only for inline images.
|
||||
|
||||
Control characters are stripped before the scheme is read, so
|
||||
`java\\tscript:alert(1)` cannot smuggle a scheme past the check.
|
||||
"""
|
||||
probe = re.sub(r"[\x00-\x20\x7f]+", "", url)
|
||||
match = re.match(r"^([a-zA-Z][a-zA-Z0-9+.\-]*):", probe)
|
||||
if match and match.group(1).lower() not in ("http", "https", "mailto"):
|
||||
return None
|
||||
return url
|
||||
if not match:
|
||||
return url
|
||||
scheme = match.group(1).lower()
|
||||
if scheme in ("http", "https", "mailto"):
|
||||
return url
|
||||
if image and re.match(
|
||||
r"^data:image/(?:avif|gif|jpe?g|png|webp);base64,", probe, re.IGNORECASE
|
||||
):
|
||||
return url
|
||||
return None
|
||||
|
||||
|
||||
def inline(text):
|
||||
|
|
@ -211,6 +236,29 @@ def render_block(block, block_id):
|
|||
# ------------------------------------------------------------------- html
|
||||
|
||||
|
||||
def sanitize_attrs(attrs):
|
||||
"""Strip event handlers and unsafe URL schemes from a reviewed HTML tag.
|
||||
|
||||
The Markdown path scheme-allowlists every link through _safe_href. Raw HTML
|
||||
input has to clear the same bar: without this an artifact containing
|
||||
`<img src=x onerror=...>` or `<a href="javascript:...">` executes inside the
|
||||
review page the moment the reviewer opens it — and reviewing a landing-page
|
||||
draft is a documented use of this skill.
|
||||
"""
|
||||
kept = []
|
||||
for name, value in attrs:
|
||||
lower = name.lower()
|
||||
if lower.startswith("on") or lower in DROP_ATTRS:
|
||||
continue
|
||||
if lower in URL_ATTRS:
|
||||
safe = _safe_href(value or "", image=(lower in ("src", "poster", "background")))
|
||||
if safe is None:
|
||||
continue
|
||||
value = safe
|
||||
kept.append((name, value))
|
||||
return kept
|
||||
|
||||
|
||||
class BlockTagger(HTMLParser):
|
||||
"""Re-emit an HTML body, tagging top-level block elements with data-hg ids."""
|
||||
|
||||
|
|
@ -231,7 +279,7 @@ class BlockTagger(HTMLParser):
|
|||
if tag == "body":
|
||||
self._in_body = True
|
||||
return
|
||||
if tag in ("script", "style", "head", "link", "meta"):
|
||||
if tag in DROP_TAGS:
|
||||
self._skip += 1
|
||||
return
|
||||
if self._skip:
|
||||
|
|
@ -242,7 +290,8 @@ class BlockTagger(HTMLParser):
|
|||
if not self._in_body:
|
||||
return
|
||||
rebuilt = "".join(
|
||||
' %s="%s"' % (k, html.escape(v or "", quote=True)) for k, v in attrs
|
||||
' %s="%s"' % (k, html.escape(v or "", quote=True))
|
||||
for k, v in sanitize_attrs(attrs)
|
||||
)
|
||||
if self.depth == 0 and tag in BLOCK_TAGS:
|
||||
self.count += 1
|
||||
|
|
@ -257,7 +306,7 @@ class BlockTagger(HTMLParser):
|
|||
if tag == "body":
|
||||
self._in_body = False
|
||||
return
|
||||
if tag in ("script", "style", "head"):
|
||||
if tag in DROP_TAGS:
|
||||
self._skip = max(0, self._skip - 1)
|
||||
return
|
||||
if self._skip or not self._in_body:
|
||||
|
|
@ -276,8 +325,11 @@ class BlockTagger(HTMLParser):
|
|||
def handle_startendtag(self, tag, attrs):
|
||||
if self._skip or not self._in_body:
|
||||
return
|
||||
if tag in DROP_TAGS:
|
||||
return
|
||||
rebuilt = "".join(
|
||||
' %s="%s"' % (k, html.escape(v or "", quote=True)) for k, v in attrs
|
||||
' %s="%s"' % (k, html.escape(v or "", quote=True))
|
||||
for k, v in sanitize_attrs(attrs)
|
||||
)
|
||||
if self.depth == 0 and tag in BLOCK_TAGS:
|
||||
self.count += 1
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue