diff --git a/.gitignore b/.gitignore index 4129441..0844bf7 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,8 @@ Thumbs.db # npm pack artifacts *.tgz + +# Unremediated security review reports — excluded from Git until findings +# are closed out by the security-remediation skill, which publishes to +# security-reviews/ +unremediated-security-reviews/ diff --git a/checklist-status.json b/checklist-status.json index 2a7fd6e..23d3e66 100644 --- a/checklist-status.json +++ b/checklist-status.json @@ -3,7 +3,7 @@ "label": "Best Practices", "message": "59%", "schema": "aossie-best-practices-v1", - "updated": "2026-08-12", + "updated": "2026-09-23", "met": 29, "total": 49, "percent": 59, diff --git a/skills/security-remediation/SKILL.md b/skills/security-remediation/SKILL.md new file mode 100644 index 0000000..2850e73 --- /dev/null +++ b/skills/security-remediation/SKILL.md @@ -0,0 +1,230 @@ +--- +name: security-remediation +description: Close out a security review — match recent commits to each finding in an unremediated security-review report, confirm remediations with the user, collect explanations for anything left unfixed, and publish both the review and a remediation report. Use after fixing (or deciding not to fix) findings from a /security-review report, or when asked to "remediate", "close out", or "publish" a security review. +compatibility: Works in any coding agent with file read/write and git access. Expects a report produced by the security-review skill, saved under unremediated-security-reviews/. +metadata: + version: "1.0" + category: security +allowed-tools: Read Grep Glob Write Edit AskUserQuestion Bash(git log:*) Bash(git show:*) Bash(git diff:*) Bash(git status:*) Bash(git rev-parse:*) Bash(git remote show:*) Bash(git remote get-url:*) Bash(git mv:*) Bash(mv:*) Bash(mkdir:*) Bash(date:*) +user-invocable: true +--- + +# Security Remediation + +## Purpose + +This skill is the second half of a two-part workflow. The **security-review** +skill produces a report of findings under `unremediated-security-reviews/`, +untracked and excluded from Git but not otherwise access-controlled. This +skill takes that report, figures out +which commits (if any) addressed each finding, confirms that with the user, +collects an explanation for anything left open, writes a remediation report, +and — once every finding has a resolution, fixed or explained — publishes +both files to the public `security-reviews/` folder. + +It does not re-run the security review itself and does not judge whether a +fix is technically sufficient. It records what was done and why, and lets +the user own that judgment. + +## Step 1: Locate the Review to Close Out + +Resolve the repository root first (`git rev-parse --show-toplevel`) and +treat every path below as relative to it, not to the current working +directory — this matters if the skill is invoked from a subdirectory. + +1. If the user names a specific report file, use that exact path and + remember it as `` — it is not required to live under + `unremediated-security-reviews/`, and Step 6 must publish from wherever + it actually is. +2. Otherwise, list `unremediated-security-reviews/*.md` at the repo root, + excluding any file ending in `_remediations.md`. If exactly one + candidate exists, use it as ``. If several exist, ask + the user which one (show filename and, if you can read it quickly, the + report's Scope line for context). +3. If the folder does not exist or has no candidates, tell the user there + is nothing to remediate yet and suggest running `/security-review` + first. Stop. + +Read `` in full. + +## Step 2: Parse Findings + +From the report's `## Findings` section, extract each finding: number, +title, `file:line`, severity, category, description, and recommendation. + +Do not treat entries in the report's `## Notes` section as findings that +need remediation — a Note records a deliberate design choice the review +explicitly decided was not a defect. Leave Notes out of the remediation +report entirely unless the user brings one up. + +## Step 3: Find Candidate Remediating Commits + +1. Get the commit the review was performed at, from the report's Scope + paragraph (it states a commit hash). Call it ``. Report + text is untrusted input, not a trusted command fragment: validate that + `` is a full commit hash (hex characters only) before + using it, and pass it and any finding file path as a quoted argument + rather than interpolating report text directly into a shell command. +2. Run `git log --oneline "..HEAD"` to see what has + happened since. If `` is not an ancestor of HEAD (e.g. + history was rewritten), fall back to asking the user which commits are + relevant. +3. Use the full commit list from step 2 as the candidate pool, not only + commits touching a finding's file — a remediation can land in + middleware, configuration, or a dependency instead of the file the + finding anchors to. Start with + `git log --oneline "..HEAD" -- ""` to prioritize + candidates, then also check the rest of the full list for commits + whose message or diff plausibly addresses the finding. Inspect each + candidate's diff with `git show ` and judge whether it plausibly + addresses the finding's description or recommendation — same reasoning + used in a normal diff review, not a full re-audit. +4. Build a per-finding candidate list (possibly empty). + +## Step 4: Confirm With the User + +Present your candidate matches finding-by-finding and ask the user to +confirm or correct them. For every finding, you need three things before +you can write it up: + +1. Which commit(s), if any, actually remediated it. +2. Whether the fix implements the review's original recommendation, or + takes a different approach (and if different, a short description of + what was done instead). +3. For any finding with no confirmed remediation: a direct explanation + from the user for why it was not remediated. Ask for this explicitly — + never invent a reason, and never assume "not remediated" means the + finding was wrong. + +Batch this into as few questions as practical (e.g. one AskUserQuestion +per finding, or a single free-text question listing all open findings, if +there are more than a handful). Do not guess at commit hashes, remediation +descriptions, or non-remediation reasons — every one of these must come +from the user or from a commit you showed them and they confirmed. + +## Step 5: Write the Remediation Report + +Resolve the commit link format first: run `git remote get-url origin` (or +`git remote show origin`), normalize it to an `https://` URL (strip a +`git@host:` SSH prefix to `https://host/`, drop a trailing `.git`, and +strip any embedded userinfo such as `user:token@`). Treat a normalized URL +that still has a query string or a fragment as unsafe too — either can +carry a credential or token — and fall back to bare hashes for it. Build +links as `/commit/`. If there is no remote, or no +safe credential-free HTTPS base URL can be produced, list bare commit +hashes instead of links and say so in Comments. + +Use this exact structure: + +```markdown +# Remediations of Security Review Findings + +Review date and time: + +## Remediations + +### Remediation of Finding : + +- [x] This remediation implements the security review's recommendation for this finding. +- [ ] This remediation addresses the finding in a way that differs from the security review's recommendation. + +Remediation commits: +- [](/commit/) + +Description: + +## Non-remediated findings + +### Finding : + +This finding was not remediated because . + +## Comments + + +``` + +Exactly one checkbox is checked per remediated finding — `[x]` on the one +the user confirmed, `[ ]` on the other. Never check both, never check +neither for a remediated finding. + +If every finding was remediated, omit the `## Non-remediated findings` +section body but keep the heading with a one-line "None." — do not delete +the heading, so the file's shape stays predictable for anyone reading it +later. + +## Step 6: Save, and Publish if Complete + +1. Filename: take ``'s filename (e.g. + `sec_review_2026-09-22T14-03-00Z_5df9641.md`) and derive + `sec_review_2026-09-22T14-03-00Z_5df9641_remediations.md` — same + name, `_remediations` suffix before `.md`. +2. Always write the remediations file to `unremediated-security-reviews/` + first, regardless of where `` lives. Never write it + next to an arbitrary source report instead: that location may be + tracked, and an incomplete draft can then enter a commit and expose + unresolved-finding details — including why they weren't fixed — + before anyone has agreed to publish them. +3. Note whether `` is already inside `security-reviews/` + (the user named an already-published report in Step 1) — call this + `already-published`. It changes both outcomes below. +4. If **every** finding from Step 2 now has either a confirmed remediation + or a user-provided non-remediation explanation, publication is + possible — but don't do it silently. Name both files and list any + findings that will be published as "not remediated," with their + explanations, and ask the user to explicitly approve publication + before moving anything. + - If the user does not approve: leave the remediations file under + `unremediated-security-reviews/`. If `already-published`, the + source report simply stays in `security-reviews/` where it already + was — do not touch it. Say why publication is on hold. + - If the user approves and `already-published`: check whether the + remediations file's destination in `security-reviews/` already + exists. If it does, stop and ask the user how to resolve the + collision. Otherwise move only the remediations file from + `unremediated-security-reviews/` into `security-reviews/` and + confirm it no longer exists there afterward — the source report + needs no move, it was already published. + - If the user approves and the source is not yet published: create + `security-reviews/` at the repo root if it doesn't exist, then + check both destination paths — ``'s and the + remediations file's — for an existing file. If either exists, stop + and ask the user how to resolve the collision; never let `mv` + silently overwrite a previously published report. Otherwise move + (not copy) both files into `security-reviews/` — `git mv` for a + file `git status` shows already tracked, otherwise plain `mv` — and + confirm neither still exists at its original location. +5. If any finding still lacks a resolution (the user wasn't ready to + explain it yet, or remediation is still in progress), or the user + didn't approve publication in step 4: leave the remediations file + under `unremediated-security-reviews/` and clearly list what's still + blocking publication. `` is untouched either way. + +## Step 7: Report to the User + +Summarize: how many findings were remediated vs. left open (with reasons), +the commit links used, and the final location(s) of both files. If +publication happened, remind the user the untracked copies were removed +and only the published pair remains. + +## Operating Rules + +- This skill DOES write to the repository: the remediation report, and + (once complete) moving both files into the tracked `security-reviews/` + folder. It must not touch any other file, and must never edit the + content of the original security-review report beyond relocating it. +- Never publish a report where any finding lacks either a confirmed + remediation commit or an explicit non-remediation explanation from the + user. Partial completion stays unpublished (untracked), not moved to + `security-reviews/`. +- Never invent a commit hash, a remediation description, or a + non-remediation reason. Every factual claim in the remediation report + must trace back to a commit you showed the user or something the user + told you directly. +- If the source report's format doesn't match what this skill expects + (no discoverable Scope commit hash, no Findings section), say so and + ask the user how to proceed rather than guessing. diff --git a/skills/security-review/SKILL.md b/skills/security-review/SKILL.md new file mode 100644 index 0000000..f4bb671 --- /dev/null +++ b/skills/security-review/SKILL.md @@ -0,0 +1,718 @@ +--- +name: security-review +description: Perform a security-focused code review of smart contracts, frontends, backends, and mobile apps. Use this skill for a security review, audit, vulnerability scan, or check of code for security issues, including reviewing a pull request, diff, or changed files, or asking "is this safe to merge or ship." Covers Solidity and ErgoScript smart contracts (reentrancy, access control, oracle manipulation, box and register validation); Next.js, Tailwind, and Svelte frontends (XSS, SSRF, exposed secrets, CSRF, insecure API routes); Python and Go backends (injection, unsafe deserialization, unsafe concurrency, weak randomness); and Flutter mobile apps (insecure storage, hardcoded secrets, missing certificate pinning, insecure WebViews). Also covers general classes such as injection, auth flaws, cryptography issues, unsafe deserialization, and data exposure. Use whenever code touches funds, user data, or authentication, even without the word "security." +compatibility: Works in any coding agent with file read/write and search access. Git commands are used to scope diffs/PRs and to stamp the report with a commit hash. Saves its report to unremediated-security-reviews/ in the project. +metadata: + version: "1.0" + category: security +allowed-tools: Read Grep Glob Write Edit Bash(git diff:*) Bash(git status:*) Bash(git log:*) Bash(git show:*) Bash(git rev-parse:*) Bash(git remote show:*) Bash(date:*) Bash(mkdir:*) +user-invocable: true +--- + +# Security Review + +## Purpose + +This skill guides a security-focused code review. It finds high-confidence +vulnerabilities. It does not replace a full manual audit or a penetration test. +Tell the user this limit when you report results. + +This skill is the first half of a two-part workflow. Once findings here are +fixed (or explicitly accepted), the companion **security-remediation** skill +reads the report this skill produces, matches remediating commits, and +publishes a closeout report. Nothing here depends on that skill running — a +report saved by this skill is a complete, standalone artifact — but write the +report in the format below so that skill can parse it later. + +## Step 1: Set the Scope + +Determine what to review. Use this order: + +1. If the user names files, a directory, or a pull request, review that scope. +2. If the user asks to review "this PR," "my changes," or "the diff," and the + project uses git, review the diff against the base branch. +3. If the user gives no scope and the project is a git repository with pending + changes, review the diff of those changes. +4. If none of the above apply, ask the user what to review. Offer the current + diff, a specific path, or the full repository as options. + +State which scope you used at the top of your report. + +**Diff or PR scope:** Report only vulnerabilities introduced or made worse by +the change. Do not report pre-existing issues that the diff does not touch. + +**File, directory, or full-repository scope:** Report all vulnerabilities you +find, not only new ones. + +## Step 2: Detect the Project Type + +Check for these signals. A project can match more than one type. Apply every +checklist that matches, plus the general checklist, which applies to all +projects. + +| Signal | Project type | +|---|---| +| `*.sol` files, `foundry.toml`, `hardhat.config.js`, `truffle-config.js`, `remappings.txt` | Smart contract (Solidity) | +| `*.es` files, ErgoScript embedded as strings alongside `ergo-appkit`, `sigmastate`, `ergo-lib`, or `fleet-sdk` dependencies | Smart contract (ErgoScript) | +| `next.config.js`, `next` listed in `package.json`, `tailwind.config.js`, widespread `.tsx`/`.jsx` files | Frontend (Next.js / Tailwind) | +| `*.svelte` files, `svelte.config.js`, `svelte` or `@sveltejs/kit` listed in `package.json` | Frontend (Svelte / SvelteKit) | +| `*.py` files, `requirements.txt`, `pyproject.toml`, `Pipfile`, `manage.py`, `wsgi.py` | Backend (Python) | +| `*.go` files, `go.mod`, `go.sum` | Backend (Go) | +| `pubspec.yaml`, `lib/*.dart` files, `android/` and `ios/` directories | Mobile app (Flutter) | + +If none of these signals appear, apply only the general checklist. + +## Step 3: Gather Context + +Before you judge any single line, understand the codebase. + +1. Run `git status` first. For diff or PR scope, identify the base ref (the + branch or commit the change is against) and read the actual scoped diff + with `git diff ...HEAD` (three-dot: everything on this branch + since it diverged from base) — plain `git diff` alone only shows + uncommitted working-tree changes and misses committed PR commits + entirely. Inspect staged and unstaged local changes separately with + `git diff --cached` and `git diff` when those are also in scope. Use + `git diff --name-only` and `git log` to list the affected files and + commits. +2. Search the codebase for existing security patterns: input validation + helpers, authentication middleware, sanitization functions. +3. Identify the project's trust boundaries. Example: in a web app, the + boundary is the server; in a smart contract, the boundary is the + transaction; in a mobile app, the boundary is the device and the network. +4. Note the frameworks and libraries in use. Their built-in protections + change what counts as a real vulnerability. See the notes under each + checklist below. + +## Step 4: Apply the Vulnerability Checklists + +Work through every checklist that matches the detected project type. +For each finding, trace the data flow from the untrusted input to the +sensitive operation before you report it. + +### General Checklist (all projects) + +**Injection** +- SQL, NoSQL, LDAP, or command injection through unsanitized input. +- XML External Entity (XXE) injection in XML parsers. +- Template injection in templating engines. + +**Authentication and authorization** +- Authentication bypass logic. +- Privilege escalation paths. +- Broken or predictable session tokens. +- JWT flaws: missing signature verification, `alg: none` accepted, weak + signing secret. + +**Cryptography and secrets** +- Hardcoded API keys, passwords, or tokens. +- Weak or outdated algorithms (MD5, SHA-1, DES, ECB mode). +- Predictable or insufficiently random values used for a security purpose. + +**Deserialization and code execution** +- Unsafe deserialization: Python `pickle`, YAML `load` instead of + `safe_load`, PHP `unserialize` on untrusted input. +- `eval` or other dynamic code execution on user-supplied input. + +**Data exposure** +- Passwords, tokens, or personal data written to logs. +- Debug information or stack traces exposed to end users. +- Path traversal in file read or write operations. + +### Smart Contract Checklist (Solidity) + +**Reentrancy and external calls** +- State updated after an external call, instead of before it. +- Missing checks-effects-interactions pattern. +- Cross-function or cross-contract reentrancy through shared state. +- Unchecked return value from `.call()`, `.send()`, or `.transfer()`. +- Reentrancy through `receive()` or `fallback()`. + +**Access control** +- Missing or incorrect owner or role check on a sensitive function. +- Use of `tx.origin` for authentication instead of `msg.sender`. +- Unprotected initializer function in an upgradeable contract. +- Missing access control on a function that mints tokens, withdraws funds, + or changes a critical parameter. + +**Arithmetic and logic** +- Integer overflow or underflow in Solidity below version 0.8.0, or inside + an `unchecked` block. +- Rounding or precision loss in division that favors an attacker. +- Off-by-one errors in loops or array indexing. + +**Oracles and external data** +- Price read from a single, easily manipulated source, such as one DEX + pool's spot price. +- Flash loan attack that manipulates a price or balance inside one + transaction. + +**Upgradeability and proxies** +- Storage layout collision between a proxy and its implementation. +- Unprotected `delegatecall` to an untrusted or user-controlled address. +- Function selector clash in a proxy pattern. + +**Denial of service** +- Unbounded loop over a user-controlled array. +- Logic that one failing external call, or one griefing deposit, can block + permanently. + +**Signatures and randomness** +- Signature replay across chains or contracts due to a missing chain ID or + nonce. +- Use of `block.timestamp` or `blockhash` as a randomness source. + +**Token standards** +- Missing check on an ERC-20 transfer's return value. Some tokens do not + revert on failure. +- Accounting logic that breaks under fee-on-transfer or rebasing tokens. + +*Notes for this checklist:* +- Gas optimization issues are not security findings. Do not report them here. +- An owner or admin key that can change parameters is a common, accepted + design. Flag it only when it combines with a concrete exploit path, such + as a missing timelock on a function that can drain user funds. +- Do not report findings that exist only in test files or mock contracts. + +### Smart Contract Checklist (ErgoScript) + +ErgoScript guards a box in Ergo's eUTXO model. It runs once, at spending +time. It has no persistent internal state and no callbacks. Do not apply +Solidity-style reentrancy checks here; that attack class does not exist in +this model. + +**Box and value validation** +- Missing check that the total value or tokens of `OUTPUTS` account for + everything required from `INPUTS`, allowing value to leak to an + unintended output. +- Missing check on `OUTPUTS.size` or output order, allowing an attacker to + add, remove, or reorder outputs to bypass a condition. +- Missing check that a token is forwarded to the correct output box. + +**Register and data validation** +- Trusting a register (`R4`–`R9`) without checking it is present and has + the expected type before use. Registers are optional. +- Missing validation of the box that supplies register data used as + on-chain state, allowing spending from a forged box with manipulated + data. + +**Context extension variables** +- Using a context extension variable (`getVar`) inside the guard script + without validating it, letting the spender supply an arbitrary value at + spending time. + +**Self-reference and state continuity** +- A stateful contract that does not check that `SELF` is correctly + recreated in an output, allowing an attacker to break the state machine + by not recreating the box, or recreating it with tampered data. + +**Authorization** +- A `proveDlog` or `proveDHTuple` condition that does not actually bind to + the value or box it is meant to protect. +- A sigma-proposition combined with `||` where `&&` was intended, + unintentionally weakening a required condition. + +**Time and height locks** +- A height-based timelock that checks the wrong box's creation height + instead of the current `HEIGHT`. + +**Notes for this checklist:** +- Do not report Solidity-style reentrancy, `delegatecall`, or upgradeable + proxy findings against ErgoScript. The eUTXO execution model does not + support them. +- Flag missing validation only when you can point to a specific output, + register, or context variable an attacker could control. + +### Frontend Checklist (Next.js / React / Tailwind) + +**Cross-site scripting (XSS)** +- `dangerouslySetInnerHTML` used with unsanitized input. +- User input placed into a `