Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

Column authorization now lives in the one place that already enumerates *every* column reference — the SQL builder (`query.Build`, made policy-aware) — so the projection, aggregation arguments, `filters`, `group_by`, `order_by`, and `time_range` are all checked against the role's allowlist in a single pass, and no current or future clause can silently skip the check. A full-row read is now requested explicitly with `"select_all": true`, which for a column-restricted role expands to exactly that role's allowed columns, so denied columns never reach ClickHouse; unrestricted roles and admin keep `SELECT *`; a role allowed the table but no columns fails closed with `403`. **Omitting `columns` now returns nothing** (an empty result) rather than expanding to a projection — so a hidden column can't leak by being left out at all. Because the generated SQL is now role-specific, the read cache key partitions by column visibility for free — one role can no longer be served another's cached columns. The structured-query and live-stream paths now share the same per-column decision (`policy.IsColumnAllowed`, plus new `AllowedProjection`/`RestrictsColumns` helpers), so the two read surfaces enforce identical column visibility and can't drift apart. New unit, handler, and end-to-end (SDK → WaveHouse → ClickHouse) tests cover each bypass shape as explicit red-team cases. One intended behavior change falls out of authorizing `order_by`/`group_by`/`filter`: a column-restricted role can no longer order, group, or filter by a column it cannot read — those clauses must reference only readable columns (unrestricted roles and admin are unaffected). The SDK's bare `.fetch()` maps to `select_all`, so it still returns a full row set (the same-release #270 fix already dropped its implicit `ORDER BY received_timestamp` default).
- **Structured queries accept any identifier ClickHouse accepts (bring-your-own schemas), with every identifier backtick-quoted to stay injection-safe** (`internal/chsql/` (new), `internal/query/builder.go`, `internal/query/ast.go`, `internal/policy/policy.go`, `internal/api/structured_query.go`, `clients/ts/src/query-builder.ts`, `clients/ts/src/types.ts`, `docs/src/content/docs/access-control.md`, `docs/src/content/docs/api.md`, plus tests in `internal/query/{builder,ast}_test.go`, `internal/api/structured_query_test.go`, `internal/policy/policy_test.go`, `internal/chsql/chsql_test.go`, `tests/integration/identifier_roundtrip_test.go` (new), `tests/e2e/sdk/{query,cache,ingest}.test.ts`): table names, column names, and aggregation aliases may now contain dots, spaces, unicode, SQL keywords, even embedded backticks/backslashes — anything ClickHouse permits in a quoted identifier. The builder and the policy row-filter path route every identifier through one shared `chsql.QuoteIdent` that escapes exactly as ClickHouse's own `backQuote()` does (`\` → `\\`, `` ` `` → `` \` ``, verified against `SHOW CREATE TABLE` on a live server). This replaces the prior safe-identifier regex, which rejected legitimate columns in existing customer schemas. It also **subsumes the earlier aggregation-alias injection hardening**: an alias such as `n FROM secrets --` is now neutralized by quoting (it becomes an inert column label) rather than rejected, so callers keep arbitrary alias names while the reparent/statement-break/subquery payloads still cannot break out. Column and table references stay bounded by schema membership (the injection boundary for those) plus the per-column allowlist (#223); aliases carry no membership but are fully contained by quoting. The lone refusal is an identifier literally containing `?` — clickhouse-go's positional value-binder counts every `?` in the query text, even inside a quoted identifier, so such a name is rejected fail-closed rather than risk shifting value parameters (tracked in [#279](https://github.com/Wave-RF/WaveHouse/issues/279)). Two related API changes fall out of treating names literally: in a query, **`*` is now a literal column name** (quoted, gated by schema membership), not a wildcard — the all-columns read moved to an explicit **`select_all`** boolean (`columns` and `select_all` are mutually exclusive; omitting both selects nothing), and the SDK gains `.selectAll()` with a bare `.fetch()` mapping to it. `count(*)` still means count-all, and `allow_columns: ["*"]` is still the policy wildcard. The `columns` field also now accepts a single string as shorthand for a one-element list. Values are unchanged (positional `?`, driver-escaped). New real-ClickHouse round-trip tests cover weird column/table/alias names (including embedded backticks and backslashes), injection containment, and the `*`/`select_all` contract; server-side `{name:Identifier}` params were evaluated and rejected because clickhouse-go cannot mix them with positional value binding.
- **Pipe parameters can no longer inject SQL through a non-scalar value** (`internal/pipes/pipes.go`, plus tests in `internal/pipes/pipes_test.go`, `internal/api/pipes_test.go`, `tests/e2e/sdk/admin.test.ts`, and docs `docs/src/content/docs/{pipes.mdx,api.md,architecture.md}`): closes #317. Pipes bind parameters by inlining escaped literals into the SQL template (no positional `?` binds), and `formatParamValue`'s fallback branch emitted whatever Go's `fmt.Sprintf("%v")` produced — so a JSON **array** or **object** supplied as a parameter value reached ClickHouse **raw and unescaped**. In a string-context placeholder an array element's inner `'` terminated the literal, giving a clean `UNION`/boolean injection that bypassed the per-column allowlist the structured-query path enforces. The execute route (`GET/POST /v1/pipes/{name}`) is authorized only by the pipe's `allowed_roles` (commonly `[public]`), so this was reachable unauthenticated. `formatParamValue` is now recursive and fails closed: every scalar leaf — including each element of an array — is escaped, a JSON array renders as a parenthesized `(v1, v2, …)` list (the same `IN (…)` shape the structured-query builder emits, so list parameters such as `WHERE id IN {{ids}}` now work safely), and a value with no scalar SQL form — a JSON object, or an empty array that would render as the invalid `IN ()` — is rejected with a `400` rather than emitted raw. A numeric-looking query-string value still renders bare (so `?limit=100` works as a number) via a strict numeric-literal check; any other string is single-quote-escaped, so it can never become invalid or injectable bare SQL. The declared parameter `type` stays advisory metadata (documenting intent for callers and the SDK), with binding keyed off the runtime value as before. New unit tests cover the array/object/empty/nested-list cases plus the original injection payloads as red-team regressions, new handler tests cover the array-binds and object-rejected paths, and an end-to-end SDK test executes an `IN`-list pipe against real ClickHouse with a quote-bearing element to exercise escaping.
- **The public read endpoints now bound the request body they decode, closing a single-request OOM vector** (`internal/api/query.go`, `internal/api/structured_query.go`, `internal/api/pipes.go`, `internal/api/policy.go`, `docs/src/content/docs/api.md`, plus tests in `internal/api/structured_query_test.go`, `internal/api/pipes_test.go`, and `internal/api/policy_test.go`): closes #315. `POST /v1/query` and `GET/POST /v1/pipes/{name}` called `json.NewDecoder(r.Body).Decode(...)` with no `http.MaxBytesReader`, so a large JSON array — e.g. a giant `in`-list `filters[].value` — decoded whole. JSON amplifies roughly an order of magnitude (~13×) bytes→live-heap into Go's in-memory representation (interface boxing, slice-growth doubling, GC headroom), so a single ~50 MiB request inflated to hundreds of MiB and a few concurrent ones OOM-killed the box — a *memory*, not throughput, vector that a per-IP rate cap doesn't mitigate. Both decoders now wrap the body in a new 1 MiB `maxControlBodyBytes` cap (a query/pipe-parameter body is bounded by nature, far under 1 MiB even with a large `in`-list) and return `413 request body exceeded 1048576 bytes` via the same `writeMaxBytesError` mapping the ingest/admin handlers already use — bringing the public read path to body-cap parity with the previously-hardened `/v1/ingest` and `/v1/admin/query` (16 MiB, the data-plane cap). The same cap is extended to the remaining body-decoding handlers in the package so none is left as an OOM gap — the admin pipe-definition `PUT /v1/admin/pipes/{name}` and the admin policy decoders `PUT /v1/admin/policy` + `POST /v1/admin/policy/validate` (a pipe or policy document is a bounded description). The pipe `Execute` parameter path keeps its lenient fallback for a malformed-but-within-cap body (parameters may come from the query string alone) and only hard-stops with `413` on an over-cap body. The cap is a fixed in-code backstop, not a tuning knob — operators set their own outer limit at the [reverse proxy](https://wavehouse.dev/reverse-proxy#request-body-size-limits); see the new deployment guide in this release.
- **Dropped chi's deprecated `middleware.RealIP` from the router** (`internal/api/router.go`, `docs/src/content/docs/reverse-proxy.mdx`): closes #281. It rewrote `r.RemoteAddr` from spoofable forwarded headers (`X-Forwarded-For` / `X-Real-IP` / `True-Client-IP`) on every request, whether or not a trusted proxy set them — the IP-spoofing GHSAs chi deprecated it for (SA1019; GHSA-3fxj-6jh8-hvhx / GHSA-rjr7-jggh-pgcp / GHSA-9g5q-2w5x-hmxf), which also blocked the weekly go-deps dependabot group on the chi bump (#209). Nothing in WaveHouse read `r.RemoteAddr` (no per-IP logic — rate limiting is the reverse proxy's job), so removing it is behavior-preserving and drops the spoofing vector; `r.RemoteAddr` is now the honest immediate peer (the proxy, when one fronts WaveHouse). Capturing the real client IP **safely** — trusted-proxy-aware and surfaced in traces and logs, plus gating trace-context propagation on the same trust boundary — is tracked as a follow-up (#333).

Expand Down
2 changes: 2 additions & 0 deletions docs/src/content/docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,8 @@ The POST parameter body is capped at 1 MiB; a body over the cap is rejected with
| 404 | `{"error":"pipe not found"}` | Pipe name not registered |
| 403 | `{"error":"forbidden"}` | Role not in pipe's `allowed_roles` (and not the admin role). Fails closed: a request with no role (no token, or a JWT missing `auth.role_claim`) is denied unless a `default_role` resolves it into the list; a pipe with no `allowed_roles` denies everyone but the admin role. |
| 400 | `{"error":"missing required parameter: x"}` | Required parameter not supplied |
| 400 | `{"error":"parameter \"x\": unsupported parameter type object"}` | A non-scalar value with no SQL literal form — a JSON object, whether supplied directly or nested as an array element. A JSON **array** is valid and renders as an `IN`-style `(…)` list. |
| 400 | `{"error":"parameter \"x\": array parameter must not be empty"}` | An empty array — it would render as the invalid `IN ()`. |
| 413 | `{"error":"request body exceeded 1048576 bytes"}` | POST body over the 1 MiB cap |

---
Expand Down
2 changes: 1 addition & 1 deletion docs/src/content/docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ The package's design invariants — stdout always 100%, WARN+ERROR always export

### `pipes/` — Named Query Pipes

- **pipes.go** — `NamedQuery` type with SQL template and parameter definitions, `Store` backed by NATS KV bucket `WAVEHOUSE_PIPES`. Supports `.sql` file directory bootstrap. `BindParams()` resolves `{{param}}` / `{{param:default}}` placeholders by inlining escaped literal values into the SQL (strings are single-quote-escaped).
- **pipes.go** — `NamedQuery` type with SQL template and parameter definitions, `Store` backed by NATS KV bucket `WAVEHOUSE_PIPES`. Supports `.sql` file directory bootstrap. `BindParams()` resolves `{{param}}` / `{{param:default}}` placeholders by inlining escaped literal values into the SQL (strings single-quote-escaped; arrays rendered as escaped `(…)` `IN`-lists). A non-scalar value with no SQL form (a JSON object, or an empty array) is rejected rather than emitted raw.

### `query/` — Structured Query Engine

Expand Down
23 changes: 21 additions & 2 deletions docs/src/content/docs/pipes.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ The `parameters` array formally declares a parameter with metadata:
| Field | Type | Description |
| ----- | ---- | ----------- |
| `name` | string | Placeholder name; matches `{{name}}` in the SQL. |
| `type` | string | Documents the expected type (`"string"`, `"number"`, `"boolean"`). Descriptive — it advertises intent to callers and the SDK; binding itself keys off the runtime value. |
| `type` | string | Documents the expected value type (`"string"`, `"number"`, `"boolean"`, `"array"`). Descriptive — it advertises intent to callers and the SDK; binding itself keys off the runtime value. |
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
| `required` | bool | If `true`, the caller must supply a value, even if the placeholder doesn't appear in the SQL. A missing required parameter is a `400`. |
| `default` | any | Value used when the caller supplies none. |

Expand Down Expand Up @@ -87,9 +87,27 @@ Bound values are **inlined directly into the SQL string** (not sent as positiona
| numeric string (e.g. `"100"`) | `100` | emitted bare, so `?limit=100` from a query string works as a number |
| number | `42` / `42.5` | whole floats render as integers |
| boolean | `1` / `0` | ClickHouse-style |
| array | `('a', 'b')` | parenthesized list of escaped elements — for `IN` clauses (see below) |
| null | `NULL` | |

Because string values are single-quote-escaped, a parameter value can't break out of its literal and inject SQL. The SQL *structure* still comes only from the operator-authored template.
Every leaf value is escaped the same way — including each element of an array — so a parameter value can't break out of its literal and inject SQL. The SQL *structure* still comes only from the operator-authored template. Values with no safe scalar form are **rejected** with a `400`: a JSON object, and an empty array (which would render as the invalid `IN ()`).

#### Array parameters and `IN` lists

A parameter whose value is a JSON array renders as a ClickHouse list literal — `(v1, v2, …)`, every element escaped — ready for an `IN` clause. Write the placeholder **without** surrounding parentheses; the value brings its own:

```sql
SELECT page, count() AS views FROM clicks WHERE page IN {{pages}} GROUP BY page
```

```bash
curl -X POST http://localhost:8080/v1/pipes/by_pages \
-H "Content-Type: application/json" \
-d '{"pages": ["/home", "/pricing", "/docs"]}'
# binds to: … WHERE page IN ('/home', '/pricing', '/docs')
```

Array values must come from a **JSON body** (`POST`) — query-string parameters are always scalar strings, so `GET …?pages=a&pages=b` binds only the first occurrence (`a`) as a string, ignoring the rest.

## Authorizing a pipe

Expand Down Expand Up @@ -158,6 +176,7 @@ The response is a JSON array of rows. Results flow through the shared in-process
| 404 | `{"error":"pipe not found"}` | No pipe registered under that name |
| 403 | `{"error":"forbidden"}` | Caller's role isn't in `allowed_roles` (and isn't admin) |
| 400 | `{"error":"missing required parameter: x"}` | A required parameter wasn't supplied |
| 400 | `{"error":"parameter \"x\": unsupported parameter type object"}` | A non-scalar value with no SQL form — a JSON object (directly, or nested in an array). An empty array is likewise rejected (`array parameter must not be empty`). |

## Bootstrapping from `.sql` files

Expand Down
46 changes: 46 additions & 0 deletions internal/api/pipes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,52 @@ func TestPipesHandler_Execute_NoAllowedRoles_NonAdminDenied(t *testing.T) {
testutil.AssertJSONErrorResponse(t, w)
}

// TestPipesHandler_Execute_ArrayParamBinds: an array body param renders into an
// IN list and passes binding (failing only later at the nil ClickHouse conn).
func TestPipesHandler_Execute_ArrayParamBinds(t *testing.T) {
t.Parallel()
store := pipes.NewMemoryStore(
&pipes.NamedQuery{
Name: "by_ids",
SQL: "SELECT * FROM clicks WHERE id IN {{ids}}",
Parameters: []pipes.ParamDef{{Name: "ids", Type: "array", Required: true}},
},
)
h := NewPipesHandler(store, policy.NewMemoryStore(&policy.Policy{}), nil, nil, 0, testutil.NopLogger())

w := httptest.NewRecorder()
body := map[string]any{"ids": []any{"a", "b"}}
r := pipesRequest(t, http.MethodPost, "/v1/pipes/by_ids/execute", "by_ids", body)
r = r.WithContext(auth.WithRole(r.Context(), "admin"))

safeHandle(h.Execute, w, r)

// Binding succeeded — the only failure left is the nil conn, never a 400.
assert.NotEqual(t, http.StatusBadRequest, w.Code)
assert.NotEqual(t, http.StatusNotFound, w.Code)
}

// TestPipesHandler_Execute_ObjectParamRejected: a JSON object has no scalar SQL
// form and is refused with a 400 before any query runs (#317).
func TestPipesHandler_Execute_ObjectParamRejected(t *testing.T) {
t.Parallel()
store := pipes.NewMemoryStore(
&pipes.NamedQuery{Name: "by_col", SQL: "SELECT * FROM clicks WHERE col = {{p}}"},
)
h := NewPipesHandler(store, policy.NewMemoryStore(&policy.Policy{}), nil, nil, 0, testutil.NopLogger())

w := httptest.NewRecorder()
body := map[string]any{"p": map[string]any{"k": "v"}}
r := pipesRequest(t, http.MethodPost, "/v1/pipes/by_col/execute", "by_col", body)
r = r.WithContext(auth.WithRole(r.Context(), "admin"))

h.Execute(w, r)

assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Contains(t, w.Body.String(), "unsupported parameter type object")
testutil.AssertJSONErrorResponse(t, w)
Comment thread
taitelee marked this conversation as resolved.
Resolved
}

// TestPipesHandler_Execute_NoAllowedRoles_AdminAllowed: the privileged built-in
// roles bypass the allowlist, so admin can run a pipe with no allowed_roles.
func TestPipesHandler_Execute_NoAllowedRoles_AdminAllowed(t *testing.T) {
Expand Down
Loading