diff --git a/.gitignore b/.gitignore index 540e0e1..9641e9b 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,7 @@ Thumbs.db # Build / Release dist/ relay + +# Unremediated security review reports — private 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 c434d22..e41415b 100644 --- a/checklist-status.json +++ b/checklist-status.json @@ -3,7 +3,7 @@ "label": "Best Practices", "message": "53%", "schema": "aossie-best-practices-v1", - "updated": "2026-08-12", + "updated": "2026-09-23", "met": 26, "total": 49, "percent": 53, diff --git a/skills/security-remediation/SKILL.md b/skills/security-remediation/SKILL.md new file mode 100644 index 0000000..99656b0 --- /dev/null +++ b/skills/security-remediation/SKILL.md @@ -0,0 +1,204 @@ +--- +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@` — never let credentials +reach a report that gets published). 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. If **every** finding from Step 2 now has either a confirmed remediation + or a user-provided non-remediation explanation: + - Create `security-reviews/` at the repo root if it doesn't exist. + - Move (not copy) both `` — from wherever it actually + is, per Step 1, not assumed to be `unremediated-security-reviews/` + — and the new remediations file, into `security-reviews/`. Use + `git mv` if `git status` shows `` already tracked, + otherwise plain `mv`. + - Confirm neither file still exists at its original location + afterward. +3. If any finding still lacks a resolution (the user wasn't ready to + explain it yet, or remediation is still in progress): + - Save the remediations file next to `` instead (do not + publish either file). + - Clearly list which finding(s) are still blocking publication. + +## 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..1b51a27 --- /dev/null +++ b/skills/security-review/SKILL.md @@ -0,0 +1,710 @@ +--- +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: injection, auth flaws, cryptography issues, unsafe deserialization, 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 `