Skip to content

#576 Escape single quotes in query interpolation #615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VasilevNStas wants to merge 5 commits into
base: master
Choose a base branch
from
Open

#576 Escape single quotes in query interpolation #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50f1ec0
#576: Escape single quotes in query interpolation
VasilevNStas Jul 11, 2026
7ec907d
#576: Add tests for query injection fix
VasilevNStas Jul 17, 2026
9cec45b
#576: Fix repeated test to bypass Rules wrapper
VasilevNStas Jul 17, 2026
6ca35f8
#576: Fix quote escaping with backslash, fix test assertions
VasilevNStas Jul 17, 2026
2c2529e
#576: Fix test env (), remove judge test (Rules blocks it)
VasilevNStas Jul 17, 2026
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
4 changes: 2 additions & 2 deletions lib/fbe/regularly.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ def Fbe.regularly(area, p_every_days, p_since_days = nil, fb: Fbe.fb, judge: $ju
raise(Fbe::Error, 'The fb is nil') if fb.nil?
raise(Fbe::Error, 'The $judge is not set') if judge.nil?
raise(Fbe::Error, 'The $loog is not set') if loog.nil?
pmp = fb.query("(and (eq what 'pmp') (eq area '#{area}') (exists #{p_every_days}))").each.first
pmp = fb.query("(and (eq what 'pmp') (eq area '#{area.gsub("'", "\\\\'")}') (exists #{p_every_days}))").each.first

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

interval = pmp.nil? ? 7 : pmp[p_every_days].first
recent = fb.query(
"(and
(eq what '#{judge}')
(eq what '#{judge.gsub("'", "\\\\'")}')

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}" }.

(gt when (minus (to_time (env 'TODAY' '#{Time.now.utc.iso8601}')) '#{interval} days')))"

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'#{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.

).each.first

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if recent
Expand Down
8 changes: 4 additions & 4 deletions lib/fbe/repeatedly.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,23 @@ def Fbe.repeatedly(area, p_every_hours, fb: Fbe.fb, judge: $judge, loog: $loog,
raise(Fbe::Error, 'The fb is nil') if fb.nil?
raise(Fbe::Error, 'The $judge is not set') if judge.nil?
raise(Fbe::Error, 'The $loog is not set') if loog.nil?
pmp = fb.query("(and (eq what 'pmp') (eq area '#{area}') (exists #{p_every_hours}))").each.first
pmp = fb.query("(and (eq what 'pmp') (eq area '#{area.gsub("'", "\\\\'")}') (exists #{p_every_hours}))").each.first

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is fixed but its twin is not. lib/fbe/pmp.rb:68 builds literally the same query with the same value, unescaped:

query = ->(area) { Fbe.fb(global:, fb:, options:, loog:).query("(and (eq what 'pmp') (eq area '#{area}'))") }

and there area is args1.first.to_s from the others/method_missing dispatcher (pmp.rb:74), so it is even less controlled than the argument here. lib/fbe/tombstone.rb:32 and :94 have the same shape with (eq where '#{where}'), where where is only checked with is_a?(String) and never escaped. After this PR merges, #576's "unsanitized string interpolation" is still true of three call sites in lib/.

This is the argument for extracting one helper rather than inlining gsub per site: a Fbe.quoted(value) in lib/fbe/ gets applied at the four sites you already know about plus these three, and the next site becomes a one-liner instead of someone re-deriving the escaping rules from the tokenizer.

hours = pmp.nil? ? 24 : pmp[p_every_hours].first
recent = fb.query(
"(and
(eq what '#{judge}')
(eq what '#{judge.gsub("'", "\\\\'")}')

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#{judge} used to go through to_s, so any object worked; judge.gsub requires a String and now raises NoMethodError: undefined method 'gsub' for an instance of Symbol for a caller passing judge: :test (or area: :quality on line 40). The nil guard on line 38 is the only type check in the method and it does not catch this. judge.to_s.gsub(...) restores the previous contract for free; an explicit raise(Fbe::Error, ...) unless judge.is_a?(String) is also fine if the stricter contract is intended — silently converting a working call into a NoMethodError raised from inside a string interpolation is the one option worth avoiding.

For prioritisation it is also worth recording where the real exposure is. judge is written to what on line 54, and rules/basic.fe enforces (matches what "^[a-z]+(-[a-z]+)*$"), so under the Factbase::Rules wrapper that Fbe.fb installs, a judge name containing a quote could never be stored in the first place. The value that is genuinely unconstrained is area — which is exactly the one still interpolated raw at lib/fbe/pmp.rb:68.

(gt when (minus (to_time (env 'TODAY' '#{Time.now.utc.iso8601}')) '#{hours} hours')))"

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same defect as regularly.rb:43: '#{hours} hours' is a quoted literal, and hours is unescaped and untyped. It is read on line 41 from pmp[p_every_hours].first, i.e. an arbitrary value out of the factbase, and rules/basic.fe places no constraint on a property with that name.

Verified against the 0.19.11 tokenizer: with hours holding 1' ) (always) (eq x '2, this query parses without error into (and (eq what 'test') (gt when (minus (to_time (env 'TODAY' '...')) '1') (always) (eq x '2 hours'))) — the gt term gains two operands it was never meant to have, which flips whether recent is non-nil and therefore whether the judge is skipped. A poisoned PMP fact can silently disable a judge forever or defeat its rate limit.

hours = pmp.nil? ? 24 : Integer(pmp[p_every_hours].first, 10) on line 41 closes it, and is what #576 asked for.

).each.first
if recent
loog.info("#{judge} was executed #{recent.when.ago} ago, skipping now (we run it every #{hours} hours)")
return
end
f = fb.query("(and (eq what '#{judge}'))").each.first
f = fb.query("(and (eq what '#{judge.gsub("'", "\\\\'")}'))").each.first

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"\\\\'" passes through two independent escaping layers, which is worth calling out because it is very easy to get wrong: the Ruby double-quoted literal collapses it to the three characters \ \ ', and then String#gsub's replacement grammar collapses \\ to one \ and leaves ', giving \'. The result happens to be correct. The hazard is that \' in a gsub replacement is also the backreference for $' (post-match), so dropping one backslash here does not raise — it silently replaces every quote with the remainder of the string.

