From 0cedbc7b3a580f50ed4b5d1dfdc2598582822a81 Mon Sep 17 00:00:00 2001 From: Bastien Chanot Date: Fri, 10 Jul 2026 12:48:54 +0200 Subject: [PATCH] =?UTF-8?q?chore(memory):=20LRN-121=20shell=20allowlist=20?= =?UTF-8?q?validation=20(grep=20-Eq=20fragile=20=E2=86=92=20whole-string?= =?UTF-8?q?=20POSIX=20case)=20+=20seo-account-mgmt=20journal=20+=20contrac?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .claude/memory/journal.md | 2 + .claude/memory/learnings.md | 6 +++ .../2026-07-10-seo-account-mgmt-0353.md | 52 +++++++++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 .claude/tasks/contracts/2026-07-10-seo-account-mgmt-0353.md diff --git a/.claude/memory/journal.md b/.claude/memory/journal.md index a097de4..24f4d44 100644 --- a/.claude/memory/journal.md +++ b/.claude/memory/journal.md @@ -375,3 +375,5 @@ rules: - GSC+CrUX data layer for `/seo` FULL shipped end-to-end (subagent-driven, superpowers): design→plan→8 tasks→final review→merge `bb1fbb2` on develop. Engine `lib/seo-data/` (label-keyed OAuth token store 0600/0700, CrUX field + GSC Search-Analytics/URL-Inspection, fail-open `fetch.sh`, `make seo-connect` consent), wired into `/seo` FULL (STEP 0 account select, CrUX-primary CWV, "Performance GSC" quick-wins). 49/49 engine tests + full `make test` green throughout. Final opus whole-branch review: security PASS, 0 Critical/Important, 5 Minors all deferred to a later chore sweep. - Decided [[BDR-063]] OAuth installed-app + explicit `(account,property)` args (no global state) → multi-account no-conflict. Learned [[LRN-119]] fail-open engine contract (always-JSON, lazy imports, degrade-not-crash), [[LRN-120]] final-review base = merge-base not ledger BASE (caught a misleading 881-vs-2163-ins diff). - Docs synced (`/doc`, `4a15c73` on `chore/doc-sync-gsc-crux`): README (seo-connect, make-test glob, /seo row) + USAGE (/seo FULL real-data) + CHANGELOG Added entry. Pending: merge `chore/doc-sync-gsc-crux`→develop (human GO), then delete transient spec+plan `docs/superpowers/…gsc-crux…`. +- Post-ship housekeeping merged to develop: `chore/doc-sync-gsc-crux` (`8a1fac0`, docs+memory+transient-cleanup), then `bugfix/seo-connect-env-source` (`61a98d3`) — `make seo-connect` never sourced `~/.claude/.env` so OAuth creds never reached connect.py; found by real `make seo-connect` run (403 discover_properties after consent = Search Console API not enabled + the env bug). Live OAuth validated end-to-end by user (consent OK, app published to Production for non-expiring refresh token). +- `/feat` feature/seo-account-mgmt (unmerged, human GO pending): account-management verbs — tokenstore remove/clear, fetch.sh forget, connect.sh wrapper (sources env, runs from any project), `/seo connect|accounts|forget` routing, Makefile delegates to wrapper. Commits `8bf7459` (feat) + `887341d` (doc USAGE). Security loop hit its cap: 3 GATE-2 BLOCKs on the label guard (injection → parser differential → per-line-grep newline), closed categorically by a whole-string POSIX `case` guard [[LRN-121]]; final fresh scan PASS (~50 vectors, 0 bypass). 85/85 engine + `make test` green throughout. forget = local delete, NOT Google revocation (surfaces myaccount.google.com/permissions). diff --git a/.claude/memory/learnings.md b/.claude/memory/learnings.md index d8a0c9c..674ea3b 100644 --- a/.claude/memory/learnings.md +++ b/.claude/memory/learnings.md @@ -1208,3 +1208,9 @@ rules: - **why it matters**: the final review is the last gate before merge; a wrong base hides real changes or invents fake ones. The ledger BASE is a task resume-map, not a merge-delta anchor. - **future application**: for ANY whole-branch/final review, derive base from `git merge-base HEAD`, never a stored/remembered SHA. Sanity-check: does `git log BASE..HEAD` list ONLY this branch's commits, nothing foreign? Diff-stats differ between candidate bases → recorded one is stale, trust merge-base. - **cousin**: [[LRN-119]] (same GSC+CrUX build); SDD skill's own "never HEAD~1" warning (same base-selection bug class). + +## LRN-121 — Shell allowlist validation: `grep -Eq` is fragile; use a whole-string POSIX `case` +- **pattern**: guarding a user-supplied label to shell-safe ASCII with `printf '%s' "$v" | grep -Eq '^[A-Za-z0-9._-]+$'` failed 3 adversarial gate passes in a row: (1) command-injection framing (label interpolated into an agent-composed Bash line); (2) parser differential — the guard pre-scanned argv for the literal token `--label` while the downstream `argparse` ALSO accepts `--label=v` and abbreviations (`--labe`, `allow_abbrev=True`), so those forms reached the parser unchecked; (3) `grep -q` matches PER LINE, so a label with an embedded newline (`ok\nrm -rf`) passes because its FIRST line matches. Fix = replace the whole mechanism, don't patch again: `_label_safe() ( LC_ALL=C; case "$1" in ''|[!A-Za-z0-9]*|*[!A-Za-z0-9._-]*) exit 1;; esac )` — POSIX `case`, whole-string, C-locale subshell. No grep (no per-line), no regex, no second grammar to differ from; a newline is just a non-allowed byte caught by `*[!...]*`; `LC_ALL=C` stops UTF-8 collation widening `[A-Za-z0-9]` to homoglyphs (U+FF11, Kelvin U+212A). +- **why it matters**: three distinct bypasses of the SAME guard = the approach was wrong, not each patch. `grep`'s line-orientation + locale-sensitive ranges, plus argv-prescan-vs-real-parser grammar drift, are the three classic ways an allowlist "passes" a string it shouldn't. Whole-string `case` in C locale closes all three at once. These were defense-in-depth (downstream used `"$2"`/`"$@"`/JSON-key, never `sh -c`/`eval` → not exploitable in the real exec chain) — but the backstop still took a categorical rewrite, and 3 security-gate BLOCKs to get there. +- **future application**: validate shell input WHOLE-STRING (`case` or bash `[[ =~ ]]`), never `grep -q` (per-line). Set `LC_ALL=C` for byte-wise ranges. A guard that pre-scans argv must be STRICTER than the downstream parser (reject `=`-joined/abbrev) or validate post-parse against the value the parser settled on. When a fix is bypassed twice → STOP patching, replace the mechanism (re-plan, not whack-a-mole). +- **cousin**: [[LRN-119]] (fail-open engine this hardens), [[BDR-063]] (token store whose labels these guard), [[LRN-045]] (renaming-command leak-guard regexes — same charset-guard family). diff --git a/.claude/tasks/contracts/2026-07-10-seo-account-mgmt-0353.md b/.claude/tasks/contracts/2026-07-10-seo-account-mgmt-0353.md new file mode 100644 index 0000000..3a38650 --- /dev/null +++ b/.claude/tasks/contracts/2026-07-10-seo-account-mgmt-0353.md @@ -0,0 +1,52 @@ +# CONTRACT — seo-account-mgmt +- date: 2026-07-10 | flow: feat | branch: feature/seo-account-mgmt +- status: active + +## REQUEST (verbatim — IMMUTABLE) +"J'aimerais qu'on rajoute quand meme une option au skill pour juste connecter +le compte. du style un argument au skill seo pour fiare un truc du genre /set +seo-connect ou quelque chjose comme cas. Et aussi pouvoir clean la liste des +compte deja enregister. pouvoir supprimer des compte ou tout supprimer" +— design proposal validated by user ("go pour l'un puis l'autre oui"): +`/seo connect [label]` / `/seo accounts` / `/seo forget