feat(p5): final 3 hook patterns complete write-your-own-hook.md#20
Open
rocklambros wants to merge 1 commit into
Open
feat(p5): final 3 hook patterns complete write-your-own-hook.md#20rocklambros wants to merge 1 commit into
rocklambros wants to merge 1 commit into
Conversation
P5 Tasks 125-127. Stacks on PR #19 (Write/Edit hooks). Appends the last three patterns to close out the 11-pattern set the plan calls for: - block-dangerously-set-inner-html (Task 125) — regex-based JSX/TSX hook. Detects React's dangerouslySetInnerHTML={{__html: expr}} prop and denies unless expr is a string literal OR a call containing a known sanitiser name (DOMPurify, sanitize-html, nh3, etc.). Template literals WITH ${} substitution are correctly rejected; WITHOUT substitution are safe. - warn-missing-pydantic-fastapi (Task 126) — the only PostToolUse advisory hook. Inspects FastAPI route handlers via AST. Emits stderr advisory (the runtime surfaces stderr into the conversation) when a route handler takes a body-shaped param (dict/list/bytes or non-Pydantic class) without a Pydantic model. Scalar primitives (str/int/float/bool) pass — those are path/query primitives, not body shape. - block-shell-true-with-interpolation (Task 127) — AST-based. Detects subprocess.{run,Popen,call,check_output,check_call} with shell=True AND a first-arg that performs string interpolation (f-string with substitution, str.format(), %, +) OR a variable name in a user-input hint set. Allows shell=False list-form and bare-literal shell=True. Each pattern follows the same template as PRs #18 and #19: Purpose, Hook event, Tools matched, Python source (~80-200 lines), Settings.json entry, enumerated bypass classes, ~30-line pytest suite, Layer-1 complement. Verified after one mid-flight correction: - All 22 Python blocks across the doc parse cleanly (11 hook sources + 11 test files) - All 11 JSON settings entries parse cleanly - 9/9 smoke tests pass across the 3 new hooks (deny/allow paths plus PostToolUse stderr capture for the advisory) The mid-flight correction: initial warn-missing-pydantic-fastapi conflated scalar primitives (str/int — fine for path/query params) with body-shaped primitives (dict/list/bytes — flag-worthy when used without a schema). Both lived in the same BUILTIN_PRIMITIVE_HINTS set, so `body: dict` was silently treated as "not body-like" and never reached the schema check. Fix: split into SCALAR_PRIMITIVE_HINTS (skip) and BODY_SHAPED_PRIMITIVE_HINTS (don't skip — flag if no Pydantic annotation). PR-21 (the final P5 piece) — docs/explanation/enforcement-coverage.md — will land separately. The Task 117-127 set is complete with this PR.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the final 3 hook-pattern sections in docs/how-to/write-your-own-hook.md, adding guidance + copy/paste hook implementations for JSX dangerouslySetInnerHTML, a FastAPI PostToolUse advisory about missing Pydantic models, and a Python subprocess(..., shell=True) interpolation blocker.
Changes:
- Added a regex-based Write/Edit hook pattern to block unsafe
dangerouslySetInnerHTMLassignments unless they’re clearly safe. - Added the guide’s only PostToolUse advisory hook pattern to warn on FastAPI request bodies without Pydantic models.
- Added an AST-based Write/Edit hook pattern to deny
subprocess.*(..., shell=True)when the command argument is interpolated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1830
to
+1836
| # Matches `dangerouslySetInnerHTML={{ __html: <expr> }}` and captures <expr> | ||
| # up to the closing brace pair. The non-greedy capture stops at the FIRST | ||
| # } } pair so nested object expressions are not silently truncated. | ||
| PROP_PATTERN = re.compile( | ||
| r"dangerouslySetInnerHTML\s*=\s*\{\s*\{\s*__html\s*:\s*([^}]+?)\s*\}\s*\}", | ||
| re.DOTALL, | ||
| ) |
Comment on lines
+1905
to
+1910
| deny( | ||
| "block-dangerously-set-inner-html refused dangerouslySetInnerHTML " | ||
| f"bound to non-literal expression: '{expr[:60]}...'. Use a " | ||
| "sanitiser (DOMPurify, sanitize-html, nh3) and wrap the call so " | ||
| "this hook sees the sanitiser name in the bound expression." | ||
| ) |
| - Routes whose body is read via `await request.json()` instead of declared as a parameter — the parameter list looks clean, but the body still flows through unvalidated. | ||
| - Models defined in a separate file and imported under a short alias — the heuristic `annotation_is_pydantic_like` accepts any uppercase-first-letter name, so a typo like `class user(BaseModel)` falsely passes the lower-case check (would be flagged) while `User` passes the case check even if it isn't actually a BaseModel subclass. | ||
| - Dataclasses or `TypedDict` — these provide types but no runtime validation. The hook treats them as Pydantic-like (uppercase first letter), which is a false negative. | ||
| - Non-FastAPI frameworks (Flask, Django, Starlette without FastAPI) — the cheap filter requires `@app.` / `@router.` / `fastapi` / `FastAPI` to appear; other frameworks short-circuit. |
Comment on lines
+2296
to
+2310
| def test_silent_on_non_fastapi_file(): | ||
| _, err = run_write( | ||
| "from flask import Flask\n" | ||
| "app = Flask(__name__)\n" | ||
| "@app.post('/x')\n" | ||
| "def f(body: dict):\n" | ||
| " return body\n" | ||
| ) | ||
| # The cheap filter requires `@app.` AND a FastAPI marker; Flask matches | ||
| # `@app.` so it goes through. The AST then runs. The heuristic flags | ||
| # `body: dict` regardless of framework — accepted false positive cost | ||
| # for the simpler filter. Document this in the bypass list above. | ||
| # If you only want FastAPI warnings, tighten the filter or strip the | ||
| # `@app.` substring from it. | ||
| pass |
Comment on lines
+2081
to
+2086
| Accepts: | ||
| - Any class name starting with an uppercase letter that is NOT a known | ||
| primitive or stdlib type (assumed to be a user model class) | ||
| - Annotated[<Model>, ...] | ||
| - Body(...) typed wrappers (FastAPI's explicit dependency form) | ||
| Rejects: |
Comment on lines
+2449
to
+2451
| text = extract_candidate_text(payload) | ||
| if not text or "shell=True" not in text or "subprocess" not in text: | ||
| return 0 |
Comment on lines
+2468
to
+2476
| write_decision( | ||
| "deny", | ||
| f"block-shell-true-with-interpolation found subprocess.{first[0]}(" | ||
| f"shell=True, ...) with first-arg {first[1]} at line {first[2]}. " | ||
| "Use shell=False with a list argument (subprocess.run([\"cmd\", " | ||
| "*args])) or shlex.quote() each interpolated value if you must " | ||
| "stay on a shell command line.", | ||
| ) | ||
| return 0 |
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked PR — base is
feat/p5-hook-patterns-write-edit(PR #19). This is the third and final hook-pattern PR. Together PRs #18, #19, and this one ship all 11 patterns the plan calls for (Tasks 117-127).Final three patterns:
block-dangerously-set-inner-html(Task 125)warn-missing-pydantic-fastapi(Task 126)block-shell-true-with-interpolation(Task 127)subprocess.{run,Popen,call,check_output,check_call}withshell=TrueAND f-string-subst /.format()/%/+/ user-input-named variable as first argTest plan
Mid-flight correction (worth surfacing)
Initial
warn-missing-pydantic-fastapiconflated scalar primitives (str/int — fine for path/query params) with body-shaped primitives (dict/list/bytes — flag-worthy when used without a schema). Both lived in the sameBUILTIN_PRIMITIVE_HINTSset, sobody: dictwas treated as "not body-like" and never reached the schema check.Fix: split into
SCALAR_PRIMITIVE_HINTS(skip — they're path/query) andBODY_SHAPED_PRIMITIVE_HINTS(don't skip — flag if no Pydantic annotation). Verified with 5 cases: dict body warns, list body warns, Pydantic-annotated silent,Annotated[Item, Body()]silent,id: intsilent.This is the second time during this hook series that smoke testing caught a logic bug the bare AST inspection let through (PR #19 caught the eval one-hop indirection gap). Worth noting both as evidence the in-band smoke testing was load-bearing.
Full series summary (#18 → #19 → this)
Reviewer
@fewdisc — final piece of the P5 hook series. Worth a holistic look at the doc once all three PRs land in
v2/modernization; the introduction sets reader expectations for the whole 11-pattern set, so if the framing should change (more cautionary, less cautionary, different ordering) it's easiest to do once.The companion
docs/explanation/enforcement-coverage.md(referenced by both the merge guide and these hooks) is still future work — it deserves its own PR after the hooks land, since it cross-references both the Layer 1 template AND every hook's bypass list.Related
feat/p5-hook-patterns-write-edit) — this is its stacked childdocs/explanation/enforcement-coverage.md(catalog of bypass classes across Layer 1 + Layer 2)