fix(query): reject reserved encoded-query characters in condition values

[michaeldcanady]

Closes #645

What

Condition values in the query builder were written into the encoded query verbatim, so a value containing ^, ,, or @ could break out of its term and append arbitrary clauses to sysparm_query — the encoded-query injection described in #645:

query.Boolean("active").Is(true).And(query.Where("name").Is("x^active=false^ORsys_id!=0")).String()
// previously => active=true^name=x^active=false^ORsys_id!=0

How

• New query.validateQueryValue renders any value via %v and rejects it if it contains a metacharacter that would be structural where that value lands: ^ (clause separator) in every value, plus , for IN / NOT IN values and @ for BETWEEN values. Those two are ordinary literals elsewhere ("Smith, John", "user@example.com"), since only the builder's own rendering makes them separators. Errors name the field, operator, value, and offending character.
• Rejection flows through the existing error channel — no API change: the value yields an error Condition whose Error() is non-nil, propagating via errors.Join through .And()/.Or() (same pattern as NumberField.Between/DateTimeField.Between).
• Trust boundary lives in query, not internal/ast: internally composed date literals (OnSpecialty, JS, NewDateTimeValue) intentionally contain @ / javascript: separators and bypass value validation. Their caller-supplied fragments are validated against ^ and @ only — commas stay legal there, so multi-argument gs.* calls like gs.dateGenerate('2024-01-01','00:00:00') keep working; an invalid fragment surfaces here as an error condition. Today(), Yesterday(), etc. are unchanged.
• BaseField.multi became a package-level generic (multi[T ast.Primitive]) since Go methods can't declare type parameters; behavior of IsOneOf/IsNotOneOf is unchanged.
• Docs: a :::warning on the query-builder page explaining the risk, which characters are rejected where, and advising an Error() check before serializing, plus an updated package comment.

Scope

Deliberately limited to the query builder. Field names (Where("x^active=false")) remain unvalidated — constructors return field structs with no error channel, so closing that needs a small refactor — and raw sysparm_query strings passed directly to request builders behave exactly as before.

Edge-case behavior changes

Degenerate combinator inputs now fail closed instead of returning nil or panicking. Callers checking == nil on combinator results will see different (safer) behavior; callers using Error()/String() are unaffected:

• And() / Or() with no arguments previously returned a nil Condition; they now return an error Condition whose Error() matches the new query.ErrNoConditions.
• A nil argument (including typed-nil) passed to .And()/.Or() or the variadic combinators previously panicked when combined; it now returns an error Condition matching the new query.ErrNilCondition.
• NewErrorCondition carries a placeholder node, so String() on a rejected or malformed condition renders <invalid query> instead of panicking.

Testing

• No existing tests modified; all pre-existing values are free of reserved characters.
• New query/validation_test.go (~60 cases): the issue's exact repro + guard-clause composition, every value-taking method across all four field types, each reserved character in each operator context (rejected exactly where structural), false-positive resistance (=, !, %, unicode, exponent floats render unchanged; builder-emitted ,/@ still work), trusted-composite carve-outs, and curveballs (a fmt.Stringer whose String() smuggles a ^).
• Full unit suite green (30 packages), integration suite green (go test -tags integration ./tests/integration/v2/), golangci-lint run ./... clean, gofmt/vet clean.

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://michaeldcanady.github.io/servicenow-sdk-go/pr-preview/pr-653/
Built to branch gh-pages at 2026-08-24 22:45 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[codecov]

Codecov Report

❌ Patch coverage is 96.82540% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.70%. Comparing base (c9f9074) to head (a410927).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/ast/stringer.go	86.20%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   86.57%   86.70%   +0.13%     
==========================================
  Files         337      338       +1     
  Lines        8439     8538      +99     
==========================================
+ Hits         7306     7403      +97     
- Misses       1132     1134       +2     
  Partials        1        1              