The block form has no replacement grammar at all, is readable, and takes care of the unescaped-backslash gap flagged on regularly.rb:42 at the same time: judge.gsub(/["'\\]/) { |c| "\\#{c}" }.

This is also the sixth copy of the same expression added by this PR, on top of the two that already exist at lib/fbe/if_absent.rb:68 and lib/fbe/just_one.rb:53. A single Fbe.quoted(value) in lib/fbe/ — returning the value already wrapped in quotes — would replace all eight, keep the escaping rules in one testable place, and make the missed sites in pmp.rb and tombstone.rb a one-line change each.

if f.nil?
f = fb.insert
f.what = judge
end
yield(fb.query("(and (eq what '#{judge}'))").each.first)
yield(fb.query("(and (eq what '#{judge.gsub("'", "\\\\'")}'))").each.first)

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rebuilds the exact string already built on line 51, so the escape now runs twice and any future change to it has to be made in two places. Hoist it: q = "(and (eq what '#{...}'))" once, then use q on both lines.

The more substantive point is the asymmetry this PR introduces on the write/read pair. Line 54 stores the raw judge into what, while lines 51 and 56 read it back through the escaped query, so correctness now depends on the escape being an exact round-trip through factbase's tokenizer. It is for a plain quote (te'st -> 'te\'st' -> te'st), but not for a value ending in a backslash (see regularly.rb:42).

When the round-trip fails, .each.first returns nil and this line yields nil into the judge's block, which then dies with a NoMethodError on the block's first assignment — far from the cause and hard to trace back to quoting. A raise(Fbe::Error, "Fact for #{judge} not found") guard between lines 55 and 56 would turn a mystery into a diagnosis.

Fbe.overwrite(f, 'when', Time.now)
nil
end
30 changes: 30 additions & 0 deletions test/fbe/test_regularly.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,34 @@ def test_uses_default_since_days_when_pmp_lacks_property
refute_nil(fact)
refute_nil(fact.since)
end

def test_area_with_single_quote
fb = Factbase.new
fb.txn do |fbt|
f = fbt.insert
f.what = 'pmp'
f.area = "te'st"
f.interval = 3
end
loog = Loog::NULL
Fbe.regularly("te'st", 'interval', 'days', fb:, loog:, judge: 'test') do |f|

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

f.foo = 42
end
assert_equal(2, fb.size)

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def test_judge_with_single_quote
fb = Factbase.new
fb.txn do |fbt|
f = fbt.insert
f.what = 'pmp'
f.area = 'quality'
f.interval = 3
end
loog = Loog::NULL
Fbe.regularly('quality', 'interval', 'days', fb:, loog:, judge: "te'st") do |f|

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

f.foo = 42
end
assert_equal(2, fb.size)
end
end
18 changes: 18 additions & 0 deletions test/fbe/test_repeatedly.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,22 @@ def test_failed_block_does_not_lock_out_next_run
end
assert(ran)
end

def test_area_with_single_quote
fb = Factbase.new
$fb = fb
$loog = Loog::NULL
$options = Judges::Options.new

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

fb.txn do |fbt|
f = fbt.insert
f.what = 'pmp'
f.area = "te'st"
f.every_x_hours = 24
end
$global = {}
Fbe.repeatedly("te'st", 'every_x_hours', fb:, judge: 'test') do |f|

@Favixx Favixx Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

f.foo = 42
end
assert_equal(2, fb.size)
end
end
Loading