From 623af9870488fbdf18e8d02cde29d0b4c380814d Mon Sep 17 00:00:00 2001 From: Syams Date: Fri, 18 Sep 2026 20:05:06 +0800 Subject: [PATCH] refactor(security-and-hardening): move code patterns out of SKILL.md into a skill-local reference SKILL.md loaded ~5.3k tokens on every activation, 30% of it inline TypeScript examples that an agent only needs when it reaches that code. docs/skill-anatomy.md already asks for SKILL.md under 500 lines with reference material in supporting files; this skill was at 524. Move the OWASP prevention patterns, input-validation and upload code, SSRF check, destructive-path discussion, dependency-audit decision tree, rate-limiting and secrets snippets, data-classification table, and LLM output example verbatim into references/hardening-patterns.md (same convention as constraint-driven-development/references/floor-guard.md). SKILL.md keeps the workflow: threat model, three-tier boundaries, and a condensed "Hardening Controls" section stating each rule with a deep link to its pattern. Frontmatter, Process, Rationalizations, Red Flags, and Verification are byte-identical to before. The skill's own review checklist duplicated references/security-checklist.md; replace it with a pointer and add its three privacy items to the shared checklist so nothing is lost. SKILL.md: 524 -> 216 lines, 4096 -> 2449 words (~-40% tokens per load). Routing unchanged: run-evals rank-1 stays 100%, no description edits. Co-Authored-By: Claude Opus 5 (1M context) --- references/security-checklist.md | 3 + skills/security-and-hardening/SKILL.md | 492 ++++-------------- .../references/hardening-patterns.md | 324 ++++++++++++ 3 files changed, 419 insertions(+), 400 deletions(-) create mode 100644 skills/security-and-hardening/references/hardening-patterns.md diff --git a/references/security-checklist.md b/references/security-checklist.md index b7df3eb0..fc5e29b9 100644 --- a/references/security-checklist.md +++ b/references/security-checklist.md @@ -138,6 +138,9 @@ cors({ origin: '*' }) // Allows any origin - [ ] PII encrypted at rest (if required by regulation) - [ ] HTTPS for all external communication - [ ] Database backups encrypted +- [ ] Personal data is classified, collected against a stated purpose, and minimized +- [ ] Personal data has a retention limit and a working deletion path (incl. backups, caches, indexes) +- [ ] Export/delete (data-subject) requests are supported where required; third-party sharing has consent and a data-processing agreement ## Dependency Security diff --git a/skills/security-and-hardening/SKILL.md b/skills/security-and-hardening/SKILL.md index c168e65e..a59ca0da 100644 --- a/skills/security-and-hardening/SKILL.md +++ b/skills/security-and-hardening/SKILL.md @@ -72,406 +72,98 @@ If you can't name the trust boundaries for a feature, you're not ready to secure - **Never store sessions in client-accessible storage** (localStorage for auth tokens) - **Never expose stack traces** or internal error details to users -## OWASP Top 10 Prevention Patterns - -These are prevention patterns, not a ranking. For the 2021 ordering, see the quick-reference table in `../../references/security-checklist.md`. - -### Injection (SQL, NoSQL, OS Command) - -```typescript -// BAD: SQL injection via string concatenation -const query = `SELECT * FROM users WHERE id = '${userId}'`; - -// GOOD: Parameterized query -const user = await db.query('SELECT * FROM users WHERE id = $1', [userId]); - -// GOOD: ORM with parameterized input -const user = await prisma.user.findUnique({ where: { id: userId } }); -``` - -### Broken Authentication - -```typescript -// Password hashing -import { hash, compare } from 'bcrypt'; - -const SALT_ROUNDS = 12; -const hashedPassword = await hash(plaintext, SALT_ROUNDS); -const isValid = await compare(plaintext, hashedPassword); - -// Session management -app.use(session({ - secret: process.env.SESSION_SECRET, // From environment, not code - resave: false, - saveUninitialized: false, - cookie: { - httpOnly: true, // Not accessible via JavaScript - secure: true, // HTTPS only - sameSite: 'lax', // CSRF protection - maxAge: 24 * 60 * 60 * 1000, // 24 hours - }, -})); -``` - -### Cross-Site Scripting (XSS) - -```typescript -// BAD: Rendering user input as HTML -element.innerHTML = userInput; - -// GOOD: Use framework auto-escaping (React does this by default) -return
{userInput}
; - -// If you MUST render HTML, sanitize first -import DOMPurify from 'dompurify'; -const clean = DOMPurify.sanitize(userInput); -``` - -### Broken Access Control - -```typescript -// Always check authorization, not just authentication -app.patch('/api/tasks/:id', authenticate, async (req, res) => { - const task = await taskService.findById(req.params.id); - - // Check that the authenticated user owns this resource - if (task.ownerId !== req.user.id) { - return res.status(403).json({ - error: { code: 'FORBIDDEN', message: 'Not authorized to modify this task' } - }); - } - - // Proceed with update - const updated = await taskService.update(req.params.id, req.body); - return res.json(updated); -}); -``` - -### Security Misconfiguration - -```typescript -// Security headers (use helmet for Express) -import helmet from 'helmet'; -app.use(helmet()); - -// Content Security Policy -app.use(helmet.contentSecurityPolicy({ - directives: { - defaultSrc: ["'self'"], - scriptSrc: ["'self'"], - styleSrc: ["'self'", "'unsafe-inline'"], // Tighten if possible - imgSrc: ["'self'", 'data:', 'https:'], - connectSrc: ["'self'"], - }, -})); - -// CORS — restrict to known origins -app.use(cors({ - origin: process.env.ALLOWED_ORIGINS?.split(',') || 'http://localhost:3000', - credentials: true, -})); -``` - -### Sensitive Data Exposure - -```typescript -// Never return sensitive fields in API responses -function sanitizeUser(user: UserRecord): PublicUser { - const { passwordHash, resetToken, ...publicFields } = user; - return publicFields; -} - -// Use environment variables for secrets -const API_KEY = process.env.STRIPE_API_KEY; -if (!API_KEY) throw new Error('STRIPE_API_KEY not configured'); -``` - -### Server-Side Request Forgery (SSRF) - -Any time the server fetches a URL the user influenced — webhooks, "import from URL", image proxies, link previews — an attacker can aim it at internal services (cloud metadata, `localhost`, private IPs). - -```typescript -// BAD: fetch whatever the user gives you -await fetch(req.body.webhookUrl); - -// GOOD: allowlist scheme + host, reject if ANY resolved IP is private, forbid redirects -import { lookup } from 'node:dns/promises'; -import ipaddr from 'ipaddr.js'; - -const ALLOWED_HOSTS = new Set(['hooks.example.com']); - -async function assertSafeUrl(raw: string): Promise { - const url = new URL(raw); - if (url.protocol !== 'https:') throw new Error('https only'); - if (!ALLOWED_HOSTS.has(url.hostname)) throw new Error('host not allowed'); - // Resolve ALL records; a single private/reserved address fails the check. - const addrs = await lookup(url.hostname, { all: true }); - if (addrs.some((a) => ipaddr.parse(a.address).range() !== 'unicast')) { - throw new Error('private/reserved IP'); - } - return url; -} - -await fetch(await assertSafeUrl(req.body.webhookUrl), { redirect: 'error' }); -``` - -The `range() !== 'unicast'` check covers loopback, link-local `169.254.169.254` (cloud metadata, the #1 SSRF target), private, and unique-local ranges across IPv4 and IPv6. - -**Caveat — this still has a TOCTOU gap.** `fetch` resolves DNS again after the check, so an attacker using a short-TTL record can rebind to an internal IP between validation and connection. For high-risk surfaces, resolve once and connect to the pinned IP, or put a filtering agent in front (`request-filtering-agent` / `ssrf-req-filter`). - -## Input Validation Patterns - -### Schema Validation at Boundaries - -```typescript -import { z } from 'zod'; - -const CreateTaskSchema = z.object({ - title: z.string().min(1).max(200).trim(), - description: z.string().max(2000).optional(), - priority: z.enum(['low', 'medium', 'high']).default('medium'), - dueDate: z.string().datetime().optional(), -}); - -// Validate at the route handler -app.post('/api/tasks', async (req, res) => { - const result = CreateTaskSchema.safeParse(req.body); - if (!result.success) { - return res.status(422).json({ - error: { - code: 'VALIDATION_ERROR', - message: 'Invalid input', - details: result.error.flatten(), - }, - }); - } - // result.data is now typed and validated - const task = await taskService.create(result.data); - return res.status(201).json(task); -}); -``` - -### File Upload Safety - -```typescript -// Restrict file types and sizes -const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'image/webp']; -const MAX_SIZE = 5 * 1024 * 1024; // 5MB - -function validateUpload(file: UploadedFile) { - if (!ALLOWED_TYPES.includes(file.mimetype)) { - throw new ValidationError('File type not allowed'); - } - if (file.size > MAX_SIZE) { - throw new ValidationError('File too large (max 5MB)'); - } - // Don't trust the file extension — check magic bytes if critical -} -``` - -### Destructive Operations on Derived Paths - -A delete, move, or overwrite is only as safe as the value that names its target. Reading that value from the kernel, a job payload, or a sibling service proves where it *arrived from*, not who *wrote* it — another process's command line is as attacker-controlled as a form field. A shape check ("absolute path, at least one directory deep") proves well-formedness and gets mistaken for authorization; that is how a cleanup routine deletes the root instead of the leaf. - -Before a destructive call, require all three: the resolved target sits under an **allowlisted root** (compare after resolving symlinks, never on the raw string); it is at least one level **below** that root, so a root is never itself the target; and it carries **evidence that it is yours**, read *before* the operation and before any teardown that removes it — otherwise "absent" and "not mine" are indistinguishable. On refusal, log the rejected target and stop: a cleanup that falls back to a broader default path is the failure this guards against. Worked example in `../../references/security-checklist.md`. - -Two limits, because the check reads stronger than it is. A marker inside the tree is self-attestation — anything that can write there can write the marker — so the expected owner has to come from authenticated state, and the marker needs integrity protection (restrictive ownership, or a MAC) before it counts as authorization. And resolving a path and then operating on the *name* is a check/use race wherever an untrusted process can swap an ancestor: on a shared volume, hold the target by descriptor and use no-follow, beneath-the-root operations, or make sure the hierarchy cannot change for the duration. - -## Triaging Dependency Audit Results - -Package-manager audits report known advisories; they do not prove a package is trustworthy or that vulnerable code is reachable. Use this decision tree: - -``` -The native package-manager audit reports a vulnerability -├── Severity: critical or high -│ ├── Is the vulnerable code reachable in runtime, build, test, or deployment paths? -│ │ ├── YES --> Fix immediately (update, patch, or replace the dependency) -│ │ └── NO (confirmed unused across those paths) --> Fix soon, but not a blocker -│ └── Is a fix available? -│ ├── YES --> Update to the patched version -│ └── NO --> Check for workarounds, consider replacing the dependency, or add to allowlist with a review date -├── Severity: moderate -│ ├── Reachable in production? --> Fix in the next release cycle -│ └── Dev-only? --> Fix when convenient, track in backlog -└── Severity: low - └── Track and fix during regular dependency updates -``` - -**Key questions:** -- Is the vulnerable function actually called in your code path? -- Is the dependency a runtime dependency or dev-only? -- Is the vulnerability exploitable given your deployment context (e.g., a server-side vulnerability in a client-only app)? - -When you defer a fix, document the reason and set a review date. - -### Supply-Chain Hygiene - -Do not assume npm or treat the nearest manifest as the install root. Apply this order: - -1. **Find the installation boundary and manager.** Use the workspace root that owns the lockfile, or an independent nested project only when it is outside that workspace. There, corroborate `packageManager` (when present), the lockfile, and CI; stop on disagreement or competing lockfiles. Pin the manager version and use the matrix in `../../references/security-checklist.md`. -2. **Block dependency scripts before first execution.** Bootstrap with scripts disabled or a documented fail-closed policy, inspect the pending script source, approve only the minimum required packages, commit the policy, then verify with a clean frozen/immutable install. Never blanket-approve scripts. - -Audits only find known advisories; they do not catch a newly malicious or typosquatted package. Therefore: - -- **Never apply forced audit remediation automatically** (`npm audit fix --force` or equivalent). Preview the remediation, read changelogs, and test each resulting upgrade; forced fixes may cross declared dependency ranges. -- **Verify registry signatures and provenance where supported** (`npm audit signatures`, `pnpm audit signatures`) and treat absence as a signal to investigate, not automatic proof of compromise. -- **Review new dependencies, lockfile diffs, and script-policy changes together** — ownership, maintenance, release age, provenance, transitive graph, and typosquats such as `cross-env` vs `crossenv` (OWASP **A06**, **LLM03**). - -## Rate Limiting - -```typescript -import rateLimit from 'express-rate-limit'; - -// General API rate limit -app.use('/api/', rateLimit({ - windowMs: 15 * 60 * 1000, // 15 minutes - max: 100, // 100 requests per window - standardHeaders: true, - legacyHeaders: false, -})); - -// Stricter limit for auth endpoints -app.use('/api/auth/', rateLimit({ - windowMs: 15 * 60 * 1000, - max: 10, // 10 attempts per 15 minutes -})); -``` - -**Count in a shared store once there is more than one process.** `express-rate-limit` keeps its counters in process memory by default. Behind a load balancer each instance holds its own count, so the effective limit is `max × instances`; on serverless or edge runtimes a fresh invocation starts from zero, so the auth limit above may never fire. Pass a shared `store` (Redis via `rate-limit-redis`), or use an HTTP-based limiter that works where a long-lived TCP connection does not (for example `@upstash/ratelimit`): - -```typescript -import { Ratelimit } from '@upstash/ratelimit'; -import { Redis } from '@upstash/redis'; - -const authLimiter = new Ratelimit({ - redis: Redis.fromEnv(), // UPSTASH_REDIS_REST_URL + _TOKEN - limiter: Ratelimit.slidingWindow(10, '15 m'), // 10 attempts per 15 minutes, across all instances -}); -const { success } = await authLimiter.limit(`login:${req.ip}`); -if (!success) return res.status(429).end(); -``` - -## Secrets Management - -``` -.env files: - ├── .env.example → Committed (template with placeholder values) - ├── .env → NOT committed (contains real secrets) - └── .env.local → NOT committed (local overrides) - -.gitignore must include: - .env - .env.local - .env.*.local - *.pem - *.key -``` - -**Always check before committing:** -```bash -# Check for accidentally staged secrets -git diff --cached | grep -i "password\|secret\|api_key\|token" -``` - -**If a secret is ever committed, rotate it.** Deleting the line or rewriting history is not enough — assume it's compromised the moment it reaches a remote. Revoke and reissue the key first, then purge it from history. - -## Data Privacy & Compliance - -Securing data is "can an attacker read it?" Privacy is "should *we* even hold it, and for how long?" — a separate question that hardening doesn't answer. The cheapest data to protect, breach, and comply over is the data you never collected. Treat personal data as a liability to minimize, not an asset to hoard. - -**Know what you hold.** You can't protect or honor a deletion request for data you can't find. Classify fields as you add them: - -| Class | Examples | Handling | -|---|---|---| -| **Non-personal** | Aggregates, anonymized counts | Normal handling | -| **Personal (PII)** | Name, email, IP, device/user IDs | Minimize, access-control, include in export/delete | -| **Sensitive** | Health, finance, location, biometrics, gov IDs, anything about minors | Extra basis to collect, stricter access, often encryption + audit logging | - -**Operating rules:** -- **Minimize and set a purpose.** Collect a field only against a stated use. "It might be useful later" is not a purpose — it's latent breach scope. Don't log PII into telemetry (the `observability-and-instrumentation` skill makes the same point from the ops side). -- **Set retention up front, then actually delete.** Every personal-data store needs a TTL and a working deletion path — including backups, caches, search indexes, and analytics copies. Data with no expiry is a breach scheduled for later. -- **Support the data-subject rights your jurisdiction requires** (GDPR/CCPA and kin): export, correct, and delete on request. These are engineering features — design the schema so a user's data is *findable* and *erasable*, not smeared irreversibly across systems. -- **Get consent before collection or third-party sharing**, and make it auditable. Sending PII to an analytics/ad/LLM vendor is "sharing" — the user's choice gates it, and the vendor needs a data-processing agreement. -- **Localize defaults, don't hardcode one region's law.** Data-residency and rules differ by user location; make the policy a configurable boundary, not an assumption. - -When data crosses a trust boundary, validate it as untrusted (see Input Validation above); when a privacy incident exposes personal data, the breach-notification clock is part of the postmortem — follow the `debugging-and-error-recovery` skill. - -## Securing AI / LLM Features - -If your app calls an LLM — chatbots, summarizers, agents, RAG — it inherits a new attack surface. Map it to the [OWASP Top 10 for LLM Applications (2025)](https://genai.owasp.org/llm-top-10/): - -- **Treat all model output as untrusted input (LLM05: Improper Output Handling).** Never pass LLM output straight into `eval`, SQL, a shell, `innerHTML`, or a file path. Validate and encode it exactly as you would raw user input. -- **Assume prompts can be hijacked (LLM01: Prompt Injection).** Untrusted text in the context window — a user message, a fetched web page, a PDF — can carry instructions. The system prompt is not a security boundary; enforce permissions in code, not in the prompt. -- **Keep secrets and other users' data out of prompts (LLM02 / LLM07).** Anything in the context can be echoed back. Don't put API keys, cross-tenant data, or the full system prompt where the model can repeat it. -- **Constrain tool and agent permissions (LLM06: Excessive Agency).** Scope tools to the minimum, require confirmation for destructive or irreversible actions, and validate every tool argument. -- **Bound consumption (LLM10: Unbounded Consumption).** Cap tokens, request rate, and loop/recursion depth so a crafted input can't run up cost or hang the system. -- **Isolate retrieval data (LLM08: Vector and Embedding Weaknesses).** In RAG, treat the vector store as a trust boundary: partition embeddings per tenant so one user can't retrieve another's data, and validate documents before indexing so poisoned content can't steer answers. - -```typescript -// BAD: trusting model output as a command or as markup -const sql = await llm.generate(`Write SQL for: ${userQuestion}`); -await db.query(sql); // arbitrary query execution -container.innerHTML = await llm.reply(userMessage); // stored XSS, via the model - -// GOOD: model output is data — parse defensively, then validate, then encode -let intent; -try { - intent = CommandSchema.parse(JSON.parse(await llm.replyJson(userMessage))); -} catch { - throw new ValidationError('unexpected model output'); // JSON.parse or schema failed -} -await runAllowlistedAction(intent.action, intent.params); -container.textContent = await llm.reply(userMessage); -``` - -## Security Review Checklist - -```markdown -### Authentication -- [ ] Passwords hashed with bcrypt/scrypt/argon2 (salt rounds ≥ 12) -- [ ] Session tokens are httpOnly, secure, sameSite -- [ ] Login has rate limiting -- [ ] Password reset tokens expire - -### Authorization -- [ ] Every endpoint checks user permissions -- [ ] Users can only access their own resources -- [ ] Admin actions require admin role verification - -### Input -- [ ] All user input validated at the boundary -- [ ] SQL queries are parameterized -- [ ] HTML output is encoded/escaped -- [ ] Server-side URL fetches are allowlisted (no SSRF to internal services) -- [ ] Delete/move/overwrite targets built from data are checked against an allowlisted root, a minimum depth, and ownership evidence read before the operation - -### Data -- [ ] No secrets in code or version control -- [ ] Sensitive fields excluded from API responses -- [ ] PII encrypted at rest (if applicable) -- [ ] Personal data is classified, collected against a stated purpose, and minimized -- [ ] Personal data has a retention limit and a working deletion path (incl. backups/indexes) -- [ ] Export/delete (data-subject) requests are supported where required; sharing with third parties has consent - -### Infrastructure -- [ ] Security headers configured (CSP, HSTS, etc.) -- [ ] CORS restricted to known origins -- [ ] Dependencies audited for vulnerabilities -- [ ] Error messages don't expose internals - -### Supply Chain -- [ ] One authoritative lockfile committed; CI uses that manager's frozen/immutable install -- [ ] Native audit triaged by reachability and fix risk; dependency install scripts blocked unless explicitly approved -- [ ] New dependencies reviewed (ownership, provenance, release age, transitive graph) - -### AI / LLM (if used) -- [ ] Model output treated as untrusted (no eval/SQL/innerHTML/shell) -- [ ] Secrets and other users' data kept out of prompts -- [ ] Tool/agent permissions scoped; destructive actions require confirmation -``` -## See Also - -For detailed security checklists and pre-commit verification steps, see `../../references/security-checklist.md`. +## Hardening Controls + +The rules below are the workflow; a concrete implementation of each lives in [references/hardening-patterns.md](references/hardening-patterns.md). Open the section you need when you reach that code, not before. + +### Injection, XSS, and access control + +- Parameterize every query. Never build SQL, NoSQL, or shell commands from input strings. +- Encode output through the framework's auto-escaping. If raw HTML is unavoidable, sanitize with an allowlist sanitizer first. +- Check **authorization** on every request, not just authentication: the authenticated user must own, or be permitted on, the specific resource (A01, IDOR). + +Patterns: [Injection](references/hardening-patterns.md#injection), [XSS](references/hardening-patterns.md#cross-site-scripting-xss), [Access control](references/hardening-patterns.md#broken-access-control). + +### Authentication and sessions + +- Hash passwords with bcrypt (≥12 rounds), scrypt, or argon2. The session secret comes from the environment, never from code. +- Session cookies are `httpOnly`, `secure`, `sameSite`, with a bounded `maxAge`. + +Pattern: [Authentication](references/hardening-patterns.md#broken-authentication). + +### Headers, CORS, and responses + +- Security headers on every response (helmet or the framework equivalent); CSP starts from `default-src 'self'` and is tightened, not loosened. +- CORS restricted to an explicit origin list from configuration. Never `*` with credentials. +- Strip sensitive fields (`passwordHash`, reset tokens) before any response. Error bodies are generic; internals go to server logs only. + +Patterns: [Misconfiguration](references/hardening-patterns.md#security-misconfiguration), [Sensitive data exposure](references/hardening-patterns.md#sensitive-data-exposure). + +### Input validation and uploads + +- Validate at the boundary with a schema: allowlisted shape, lengths, enums, formats. Reject with 422 and structured details; downstream code uses only the parsed, typed value. +- Uploads: allowlist MIME types, cap size, verify content (magic bytes) when it matters. The extension proves nothing. + +Patterns: [Schema validation](references/hardening-patterns.md#schema-validation-at-boundaries), [File upload](references/hardening-patterns.md#file-upload-safety). + +### Server-side fetches (SSRF) + +Any URL the user influences — webhooks, import-from-URL, image proxies, link previews — can be aimed at internal services. Allowlist scheme and host, resolve **all** DNS records and reject any private or reserved address (loopback, link-local `169.254.169.254`, private, unique-local, for IPv4 and IPv6), and forbid redirects. That check still has a DNS-rebinding TOCTOU gap: for high-risk surfaces, pin the resolved IP or put a filtering agent in front. + +Pattern: [SSRF](references/hardening-patterns.md#server-side-request-forgery-ssrf). + +### Destructive operations on derived paths + +A delete, move, or overwrite is only as safe as the value naming its target, and trust follows who *wrote* that value, not which channel delivered it: another process's command line is as attacker-controlled as a form field. A shape check proves well-formedness, not authorization. Before the call, require all three: the resolved target (symlinks resolved) sits under an **allowlisted root**; it is at least one level **below** that root; and it carries **ownership evidence read before the operation**. On refusal, log the rejected target and stop; never fall back to a broader default path. + +Why the check is weaker than it reads (marker self-attestation, check/use races): [Destructive paths](references/hardening-patterns.md#destructive-operations-on-derived-paths). Worked code: `../../references/security-checklist.md`. + +### Rate limiting + +Limit the API generally and auth endpoints strictly (about 10 attempts per 15 minutes). Once more than one process serves traffic, in-memory counters silently become `max × instances`, or never fire on serverless: back the limiter with a shared store. + +Pattern: [Rate limiting](references/hardening-patterns.md#rate-limiting). + +### Secrets + +Secrets come from the environment. `.env.example` is committed with placeholders; real `.env*` files and key material are gitignored; grep the staged diff before committing. **A secret that reaches a remote is compromised the moment it lands: rotate it first, then purge history.** + +Pattern: [Secrets management](references/hardening-patterns.md#secrets-management). + +### Dependencies and supply chain + +1. **Find the installation boundary and manager.** Use the workspace root that owns the lockfile, or an independent nested project only when it is outside that workspace. Corroborate `packageManager` (when present), the lockfile, and CI; stop on disagreement or competing lockfiles. Pin the manager version. +2. **Block dependency scripts before first execution.** Bootstrap with scripts disabled or a documented fail-closed policy, inspect the pending script source, approve only the minimum, commit the policy, then verify with a clean frozen/immutable install. Never blanket-approve. +3. **Run the native audit against the committed lockfile before every release.** Triage critical/high by **reachability** (runtime, build, test, deploy paths) and fix availability. Never apply forced remediation (`npm audit fix --force` or equivalent) automatically; preview, read changelogs, test each upgrade. Document every deferral with a reason and a review date. +4. **Audits only match known advisories.** They do not catch a newly malicious or typosquatted package (`cross-env` vs `crossenv`). Review new dependencies, lockfile diffs, and script-policy changes together: ownership, maintenance, release age, provenance, transitive graph. Verify registry signatures where supported and treat their absence as a signal to investigate (A06, LLM03). + +Triage decision tree: [Dependency audit triage](references/hardening-patterns.md#dependency-audit-triage). Manager matrix and install-script gate: `../../references/security-checklist.md`. + +### Personal data and privacy + +Hardening asks "can an attacker read it?" Privacy asks "should *we* hold it at all, and for how long?" The cheapest data to protect, breach, and comply over is the data you never collected; treat personal data as a liability to minimize. + +- **Classify fields as you add them** (non-personal, PII, sensitive) and handle each class accordingly. You cannot protect, or honor a deletion request for, data you cannot find. +- **Collect only against a stated purpose.** "Might be useful later" is latent breach scope, not a purpose. Keep PII out of telemetry (the `observability-and-instrumentation` skill makes the same point from the ops side). +- **Set retention up front, then actually delete** — including backups, caches, search indexes, and analytics copies. +- **Support the data-subject rights your jurisdiction requires** (GDPR, CCPA, and kin): export, correct, delete. Design the schema so a user's data is findable and erasable, not smeared irreversibly across systems. +- **Consent gates collection and third-party sharing.** Sending PII to an analytics, ad, or LLM vendor is sharing; the vendor needs a data-processing agreement. Make region a configurable policy, not a hardcoded assumption. + +Classification table: [Data classification](references/hardening-patterns.md#data-classification). A privacy incident starts the breach-notification clock; run the postmortem with the `debugging-and-error-recovery` skill. + +### AI / LLM features + +Calling an LLM — chatbots, summarizers, agents, RAG — adds a new attack surface; map it to the [OWASP Top 10 for LLM Applications](https://genai.owasp.org/llm-top-10/): + +- **Model output is untrusted input** (LLM05). Never into `eval`, SQL, a shell, `innerHTML`, or a file path; parse defensively, validate against a schema, then encode. +- **Prompts can be hijacked** (LLM01). Untrusted text in the context — a user message, a fetched page, a PDF — can carry instructions. The system prompt is not a security boundary; enforce permissions in code. +- **Keep secrets and other tenants' data out of the context window** (LLM02, LLM07); scope tool permissions and confirm destructive actions (LLM06); cap tokens, request rate, and recursion depth (LLM10); partition RAG embeddings per tenant and validate documents before indexing (LLM08). + +Pattern: [LLM output handling](references/hardening-patterns.md#llm-output-handling). + +## Review Checklist + +Before sign-off, walk `../../references/security-checklist.md`: it covers authentication, authorization, input, data protection and privacy, headers and CORS, dependencies and supply chain, AI/LLM, and error handling, plus the OWASP quick-reference tables. ## Common Rationalizations diff --git a/skills/security-and-hardening/references/hardening-patterns.md b/skills/security-and-hardening/references/hardening-patterns.md new file mode 100644 index 00000000..9af26044 --- /dev/null +++ b/skills/security-and-hardening/references/hardening-patterns.md @@ -0,0 +1,324 @@ +# Hardening Patterns + +Code patterns and decision aids for the `security-and-hardening` skill. The skill states each rule; this file shows one concrete implementation of it. Open the section you need when you reach that code — it is not meant to be read top to bottom. + +Shared checklists (threat modeling, pre-commit checks, security headers, CORS, the package-manager matrix, install-script gate, and the OWASP quick-reference tables) live in `../../../references/security-checklist.md`. + +## OWASP Top 10 Prevention Patterns + +These are prevention patterns, not a ranking. For the 2021 ordering, see the quick-reference table in `../../../references/security-checklist.md#owasp-top-10-quick-reference`. + +### Injection + +```typescript +// BAD: SQL injection via string concatenation +const query = `SELECT * FROM users WHERE id = '${userId}'`; + +// GOOD: Parameterized query +const user = await db.query('SELECT * FROM users WHERE id = $1', [userId]); + +// GOOD: ORM with parameterized input +const user = await prisma.user.findUnique({ where: { id: userId } }); +``` + +### Broken Authentication + +```typescript +// Password hashing +import { hash, compare } from 'bcrypt'; + +const SALT_ROUNDS = 12; +const hashedPassword = await hash(plaintext, SALT_ROUNDS); +const isValid = await compare(plaintext, hashedPassword); + +// Session management +app.use(session({ + secret: process.env.SESSION_SECRET, // From environment, not code + resave: false, + saveUninitialized: false, + cookie: { + httpOnly: true, // Not accessible via JavaScript + secure: true, // HTTPS only + sameSite: 'lax', // CSRF protection + maxAge: 24 * 60 * 60 * 1000, // 24 hours + }, +})); +``` + +### Cross-Site Scripting (XSS) + +```typescript +// BAD: Rendering user input as HTML +element.innerHTML = userInput; + +// GOOD: Use framework auto-escaping (React does this by default) +return
{userInput}
; + +// If you MUST render HTML, sanitize first +import DOMPurify from 'dompurify'; +const clean = DOMPurify.sanitize(userInput); +``` + +### Broken Access Control + +```typescript +// Always check authorization, not just authentication +app.patch('/api/tasks/:id', authenticate, async (req, res) => { + const task = await taskService.findById(req.params.id); + + // Check that the authenticated user owns this resource + if (task.ownerId !== req.user.id) { + return res.status(403).json({ + error: { code: 'FORBIDDEN', message: 'Not authorized to modify this task' } + }); + } + + // Proceed with update + const updated = await taskService.update(req.params.id, req.body); + return res.json(updated); +}); +``` + +### Security Misconfiguration + +```typescript +// Security headers (use helmet for Express) +import helmet from 'helmet'; +app.use(helmet()); + +// Content Security Policy +app.use(helmet.contentSecurityPolicy({ + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'"], + styleSrc: ["'self'", "'unsafe-inline'"], // Tighten if possible + imgSrc: ["'self'", 'data:', 'https:'], + connectSrc: ["'self'"], + }, +})); + +// CORS — restrict to known origins +app.use(cors({ + origin: process.env.ALLOWED_ORIGINS?.split(',') || 'http://localhost:3000', + credentials: true, +})); +``` + +### Sensitive Data Exposure + +```typescript +// Never return sensitive fields in API responses +function sanitizeUser(user: UserRecord): PublicUser { + const { passwordHash, resetToken, ...publicFields } = user; + return publicFields; +} + +// Use environment variables for secrets +const API_KEY = process.env.STRIPE_API_KEY; +if (!API_KEY) throw new Error('STRIPE_API_KEY not configured'); +``` + +### Server-Side Request Forgery (SSRF) + +Any time the server fetches a URL the user influenced — webhooks, "import from URL", image proxies, link previews — an attacker can aim it at internal services (cloud metadata, `localhost`, private IPs). + +```typescript +// BAD: fetch whatever the user gives you +await fetch(req.body.webhookUrl); + +// GOOD: allowlist scheme + host, reject if ANY resolved IP is private, forbid redirects +import { lookup } from 'node:dns/promises'; +import ipaddr from 'ipaddr.js'; + +const ALLOWED_HOSTS = new Set(['hooks.example.com']); + +async function assertSafeUrl(raw: string): Promise { + const url = new URL(raw); + if (url.protocol !== 'https:') throw new Error('https only'); + if (!ALLOWED_HOSTS.has(url.hostname)) throw new Error('host not allowed'); + // Resolve ALL records; a single private/reserved address fails the check. + const addrs = await lookup(url.hostname, { all: true }); + if (addrs.some((a) => ipaddr.parse(a.address).range() !== 'unicast')) { + throw new Error('private/reserved IP'); + } + return url; +} + +await fetch(await assertSafeUrl(req.body.webhookUrl), { redirect: 'error' }); +``` + +The `range() !== 'unicast'` check covers loopback, link-local `169.254.169.254` (cloud metadata, the #1 SSRF target), private, and unique-local ranges across IPv4 and IPv6. + +**Caveat — this still has a TOCTOU gap.** `fetch` resolves DNS again after the check, so an attacker using a short-TTL record can rebind to an internal IP between validation and connection. For high-risk surfaces, resolve once and connect to the pinned IP, or put a filtering agent in front (`request-filtering-agent` / `ssrf-req-filter`). + +## Input Validation Patterns + +### Schema Validation at Boundaries + +```typescript +import { z } from 'zod'; + +const CreateTaskSchema = z.object({ + title: z.string().min(1).max(200).trim(), + description: z.string().max(2000).optional(), + priority: z.enum(['low', 'medium', 'high']).default('medium'), + dueDate: z.string().datetime().optional(), +}); + +// Validate at the route handler +app.post('/api/tasks', async (req, res) => { + const result = CreateTaskSchema.safeParse(req.body); + if (!result.success) { + return res.status(422).json({ + error: { + code: 'VALIDATION_ERROR', + message: 'Invalid input', + details: result.error.flatten(), + }, + }); + } + // result.data is now typed and validated + const task = await taskService.create(result.data); + return res.status(201).json(task); +}); +``` + +### File Upload Safety + +```typescript +// Restrict file types and sizes +const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'image/webp']; +const MAX_SIZE = 5 * 1024 * 1024; // 5MB + +function validateUpload(file: UploadedFile) { + if (!ALLOWED_TYPES.includes(file.mimetype)) { + throw new ValidationError('File type not allowed'); + } + if (file.size > MAX_SIZE) { + throw new ValidationError('File too large (max 5MB)'); + } + // Don't trust the file extension — check magic bytes if critical +} +``` + +### Destructive Operations on Derived Paths + +A delete, move, or overwrite is only as safe as the value that names its target. Reading that value from the kernel, a job payload, or a sibling service proves where it *arrived from*, not who *wrote* it — another process's command line is as attacker-controlled as a form field. A shape check ("absolute path, at least one directory deep") proves well-formedness and gets mistaken for authorization; that is how a cleanup routine deletes the root instead of the leaf. + +Before a destructive call, require all three: the resolved target sits under an **allowlisted root** (compare after resolving symlinks, never on the raw string); it is at least one level **below** that root, so a root is never itself the target; and it carries **evidence that it is yours**, read *before* the operation and before any teardown that removes it — otherwise "absent" and "not mine" are indistinguishable. On refusal, log the rejected target and stop: a cleanup that falls back to a broader default path is the failure this guards against. Worked example in `../../../references/security-checklist.md#destructive-path-operations`. + +Two limits, because the check reads stronger than it is. A marker inside the tree is self-attestation — anything that can write there can write the marker — so the expected owner has to come from authenticated state, and the marker needs integrity protection (restrictive ownership, or a MAC) before it counts as authorization. And resolving a path and then operating on the *name* is a check/use race wherever an untrusted process can swap an ancestor: on a shared volume, hold the target by descriptor and use no-follow, beneath-the-root operations, or make sure the hierarchy cannot change for the duration. + +## Dependency Audit Triage + +Package-manager audits report known advisories; they do not prove a package is trustworthy or that vulnerable code is reachable. Use this decision tree: + +``` +The native package-manager audit reports a vulnerability +├── Severity: critical or high +│ ├── Is the vulnerable code reachable in runtime, build, test, or deployment paths? +│ │ ├── YES --> Fix immediately (update, patch, or replace the dependency) +│ │ └── NO (confirmed unused across those paths) --> Fix soon, but not a blocker +│ └── Is a fix available? +│ ├── YES --> Update to the patched version +│ └── NO --> Check for workarounds, consider replacing the dependency, or add to allowlist with a review date +├── Severity: moderate +│ ├── Reachable in production? --> Fix in the next release cycle +│ └── Dev-only? --> Fix when convenient, track in backlog +└── Severity: low + └── Track and fix during regular dependency updates +``` + +**Key questions:** +- Is the vulnerable function actually called in your code path? +- Is the dependency a runtime dependency or dev-only? +- Is the vulnerability exploitable given your deployment context (e.g., a server-side vulnerability in a client-only app)? + +When you defer a fix, document the reason and set a review date. + +## Rate Limiting + +```typescript +import rateLimit from 'express-rate-limit'; + +// General API rate limit +app.use('/api/', rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 100, // 100 requests per window + standardHeaders: true, + legacyHeaders: false, +})); + +// Stricter limit for auth endpoints +app.use('/api/auth/', rateLimit({ + windowMs: 15 * 60 * 1000, + max: 10, // 10 attempts per 15 minutes +})); +``` + +**Count in a shared store once there is more than one process.** `express-rate-limit` keeps its counters in process memory by default. Behind a load balancer each instance holds its own count, so the effective limit is `max × instances`; on serverless or edge runtimes a fresh invocation starts from zero, so the auth limit above may never fire. Pass a shared `store` (Redis via `rate-limit-redis`), or use an HTTP-based limiter that works where a long-lived TCP connection does not (for example `@upstash/ratelimit`): + +```typescript +import { Ratelimit } from '@upstash/ratelimit'; +import { Redis } from '@upstash/redis'; + +const authLimiter = new Ratelimit({ + redis: Redis.fromEnv(), // UPSTASH_REDIS_REST_URL + _TOKEN + limiter: Ratelimit.slidingWindow(10, '15 m'), // 10 attempts per 15 minutes, across all instances +}); +const { success } = await authLimiter.limit(`login:${req.ip}`); +if (!success) return res.status(429).end(); +``` + +## Secrets Management + +``` +.env files: + ├── .env.example → Committed (template with placeholder values) + ├── .env → NOT committed (contains real secrets) + └── .env.local → NOT committed (local overrides) + +.gitignore must include: + .env + .env.local + .env.*.local + *.pem + *.key +``` + +**Always check before committing:** +```bash +# Check for accidentally staged secrets +git diff --cached | grep -i "password\|secret\|api_key\|token" +``` + +**If a secret is ever committed, rotate it.** Deleting the line or rewriting history is not enough — assume it's compromised the moment it reaches a remote. Revoke and reissue the key first, then purge it from history. + +## Data Classification + +Classify personal data as you add fields, so export and deletion requests can find it later: + +| Class | Examples | Handling | +|---|---|---| +| **Non-personal** | Aggregates, anonymized counts | Normal handling | +| **Personal (PII)** | Name, email, IP, device/user IDs | Minimize, access-control, include in export/delete | +| **Sensitive** | Health, finance, location, biometrics, gov IDs, anything about minors | Extra basis to collect, stricter access, often encryption + audit logging | + +## LLM Output Handling + +```typescript +// BAD: trusting model output as a command or as markup +const sql = await llm.generate(`Write SQL for: ${userQuestion}`); +await db.query(sql); // arbitrary query execution +container.innerHTML = await llm.reply(userMessage); // stored XSS, via the model + +// GOOD: model output is data — parse defensively, then validate, then encode +let intent; +try { + intent = CommandSchema.parse(JSON.parse(await llm.replyJson(userMessage))); +} catch { + throw new ValidationError('unexpected model output'); // JSON.parse or schema failed +} +await runAllowlistedAction(intent.action, intent.params); +container.textContent = await llm.reply(userMessage); +```