security-review

Generated at build time from skills/security-review/SKILL.md. Do not edit this page directly.

Security Review exists to find meaningful security failures introduced or exposed by a change.

Δ → Graph → Security<P, T, E>
│                │      │  │  │
│                │      │  │  └─ evidence that the property fails or holds (§10)
│                │      │  └──── reachable attacker / trust path          (§4-9)
│                │      └─────── protected asset or security property     (§3)
│                │
│                └─ nodes = actors, boundaries, controls, assets, sinks
│                   edges = authority and untrusted/sensitive data flow
│
└─ changed behavior, configuration, dependency, or exposure

§1  Gate         invoke only when security-relevant behavior changed
§2  Delta        smallest affected security graph
§3  P            protected asset and required security property
§4  Authority    actor → authorization → protected effect
§5  Data         untrusted source → transformation → sensitive sink
§6  Secrets      sensitive-data flow and exposure boundaries
§7  Boundary     external systems, network, files, webhooks, configuration
§8  Supply       dependency and build/release security applicability
§9  Abuse        adversarial availability / resource abuse only when relevant
§10 E            evidence-backed reachable finding
§11 Proof        targeted security verification; test-engineering if complex
§12 Handoff      architecture/control changes go to the owning skill

Start from the change, not a vulnerability catalog.

Ask:

What security property must remain true?
What can an untrusted or less-privileged actor reach now?
What authority can they exercise?
What sensitive data can flow?
What privileged effect can occur?

Then trace the smallest realistic path from attacker capability to protected asset/effect.

If there is no reachable path, broken control, and meaningful impact, there is no security finding.

1. Applicability gate

security-review is a specialist capability, not a mandatory stage.

Invoke it when a change materially touches:

authentication
authorization / RBAC / ABAC
session management
cookies / tokens / JWT
password / credential handling

tenant or user isolation
ownership checks
admin / privileged operations
impersonation / delegated authority

user-controlled input reaching:
- database/query
- HTML/template output
- shell/process execution
- filesystem/path
- URL/network request
- parser/deserializer
- redirect
- dynamic code/config

file upload/download
secrets / key management
cryptography
sensitive/private data
payment or security-sensitive workflows

public API exposure
CORS / CSRF
webhooks
OAuth/OIDC
external integrations
rate / abuse controls

security configuration
IAM / permissions
public/private exposure
dependency changes with meaningful security impact
new trust boundary

Do not invoke deep security review by default for:

formatting
renaming
CSS/layout
documentation
mechanical refactors
trivial type changes
internal mapping code
local changes that do not alter:
- trust
- authority
- exposure
- sensitive data flow
- privileged effects

Normal code-review remains responsible for noticing when a change should escalate.

Canonical trigger:

If a change modifies
what an untrusted actor can reach,
what authority an actor has,
what sensitive data can flow,
or what privileged effect can occur,
security-review becomes relevant.

2. Scope the changed security graph

Default to diff-based review when a change set exists.

Build only the affected graph:

untrusted / less-trusted actor
        ↓
entry point
        ↓
authentication / identity
        ↓
authorization / policy
        ↓
validation / normalization
        ↓
business operation
        ↓
sensitive state / privileged effect / external sink

Also trace configuration or dependency edges when they change effective security behavior.

Inspect surrounding code only far enough to establish:

who controls the input/authority
where trust changes
which control is supposed to stop misuse
which asset/effect is reachable
what actually happens

Do not turn every PR into a whole-application penetration review.

Use broader baseline/threat-based review only when justified:

new application
major security-sensitive redesign
new tenant/isolation model
large trust-boundary change
legacy system onboarding
post-incident investigation
explicit security audit

For baseline work, map the relevant application trust zones, assets, privileged workflows, external boundaries, and existing controls before looking for attack paths.

3. Define P: asset and required security property

Do not begin with vulnerability names.

Begin with:

What are we protecting?
Who is allowed to do what?
What input is untrusted?
What boundary is crossed?
What must never become possible?

Examples:

User A must never read or modify User B's private record.

A normal user must not invoke an administrator-only operation.

An unauthenticated caller must not create privileged state.

Uploaded content must not escape the intended storage boundary.

A user-controlled network destination must not expose internal-only resources.

A webhook replay must not duplicate a privileged effect.

Secrets must not reach client output, logs, traces, source artifacts, or third parties.

A tenant-scoped worker must not operate on another tenant's objects.

Write the property before judging the implementation.

Only inspect controls that trace to a real asset/property/threat.

