From 7ae9721c2bd3ad98865ffab28b11ecc7cb2ff4fc Mon Sep 17 00:00:00 2001 From: viettranx Date: Sun, 12 Apr 2026 10:48:31 +0700 Subject: [PATCH] docs(contrib): tenant-scope guard rules for admin writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Role checks are not tenant checks — a non-master tenant admin holds RoleAdmin in their own tenant and passes role-only middleware by design. CLAUDE.md gains a one-line directive pointing at CONTRIBUTING for the full decision table + anti-patterns. CONTRIBUTING gains: - Target-table decision table (global vs tenant-scoped) with the matching guard for each (requireMasterScope vs requireTenantAdmin). - Shared predicate reference: store.IsMasterScope(ctx). - Anti-pattern list for reviewers: writes to no-tenant_id tables without master-scope check, SQL tenant_id IS NULL arms on write paths, role-only admin gates, revoke/delete handlers that skip pre-fetch ownership verification. --- CLAUDE.md | 1 + CONTRIBUTING.md | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8d891e1e..85748225 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -197,6 +197,7 @@ Go conventions to follow: - **SQL safety:** When implementing or modifying SQL store code (`store/pg/*.go`), always verify: (1) All user inputs use parameterized queries (`$1, $2, ...`), never string concatenation — prevents SQL injection. (2) Queries are optimized — no N+1 queries, no unnecessary full table scans. (3) WHERE clauses, JOINs, and ORDER BY columns use existing indices — check migration files for available indexes - **DB query reuse:** Before adding a new DB query for key entities (teams, agents, sessions, users), check if the same data is already fetched earlier in the current flow/pipeline. Prefer passing resolved data through context, event payloads, or function params rather than re-querying. Duplicate queries waste DB resources and add latency - **Solution design:** When designing a fix or feature, identify the root cause first — don't just patch symptoms. Think through production scenarios (high concurrency, multi-tenant isolation, failure cascades, long-running sessions) to ensure the solution holds up. Prefer explicit configuration over runtime heuristics. Prefer the simplest solution that addresses the root cause directly +- **Tenant-scope guards on admin writes:** `RoleAdmin` is not a tenant check. Writes to **global** tables (no `tenant_id` column — e.g. `builtin_tools`, disk config, package mgmt) must gate with `http.requireMasterScope` / WS `requireMasterScope(requireOwner(...))`. Writes to **tenant-scoped** tables must gate with `http.requireTenantAdmin` + SQL `WHERE tenant_id = $N`. Shared predicate: `store.IsMasterScope(ctx)`. See `CONTRIBUTING.md` → "Tenant-scope guards" for the full decision table and anti-patterns. ## Mobile UI/UX Rules diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e1dd3ce9..1ddc6d2b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,11 +50,28 @@ Based on our automated review checklist: - **Correctness**: No logic errors, nil dereference, race conditions - **Security**: Parameterized SQL, no hardcoded secrets, input validation - **Breaking changes**: API contracts, DB migrations, config format -- **Tenant isolation**: All queries scoped by `tenant_id` +- **Tenant isolation**: All queries scoped by `tenant_id`. **Admin writes require the correct scope guard** — see section below - **i18n**: User-facing strings in all 3 locales (en/vi/zh) - **SQLite parity**: Changes compile with `-tags sqliteonly` - **Mobile UI**: `h-dvh` not `h-screen`, 16px input fonts, safe areas +### Tenant-Scope Guards + +`RoleAdmin` checks role, not tenant. A non-master tenant admin holds `RoleAdmin` in their own tenant and passes role-only middleware. Pick the guard by the **target table**: + +| Target | Example | Guard | +|---|---|---| +| **Global** (no `tenant_id` column) | `builtin_tools`, disk config, `pip`/`npm`/`apk` | HTTP `requireMasterScope` · WS `requireMasterScope(requireOwner(...))` | +| **Tenant-scoped** (has `tenant_id` column) | `agents`, `skills`, `llm_providers` | `requireTenantAdmin` + store SQL `WHERE tenant_id = $N` | + +Shared predicate: `store.IsMasterScope(ctx)` (`internal/store/context.go`). + +**Anti-patterns flagged in review:** +- `store.Update(...)` on a no-`tenant_id` table without a master-scope check upstream +- Write SQL with `WHERE ... (tenant_id = $N OR tenant_id IS NULL)` — the `IS NULL` arm lets tenants reach system rows +- `requireAuth(RoleAdmin)` as the **sole** gate on a global-state write +- Admin revoke/delete handlers that skip pre-fetch ownership verification (store SQL alone is not enough when it matches `IS NULL` arms) + ### Commit Messages Use conventional commits: