diff --git a/audit/pkg/functionspec.audit.md b/audit/pkg/functionspec.audit.md new file mode 100644 index 0000000..522b6e7 --- /dev/null +++ b/audit/pkg/functionspec.audit.md @@ -0,0 +1,228 @@ +# Audit: `pkg/funcspec` + +| | | +|---|---| +| **Package** | `github.com/bitechdev/ResolveSpec/pkg/funcspec` | +| **Files** | `function_api.go` (1251), `parameters.go` (411), `hooks.go` (179), `hooks_example.go`, `security_adapter.go` (117) | +| **Tests** | `function_api_test.go` (1278), `hooks_test.go` (589), `parameters_test.go` (549) — 2 416 lines; `go test` and `go test -race` pass, 76.1 % statement coverage | +| **Audit date** | 2026-09-30 | +| **Axes** | thread locking/waiting, slowness, security, panic handling & logging | +| **Threat model** | hostile internet client; query string, headers and body are attacker-controlled | +| **Depth** | targeted (server-side request path; verified against source) | + +## Summary + +`funcspec` exposes app-defined SQL templates as endpoints. The template is +trusted; everything the client adds to it is not. The package builds SQL by +string manipulation and has two kinds of client-controlled SQL fragments +(`X-Custom-SQL-W`, `X-Custom-SQL-Or`, `sort`) that are guarded only by a keyword +denylist (`ValidSQL(..., "select")`, `function_api.go:951-980`). That is not an +injection boundary: the fragment lands inside a query that may already carry +tenant or auth predicates, and the OR path produces wrong precedence that +widens results (findings 1-3). + +The auth integration is weaker than it looks. `RegisterSecurityHooks` is opt-in, +the anonymous default is `UserID 0`, and the auth hooks return an error *and* +set `Abort`, so `Execute` returns the error first and the handler answers +**400 `hook_error`**, not 401 (finding 6). + +Error handling leaks: `sendError` returns the DB error text and the full SQL to +the client, and the panic recovery writes the panic value into the 500 body +(finding 5). + +Resource limits are absent: default limit 100 000, no cap on `X-Limit`, no +LIMIT at all when counting is skipped, unbounded `[post_body]` read, a 15-minute +timeout, and a `COUNT(1)` over the full query on every list request (finding 8). + +Positives: there are no data races under the existing tests; the `[variable]` +substitution is quote-context aware; `Content-Type` and transaction handling are +consistent; hooks run inside the transaction. + +## Findings + +| # | Severity | Axis | Finding | +|---|---|---|---| +| 1 | **High** | security | `X-Custom-SQL-W`, `X-Custom-SQL-Or` and `sort` are appended as raw SQL, protected only by a keyword denylist (`ValidSQL "select"`) | +| 2 | **High** | security | `sqlQryWhereOr` emits `a AND b OR (c)`; OR conditions escape the AND-ed predicates (auth/tenant filters) | +| 3 | **Medium** | security | `sqlQryWhere`/`sqlQryWhereOr` locate WHERE/GROUP BY/ORDER BY/LIMIT by substring on the lower-cased query, including inside literals, subqueries and CTEs | +| 4 | **Medium** | security | Unquoted string `X-FieldFilter` value in `ApplyFilters`; `SearchOps` keyed per column (one op per column, random order) | +| 5 | **High** | security / logging | `sendError` returns `err.Error()` and the full SQL; panic recovery writes the panic value to the 500 body | +| 6 | **High** | security / correctness | Auth hooks return an error plus `Abort`; handler replies 400 `hook_error` instead of 401; hooks are opt-in; anonymous = `UserID 0` | +| 7 | **Medium** | security | `DecodeParam` (`ZIP_`/`__`) is applied to every header/param, ignores errors and recurses without a depth limit; headers matched with `HasPrefix` | +| 8 | **High** | slowness | Default limit 100 000, no `X-Limit` cap, no LIMIT with `NoCount`/`skipcount`, unbounded `io.ReadAll` of `[post_body]`, 15-minute timeout, full `COUNT(1)` per list request | +| 9 | **Medium** | locking | `HookRegistry` map and `variablesCallback` are unsynchronized; `Register`/`Clear*` race with `Execute` | +| 10 | **Medium** | security | Dollar-quote substitution (`[post_body]`, `[user]`, `[method]`, …) skips backslash escaping; `[id_session]` substituted unquoted | +| 11 | **Low** | correctness | `Content-Range` offset comes from the `offset` query param only; header offset ignored | +| 12 | **Low** | correctness | `X-Select-Fields`/`X-Not-Select-Fields` accepted but no-ops; `sort` `-col` negates instead of DESC | +| 13 | **Low** | panic / logging | Recovery is handler-level only; `Serving: Records` logged at Info per request; hook/filter strings logged unscrubbed (X8) | +| 14 | **Low** | correctness | `BeforeResponse` runs post-commit on the pool, not the tx (see `audit/single_tran.md`) | +| 15 | **Low** | security | Security adapter hard-codes schema `public` and entity `sql_query`; per-entity rules cannot be applied | +| 16 | **Info** | testing | Regexes compiled per call (`ValidSQL`, `sqlStripStringLiterals`); no `-race` in CI (X1); no hostile-input tests for findings 1-4 | + +## 1. Raw SQL fragments behind a keyword denylist — High + +`ApplyFilters` (`parameters.go:283-297`) passes `X-Custom-SQL-W` and +`X-Custom-SQL-Or` through `ValidSQL(..., "select")` and splices the result into +the query. `sort` goes the same way into `ORDER BY` (`function_api.go:~226`). +The denylist (`function_api.go:964-979`) removes `;`, `--`, `/*`, `*/`, `xp_`, +`sp_` and a few keywords **followed by a space**. It is not a parser: +- Subqueries, function calls (`pg_sleep`, `pg_read_*` where permitted), + `SELECT` itself and `)` are not blocked; a `)` can close the + `COUNT(1) FROM (%s) cnts` wrapper (`function_api.go:~241`). +- Keywords are removed rather than rejected, so input can be shaped so that + removal assembles a different token. +- Whitespace variants (tab, newline) bypass the `keyword␠` patterns. + +Whether the raw fragments are reachable is decided by the handler; they are +parsed whenever `ParseParameters` runs, i.e. always. Fix: drop the two headers +from the wire contract, or accept only a column/operator/value structure built +by the server; validate `sort` against `^[A-Za-z0-9_.]+( (ASC|DESC))?(,…)*$` +and ideally an allowlist of columns. + +## 2. OR precedence widens results — High + +`sqlQryWhereOr` (`parameters.go:381-411`) rewrites `WHERE a AND b` into +`WHERE a AND b OR (c)`. SQL evaluates `AND` first, so the result is +`(a AND b) OR c`: any row satisfying `c` is returned regardless of `a`/`b`. +Where `a` is a tenant or ownership predicate in the template, a client-supplied +OR condition (`X-SearchOr`, `X-Custom-SQL-Or`, search operator with logic OR) +returns other tenants' rows. Verified with `ParseParameters` + `ApplyFilters` +on generated headers. Fix: wrap the existing WHERE body in parentheses before +appending `OR (...)`, or build a predicate tree. + +## 3. Substring-based clause location — Medium + +Both helpers use `strings.Index` on `" where "`, `" group by"`, `" order by"`, +`" limit "` over the whole lower-cased query. A match inside a string literal, +a subquery, a CTE or a column alias selects the wrong insertion point, and +`wherePos > 0` decides AND-append vs. new WHERE on the first match anywhere. +`ApplyDistinct` (`parameters.go:363-378`) similarly inserts after the first +`SELECT` substring, and the ORDER BY test (`function_api.go:~224`) compares the +first `order by` to the first `from `. `sqlStripStringLiterals` exists +(`function_api.go:858`) but is not used by these helpers. + +## 4. Filter handling inconsistencies — Medium + +- `ApplyFilters` builds `col = value` for `X-FieldFilter` without quoting the + value (`parameters.go:248-250`), so a string value becomes a column reference + (`status = active`). `mergeHeaderParams` quotes the same filter, so the + `SqlQuery` path applies it twice with different semantics. +- `RequestParameters.SearchOps` is a map keyed by column; two operators on one + column overwrite each other and map iteration order makes the generated WHERE + non-deterministic. + +## 5. Information disclosure in errors — High + +- `sendError` (`function_api.go:1150-1172`) sets `Detail = err.Error()` and, + for `*common.SQLError`, `SQL` = the final statement, including the template, + substituted values and any injected fragment. Used by every failure path + (`query_failed`, `count_failed`, `hook_error`). +- Panic recovery in `SqlQueryList` (`:80-86`) and `SqlQuery` (`:433-439`) calls + `http.Error(w, fmt.Sprintf("Internal server error: %v", err), 500)`; the + panic value reaches the client. Same class as `middleware` finding 4. +Fix: log server-side, return a generic message plus a request id. + +## 6. Auth hook abort returns 400, hooks opt-in — High + +`RegisterSecurityHooks` (`security_adapter.go:14-55`) sets `Abort`, +`AbortCode=401` **and returns an error**. `HookRegistry.Execute` +(`hooks.go:113-137`) returns the error before it evaluates `Abort`, and the +handler maps that to `sendError(400, "hook_error", …)` +(`function_api.go:~202`). The 401 branch in the handler is only reachable for +hooks that set `Abort` without returning an error. Clients therefore see 400 +with `Detail: "hook execution failed: authentication required"`. + +Also: without `RegisterSecurityHooks` there is no authentication at all; a +missing user context is replaced with `UserID 0, "anonymous"` +(`function_api.go:~103`) and the request proceeds. Fix: return nil after +setting `Abort` in the auth hooks, or have the handler honour `AbortCode` when +the error wraps an abort; consider fail-closed by default. + +## 7. Header/param decoding — Medium + +`decodeValue` (`parameters.go:203`) calls `restheadspec.DecodeParam` and drops +the error. `DecodeParam` replaces all `ZIP_`/`__` occurrences and decodes +recursively with no depth limit, so one value can force repeated base64/gzip +work (decompression amplification, since size is not capped). Header keys are +matched with `HasPrefix`, so `X-SearchOp-` variants and unrelated +headers with the same prefix are interpreted. + +## 8. Unbounded resource use — High + +- `parameters.go:54` default `Limit: 100000`; `X-Limit` and `limit` accept any + positive integer. +- In `SqlQueryList` the `LIMIT`/`OFFSET` clause is added **only inside + `if !options.NoCount`** (`function_api.go:~232-251`); `NoCount` or + `X-SkipCount` returns the whole result set. +- `COUNT(1) FROM ()` runs on every list request (double execution + cost). +- `[post_body]` uses `io.ReadAll(r.Body)` (`function_api.go:913`) with no + `http.MaxBytesReader`; the body is also embedded into the SQL text. +- `context.WithTimeout(…, 15*time.Minute)` (`:91`, `:444`) holds a transaction + and pooled connection for up to 15 minutes per request. +- `ValidSQL` and `sqlStripStringLiterals` compile regexes on each call. +Fix: hard cap on limit, always apply LIMIT, cap body size, configurable timeout. + +## 9. Unsynchronized registry — Medium + +`HookRegistry.hooks` (`hooks.go`) is a plain map; `Register`, `Clear`, +`ClearAll` mutate it while `Execute` reads it from request goroutines. Safe +only if all registration completes before serving. `Handler.variablesCallback` +(`function_api.go:60-68`) has the same property. Fix: `sync.RWMutex` and copy-on- +read, or document and enforce "register before serve". + +## 10. Dollar-quote substitution — Medium + +`safeSubstituteVar` returns the raw value when the placeholder is adjacent to +`$` (`function_api.go:1044-1049`), so neither backslash nor quote escaping +applies. The tag is neutralised only for `$M$`, `$PBODY$` and the equivalents +in `replaceMetaVariables`; a caller-supplied value in a template that uses a +different tag (or `$$`) is not. `isInsideDollarQuote` inspects only the first +occurrence of the placeholder. `[id_session]` is replaced without any quoting +(`function_api.go:~900`); its source is the auth layer, but it becomes an +injection point if a session token format allows quotes. + +## 11-12. Behavioural defects — Low + +- `Content-Range` offset uses only `r.URL.Query().Get("offset")` + (`function_api.go:~319`) while the applied offset can come from + `X-Offset`; the reported range is wrong for header-driven paging. +- `ApplyFieldSelection` (`parameters.go:226-241`) only logs; the headers have + no effect. `sort=-col` is not converted to DESC; it is emitted as `ORDER BY + -col`, which negates the column value. + +## 13. Panic handling and logging — Low + +Recovery exists per handler only (no middleware-level recovery for hooks run +outside), and the stack is logged via `logger.Error`, which forwards to Sentry +unscrubbed (X8). `logger.Info("Serving: Records …")` runs on every list request. +`logger.Debug` lines include the generated filter SQL and attacker-supplied +values. Hook failures log `err` with attacker-influenced text. + +## 14. `BeforeResponse` outside the transaction — Low + +`BeforeResponse` executes after `RunInTransaction` returns, with +`hookCtx.Tx = h.db` (`function_api.go:~336-343`, `:~640`). A hook that writes +cannot be rolled back with the query, and a failure returns 500 after the work +committed. Tracked in `audit/single_tran.md`. + +## 15. Security adapter — Low + +`funcSpecSecurityContext.GetSchema()` returns `"public"` and `GetEntity()` +returns `"sql_query"` for every endpoint (`security_adapter.go:84-92`), so +column/row security rules keyed by entity cannot distinguish funcspec endpoints. +`GetModel`, `GetQuery`, `SetQuery` are stubs. + +## 16. Testing — Info + +Tests cover handler flow, hooks and parameter parsing. No test exercises the +hostile inputs of findings 1-4 or the 401-vs-400 outcome. There is no `-race` +job in CI (X1). The earlier note about a failing +`TestReplaceMetaVariables/Replace_[user]` no longer reproduces: the package +passes today. + +## Cross-references + +X1 (no `-race`), X7 (inconsistent panic handling), X8 (logger forwards to +Sentry unscrubbed), `middleware` finding 4 (panic value in body), +`audit/single_tran.md` (post-commit hooks).