From 547cc04f142a2efd91caefe4e85ec7eeed34f668 Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Thu, 1 Oct 2026 13:00:20 +0530 Subject: [PATCH 1/2] Address follow-up CodeRabbit feedback on the two skills - security-remediation Step 6: always draft the remediations file in unremediated-security-reviews/ rather than beside an arbitrary source report (a tracked location could commit unresolved-finding details before anyone agreed to publish them); require explicit user approval before actually publishing; skip the move when the source report is already in security-reviews/; stop and ask instead of letting `mv` silently overwrite a destination collision - security-remediation Step 5: also reject commit-link URLs that still carry a query string or fragment after stripping userinfo, since either can carry a credential too - security-review: make the report filename collision check real (append _2, _3, ... instead of just lowering the odds with a hash suffix), and broaden the DoS exclusion so it doesn't accidentally swallow the Go checklist's own "report severe goroutine-exhaustion" carve-out - .gitignore: describe the ignored folder as "excluded from Git," not "private" (an exclusion isn't an access control) Co-Authored-By: Claude Sonnet 5 --- .gitignore | 5 ++- skills/security-remediation/SKILL.md | 55 +++++++++++++++++++--------- skills/security-review/SKILL.md | 11 ++++-- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index 9641e9b..bd41d1f 100644 --- a/.gitignore +++ b/.gitignore @@ -33,6 +33,7 @@ Thumbs.db dist/ relay -# Unremediated security review reports — private until findings are closed -# out by the security-remediation skill, which publishes to security-reviews/ +# 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/skills/security-remediation/SKILL.md b/skills/security-remediation/SKILL.md index 99656b0..1a536cd 100644 --- a/skills/security-remediation/SKILL.md +++ b/skills/security-remediation/SKILL.md @@ -107,11 +107,12 @@ from the user or from a commit you showed them and they confirmed. 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. +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: @@ -162,21 +163,41 @@ later. `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, +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. 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 both files under + `unremediated-security-reviews/` and say why publication is on + hold. + - On approval: create `security-reviews/` at the repo root if it + doesn't exist. If `` is already inside + `security-reviews/` (the user named an already-published report), + it needs no move — just write the remediations file there directly + and skip the rest of this step. + - Otherwise, for each of the two files, check whether its destination + path inside `security-reviews/` already exists. If either does, + stop and ask the user how to resolve the collision — never let + `mv` silently overwrite a previously published report. + - Move (not copy) `` — from wherever it actually is, + per Step 1 — and the remediations file into `security-reviews/`. + Use `git mv` for a file `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. +4. 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 3: leave the remediations file + under `unremediated-security-reviews/` and clearly list what's still + blocking publication. ## Step 7: Report to the User diff --git a/skills/security-review/SKILL.md b/skills/security-review/SKILL.md index 1b51a27..c5b7fd2 100644 --- a/skills/security-review/SKILL.md +++ b/skills/security-review/SKILL.md @@ -459,7 +459,9 @@ scope — those get no mention in the report at all. **General exclusions** - Denial of service from resource exhaustion or rate limiting, unless it - can permanently lock funds or permanently disable a contract. + can permanently lock funds, permanently disable a contract, or cause a + severe, unauthenticated-triggerable backend outage (see the Go + checklist's goroutine-exhaustion note for what counts as severe here). - A secret stored on disk that is already protected by OS file permissions or a secrets manager. - A missing best practice with no concrete exploit path. Code does not need @@ -670,8 +672,11 @@ keeps unfixed findings out of the public repository in the meantime. report's metadata text — only the filename is sanitized. 3. The filename is `sec_review__.md`, where `` is the first 7 characters of the commit hash - from the Scope section (or `nogit` if none is available) — this keeps - two reviews started in the same second from overwriting each other. + from the Scope section (or `nogit` if none is available). Before + writing, check whether a file already exists at that exact path (e.g. + two reviews of the same commit started in the same second) — if it + does, append `_2`, `_3`, etc. before `.md` until the path is free. + Never overwrite an existing report. 4. The report is saved to `unremediated-security-reviews/` at the repository root. Create the directory if it does not exist. 5. Before writing the report, check whether the project has a `.gitignore` From e726d98c49497823d2fd43e43b17d2d1f7fd2b4a Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Thu, 1 Oct 2026 13:09:51 +0530 Subject: [PATCH 2/2] Fix two more CodeRabbit findings on the follow-up PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - security-remediation Step 6: handle an already-published source report correctly in BOTH outcomes, not just the approval path — on decline, leave it in security-reviews/ untouched rather than claiming it's under unremediated-security-reviews/; on approval, check the remediation file's destination for a collision before writing it there directly - security-review: break the circular "see Step 5" / "see the Go checklist" cross-reference for DoS severity by stating a concrete impact criterion once, in the General exclusions, and having the Go checklist point to that single definition Co-Authored-By: Claude Sonnet 5 --- skills/security-remediation/SKILL.md | 57 +++++++++++++++------------- skills/security-review/SKILL.md | 9 +++-- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/skills/security-remediation/SKILL.md b/skills/security-remediation/SKILL.md index 1a536cd..2850e73 100644 --- a/skills/security-remediation/SKILL.md +++ b/skills/security-remediation/SKILL.md @@ -169,35 +169,40 @@ later. 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. If **every** finding from Step 2 now has either a confirmed remediation +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 both files under - `unremediated-security-reviews/` and say why publication is on - hold. - - On approval: create `security-reviews/` at the repo root if it - doesn't exist. If `` is already inside - `security-reviews/` (the user named an already-published report), - it needs no move — just write the remediations file there directly - and skip the rest of this step. - - Otherwise, for each of the two files, check whether its destination - path inside `security-reviews/` already exists. If either does, - stop and ask the user how to resolve the collision — never let - `mv` silently overwrite a previously published report. - - Move (not copy) `` — from wherever it actually is, - per Step 1 — and the remediations file into `security-reviews/`. - Use `git mv` for a file `git status` shows already tracked, - otherwise plain `mv`. - - Confirm neither file still exists at its original location - afterward. -4. If any finding still lacks a resolution (the user wasn't ready to + 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 3: leave the remediations file + didn't approve publication in step 4: leave the remediations file under `unremediated-security-reviews/` and clearly list what's still - blocking publication. + blocking publication. `` is untouched either way. ## Step 7: Report to the User diff --git a/skills/security-review/SKILL.md b/skills/security-review/SKILL.md index c5b7fd2..c1195b6 100644 --- a/skills/security-review/SKILL.md +++ b/skills/security-review/SKILL.md @@ -404,7 +404,8 @@ this model. an authorization check), when the state is shared across goroutines without a mutex or channel. - Unbounded goroutine creation driven by unauthenticated input. Report - only when the impact is severe; see the exclusions in Step 5. + only when it meets the severe-impact bar defined in the General + exclusions (Step 5) — not routine resource exhaustion below that bar. **Error handling** - An ignored error return value on a security-relevant operation, such as @@ -460,8 +461,10 @@ scope — those get no mention in the report at all. **General exclusions** - Denial of service from resource exhaustion or rate limiting, unless it can permanently lock funds, permanently disable a contract, or cause a - severe, unauthenticated-triggerable backend outage (see the Go - checklist's goroutine-exhaustion note for what counts as severe here). + severe backend outage. "Severe" here means: a single unauthenticated + request, or a small fixed number of them, crashes the process, exhausts + memory, or makes the service unresponsive to other users until it is + manually restarted. - A secret stored on disk that is already protected by OS file permissions or a secrets manager. - A missing best practice with no concrete exploit path. Code does not need