fix: request-validator hardening (#192, heuristic scope)

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'<newline>OR 2>1<newline>OR 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.
+
 ### Security: three rate-limit bypasses
 
 Found by a review of the server and reproduced against a real binary:

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`
 
 ---
 

src/cache_manager.cpp

@@ -980,9 +980,12 @@ void CacheManager::addQueryCacheParamsIfNecessary(std::shared_ptr<ConfigManager>
     }
 
     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<ConfigManager> config_manager, const EndpointConfig& endpoint, const std::vector<std::string> previousTableNames) {

src/config_manager.cpp

@@ -962,7 +962,7 @@ void ConfigManager::parseEndpointValidators(const YAML::Node& req, RequestFieldC
             ValidatorConfig validatorConfig;
             validatorConfig.type = safeGet<std::string>(validator, "type", "validators.type");
 
-            if (validatorConfig.type == "int") {
+            if (validatorConfig.type == "int" || validatorConfig.type == "integer") {
                 validatorConfig.min = safeGet<int>(validator, "min", "validators.min", std::numeric_limits<int>::min());
                 validatorConfig.max = safeGet<int>(validator, "max", "validators.max", std::numeric_limits<int>::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<int>(v["min"].i()) : std::numeric_limits<int>::min();
         out.max = v.has("max") ? static_cast<int>(v["max"].i()) : std::numeric_limits<int>::max();
     } else if (out.type == "string") {

src/include/request_validator.hpp

@@ -24,6 +24,8 @@ class RequestValidator {
 
 private:
     std::vector<ValidationError> validateField(const RequestFieldConfig& field, const std::map<std::string, std::string>& params);
+    std::vector<ValidationError> validateNumber(const std::string& fieldName, const std::string& value);
+    std::vector<ValidationError> validateBoolean(const std::string& fieldName, const std::string& value);
     std::vector<ValidationError> validateString(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator);
     std::vector<ValidationError> validateInt(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator);
     std::vector<ValidationError> validateEmail(const std::string& fieldName, const std::string& value);

src/request_validator.cpp

@@ -54,9 +54,17 @@ std::vector<ValidationError> 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<ValidationError> RequestValidator::validateField(const RequestFieldC
     return errors;
 }
 
+std::vector<ValidationError> 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<ValidationError> 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<ValidationError> RequestValidator::validateString(const std::string& fieldName, const std::string& value, const ValidatorConfig& validator) {
     std::vector<ValidationError> errors;
     if (validator.min > 0 && static_cast<int>(value.size()) < validator.min) {
@@ -312,42 +339,35 @@ std::vector<ValidationError> 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'<newline><newline>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<char>(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;