If the property itself is missing or ambiguous because the architecture does not define the trust boundary, escalate that design gap to engineering-design.

4. Trace authority to the protected effect

Authentication answers:

Who is this actor?

Authorization answers:

May this actor perform this action on this resource?

For every changed privileged operation trace:

actor
  ↓
identity/session
  ↓
requested resource/action
  ↓
authorization decision
  ↓
protected effect

Authorization must hold at the server-side effect boundary.

Do not accept authorization that exists only:

in the UI
in hidden/disabled controls
in client-side state
in routing/navigation
in a previous unrelated request
in a guessed role from client input

Pay special attention when changed behavior touches:

object ownership
IDOR / BOLA-style resource access
tenant isolation
admin/user transitions
bulk operations
indirect references
alternate API paths
background jobs acting for users
impersonation
delegated authority
resource creation followed by mutation

Prefer deny-by-default semantics when access is not explicitly granted.

Check the actual resource/action pair, not merely whether the actor is authenticated.

For multi-step workflows, confirm authorization is preserved across every step that can produce the protected effect.

5. Trace untrusted data to sensitive sinks

Use source → transformation → sink reasoning.

SOURCE
user input
request body/query/header
URL
uploaded file
external API response
webhook
stored untrusted content
environment/config from a less-trusted source

        ↓

TRANSFORMATION
parse
canonicalize
validate
normalize
authorize
encode
map to safe primitive

        ↓

SINK
database/query
HTML/template
filesystem/path
command/process
network request
redirect
log
deserializer
dynamic code/config
privileged state transition

Do not report “missing validation” without explaining the actual path.

For every suspected issue establish:

attacker-controlled value
        ↓
crossed boundary
        ↓
sensitive operation
        ↓
broken property

Reason about the actual library/framework semantics in use.

Do not infer a vulnerability merely because a source and sink both exist.

6. Respect trusted security primitives

Do not ask application code to reimplement protections already correctly provided by established framework/library primitives.

Examples:

parameterized database API
framework output escaping
standard password hashing library
framework CSRF middleware
mature session implementation
well-established cryptographic primitive

Review whether the primitive is:

used correctly
used on every required path
bypassed
disabled
misconfigured
fed invalid assumptions
wrapped in a way that removes its guarantee

Prefer secure defaults over custom hardening steps.

Do not invent custom cryptography, sanitizers, authentication protocols, token formats, or authorization frameworks when a trusted mechanism already satisfies the property.

If cryptography changes materially, verify purpose, primitive/library, key lifecycle, nonce/IV rules, rotation/revocation implications, failure handling, and compatibility. Escalate specialized cryptographic design when needed.

7. Trace secrets and sensitive data

Sensitive data is a flow problem.

Classify only data relevant to the change:

credential
session/token
private key
API secret
personal/private record
payment/security-sensitive data
tenant-confidential data
internal security metadata

Trace:

source
 ↓
processing
 ↓
storage
 ↓
transport
 ↓
telemetry
 ↓
client/third party

Verify it does not unintentionally reach:

source control
client bundle
HTML/client response
logs
traces
metrics labels
error messages
analytics
URLs/query strings
third-party systems
build artifacts
cache keys or debug dumps

Telemetry is another data boundary.

Do not improve diagnosis by logging a secret or sensitive payload that does not belong there.

For stored/transmitted sensitive data, check only the protections required by the product/system property. Do not demand “encrypt everything” by ritual.

8. Review changed external and configuration boundaries

Configuration is part of the security graph when it changes effective authority or exposure.

Review relevant changes to:

CORS
cookies
TLS
authentication mode
session settings
public/private exposure
network access
IAM / permissions
secret injection
debug/development mode
storage permissions
security headers
deployment environment
proxy/trust settings

Ask:

Did the effective trust boundary move?
Did a previously private resource become reachable?
Did authority broaden?
Did a secure default become opt-in or disabled?

Network destinations / SSRF-sensitive flows

When attacker-controlled data influences an outbound request, establish the required destination policy.

Trace:

attacker-controlled destination
        ↓
parse / canonicalize
        ↓
scheme + host + port resolution
        ↓
redirect behavior
        ↓
DNS / network destination
        ↓
request

When only known destinations are valid, prefer explicit allowlisting and server-constructed requests.

When arbitrary public destinations are a product requirement, verify controls that keep internal/private/unsafe destinations out of reach and account for redirect/resolution behavior relevant to the implementation.

