forked from bchanot/claude
chore(memory): LRN-121 shell allowlist validation (grep -Eq fragile → whole-string POSIX case) + seo-account-mgmt journal + contract
This commit is contained in:
@@ -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).
|
||||
|
||||
@@ -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 <target> 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).
|
||||
|
||||
@@ -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 <label>` /
|
||||
`/seo forget --all`; tokenstore remove+clear verbs; fetch.sh forget dispatch;
|
||||
new connect.sh wrapper (sources env internally, usable from any project);
|
||||
Makefile delegates to it; SKILL.md arg routing + STEP 0 fix; forget output
|
||||
must state local removal ≠ Google revocation (myaccount.google.com/permissions).
|
||||
|
||||
## CLARIFICATIONS
|
||||
none — request complete (design pre-validated in conversation).
|
||||
|
||||
## ACCEPTANCE CRITERIA
|
||||
1. `python3 lib/seo-data/tokenstore.py remove --file F --label X` deletes only
|
||||
label X (others preserved), prints `{"status":"ok","removed":true|false}`,
|
||||
never prints a refresh token; atomic write + fcntl lock as set.
|
||||
2. `python3 lib/seo-data/tokenstore.py clear --file F` empties the store
|
||||
(subsequent list → `"accounts": []`), JSON ok, same write discipline.
|
||||
3. Fail-open preserved on new verbs: bad usage → `{"status":"error",...}` +
|
||||
exit 2; unexpected error → degraded JSON (existing _cli try/except covers).
|
||||
4. `fetch.sh forget --label X` / `forget --all` dispatch to remove/clear
|
||||
within the existing contract (JSON stdout, exit 0 ok, exit 2 bad usage);
|
||||
`fetch.sh forget` with no/invalid flag → exit 2 + JSON.
|
||||
5. New `lib/seo-data/connect.sh`: sources `${SEO_DATA_ENV_FILE:-~/.claude/.env}`
|
||||
internally (set -a, never echoed), picks venv python else system, execs
|
||||
connect.py with passed args; with no creds exits nonzero with the
|
||||
"Set GOOGLE_OAUTH_CLIENT_ID/SECRET" gate message (deterministic, offline).
|
||||
6. Makefile `seo-connect` delegates to connect.sh (env-sourcing duplication
|
||||
from caa5bed removed); venv creation + pip install kept before.
|
||||
7. `skills/seo/SKILL.md` routes `connect [label]` / `accounts` /
|
||||
`forget <label>|--all` BEFORE the audit flow (audit `/seo <url>` unchanged);
|
||||
forget path includes the Google revocation notice
|
||||
(myaccount.google.com/permissions); STEP 0 no longer proposes bare
|
||||
`make seo-connect` as the only path (connect.sh tilde path offered).
|
||||
8. `lib/seo-data/README.md` documents connect.sh, forget verbs, revocation note.
|
||||
9. `lib/seo-data/seo-data.test.sh` covers: remove keeps others / removed:false
|
||||
on missing label / clear empties / redaction on remove / forget via fetch.sh
|
||||
(JSON + exit codes, bad usage 2) / connect.sh offline negative path; plus
|
||||
wiring locks (connect.sh sources vault, Makefile delegates, SKILL routes,
|
||||
README documents). Whole suite + `make test` green.
|
||||
10. No commit attribution trailers; tilde paths for engine calls in SKILL.md.
|
||||
|
||||
## FILE SCOPE
|
||||
lib/seo-data/tokenstore.py, lib/seo-data/fetch.sh, lib/seo-data/connect.sh (new),
|
||||
lib/seo-data/seo-data.test.sh, lib/seo-data/README.md, Makefile, skills/seo/SKILL.md
|
||||
Reference in New Issue
Block a user