diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 507d66a..5c32d5e 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -20,7 +20,7 @@ { "name": "agentic-engineering", "description": "The autonomous engineering system for repository portfolios and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; portfolio, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use; configured by the consumer AGENTS.md", - "version": "7.4.0", + "version": "7.5.0", "source": "./plugins/agentic-engineering" }, { diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json index 507d66a..5c32d5e 100644 --- a/.github/plugin/marketplace.json +++ b/.github/plugin/marketplace.json @@ -20,7 +20,7 @@ { "name": "agentic-engineering", "description": "The autonomous engineering system for repository portfolios and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; portfolio, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use; configured by the consumer AGENTS.md", - "version": "7.4.0", + "version": "7.5.0", "source": "./plugins/agentic-engineering" }, { diff --git a/docs/plugins.md b/docs/plugins.md index a152be0..88eb5de 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -6,7 +6,7 @@ A **skill** teaches an assistant a task. An **agent** gives it a specialized rol | Plugin | Resources | Description | |--------|-----------|-------------| -| [`agentic-engineering`](../plugins/agentic-engineering/) | `accessibility-review`, `agent-improvement`, `agent-instructions`, `architecture`, `audit-support`, `autofix`, `brand-review`, `campaign-plan`, `check-skills`, `close-management`, `code-review`, `codeql`, `competitive-brief`, `connector-setup`, `content-creation`, `context7-mcp`, `conventional-release`, `debug`, `dependabot`, `dependency-scanning`, `deploy-checklist`, `design-critique`, `design-handoff`, `design-system`, `documentation`, `draft-content`, `email-sequence`, `financial-statements`, `find-skills`, `frontend-design`, `gh-cli`, `gh-stack`, `git-commit`, `github-actions-docs`, `github-actions-efficiency`, `github-actions-hardening`, `github-issues`, `gitops-cluster-debug`, `gitops-knowledge`, `gitops-tenant-onboarding`, `golang-pro`, `grilling`, `incident-response`, `journal-entry`, `journal-entry-prep`, `memory-management`, `metrics-review`, `performance-report`, `portfolio-maintenance`, `product-brainstorming`, `product-documentation`, `product-engineering`, `prometheus`, `promql`, `prototype`, `reconciliation`, `refactor`, `research-synthesis`, `roadmap-update`, `secret-scanning`, `self-improvement`, `seo-audit`, `siderolabs`, `skill-lifecycle`, `sox-testing`, `sprint-planning`, `stakeholder-update`, `standup`, `start`, `stripe-best-practices`, `stripe-docs`, `svelte-core-bestpractices`, `synthesize-research`, `system-design`, `task-management`, `tech-debt`, `test-driven-development`, `testing-strategy`, `update`, `upgrade-stripe`, `user-research`, `ux-copy`, `variance-analysis`, `ways-of-working`, `web-design-guidelines`, `workspace-conventions`, `write-contributing`, `write-docs-introduction`, `write-readme`, `write-spec` (skills) · `agent-improver`, `agentic-engineer`, `flux-troubleshooter`, `portfolio-surveyor` (agents) · `aws-knowledge`, `cloudflare-docs`, `context7`, `coroot`, `flux-operator-mcp`, `github`, `github-ghas-tools`, `kubescape`, `siderolabs-docs`, `stripe`, `svelte`, `terraform-registry` (MCP servers) | The autonomous engineering system for a whole repository portfolio, and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; operating, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use. Configured by the consumer's `AGENTS.md` | +| [`agentic-engineering`](../plugins/agentic-engineering/) | `accessibility-review`, `agent-improvement`, `agent-instructions`, `application-security`, `architecture`, `audit-support`, `autofix`, `brand-review`, `campaign-plan`, `check-skills`, `close-management`, `code-review`, `codeql`, `competitive-brief`, `connector-setup`, `content-creation`, `context7-mcp`, `conventional-release`, `debug`, `dependabot`, `dependency-scanning`, `deploy-checklist`, `design-critique`, `design-handoff`, `design-system`, `documentation`, `draft-content`, `email-sequence`, `financial-statements`, `find-skills`, `frontend-design`, `gh-cli`, `gh-stack`, `git-commit`, `github-actions-docs`, `github-actions-efficiency`, `github-actions-hardening`, `github-issues`, `gitops-cluster-debug`, `gitops-knowledge`, `gitops-tenant-onboarding`, `golang-pro`, `grilling`, `incident-response`, `journal-entry`, `journal-entry-prep`, `memory-management`, `metrics-review`, `performance-report`, `portfolio-maintenance`, `product-brainstorming`, `product-documentation`, `product-engineering`, `prometheus`, `promql`, `prototype`, `reconciliation`, `refactor`, `research-synthesis`, `roadmap-update`, `secret-scanning`, `self-improvement`, `seo-audit`, `siderolabs`, `skill-lifecycle`, `sox-testing`, `sprint-planning`, `stakeholder-update`, `standup`, `start`, `stripe-best-practices`, `stripe-docs`, `svelte-core-bestpractices`, `synthesize-research`, `system-design`, `task-management`, `tech-debt`, `test-driven-development`, `testing-strategy`, `update`, `upgrade-stripe`, `user-research`, `ux-copy`, `variance-analysis`, `ways-of-working`, `web-design-guidelines`, `workspace-conventions`, `write-contributing`, `write-docs-introduction`, `write-readme`, `write-spec` (skills) · `agent-improver`, `agentic-engineer`, `flux-troubleshooter`, `portfolio-surveyor` (agents) · `aws-knowledge`, `cloudflare-docs`, `context7`, `coroot`, `flux-operator-mcp`, `github`, `github-ghas-tools`, `kubescape`, `siderolabs-docs`, `stripe`, `svelte`, `terraform-registry` (MCP servers) | The autonomous engineering system for a whole repository portfolio, and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; operating, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use. Configured by the consumer's `AGENTS.md` | | [`vibe-coding`](../plugins/vibe-coding/) | `needs-stack-mapping`, `allowed-stack-guardrail`, `jargon-free-voice` (skills) · `vibe-coding-companion` (agent) | Build a product by conversation alone — plain-language companion agent + guardrailed needs-to-stack skills for people with no technical background | | [`ksail`](../plugins/ksail/) | `ksail` (skill) · `ksail` (MCP server) | Create and operate Kubernetes clusters and GitOps workloads with the KSail CLI — needs `ksail` on `PATH`; PolyForm Shield licensed | @@ -22,6 +22,7 @@ license file the plugin ships; keep both when you copy or redistribute those ski | `accessibility-review` | `agentic-engineering` | anthropics/knowledge-work-plugins | Apache-2.0 | | `agent-improvement` | `agentic-engineering` | devantler-tech/agent-plugins | Apache-2.0 | | `agent-instructions` | `agentic-engineering` | devantler-tech/agent-plugins | Apache-2.0 | +| `application-security` | `agentic-engineering` | devantler-tech/agent-plugins | Apache-2.0 | | `architecture` | `agentic-engineering` | anthropics/knowledge-work-plugins | Apache-2.0 | | `audit-support` | `agentic-engineering` | anthropics/knowledge-work-plugins | Apache-2.0 | | `autofix` | `agentic-engineering` | coderabbitai/skills | MIT | diff --git a/docs/skill-coverage.md b/docs/skill-coverage.md index bf333a0..e074da7 100644 --- a/docs/skill-coverage.md +++ b/docs/skill-coverage.md @@ -34,18 +34,18 @@ This is an engineering inventory. It makes no legal or compliance claim. |---|---|---|---|---|---| | Privacy and GDPR | [contact handling][bs-contacts], [customer identity][bs-identity] and [invoices][bs-invoices] in the business-site portal; [guest RSVPs][wa-schema] in wedding-app | none | all of it | High | **Owned** by [#638](https://github.com/devantler-tech/agent-plugins/issues/638) | | Danish commerce, consumer and accessibility rules | client sites with service offerings and booking ([ascoachingogvaner][asc-agents]); [business-site invoicing][bs-invoices] | `web-design-guidelines` covers UI accessibility review, not Danish law | the legal rules | High | **Owned** by [#639](https://github.com/devantler-tech/agent-plugins/issues/639) | -| Application security and tenant isolation | the [business-site customer portal][bs-identity], [KHoist's data product API][khoist-api], tenant apps on the shared platform | CI and supply chain: `github-actions-hardening`, `codeql`, `secret-scanning`, `dependency-scanning`, `dependabot`. Cluster tenants: `gitops-tenant-onboarding` | application layer: object-level authorization, sessions, webhook verification, threat models | High | **Owned** by [#640](https://github.com/devantler-tech/agent-plugins/issues/640) | +| Application security and tenant isolation | the [business-site customer portal][bs-identity], [KHoist's data product API][khoist-api], tenant apps on the shared platform | `application-security` (threat model, access rules, tenant isolation, webhooks, uploads, limits and the tests that prove them). CI and supply chain: `github-actions-hardening`, `codeql`, `secret-scanning`, `dependency-scanning`, `dependabot`. Cluster tenants: `gitops-tenant-onboarding` | none in guidance. Each product still writes its own isolation tests, and its own instructions name its roles and tenants | High | **Covered** since 7.5.0 by the first-party `application-security` ([#640](https://github.com/devantler-tech/agent-plugins/issues/640)): no upstream candidate could be adopted, as the candidate table says | | SvelteKit and TypeScript | business-site [`web/`][bs-web], [wedding-app][wa-pkg], [ascoachingogvaner][asc-pkg] | `svelte-core-bestpractices` and the `svelte` documentation server (documentation, autofixer and playground links), beside the framework-neutral `frontend-design` and `web-design-guidelines` | none for Svelte. TypeScript itself is not treated as a gap; if that needs checking, add a TypeScript case to #651 | Medium | **Covered** since 7.0.0 ([#652](https://github.com/devantler-tech/agent-plugins/issues/652)) | | Astro | the [KSail docs][ksail-docs] and the [KHoist website][khoist-web] | `product-documentation`, `write-docs-introduction` | Astro framework guidance: the `astro` skill left the bundle in 7.0.0 | Medium | **Not bundled.** The 7.0.0 entry in the plugin's [changelog][changelog] has the one command that installs `astro` from its upstream for work on these two sites | | Go | [ksail][ksail-go], [khoist][khoist-go], the [business-site server][bs-go], platform helpers | `golang-pro`, `test-driven-development`, `refactor` | activation only: 0 loads in a week with at least 34 Go sessions (#642) | Medium | **Covered**; measured by #651 | -| API and form validation | request handling in the SvelteKit sites (for example the [wedding-app RSVP endpoint][wa-rsvp]) and the [Go portal][bs-go] | `web-design-guidelines` covers form markup | server-side validation, rate limits and uploads belong to application security; personal data in forms belongs to privacy | Medium | **Do not add** a separate skill: owned by #640 and #638 | +| API and form validation | request handling in the SvelteKit sites (for example the [wedding-app RSVP endpoint][wa-rsvp]) and the [Go portal][bs-go] | `web-design-guidelines` covers form markup | none for validation, limits and uploads (`application-security`); personal data in forms belongs to privacy | Medium | **Do not add** a separate skill: `application-security` covers validation, limits and uploads, and #638 owns personal data | | PostgreSQL schema and migrations | the [business-site portal schema][bs-mig], wedding-app on [Drizzle migrations][wa-drizzle], KHoist's [CloudNativePG provider][khoist-cnpg] | none | schema design, query plans, lock-safe migrations | High | **Open.** The one vendor-maintained candidate was evaluated and not adopted ([#653](https://github.com/devantler-tech/agent-plugins/issues/653)); the candidate table says why. Each repository's own migration and tenant-isolation rules stay the authority | | PostgreSQL backup and restore | the platform's [CloudNativePG and Velero procedures][plat-dr] | none | none for a skill: the procedure is specific to one repository | High | **Do not add**: stays in the platform's docs and verifiers | | ClickHouse | only as [Coroot][plat-coroot]'s log and trace storage, which the platform runs but no product queries | none | none today | Low | **Do not add**. Revisit when a product writes ClickHouse schemas or queries | | Prometheus and OpenTelemetry | [Coroot][plat-coroot] with its bundled Prometheus, which Flagger and KEDA query in PromQL; [Alertmanager][plat-am]; an [OpenTelemetry log forwarder][plat-otel] | `promql` (writing and checking PromQL) and `prometheus` (alert and recording rules, Alertmanager routing); `gitops-cluster-debug` reads metrics only through the Flux MCP server | OpenTelemetry collector pipelines: the only vendor-maintained candidate defaults to a hosted service the platform does not use. Look collector configuration up through the `context7` documentation server | Medium | **Covered** for PromQL and alert rules since 7.3.0 ([#654](https://github.com/devantler-tech/agent-plugins/issues/654)); collector pipelines **not added** | | SLOs | the platform alerts on [Coroot's built-in per-application SLOs][plat-alert]; no repository sets its own SLO targets or error-budget policy | none | SLO targets and error-budget policy, which nobody authors today | Low | **Do not add**. Tuning Coroot's SLO alerts goes with #654. Revisit when a product sets its own SLO targets | | FinOps | the engineer's spend mandate (stewardship disabled by contract) | the [`agentic-engineer`](../plugins/agentic-engineering/agents/agentic-engineer.agent.md) agent and the `github-actions-efficiency` skill; the monorepo `finops` overlay is outside this repository | none | Medium | **Covered** | -| Payments, subscriptions and webhooks | no product takes payments: the code search `org:devantler-tech stripe` on 2026-11-04 returned 7 matches, all dependency metadata, a bookmark, game assets or a skill reference | none | none today | Low | **Do not add**. Webhook verification belongs to #640, subscription disclosures to #639. Revisit when a product commits to payments | +| Payments, subscriptions and webhooks | no product takes payments: the code search `org:devantler-tech stripe` on 2026-11-04 returned 7 matches, all dependency metadata, a bookmark, game assets or a skill reference | none | none today | Low | **Do not add**. Webhook verification is in `application-security`; subscription disclosures belong to #639. Revisit when a product commits to payments | | GitOps, Kubernetes and deployment | the [platform][plat-root], [ksail][ksail-go] and the templates | skills `gitops-knowledge`, `gitops-cluster-debug`, `gitops-tenant-onboarding`, `ksail` and `siderolabs`; the bundled Flux MCP server | activation only: 0 loads with at least 11 manifest sessions (#642) | High | **Covered**; measured by #651 | **Owned** means the audit hands the gap to an existing issue instead of filing an overlapping one. @@ -81,13 +81,18 @@ lists them: should-trigger, near-miss and over-trigger cases that #651's suite r | Observability | community skills such as [`opentelemetry`][c-bagel] and [`observability-sre`][c-arsenal] | MIT, individual maintainers | 2026-05-22, 2026-11-04 | General-purpose. The vendor-maintained Grafana skills cover the same scope | Rejected | | ClickHouse | [`clickhouse-best-practices`][c-ch] | Apache-2.0, ClickHouse | 2026-11-02 | Strong, but no product authors ClickHouse work | Do not add yet | | Payments | [`stripe-best-practices`][c-stripe] | MIT, Stripe | 2026-11-04 | Official, but no product takes payments and no provider is chosen | Do not add yet | -| Any gap | author a first-party skill | this repository | — | A maintained official candidate exists for every adopted gap | Rejected (#637's adoption-first workflow) | - -Candidates for privacy, Danish rules and application security will be compared in #638, #639 and -#640, as the note under the matrix says. One note for #640: [trailofbits/skills][c-tob] is -licensed CC-BY-SA-4.0. Its share-alike terms need the redistribution review that -[#581](https://github.com/devantler-tech/agent-plugins/issues/581) owns before anything is -bundled. +| Application security | [`security-review`, `security-model`][c-sentry] | Apache-2.0 repository, Sentry; the references of `security-review` are CC BY-SA 4.0 | 2026-11-05 | `security-review` reviews finished code by vulnerability class and is well made, but its references derive from OWASP material under share-alike terms, and it has no threat model, tenant design or webhook guidance. `security-model` writes a policy file for scanners and needs Python helpers | Rejected for now: the licence review is #581's. Revisit `security-review` for the review-time half when #581 clears share-alike redistribution | +| Application security | [`security-review`, `threat-model-analyst`][c-ac] | MIT, github/awesome-copilot | 2026-11-10 | The first is a scanner procedure that repeats the secret and dependency checks this plugin already has. The second is threat modelling only, as a multi-agent report generator of 17 files and 335 KB | Rejected | +| Application security | [`owasp-security`][c-agamm] | MIT, one maintainer | 2026-11-03 | Closest in content, with evaluation cases upstream. Its frontmatter uses a field the open skill format does not define, so the copy fails validation here unmodified, and its largest reference restates OWASP material under an MIT label | Rejected | +| Application security | [`VibeSec-Skill`][c-vibesec] | Apache-2.0, one maintainer | 2026-02-17 | One long secure-coding file. Its name is not a valid skill name, so it fails validation unmodified | Rejected | +| Application security | [the Trail of Bits collection][c-tob] | CC BY-SA 4.0, Trail of Bits | 2026-11-06 | Audit workflows for security researchers, not design guidance. Share-alike terms | Rejected for now: the licence review is #581's | +| Any gap | author a first-party skill | this repository | — | A maintained official candidate exists for every gap adopted so far. Application security is the one gap where none could be adopted, so `application-security` was built after the comparison above | Built only where the comparison rejects every candidate (#637's adoption-first workflow) | + +Candidates for privacy and Danish rules will be compared in #638 and #639, as the note under the +matrix says. The Sentry and [trailofbits/skills][c-tob] candidates above carry CC BY-SA 4.0 +terms. Share-alike redistribution needs the review that +[#581](https://github.com/devantler-tech/agent-plugins/issues/581) owns before anything under +those terms is bundled. ## Reusable skills and repository-specific instructions @@ -113,8 +118,8 @@ with SvelteKit and a Go server. An agent that trusts the card would choose `astr ([monorepo#4840](https://github.com/devantler-tech/monorepo/issues/4840)). Each adoption also adds description text, and #642 found 30 of 94 listed skills shown by name only. So every adoption PR reports how many description characters it adds. -2. Application security and tenant isolation (#640). High exposure, and the customer portal is in - active development. +2. Application security and tenant isolation (#640). Covered in 7.5.0 by the first-party + `application-security` skill. What remains is per product: the isolation tests themselves. 3. Privacy and GDPR (#638). Personal data in two products. 4. PostgreSQL schema and migrations (#653). Evaluated: the candidate was not adopted, so the gap stays open. @@ -150,6 +155,10 @@ when an adoption issue above closes, or after #651's first two baselines. [c-grafana]: https://github.com/grafana/skills/tree/8fb72a527961f05e708f53d2a0500709e12d7970/skills [c-stripe]: https://github.com/stripe/ai/tree/a3196df9925589ec7dd96c90692d13ea1c453cae [c-tob]: https://github.com/trailofbits/skills/tree/470419e16f97e112d01fc32f4898aea1755015d9 +[c-sentry]: https://github.com/getsentry/skills/tree/e94f22214528d69899d09c627b3315ddc3544816/skills +[c-ac]: https://github.com/github/awesome-copilot/tree/227949a7542a55d2155c00e7cdb998d9a0fee4f3/skills +[c-agamm]: https://github.com/agamm/claude-code-owasp/tree/1ae29cb22518560e3625d77f2ae64f61647d9201/.claude/skills/owasp-security +[c-vibesec]: https://github.com/BehiSecc/VibeSec-Skill/tree/0590993b35ad51961f65a4d01cf1196dfead05bb [c-ch-pg]: https://github.com/ClickHouse/agent-skills/tree/d3b2b5ba43a8b80c2d08858b68d53edc701b5ace/skills/infra-postgres [c-bagel]: https://github.com/bagelhole/devops-security-agent-skills/tree/0365f57a079b1332f95cf26e31dd2d5332a8399f/devops/observability/opentelemetry [c-arsenal]: https://github.com/majiayu000/spellbook/tree/238e07625aeca5bcf0d6f7cb3e2d07f94c717194/skills/observability-sre diff --git a/plugins/agentic-engineering/.claude-plugin/plugin.json b/plugins/agentic-engineering/.claude-plugin/plugin.json index d699612..fd7df2f 100644 --- a/plugins/agentic-engineering/.claude-plugin/plugin.json +++ b/plugins/agentic-engineering/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "agentic-engineering", "description": "The autonomous engineering system for repository portfolios and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; portfolio, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use; configured by the consumer AGENTS.md", - "version": "7.4.0", + "version": "7.5.0", "author": { "name": "devantler-tech", "url": "https://github.com/devantler-tech" diff --git a/plugins/agentic-engineering/.codex-plugin/plugin.json b/plugins/agentic-engineering/.codex-plugin/plugin.json index bb762f2..00c64e1 100644 --- a/plugins/agentic-engineering/.codex-plugin/plugin.json +++ b/plugins/agentic-engineering/.codex-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "agentic-engineering", "description": "The autonomous engineering system for repository portfolios and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; portfolio, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use; configured by the consumer AGENTS.md", - "version": "7.4.0", + "version": "7.5.0", "author": { "name": "devantler-tech", "url": "https://github.com/devantler-tech" diff --git a/plugins/agentic-engineering/CHANGELOG.md b/plugins/agentic-engineering/CHANGELOG.md index aae6308..4e5fe69 100644 --- a/plugins/agentic-engineering/CHANGELOG.md +++ b/plugins/agentic-engineering/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog — `agentic-engineering` +## 7.5.0 — 2026-11-10 + +**Added** — `application-security`, a first-party skill for the security decisions scanners cannot +make: who may read or change which record, how tenants are kept apart, and how to prove both +before code ships. It holds a fifteen-minute threat model with a filled example, the three levels +of access control, tenant isolation in the data layer and everywhere else data rests (with +PostgreSQL row-level security as a second line and its limits), webhook verification, uploads, +request limits and outbound requests, and the two-tenant test design with runnable examples in +Go and TypeScript. It names when to stop and escalate, and it sends scanning, pipeline hardening, +cluster tenancy and incident response to the skills that own them. + +Seven upstream candidates were compared first and none could be adopted: each was a code scanner +or a review procedure, failed this marketplace's format validation, or carries share-alike terms +whose redistribution review is still open (#581). The comparison is on the coverage page (#640). + +Every example was run before release: the Go and TypeScript tests fail when the tenant check is +removed, and the row-level security statements were checked on PostgreSQL 17.11. + ## 7.4.0 — 2026-11-10 **Added** — `skill-lifecycle`, a first-party skill: how to decide whether to adopt, build or reject diff --git a/plugins/agentic-engineering/README.md b/plugins/agentic-engineering/README.md index aa3f482..7d2f0a6 100644 --- a/plugins/agentic-engineering/README.md +++ b/plugins/agentic-engineering/README.md @@ -114,6 +114,7 @@ are maintained here. | GitHub | `gh-cli`, `github-issues`, `gh-stack` (stacked pull requests), `github-actions-docs`, `github-actions-efficiency` | | Code review (CodeRabbit) | `code-review` (a local review with the CodeRabbit CLI), `autofix` (work through CodeRabbit's review threads on a pull request) | | Repository security | `dependency-scanning`, `dependabot`, `secret-scanning`, `codeql`, `github-actions-hardening`, `check-skills` (review a skill before installing it) | +| Application security | `application-security` (a short threat model, access rules, tenant isolation, webhooks, uploads and limits, each with the test that proves it) | | Library documentation | `context7-mcp` (current documentation for any library or framework, through the `context7` server) | | Go | `golang-pro` | | Kubernetes and GitOps | `gitops-cluster-debug`, `gitops-knowledge`, `gitops-tenant-onboarding`, `siderolabs` (Talos Linux and Omni) | diff --git a/plugins/agentic-engineering/plugin.json b/plugins/agentic-engineering/plugin.json index d699612..fd7df2f 100644 --- a/plugins/agentic-engineering/plugin.json +++ b/plugins/agentic-engineering/plugin.json @@ -1,7 +1,7 @@ { "name": "agentic-engineering", "description": "The autonomous engineering system for repository portfolios and everything its agents need — engineer, read-only surveyor, meta-engineer and Flux troubleshooter agents; portfolio, product, spend and improvement workflows; GitHub, CodeRabbit review, security, library documentation, Go, Kubernetes, Flux, Talos, SvelteKit frontend, Stripe payments, engineering-practice, design, product-management, marketing, finance and productivity skills; and the MCP servers they use; configured by the consumer AGENTS.md", - "version": "7.4.0", + "version": "7.5.0", "author": { "name": "devantler-tech", "url": "https://github.com/devantler-tech" diff --git a/plugins/agentic-engineering/skills/application-security/SKILL.md b/plugins/agentic-engineering/skills/application-security/SKILL.md new file mode 100644 index 0000000..0dc38c0 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/SKILL.md @@ -0,0 +1,111 @@ +--- +name: application-security +description: Design and test access rules for a web app or API - a short threat model, server-side authorization, tenant isolation, webhook verification, uploads and rate limits, each proved by a test that fails when the control is removed. Use when adding or changing an endpoint, form, sign-in, roles, tenant or customer data access, a webhook receiver or a file upload. Not for scanner setup or CI hardening. +license: Apache-2.0 +--- + +# Application security + +A scanner finds known bad patterns in code. It cannot tell that an endpoint hands one customer's +invoice to another, because that code is valid: it is only missing a rule. This skill is about +those rules. They are decided when a change is designed and proved when it is tested. + +## What must not go wrong + +1. **The server decides access on every request.** A hidden button, a check in the browser or + an identifier that is hard to guess is not access control. +2. **For a user's request, the tenant comes from the verified session.** Never from the URL, + a header, a query parameter or the request body. A background job carries the caller and + tenant it was queued with and re-checks access when it runs. A webhook is verified first, + and its tenant is looked up in your own records. +3. **Deny by default.** A route, field or action that nobody allowed is refused. A check that + cannot run refuses. +4. **Every control has a test that fails without it.** A test that still passes after the check + is deleted proves nothing. +5. **Data that crossed a tenant boundary is an incident, not a bug.** Stop and + [escalate](#escalate-do-not-decide-alone). + +## When to use it, and when not + +Use it when a change: + +- adds or alters an endpoint, route handler, server action or background job that reads or + writes data; +- alters sign-in, sessions, tokens, roles or permissions; +- touches data that belongs to a tenant, a customer or a user; +- receives a webhook, accepts a file upload or adds a public form; +- adds a place where data rests: a table, a bucket, a cache, a search index, a queue. + +Other work has its own skill, when it is installed: + +| Task | Use instead | +|---|---| +| Workflow files and CI permissions | `github-actions-hardening` | +| Code scanning, secret scanning, vulnerable dependencies | `codeql`, `secret-scanning`, `dependency-scanning`, `dependabot` | +| A tenant's namespace, network policy and secrets on a cluster | `gitops-tenant-onboarding` | +| One payment provider's own flow and webhook format | that provider's skill, such as `stripe-best-practices` | +| Something is being exploited or is leaking right now | `incident-response` first, then come back | + +A repository's own instructions say which roles and tenants it has, and who is accountable. +They win over this skill wherever they are more specific. + +## The order of work + +1. **Model the change.** Fifteen minutes, not a document project. Answer four questions: what + are we building, what can go wrong, what will we do about it, and how will we know it + worked. The [threat model guide](references/threat-model.md) has the prompts and a filled + example. The result is a short list of threats, each with a control, the name of the + person who accepted the risk, or the name of the person who is asked to decide. +2. **Decide who may do what.** Three questions for every operation: may this caller use this + function, may they touch this record, and may they read or set these fields. Sign-in and + sessions come first. See [access control](references/access-control.md). +3. **Keep tenants apart.** The tenant is on every row and in every query, and in every other + place data rests: caches, files, jobs, search, exports and logs. See + [tenant isolation](references/tenant-isolation.md). +4. **Guard the edges.** Input validation, webhooks, uploads, request limits and requests the + server makes on a user's behalf. See [edges](references/edges.md). +5. **Prove it.** Two tenants in every test run. For each route: the owner succeeds; the other + tenant is refused and learns nothing; a write by the other tenant leaves the record + unchanged; a lower role is refused; and a caller without a session is refused. A route + that is public on purpose (a webhook receiver, a public form) is on a short reviewed + list instead, and is tested for its own control: the signature, the limit. Remove + the control once and watch the test fail. See + [isolation tests](references/isolation-tests.md). +6. **Record it in the pull request.** The threats you considered, the control for each, the + test that proves it, and any risk that was accepted and by whom. + +## Escalate, do not decide alone + +Stop and bring in the accountable person when: + +- data crossed a tenant boundary in a running system, or may have. Treat it as an incident: + keep the logs, do not fix it quietly. If personal data is involved, a legal reporting + deadline may already be running (72 hours under the GDPR); the person accountable for privacy + decides, not the engineer; +- a control cannot be built as designed and the choice is to ship without it; +- the change needs a new way to cross tenants: support access, an administrator acting as a + user, a report across customers; +- sign-in or password handling is being written instead of delegated to an identity provider; +- you find a secret in code, logs or history. Rotate it first, then clean up. + +An accepted risk is written down: who accepted it, why, and when it is looked at again. An +agent never accepts a risk on someone's behalf, and never states that a system is compliant or +secure. It states what was checked and what the tests prove. + +## Standards behind this skill + +Checked 2026-11-10 against OWASP ASVS 5.0.0 (released 2025-05-30) and the OWASP API Security +Top 10 2023. Cite them by chapter and item in a review; read the standard for the exact +requirement. + +| Topic | ASVS 5.0.0 | API Security Top 10 2023 | +|---|---|---| +| Access rules | V8 Authorization (tenants: 8.4.1) | API1 object level, API3 property level, API5 function level | +| Sign-in and sessions | V6 Authentication, V7 Session Management, V9 Self-contained Tokens, V10 OAuth and OIDC | API2 Broken Authentication | +| Input and business rules | V2 Validation and Business Logic | API6 Unrestricted Access to Sensitive Business Flows | +| Request limits | V2.4 Anti-automation | API4 Unrestricted Resource Consumption | +| APIs and webhooks | V4 API and Web Service | API10 Unsafe Consumption of APIs | +| Uploads and downloads | V5 File Handling | | +| Requests made for a user | V1.3 Sanitization (1.3.6) | API7 Server Side Request Forgery | +| Configuration and secrets | V13 Configuration | API8 Security Misconfiguration, API9 Improper Inventory Management | +| Logs and errors | V16 Security Logging and Error Handling | | diff --git a/plugins/agentic-engineering/skills/application-security/references/access-control.md b/plugins/agentic-engineering/skills/application-security/references/access-control.md new file mode 100644 index 0000000..5ac44a3 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/access-control.md @@ -0,0 +1,64 @@ +# Access control + +Who the caller is, then what they may do. Get the first wrong and the second does not matter. + +## Sign-in and sessions + +- **Delegate sign-in to an identity provider** over OpenID Connect. Do not store passwords or + write a reset flow yourself. If the product must, that is an escalation, not a task. +- **Verify every token on the server**: signature, issuer, audience and expiry, with a library. + Pin the accepted algorithms. A token that was only decoded was not verified. +- **Keep the session in a cookie the page's scripts cannot read**: `HttpOnly`, `Secure`, + `SameSite=Lax` or `Strict`, and the `__Host-` name prefix when the site allows it. Do not + put tokens in browser storage. +- **Change the session identifier at sign-in and when rights change.** End the session on the + server at sign-out, not only in the browser. +- **A cookie session needs protection against cross-site requests.** Use the framework's own + mechanism and keep it switched on. Requests that change state are never `GET`. +- **Take identity from the verified session and nowhere else.** A user identifier, role or + tenant in a header, query or body is input from the caller, not a fact. + +## Three levels of authorization + +Check all three, on the server, for every operation. + +| Level | The question | The common failure | +|---|---|---| +| Function | May this caller use this operation at all? | An administrator's route that only the menu hides | +| Object | May this caller touch this record? | Reading or changing a record by identifier without checking whose it is | +| Property | May this caller read or set these fields? | A request body bound straight onto a record, so the caller sets `role`, `tenant_id` or `price`; a response that returns every column | + +Rules that keep them right: + +- **Deny by default.** A new route is closed until a rule opens it. Put the check where a route + cannot be added without one: middleware that refuses routes with no declared policy, or a + data layer that cannot be called without the caller. +- **Check close to the data.** A check in the handler is forgotten by the next handler. A data + layer that takes the caller on every call cannot be. +- **Accept fields by name.** List the fields a caller may set; ignore or refuse the rest. List + the fields a response returns; do not serialise the stored record. +- **Decide from current stored facts.** The record's owner and the caller's role come from + the database and the session, never from the request. A role or membership kept in a + session goes stale: when someone is demoted or removed, either read the current role on + each request or end every session that person has. Changing one session's identifier + does not update the others. +- **Give one answer for "missing" and "not yours"** when the identifier is not public, so + identifiers cannot be probed. Pick "not found" or "forbidden" and use it for both. +- **Re-check in background work.** A job or message handler acts with the rights of whoever + queued it. Carry the caller and tenant with the job and check again when it runs. +- **Administrators are callers too.** Staff tools and "act as this user" features need their + own rules, their own log and usually a second person. Adding one is an escalation. + +## Fail closed, and say little + +- When the access check cannot run (the database is down, the policy is missing), refuse. +- An error shown to a caller names no table, no query, no stack and no other user's data. +- Record refusals and sensitive actions: who, which tenant, which operation, which record, + the outcome. Never the secret, the token or the personal data itself. +- A burst of refusals for one caller is a signal. Make it visible to whoever watches the system. + +## Secrets + +Secrets come from the platform's secret store at run time, never from the repository, a build +argument or a log line. Finding one is an escalation: rotate first. Scanning for them is the +`secret-scanning` skill's subject, not this one's. diff --git a/plugins/agentic-engineering/skills/application-security/references/edges.md b/plugins/agentic-engineering/skills/application-security/references/edges.md new file mode 100644 index 0000000..56c8366 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -0,0 +1,438 @@ +# Edges: input, webhooks, uploads, limits and outbound requests + +The places where a party you do not control hands you bytes, or makes your server act. + +## Input from a form or an API + +- **Validate on the server**, against a schema: type, length, range and allowed values for + every field. Validation in the browser is a convenience for the user, not a control. +- **Refuse what the schema does not name.** Unknown fields are dropped or rejected, never + passed through to the record. +- **Check the business rule, not only the shape.** A well-formed quantity of minus one, a + date in the past or a price sent by the client are valid input and wrong requests. +- **Keep data and code apart.** Queries take parameters; never build SQL, a shell command or + a template from input. Encode on output for the place it lands (HTML, a URL, a header). +- **Answer a bad request with what was wrong with the input**, and nothing about the system. + +## Receiving a webhook + +A webhook endpoint is public. Anyone can post to it, so the signature is the only thing that +makes a delivery trustworthy. + +- **Use the provider's own library and documented scheme** when there is one. Header names, + the signed string and the tolerance differ between providers. +- **Verify over the raw body**, the exact bytes received, before any parsing. A body that was + parsed and re-serialised no longer matches its signature. +- **Compare in constant time** with the standard library's function for it. Never `==`. +- **Refuse old deliveries.** A signed timestamp with a tolerance of a few minutes stops a + captured delivery being replayed later. A sender that retries has to sign each attempt + afresh, with the same identifier and a new timestamp. If a provider resends the original + signature instead, use the tolerance its documentation gives, not a guess. +- **Deduplicate on an identifier that is inside the signed content**: in the signed body, or + in a header the signature covers. An identifier outside the signature can be changed on a + captured delivery, which then counts as new. +- **Handle each event once, and never lose one.** Providers redeliver. Record the event + identifier in the same transaction as its effect, so a failed effect is retried and a + repeated delivery is answered with success without acting again. +- **Trust the event only as far as needed.** Prefer to take the identifier from the event and + fetch the current state from the provider, rather than acting on amounts in the body. +- **Answer quickly** and do slow work in a job. **Limit the body size.** Keep the secret in the + secret store, one per endpoint, and accept the current and the previous secret while one + is being rotated. Refuse to start when a secret is missing or empty: anyone can sign with + an empty key. +- **Map the event to a tenant from your own records**, such as the stored customer identifier. + Never from a tenant field in the body. + +The properties above, in Go. Checked with Go 1.27. The scheme signs the identifier, the +timestamp and the body together; it is an example, not any provider's: + +```go +package webhook + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "database/sql" + "encoding/hex" + "errors" + "io" + "net/http" + "regexp" + "strconv" + "time" +) + +var ( + ErrStale = errors.New("webhook timestamp outside the tolerance") + ErrSignature = errors.New("webhook signature does not match") + ErrNoSecret = errors.New("webhook needs at least one secret, and none may be empty") +) + +// Verify checks a delivery signed as +// hex(HMAC-SHA256(secret, eventID + "." + timestamp + "." + body)). +// The identifier and the timestamp are inside the signature, so neither can be +// swapped on a captured delivery. Several secrets may be valid at once, so a +// secret can be rotated without dropping deliveries. +// +// A sender that retries signs each attempt afresh: the same identifier, a new +// timestamp. Providers differ in header names, layout and how they retry: +// follow the provider's documentation, and use its library when it has one. +func Verify(secrets [][]byte, eventID, timestamp string, body []byte, signature string, now time.Time, tolerance time.Duration) error { + seconds, err := strconv.ParseInt(timestamp, 10, 64) + if err != nil { + return ErrStale + } + age := now.Sub(time.Unix(seconds, 0)) + if age > tolerance || age < -tolerance { + return ErrStale + } + given, err := hex.DecodeString(signature) + if err != nil { + return ErrSignature + } + for _, secret := range secrets { + if len(secret) == 0 { + continue // anyone can sign with an empty key + } + mac := hmac.New(sha256.New, secret) + mac.Write([]byte(eventID + "." + timestamp + ".")) + mac.Write(body) + if hmac.Equal(given, mac.Sum(nil)) { // constant-time comparison + return nil + } + } + return ErrSignature +} + +// Deliveries applies each event at most once. +type Deliveries interface { + // Once runs effect unless eventID was handled before. The record of the + // identifier and everything effect writes go through the one transaction + // it is given, so they commit together or not at all, and two deliveries + // of one event at the same moment cannot both run. It reports whether + // effect ran. + Once(ctx context.Context, eventID string, effect func(tx *sql.Tx) error) (ran bool, err error) +} + +// Handle applies one verified event. It writes only through tx. +type Handle func(ctx context.Context, tx *sql.Tx, eventID string, body []byte) error + +const maxBody = 1 << 20 // 1 MiB + +// The identifier is part of the signed string, so it may not contain the +// separator, and it is bounded. +var validEventID = regexp.MustCompile(`^[A-Za-z0-9_-]{1,128}$`) + +// NewHandler refuses to start without a usable secret: a missing configuration +// value must not become a key that anyone can sign with. +func NewHandler(secrets [][]byte, seen Deliveries, handle Handle) (http.Handler, error) { + if len(secrets) == 0 { + return nil, ErrNoSecret + } + for _, secret := range secrets { + if len(secret) == 0 { + return nil, ErrNoSecret + } + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Read the exact bytes that were signed, before any parsing. + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxBody)) + if err != nil { + http.Error(w, "body too large", http.StatusRequestEntityTooLarge) + return + } + eventID := r.Header.Get("X-Event-Id") + if !validEventID.MatchString(eventID) { + http.Error(w, "invalid event id", http.StatusBadRequest) + return + } + err = Verify(secrets, eventID, r.Header.Get("X-Event-Timestamp"), body, + r.Header.Get("X-Event-Signature"), time.Now(), 5*time.Minute) + if err != nil { + http.Error(w, "invalid signature", http.StatusUnauthorized) + return + } + _, err = seen.Once(r.Context(), eventID, func(tx *sql.Tx) error { + return handle(r.Context(), tx, eventID, body) + }) + if err != nil { + // Nothing was recorded, so the provider's next delivery is handled. + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) // handled now, or already handled + }), nil +} +``` + +Its tests. The last three steps of `TestHandler` are the ones most often missing: an effect +that fails must not be recorded as handled, or the provider's retry is swallowed and the event +is lost. That guarantee belongs to the `Deliveries` implementation: it records the identifier +and runs the effect in one transaction, which is why the effect is handed that transaction +and writes only through it. `deliveriesContract` is written to be run against the real, +database-backed implementation as well, including twenty deliveries of one event at once. + +```go +package webhook + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "database/sql" + "encoding/hex" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" +) + +var ( + current = []byte("current-secret") + previous = []byte("previous-secret") + secrets = [][]byte{current, previous} +) + +func sign(key []byte, eventID, timestamp string, body []byte) string { + mac := hmac.New(sha256.New, key) + mac.Write([]byte(eventID + "." + timestamp + ".")) + mac.Write(body) + return hex.EncodeToString(mac.Sum(nil)) +} + +func TestVerify(t *testing.T) { + now := time.Unix(1_700_000_000, 0) + fresh := strconv.FormatInt(now.Unix(), 10) + old := strconv.FormatInt(now.Add(-6*time.Minute).Unix(), 10) + ahead := strconv.FormatInt(now.Add(6*time.Minute).Unix(), 10) + body := []byte(`{"type":"invoice.paid"}`) + + cases := []struct { + name string + secrets [][]byte + eventID string + body []byte + timestamp string + signature string + want error + }{ + {"valid delivery", secrets, "evt-1", body, fresh, sign(current, "evt-1", fresh, body), nil}, + {"signed with the previous secret during rotation", secrets, "evt-1", body, fresh, sign(previous, "evt-1", fresh, body), nil}, + {"changed body", secrets, "evt-1", []byte(`{"type":"invoice.void"}`), fresh, sign(current, "evt-1", fresh, body), ErrSignature}, + {"changed identifier", secrets, "evt-2", body, fresh, sign(current, "evt-1", fresh, body), ErrSignature}, + {"unknown secret", secrets, "evt-1", body, fresh, sign([]byte("other"), "evt-1", fresh, body), ErrSignature}, + {"an empty secret never verifies", [][]byte{current, {}}, "evt-1", body, fresh, sign(nil, "evt-1", fresh, body), ErrSignature}, + {"signature is not hex", secrets, "evt-1", body, fresh, "not-hex", ErrSignature}, + {"missing signature", secrets, "evt-1", body, fresh, "", ErrSignature}, + {"replayed after the tolerance", secrets, "evt-1", body, old, sign(current, "evt-1", old, body), ErrStale}, + {"timestamp in the future", secrets, "evt-1", body, ahead, sign(current, "evt-1", ahead, body), ErrStale}, + {"timestamp is not a number", secrets, "evt-1", body, "soon", sign(current, "evt-1", "soon", body), ErrStale}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Verify(tc.secrets, tc.eventID, tc.timestamp, tc.body, tc.signature, now, 5*time.Minute) + if !errors.Is(got, tc.want) { + t.Fatalf("Verify = %v, want %v", got, tc.want) + } + }) + } +} + +func TestNewHandlerRefusesMissingSecrets(t *testing.T) { + for name, bad := range map[string][][]byte{"none": nil, "one empty": {current, {}}} { + if _, err := NewHandler(bad, &memoryDeliveries{}, nil); !errors.Is(err, ErrNoSecret) { + t.Errorf("%s: NewHandler error = %v, want ErrNoSecret", name, err) + } + } +} + +// memoryDeliveries stands in for a table with a unique event identifier, +// written in the same transaction as the effect. It has no database, so it +// hands the effect a nil transaction; the lock stands in for the row lock. +type memoryDeliveries struct { + mu sync.Mutex + done map[string]bool +} + +func (m *memoryDeliveries) Once(_ context.Context, eventID string, effect func(tx *sql.Tx) error) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + if m.done[eventID] { + return false, nil + } + if err := effect(nil); err != nil { + return false, err // not recorded: the next delivery tries again + } + if m.done == nil { + m.done = map[string]bool{} + } + m.done[eventID] = true + return true, nil +} + +func TestHandler(t *testing.T) { + handled, failNext := 0, false + handle := func(context.Context, *sql.Tx, string, []byte) error { + if failNext { + failNext = false + return errors.New("database unavailable") + } + handled++ + return nil + } + handler, err := NewHandler(secrets, &memoryDeliveries{}, handle) + if err != nil { + t.Fatal(err) + } + body := `{"type":"invoice.paid"}` + timestamp := strconv.FormatInt(time.Now().Unix(), 10) + + deliver := func(signature, eventID string) int { + req := httptest.NewRequest(http.MethodPost, "/webhooks/payments", strings.NewReader(body)) + req.Header.Set("X-Event-Id", eventID) + req.Header.Set("X-Event-Timestamp", timestamp) + req.Header.Set("X-Event-Signature", signature) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + return recorder.Code + } + expect := func(step string, got, want, wantHandled int) { + t.Helper() + if got != want || handled != wantHandled { + t.Fatalf("%s: status = %d (want %d), handled = %d (want %d)", step, got, want, handled, wantHandled) + } + } + + first := sign(current, "evt-1", timestamp, []byte(body)) + expect("forged delivery", deliver("00", "evt-1"), http.StatusUnauthorized, 0) + expect("first delivery", deliver(first, "evt-1"), http.StatusOK, 1) + expect("redelivery", deliver(first, "evt-1"), http.StatusOK, 1) + // A captured delivery sent again under a new identifier must not count as new. + expect("replay under a new identifier", deliver(first, "evt-2"), http.StatusUnauthorized, 1) + expect("identifier with a separator", deliver(first, "evt.1"), http.StatusBadRequest, 1) + expect("missing identifier", deliver(first, ""), http.StatusBadRequest, 1) + + // A failed effect must not be recorded as handled, or the event is lost. + second := sign(current, "evt-3", timestamp, []byte(body)) + failNext = true + expect("effect fails", deliver(second, "evt-3"), http.StatusInternalServerError, 1) + expect("provider retries", deliver(second, "evt-3"), http.StatusOK, 2) + expect("and retries again", deliver(second, "evt-3"), http.StatusOK, 2) +} + +// deliveriesContract is what every Deliveries implementation must pass. The +// handler relies on it, so run it against the database-backed one as well. +func deliveriesContract(t *testing.T, seen Deliveries) { + t.Helper() + ctx := context.Background() + if ran, err := seen.Once(ctx, "evt-c", func(*sql.Tx) error { return errors.New("failed") }); ran || err == nil { + t.Fatalf("a failed effect: ran = %v, err = %v", ran, err) + } + calls := 0 + effect := func(*sql.Tx) error { calls++; return nil } + if ran, err := seen.Once(ctx, "evt-c", effect); !ran || err != nil { + t.Fatalf("after a failure the event must run: ran = %v, err = %v", ran, err) + } + if ran, err := seen.Once(ctx, "evt-c", effect); ran || err != nil { + t.Fatalf("a handled event must not run again: ran = %v, err = %v", ran, err) + } + if calls != 1 { + t.Fatalf("the effect ran %d times, want once", calls) + } + + // The same event delivered many times at once still runs once. The effect + // takes a moment, so an implementation that does not hold its lock or + // transaction across the effect lets the other deliveries through. + var concurrent, reported atomic.Int32 + var wait sync.WaitGroup + for range 20 { + wait.Add(1) + go func() { + defer wait.Done() + ran, err := seen.Once(ctx, "evt-d", func(*sql.Tx) error { + concurrent.Add(1) + time.Sleep(20 * time.Millisecond) + return nil + }) + if err != nil { + t.Errorf("a concurrent delivery failed: %v", err) + } + if ran { + reported.Add(1) + } + }() + } + wait.Wait() + if concurrent.Load() != 1 || reported.Load() != 1 { + t.Fatalf("20 deliveries at once: the effect ran %d times and %d calls reported running it, want 1 and 1", + concurrent.Load(), reported.Load()) + } +} + +func TestMemoryDeliveriesContract(t *testing.T) { + deliveriesContract(t, &memoryDeliveries{}) +} +``` + +If recording the identifier and applying the effect cannot share a transaction, apply the +effect first, make it safe to repeat, and record the identifier after it succeeded. + +## Accepting a file + +- **Limit the size before reading**, at the server and at the proxy in front of it. +- **Decide the type from the content**, against a short allow-list. The file name and the + `Content-Type` header are the uploader's claims. Images are safest re-encoded. +- **Choose the stored name yourself**: a random identifier under the tenant's prefix. Keep the + original name only as text in the database. +- **Store outside anything the web server serves or executes.** Serve downloads through a + handler that runs the access check, with `Content-Disposition: attachment` and + `X-Content-Type-Options: nosniff`, or through a short-lived signed link created after the + check. A signed link needs the same two headers on the storage response, or storage on a + separate origin that shares no cookies with the application: an upload opened in the + application's own origin can run as script. +- **Never unpack, render or convert an upload in the request process** without limits on + time, size and file count. Archives and documents are programs for a parser. +- **Tie the upload to its tenant and uploader** in the database, and delete the file when the + record goes. + +## Limits + +Every public operation has a ceiling: a caller cannot make it arbitrarily slow, large or +frequent. + +- **Requests**: a limit per signed-in user or tenant, and per address for anonymous routes. + Stricter for sign-in, password reset, invitation and anything that sends a message or costs + money. Refuse with status 429 and a `Retry-After` header. +- **Sizes**: a maximum body size, page size, upload size, and number of items in a batch. + A list endpoint always pages, with a server-side maximum. +- **Work**: a timeout on every query and outbound call. Expensive operations go to a queue + with a per-tenant cap, so one tenant cannot starve the others. +- **Enumeration**: sign-in, reset and invitation answer the same way whether or not the + account exists. + +A limit kept in one process's memory does not hold once there are two processes. Say where the +count lives. + +## Requests the server makes for a user + +When a user supplies a URL (an avatar address, a webhook target, an import source), the server +becomes their proxy into your network. + +- Allow only `https`, and only the hosts or patterns the feature needs. +- Resolve the name, refuse every address that is not public (private, loopback, link-local, + unspecified such as `0.0.0.0` and `::`, and your own internal ranges), and connect to the + address you checked. Do not follow redirects without re-checking. +- Set a timeout and a response size limit, and do not return the raw response or error to the + user. + +## Defaults + +A new route is closed, a new bucket is private, a new flag is off, a new role has no rights. +Error pages and debug output are for development builds only. An endpoint that is no longer +used is removed, not left in place. diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md new file mode 100644 index 0000000..800effd --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -0,0 +1,524 @@ +# Isolation tests + +A control that is not tested will be removed by a refactor without anyone noticing. These +tests are the enforcement of everything else in this skill. They run in CI on every change. + +## What the tests must show + +Create **two tenants** in every run, each with its own users and its own records. Then, for +every route, action and job that touches tenant data: + +| Case | Expect | +|---|---| +| The owner uses their own record | Success. This is the positive control: without it, a route that is simply broken passes every other case | +| The other tenant uses the same identifier | Refused, with the same answer as for a missing record, and nothing of the owner's in the body | +| No session, or a session the server does not know | Refused, on every route that needs a session. A route that is public on purpose is tested for its own control instead | +| The other tenant lists, searches, counts or exports | Only their own records, and counts that do not include the owner's | +| The other tenant writes: update, delete, attach, move | Refused, **and the owner's record is unchanged afterwards**. Read it back as the owner | +| A request body names another tenant, owner or role | The field is ignored or refused; the stored record keeps its tenant | +| A lower role calls a higher role's operation | Refused | + +Rules that keep the tests honest: + +- **Go through the real entry point**: the HTTP handler or the server action, with the real + routing and the real session check. A test that sets the caller directly skips the layer + where the check usually lives. +- **Use the real data layer and a test database** wherever the query is the control. A + hand-written fake that filters correctly proves nothing about the SQL. +- **Prove each test once.** Delete the control in the product code (drop the tenant + condition) and watch the test fail. If it still passes, it tests nothing. Put the proof in + the pull request. +- **Cover every route by construction.** Keep the routes where a test can list them (the + router's own listing, or one route table as in the example below), and fail for any route + that has no isolation case and is not on a short, reviewed list of public routes. A new + endpoint then cannot be added without its test. +- **Test the other places data rests**: the cache, the job queue, file downloads and exports + each get the same two-tenant case. + +## Limits and cross-site requests + +- **A request limit**: configure a low limit in the test, send one request more than it + allows, and expect status 429 with a `Retry-After` header. Send the same number as another + user and expect success, so the test shows the limit is per caller. Use a clock the test + controls rather than sleeping. +- **Cross-site protection** on a route that changes state with a cookie session: send the + request with a valid session but without the token (or with another site's `Origin`), and + expect a refusal with the record unchanged. Then send it correctly and expect success. + +## Go + +A session check, a route table in which every route names its role or is marked public, a +data layer that cannot be called without the tenant, and the tests. Checked with Go 1.27. The +module must declare `go 1.22` or later: the route patterns and the range over an integer +need it. Each of these makes a test fail: looking an invoice up by identifier alone in +`MemoryStore.Get` or `MemoryStore.Delete`, making `allowed` return true, letting an admin +through a route whose policy is missing or misspelt, adding a route +without its kinds of case, dropping a route's role case, marking a route public without +adding it to the reviewed list, a delete that answers success without deleting, and a +refused delete that alters the record. + +```go +package invoices + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "sync" +) + +// Invoice belongs to exactly one tenant. The tenant is never sent to the client. +type Invoice struct { + ID string `json:"id"` + TenantID string `json:"-"` + Total int `json:"total"` +} + +var ErrNotFound = errors.New("invoice not found") + +// Store takes the tenant on every call. No method works on an ID alone, so a +// handler cannot forget the tenant. +type Store interface { + Get(ctx context.Context, tenantID, id string) (Invoice, error) + Delete(ctx context.Context, tenantID, id string) error +} + +// MemoryStore is this example's data layer. In a product it is SQL, and each +// lookup below is `WHERE tenant_id = $1 AND id = $2`. +type MemoryStore struct { + mu sync.Mutex + invoices map[key]Invoice +} + +// key is the tenant and the identifier together, as in a table whose primary +// key is (tenant_id, id): two tenants can never collide. +type key struct{ tenantID, id string } + +func NewMemoryStore(invoices ...Invoice) *MemoryStore { + store := &MemoryStore{invoices: map[key]Invoice{}} + for _, invoice := range invoices { + store.invoices[key{invoice.TenantID, invoice.ID}] = invoice + } + return store +} + +func (s *MemoryStore) Get(_ context.Context, tenantID, id string) (Invoice, error) { + s.mu.Lock() + defer s.mu.Unlock() + invoice, ok := s.invoices[key{tenantID, id}] + if !ok { + return Invoice{}, ErrNotFound + } + return invoice, nil +} + +func (s *MemoryStore) Delete(_ context.Context, tenantID, id string) error { + s.mu.Lock() + defer s.mu.Unlock() + if _, ok := s.invoices[key{tenantID, id}]; !ok { + return ErrNotFound + } + delete(s.invoices, key{tenantID, id}) + return nil +} + +// Caller is who the verified session says is calling. +type Caller struct { + TenantID string + Role string // "member" or "admin" +} + +// Sessions resolves a session cookie to a caller. A product verifies the +// session here and reads the caller's current tenant membership and role, so a +// removed member or a demoted admin loses access on the next request. +type Sessions interface { + Caller(sessionID string) (Caller, bool) +} + +type callerKey struct{} + +// Authenticate is the only place a caller is set. A request without a valid +// session never reaches a handler. +func Authenticate(sessions Sessions, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cookie, err := r.Cookie("__Host-session") + if err != nil { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + caller, ok := sessions.Caller(cookie.Value) + if !ok || caller.TenantID == "" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), callerKey{}, caller))) + }) +} + +// Route is one entry of the route table. Every route names the role it needs, +// or "public", so a route cannot be added without a policy, and a test can +// list them all. +type Route struct { + Pattern string + Role string + Handle func(store Store, caller Caller, w http.ResponseWriter, r *http.Request) +} + +var Routes = []Route{ + {"GET /invoices/{id}", "member", getInvoice}, + {"DELETE /invoices/{id}", "admin", deleteInvoice}, + {"GET /healthz", "public", health}, +} + +// allowed is closed by default: a route whose policy is missing or misspelt +// admits nobody, not even an admin. +func allowed(caller Caller, needed string) bool { + switch needed { + case "member": + return caller.Role == "member" || caller.Role == "admin" + case "admin": + return caller.Role == "admin" + default: + return false + } +} + +func Handler(store Store, sessions Sessions) http.Handler { + mux := http.NewServeMux() + for _, route := range Routes { + mux.Handle(route.Pattern, serve(store, sessions, route)) + } + return mux +} + +// serve builds the handler for one route. A public route has its own control +// (a signature check, a request limit) instead of a session. +func serve(store Store, sessions Sessions, route Route) http.Handler { + if route.Role == "public" { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + route.Handle(store, Caller{}, w, r) + }) + } + return Authenticate(sessions, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A check that cannot run refuses: no caller, no access. + caller, ok := r.Context().Value(callerKey{}).(Caller) + if !ok || !allowed(caller, route.Role) { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + route.Handle(store, caller, w, r) + })) +} + +func health(_ Store, _ Caller, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) +} + +func getInvoice(store Store, caller Caller, w http.ResponseWriter, r *http.Request) { + invoice, err := store.Get(r.Context(), caller.TenantID, r.PathValue("id")) + if !answered(w, err) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(invoice) + } +} + +func deleteInvoice(store Store, caller Caller, w http.ResponseWriter, r *http.Request) { + if !answered(w, store.Delete(r.Context(), caller.TenantID, r.PathValue("id"))) { + w.WriteHeader(http.StatusNoContent) + } +} + +// answered writes the error response, if any. "Does not exist" and "belongs to +// another tenant" get one answer. +func answered(w http.ResponseWriter, err error) bool { + switch { + case errors.Is(err, ErrNotFound): + http.Error(w, "not found", http.StatusNotFound) + case err != nil: + http.Error(w, "internal error", http.StatusInternalServerError) + default: + return false + } + return true +} +``` + +```go +package invoices + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" +) + +// Two tenants in every run, each with an admin and a member. +type sessions map[string]Caller + +func (s sessions) Caller(id string) (Caller, bool) { caller, ok := s[id]; return caller, ok } + +var testSessions = sessions{ + "a-admin": {TenantID: "tenant-a", Role: "admin"}, + "a-member": {TenantID: "tenant-a", Role: "member"}, + "b-admin": {TenantID: "tenant-b", Role: "admin"}, +} + +var ( + invoiceA = Invoice{ID: "inv-a", TenantID: "tenant-a", Total: 100} + invoiceB = Invoice{ID: "inv-b", TenantID: "tenant-b", Total: 200} +) + +// call goes through the real handler: session cookie, middleware, routing. +func call(handler http.Handler, session, method, path string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, nil) + if session != "" { + req.AddCookie(&http.Cookie{Name: "__Host-session", Value: session}) + } + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + return recorder +} + +type isolationCase struct { + kind string // "owner", "foreign", "anonymous" or "role" + name string + session string + path string + wantStatus int + wantInBody string + notInBody string +} + +// One entry per route in the route table. TestEveryRouteHasItsIsolationCases +// fails when a route is added without the kinds of case it needs. +var isolationCases = map[string][]isolationCase{ + "GET /invoices/{id}": { + // The positive control: without it, a route that is simply broken passes. + {"owner", "owner reads own invoice", "a-member", "/invoices/inv-a", http.StatusOK, `"inv-a"`, ""}, + {"foreign", "other tenant is refused", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, + {"anonymous", "no session is refused", "", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, + {"anonymous", "unknown session is refused", "forged", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, + }, + "DELETE /invoices/{id}": { + {"foreign", "other tenant cannot delete", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, + {"role", "lower role cannot delete", "a-member", "/invoices/inv-a", http.StatusForbidden, "", ""}, + {"anonymous", "no session cannot delete", "", "/invoices/inv-a", http.StatusUnauthorized, "", ""}, + {"owner", "owner admin deletes own invoice", "a-admin", "/invoices/inv-a", http.StatusNoContent, "", ""}, + }, +} + +func TestTenantIsolation(t *testing.T) { + for pattern, cases := range isolationCases { + method, _, _ := strings.Cut(pattern, " ") + for _, tc := range cases { + t.Run(pattern+"/"+tc.name, func(t *testing.T) { + store := NewMemoryStore(invoiceA, invoiceB) + response := call(Handler(store, testSessions), tc.session, method, tc.path) + body := response.Body.String() + if response.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d", response.Code, tc.wantStatus) + } + if tc.wantInBody != "" && !strings.Contains(body, tc.wantInBody) { + t.Errorf("body %q lacks %q", body, tc.wantInBody) + } + if tc.notInBody != "" && strings.Contains(body, tc.notInBody) { + t.Errorf("body %q leaks %q", body, tc.notInBody) + } + // Read back. Only an allowed delete may change anything, and it + // removes exactly the owner's invoice. + own, ownErr := store.Get(context.Background(), "tenant-a", "inv-a") + other, otherErr := store.Get(context.Background(), "tenant-b", "inv-b") + if method == http.MethodDelete && tc.wantStatus == http.StatusNoContent { + if !errors.Is(ownErr, ErrNotFound) { + t.Errorf("the deleted invoice is still there: %+v, %v", own, ownErr) + } + } else if ownErr != nil || own != invoiceA { + t.Errorf("the owner's invoice changed: %+v, %v", own, ownErr) + } + if otherErr != nil || other != invoiceB { + t.Errorf("the other tenant's invoice changed: %+v, %v", other, otherErr) + } + }) + } + } +} + +// A record that belongs to someone else and a record that does not exist get +// the same answer, for reads and for writes. +func TestMissingAndForeignInvoicesAnswerAlike(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodDelete} { + handler := Handler(NewMemoryStore(invoiceA, invoiceB), testSessions) + foreign := call(handler, "b-admin", method, "/invoices/inv-a") + missing := call(handler, "b-admin", method, "/invoices/no-such-invoice") + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Errorf("%s: a foreign invoice answers %d %q, a missing one %d %q", method, + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } + } +} + +// The reviewed list of routes that need no session. Adding one is a decision. +var publicRoutes = map[string]bool{"GET /healthz": true} + +func TestEveryRouteHasItsIsolationCases(t *testing.T) { + for _, route := range Routes { + if route.Role == "public" { + if !publicRoutes[route.Pattern] { + t.Errorf("route %q is public but not on the reviewed list", route.Pattern) + } + continue + } + if route.Role != "member" && route.Role != "admin" { + t.Errorf("route %q has the unknown policy %q", route.Pattern, route.Role) + } + needed := []string{"owner", "foreign", "anonymous"} + if route.Role != "member" { + needed = append(needed, "role") + } + var have []string + for _, tc := range isolationCases[route.Pattern] { + have = append(have, tc.kind) + } + for _, kind := range needed { + if !slices.Contains(have, kind) { + t.Errorf("route %q has no %q case", route.Pattern, kind) + } + } + } +} + +// A route whose policy is missing or misspelt is closed, even to an admin. +func TestRouteWithoutPolicyIsClosed(t *testing.T) { + for _, role := range []string{"", "owner"} { + route := Route{Pattern: "GET /unreviewed", Role: role, Handle: health} + response := call(serve(NewMemoryStore(), testSessions, route), "a-admin", http.MethodGet, "/unreviewed") + if response.Code != http.StatusForbidden { + t.Errorf("policy %q: status = %d, want 403", role, response.Code) + } + } +} + +func TestPublicRouteNeedsNoSession(t *testing.T) { + response := call(Handler(NewMemoryStore(), testSessions), "", http.MethodGet, "/healthz") + if response.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204", response.Code) + } +} +``` + +## TypeScript + +The read case for a request handler in the style of SvelteKit, where a server hook fills +`locals` after verifying the session. The tests go through that check and the tenant +condition is in the product code, as in the Go example; add the write, role and +route-coverage cases in the same way. Checked with Node.js 26.13, which runs TypeScript + +```ts +export type Invoice = { id: string; tenantId: string; total: number }; + +// Every method takes the tenant. Nothing reads by id alone. +export interface InvoiceStore { + get(tenantId: string, id: string): Promise; +} + +// This example's data layer. In a product it is a query with the tenant in +// its condition. +export function memoryStore(invoices: Invoice[]): InvoiceStore { + return { + async get(tenantId, id) { + return invoices.find((invoice) => invoice.tenantId === tenantId && invoice.id === id); + }, + }; +} + +type Event = { request: Request; params: { id: string }; locals: { tenantId?: string } }; +type Handler = (event: Event) => Promise; + +// The role of a SvelteKit server hook: it alone fills `locals`, from a session +// it has verified, and overwrites whatever the caller supplied. +export function authenticate(sessions: Map, next: Handler): Handler { + return async (event) => { + const cookies = event.request.headers.get('cookie') ?? ''; + const sessionId = /(?:^|;\s*)__Host-session=([^;]+)/.exec(cookies)?.[1]; + const tenantId = sessionId ? sessions.get(sessionId) : undefined; + if (!tenantId) return new Response('unauthorized', { status: 401 }); + return next({ ...event, locals: { tenantId } }); + }; +} + +export function getInvoice(store: InvoiceStore): Handler { + return async ({ params, locals }) => { + // A check that cannot run refuses: no tenant, no access. + if (!locals.tenantId) return new Response('unauthorized', { status: 401 }); + const invoice = await store.get(locals.tenantId, params.id); + // One answer for "does not exist" and "belongs to another tenant". + if (!invoice) return new Response('not found', { status: 404 }); + return Response.json({ id: invoice.id, total: invoice.total }); + }; +} +``` + +```ts +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { authenticate, getInvoice, memoryStore } from './invoices.ts'; + +// Two tenants in every run. +const sessions = new Map([ + ['session-a', 'tenant-a'], + ['session-b', 'tenant-b'], +]); +const handler = authenticate( + sessions, + getInvoice( + memoryStore([ + { id: 'inv-a', tenantId: 'tenant-a', total: 100 }, + { id: 'inv-b', tenantId: 'tenant-b', total: 200 }, + ]), + ), +); + +// call goes through the session check, as a real request does. +function call(session: string | undefined, id: string, locals = {}) { + const headers = session ? { cookie: `__Host-session=${session}` } : undefined; + const request = new Request(`https://example.test/invoices/${id}`, { headers }); + return handler({ request, params: { id }, locals }); +} + +test('the owner reads their own invoice', async () => { + // The positive control: without it, a route that is simply broken passes. + const response = await call('session-a', 'inv-a'); + assert.equal(response.status, 200); + assert.deepEqual(await response.json(), { id: 'inv-a', total: 100 }); +}); + +test('another tenant is refused and learns nothing', async () => { + const foreign = await call('session-b', 'inv-a'); + const missing = await call('session-b', 'no-such-invoice'); + assert.equal(foreign.status, 404); + const body = await foreign.text(); + assert.ok(!body.includes('inv-a')); + assert.equal(body, await missing.text()); +}); + +test('a request without a session, or with an unknown one, is refused', async () => { + assert.equal((await call(undefined, 'inv-a')).status, 401); + assert.equal((await call('forged', 'inv-a')).status, 401); +}); + +test('a tenant supplied by the caller is ignored', async () => { + const response = await call('session-b', 'inv-a', { tenantId: 'tenant-a' }); + assert.equal(response.status, 404); +}); +``` + +## When a test finds a leak + +In code that has not shipped: fix it, keep the test. In a running system: stop, and follow the +[escalation rules](../SKILL.md#escalate-do-not-decide-alone). A cross-tenant leak in production is an incident +even when the fix is one line. diff --git a/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md b/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md new file mode 100644 index 0000000..0fb6acd --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md @@ -0,0 +1,98 @@ +# Tenant isolation + +A tenant is whoever the data belongs to: a company, a household, a customer account. Isolation +means nothing one tenant does can read, change or slow another tenant's data. It fails most +often not in the main table but in a place somebody forgot: a cache, a job, an export. + +## Where the tenant comes from + +One place: the verified session. The sign-in step resolves the caller to a tenant and stores it +server-side; every later step reads it from there. + +- A tenant identifier in the URL, a header, a query or a body is **input**. If the product + shows one (a path such as `/acme/projects`), compare it with the session's tenant and + refuse on a mismatch. Never use it to select data. +- A user in several tenants picks one, and the server checks the membership before storing the + choice. Switching tenant is a server-side change with a log line. +- When a membership ends, access ends with it. Check the membership on each request, or end + that person's sessions for the tenant when it is removed. A tenant stored at sign-in and + never looked at again outlives the membership. +- Background work has no session. Carry the tenant in the job itself and set it explicitly + when the job starts. + +## In the data layer + +- **Every tenant-owned row carries the tenant.** Make it part of the key, so two tenants can + never collide and a relation cannot point across tenants: primary key `(tenant_id, id)`, + foreign keys that include `tenant_id`. +- **No function reads by identifier alone.** Give the data layer signatures that take the + tenant on every call (`Get(tenantID, id)`, never `Get(id)`). A handler then cannot forget it. +- **Lists, counts, searches and exports are queries too.** The filter belongs in each of them. +- **Uniqueness is per tenant.** A unique index on `email` alone tells one tenant that another + tenant already has that address. Index `(tenant_id, email)`. + +## Row-level security in PostgreSQL: a second line, with limits + +The database can refuse rows the application forgot to filter. Checked on PostgreSQL 17.11: + +```sql +ALTER TABLE invoices ENABLE ROW LEVEL SECURITY; +ALTER TABLE invoices FORCE ROW LEVEL SECURITY; + +CREATE POLICY tenant_isolation ON invoices + USING (tenant_id = NULLIF(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = NULLIF(current_setting('app.tenant_id', true), '')::uuid); +``` + +The application sets the tenant at the start of each transaction, with the third argument +`true` so the value ends with the transaction: + +```sql +BEGIN; +SELECT set_config('app.tenant_id', $1, true); +-- the request's queries +COMMIT; +``` + +What that test run showed, and what to hold on to: + +- **Use the transaction-local form.** A plain `SET app.tenant_id = ...` lasts for the whole + connection. With a connection pool the next request, for another tenant, inherits it. +- **Keep `NULLIF(..., '')`.** After a transaction-local value ends, the setting reads as an + empty string, not as missing. Without `NULLIF` the next query on that connection fails with + a cast error instead of returning no rows. +- **Keep `FORCE`.** Without it the table's owner sees every row. Run the application as a role + that does not own the tables and is not a superuser: a superuser, and any role with + `BYPASSRLS`, ignores every policy. +- **No tenant set means no rows.** That is the safe failure. Test it. +- **It is not a wall against the application itself.** The application's role can set the + value to any tenant. So this catches a forgotten filter; it does not contain someone who can + run SQL as that role, for example through an injection flaw. If the product needs that + boundary, each tenant needs its own database role that the application connects as for + that tenant, with rights on that tenant's schema or database only and no way to become + another tenant's role. A separate schema that one shared role can read is not a boundary. + Say which you chose in the threat model. + +Row-level security is added to the filters in the data layer, not instead of them. Migrations, +reports and maintenance jobs often run as a privileged role that skips the policies: give them +the same tests. + +## Every other place data rests + +| Place | Rule | +|---|---| +| Cache | The tenant is part of every key. A response cached for one tenant is never served to another: check shared caches and CDN rules for pages that hold tenant data | +| Files and object storage | A per-tenant prefix, chosen by the server. The access check runs before the file is read. No public bucket, no path built from a client's file name | +| Queues and jobs | The tenant travels in the message and is set before any query. A job that loops over tenants sets the tenant inside the loop | +| Search index | The tenant is a required filter added by the server, never an optional parameter | +| Exports and reports | Generated by the same data layer, stored under the tenant's prefix, fetched through the same access check | +| Logs, metrics and traces | Carry the tenant identifier, never the tenant's data. A support view of logs is itself tenant data | +| Email and notifications | The recipient list is built from the tenant's own members | +| Backups and copies | A copy in a test environment is still tenant data. Restore tests use the same isolation tests | + +## Crossing the boundary on purpose + +Support access, an administrator acting as a user, a report across customers: each is a +deliberate hole. Each needs its own narrow operation, a reason recorded at use, a log the +tenant could be shown, and a named person who approved the design. Adding one is an +escalation, not a refactor. diff --git a/plugins/agentic-engineering/skills/application-security/references/threat-model.md b/plugins/agentic-engineering/skills/application-security/references/threat-model.md new file mode 100644 index 0000000..ca73123 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/threat-model.md @@ -0,0 +1,76 @@ +# A threat model in fifteen minutes + +A threat model is the list of ways a change could be abused, written before the code, with a +decision beside each. It is small on purpose: one model per change, kept in the pull request. +The four questions are adapted from the [Threat Modeling Manifesto](https://www.threatmodelingmanifesto.org/). + +## 1. What are we building? + +Write five lines, not a diagram project: + +- **Who calls it.** Anonymous visitor, signed-in user, tenant administrator, staff, another + service, a provider's webhook. +- **How they reach it.** Each route, action, job or message the change adds or alters. +- **What it reads and writes.** Each table, bucket, cache or queue, and whose data is in it. +- **Where trust changes.** Every point where data moves from a party you do not control to code + that acts on it: the browser to the server, one tenant's data to shared code, a provider to + your webhook, your server to a URL a user supplied. +- **What it must never do.** One or two sentences, such as "a customer never sees another + customer's invoice" and "an invoice is marked paid only by the payment provider". + +## 2. What can go wrong? + +Ask these of every line from step 1. They are the classic six, in plain words, plus the two +that matter most for a product with tenants. + +| Ask | Looks like | +|---|---| +| Can someone pretend to be another caller? | A forged session, a guessed token, a webhook anyone can post to | +| Can someone change data they should not? | Editing another user's record, setting `role` or `price` in a request body | +| Can someone deny having done it? | A sensitive action with no record of who did it | +| Can someone read data they should not? | Another tenant's record by its identifier, a list that is not filtered, an error that prints a row | +| Can someone make it unavailable or expensive? | Unbounded uploads, a search with no limit, a sign-in form with no attempt limit | +| Can someone get more rights than they have? | A user calling an administrator's route, a tenant administrator acting outside the tenant | +| **Can tenant A reach tenant B at all?** | Through an identifier, a shared cache key, a file path, a background job, an export, a search | +| **Can the feature be used as built, but against its purpose?** | Buying at a stale price, replaying a "paid" message, creating accounts in bulk | + +Write down only the ones that apply. Three to eight is normal for one endpoint. + +## 3. What will we do about it? + +Beside each threat write one of: + +- **A control**, and where it lives: "tenant taken from the session in the data layer", + "signature checked before the body is parsed". +- **Removed**: the design changed so the threat is gone. Often the best answer: do not accept + the field, do not expose the route. +- **Accepted**, with the name of the person who accepted it, the reason and a date to look + again. Only a person accepts a risk. +- **Open**: you have proposed a control or an acceptance and a named person has not decided + yet. Say who is asked. A change does not ship with an open threat unless that person says + so. + +"We will be careful" is not a control. Neither is "the identifier is random". + +## 4. How will we know it worked? + +Each control gets a test that fails when the control is removed +([isolation tests](isolation-tests.md)). A threat with a control and no test is recorded as +unproven, not as handled. + +## A filled example + +A change adds "download my invoice as a PDF" to a product whose customers are companies. + +**What we are building.** A signed-in user of a company calls `GET /invoices/{id}/pdf`. It +reads the invoice row and a stored PDF. Trust changes at the browser-to-server boundary. It +must never return another company's invoice. + +| # | What can go wrong | Decision | Proved by | +|---|---|---|---| +| 1 | A user of company B requests company A's invoice by its identifier | Control: the data layer reads by company and identifier together; the company comes from the session | Test: B's session on A's identifier gets "not found", and the body holds nothing of A's | +| 2 | The response says "forbidden" for A's invoice and "not found" for a missing one, so identifiers can be probed | Control: one answer for both | Same test, comparing both responses | +| 3 | The PDF is stored under a guessable path and served directly | Removed: the file is read by the server after the access check, never from a public path | Test: the storage path is not reachable without a session | +| 4 | A script downloads every invoice in a loop | Control: a request limit per user on this route | Test: the request after the limit is refused with status 429, and another user is not affected | +| 5 | Nobody can tell later who downloaded what | Control: a log line with user, company and invoice identifier, and no invoice content | Test: the log line exists and holds no amounts | +| 6 | A former employee's session still works | Open: how long the application's own session lives after the identity provider ends its session has not been tested, so no limit can be promised yet. Proposed: test expiry and removal first, then decide. Asked: the product owner, named in the pull request | Not proven. It stays open until that test exists and that person decides |