#576 Escape single quotes in query interpolation

[VasilevNStas]

Problem

Factbase query strings are built via string interpolation with user-supplied values (area, judge). If these contain a single quote ('), the query syntax breaks, allowing query injection.

Solution

Doubled single quotes ("''") in all user-supplied values interpolated into query strings, matching SQL-style escaping:

• regularly.rb: area and judge in queries
• repeatedly.rb: area and judge in all 4 queries

Numeric values (interval, hours) and bare identifiers (p_every_days, p_every_hours) are not affected since they don't appear inside quoted strings.

Closes #576

[VasilevNStas]

@yegor256 plz review this PR

[yegor256]

@VasilevNStas why no tests?

[VasilevNStas]

@yegor256
You are right, my apologies. Added tests for area and judge with single quotes in both files.

[VasilevNStas]

You are right, my apologies. Added tests for area and judge with single quotes in both files.

[VasilevNStas]

@yegor256
plz review

[Favixx]

The direction is right, but the escape is incomplete in ways that leave #576 open, and three call sites were missed. Requesting changes.

1. " is not escaped, and that is an actual break-out. Factbase::Syntax#to_tokens (factbase 0.19.11, syntax.rb:102-115) keeps a single boolean string flag and flips it on either ' or " — it does not remember which quote opened the literal. So area = 'x" (exists _id) (eq area "y' passes through gsub("'", ...) untouched and lands two attacker-controlled terms in the parsed AST. Notably, the repo's own escape at lib/fbe/if_absent.rb:68 and lib/fbe/just_one.rb:53 already handles both quote characters; this PR is weaker than code that exists two files away.

2. \ is not escaped, and the scheme is backslash-based. The tokenizer's unescape rule is "a quote preceded by \ is literal" with no \\ case, so a value ending in a backslash consumes the template's own closing quote. Result is String not closed -> Factbase::Syntax::Broken, or — in the multi-line recent queries — re-synchronisation on a later ', i.e. the rest of the template re-tokenized at a caller-chosen offset.

3. Missed call sites and out-of-scope claims. lib/fbe/pmp.rb:68 builds the identical (eq area '#{area}') unescaped, and lib/fbe/tombstone.rb:32/:94 build (eq where '#{where}') unescaped. Issue #576 also names p_every_days and the PMP interval; the description declares both safe "since they don't appear inside quoted strings" — wrong for interval/hours, which are inside quotes on regularly.rb:43 and repeatedly.rb:45 and are injectable, and backwards for p_every_days, where being outside quotes is exactly why escaping cannot help and validation is required.

Worth evaluating instead of escaping: Factbase::Query#each/#one accept a params hash exposed as $name (query.rb:44-49, tee.rb:33-40), and lib/fbe/iterate.rb:309,318 already uses it. That eliminates the whole class of bug. The one blocker is CachedQuery#each, which keys its cache on query text and ignores params (cached/cached_query.rb:39) while Fbe.fb wraps CachedFactbase — so it needs an upstream fix first.

Smaller but related: six inline copies of gsub("'", "\\\\'") (plus the two pre-existing) should be one helper, preferably in gsub's block form so the doubly-interpreted replacement string goes away. And the tests assert fb.size, which holds whether or not the escaped query matched anything — they detect a parse error, not a correct escape, and none of them attempts an injection. Details inline.

[Favixx]

Escaping ' alone does not close this hole, because factbase's tokenizer does not track which quote opened a literal. In factbase-0.19.11/lib/factbase/syntax.rb:102-115 the state is a single boolean, quotes = ['\'', '"'] and string = !string is flipped by either character. A " inside a single-quoted literal therefore terminates it, and everything after it is parsed as query syntax.

Concrete input: area = 'x" (exists _id) (eq area "y'. gsub("'", ...) leaves it untouched, so this line builds (and (eq what 'pmp') (eq area 'x" (exists _id) (eq area "y') (exists interval)), which tokenizes to ( and ( eq what 'pmp' ) ( eq area 'x' ) ( exists _id ) ( eq area 'y' ) ( exists interval ) ) — two attacker-supplied terms inside the AST. A shorter payload, area = 'zzz" (always) (eq area "', instead trips String literal can't be empty and surfaces as Factbase::Syntax::Broken out of the judge.

The repo already contains a stronger version of this same escape: lib/fbe/if_absent.rb:68 and lib/fbe/just_one.rb:53 do .gsub('"', '\\\\"').gsub("'", "\\\\'") — both quote characters. As written, this PR is weaker than escaping that already exists two files away. Minimum fix is to escape " as well; the better fix is one shared helper used by all of these sites.

[Favixx]

(exists #{p_every_days}) on this line is unchanged, and it is the one interpolation here that escaping can never protect: the value lands in symbol position, outside any quotes. Issue #576 names p_every_days explicitly, and the PR description dismisses it — "bare identifiers ... are not affected since they don't appear inside quoted strings". That is backwards: not being inside quotes is precisely what makes it un-escapable.

p_every_days = "interval) (always" produces (and (eq what 'pmp') (eq area 'quality') (exists interval) (always)), which parses cleanly and adds an attacker term to the conjunction. Same hole at repeatedly.rb:40 with p_every_hours.

The fix here is validation, not escaping. factbase accepts only /^([_a-z][a-zA-Z0-9_]*|\$[_a-z]+)$/ for symbols (syntax.rb:150), so a guard next to the existing nil checks — raise(Fbe::Error, "Invalid property name: #{p_every_days}") unless p_every_days.to_s.match?(/\A[_a-z][a-zA-Z0-9_]*\z/) — closes it and produces a clear error instead of a parse failure deep inside factbase.

[Favixx]

Because the chosen scheme is backslash-based, \ itself has to be escaped first, and it is not. The tokenizer's unescape rule (syntax.rb:111-113) is only "if we are inside a string and acc[-1] == '\\', drop that backslash and take the quote literally" — there is no \\ case, so a literal backslash in the value is still an active escape character when the template's own closing quote arrives.

So a value ending in a single backslash eats the closing quote. judge ending in \ contains no quote at all, so gsub is a no-op, and this line emits (eq what 'a\'); the tokenizer consumes the template's ' as an escaped quote, the literal stays open, and parsing runs off the end until raise 'String not closed' (syntax.rb:136) becomes Factbase::Syntax::Broken. In this multi-line query it is worse than a crash: the literal swallows ') (gt when (minus (to_time (env and then re-synchronises on the ' before TODAY, so the remainder of the template is re-tokenized at an offset the caller chose.

Both gaps (this one and the " on line 38) disappear with the block form of gsub, which also avoids the replacement-string grammar discussed on repeatedly.rb:51: judge.gsub(/["'\\]/) { |c| "\\#{c}" }.

[Favixx]

'#{interval} days' is a quoted literal built from an unescaped value, which directly contradicts the PR description's claim that "Numeric values (interval, hours) ... are not affected since they don't appear inside quoted strings". They do appear inside quoted strings — on this exact line, and on repeatedly.rb:45.

interval comes from pmp[p_every_days].first on line 39, straight out of the factbase with no type check, and nothing in rules/basic.fe constrains a property named interval (the rules only constrain _id, what, details, where, _time, _job). With interval holding the string 1' ) (always) (eq x '2, this line tokenizes to (and (eq what 'test') (gt when (minus (to_time (env 'TODAY' '...')) '1') (always) (eq x '2 days'))) — two injected operands on the gt term, changing whether recent matches and therefore whether the judge runs at all.

Issue #576 asked for this explicitly ("The interval value from PMP is also interpolated as-is without numeric validation"). interval = pmp.nil? ? 7 : Integer(pmp[p_every_days].first, 10) on line 39 settles it, and has the side benefit of failing loudly when a judge writes a non-numeric PMP value.

[Favixx]

Before refining the escape: the query API already supports binding, so string substitution is the weaker of two options that are both available today. Factbase::Query#each(fb, params) and #one(fb, params) take a params hash exposed inside the query as $name symbols (factbase-0.19.11 query.rb:44-49, tee.rb:33-40, and the \$[_a-z]+ branch of the symbol regex at syntax.rb:150). This repo already uses it — lib/fbe/iterate.rb:309 and :318 call .each(@fb, before:, repository:) and .one(@fb, before:, repository:).

Written that way the two queries here become (and (eq what 'pmp') (eq area $area) (exists ...)) with .each(fb, area:), and (and (eq what $judge) (gt when ...)) with .each(fb, judge:). No caller-supplied value ever reaches the parser, which removes the quote, backslash, " and # problems in one move rather than chasing metacharacters one at a time.

One genuine blocker to check before switching, so this is not a naive "just use params" suggestion: Factbase::CachedQuery#each keys its cache on the query text alone — key = "each #{@origin}" # params are ignored! (cached/cached_query.rb:39) — and Fbe.fb wraps the factbase in CachedFactbase (lib/fbe/fb.rb:52-58). Two calls with identical parameterised text and different area values would return the first one's result. So either fix that cache key upstream first, or keep escaping here and file the follow-up — but if escaping stays, it has to be complete.

[Favixx]

This assertion is satisfied whether or not the escaped query matched anything, so it cannot fail for the reason the test exists. If the escaping mangled te'st into a value matching no fact, line 38 of regularly.rb returns nil, interval falls back to the default 7, the block still runs, one fact is still inserted, and fb.size is still 2. The only failure this test can detect is the parser raising.

To assert the match, make the PMP interval observable — its only observable effect is the recent threshold. Insert a what='test' fact with when = Time.now - (5 * 24 * 60 * 60) alongside the pmp fact that has interval = 3. If the area lookup works, interval is 3, the fact is 5 days old, recent does not match, and the block runs. If the lookup silently misses, interval defaults to 7, recent matches, and the block is skipped.

Then a flag set inside the block (or assert_equal(3, fb.size)) fails exactly when the escaping is wrong, instead of only when factbase raises.

[Favixx]

Same gap as the area test, and here the fix is already written elsewhere in this file: test_simple (line 16) calls Fbe.regularly twice with a plain judge name and asserts one fact, because the second call has to find what the first one wrote.

The quoted-judge test should be that same loop with judge: "te'st". That forces (eq what 'te\'st') on line 42 of regularly.rb to match the fact inserted by the first call — which is precisely the round-trip this PR claims — and assert_equal(2, fb.size) then means something.

As written, one call and fb.size == 2: the recent query is never required to match anything, so the assertion holds even if judge.gsub produced a string that matches no fact at all. In other words, this test would still pass if line 42 escaped judge into garbage.

[Favixx]

Every test added by this PR uses te'st, which is a quoting bug, not an injection. No test supplies input that is trying to change the query, so the suite cannot distinguish "the escape works" from "the escape is missing but the parser tolerated this particular input" — which matters, because the escape is in fact incomplete.

Three cases that fail against this branch and should be in here:

• area = 'x" (exists _id) (eq area "y' — the " is not escaped, terminates the literal, and injects two terms into the AST (see regularly.rb:38).
• an area ending in a single backslash — no quote in the input, so gsub is a no-op, and the tokenizer eats the closing quote: Factbase::Syntax::Broken, "String not closed".
• p_every_days = 'interval) (always' — injects a term through the unquoted symbol position, which no amount of quote-escaping can stop.

The useful assertion is not fb.size; it is negative. Insert a fact that a widened query would match and assert it is left untouched, or assert_raises(Fbe::Error) if the decision is to reject such inputs up front.

[Favixx]

$fb, $options and $global are dead in this test. fb: is passed explicitly on line 82, so the fb: Fbe.fb default is never evaluated and none of those three globals is read. The one that does matter is $loog on line 73, because loog: is not passed and the loog: $loog default is evaluated — which is easy to miss and makes the test look like it depends on the full global setup when it depends on exactly one thing.

Pass loog: Loog::NULL on line 82 and drop lines 72, 74 and 81. That is how the two new tests in test_regularly.rb are written, and it makes the dependency visible.

The globals are not merely redundant: nothing restores them, so they leak into whichever test runs next in the same process — $fb in particular then points at a factbase containing a what='pmp', area="te'st" fact. The pre-existing tests in this file do the same, so this PR is not the origin, but it is a good moment not to add a fourth instance.

[Favixx]

Three of the four escapes this PR adds to repeatedly.rb are on judge (lines 44, 51 and 56), and none of them is exercised: this call passes judge: 'test', with no quote in it. The file that changed more has less coverage than test_regularly.rb, which does have a judge case.

The commit message "remove judge test (Rules blocks it)" suggests the judge variant was dropped because of rules/basic.fe, which enforces (matches what "^[a-z]+(-[a-z]+)*$"). That constraint does not apply here: this test builds a bare Factbase.new on line 71 and passes it as fb:, so Factbase::Rules is not in the stack and nothing prevents judge: "te'st".

The 51/56 path is the one worth covering, because it is the asymmetric one — line 54 writes the raw judge string, lines 51 and 56 read it back through the escaped query. Call Fbe.repeatedly twice with judge: "te'st" and assert that the second call finds the fact the first one wrote and that the block receives a non-nil fact. That would be the only assertion in either test file that actually fails if the escape is not an exact round-trip.