Files with missing lines	Coverage Δ
internal/ast/array_node.go	100.00% <100.00%> (ø)
internal/ast/binary_node.go	100.00% <100.00%> (ø)
internal/ast/literal_node.go	100.00% <100.00%> (ø)
internal/ast/operator.go	100.00% <100.00%> (ø)
internal/ast/pair_node.go	100.00% <100.00%> (ø)
internal/ast/unary_node.go	100.00% <100.00%> (ø)
query/base_field.go	100.00% <100.00%> (ø)
query/condition.go	100.00% <100.00%> (ø)
query/date_time_field.go	100.00% <100.00%> (+5.40%)	⬆️
query/datetime_value.go	100.00% <100.00%> (ø)
... and 5 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[michaeldcanady]

/oc review

[github-actions]

Overall (Request changes)

The core approach is sound: validating the %v-rendered form (which exactly matches what ast.NewLiteralNode will serialize) and failing closed via the existing error-Condition channel is the right design given encoded queries have no escapes, and the test suite is unusually thorough (injection repro, per-operator, per-character, false-positive resistance, trusted-composite carve-outs). The multi generic refactor is behavior-preserving and clean, and the docs warning is a good addition.

However, the trust-boundary carve-out has a hole that matters for a priority: high injection fix: JS()/Javascript() and OnSpecialty() are public methods taking caller-supplied strings, yet their output is wrapped in DateTimeValue, which unconditionally bypasses validation. They provide the exact clause-injection the PR claims to close, with Error() == nil. Secondarily, this PR greatly multiplies the paths that produce error conditions, and calling String() on one panics (nil AST node) — a harsh failure mode for the exact users who skip the Error() check the new docs tell them to perform. Both should be addressed before merge; neither requires an API change.

1. Javascript()/OnSpecialty() remain full encoded-query injection vectors

query/date_time_field.go:17-21

The case DateTimeValue branch bypasses validation on the theory that these are "trusted composites built by this package." That's true only of NewDateTimeValue/Time (which format a time.Time). It is not true of:

• JS(expr) / Javascript(expr) (query/datetime_value.go:30-32, query/date_time_field.go:77-79) — expr is arbitrary caller text embedded verbatim.
• OnSpecialty(label, startExpr, endExpr) (query/date_time_field.go:127-130) — all three arguments are arbitrary caller text.

Concretely, today's HEAD behavior after this PR:

q := query.DateTime("f").Javascript("gs.daysAgoStart(0)^ORactive=false")
// q.Error() == nil
// q.String() == "fONjavascript:gs.daysAgoStart(0)^ORactive=false"

That is precisely the #645 injection class (a structural ^ indistinguishable from the builder's own), reachable through a first-class public method and invisible to the recommended Error() check. The intentional part of these literals is the @/javascript: separator structure — not the ^. Suggested fix: keep the DateTimeValue fast path, but validate the inputs at the public boundary — reject ^ (and arguably ,) in expr inside JS(), and in each of label/startExpr/endExpr inside OnSpecialty(). The built-in helpers (Today(), Yesterday(), …) contain none of those characters, so they stay byte-for-byte unchanged, and the existing TestTrustedDateCompositesUnaffected cases still pass. If you'd rather defer this, the PR description and the docs warning should stop describing JS/OnSpecialty as internally composed/trusted, since that materially overstates the guarantee.

2. String() on a rejected condition panics

query/condition.go:45-49

NewErrorCondition(err) leaves node nil (query/condition.go:57-59), and baseCondition.String() calls c.node.Accept(visitor) on that nil interface — a guaranteed nil-pointer panic. The same happens for any combined condition containing a rejected side, because .And()/.Or() embed the nil node via ToNode() and VisitBinary/VisitUnary then dereference it. This mechanism predates the PR (NumberField.Between already produced error conditions), but this change turns every value-taking method into a producer of such conditions, and the failure mode for a caller who follows the old idiom (q := ...; client...Query: &q.String()) without the new Error() check changes from "bad query sent" to "panic in production." Since the fix's whole premise is fail-closed ergonomics, please make String() nil-tolerant — return "" (or a placeholder like <invalid query>) when node == nil — or give NewErrorCondition a sentinel node. A table-driven test asserting String() doesn't panic on error conditions would lock it in.

3. Field-name injection remains open (acknowledged)

A general observation, not a blocker: Where("x^active=false").Is("y") still injects via the field-name position, and raw sysparm_query strings are untouched. The PR description scopes this out honestly (constructors have no error channel), and the docs page covers value-side handling — just noting it so the follow-up refactor (threading an error channel through field constructors, or validating names in Where/String/Number/Boolean/DateTime by returning an error condition on first use) stays tracked against #645.