Do not label every URL field “SSRF”; prove the server actually performs a reachable request under attacker influence.

Files

When untrusted input affects a path or upload:

original name/input
        ↓
server-side identity/path decision
        ↓
type/size/content checks required by the product
        ↓
storage boundary
        ↓
serving/download boundary

Do not let user-supplied paths define arbitrary filesystem locations.

Review traversal/canonicalization, executable serving, access control, and storage exposure only where relevant to the actual file workflow.

Webhooks / callbacks

Trace:

sender identity/authenticity
        ↓
payload validation
        ↓
replay/duplicate semantics
        ↓
authorization/business rule
        ↓
privileged effect

Do not assume signature verification alone makes a replay-sensitive effect safe.

9. Review dependencies and supply-chain changes by applicability

A scanner finding is a lead, not automatically a security finding.

For a changed dependency ask:

Was the package/version actually added or changed?
        ↓
Does the advisory apply to this version/configuration?
        ↓
Is the vulnerable capability present?
        ↓
Is it reachable in this application/deployment?
        ↓
Can an attacker satisfy the required preconditions?
        ↓
What is the realistic impact?

Prioritize reachable/applicable risk.

Do not block solely because:

a CVE exists
a scanner reports "high"
a transitive dependency is present

without understanding applicability.

Escalate quickly when the change indicates:

known malicious or compromised package
dependency provenance/integrity failure
credential or signing-key exposure
build/release integrity compromise
critical remotely reachable vulnerable functionality
unexpected dependency substitution/typosquatting

Supply-chain protection for build artifacts, provenance, signing, and release integrity belongs primarily to release-engineering; Security Review identifies the security property/risk.

Automated dependency review/SCA is useful for discovering candidate issues. Manual review decides whether a candidate is relevant to the changed application path.

10. Treat adversarial availability separately from capacity

Normal latency, capacity, autoscaling, and organic traffic saturation belong to production-ops.

Security Review owns availability when an attacker can intentionally create disproportionate impact through the changed behavior.

Examples:

unbounded expensive request
credential brute force
unbounded upload
resource exhaustion
fan-out/amplification
algorithmic complexity abuse
missing rate/abuse boundary on privileged or costly operations
replay causing repeated expensive effects

Trace:

attacker cost
        ↓
reachable operation
        ↓
system/resource cost
        ↓
existing bound/control
        ↓
impact

Do not demand rate limiting for every endpoint.

Add an abuse control only when the attacker/resource asymmetry justifies it.

11. Use threat frameworks as challenge libraries, not proof

OWASP Top 10, ASVS, CWE, STRIDE, and similar frameworks can help challenge the affected graph.

Use them after the graph is known:

affected asset/property
        ↓
reachable trust/authority/data path
        ↓
relevant threat categories
        ↓
challenge existing controls

Do not:

walk every ASVS item for a CSS change
run STRIDE on every CRUD patch
report a vulnerability because it matches a CWE name
add controls only to satisfy a checklist

For broad/baseline security review, a structured threat model is appropriate because design-level risks and forgotten attack paths are part of the scope.

For normal PR review, keep threat reasoning on the changed subgraph.

Vulnerability names are classification after understanding, not evidence before understanding.

12. Require evidence for every finding

Every finding must establish:

Location
   ↓
Security property
   ↓
Attacker capability / trust assumption
   ↓
Actual authority/data flow
   ↓
Broken or missing control
   ↓
Reachable failure path
   ↓
Impact
   ↓
Smallest corrective direction

Minimum form:

[Severity] file:line — concise security issue

Property:
<what must remain protected>

Actual:
<what the implementation currently allows>

Attack/failure path:
<minimal realistic reachable path>

Impact:
<unauthorized capability, exposure, integrity loss, or abuse>

Fix:
<smallest change that restores the property>

Do not report a theoretical vulnerability merely because code resembles a dangerous pattern.

If a suspected path is cheap and safe to verify, run the smallest targeted check.

If evidence falsifies the suspicion, remove the finding.

State residual uncertainty explicitly when reachability or deployment context cannot be established.

13. Assign severity from realistic impact and reachability

Use the same review language as code-review:

Critical

A reachable issue can plausibly cause severe impact such as:

broad authentication/authorization bypass
cross-tenant or large-scale sensitive-data exposure
remote code execution in the deployed context
credential/signing-key compromise
destructive integrity failure with large blast radius
build/release integrity compromise

Major

A concrete reachable weakness can cause:

