diff --git a/README.md b/README.md index 89f7139..788f301 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ ResolveSpec is a flexible and powerful REST API specification and implementation All share the same core architecture and provide dynamic data querying, relationship preloading, and complex filtering. -![1.00](./generated_slogan.webp) + ## Table of Contents @@ -859,3 +859,6 @@ This project is licensed under the MIT License - see the [LICENSE](LICENSE) file * Slogan generated using DALL-E * AI used for documentation checking and correction * Community feedback and contributions that made v2.0 and v2.1 possible + + +![1.00](./generated_slogan.webp) \ No newline at end of file diff --git a/audit/mcp_plan.md b/audit/mcp_plan.md new file mode 100644 index 0000000..61d6676 --- /dev/null +++ b/audit/mcp_plan.md @@ -0,0 +1,145 @@ +# resolvemcp rewrite plan + +Source: `audit/pkg/resolvemcp.audit.md`. Status: plan only, no code changed. + +## Goal + +Replace 4 tools + 1 resource per model with a fixed set of meta tools. +Endpoint guarded by OAuth / session token / API key; tools run as the authenticated caller. +Same rules as resolvespec CRUD, plus guardrails. + +## Decisions + +| Topic | Decision | +|---|---| +| Tools | Fixed meta tools; per-model tools/resources removed (breaking) | +| Functions | Explicit registry `Handler.RegisterFunction`; two kinds: Go callback (`func(ctx, tx, args)` + JSON-schema params) and SQL procedure by name (declared params); both behind `call_function`, run in tx with hooks | +| Create | `insert_into_table` included | +| Writes | update/delete by id **or** filters | +| Guardrails | require id or filters, max rows, `dry_run`, confirm token | +| Token scope | filter writes only; id writes = single row, no token | +| Confirm token store | in-memory, TTL, bound to user/table/filter hash; lost on restart, single instance | +| Read limits | server caps: limit, offset, batch, preload depth, timeout | +| Model exposure | all registered models visible; rules only restrict operations | +| Visibility | list tools show only what the caller may do (rules) | +| Identity | the authenticated caller's `UserContext`; **no fixed MCP user, no `SetUsername`, no service session, no background refresh** | +| Guard | endpoint always requires one of: OAuth bearer, session token, API key; no guest/optional mode | +| API key login | new `DatabaseAuthenticator.LoginWithAPIKey(ctx, rawKey)` + procedure `resolvespec_login_api_key`; validates key via keystore, creates session, returns `LoginResponse` | +| Session SQL | procedure mode + direct-SQL fallback (`ShouldUseProcedure`), same as `Login` | +| OAuth routes | `oauth2.go`/`oauth2_server.go` kept, part of the guard | +| Annotations | opt-in `Config.EnableAnnotations`, via `BeforeHandle` | + +## Open + +- None. + +## Tools + +| Tool | Purpose | +|---|---| +| `list_tables` | visible `schema.entity` + allowed ops | +| `describe_table` | columns, PK, relations, writable columns, rules, limits | +| `select_table` | filters, sort, columns, preloads, cursor; capped | +| `insert_into_table` | one or batch (capped); column allowlist | +| `update_table` | validated keys; id or filters; guardrails | +| `delete_from_table` | id or filters; guardrails | +| `list_functions` | registered functions + parameter schemas | +| `call_function` | validated args; tx + hooks + rules | + +## Config additions + +| Field | Purpose | +|---|---| +| `DefaultLimit`, `MaxLimit`, `MaxOffset` | read paging caps | +| `MaxBatch` | insert batch cap | +| `MaxPreloadDepth` | preload cap | +| `MaxWriteRows` | filter-write row cap | +| `QueryTimeout` | per-call context timeout | +| `ConfirmTTL` | confirm token lifetime | +| `EnableAnnotations` | opt-in annotate tool | + +## Guardrail rules + +| Rule | Behaviour | +|---|---| +| Target required | update/delete with neither id nor filters rejected | +| Max rows | count matches inside tx; abort above `MaxWriteRows` | +| `dry_run` | returns match count + preview, no write | +| Confirm token | filter write: first call returns token + preview; second call with token executes; bound to user, table, filter hash; expires at `ConfirmTTL` | +| Id write | single row, no token | + +## Work items + +### 1. API key login (`pkg/security`) +- Existing: keystore has `ValidateKey` and `KeyStoreAuthenticator`; `Login` needs a password; no key-to-session path. +- Add `resolvespec_login_api_key` to `SQLNames` (default + override) and a SQL script beside the existing procedures. Contract: `p_success, p_error, p_data`, input raw key; hashes, validates active/non-expired key, creates session for the key's user. +- Add `DatabaseAuthenticator.LoginWithAPIKey(ctx, rawKey)`; procedure first, direct-SQL fallback via `ShouldUseProcedure`. +- Hashed lookup; same generic error for unknown, expired or inactive key; no key material in logs. +- Expose through the chain/composite authenticators so the middleware can accept it. + +### 2. Endpoint guard +- Wire `security.NewAuthMiddleware` with a chain of OAuth bearer, session token (header/cookie) and API key. +- `SetupMux*`/`SetupBunRouter*` helpers require the guard; unauthenticated serving only when explicitly constructed without it, logged loudly. Remove `OptionalAuth*` from the MCP path. +- Caller `UserContext` flows to every tool call context; rules, RLS and `OnTxBegin` apply to that user. + +### 3. Security fixes (audit #1-6) +- Put model rules in request context (`withRequestData`) and/or `AddRegistry` on construction. +- Add `BeforeCreate` -> `CheckModelCreateAllowed` (new in `pkg/security`). +- Call `BeforeHandle` first in `executeUpdate`. +- Validate create/update keys against `ColumnValidator`; reject unknown; resolve column names from model, not json tags. +- Apply row security to update/delete pre-read; fail if row not visible. +- Annotate tool: opt-in + `BeforeHandle`. + +### 4. Limits (audit #7, #13) +- Apply default/max limit, max offset, batch cap, preload depth cap, timeout. +- Validate preload names against model relations. +- Count only when requested. + +### 5. Error and panic surface (audit #9, #17) +- Stable error codes + short message to client. +- Details and stack logged server-side. +- Recover hook panics. + +### 6. Update/create semantics (audit #10-12) +- `SET` from validated incoming keys only; explicit null supported. +- Lock row (`FOR UPDATE`) on update. +- Refetch and `After*` hooks inside the same tx, or report committed write with warning if not possible (see `audit/single_tran.md`). + +### 7. Smaller fixes (audit #8, #14, #15) +- SSE pool: require `BaseURL` or cap/evict; allowlist Host. +- Uniform not-found vs hook error text. +- Add mutex to `HookRegistry`. + +### 8. Meta tools +- New file for meta tools; reuse parse helpers and `buildModelInfo` for `describe_table`. +- Remove per-model register functions and resources. +- `RegisterModel` only registers to registry. +- Function registry (Go callback kind + SQL procedure kind) + validation of args against declared schema. + +### 9. Tests +- Update `tx_test.go` (calls `executeRead/Create/Update/Delete`) and `tools_test.go`. +- New: rule enforcement, unknown keys, limits, guardrails (cap, dry_run, token expiry/binding), guard rejects unauthenticated, API key login (valid, expired, inactive, unknown), visibility filtering, `-race`. +- Check for existing test data first; ask before generating any. + +### 10. Docs +- Rewrite `pkg/resolvemcp/README.md` cheatsheet style. +- Document `resolvespec_login_api_key` in `pkg/security` docs. +- Update root README references. +- Update audit file when findings are closed. + +## Order + +1. `LoginWithAPIKey` + procedure in `pkg/security` (1) +2. Guard + security fixes (2-3) +3. Limits, errors, update/create semantics (4-6) +4. Meta tools + function registry (8) +5. Smaller fixes (7) +6. Tests (9), docs (10) + +## Breaking changes + +- Per-model tools and resources gone. +- MCP endpoint requires authentication. +- Annotate tool off by default. +- Update/create reject unknown keys. +- Reads capped by default. diff --git a/audit/pkg/resolvemcp.audit.md b/audit/pkg/resolvemcp.audit.md new file mode 100644 index 0000000..6f1d8df --- /dev/null +++ b/audit/pkg/resolvemcp.audit.md @@ -0,0 +1,121 @@ +# Audit: `pkg/resolvemcp` + +| | | +|---|---| +| **Package** | `github.com/bitechdev/ResolveSpec/pkg/resolvemcp` | +| **Files** | `handler.go` (901), `tools.go` (720), `cursor.go`, `oauth2.go`, `oauth2_server.go`, `annotation.go`, `hooks.go`, `security_hooks.go`, `context.go`, `resolvemcp.go` | +| **Tests** | `tools_test.go` (34), `tx_test.go` (207); `go test` passes. No hostile-input tests, no `-race` | +| **Audit date** | 2026-09-30 | +| **Axes** | thread locking/waiting, slowness, security, panic handling & logging, agent usability | +| **Threat model** | hostile or confused MCP client (LLM agent, possibly prompt-injected); tool arguments are attacker-controlled | +| **Depth** | targeted (request path, security wiring; verified against source) | + +## Summary + +Every model registers 4 tools + 1 resource (`read_/create_/update_/delete__`), each with an +inlined column list, relation list and schema doc. Tool list grows 4N; context cost is +paid on every session whether or not the table is used. Replace with fixed meta tools +(see Rewrite). + +Security wiring fails open in several places: model rules never reach the hooks, +`create` has no rule check, `update` skips `BeforeHandle`, update/delete skip row-level +security, and create/update write client-chosen column names. Reads have no size cap. +`resolvespec_annotate` is an unauthenticated write channel into agent-visible text. + +## Findings + +| # | Severity | Axis | Finding | +|---|---|---|---| +| 1 | **High** | security | Model rules set via `RegisterModelWithRules` never reach `security.Check*`: handler uses a private registry that is not `modelregistry.AddRegistry`'d; hooks look up the global list | +| 2 | **High** | security | `create` has no rule check: `CheckModelAuthAllowed` only tests `CanPublicCreate`/auth; no `BeforeCreate` hook registered, `CanCreate` never read | +| 3 | **High** | security | `executeUpdate` never fires `BeforeHandle` (create/read/delete do); auth + public-rule check skipped, only `BeforeUpdate` (`CanUpdate`) runs | +| 4 | **High** | security | Create/update data keys are not validated against model columns (`q.Value(key,…)`, `SetMap(existingMap)`): mass assignment of any column, arbitrary identifiers | +| 5 | **High** | security | Row-level security (`ApplyRowSecurity`) is wired to `BeforeRead` only; update/delete by id bypass app-level RLS (DB-level RLS via `OnTxBegin` still applies) | +| 6 | **High** | security | `resolvespec_annotate` has no auth/rule check, writes through `h.db` (outside tx, no `OnTxBegin`), any `tool_name` key; annotations are agent-facing text, so it is a prompt-injection store | +| 7 | **High** | slowness | No default/max `limit`, no max `offset`, `COUNT(*)` on every read, `[]` batch create unbounded, no statement timeout | +| 8 | **Medium** | security | `dynamicSSEHandler.pool` keyed by `Host` + `X-Forwarded-Proto` (attacker-controlled): unbounded map growth and poisoned `message` endpoint URL sent to the client | +| 9 | **Medium** | security / logging | Raw `err.Error()` (DB errors, hook errors, panic value `"internal error: %s"`) returned as tool text; `logger.Error` of the same forwards to Sentry (X8) | +| 10 | **Medium** | correctness | Update reads row, merges **json-tag keys** into `SetMap` as column names, writes every column back; breaks when json tag ≠ db column, clobbers concurrent edits (no `FOR UPDATE`) | +| 11 | **Medium** | correctness | Update ignores `nil` and `""` values: a column cannot be set to NULL or empty | +| 12 | **Medium** | correctness | Create/update commit tx 1, then run tx 2 (refetch + `AfterCreate`). Tx 2 failure returns an error for a committed write; an agent retry duplicates the insert | +| 13 | **Medium** | security | Preload relation names are passed straight to `PreloadRelation` without checking the model's relations; no depth/breadth cap | +| 14 | **Low** | security | Update/delete distinguish `record not found` from hook errors, so ids can be enumerated by error text | +| 15 | **Medium** | locking | `HookRegistry.hooks` map unsynchronized; `Register`/`Clear*` race with `Execute` (same as funcspec #9) | +| 16 | **Low** | security | Filter columns are validated by `ColumnValidator` for reads only; sort/column values are interpolated unquoted after validation (relies on validator being exact); `CustomOperators`/`ComputedColumns` unreachable from tools today, keep it that way | +| 17 | **Low** | panic | `recoverPanic` returns the panic value to the client and loses the stack; hook panics in `Execute` are not recovered before the handler-level recover | +| 18 | **Low** | agent usability | Tool names embed schema+entity (`read_public_users`); no discovery tool, so clients cannot list tables without loading every tool schema | +| 19 | **Info** | testing | No tests for auth/rule enforcement, hostile filters, key validation, limits, or `-race` | + +## Details + +### 1. Rules invisible to hooks (High) +`NewHandlerWithGORM/Bun/DB` call `modelregistry.NewModelRegistry()`. `security` resolves rules +via `GetModelRulesFromContext` then `modelregistry.GetModelRulesByName`, which walks the +**global** list (`registries`). The handler registry is never added, so +`ErrModelNotFound` → `CheckModelUpdate/DeleteAllowed` return `nil` (allow) and +`CheckModelAuthAllowed` falls back to "auth required, public flags ignored". +`CanUpdate=false`, `CanDelete=false` are not enforced. Fix: put rules into the +request context in `withRequestData` (`security.ModelRulesKey`) and/or `AddRegistry` on +construction. + +### 2-3. Create/update gating (High) +`CheckModelAuthAllowed(op)` handles public flags only. Add `BeforeCreate` → +`CheckModelCreateAllowed` (new, mirrors update/delete), and call `BeforeHandle` at the +top of `executeUpdate`. + +### 4. Column allowlist (High) +Validate every key in create/update `data` against `common.NewColumnValidator(model)`; +reject unknown keys with an error (do not silently drop on writes). Also consider a +per-model writable-column set (excluding PK, `CanPublic*`-guarded columns) for agents. + +### 5. RLS on writes (High) +Run `LoadSecurityRules` + a row predicate on the update/delete pre-read query; fail the +write when the row is not visible to the user. + +### 6. Annotation tool (High) +Remove from default registration or gate behind `BeforeHandle` + explicit rule. Values +returned to the agent must be treated as data, not instructions. + +### 7. Limits (High) +Server config: `DefaultLimit` (e.g. 50), `MaxLimit`, `MaxOffset`, `MaxBatch`, `MaxPreloadDepth`, +per-call `context.WithTimeout`. Skip `COUNT(*)` unless requested (`with_count`). + +### 8. SSE pool (Medium) +Require `Config.BaseURL` for SSE, or cap/evict `pool`, and validate `Host` against an +allowlist. + +### 9/17. Error surface (Medium/Low) +Map errors to stable codes + short message; log details server-side with stack +(`logger.HandlePanic`). + +### 10-12. Update/create semantics (Medium) +Build the `SET` only from validated incoming keys (column names resolved from model, +not json tags); use `NULL` for explicit null; single tx including refetch and `After*` +hooks (see `audit/single_tran.md`), or return success + warning when tx 2 fails. + +## Rewrite (agreed design) + +Replace per-model tools with fixed meta tools. Decisions recorded 2026-09-30: + +| Decision | Choice | +|---|---| +| Functions source | Explicit registry: `Handler.RegisterFunction(name, meta, fn)`; only registered functions visible/callable | +| Old tools/resources | Removed (breaking) | +| Discovery | `list_tables`, `describe_table`, `list_functions` | +| Create | `insert_into_table` added | +| Write scope | update/delete by id **or** filters; max-rows cap, `dry_run` and confirm token apply to filter writes; id writes are single-row, no token | +| Guardrails | require id/filter, max rows affected, `dry_run`, confirm token | +| Read limits / ACL | server caps (limit, offset, preload depth); list tools filtered per caller rules | +| Identity | Authenticated caller's `UserContext`; no fixed MCP user. Endpoint guarded by OAuth / session token / API key (new `resolvespec_login_api_key`); no guest mode | +| Annotations | `resolvespec_annotate` becomes opt-in (`Config.EnableAnnotations`) and goes through `BeforeHandle` | + +| Tool | Purpose | +|---|---| +| `list_tables` | registered `schema.entity` visible to caller, with allowed ops | +| `describe_table` | columns, PK, relations, writable columns, rules, limits for one table | +| `select_table` | filters/sort/columns/preloads/cursor; capped | +| `insert_into_table` | one or batch (capped); column allowlist | +| `update_table` | validated keys; guardrails | +| `delete_from_table` | guardrails | +| `list_functions` | registered functions + parameter schemas | +| `call_function` | validated args, tx + hooks |