4. Cover dateTimeBinary's default: branch

A general note tied to Codecov's report of 4 uncovered lines in query/date_time_field.go: the new default: arm (render-then-validate for non-string, non-time.Time, non-DateTimeValue values) is only exercised indirectly via validateQueryValue's own tests (evilStringer never flows through dateTimeBinary). One table row driving e.g. DateTime("f").dateTimeBinary(ast.OperatorOn, evilStringer{}) through the exported surface would close the patch-coverage gap and prove the render-then-validate ordering end to end.

[michaeldcanady]

/oc review

[github-actions]

Overall (Approve)

Both blockers from the previous review are properly resolved in 1e592a5a. The composite-fragment hole is closed at the right boundary: JS() validates its expression at construction (query/datetime_value.go:38-42), OnSpecialty() validates label/start/end fragments (query/date_time_field.go:137-149), and dateTimeBinary surfaces a stored DateTimeValue.err as an error Condition (query/date_time_field.go:24-27) — so Javascript()/JS()/OnSpecialty() no longer smuggle ^ into sysparm_query past an Error() == nil check. The panic-on-render problem is fixed the better way: NewErrorCondition carries a <invalid query> sentinel node (query/condition.go:72-77) so combined trees stay traversable, String() nil-guards (query/condition.go:54-57), and every AST Accept is nil-receiver safe. Choosing a deliberately unusable placeholder over an empty render (an empty sysparm_query returns everything) shows good fail-closed instincts, and the reasoning is documented at the const. The previously-missing dateTimeBinary default-branch coverage is in place, and the new tests cover the injection repro end-to-end through every public method. Remaining items below are non-blocking refinements.

1. Fragment validation rejects commas inside legitimate JavaScript expressions

query/validation.go:44-56

validateQueryFragment rejects , along with ^ and @. The @/^ rejections are clearly correct — @javascript: structures the range pair and ^ terminates the clause. A comma, though, is only structural as the list separator for IN/NOT IN; inside an ON javascript:<expr> value the expression is opaque to the query parser, so real-world expressions like var d=new Date();d.setHours(0,0,0,0);d or gs.dateGenerate('2024-01-01','12:00:00') are plausibly valid server-side yet now fail closed with an error. This is a deliberate tradeoff under the PR's posture and errs safe, so it doesn't block — but it's worth verifying against a live instance whether , survives in a javascript: fragment; if it does, dropping , from the fragment set (keeping it for plain values) would avoid breaking legitimate custom-date filters while keeping the injection closed.

2. Consider per-operator reserved-character sets for plain values

query/validation.go:17-33

Related and longer-term: validateQueryValue rejects , uniformly across all operators, including =, where a comma has no structural meaning — so ordinary data like Where("name").Is("Smith, John") or "1,000.00" now produces an error condition. Fail-closed is the right default for this fix, but scoping , to IN-family operators and @ to pair positions (BETWEEN) would shrink the false-positive surface considerably without weakening the defense. Fine to defer to a follow-up; just noting the compat cost is broader than the actual attack surface.

3. Unused LiteralVisitor/UnaryVisitor interfaces

internal/ast/visitor.go:3-10

The new split of Visitor into embeddable LiteralVisitor/UnaryVisitor sub-interfaces has no consumers — nothing in the repo accepts or implements them separately, and the Accept methods still take the full Visitor. As written it's dead abstraction; either give it a purpose (e.g., have node Accept methods accept the narrowest sufficient interface) or revert to the single flat interface to avoid implying a contract nobody relies on.

4. Residual nil-node panic when combining NewCondition(nil)

query/condition.go:31-43

The nil-safety work covers typed-nil receivers and the top-level nil check, but combining a condition built via the exported NewCondition(nil) embeds a nil interface as a binary child, and VisitBinary's n.Left.Accept(v) on a nil interface still panics (internal/ast/stringer.go:24-28). This predates the PR and is a degenerate input, so not blocking — but if the goal is "malformed trees traverse safely," guarding the visitor against nil child interfaces (or documenting NewCondition(nil) as invalid input) would close the last gap.

