mirror of
https://github.com/bitechdev/ResolveSpec.git
synced 2026-10-01 19:20:31 +00:00
229 lines
13 KiB
Markdown
229 lines
13 KiB
Markdown
# 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-<anything>` 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 (<full query>)` 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).
|