unauthorized access/action
meaningful private-data exposure
privilege escalation with bounded scope
security-sensitive workflow bypass
real injection/path/network boundary violation
replay/abuse with meaningful effect
material security regression

Minor

A real security weakness with limited scope/impact, for example:

narrow information exposure
localized hardening gap with reachable but constrained impact
missing defensive control on a low-risk path
test gap around an already-correct important security property

Severity is not copied blindly from a scanner/CVE.

Consider attacker capability, reachability, preconditions, privilege level, data/effect scope, blast radius, and compensating controls.

14. Design targeted executable proof

Security Review owns:

which security property matters
which attack/trust path threatens it

For obvious verification:

simple policy decision
→ targeted unit test

API authorization / tenant isolation
→ integration test

real parser/validation boundary
→ unit or integration at that boundary

database-enforced isolation
→ real DB integration test

multi-step browser/session security workflow
→ targeted E2E only when lower levels cannot prove it

Examples:

anonymous
→ privileged endpoint
→ denied

User B
→ User A's resource
→ denied

revoked session
→ protected action
→ denied

untrusted path
→ outside allowed root
→ rejected

replayed webhook
→ privileged effect occurs at most once

If verification itself requires non-trivial environment, concurrency, protocol, failure injection, or boundary design:

security-review
→ property + reachable attack path
→ test-engineering

Do not recreate testing architecture here.

Do not create a giant security suite when one faithful regression proof protects the property.

15. Keep ownership boundaries clean

engineering-design

Owns:

trust boundaries
state ownership
system-level security guarantees
architecture

Security Review asks whether the implementation/change violates them.

If the correct fix requires a new trust boundary, different state owner, different isolation model, or new architectural control:

security-review
→ finding/property
→ engineering-design

design-thinking

Owns implementation of security controls inside the software graph.

code-review

Owns general correctness/maintainability review and detects whether deeper security review is warranted.

code-review
→ changed trust/authority/data boundary?
→ security-review

test-engineering

Owns complex executable proof.

security-review
→ property / attack path

test-engineering
→ smallest faithful proof

release-engineering

Owns build/deploy/release mechanics and protections such as artifact/release integrity where those are part of the shipping system.

production-ops

Owns runtime detection, diagnosis, mitigation, and recovery.

Security Review can identify security-relevant telemetry or operational properties, but monitoring/incident mechanics belong to production-ops.

16. Review-only vs remediation

For an explicit request:

"security-review this PR/change"

default:

inspect
→ prove findings
→ report
→ do not modify code

When the parent task explicitly asks to review and fix, or Security Review is an internal stage of implementation:

inspect
→ finding
→ smallest effective control
→ targeted verification
→ re-review affected security graph

Do not broaden remediation into unrelated hardening.

Prefer:

smallest effective control
+
smallest faithful proof
+
clear residual risk

17. Output and persistence

Lead with actionable findings ordered by severity.

Critical
- ...

Major
- ...

Minor
- ...

Each finding must include the security property, reachable path, impact, and corrective direction.

After findings, optionally include:

Verified
- targeted security checks actually performed

Residual risk
- material uncertainty that could not be resolved

If there are no evidence-backed findings:

No actionable security findings.

Do not manufacture security feedback.

Do not create .agents/security.md, SECURITY_REVIEW.md, or threat-model.md by default for every change.

Per-change review evidence belongs in the PR/task/review context.

Durable system security properties belong in .agents/engineering.md.

Consequential security architecture decisions belong in ADRs.

Create a durable threat/security artifact only when a baseline audit, major architecture, compliance requirement, or explicit task actually needs one.

---

Proportionality

Depth follows security risk, not diff size.

mechanical / non-security change
→ no dedicated security-review

normal feature with unchanged trust/exposure
→ code-review security awareness only

auth / authorization / sensitive input/data change
→ focused review of affected security graph

new external boundary / tenant model / privileged workflow
→ deeper review + targeted proof

major security architecture change / incident / explicit audit
→ broader threat-based review

A five-line authorization change can deserve deeper review than a thousand-line internal refactor.

---

Anti-goals

Security Review is not:

OWASP checklist completion
mandatory STRIDE ceremony
"sanitize everything"
"encrypt everything"
one security test per endpoint
one threat model per PR
custom crypto design
custom auth framework design
CVE severity copying
scanner finding = vulnerability
speculative attacker stories
whole-app pentest for every change
security theater

Frameworks and scanners are inputs.

Reachable security properties and evidence determine findings.

---