hanahmily commented on code in PR #1209: URL: https://github.com/apache/skywalking-banyandb/pull/1209#discussion_r3541263665
########## pkg/bydbql/binder.go: ########## @@ -0,0 +1,310 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package bydbql + +import ( + "fmt" + "math" + "time" + + modelv1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/model/v1" +) + +// BindParams binds positional parameters to the `?` placeholders in the parsed grammar, +// in their order of appearance in the query text. After a successful call, the grammar is +// indistinguishable from one parsed with literal values, so the transformer needs no +// placeholder awareness. Callers must invoke it before Transform even when params is empty, +// so a query holding unbound placeholders is rejected instead of silently misinterpreted. +// Parameter positions in error messages are 1-based. On error the grammar may be partially +// bound and must be discarded; binding is not retryable on the same grammar. +func BindParams(g *Grammar, params []*modelv1.TagValue) error { Review Comment: **HIGH — `BindParams` mutates the parsed AST in place, so the parse-once/bind-many reuse this feature is meant to unlock isn't possible. Please track the immutable-bind redesign as a follow-up PR and guard the leak here.** **Why reuse is the goal.** Positional parameter binding has two payoffs, and this PR banks only one. The first is injection safety (fully delivered). The second — the classic reason prepared statements exist — is amortizing parse/plan cost across many executions of the same query shape with different values: lex + parse + grammar-build happens once, and each request only binds fresh params and transforms. Our own wire contract already models this — `QueryRequest{ query, params }` is a prepared statement — so consumers (high-QPS dashboards re-issuing the same query every refresh, the MCP/LLM loop re-running one query shape with varying values, any client caching parsed queries keyed by query text) will reasonably expect to parse once and rebind. Without reuse we pay full lex+parse on every request and capture only half the feature's value. **Why it's blocked today.** Binding writes values straight into the grammar nodes and clears the placeholder markers — `bindScalarValue`/`bindTimeValue` do `v.String = … ; v.Param = false` (lines 291, 308), `collectIntSlot` does `*value = … ; *param = false` (line 118), and `expandLists` rewrites the IN/MATCH/HAVING slices via `list.assign(rebuilt)` (line 269). Since `collect` only registers a slot while the marker is set, the parsed AST is one-shot: - **Rebind the same grammar** → 2nd call collects 0 slots → `parameter count mismatch: query contains 0 placeholder(s) but N parameter(s) provided`. - **Silent cross-request leak** → if a cached grammar is rebound with empty `params`, the count check passes (0==0), `Transform` sees no unbound placeholders, and the query runs **with the previous request's values still baked in** — no error, wrong/other-tenant results. **Ask.** This need not block the PR — the injection-safety win is real and self-contained. 1. **This PR:** make the one-shot contract safe and explicit — document the invariant loudly on `Grammar`/`ParseQuery`/`BindParams`, so nobody adds a query cache before reuse lands and trips the silent-leak path. 2. **Follow-up PR:** implement reusable-AST binding so the template is never mutated. Preferred is a positional value **overlay** the transformer reads instead of `GrammarValue` fields (immutable template, true reuse — accepting the transformer gains minimal placeholder awareness); a per-request **deep clone** is the simpler fallback but re-introduces per-request tree allocation, which partly defeats the point. Let's open a tracking issue and link it here. ########## pkg/bydbql/grammar.go: ########## @@ -319,13 +323,15 @@ type GrammarOrderByWithIdent struct { // GrammarLimitClause represents LIMIT clause. type GrammarLimitClause struct { Limit string `parser:"@'LIMIT'"` - Value int `parser:"@Int"` + Value int `parser:"( @Int"` + Param bool `parser:"| @Param )"` Review Comment: **MEDIUM — literal count positions aren't range-checked, so the new `?` path is stricter than a literal.** `BindParams` correctly bounds a bound count to `[0, MaxInt32]` (binder.go:112), but the literal path casts unchecked in the transformer: `uint32(statement.Limit.Value)` / `uint32(statement.Offset.Value)` (transformer.go:184,187) and `int32(statement.N)` / `int32(topn.N)` (transformer.go:590,1565). So `LIMIT ?`=-5 is rejected cleanly, yet `LIMIT -5` wraps to a huge `uint32` and `SHOW TOP 3000000000` wraps negative. Pre-existing, but this PR establishes the correct guard and touches these count fields — good moment to apply the same `[0, MaxInt32]` check to the literal path via a shared helper so the two paths can't diverge. ########## pkg/bydbql/transformer.go: ########## @@ -101,6 +101,11 @@ func NewTransformer(registry metadata.Repo) *Transformer { // Transform transforms a Grammar into a native query request. func (t *Transformer) Transform(ctx context.Context, grammar *Grammar) (*TransformResult, error) { + // Defense in depth: an unbound placeholder would otherwise transform into an + // empty string or a zero count silently instead of failing loudly. + if unbound := countUnboundParams(grammar); unbound > 0 { Review Comment: **MEDIUM (perf) — the whole grammar is walked twice per query.** `BindParams` already walked the tree via `collect`; `Transform` then builds a second `binder` in `countUnboundParams` and reruns the same recursion, allocating `[]func(...)`/`listContainer`s it immediately discards — paid on every query including zero-placeholder ones. Give `countUnboundParams` a closure-free counting walk (increment on `Param==true`), or have `BindParams` stamp the grammar "bound" so `Transform` can skip the re-walk. Note this interacts with the reuse redesign — an overlay/clone approach likely removes this walk entirely. ########## mcp/src/query/validation.ts: ########## @@ -119,12 +135,83 @@ export function normalizeQueryHints(args: unknown): QueryHints { return { description: typeof rawArgs.description === 'string' ? rawArgs.description.trim() : undefined, BydbQL: typeof rawArgs.BydbQL === 'string' ? rawArgs.BydbQL.trim() : undefined, + params: Array.isArray(rawArgs.params) ? (rawArgs.params as BydbQLParam[]) : undefined, resource_type: typeof rawArgs.resource_type === 'string' ? rawArgs.resource_type.trim() : undefined, resource_name: typeof rawArgs.resource_name === 'string' ? rawArgs.resource_name.trim() : undefined, group: typeof rawArgs.group === 'string' ? rawArgs.group.trim() : undefined, }; } +function validateParamString(value: unknown, position: number): string { + if (typeof value !== 'string') { + throw new Error(`params[${position}]: str value must be a string`); + } + if (value.length > maxParamValueLength) { + throw new Error(`params[${position}]: value exceeds ${maxParamValueLength} characters`); + } + return value; +} + +function validateParamInteger(value: unknown, position: number): string { + if (typeof value === 'number' && Number.isSafeInteger(value)) { + return String(value); + } + if (typeof value === 'string' && value.length <= maxIntParamLength && integerPattern.test(value)) { + return value; + } + throw new Error(`params[${position}]: int value must be an integer of at most ${maxIntParamLength} characters`); +} + +function validateParamArray(value: unknown, position: number): unknown[] { + if (!Array.isArray(value) || value.length === 0) { + throw new Error(`params[${position}]: array value must be a non-empty array`); + } + if (value.length > maxParamArrayLength) { + throw new Error(`params[${position}]: array value exceeds ${maxParamArrayLength} entries`); + } + return value; +} + +/** + * Validate BydbQL parameters and convert them to protojson TagValue form + * accepted by the BanyanDB HTTP API. + * + * The server-side `timestamp` variant is deliberately not exposed here: + * an RFC3339 `str` is equivalent for TIME positions. + */ +export function toTagValueParams(params: BydbQLParam[]): TagValueParam[] { Review Comment: **MEDIUM — new param validation ships with zero unit tests.** `git diff --stat` shows `validation.ts +90` (this function plus `validateParamString`/`Integer`/`Array` and the new limits) but `validation.test.ts` is untouched, despite the existing MCP test convention (#1036). This is the untrusted, LLM-facing entry point. Add cases for: param-count cap, each type's happy path, type mismatch, empty/oversized array, int overflow (ties to the comment above), and the `{ null: null }` shape. Contrast the Go side's 544 specs. ########## mcp/src/query/validation.ts: ########## @@ -119,12 +135,83 @@ export function normalizeQueryHints(args: unknown): QueryHints { return { description: typeof rawArgs.description === 'string' ? rawArgs.description.trim() : undefined, BydbQL: typeof rawArgs.BydbQL === 'string' ? rawArgs.BydbQL.trim() : undefined, + params: Array.isArray(rawArgs.params) ? (rawArgs.params as BydbQLParam[]) : undefined, resource_type: typeof rawArgs.resource_type === 'string' ? rawArgs.resource_type.trim() : undefined, resource_name: typeof rawArgs.resource_name === 'string' ? rawArgs.resource_name.trim() : undefined, group: typeof rawArgs.group === 'string' ? rawArgs.group.trim() : undefined, }; } +function validateParamString(value: unknown, position: number): string { + if (typeof value !== 'string') { + throw new Error(`params[${position}]: str value must be a string`); + } + if (value.length > maxParamValueLength) { + throw new Error(`params[${position}]: value exceeds ${maxParamValueLength} characters`); + } + return value; +} + +function validateParamInteger(value: unknown, position: number): string { + if (typeof value === 'number' && Number.isSafeInteger(value)) { + return String(value); + } + if (typeof value === 'string' && value.length <= maxIntParamLength && integerPattern.test(value)) { Review Comment: **MEDIUM — `maxIntParamLength = 20` admits int64-overflowing values, deferring the failure to an opaque gateway error.** A 20-char string like `"99999999999999999999"` passes `integerPattern` + the length cap and is emitted as `{ int: { value: "…" } }`, then fails late in protojson int64 decoding on the server instead of returning a clean `params[i]` message. Range-check against int64 bounds (e.g. a `BigInt` compare) here so the error is client-side and specific. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
