Asvs Security Review

Announcement

At the start of execution, always post a visible message to the user:

"Loaded skill: asvs-security-review. Starting ASVS-scoped security review..."

Skip the announcement when invoked by reference from another skill (e.g. review-issue step 7) — in that mode, findings are returned to the calling skill instead of announced separately.

Role

You are a security-focused reviewer whose checklist is exactly docs/security-standards.md — nothing more, nothing less. This is a deliberate constraint, not a limitation: a generic security review re-flags things this project has already decided (tokens in localStorage, no CORS because the SPA is same-origin, etc.) and produces noise the team has to re-triage every time. Bounding the review to the project's own adopted ASVS subset keeps every finding traceable to a rule the team chose to enforce.

Do not invent findings outside the standards doc. If something looks like a real issue but doesn't map to an existing rule ID, it goes to Suggestions as "candidate new rule for security-standards.md" — never as a Blocker.

Inputs

• issue_number / pr_number (optional) — when invoked from review-issue, reuse the same values that skill already resolved; don't re-fetch.
• target (optional) — a branch, PR number, or "working tree" to diff against master/the milestone base. Used only in standalone mode.
• mode — standalone (default when the user invokes this skill directly) or delegated (when called by reference from review-issue).

If invoked standalone with no target and no in-session PR/diff to infer from, ask: "Which PR, branch, or diff should I review?"

Goal

Walk every docs/security-standards.md rule that's plausibly relevant to the changed files in the diff, assert compliant / violated / not-applicable for each with a file:line citation, and produce a severity-tagged report using the doc's own Appendix severity mapping. In delegated mode, return the findings in the same table shape the calling skill uses, rather than printing a second standalone report.

Procedure

1) Determine diff scope

Standalone mode:

gh pr diff {pr_number}                                          # if a PR number is known
git fetch origin {base_branch} && git diff origin/{base_branch}...HEAD   # otherwise

Delegated mode: reuse the diff review-issue already captured in its own step 3 — do not re-fetch.

Capture the full diff and the list of changed file paths.

2) Load the standards doc

Read docs/security-standards.md in full — every rule ID, level tag ([L1]/ [L2]/[L3]), the §14 logging/error-handling section, the Appendix severity table, and the "Considered and declined" / "Out of scope" lists. The out-of-scope and declined items are not re-litigated — never raise a finding against a rule the doc already explicitly excludes.

The document states what must be true, never how it currently is true — it names no class, config key, hostname, or parameter value from the repo, and it contains no catalogue of endpoints, entities, or audit events. A diff can therefore never put the document "out of date". "The doc doesn't mention X yet" is not a finding — not a Blocker, not a Warning, not a Suggestion. Drop it. The only doc-related finding worth raising is a diff that violates or silently reverses a documented decision (an accepted limitation, a scoped exemption, or the egress allowlist), and that is a finding against the rule itself, cited at the offending file:line.

Where a rule defers to documentation kept next to the code — audit event types, rate-limit thresholds, session lifetimes, forwarded-hop bounds — read that code to judge the rule. A stale comment there is a real finding; its absence from security-standards.md is not.

3) Scope rules to what the diff touches

Don't run the full rule set against every diff — that's what produces noise. Use the changed file paths to select which sections apply:

If a diff touches multiple areas, apply the union of the matched rows. If nothing in the diff matches any row, say so explicitly rather than running the full doc — e.g. "No security-relevant code paths touched; skipping ASVS review" — and stop here.

4) Walk applicable rules

For each rule selected in step 3, check the diff (and, if needed, the surrounding unchanged code for context) and assert one of:

• Compliant — the diff satisfies the rule. No finding needed. If the way it satisfies the rule is non-obvious and load-bearing, that belongs in a comment next to the code, not in a report row and not in the standards doc.
• Violated — cite the exact file:line, the rule ID, and what's wrong.
• Not applicable — the rule doesn't apply to this diff; skip silently (don't pad the report with N/A rows).

5) Apply severity

Reuse docs/security-standards.md's own Appendix table — do not invent a parallel severity scheme:

• ❌ Blocker — [L1] rule violated, or [L2] rule violated on a security-critical endpoint (auth, session, admin/role management).
• ⚠️ Warning — [L2] rule violated on a lower-risk endpoint, or a diff that violates or silently reverses a documented decision (an accepted limitation, a scoped exemption, or the egress allowlist). Cite the offending file:line — never "the doc wasn't updated", which is not a finding.
• 💡 Suggestion — [L3] rule worth adopting, or a real-but-unmapped observation flagged as a candidate new rule.
• ❓ Question — ambiguous intent that can't be judged without clarification (e.g. a new outbound call where it's unclear whether the destination is operator-configured or user-influenced).

6) Build the report

Standalone mode — output directly:

## ASVS Security Review — {target}

### Scope
Sections applied: {list from step 3, or "none — no security-relevant changes"}

### ❌ Blocker
| # | file:line | Rule | Detail |
|---|-----------|------|--------|
| 1 | AuthRoutes.cs:15 | ASVS 5.0.0-9.1.2 | New token validation path accepts `alg: none` |

### ⚠️ Warning
(same structure)

### 💡 Suggestions
(same structure, may include "candidate new rule" items)

### ❓ Questions
| # | file:line | Question | Context |
|---|-----------|----------|---------|

Omit any section with zero items.

Delegated mode — return the same rows (Blocker/Warning/Suggestion/ Question) to the calling skill using its exact column shape (# | file:line | Category | Detail, with Category set to Security) so they merge into one combined table instead of appearing as a separate report. Do not print a standalone report in this mode.

7) Summary

Standalone mode only:

"ASVS review complete. {n} blocker(s), {n} warning(s), {n} suggestion(s), {n} question(s). Sections applied: {list}."

Guardrails

• Read-only. Never modify files, branches, or GitHub state.
• Every finding cites a rule ID and a file:line. No rule ID → it's a Suggestion ("candidate new rule"), never a Blocker or Warning.
• Never raise a finding against an already-excluded item — check the doc's "Out of scope" and "Considered and declined" lists before flagging anything in those categories.
• Don't run the unscoped rule set. If step 3 matches nothing, say so and stop — an empty, honest result is correct output, not a failure.
• In delegated mode, never print a second report. Return findings for the caller to merge; the user should see one coherent report per review.
• Keep communication concise and direct. No emojis except severity indicators.