5. Field-name injection stays open (tracking)

General observation carried over from the previous review, unchanged and honestly scoped: Where("x^active=false").IsAnything() and friends still inject via the field-name position, since field constructors have no error channel. The docs warning covers value-side handling only. Keeping this visible against #645 so it isn't lost when the value-side fix ships — the natural follow-up is validating the name lazily on first use and returning an error condition, mirroring this PR's mechanism.

[michaeldcanady]

/oc review

[github-actions]

Overall (Approve)

Both blockers from the first review remain fixed, and the two commits added since my last review (7173d37f, 8e393f51) hold up under scrutiny. The error-returning traversal is implemented soundly: every declared operator constant has an entry in operatorStrings, so Operator.known() cannot false-positive on a legitimate operator while correctly excluding the OperatorUnknown sentinel despite its map entry; conversion.IsNil covers both nil interfaces and typed-nil child pointers; and NewLiteralNode's %v rendering matches exactly what validateQueryValue inspects, so there is no render/validation mismatch to smuggle characters through. I re-traced every value-taking method across all four field types — each routes through the validating binary/pair/multi/dateTimeBinary paths, and DateTimeField.Between's time.Time signature keeps err-carrying DateTimeValues away from the one unguarded composite path. StringerVisitor/Accept consumers are confined to baseCondition.render() plus tests, so the visitor signature change is fully contained, and the structural-fault degradation to <invalid query> (with Query() exposing the cause via errors.Is-matchable sentinels) is well tested including hand-built malformed trees. Prior non-blocking items are also addressed: the unused LiteralVisitor/UnaryVisitor split is gone, and the NewCondition(nil) combination panic is closed with tests.

Remaining findings below are non-blocking refinements. One process note: 8e393f51 bundles unrelated Vale-lint docs changes (conventions.md wording, batch.mdx, vocabulary regex) into this security-fix PR — harmless, but ideally those ride in their own PR.

1. AST sentinel docs promise context-wrapping that isn't implemented

internal/ast/errors.go:5-8

The comment says visitors "wrap these with node context (position, element index) so the resulting message identifies where traversal failed," but StringerVisitor returns the bare sentinels unwrapped — callers matching with errors.Is work fine, yet the messages are generic ("child cannot be nil") with no position information. Either wrap at each fault site (fmt.Errorf("unary left: %w", ErrNilChild)) or fix the comment to match. Relatedly, internal/ast/stringer.go:84 StringerVisitor.String() returns whatever partial output was written before a mid-tree short-circuit; safe today because render() discards on error and no other production consumer exists, but a failed flag that makes String() return "" after an error would make the visitor misuse-proof as future consumers appear.

2. Query() widens the exported Condition interface

query/condition.go:10-17

Adding a method to an exported interface breaks external implementers on upgrade within v2 semver. Since baseCondition is the only implementation and Condition is unambiguously meant to be produced by this package rather than implemented by consumers, practical risk is negligible — just confirming this widening is deliberate rather than accidental, and worth a line in release notes since it's technically breaking for anyone who rolled their own Condition.

3. Combining against a nil condition argument still panics

query/condition.go:32-44

The nil-safety work fixed the malformed-tree case (NewCondition(nil) renders safely), but the nil-argument case remains: String("f").Is("v").And(nil) panics at other.ToNode() on the nil interface, and the package-level combinators return a nil Condition for empty input (query/query.go:42-44), whose String()/Error() then panic in turn. Both predate this PR and are degenerate inputs, so not blocking — noting them so the fail-closed sweep eventually covers them (guard And/Or with conversion.IsNil(other) degrading to self or an error condition, and have empty And()/Or() return a valid empty-ish condition).

4. Standing items carried forward

No re-review needed, status only: fragment validation still rejects , inside javascript:/OnSpecialty expressions (deliberate fail-closed tradeoff — still recommend verifying , against a live instance and scoping reserved characters per-operator — , to IN-family, @ to BETWEEN — in a follow-up); field-name injection (Where("x^active=false")) remains open and honestly scoped out, tracked against #645.

[michaeldcanady]

@Atishyy27 would you mind taking a look and see if this addresses your concerns?