From 1cab5cd5a76fb5cbd72a3ffffa984a5b1c6732fa Mon Sep 17 00:00:00 2001 From: Joachim Rosskopf Date: Sun, 4 Oct 2026 11:34:10 +0200 Subject: [PATCH] fix: harden the request validator - whitespace-proof quote screen, number/boolean validation, server-owned cache params (#192) - Collapse whitespace and inspect the whole value when it contains a quote - Validate number/float/double/boolean/integer/bool strictly - cacheTable/cacheSchema/cacheCatalog are assigned, not emplaced - Docs: bound {{ }} is the defence; the screen is a safety net - Real-binary tests: 85 cases - Refs #192 (structural binding of triple-brace values deliberately not done) --- CHANGELOG.md | 18 +++ docs/CONFIG_REFERENCE.md | 25 ++- src/cache_manager.cpp | 9 +- src/config_manager.cpp | 6 +- src/include/request_validator.hpp | 2 + src/request_validator.cpp | 88 ++++++---- test/integration/test_validator_hardening.py | 161 +++++++++++++++++++ 7 files changed, 268 insertions(+), 41 deletions(-) create mode 100644 test/integration/test_validator_hardening.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e990ecb8..0995facc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ All notable changes to flAPI are documented here. Versions follow `vYY.MM.DD` (t ## Unreleased +### Security: request-validator hardening + +Found by a review of the server and reproduced against a real binary: + +- The SQL-injection screen was bypassed with whitespace: `x'OR 2>1OR n = 'x` passed it and, + spliced into `WHERE n = '{{{ params.name }}}'`, returned every row. The screen now collapses whitespace and + inspects the whole value whenever it contains a single quote. It is still a heuristic: **use `{{ params.x }}` + (bound) for request values**; the docs now say so (CONFIG_REFERENCE 5.9). +- `number`, `float`, `double`, `boolean` (and the aliases `integer`, `bool`) were treated as safe-to-skip-the-screen + but were not validated at all, so `0 OR true` reached a `{{{ params.amount }}}` site. They are validated now. +- A request could override a cached endpoint's server-owned `cacheTable` / `cacheSchema` / `cacheCatalog` with + `?cacheTable=...` and make the template read another table. Those values are now always the configured ones. + +**Behaviour change:** a value containing a single quote **and** an SQL-looking character or word +(`( ) = < > + * / % & | ;`, `or`, `and`, `like`, ...) is rejected (HTTP 400), even for a bound `{{ }}` site - +e.g. `O'Brien (retired)`. A lone apostrophe (`O'Brien`) is fine. Set `preventSqlInjection: false` on a field you +use only through `{{ }}` if you need such values. + ### Fixed: an invalid `cache.schedule` crashed the server; a failed refresh took a working cache offline Found by a review of the server and reproduced against a real binary: diff --git a/docs/CONFIG_REFERENCE.md b/docs/CONFIG_REFERENCE.md index 46786515..9cc4c42c 100644 --- a/docs/CONFIG_REFERENCE.md +++ b/docs/CONFIG_REFERENCE.md @@ -1268,7 +1268,30 @@ validators: max: "18:00:00" ``` -> **Implementation:** `src/request_validator.cpp` | **Tests:** `test/cpp/request_validator_test.cpp`, `test/integration/test_customers.tavern.yaml` +### 5.8 Number and Boolean Validators + +`type: number` (aliases `float`, `double`) requires the **whole** value to be one plain decimal number +(`-3`, `1.5`, `.5`, `1e3`; no spaces, hex or `NaN`). `type: boolean` (alias `bool`) accepts `true`, `false`, +`1`, `0` (any case). `type: integer` is an alias of `int`. A value that does not match is rejected with +HTTP 400 before any SQL is rendered. + +```yaml +validators: + - type: number +``` + +### 5.9 The SQL-injection screen is a safety net, not the defence + +Every string-like field (and any field without a bound type) is screened for SQL keywords, comment markers +and, when the value contains a single quote, SQL-shaped tokens next to it (operators, `or`/`and`/`like`, a +parenthesis, ...) after collapsing all whitespace. That is a heuristic: it can be evaded by a payload it +does not know. **Use `{{ params.x }}` (double braces): flAPI binds it as a prepared-statement parameter, so +the value can never change the SQL.** `{{{ params.x }}}` interpolates the value as text; reserve it for +identifiers and fragments you validate with an `enum` or a strict `regex`. Set `preventSqlInjection: false` +on a field only when it is used exclusively through `{{ }}` and legitimately contains such characters +(e.g. a free-text name with an apostrophe and parentheses). + +> **Implementation:** `src/request_validator.cpp` | **Tests:** `test/cpp/request_validator_test.cpp`, `test/integration/test_customers.tavern.yaml`, `test/integration/test_validator_hardening.py` --- diff --git a/src/cache_manager.cpp b/src/cache_manager.cpp index c2909100..714255e9 100644 --- a/src/cache_manager.cpp +++ b/src/cache_manager.cpp @@ -980,9 +980,12 @@ void CacheManager::addQueryCacheParamsIfNecessary(std::shared_ptr } std::string schema = cacheConfig.schema.empty() ? config_manager->getCacheSchema() : cacheConfig.schema; - params.emplace("cacheCatalog", config_manager->getDuckLakeConfig().alias); - params.emplace("cacheSchema", schema); - params.emplace("cacheTable", cacheConfig.table); + // ASSIGN, never emplace: emplace left a request-supplied `?cacheTable=...` in place, so a + // caller could point a cached endpoint's template at any table (#192). These are + // server-owned. + params["cacheCatalog"] = config_manager->getDuckLakeConfig().alias; + params["cacheSchema"] = schema; + params["cacheTable"] = cacheConfig.table; } void CacheManager::performGarbageCollection(std::shared_ptr config_manager, const EndpointConfig& endpoint, const std::vector previousTableNames) { diff --git a/src/config_manager.cpp b/src/config_manager.cpp index a8c73a85..899a948c 100644 --- a/src/config_manager.cpp +++ b/src/config_manager.cpp @@ -962,7 +962,7 @@ void ConfigManager::parseEndpointValidators(const YAML::Node& req, RequestFieldC ValidatorConfig validatorConfig; validatorConfig.type = safeGet(validator, "type", "validators.type"); - if (validatorConfig.type == "int") { + if (validatorConfig.type == "int" || validatorConfig.type == "integer") { validatorConfig.min = safeGet(validator, "min", "validators.min", std::numeric_limits::min()); validatorConfig.max = safeGet(validator, "max", "validators.max", std::numeric_limits::max()); } else if (validatorConfig.type == "string") { @@ -1980,7 +1980,7 @@ crow::json::wvalue ConfigManager::serializeEndpointConfig(const EndpointConfig& if (validator.max > 0) { validatorJson["max-length"] = validator.max; } - } else if (validator.type == "int") { + } else if (validator.type == "int" || validator.type == "integer") { validatorJson["min"] = validator.min; validatorJson["max"] = validator.max; } else if (validator.type == "enum") { @@ -2126,7 +2126,7 @@ ValidatorConfig validatorFromJson(const crow::json::rvalue& v) { ValidatorConfig out; out.type = v.has("type") ? std::string(v["type"].s()) : std::string(); auto str = [&](const char* key) { return v.has(key) ? std::string(v[key].s()) : std::string(); }; - if (out.type == "int") { + if (out.type == "int" || out.type == "integer") { out.min = v.has("min") ? static_cast(v["min"].i()) : std::numeric_limits::min(); out.max = v.has("max") ? static_cast(v["max"].i()) : std::numeric_limits::max(); } else if (out.type == "string") { diff --git a/src/include/request_validator.hpp b/src/include/request_validator.hpp index df7e2e08..52171883 100644 --- a/src/include/request_validator.hpp +++ b/src/include/request_validator.hpp @@ -24,6 +24,8 @@ class RequestValidator { private: std::vector validateField(const RequestFieldConfig& field, const std::map& params); + std::vector validateNumber(const std::string& fieldName, const std::string& value); + std::vector validateBoolean(const std::string& fieldName, const std::string& value); std::vector validateString(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator); std::vector validateInt(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator); std::vector validateEmail(const std::string& fieldName, const std::string& value); diff --git a/src/request_validator.cpp b/src/request_validator.cpp index a620a980..fb6d6d2d 100644 --- a/src/request_validator.cpp +++ b/src/request_validator.cpp @@ -54,9 +54,17 @@ std::vector RequestValidator::validateField(const RequestFieldC if (validator.type == "string") { auto stringErrors = validateString(field.fieldName, value, validator); errors.insert(errors.end(), stringErrors.begin(), stringErrors.end()); - } else if (validator.type == "int") { + } else if (validator.type == "int" || validator.type == "integer") { auto intErrors = validateInt(field.fieldName, value, validator); errors.insert(errors.end(), intErrors.begin(), intErrors.end()); + } else if (validator.type == "number" || validator.type == "float" || validator.type == "double") { + // Classified as bindable (which skips the SQL screen below), so it must actually + // be a number: `0 OR true` passed for a `{{{ params.amount }}}` site (#192). + auto numberErrors = validateNumber(field.fieldName, value); + errors.insert(errors.end(), numberErrors.begin(), numberErrors.end()); + } else if (validator.type == "boolean" || validator.type == "bool") { + auto boolErrors = validateBoolean(field.fieldName, value); + errors.insert(errors.end(), boolErrors.begin(), boolErrors.end()); } else if (validator.type == "email") { auto emailErrors = validateEmail(field.fieldName, value); errors.insert(errors.end(), emailErrors.begin(), emailErrors.end()); @@ -111,6 +119,25 @@ std::vector RequestValidator::validateField(const RequestFieldC return errors; } +std::vector RequestValidator::validateNumber(const std::string& fieldName, const std::string& value) { + // The whole string must be one plain decimal number: optional sign, digits with an + // optional fraction (or a leading '.'), optional exponent. No spaces, no hex, no NaN. + static const std::regex number(R"(^[+-]?([0-9]+(\.[0-9]*)?|\.[0-9]+)([eE][+-]?[0-9]+)?$)"); + if (!std::regex_match(value, number)) { + return {{fieldName, "Value is not a valid number"}}; + } + return {}; +} + +std::vector RequestValidator::validateBoolean(const std::string& fieldName, const std::string& value) { + std::string lower = value; + std::transform(lower.begin(), lower.end(), lower.begin(), [](unsigned char c) { return std::tolower(c); }); + if (lower == "true" || lower == "false" || lower == "1" || lower == "0") { + return {}; + } + return {{fieldName, "Value is not a valid boolean (true, false, 1 or 0)"}}; +} + std::vector RequestValidator::validateString(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator) { std::vector errors; if (validator.min > 0 && static_cast(value.size()) < validator.min) { @@ -312,42 +339,35 @@ std::vector RequestValidator::validateSqlInjection(const std::s } } - // Check for suspicious characters that are commonly used in SQL injection - // Single quotes are a key indicator of SQL injection attempts - const std::string suspiciousChars = "\'"; - for (char c : suspiciousChars) { - if (value.find(c) != std::string::npos) { - // Check if it's part of a dangerous pattern (already checked above) - // Only flag if it appears in a context that suggests injection - // Look for patterns like ' OR ' or '; or similar - bool isPartOfPattern = false; - size_t pos = value.find(c); - while (pos != std::string::npos) { - // Check context around the quote - std::string context = ""; - if (pos > 0 && pos < value.length() - 1) { - context = value.substr(std::max(0, (int)pos - 2), std::min(5, (int)(value.length() - pos + 2))); - } else if (pos == 0 && value.length() > 1) { - context = value.substr(0, 3); - } else if (pos == value.length() - 1 && value.length() > 1) { - context = value.substr(std::max(0, (int)pos - 2), 3); + // A single quote is where a value leaves its string literal. It is only suspicious + // when something SQL-shaped sits next to it - an operator, a boolean/logic word, a + // parenthesis. The old check looked at a 5-character window around the quote for + // "OR"/"AND"/";"/"=", so whitespace pushed the tell-tale out of the window + // (`x'OR 2>1 ...`). Look at the WHOLE value instead, with every + // whitespace/control character collapsed to one space first. A lone apostrophe + // (O'Brien, it's) is left alone. + if (value.find('\'') != std::string::npos) { + std::string normalised; + normalised.reserve(value.size()); + bool in_space = false; + for (const unsigned char c : value) { + const bool space = (c <= 0x20) || c == 0x7f || c == 0xa0; + if (space) { + if (!in_space) { + normalised.push_back(' '); } - std::transform(context.begin(), context.end(), context.begin(), ::toupper); - // If quote appears with OR, AND, or ; nearby, it's likely injection - if (context.find("OR") != std::string::npos || - context.find("AND") != std::string::npos || - context.find(";") != std::string::npos || - context.find("=") != std::string::npos) { - isPartOfPattern = true; - break; - } - pos = value.find(c, pos + 1); - } - if (isPartOfPattern) { - errors.push_back({fieldName, "Potential SQL injection detected"}); - return errors; + in_space = true; + } else { + normalised.push_back(static_cast(std::tolower(c))); + in_space = false; } } + static const std::regex suspicious( + R"([;=<>!|&()+*/%\\]|--|\b(or|and|not|xor|like|ilike|similar|in|between|is|null|true|false|union|select|from|where|having|limit|offset|order|group|join|case|when|then|else|end|exists|any|all|cast|convert)\b)"); + if (std::regex_search(normalised, suspicious)) { + errors.push_back({fieldName, "Potential SQL injection detected"}); + return errors; + } } return errors; diff --git a/test/integration/test_validator_hardening.py b/test/integration/test_validator_hardening.py new file mode 100644 index 00000000..3111b064 --- /dev/null +++ b/test/integration/test_validator_hardening.py @@ -0,0 +1,161 @@ +"""Request-validator hardening (#192). + +Found by the crew review of the server, reproduced against a real binary: + * the SQL keyword heuristic was bypassed with whitespace: a value such as + x'OR 2>1OR n = 'x passed it and, spliced into + `WHERE n = '{{{ params.name }}}'`, returned every row; + * `number` / `float` / `double` / `boolean` were classified as bindable (which skips + the heuristic) but had NO type validation, so `0 OR true` passed for a + `{{{ params.amount }}}` site; + * request-supplied `cacheTable` / `cacheSchema` / `cacheCatalog` overrode the + server-owned cache metadata a template renders. +""" + +import os +import subprocess +import tempfile +import time + +import pytest +import requests + +from otel_helpers import flapi_binary, free_port + + +def _w(base, rel, text): + path = os.path.join(base, rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + f.write(text) + + +def _endpoint(name, field, vtype, sql): + return (f"url-path: /{name}\nmethod: GET\ntemplate-source: {name}.sql\nconnection: [inmem]\n" + f"request:\n - field-name: {field}\n field-in: query\n validators:\n - type: {vtype}\n", + sql) + + +@pytest.fixture(scope="module") +def server(): + tmp = tempfile.mkdtemp(prefix="flapi_valhard_") + port = free_port() + _w(tmp, "flapi.yaml", + "project-name: p\nproject-description: d\ntemplate:\n path: ./sqls\n" + "connections:\n inmem:\n properties:\n database: ':memory:'\n" + "ducklake:\n enabled: true\n alias: cache\n metadata-path: ./cache.ducklake\n data-path: ./cache\n") + rows = "(VALUES ('alice', 1, true), ('bob', 2, false), ('O''Brien', 3, true)) t(n, a, b)" + specs = { + "s": _endpoint("s", "name", "string", f"SELECT * FROM {rows} WHERE n = '{{{{{{params.name}}}}}}'\n"), + "sb": _endpoint("sb", "name", "string", f"SELECT * FROM {rows} WHERE n = {{{{ params.name }}}}\n"), + "num": _endpoint("num", "amount", "number", f"SELECT * FROM {rows} WHERE a > {{{{{{params.amount}}}}}}\n"), + "flt": _endpoint("flt", "amount", "float", f"SELECT * FROM {rows} WHERE a > {{{{{{params.amount}}}}}}\n"), + "dbl": _endpoint("dbl", "amount", "double", f"SELECT * FROM {rows} WHERE a > {{{{{{params.amount}}}}}}\n"), + "bool": _endpoint("bool", "flag", "boolean", f"SELECT * FROM {rows} WHERE b = {{{{{{params.flag}}}}}}\n"), + "bl2": _endpoint("bl2", "flag", "bool", f"SELECT * FROM {rows} WHERE b = {{{{{{params.flag}}}}}}\n"), + "integer": _endpoint("integer", "amount", "integer", f"SELECT * FROM {rows} WHERE a > {{{{{{params.amount}}}}}}\n"), + } + for name, (yaml_text, sql) in specs.items(): + _w(tmp, f"sqls/{name}.yaml", yaml_text) + _w(tmp, f"sqls/{name}.sql", sql) + for t in ("one", "two"): + _w(tmp, f"sqls/{t}.yaml", + f"url-path: /{t}\nmethod: GET\ntemplate-source: {t}.sql\nconnection: [inmem]\n" + f"cache:\n enabled: true\n table: {t}_cache\n schedule: 1h\n template-file: {t}_populate.sql\n") + _w(tmp, f"sqls/{t}.sql", "SELECT * FROM {{{cache.catalog}}}.{{{cache.schema}}}.{{{cache.table}}}\n") + _w(tmp, f"sqls/{t}_populate.sql", + f"CREATE OR REPLACE TABLE {{{{{{cache.catalog}}}}}}.{{{{{{cache.schema}}}}}}.{{{{{{cache.table}}}}}} AS SELECT '{t}' AS which\n") + proc = subprocess.Popen( + [flapi_binary(), "-c", os.path.join(tmp, "flapi.yaml"), "-p", str(port), "--log-level", "warning"], + stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT, cwd=tmp, + env={**os.environ, "DATAZOO_DISABLE_TELEMETRY": "1"}) + base = f"http://127.0.0.1:{port}" + for _ in range(300): + try: + h = requests.get(f"{base}/health", timeout=2) + if h.status_code == 200 and h.json().get("caches", {}).get("total", 0) >= 2: + break + except requests.RequestException: + pass + time.sleep(0.2) + yield base + proc.terminate() + proc.wait(timeout=30) + + +def _get(base, path, **params): + return requests.get(f"{base}{path}", params=params, timeout=10) + + +INJECTIONS = [ + "x'\n\nOR 2>1\n\nOR n = 'x", # newlines (the reported bypass) + "x'\tOR\t2>1\tOR\tn\t=\t'x", # tabs + "x'\x0bOR\x0b2>1\x0bOR\x0bn='x", # vertical tab + "x'\x0cOR\x0c2>1\x0cOR n='x", # form feed + "x' OR n LIKE '%", # LIKE, no digits + "x'||(SELECT 'a')||'", # concatenation + "x' ) OR ( 'a'='a", # parentheses + "x' or true or n = 'x", # extra spaces, boolean literal + "x'\r\nOR\r\n1<2\r\nOR n='x", # CRLF + "x' UNION ALL SELECT 'a", # keyword, mixed whitespace handled by the old list too +] + + +class TestTheStringHeuristicSurvivesWhitespaceTricks: + + @pytest.mark.parametrize("payload", INJECTIONS, ids=[repr(p)[:28] for p in INJECTIONS]) + def test_an_injection_is_rejected_and_returns_no_rows(self, server, payload): + r = _get(server, "/s", name=payload) + assert r.status_code == 400, f"{payload!r} -> {r.status_code} {r.text[:150]}" + + @pytest.mark.parametrize("name,expected_rows", [("alice", 1), ("bob", 1), ("nobody", 0)]) + def test_ordinary_values_still_work(self, server, name, expected_rows): + r = _get(server, "/s", name=name) + assert r.status_code == 200, f"{name!r} -> {r.status_code} {r.text[:150]}" + assert len(r.json()["data"]) == expected_rows + + @pytest.mark.parametrize("name,expected_rows", [("O'Brien", 1), ("it's", 0), ("Bread and Butter", 0), ("Black or White", 0)]) + def test_names_with_apostrophes_and_everyday_words_work_where_the_value_is_bound(self, server, name, expected_rows): + # `{{ params.name }}` is a prepared-statement parameter: an apostrophe is just a + # character there, and the heuristic must not turn it into a 400. + r = _get(server, "/sb", name=name) + assert r.status_code == 200, f"{name!r} -> {r.status_code} {r.text[:150]}" + assert len(r.json()["data"]) == expected_rows + + +class TestNumericAndBooleanTypesAreValidated: + + @pytest.mark.parametrize("path", ["/num", "/flt", "/dbl", "/integer"]) + @pytest.mark.parametrize("bad", ["0 OR true", "1; SELECT 1", "abc", "1 2", "", "1e", "--1", "0x10", "NaN-1"]) + def test_a_non_number_is_rejected(self, server, path, bad): + r = _get(server, path, amount=bad) + assert r.status_code == 400, f"{path} amount={bad!r} -> {r.status_code} {r.text[:150]}" + + @pytest.mark.parametrize("path,good", [("/num", "1"), ("/num", "-1"), ("/num", "1.5"), ("/num", "1e0"), + ("/flt", "0.5"), ("/dbl", ".5"), ("/integer", "2")]) + def test_a_real_number_is_accepted(self, server, path, good): + r = _get(server, path, amount=good) + assert r.status_code == 200, f"{path} amount={good!r} -> {r.status_code} {r.text[:150]}" + + @pytest.mark.parametrize("path", ["/bool", "/bl2"]) + @pytest.mark.parametrize("bad", ["true OR 1=1", "yes", "2", "true;", "t", ""]) + def test_a_non_boolean_is_rejected(self, server, path, bad): + r = _get(server, path, flag=bad) + assert r.status_code == 400, f"{path} flag={bad!r} -> {r.status_code} {r.text[:150]}" + + @pytest.mark.parametrize("path", ["/bool", "/bl2"]) + @pytest.mark.parametrize("good", ["true", "false", "TRUE", "False", "1", "0"]) + def test_a_real_boolean_is_accepted(self, server, path, good): + r = _get(server, path, flag=good) + assert r.status_code == 200, f"{path} flag={good!r} -> {r.status_code} {r.text[:150]}" + + +class TestCacheMetadataIsServerOwned: + + def test_a_request_cannot_redirect_a_cached_endpoint_to_another_table(self, server): + assert _get(server, "/one").json()["data"][0]["which"] == "one" + assert _get(server, "/two").json()["data"][0]["which"] == "two" + for key, value in (("cacheTable", "two_cache"), ("cacheSchema", "audit"), ("cacheCatalog", "memory")): + r = _get(server, "/one", **{key: value}) + body = r.json() if r.status_code == 200 else {} + assert r.status_code == 200 and body["data"][0]["which"] == "one", ( + f"{key}={value} changed what /one reads: {r.status_code} {r.text[:150]}")