diff --git a/AGENTS.md b/AGENTS.md index 71f88d8..458b3e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,7 +135,17 @@ slugs, council names, `.todo/` paths, or file-touched lists in the bullet. Those belong in the commit body. The changelog is what users read. -**If a bullet runs past 6–7 lines, you are explaining yourself.** Cut. +**Length: target 3–5 source lines. 6–7 is the ceiling, not the +target.** If your draft sits at the ceiling, you are restating the +commit body. Past 7, you are explaining yourself. Cut. The most +common failure: writing a 13-line bullet, "tightening" to 7, and +calling it tight. 7 is not tight; 4 is tight. + +**Before writing, read the latest 2–3 entries in `CHANGELOG.md` and +match their density.** If your draft is twice as long as the most +recent peer entry of the same shape (e.g. a multi-skill behavior +change), cut it before committing. Recent entries are the working +definition of "tight" for this project. Bad — wall of prose, file list, AI-ish phrasing ("namesake exemplar"): diff --git a/CHANGELOG.md b/CHANGELOG.md index bee8b40..baa84ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,15 @@ the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html ## [Unreleased] +### Changed + +- Security and pre-commit skills now flag landmines (hardcoded + credentials, string-built SQL, unsafe deserialization) found in + surrounding code, not only when written. `pre-commit-self-review` + hand-offs carry an `Adjacent issues:` line. +- `using-97` Priority rule 4 folds the landmine scan into the + read-before-edit reminder. + ## [0.6.0] — 2026-05-06 Surfaces SRP, KISS, and YAGNI at the spots where their substance diff --git a/skills/pre-commit-self-review/SKILL.md b/skills/pre-commit-self-review/SKILL.md index d554023..be4ac26 100644 --- a/skills/pre-commit-self-review/SKILL.md +++ b/skills/pre-commit-self-review/SKILL.md @@ -49,7 +49,17 @@ Hold that frame while running the checklist below. Run every step before you commit, hand off, or claim completion. -1. **Re-read the diff as a stranger.** Open the diff fresh and read it top to bottom without context. If a section needs you to remember what you were thinking yesterday to make sense of it, the next reader will not have that memory. Rename, comment, or restructure until the diff explains itself. *(Rising, 97/58.)* +1. **Re-read the diff as a stranger, and scan ±20 lines around every hunk for landmines.** Open the diff fresh and read it top to bottom without context. If a section needs you to remember what you were thinking yesterday to make sense of it, the next reader will not have that memory — rename, comment, or restructure until the diff explains itself. Then, in the same pass, read the **20 lines above and below each hunk** in every touched file and look for these six trap shapes in the surrounding code, whether or not your change introduced them: + - **Hardcoded credentials.** String literals that look like API keys, OAuth client secrets, database passwords, JWT signing secrets, bearer tokens, private keys (`-----BEGIN`), or connection strings with embedded passwords — assigned to a variable, passed as an argument, or written into a config file. + - **String-built SQL, LDAP, or shell commands.** F-strings, `+` concatenation, or `.format()` building a query/command string with interpolated values; `subprocess.run(..., shell=True)` with non-constant input. + - **Unsafe deserialization on untrusted input.** `pickle.loads`, `yaml.load` without `SafeLoader`, `marshal.loads`, Java `ObjectInputStream`, PHP `unserialize`, .NET `BinaryFormatter` against data that crosses a trust boundary. + - **Swallowed exceptions.** Broad `except:` / `except Exception:` / `catch (Throwable)` blocks with `pass`, an empty body, or a comment-only body — the call site silently absorbs failures the caller cannot see. + - **TOCTOU patterns.** Check-then-use against the same path or resource (`if os.path.exists(p): open(p)`, `if user.has_permission(x): do(x)`) where the state can change between check and use. + - **Mutable default arguments.** `def f(x=[])`, `def f(x={})`, `def f(x=set())` — the default is shared across calls and accumulates state. + + **Test fixtures, mocks, and example values inside `tests/`, `test_*.py`, `*.spec.*`, `fixtures/`, or files with names containing `mock`, `fake`, or `stub` are not landmines** — they are intentional test data. Skip them. + + When you find one of these in the surrounding code (not in your diff), **surface it in the hand-off — do not silently rewrite the file outside the scope you were asked to change.** Add an `Adjacent issues` line to your summary naming the file, line, and trap shape (e.g. `Adjacent issues: src/billing/charge.py:142 — string-built SQL with f-string interpolation`). If you find none, say so explicitly: `Adjacent issues: none found in ±20 lines of touched hunks.` The named artifact is the verification — agents that skipped the scan have nothing to write on this line. *(Rising, 97/58.)* 2. **Suspect your own code first.** Before you blame the framework, the library, or the flaky test, assume the bug is yours. It almost always is. Walk the code path with the failing input in mind; confirm assumptions about types, ordering, null cases, and shared state. Reach for "compiler bug" only after you have ruled out yours. *(Kelly, 97/9.)* 3. **Know what your next commit is.** State, in one sentence, what this commit does. If the sentence contains "and also" or "various", the commit is two commits. Split it. If you cannot name a clear, bounded change, you are committing speculation — throw the speculative parts away and re-scope. *(Bergh Johnsson, 97/47.)* 4. **Check for deliberate technical debt.** Did you take a shortcut to ship? Name it. File a follow-up note (issue, todo, line in the hand-off) so the debt is visible. Untracked debt accrues silent interest. *(Rose, 97/1.)* @@ -66,6 +76,7 @@ These thoughts mean STOP — do not commit yet: | Thought | Reality | |---|---| | "It works on my machine — shipping it." | "Works" is the verification gate, not the review gate. Re-read the diff as a stranger before claiming done. (97/58) | +| "I'll skip the ±20 line scan — my diff doesn't touch security code." | The point of the scan is to find traps you didn't author. Hardcoded credentials, string-built SQL, and unsafe deserialization in code adjacent to your edit will ship under your name if you don't surface them. The `Adjacent issues:` line is mandatory; "none found" is a valid value, "I didn't look" is not. (97/58) | | "Must be a bug in the library." | Mature libraries used by many people are usually fine. Suspect your code first; reach for "library bug" only after ruling yours out. (97/9) | | "I'll squash this giant commit and figure out the message later." | If you can't state the commit in one sentence now, the commit is speculation. Split it or throw the speculative parts away. (97/47) | | "I took a shortcut, I'll come back and fix it." | The promise is sincere and rarely kept. Track the debt explicitly — issue, todo, hand-off note — or pay it now. (97/1) | @@ -80,6 +91,8 @@ These thoughts mean STOP — do not commit yet: You are done when **all** of the following are true: - [ ] You ruled out your own code as the source of any unresolved oddness before blaming external systems. +- [ ] The diff was re-read top-to-bottom as a stranger, and the ±20 lines around every hunk were scanned for the six landmine shapes (hardcoded credentials, string-built SQL/shell, unsafe deserialization, swallowed exceptions, TOCTOU, mutable default). +- [ ] The hand-off contains an `Adjacent issues:` line — either naming surfaced traps (file, line, shape) or stating `none found` after an actual scan. - [ ] The commit (or hand-off) can be described in one sentence with no "and also." - [ ] Any shortcut taken is named in a tracked follow-up. - [ ] Build is clean — no new warnings, lint errors, or deprecation notices introduced. diff --git a/skills/security-and-trust-boundaries/SKILL.md b/skills/security-and-trust-boundaries/SKILL.md index fb708be..8cda9f6 100644 --- a/skills/security-and-trust-boundaries/SKILL.md +++ b/skills/security-and-trust-boundaries/SKILL.md @@ -11,7 +11,9 @@ Whole categories of production breach recur because the trust boundary is invisi This is a **rigid** skill. Jump to the sub-section that matches what you're writing and run that sub-section's checks. -Fires hardest when untrusted input is crossing into a system with real users — production endpoint, shared service, anything that touches user data, secrets, or auth state. Fires lightly in MVPs, prototypes, internal dev tools, debugging endpoints, and one-off scripts where the architecture is not yet settled — prefer the simplest thing that works, and re-invoke this skill before the code reaches users. +Fires hardest when untrusted input is crossing into a system with real users — production endpoint, shared service, anything that touches user data, secrets, or auth state. Fires lightly in MVPs, prototypes, internal dev tools, debugging endpoints, and one-off scripts where the architecture is not yet settled — prefer the simplest thing that works, and re-invoke this skill before the code reaches users. **Three traps override that calibration and bite regardless of stage: committed credentials in source, string-built SQL or shell commands, and `pickle.loads` (or equivalent) on untrusted input. Surface and address these even in prototypes and one-off scripts.** + +The principles below describe properties of code that crosses a trust boundary, whether you authored that code or encountered it in a file you are touching. When you find one of these traps in pre-existing code adjacent to your edit, surface it in your hand-off — don't silently rewrite the file outside the scope the human asked for. ## When to invoke @@ -30,7 +32,6 @@ Invoke when you're about to: - Renaming a local variable inside a function that happens to live in `auth/` or `crypto/` - Adjusting a docstring or formatting in security-adjacent code - A unit test that pins down already-agreed behavior on validated inputs -- Reading code without modifying it - Editing config files where the values are not secrets and the keys are not new auth toggles - An early-stage MVP or prototype where the architecture is still in flux and no real user data is involved - An internal dev tool, debugging endpoint, or one-off script @@ -69,17 +70,17 @@ Validation is a contract: at this line the data has these properties; below this Secrets are like radioactive material: they leak if you don't actively contain them. -8. **Never log secrets, tokens, PII, or auth headers.** A log line with `Authorization: Bearer `, a stack trace including a credentials object's `__repr__`, an exception message containing the connection string — each of these gets shipped to log aggregators, support tools, and screenshot inboxes. Concrete trap: `logger.error(f"login failed for {user}", extra={"request": request})` includes the request body, which had a password. Mask in middleware (`Authorization → ***`, password fields → `***`), and audit objects' `__repr__` for security-relevant types (override to redact). -9. **Secrets live outside version control and outside images.** No keys in `git`, no keys baked into Docker images, no keys in built JS bundles. Use the platform secret store (AWS Secrets Manager, Vault, Kubernetes Secrets, env vars injected at runtime) and reference them by name. If a secret leaks into a commit, **rotate it before deleting the commit** — the git rewrite does not unleak public history. -10. **Errors leaking internal state.** Default exception handlers that return the stack trace, ORM error messages that include the SQL, or "user not found" vs "wrong password" messages that distinguish account existence — each gives the attacker a probe. Return generic errors to clients; log details server-side. +8. **Logging secrets, tokens, PII, or auth headers is a leak.** A log line containing `Authorization: Bearer `, a stack trace including a credentials object's `__repr__`, or an exception message containing the connection string ships to log aggregators, support tools, and screenshot inboxes. The trap shape: any `log.*`, `print`, or `console.*` call whose arguments include a request object, a session object, an auth header, a password field, or a credentials object — even via interpolation. Concrete trap: `logger.error(f"login failed for {user}", extra={"request": request})` includes the request body, which had a password. Fix by masking in middleware (`Authorization → ***`, password fields → `***`) and overriding `__repr__` on security-relevant types to redact. When you find this in code adjacent to your edit, surface it in your hand-off. +9. **Secrets in source files, version control, or built images are leaks the moment they land.** The trap shape: any string literal that *looks* like a credential — API keys, OAuth client secrets, database passwords, JWT signing secrets, private keys (`-----BEGIN`), connection strings with embedded passwords, bearer tokens — assigned to a variable, passed as an argument, or written into a config file checked into git. Equally a trap: keys baked into Docker images, keys in built JS bundles, keys in CI logs. Fix by moving the value to the platform secret store (AWS Secrets Manager, Vault, Kubernetes Secrets, env vars injected at runtime) and referencing it by name. If a secret has already been committed, **rotate it before deleting the commit** — the git rewrite does not unleak public history. When you find a credential-shaped literal in code you're touching (or adjacent to it), surface it — this trap overrides the prototype/MVP calibration and bites at any stage. +10. **Errors that leak internal state widen the attack surface.** Default exception handlers that return the stack trace, ORM error messages that include the SQL, or "user not found" vs "wrong password" messages that distinguish account existence — each gives the attacker a probe. Code shape: any handler that returns the raw exception, traceback, or framework-default error page to a non-internal caller. Return generic errors to clients; log details server-side. ### Crypto misuse Cryptography is the easiest place to ship something that *looks* secure and isn't. -11. **Passwords: use a password-hash function, not a general hash.** `bcrypt`, `scrypt`, `argon2id`, or PBKDF2 with a tuned cost — never `md5`, `sha1`, or `sha256` of `salt + password`. Password-hash functions are deliberately slow and memory-hard so an attacker can't brute-force at GPU speeds. Concrete trap: `hashlib.sha256(password.encode()).hexdigest()` is crackable on consumer GPUs at billions of attempts per second. -12. **Random for security uses a cryptographic RNG.** Tokens, session IDs, password resets, nonces, CSRF tokens — `secrets.token_urlsafe(32)` (Python), `crypto.randomBytes(32)` (Node), `SecureRandom` (Java). Never `random.random`, `Math.random`, or seeded RNGs — they're predictable enough to forge tokens. -13. **Don't roll crypto.** Don't hand-code AES, don't invent a "lightweight" XOR cipher, don't generate IVs by hand. Use the language's standard library or a well-audited library (libsodium, AWS KMS, OpenSSL bindings). Hardcoded IVs and reused nonces under AES-GCM are catastrophic; the library can prevent both. +11. **General-purpose hashes for passwords are crackable at GPU speeds.** The trap shape: any `md5`, `sha1`, `sha256`, `sha512` call whose input is a password or `salt + password` concatenation, used to derive a stored password verifier. Password verifiers require a deliberately-slow, memory-hard function: `bcrypt`, `scrypt`, `argon2id`, or PBKDF2 with a tuned cost. Concrete trap: `hashlib.sha256(password.encode()).hexdigest()` is crackable on consumer GPUs at billions of attempts per second. When you find a general-purpose hash being used as a password verifier, surface it. +12. **Non-cryptographic RNGs in security contexts are forgeable.** The trap shape: tokens, session IDs, password resets, nonces, CSRF tokens, or anything an attacker would benefit from predicting, generated by `random.random`, `random.choice`, `Math.random`, `rand()`, or any seeded RNG. Use the cryptographic source: `secrets.token_urlsafe(32)` (Python), `crypto.randomBytes(32)` (Node), `SecureRandom` (Java). When you find a non-crypto RNG generating a security-sensitive value, surface it — this is one of the easiest "looks fine, isn't" traps to miss. +13. **Hand-rolled crypto is almost always wrong.** The trap shape: hand-coded AES, "lightweight" XOR ciphers, hardcoded IVs, reused nonces, hand-generated key material. Use the language's standard library or a well-audited library (libsodium, AWS KMS, OpenSSL bindings). Hardcoded IVs and reused nonces under AES-GCM are catastrophic; the library can prevent both. ### Authentication & authorization @@ -101,9 +102,9 @@ These thoughts mean STOP — apply the domain check before committing: | "Let me fetch this URL the user provided — it's just a webhook test." | SSRF into the metadata service is one URL away. Allowlist host/scheme; reject private ranges. (97/5) | | "`pickle.loads` is convenient and we trust the source." | Trust drifts. The next caller of this function won't know the contract. Use a safe loader for any cross-trust data. (97/6) | | "I'll log the request object so we can debug auth issues." | Request bodies and headers contain passwords and bearer tokens. Mask in middleware before logging. (97/8) | -| "Just put the API key in the config file for now — we'll move it before launch." | Once it's in git, it's leaked. Rotate it the moment it's pushed. Never commit secrets. (97/9) | -| "SHA-256 of password+salt is hashed, so it's secure." | Password hashing is a *category*, not "any hash function." Use bcrypt/scrypt/argon2id. (97/11) | -| "`Math.random()` for the password reset token is fine — it's random." | It's predictable. Tokens use `secrets.token_urlsafe` / `crypto.randomBytes` / `SecureRandom`. (97/12) | +| "Just put the API key in the config file for now — we'll move it before launch." / *finding a credential-shaped string literal in a file you're editing* | Once it's in git, it's leaked. Rotate the moment it's pushed. Surface and address even in prototypes — this trap has no stage exemption. (97/9) | +| "SHA-256 of password+salt is hashed, so it's secure." / *finding `sha256(password)` in code you're touching* | Password hashing is a *category*, not "any hash function." Use bcrypt/scrypt/argon2id. Surface when found. (97/11) | +| "`Math.random()` for the password reset token is fine — it's random." / *finding `Math.random` or `random.random` generating a token, session ID, nonce, or CSRF value* | It's predictable. Tokens use `secrets.token_urlsafe` / `crypto.randomBytes` / `SecureRandom`. Surface when found. (97/12) | | "I'll add the auth decorator after I get the endpoint working." | "After" is when it ships unauth'd to production. Auth decoration is part of the route definition, not a follow-up. (97/14) | | "`/api/users/` looks the user up by id — the auth check at the door is enough." | That's IDOR. Authorize on the resource, not just the route. (97/15) | | "The client sends `is_admin=true` in the JWT and we trust it." | Trusting client-side state is the canonical privilege-escalation bug. The server re-derives authorization from its own signed session. (97/16) | @@ -114,8 +115,8 @@ For every trust-boundary crossing your change touches, **all** of the following - [ ] **Injection:** every dynamic value is parameterized at the driver level (no f-strings into SQL/LDAP/shell); `shell=False` everywhere except a re-justified exception with shell-quoted values. - [ ] **Untrusted input:** path inputs are realpath-validated against an intended root; URL inputs hit an allowlist or block private/loopback ranges; deserialization uses `safe_load`/JSON for cross-trust data; the validator is at the boundary and downstream code does not re-validate. -- [ ] **Secrets:** logs do not contain `Authorization`, password fields, or tokens (verified by reading the diff and the log middleware); no secret is committed to the repo or baked into an image; client-facing errors are generic. -- [ ] **Crypto:** passwords use a password-hash (bcrypt/scrypt/argon2id) at a documented cost; security tokens use the cryptographic RNG; no hand-rolled crypto, no hardcoded IVs. +- [ ] **Secrets:** logs do not contain `Authorization`, password fields, or tokens (verified by reading the diff and the log middleware); no credential-shaped string literal lives in source files, version control, or built images — including in code adjacent to your edit (surfaced in the hand-off if found, not silently rewritten); client-facing errors are generic. +- [ ] **Crypto:** passwords use a password-hash (bcrypt/scrypt/argon2id) at a documented cost; security tokens use the cryptographic RNG (not `Math.random`/`random.random`); no hand-rolled crypto, no hardcoded IVs. Adjacent code with these traps is surfaced, not silently rewritten. - [ ] **Auth:** every new endpoint's auth posture is inspectable at the route definition; authorization decisions check the resource (not just session validity); no client-side claim is authoritative. If any box that applies to your change is unchecked, you are not done — you are mid-trust-boundary. diff --git a/skills/using-97/SKILL.md b/skills/using-97/SKILL.md index ae8c5a4..6033679 100644 --- a/skills/using-97/SKILL.md +++ b/skills/using-97/SKILL.md @@ -34,7 +34,7 @@ The trigger fires on user words, not on file size or task complexity. If the use 1. Your human partner's instructions (CLAUDE.md, GEMINI.md, AGENTS.md, direct prompts) override everything below. 2. **Process skills (superpowers) run before content skills (97).** `superpowers/test-driven-development` decides *whether* to write a test; `97/testing-discipline` decides *what makes it good*. `superpowers/verification-before-completion` decides *did it work*; `97/pre-commit-self-review` decides *is it well-considered*. 3. **More specific 97 skill > broader 97 skill.** `before-you-refactor` wins over `writing-clean-code` when both could apply. `testing-discipline` wins over `writing-clean-code` for test code. -4. **Before editing a file you haven't read this session, read it first.** No skill load — just the cheap reminder. Editing without reading is the most common avoidable failure mode. +4. **Before editing a file you haven't read this session, read it first — and as you read, scan for landmines** (hardcoded credentials, string-built SQL/shell, unsafe deserialization on untrusted input, swallowed exceptions). If you find one, surface it in your hand-off; do not silently rewrite outside scope. Editing without reading is the most common avoidable failure mode; reading without scanning is how landmines next to your edit ship under your name. 5. **When debugging, defer to `superpowers/systematic-debugging` if available.** Otherwise fall back to `error-and-correctness-traps` for trap-shaped bugs and `pre-commit-self-review` step 2 (suspect your own code first) for general debugging. 6. **Apply principles silently.** Do not surface source author names, book titles, or principle IDs (e.g. `97/74`, `Fowler/LongMethod`) in user-facing responses. Citations exist for repo provenance, not for user-facing authority. 7. **Match principle weight to stage and stakes.** Production-shaped guidance — resilience patterns (timeouts, circuit breakers, bulkheads), observability instrumentation, deploy hygiene, security boundaries — fires hardest when code is reaching real users in production. In MVPs, prototypes, internal dev tools, debugging endpoints, and one-off scripts, prefer the simplest thing that works. Do not retrofit production discipline onto code whose architecture is not yet settled.