From c569a2ffa1025fa48555b636f604cf23ccb06747 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 00:38:06 +0100 Subject: [PATCH 1/6] feat(agentic-engineering): add the application-security skill A first-party skill for the security decisions scanners cannot make: a short threat model, server-side access control, tenant isolation, webhook verification, uploads, limits and outbound requests, and the two-tenant test design that proves each control. Examples in Go, TypeScript and PostgreSQL were run before release. Seven upstream candidates were compared and none could be adopted; the coverage page records why. Version 7.5.0. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .claude-plugin/marketplace.json | 2 +- .github/plugin/marketplace.json | 2 +- docs/plugins.md | 3 +- docs/skill-coverage.md | 33 ++- .../.claude-plugin/plugin.json | 2 +- .../.codex-plugin/plugin.json | 2 +- plugins/agentic-engineering/CHANGELOG.md | 18 ++ plugins/agentic-engineering/README.md | 1 + plugins/agentic-engineering/plugin.json | 2 +- .../skills/application-security/SKILL.md | 105 +++++++ .../references/access-control.md | 61 ++++ .../application-security/references/edges.md | 181 +++++++++++ .../references/isolation-tests.md | 280 ++++++++++++++++++ .../references/tenant-isolation.md | 93 ++++++ .../references/threat-model.md | 73 +++++ 15 files changed, 840 insertions(+), 18 deletions(-) create mode 100644 plugins/agentic-engineering/skills/application-security/SKILL.md create mode 100644 plugins/agentic-engineering/skills/application-security/references/access-control.md create mode 100644 plugins/agentic-engineering/skills/application-security/references/edges.md create mode 100644 plugins/agentic-engineering/skills/application-security/references/isolation-tests.md create mode 100644 plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md create mode 100644 plugins/agentic-engineering/skills/application-security/references/threat-model.md 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..ddaba62 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/SKILL.md @@ -0,0 +1,105 @@ +--- +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, 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. **The tenant comes from the verified session.** Never from the URL, a header, a query + parameter or the request body. +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 or with the name of the + person who accepted the risk. +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, and a caller without a session is refused. 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..0def8db --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/access-control.md @@ -0,0 +1,61 @@ +# 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 stored facts.** The record's owner and the caller's role come from the + database and the session, never from the request. +- **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..5f79688 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -0,0 +1,181 @@ +# 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. +- **Handle each event once.** Providers redeliver. Record the event identifier in the same + transaction as its effect, and answer a redelivery 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 support two at once so it can be rotated. +- **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 is an example, not any +provider's: + +```go +package webhook + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "errors" + "io" + "net/http" + "strconv" + "time" +) + +var ( + ErrStale = errors.New("webhook timestamp outside the tolerance") + ErrSignature = errors.New("webhook signature does not match") +) + +// Verify checks a delivery signed as hex(HMAC-SHA256(secret, timestamp + "." + body)). +// Providers differ in header names and layout: follow the provider's own +// documentation, and use its library when it has one. +func Verify(secret, body []byte, timestamp, 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 + } + mac := hmac.New(sha256.New, secret) + mac.Write([]byte(timestamp + ".")) + mac.Write(body) + given, err := hex.DecodeString(signature) + if err != nil || !hmac.Equal(given, mac.Sum(nil)) { // constant-time comparison + return ErrSignature + } + return nil +} + +// Deliveries remembers which event IDs were handled. Back it with the database, +// in the same transaction as the event's effect. +type Deliveries interface { + // FirstTime records the ID and reports whether it was new. + FirstTime(eventID string) (bool, error) +} + +const maxBody = 1 << 20 // 1 MiB + +func Handler(secret []byte, seen Deliveries, handle func(eventID string, body []byte) error) http.Handler { + 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 + } + err = Verify(secret, body, r.Header.Get("Webhook-Timestamp"), r.Header.Get("Webhook-Signature"), time.Now(), 5*time.Minute) + if err != nil { + http.Error(w, "invalid signature", http.StatusUnauthorized) + return + } + eventID := r.Header.Get("Webhook-Id") + if eventID == "" { + http.Error(w, "missing event id", http.StatusBadRequest) + return + } + first, err := seen.FirstTime(eventID) + if err != nil { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if !first { + w.WriteHeader(http.StatusOK) // a redelivery: already handled + return + } + if err := handle(eventID, body); err != nil { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + }) +} +``` + +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. The tests +for this handler cover a valid delivery, a changed body, a wrong secret, a stale and a future +timestamp, and a redelivery handled once. + +## 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. +- **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 private, loopback and link-local addresses, 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..22c9a58 --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -0,0 +1,280 @@ +# 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 user 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 | Refused | +| 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 middleware. A test that calls an inner function 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 (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.** Walk the router's list of routes in a test 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. + +## Go + +A handler, a data layer that cannot be called without the tenant, and the test. Checked with +Go 1.27 (the route patterns need Go 1.22 or later). Removing the tenant condition from `Get` +or `List` makes the test fail. + +```go +package invoices + +import ( + "context" + "encoding/json" + "errors" + "net/http" +) + +// 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 reads by ID alone, so a +// handler cannot forget the tenant. +type Store interface { + Get(ctx context.Context, tenantID, id string) (Invoice, error) + List(ctx context.Context, tenantID string) ([]Invoice, error) +} + +type tenantKey struct{} + +// WithTenant is called by the authentication middleware, after it has verified +// the session or token. Nothing else sets the tenant. +func WithTenant(ctx context.Context, tenantID string) context.Context { + return context.WithValue(ctx, tenantKey{}, tenantID) +} + +func tenantFrom(ctx context.Context) (string, bool) { + tenantID, ok := ctx.Value(tenantKey{}).(string) + return tenantID, ok && tenantID != "" +} + +func Handler(store Store) http.Handler { + mux := http.NewServeMux() + mux.HandleFunc("GET /invoices/{id}", func(w http.ResponseWriter, r *http.Request) { + tenantID, ok := tenantFrom(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + invoice, err := store.Get(r.Context(), tenantID, r.PathValue("id")) + switch { + case errors.Is(err, ErrNotFound): + // One answer for "does not exist" and "belongs to another tenant". + http.Error(w, "not found", http.StatusNotFound) + case err != nil: + http.Error(w, "internal error", http.StatusInternalServerError) + default: + writeJSON(w, invoice) + } + }) + mux.HandleFunc("GET /invoices", func(w http.ResponseWriter, r *http.Request) { + tenantID, ok := tenantFrom(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + invoices, err := store.List(r.Context(), tenantID) + if err != nil { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + writeJSON(w, invoices) + }) + return mux +} + +func writeJSON(w http.ResponseWriter, value any) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(value) +} +``` + +```go +package invoices + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// memoryStore stands in for the data layer. In a product, run these tests +// against the real store and a test database: the query is the control. +type memoryStore struct{ invoices []Invoice } + +func (s memoryStore) Get(_ context.Context, tenantID, id string) (Invoice, error) { + for _, invoice := range s.invoices { + if invoice.TenantID == tenantID && invoice.ID == id { + return invoice, nil + } + } + return Invoice{}, ErrNotFound +} + +func (s memoryStore) List(_ context.Context, tenantID string) ([]Invoice, error) { + var own []Invoice + for _, invoice := range s.invoices { + if invoice.TenantID == tenantID { + own = append(own, invoice) + } + } + return own, nil +} + +func request(handler http.Handler, tenantID, path string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, path, nil) + if tenantID != "" { + req = req.WithContext(WithTenant(req.Context(), tenantID)) + } + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + return recorder +} + +func TestTenantsCannotReadEachOthersInvoices(t *testing.T) { + handler := Handler(memoryStore{invoices: []Invoice{ + {ID: "inv-a", TenantID: "tenant-a", Total: 100}, + {ID: "inv-b", TenantID: "tenant-b", Total: 200}, + }}) + + cases := []struct { + name string + tenantID string + path string + wantStatus int + wantInBody string + notInBody string + }{ + // The positive control: without it, a route that is simply broken passes. + {"owner reads own invoice", "tenant-a", "/invoices/inv-a", http.StatusOK, `"inv-a"`, ""}, + {"other tenant is refused", "tenant-b", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, + {"no session is refused", "", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, + {"list shows own invoices only", "tenant-b", "/invoices", http.StatusOK, `"inv-b"`, "inv-a"}, + {"list without a session is refused", "", "/invoices", http.StatusUnauthorized, "", "inv-"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + response := request(handler, tc.tenantID, 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) + } + }) + } +} +``` + +## TypeScript + +The same shape for a request handler in the style of SvelteKit, where a server hook fills +`locals` after verifying the session. Checked with Node.js 26.13, which runs TypeScript +directly (`node --test invoices.test.ts`); it has no dependencies. + +```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; +} + +// The shape of a SvelteKit request handler: `locals` is filled by the server +// hook after it has verified the session. The handler never reads the tenant +// from the URL, a header or the body. +type Event = { params: { id: string }; locals: { tenantId?: string } }; + +export function getInvoice(store: InvoiceStore) { + return async ({ params, locals }: Event): Promise => { + 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 { getInvoice, type Invoice, type InvoiceStore } from './invoices.ts'; + +// Stands in for the data layer. In a product, run these tests against the real +// store and a test database: the query is the control. +function memoryStore(invoices: Invoice[]): InvoiceStore { + return { + async get(tenantId, id) { + return invoices.find((invoice) => invoice.tenantId === tenantId && invoice.id === id); + }, + }; +} + +const handler = getInvoice( + memoryStore([ + { id: 'inv-a', tenantId: 'tenant-a', total: 100 }, + { id: 'inv-b', tenantId: 'tenant-b', total: 200 }, + ]), +); + +test('the owner reads their own invoice', async () => { + // The positive control: without it, a route that is simply broken passes. + const response = await handler({ params: { id: 'inv-a' }, locals: { tenantId: 'tenant-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 response = await handler({ params: { id: 'inv-a' }, locals: { tenantId: 'tenant-b' } }); + assert.equal(response.status, 404); + assert.ok(!(await response.text()).includes('inv-a')); +}); + +test('a request without a session is refused', async () => { + const response = await handler({ params: { id: 'inv-a' }, locals: {} }); + assert.equal(response.status, 401); +}); +``` + +## 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..3a1e4bb --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md @@ -0,0 +1,93 @@ +# 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. +- 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, give each tenant its own database role, schema or database, and say so 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..ffa81fc --- /dev/null +++ b/plugins/agentic-engineering/skills/application-security/references/threat-model.md @@ -0,0 +1,73 @@ +# 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. An agent proposes; a person accepts. + +"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 | +| 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 | Accepted for now by the product owner: sessions end after the identity provider's timeout. Look again when company administrators can remove users | Unproven here: tracked separately | From 65aa19bc27f0e856327f9f0fdc8adbb14ee10eac Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 00:39:56 +0100 Subject: [PATCH 2/6] fix(agentic-engineering): sign the event identifier in the webhook example A delivery signed over the body and timestamp alone could be replayed within the tolerance under a new identifier and counted as new. The example now signs the identifier too, and its tests cover that replay. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .../application-security/references/edges.md | 36 +++++++++++-------- .../references/isolation-tests.md | 3 +- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/plugins/agentic-engineering/skills/application-security/references/edges.md b/plugins/agentic-engineering/skills/application-security/references/edges.md index 5f79688..677112b 100644 --- a/plugins/agentic-engineering/skills/application-security/references/edges.md +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -26,6 +26,8 @@ makes a delivery trustworthy. - **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. +- **The event identifier is signed too.** If only the body is signed, a captured delivery can + be sent again under a new identifier and counted as new. - **Handle each event once.** Providers redeliver. Record the event identifier in the same transaction as its effect, and answer a redelivery with success without acting again. - **Trust the event only as far as needed.** Prefer to take the identifier from the event and @@ -35,8 +37,8 @@ makes a delivery trustworthy. - **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 is an example, not any -provider's: +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 @@ -57,10 +59,13 @@ var ( ErrSignature = errors.New("webhook signature does not match") ) -// Verify checks a delivery signed as hex(HMAC-SHA256(secret, timestamp + "." + body)). -// Providers differ in header names and layout: follow the provider's own -// documentation, and use its library when it has one. -func Verify(secret, body []byte, timestamp, signature string, now time.Time, tolerance time.Duration) error { +// 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. Providers differ in header names and +// layout: follow the provider's documentation, and use its library when it +// has one. +func Verify(secret []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 @@ -70,7 +75,7 @@ func Verify(secret, body []byte, timestamp, signature string, now time.Time, tol return ErrStale } mac := hmac.New(sha256.New, secret) - mac.Write([]byte(timestamp + ".")) + mac.Write([]byte(eventID + "." + timestamp + ".")) mac.Write(body) given, err := hex.DecodeString(signature) if err != nil || !hmac.Equal(given, mac.Sum(nil)) { // constant-time comparison @@ -96,16 +101,17 @@ func Handler(secret []byte, seen Deliveries, handle func(eventID string, body [] http.Error(w, "body too large", http.StatusRequestEntityTooLarge) return } - err = Verify(secret, body, r.Header.Get("Webhook-Timestamp"), r.Header.Get("Webhook-Signature"), time.Now(), 5*time.Minute) - if err != nil { - http.Error(w, "invalid signature", http.StatusUnauthorized) - return - } - eventID := r.Header.Get("Webhook-Id") + eventID := r.Header.Get("X-Event-Id") if eventID == "" { http.Error(w, "missing event id", http.StatusBadRequest) return } + err = Verify(secret, 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 + } first, err := seen.FirstTime(eventID) if err != nil { http.Error(w, "internal error", http.StatusInternalServerError) @@ -126,8 +132,8 @@ func Handler(secret []byte, seen Deliveries, handle func(eventID string, body [] 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. The tests -for this handler cover a valid delivery, a changed body, a wrong secret, a stale and a future -timestamp, and a redelivery handled once. +for this handler cover a valid delivery, a changed body, a changed identifier, a wrong secret, +a stale and a future timestamp, a redelivery handled once, and a replay under a new identifier. ## Accepting a file diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md index 22c9a58..2216789 100644 --- a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -27,7 +27,8 @@ Rules that keep the tests honest: hand-written fake that filters correctly proves nothing about the SQL. - **Prove each test once.** Delete the control (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.** Walk the router's list of routes in a test and fail +- **Cover every route by construction.** Keep the routes where a test can list them (the + router's own listing, or one route table), 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 From 3db1faf98da487716d6721b55412cdc6d8937b97 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 01:13:02 +0100 Subject: [PATCH 3/6] fix(agentic-engineering): correct the application-security examples after a trial A fresh agent followed the skill and found defects. The webhook example recorded an event before handling it, so a failed effect was lost on redelivery: the record and the effect now succeed or fail together. The isolation example set the caller directly and kept its tenant check in a test fake: it now goes through a session check and a route table, tests a write, a role and route coverage, and its tenant check is product code. The threat model gains a state for a decision that is still open. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .../skills/application-security/SKILL.md | 14 +- .../application-security/references/edges.md | 213 +++++++++-- .../references/isolation-tests.md | 335 ++++++++++++------ .../references/threat-model.md | 9 +- 4 files changed, 420 insertions(+), 151 deletions(-) diff --git a/plugins/agentic-engineering/skills/application-security/SKILL.md b/plugins/agentic-engineering/skills/application-security/SKILL.md index ddaba62..59dcfa6 100644 --- a/plugins/agentic-engineering/skills/application-security/SKILL.md +++ b/plugins/agentic-engineering/skills/application-security/SKILL.md @@ -1,6 +1,6 @@ --- 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, sign-in, roles, tenant or customer data access, a webhook receiver or a file upload. Not for scanner setup or CI hardening. +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 --- @@ -52,8 +52,8 @@ They win over this skill wherever they are more specific. 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 or with the name of the - person who accepted the risk. + 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). @@ -62,9 +62,11 @@ They win over this skill wherever they are more specific. [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, and a caller without a session is refused. Remove the - control once and watch the test fail. See [isolation tests](references/isolation-tests.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. 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. diff --git a/plugins/agentic-engineering/skills/application-security/references/edges.md b/plugins/agentic-engineering/skills/application-security/references/edges.md index 677112b..210444c 100644 --- a/plugins/agentic-engineering/skills/application-security/references/edges.md +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -26,14 +26,17 @@ makes a delivery trustworthy. - **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. -- **The event identifier is signed too.** If only the body is signed, a captured delivery can - be sent again under a new identifier and counted as new. -- **Handle each event once.** Providers redeliver. Record the event identifier in the same - transaction as its effect, and answer a redelivery with success without acting again. +- **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 support two at once so it can be rotated. + secret store, one per endpoint, and accept the current and the previous secret while one + is being rotated. - **Map the event to a tenant from your own records**, such as the stored customer identifier. Never from a tenant field in the body. @@ -50,6 +53,7 @@ import ( "errors" "io" "net/http" + "regexp" "strconv" "time" ) @@ -61,11 +65,12 @@ var ( // 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. Providers differ in header names and -// layout: follow the provider's documentation, and use its library when it -// has one. -func Verify(secret []byte, eventID, timestamp string, body []byte, signature string, now time.Time, tolerance time.Duration) error { +// 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. Providers differ in header +// names and layout: 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 @@ -74,26 +79,36 @@ func Verify(secret []byte, eventID, timestamp string, body []byte, signature str if age > tolerance || age < -tolerance { return ErrStale } - mac := hmac.New(sha256.New, secret) - mac.Write([]byte(eventID + "." + timestamp + ".")) - mac.Write(body) given, err := hex.DecodeString(signature) - if err != nil || !hmac.Equal(given, mac.Sum(nil)) { // constant-time comparison + if err != nil { return ErrSignature } - return nil + for _, secret := range secrets { + 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 remembers which event IDs were handled. Back it with the database, -// in the same transaction as the event's effect. +// Deliveries applies each event at most once. type Deliveries interface { - // FirstTime records the ID and reports whether it was new. - FirstTime(eventID string) (bool, error) + // Once runs effect unless eventID was handled before, and records the + // identifier only when effect succeeds. With a database, the record and + // the effect share one transaction. It reports whether effect ran. + Once(eventID string, effect func() error) (ran bool, err error) } const maxBody = 1 << 20 // 1 MiB -func Handler(secret []byte, seen Deliveries, handle func(eventID string, body []byte) error) http.Handler { +// 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}$`) + +func Handler(secrets [][]byte, seen Deliveries, handle func(eventID string, body []byte) error) http.Handler { 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)) @@ -102,38 +117,166 @@ func Handler(secret []byte, seen Deliveries, handle func(eventID string, body [] return } eventID := r.Header.Get("X-Event-Id") - if eventID == "" { - http.Error(w, "missing event id", http.StatusBadRequest) + if !validEventID.MatchString(eventID) { + http.Error(w, "invalid event id", http.StatusBadRequest) return } - err = Verify(secret, eventID, r.Header.Get("X-Event-Timestamp"), body, + 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 } - first, err := seen.FirstTime(eventID) + _, err = seen.Once(eventID, func() error { return handle(eventID, body) }) if err != nil { + // Nothing was recorded, so the provider's next delivery is handled. http.Error(w, "internal error", http.StatusInternalServerError) return } - if !first { - w.WriteHeader(http.StatusOK) // a redelivery: already handled - return + w.WriteHeader(http.StatusOK) // handled now, or already handled + }) +} +``` + +Its tests. The last three steps 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. + +```go +package webhook + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "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 + eventID string + body []byte + timestamp string + signature string + want error + }{ + {"valid delivery", "evt-1", body, fresh, sign(current, "evt-1", fresh, body), nil}, + {"signed with the previous secret during rotation", "evt-1", body, fresh, sign(previous, "evt-1", fresh, body), nil}, + {"changed body", "evt-1", []byte(`{"type":"invoice.void"}`), fresh, sign(current, "evt-1", fresh, body), ErrSignature}, + {"changed identifier", "evt-2", body, fresh, sign(current, "evt-1", fresh, body), ErrSignature}, + {"unknown secret", "evt-1", body, fresh, sign([]byte("other"), "evt-1", fresh, body), ErrSignature}, + {"signature is not hex", "evt-1", body, fresh, "not-hex", ErrSignature}, + {"missing signature", "evt-1", body, fresh, "", ErrSignature}, + {"replayed after the tolerance", "evt-1", body, old, sign(current, "evt-1", old, body), ErrStale}, + {"timestamp in the future", "evt-1", body, ahead, sign(current, "evt-1", ahead, body), ErrStale}, + {"timestamp is not a number", "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(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) + } + }) + } +} + +// memoryDeliveries stands in for a table with a unique event identifier, +// written in the same transaction as the effect. +type memoryDeliveries struct { + mu sync.Mutex + done map[string]bool +} + +func (m *memoryDeliveries) Once(eventID string, effect func() error) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + if m.done[eventID] { + return false, nil + } + if err := effect(); err != nil { + return false, err // not recorded: the next delivery tries again + } + m.done[eventID] = true + return true, nil +} + +func TestHandler(t *testing.T) { + handled, failNext := 0, false + handle := func(string, []byte) error { + if failNext { + failNext = false + return errors.New("database unavailable") } - if err := handle(eventID, body); err != nil { - http.Error(w, "internal error", http.StatusInternalServerError) - return + handled++ + return nil + } + handler := Handler(secrets, &memoryDeliveries{done: map[string]bool{}}, handle) + 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) } - w.WriteHeader(http.StatusOK) - }) + } + + 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) } ``` 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. The tests -for this handler cover a valid delivery, a changed body, a changed identifier, a wrong secret, -a stale and a future timestamp, a redelivery handled once, and a replay under a new identifier. +effect first, make it safe to repeat, and record the identifier after it succeeded. ## Accepting a file diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md index 2216789..3545bc7 100644 --- a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -5,14 +5,14 @@ tests are the enforcement of everything else in this skill. They run in CI on ev ## What the tests must show -Create **two tenants** in every run, each with its own user and its own records. Then, for +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 | Refused | +| No session, or a session the server does not know | Refused | | 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 | @@ -21,24 +21,37 @@ every route, action and job that touches tenant data: Rules that keep the tests honest: - **Go through the real entry point**: the HTTP handler or the server action, with the real - routing and middleware. A test that calls an inner function skips the layer where the check - usually lives. + 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 (drop the tenant condition) and watch the test - fail. If it still passes, it tests nothing. Put the proof in the pull request. +- **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), 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. + 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 handler, a data layer that cannot be called without the tenant, and the test. Checked with -Go 1.27 (the route patterns need Go 1.22 or later). Removing the tenant condition from `Get` -or `List` makes the test fail. +A session check, a route table in which every route names its role, a data layer that cannot +be called without the tenant, and the tests. Checked with Go 1.27 (the route patterns need +Go 1.22 or later). Each of these makes a test fail: dropping the tenant condition from +`MemoryStore.Get` or `MemoryStore.Delete`, making `allowed` return true, and adding a route +without cases. ```go package invoices @@ -48,6 +61,7 @@ import ( "encoding/json" "errors" "net/http" + "sync" ) // Invoice belongs to exactly one tenant. The tenant is never sent to the client. @@ -59,64 +73,139 @@ type Invoice struct { var ErrNotFound = errors.New("invoice not found") -// Store takes the tenant on every call. No method reads by ID alone, so a +// 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) - List(ctx context.Context, tenantID string) ([]Invoice, error) + Delete(ctx context.Context, tenantID, id string) error } -type tenantKey struct{} +// MemoryStore is this example's data layer. In a product it is SQL, and the +// tenant condition below is `WHERE tenant_id = $1 AND id = $2`. +type MemoryStore struct { + mu sync.Mutex + invoices map[string]Invoice +} -// WithTenant is called by the authentication middleware, after it has verified -// the session or token. Nothing else sets the tenant. -func WithTenant(ctx context.Context, tenantID string) context.Context { - return context.WithValue(ctx, tenantKey{}, tenantID) +func NewMemoryStore(invoices ...Invoice) *MemoryStore { + store := &MemoryStore{invoices: map[string]Invoice{}} + for _, invoice := range invoices { + store.invoices[invoice.ID] = invoice + } + return store } -func tenantFrom(ctx context.Context) (string, bool) { - tenantID, ok := ctx.Value(tenantKey{}).(string) - return tenantID, ok && tenantID != "" +func (s *MemoryStore) Get(_ context.Context, tenantID, id string) (Invoice, error) { + s.mu.Lock() + defer s.mu.Unlock() + invoice, ok := s.invoices[id] + if !ok || invoice.TenantID != tenantID { + return Invoice{}, ErrNotFound + } + return invoice, nil } -func Handler(store Store) http.Handler { - mux := http.NewServeMux() - mux.HandleFunc("GET /invoices/{id}", func(w http.ResponseWriter, r *http.Request) { - tenantID, ok := tenantFrom(r.Context()) - if !ok { +func (s *MemoryStore) Delete(_ context.Context, tenantID, id string) error { + s.mu.Lock() + defer s.mu.Unlock() + invoice, ok := s.invoices[id] + if !ok || invoice.TenantID != tenantID { + return ErrNotFound + } + delete(s.invoices, 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 with its identity provider or its session store here. +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 } - invoice, err := store.Get(r.Context(), tenantID, r.PathValue("id")) - switch { - case errors.Is(err, ErrNotFound): - // One answer for "does not exist" and "belongs to another tenant". - http.Error(w, "not found", http.StatusNotFound) - case err != nil: - http.Error(w, "internal error", http.StatusInternalServerError) - default: - writeJSON(w, invoice) - } - }) - mux.HandleFunc("GET /invoices", func(w http.ResponseWriter, r *http.Request) { - tenantID, ok := tenantFrom(r.Context()) - if !ok { + caller, ok := sessions.Caller(cookie.Value) + if !ok || caller.TenantID == "" { http.Error(w, "unauthorized", http.StatusUnauthorized) return } - invoices, err := store.List(r.Context(), tenantID) - if err != nil { - http.Error(w, "internal error", http.StatusInternalServerError) - return - } - writeJSON(w, invoices) + next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), callerKey{}, caller))) }) - return mux } -func writeJSON(w http.ResponseWriter, value any) { - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(value) +// Route is one entry of the route table. Every route names the role it needs, +// 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}, +} + +func allowed(caller Caller, needed string) bool { + return caller.Role == "admin" || caller.Role == needed +} + +func Handler(store Store, sessions Sessions) http.Handler { + mux := http.NewServeMux() + for _, route := range Routes { + mux.HandleFunc(route.Pattern, func(w http.ResponseWriter, r *http.Request) { + caller := r.Context().Value(callerKey{}).(Caller) + if !allowed(caller, route.Role) { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + route.Handle(store, caller, w, r) + }) + } + return Authenticate(sessions, mux) +} + +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 } ``` @@ -131,82 +220,114 @@ import ( "testing" ) -// memoryStore stands in for the data layer. In a product, run these tests -// against the real store and a test database: the query is the control. -type memoryStore struct{ invoices []Invoice } +// Two tenants in every run, each with an admin and a member. +type sessions map[string]Caller -func (s memoryStore) Get(_ context.Context, tenantID, id string) (Invoice, error) { - for _, invoice := range s.invoices { - if invoice.TenantID == tenantID && invoice.ID == id { - return invoice, nil - } - } - return Invoice{}, ErrNotFound +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"}, } -func (s memoryStore) List(_ context.Context, tenantID string) ([]Invoice, error) { - var own []Invoice - for _, invoice := range s.invoices { - if invoice.TenantID == tenantID { - own = append(own, invoice) - } - } - return own, nil +func newStore() *MemoryStore { + return NewMemoryStore( + Invoice{ID: "inv-a", TenantID: "tenant-a", Total: 100}, + Invoice{ID: "inv-b", TenantID: "tenant-b", Total: 200}, + ) } -func request(handler http.Handler, tenantID, path string) *httptest.ResponseRecorder { - req := httptest.NewRequest(http.MethodGet, path, nil) - if tenantID != "" { - req = req.WithContext(WithTenant(req.Context(), tenantID)) +// 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 } -func TestTenantsCannotReadEachOthersInvoices(t *testing.T) { - handler := Handler(memoryStore{invoices: []Invoice{ - {ID: "inv-a", TenantID: "tenant-a", Total: 100}, - {ID: "inv-b", TenantID: "tenant-b", Total: 200}, - }}) - - cases := []struct { - name string - tenantID string - path string - wantStatus int - wantInBody string - notInBody string - }{ +type isolationCase struct { + name string + session string + path string + wantStatus int + wantInBody string + notInBody string +} + +// One entry per route in the route table. TestEveryRouteHasIsolationCases +// fails when a route is added without one. +var isolationCases = map[string][]isolationCase{ + "GET /invoices/{id}": { // The positive control: without it, a route that is simply broken passes. - {"owner reads own invoice", "tenant-a", "/invoices/inv-a", http.StatusOK, `"inv-a"`, ""}, - {"other tenant is refused", "tenant-b", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, + {"owner reads own invoice", "a-member", "/invoices/inv-a", http.StatusOK, `"inv-a"`, ""}, + {"other tenant is refused", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, {"no session is refused", "", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, - {"list shows own invoices only", "tenant-b", "/invoices", http.StatusOK, `"inv-b"`, "inv-a"}, - {"list without a session is refused", "", "/invoices", http.StatusUnauthorized, "", "inv-"}, + {"unknown session is refused", "forged", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, + }, + "DELETE /invoices/{id}": { + {"other tenant cannot delete", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", ""}, + {"lower role cannot delete", "a-member", "/invoices/inv-a", http.StatusForbidden, "", ""}, + {"no session cannot delete", "", "/invoices/inv-a", http.StatusUnauthorized, "", ""}, + {"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 := newStore() + 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) + } + // A refused write must leave the owner's record as it was. + if method == http.MethodDelete && tc.wantStatus != http.StatusNoContent { + if _, err := store.Get(context.Background(), "tenant-a", "inv-a"); err != nil { + t.Errorf("the owner's invoice is gone after a refused delete: %v", err) + } + } + }) + } } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - response := request(handler, tc.tenantID, 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) - } - }) +} + +func TestMissingAndForeignInvoicesAnswerAlike(t *testing.T) { + handler := Handler(newStore(), testSessions) + foreign := call(handler, "b-admin", http.MethodGet, "/invoices/inv-a") + missing := call(handler, "b-admin", http.MethodGet, "/invoices/no-such-invoice") + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Fatalf("a foreign invoice answers %d %q, a missing one %d %q", + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } +} + +func TestEveryRouteHasIsolationCases(t *testing.T) { + for _, route := range Routes { + if len(isolationCases[route.Pattern]) == 0 { + t.Errorf("route %q has no isolation cases", route.Pattern) + } } } ``` ## TypeScript -The same shape for a request handler in the style of SvelteKit, where a server hook fills -`locals` after verifying the session. Checked with Node.js 26.13, which runs TypeScript +The read case for a request handler in the style of SvelteKit, where a server hook fills +`locals` after verifying the session. It shows the shape only: add the write, role and +route-coverage cases as in the Go example. Checked with Node.js 26.13, which runs TypeScript directly (`node --test invoices.test.ts`); it has no dependencies. ```ts diff --git a/plugins/agentic-engineering/skills/application-security/references/threat-model.md b/plugins/agentic-engineering/skills/application-security/references/threat-model.md index ffa81fc..6a25ead 100644 --- a/plugins/agentic-engineering/skills/application-security/references/threat-model.md +++ b/plugins/agentic-engineering/skills/application-security/references/threat-model.md @@ -45,7 +45,10 @@ Beside each threat write one of: - **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. An agent proposes; a person accepts. + 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". @@ -68,6 +71,6 @@ must never return another company's invoice. | 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 | +| 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 | Accepted for now by the product owner: sessions end after the identity provider's timeout. Look again when company administrators can remove users | Unproven here: tracked separately | +| 6 | A former employee's session still works | Open: proposed to accept for now, because sessions end at the identity provider's timeout, and to look again when company administrators can remove users. Asked: the product owner | Not proven here; the pull request names who decided and when | From ef9e6c306b0c74badcad264d21b524745cfaa740 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 01:19:49 +0100 Subject: [PATCH 4/6] fix(agentic-engineering): close the gaps a second trial found in the examples The example store is keyed by tenant and identifier; a delete is read back; a missing caller refuses instead of panicking; public routes are marked and held to a reviewed list; the delivery store has a contract test for real implementations; and the TypeScript example goes through its session check with the tenant condition in product code. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .../application-security/references/edges.md | 30 ++- .../references/isolation-tests.md | 186 ++++++++++++------ .../references/threat-model.md | 2 +- 3 files changed, 156 insertions(+), 62 deletions(-) diff --git a/plugins/agentic-engineering/skills/application-security/references/edges.md b/plugins/agentic-engineering/skills/application-security/references/edges.md index 210444c..9723e96 100644 --- a/plugins/agentic-engineering/skills/application-security/references/edges.md +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -138,8 +138,10 @@ func Handler(secrets [][]byte, seen Deliveries, handle func(eventID string, body } ``` -Its tests. The last three steps 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. +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, so `deliveriesContract` +is written to be run against the real, database-backed one as well. ```go package webhook @@ -273,6 +275,30 @@ func TestHandler(t *testing.T) { 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() + if ran, err := seen.Once("evt-c", func() error { return errors.New("failed") }); ran || err == nil { + t.Fatalf("a failed effect: ran = %v, err = %v", ran, err) + } + calls := 0 + effect := func() error { calls++; return nil } + if ran, err := seen.Once("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("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) + } +} + +func TestMemoryDeliveriesContract(t *testing.T) { + deliveriesContract(t, &memoryDeliveries{done: map[string]bool{}}) +} ``` If recording the identifier and applying the effect cannot share a transaction, apply the diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md index 3545bc7..bb66b87 100644 --- a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -47,11 +47,12 @@ Rules that keep the tests honest: ## Go -A session check, a route table in which every route names its role, a data layer that cannot -be called without the tenant, and the tests. Checked with Go 1.27 (the route patterns need -Go 1.22 or later). Each of these makes a test fail: dropping the tenant condition from -`MemoryStore.Get` or `MemoryStore.Delete`, making `allowed` return true, and adding a route -without cases. +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 +route patterns need Go 1.22 or later). Each of these makes a test fail: looking an invoice up +by identifier alone in `MemoryStore.Get` or `MemoryStore.Delete`, making `allowed` return +true, adding a route without cases, marking a route public without adding it to the reviewed +list, and a delete that answers success without deleting. ```go package invoices @@ -80,17 +81,21 @@ type Store interface { Delete(ctx context.Context, tenantID, id string) error } -// MemoryStore is this example's data layer. In a product it is SQL, and the -// tenant condition below is `WHERE tenant_id = $1 AND id = $2`. +// 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[string]Invoice + 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[string]Invoice{}} + store := &MemoryStore{invoices: map[key]Invoice{}} for _, invoice := range invoices { - store.invoices[invoice.ID] = invoice + store.invoices[key{invoice.TenantID, invoice.ID}] = invoice } return store } @@ -98,8 +103,8 @@ func NewMemoryStore(invoices ...Invoice) *MemoryStore { func (s *MemoryStore) Get(_ context.Context, tenantID, id string) (Invoice, error) { s.mu.Lock() defer s.mu.Unlock() - invoice, ok := s.invoices[id] - if !ok || invoice.TenantID != tenantID { + invoice, ok := s.invoices[key{tenantID, id}] + if !ok { return Invoice{}, ErrNotFound } return invoice, nil @@ -108,11 +113,10 @@ func (s *MemoryStore) Get(_ context.Context, tenantID, id string) (Invoice, erro func (s *MemoryStore) Delete(_ context.Context, tenantID, id string) error { s.mu.Lock() defer s.mu.Unlock() - invoice, ok := s.invoices[id] - if !ok || invoice.TenantID != tenantID { + if _, ok := s.invoices[key{tenantID, id}]; !ok { return ErrNotFound } - delete(s.invoices, id) + delete(s.invoices, key{tenantID, id}) return nil } @@ -149,7 +153,8 @@ func Authenticate(sessions Sessions, next http.Handler) http.Handler { } // Route is one entry of the route table. Every route names the role it needs, -// so a route cannot be added without a policy, and a test can list them all. +// 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 @@ -159,6 +164,7 @@ type Route struct { var Routes = []Route{ {"GET /invoices/{id}", "member", getInvoice}, {"DELETE /invoices/{id}", "admin", deleteInvoice}, + {"GET /healthz", "public", health}, } func allowed(caller Caller, needed string) bool { @@ -168,16 +174,27 @@ func allowed(caller Caller, needed string) bool { func Handler(store Store, sessions Sessions) http.Handler { mux := http.NewServeMux() for _, route := range Routes { - mux.HandleFunc(route.Pattern, func(w http.ResponseWriter, r *http.Request) { - caller := r.Context().Value(callerKey{}).(Caller) - if !allowed(caller, route.Role) { + if route.Role == "public" { + mux.HandleFunc(route.Pattern, func(w http.ResponseWriter, r *http.Request) { + route.Handle(store, Caller{}, w, r) + }) + continue + } + mux.Handle(route.Pattern, 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) - }) + }))) } - return Authenticate(sessions, mux) + return mux +} + +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) { @@ -214,6 +231,7 @@ package invoices import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -293,11 +311,19 @@ func TestTenantIsolation(t *testing.T) { if tc.notInBody != "" && strings.Contains(body, tc.notInBody) { t.Errorf("body %q leaks %q", body, tc.notInBody) } - // A refused write must leave the owner's record as it was. - if method == http.MethodDelete && tc.wantStatus != http.StatusNoContent { - if _, err := store.Get(context.Background(), "tenant-a", "inv-a"); err != nil { - t.Errorf("the owner's invoice is gone after a refused delete: %v", err) - } + if method != http.MethodDelete { + return + } + // Read back: a refused delete leaves the owner's invoice, an allowed + // one removes exactly that invoice and nothing of the other tenant's. + _, ownErr := store.Get(context.Background(), "tenant-a", "inv-a") + _, otherErr := store.Get(context.Background(), "tenant-b", "inv-b") + deleted := tc.wantStatus == http.StatusNoContent + if deleted != errors.Is(ownErr, ErrNotFound) { + t.Errorf("after status %d the owner's invoice lookup gives %v", tc.wantStatus, ownErr) + } + if otherErr != nil { + t.Errorf("the other tenant's invoice is gone: %v", otherErr) } }) } @@ -314,9 +340,17 @@ func TestMissingAndForeignInvoicesAnswerAlike(t *testing.T) { } } +// The reviewed list of routes that need no session. Adding one is a decision. +var publicRoutes = map[string]bool{"GET /healthz": true} + func TestEveryRouteHasIsolationCases(t *testing.T) { for _, route := range Routes { - if len(isolationCases[route.Pattern]) == 0 { + switch { + case route.Role == "public": + if !publicRoutes[route.Pattern] { + t.Errorf("route %q is public but not on the reviewed list", route.Pattern) + } + case len(isolationCases[route.Pattern]) == 0: t.Errorf("route %q has no isolation cases", route.Pattern) } } @@ -326,9 +360,9 @@ func TestEveryRouteHasIsolationCases(t *testing.T) { ## TypeScript The read case for a request handler in the style of SvelteKit, where a server hook fills -`locals` after verifying the session. It shows the shape only: add the write, role and -route-coverage cases as in the Go example. Checked with Node.js 26.13, which runs TypeScript -directly (`node --test invoices.test.ts`); it has no dependencies. +`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 }; @@ -338,13 +372,34 @@ export interface InvoiceStore { get(tenantId: string, id: string): Promise; } -// The shape of a SvelteKit request handler: `locals` is filled by the server -// hook after it has verified the session. The handler never reads the tenant -// from the URL, a header or the body. -type Event = { params: { id: string }; locals: { tenantId?: string } }; +// 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); + }, + }; +} -export function getInvoice(store: InvoiceStore) { - return async ({ params, locals }: Event): Promise => { +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". @@ -357,41 +412,54 @@ export function getInvoice(store: InvoiceStore) { ```ts import assert from 'node:assert/strict'; import { test } from 'node:test'; -import { getInvoice, type Invoice, type InvoiceStore } from './invoices.ts'; +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 }, + ]), + ), +); -// Stands in for the data layer. In a product, run these tests against the real -// store and a test database: the query is the control. -function memoryStore(invoices: Invoice[]): InvoiceStore { - return { - async get(tenantId, id) { - return invoices.find((invoice) => invoice.tenantId === tenantId && invoice.id === id); - }, - }; +// 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 }); } -const handler = getInvoice( - memoryStore([ - { id: 'inv-a', tenantId: 'tenant-a', total: 100 }, - { id: 'inv-b', tenantId: 'tenant-b', total: 200 }, - ]), -); - test('the owner reads their own invoice', async () => { // The positive control: without it, a route that is simply broken passes. - const response = await handler({ params: { id: 'inv-a' }, locals: { tenantId: 'tenant-a' } }); + 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 response = await handler({ params: { id: 'inv-a' }, locals: { tenantId: 'tenant-b' } }); - assert.equal(response.status, 404); - assert.ok(!(await response.text()).includes('inv-a')); + 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 is refused', async () => { - const response = await handler({ params: { id: 'inv-a' }, locals: {} }); - assert.equal(response.status, 401); +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); }); ``` diff --git a/plugins/agentic-engineering/skills/application-security/references/threat-model.md b/plugins/agentic-engineering/skills/application-security/references/threat-model.md index 6a25ead..713de44 100644 --- a/plugins/agentic-engineering/skills/application-security/references/threat-model.md +++ b/plugins/agentic-engineering/skills/application-security/references/threat-model.md @@ -73,4 +73,4 @@ must never return another company's invoice. | 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: proposed to accept for now, because sessions end at the identity provider's timeout, and to look again when company administrators can remove users. Asked: the product owner | Not proven here; the pull request names who decided and when | +| 6 | A former employee's session still works | Open: proposed to accept for now, because sessions end at the identity provider's timeout, and to look again when company administrators can remove users. Asked: the product owner, named in the pull request | Not proven. It stays open until that person decides | From 38b0ac14282a597219a05d8a5d89d12d1499a5b5 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 02:10:22 +0100 Subject: [PATCH 5/6] fix(agentic-engineering): address the review of the application-security skill Roles and tenant membership stay current after a change. The webhook example refuses empty secrets, hands the effect the deduplication transaction, documents how retries are signed, and its contract test covers deliveries at the same moment. The isolation tests compare records after a refused write, compare foreign and missing answers for writes too, and require each kind of case per route. Signed download links, non-public addresses, per-tenant database roles and public routes are stated precisely. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .../skills/application-security/SKILL.md | 10 +- .../references/access-control.md | 7 +- .../application-security/references/edges.md | 156 +++++++++++++----- .../references/isolation-tests.md | 154 ++++++++++------- .../references/tenant-isolation.md | 9 +- .../references/threat-model.md | 2 +- 6 files changed, 231 insertions(+), 107 deletions(-) diff --git a/plugins/agentic-engineering/skills/application-security/SKILL.md b/plugins/agentic-engineering/skills/application-security/SKILL.md index 59dcfa6..0dc38c0 100644 --- a/plugins/agentic-engineering/skills/application-security/SKILL.md +++ b/plugins/agentic-engineering/skills/application-security/SKILL.md @@ -14,8 +14,10 @@ those rules. They are decided when a change is designed and proved when it is te 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. **The tenant comes from the verified session.** Never from the URL, a header, a query - parameter or the request body. +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 @@ -64,7 +66,9 @@ They win over this skill wherever they are more specific. 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. Remove + 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 diff --git a/plugins/agentic-engineering/skills/application-security/references/access-control.md b/plugins/agentic-engineering/skills/application-security/references/access-control.md index 0def8db..5ac44a3 100644 --- a/plugins/agentic-engineering/skills/application-security/references/access-control.md +++ b/plugins/agentic-engineering/skills/application-security/references/access-control.md @@ -37,8 +37,11 @@ Rules that keep them right: 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 stored facts.** The record's owner and the caller's role come from the - database and the session, never from the request. +- **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 diff --git a/plugins/agentic-engineering/skills/application-security/references/edges.md b/plugins/agentic-engineering/skills/application-security/references/edges.md index 9723e96..56c8366 100644 --- a/plugins/agentic-engineering/skills/application-security/references/edges.md +++ b/plugins/agentic-engineering/skills/application-security/references/edges.md @@ -25,7 +25,9 @@ makes a delivery trustworthy. 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. + 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. @@ -36,7 +38,8 @@ makes a delivery trustworthy. 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. + 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. @@ -47,8 +50,10 @@ timestamp and the body together; it is an example, not any provider's: package webhook import ( + "context" "crypto/hmac" "crypto/sha256" + "database/sql" "encoding/hex" "errors" "io" @@ -61,15 +66,18 @@ import ( 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. Providers differ in header -// names and layout: follow the provider's documentation, and use its library -// when it has one. +// 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 { @@ -84,6 +92,9 @@ func Verify(secrets [][]byte, eventID, timestamp string, body []byte, signature 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) @@ -96,19 +107,34 @@ func Verify(secrets [][]byte, eventID, timestamp string, body []byte, signature // Deliveries applies each event at most once. type Deliveries interface { - // Once runs effect unless eventID was handled before, and records the - // identifier only when effect succeeds. With a database, the record and - // the effect share one transaction. It reports whether effect ran. - Once(eventID string, effect func() error) (ran bool, err error) + // 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}$`) -func Handler(secrets [][]byte, seen Deliveries, handle func(eventID string, body []byte) error) http.Handler { +// 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)) @@ -127,28 +153,34 @@ func Handler(secrets [][]byte, seen Deliveries, handle func(eventID string, body http.Error(w, "invalid signature", http.StatusUnauthorized) return } - _, err = seen.Once(eventID, func() error { return handle(eventID, body) }) + _, 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, so `deliveriesContract` -is written to be run against the real, database-backed one as well. +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" @@ -156,6 +188,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "testing" "time" ) @@ -182,26 +215,28 @@ func TestVerify(t *testing.T) { cases := []struct { name string + secrets [][]byte eventID string body []byte timestamp string signature string want error }{ - {"valid delivery", "evt-1", body, fresh, sign(current, "evt-1", fresh, body), nil}, - {"signed with the previous secret during rotation", "evt-1", body, fresh, sign(previous, "evt-1", fresh, body), nil}, - {"changed body", "evt-1", []byte(`{"type":"invoice.void"}`), fresh, sign(current, "evt-1", fresh, body), ErrSignature}, - {"changed identifier", "evt-2", body, fresh, sign(current, "evt-1", fresh, body), ErrSignature}, - {"unknown secret", "evt-1", body, fresh, sign([]byte("other"), "evt-1", fresh, body), ErrSignature}, - {"signature is not hex", "evt-1", body, fresh, "not-hex", ErrSignature}, - {"missing signature", "evt-1", body, fresh, "", ErrSignature}, - {"replayed after the tolerance", "evt-1", body, old, sign(current, "evt-1", old, body), ErrStale}, - {"timestamp in the future", "evt-1", body, ahead, sign(current, "evt-1", ahead, body), ErrStale}, - {"timestamp is not a number", "evt-1", body, "soon", sign(current, "evt-1", "soon", body), ErrStale}, + {"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(secrets, tc.eventID, tc.timestamp, tc.body, tc.signature, now, 5*time.Minute) + 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) } @@ -209,29 +244,41 @@ func TestVerify(t *testing.T) { } } +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. +// 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(eventID string, effect func() error) (bool, error) { +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(); err != 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(string, []byte) error { + handle := func(context.Context, *sql.Tx, string, []byte) error { if failNext { failNext = false return errors.New("database unavailable") @@ -239,7 +286,10 @@ func TestHandler(t *testing.T) { handled++ return nil } - handler := Handler(secrets, &memoryDeliveries{done: map[string]bool{}}, handle) + handler, err := NewHandler(secrets, &memoryDeliveries{}, handle) + if err != nil { + t.Fatal(err) + } body := `{"type":"invoice.paid"}` timestamp := strconv.FormatInt(time.Now().Unix(), 10) @@ -280,24 +330,53 @@ func TestHandler(t *testing.T) { // handler relies on it, so run it against the database-backed one as well. func deliveriesContract(t *testing.T, seen Deliveries) { t.Helper() - if ran, err := seen.Once("evt-c", func() error { return errors.New("failed") }); ran || err == nil { + 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() error { calls++; return nil } - if ran, err := seen.Once("evt-c", effect); !ran || err != nil { + 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("evt-c", effect); ran || err != nil { + 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{done: map[string]bool{}}) + deliveriesContract(t, &memoryDeliveries{}) } ``` @@ -314,7 +393,9 @@ effect first, make it safe to repeat, and record the identifier after it succeed - **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. + 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 @@ -344,7 +425,8 @@ When a user supplies a URL (an avatar address, a webhook target, an import sourc becomes their proxy into your network. - Allow only `https`, and only the hosts or patterns the feature needs. -- Resolve the name, refuse private, loopback and link-local addresses, and connect to the +- 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. diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md index bb66b87..ab4939e 100644 --- a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -12,7 +12,7 @@ every route, action and job that touches tenant data: |---|---| | 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 | +| 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 | @@ -48,11 +48,13 @@ Rules that keep the tests honest: ## 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 -route patterns need Go 1.22 or later). Each of these makes a test fail: looking an invoice up -by identifier alone in `MemoryStore.Get` or `MemoryStore.Delete`, making `allowed` return -true, adding a route without cases, marking a route public without adding it to the reviewed -list, and a delete that answers success without deleting. +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, 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 @@ -127,7 +129,8 @@ type Caller struct { } // Sessions resolves a session cookie to a caller. A product verifies the -// session with its identity provider or its session store here. +// 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) } @@ -174,25 +177,30 @@ func allowed(caller Caller, needed string) bool { func Handler(store Store, sessions Sessions) http.Handler { mux := http.NewServeMux() for _, route := range Routes { - if route.Role == "public" { - mux.HandleFunc(route.Pattern, func(w http.ResponseWriter, r *http.Request) { - route.Handle(store, Caller{}, w, r) - }) - continue - } - mux.Handle(route.Pattern, 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) - }))) + 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) } @@ -234,6 +242,7 @@ import ( "errors" "net/http" "net/http/httptest" + "slices" "strings" "testing" ) @@ -249,12 +258,10 @@ var testSessions = sessions{ "b-admin": {TenantID: "tenant-b", Role: "admin"}, } -func newStore() *MemoryStore { - return NewMemoryStore( - Invoice{ID: "inv-a", TenantID: "tenant-a", Total: 100}, - Invoice{ID: "inv-b", TenantID: "tenant-b", Total: 200}, - ) -} +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 { @@ -268,6 +275,7 @@ func call(handler http.Handler, session, method, path string) *httptest.Response } type isolationCase struct { + kind string // "owner", "foreign", "anonymous" or "role" name string session string path string @@ -276,21 +284,21 @@ type isolationCase struct { notInBody string } -// One entry per route in the route table. TestEveryRouteHasIsolationCases -// fails when a route is added without one. +// 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 reads own invoice", "a-member", "/invoices/inv-a", http.StatusOK, `"inv-a"`, ""}, - {"other tenant is refused", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", "inv-a"}, - {"no session is refused", "", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, - {"unknown session is refused", "forged", "/invoices/inv-a", http.StatusUnauthorized, "", "inv-a"}, + {"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}": { - {"other tenant cannot delete", "b-admin", "/invoices/inv-a", http.StatusNotFound, "", ""}, - {"lower role cannot delete", "a-member", "/invoices/inv-a", http.StatusForbidden, "", ""}, - {"no session cannot delete", "", "/invoices/inv-a", http.StatusUnauthorized, "", ""}, - {"owner admin deletes own invoice", "a-admin", "/invoices/inv-a", http.StatusNoContent, "", ""}, + {"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, "", ""}, }, } @@ -299,7 +307,7 @@ func TestTenantIsolation(t *testing.T) { method, _, _ := strings.Cut(pattern, " ") for _, tc := range cases { t.Run(pattern+"/"+tc.name, func(t *testing.T) { - store := newStore() + store := NewMemoryStore(invoiceA, invoiceB) response := call(Handler(store, testSessions), tc.session, method, tc.path) body := response.Body.String() if response.Code != tc.wantStatus { @@ -311,48 +319,70 @@ func TestTenantIsolation(t *testing.T) { if tc.notInBody != "" && strings.Contains(body, tc.notInBody) { t.Errorf("body %q leaks %q", body, tc.notInBody) } - if method != http.MethodDelete { - return + // 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) } - // Read back: a refused delete leaves the owner's invoice, an allowed - // one removes exactly that invoice and nothing of the other tenant's. - _, ownErr := store.Get(context.Background(), "tenant-a", "inv-a") - _, otherErr := store.Get(context.Background(), "tenant-b", "inv-b") - deleted := tc.wantStatus == http.StatusNoContent - if deleted != errors.Is(ownErr, ErrNotFound) { - t.Errorf("after status %d the owner's invoice lookup gives %v", tc.wantStatus, ownErr) - } - if otherErr != nil { - t.Errorf("the other tenant's invoice is gone: %v", otherErr) + 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) { - handler := Handler(newStore(), testSessions) - foreign := call(handler, "b-admin", http.MethodGet, "/invoices/inv-a") - missing := call(handler, "b-admin", http.MethodGet, "/invoices/no-such-invoice") - if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { - t.Fatalf("a foreign invoice answers %d %q, a missing one %d %q", - foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + 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 TestEveryRouteHasIsolationCases(t *testing.T) { +func TestEveryRouteHasItsIsolationCases(t *testing.T) { for _, route := range Routes { - switch { - case route.Role == "public": + if route.Role == "public" { if !publicRoutes[route.Pattern] { t.Errorf("route %q is public but not on the reviewed list", route.Pattern) } - case len(isolationCases[route.Pattern]) == 0: - t.Errorf("route %q has no isolation cases", route.Pattern) + continue + } + 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) + } + } + } +} + +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) } } ``` diff --git a/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md b/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md index 3a1e4bb..0fb6acd 100644 --- a/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md +++ b/plugins/agentic-engineering/skills/application-security/references/tenant-isolation.md @@ -14,6 +14,9 @@ server-side; every later step reads it from there. 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. @@ -65,8 +68,10 @@ What that test run showed, and what to hold on to: - **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, give each tenant its own database role, schema or database, and say so in the - threat model. + 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 diff --git a/plugins/agentic-engineering/skills/application-security/references/threat-model.md b/plugins/agentic-engineering/skills/application-security/references/threat-model.md index 713de44..ca73123 100644 --- a/plugins/agentic-engineering/skills/application-security/references/threat-model.md +++ b/plugins/agentic-engineering/skills/application-security/references/threat-model.md @@ -73,4 +73,4 @@ must never return another company's invoice. | 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: proposed to accept for now, because sessions end at the identity provider's timeout, and to look again when company administrators can remove users. Asked: the product owner, named in the pull request | Not proven. It stays open until that person decides | +| 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 | From 6faaef1bdcc2f76fbf3414c7d854c7620a76b6fa Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 02:28:42 +0100 Subject: [PATCH 6/6] fix(agentic-engineering): close a route whose policy is missing in the example The example's policy check let a tenant admin through a route whose required role was empty or misspelt. It now admits nobody unless the policy is one it knows, with a test, and the coverage test rejects an unknown policy. Part of #640 Co-Authored-By: Claude Fable 5.2 --- .../references/isolation-tests.md | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md index ab4939e..800effd 100644 --- a/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md +++ b/plugins/agentic-engineering/skills/application-security/references/isolation-tests.md @@ -51,7 +51,8 @@ A session check, a route table in which every route names its role or is marked 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, adding a route +`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. @@ -170,8 +171,17 @@ var Routes = []Route{ {"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 { - return caller.Role == "admin" || caller.Role == needed + 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 { @@ -363,6 +373,9 @@ func TestEveryRouteHasItsIsolationCases(t *testing.T) { } 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") @@ -379,6 +392,17 @@ func TestEveryRouteHasItsIsolationCases(t *testing.T) { } } +// 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 {