From 984a3c11bb658762783401c7e92de02eaf9718a3 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Tue, 3 Nov 2026 20:43:38 +0100 Subject: [PATCH 1/3] security: validate_ident let a name with a trailing newline through Python's `$` also matches just before a trailing newline, so `_SAFE_IDENT.match("orders\n")` succeeded and validate_ident returned "orders\n" unchanged. Every DDL emitter routes identifiers through it, and a name written into a line-based file (a Java properties file, as in #784) gains a second line. The four whole-string allowlists in _sql_safety.py now end in `\Z`. Only _SAFE_IDENT changes behaviour: the type-name, type-parameter and RLS-expression helpers already strip their input, so `\Z` there keeps a later edit that drops the strip from reopening the gap. tests/providers/test_sql_safety_type_param.py pins validate_ident against "\n", "\r\n" and an embedded "\n", and each pattern against a trailing "\n". --- fluid_build/providers/_sql_safety.py | 14 ++++-- tests/providers/test_sql_safety_type_param.py | 48 ++++++++++++++++++- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/fluid_build/providers/_sql_safety.py b/fluid_build/providers/_sql_safety.py index 6722aac88..2d2b30539 100644 --- a/fluid_build/providers/_sql_safety.py +++ b/fluid_build/providers/_sql_safety.py @@ -17,7 +17,11 @@ import re from typing import Any, Mapping, Optional -_SAFE_IDENT = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +# The whole-string allowlists in this module end in ``\Z``, not ``$``. Python's +# ``$`` also matches just before a trailing newline, so ``validate_ident`` used +# to return ``"orders\n"`` unchanged, and that newline split any line-based +# file the name was written into (a Java properties file, a log line). +_SAFE_IDENT = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*\Z") # Identifier-quote characters for every supported dialect belong in the # safe set: ``"`` (ANSI / Snowflake), ``[`` ``]`` (T-SQL), and ``` ` ``` # (BigQuery / MySQL). The backtick was the missing one — BigQuery table @@ -28,7 +32,7 @@ # be rejected and the ``query`` tool fails on BigQuery. A backtick can't # open a statement or a comment, so the ``;`` / ``--`` / ``/* */`` and # blocked-keyword guards below still hold. -_SAFE_EXPR_CHARS = re.compile(r"^[A-Za-z0-9_\s().,<>=!'+\-*/%|&\"`:\[\]]+$") +_SAFE_EXPR_CHARS = re.compile(r"^[A-Za-z0-9_\s().,<>=!'+\-*/%|&\"`:\[\]]+\Z") _BLOCKED_EXPR_TOKENS = re.compile( r"(?i)\b(" r"alter|call|copy|create|delete|drop|execute|grant|insert|merge|put|remove|" @@ -132,7 +136,7 @@ def dialect_uses_backslash_escapes(dialect: str | None) -> bool: # at most one comma so a contract cannot smuggle DDL through a type parameter # (identifiers/literals are already routed through ``validate_ident`` / # ``quote_string_literal``). -_SQL_TYPE_PARAM_PAYLOAD = re.compile(r"^[0-9]+(\s*,\s*[0-9]+)?$") +_SQL_TYPE_PARAM_PAYLOAD = re.compile(r"^[0-9]+(\s*,\s*[0-9]+)?\Z") # A *whole* column-type token (not just the ``(...)`` payload) such as # ``INT`` / ``VARCHAR(255)`` / ``NUMBER(18,4)`` / ``TIMESTAMP_NTZ`` / @@ -143,7 +147,7 @@ def dialect_uses_backslash_escapes(dialect: str | None) -> bool: # set deliberately excludes ``;`` ``"`` ``-`` ``*`` ``/`` ``=`` so a type # token cannot carry a statement terminator, an identifier-quote, or a # comment sequence into the surrounding ``CREATE TABLE`` DDL. -_SQL_TYPE_NAME = re.compile(r"^[A-Za-z0-9_ ()',]+$") +_SQL_TYPE_NAME = re.compile(r"^[A-Za-z0-9_ ()',]+\Z") class SqlTypeError(ValueError): @@ -161,7 +165,7 @@ def validate_sql_type_name(type_name: str) -> str: (e.g. ``VARCHAR(255)``) directly into ``CREATE TABLE`` SQL. ``validate_ident`` is too strict for parameterised / multi-word types (it forbids parens and spaces), so this applies the conservative type allowlist - ``^[A-Za-z0-9_ ()',]+$`` and additionally fails closed on comment + ``^[A-Za-z0-9_ ()',]+\\Z`` and additionally fails closed on comment sequences (``--`` / ``/*`` / ``*/``) — defence-in-depth mirroring :func:`validate_sql_expression_allowlist`. Raises :class:`SqlTypeError` (a ``ValueError`` subclass) on anything outside the allowlist so callers diff --git a/tests/providers/test_sql_safety_type_param.py b/tests/providers/test_sql_safety_type_param.py index 912fe0ac3..eba0a2ca6 100644 --- a/tests/providers/test_sql_safety_type_param.py +++ b/tests/providers/test_sql_safety_type_param.py @@ -21,11 +21,22 @@ at most one comma), so a contract cannot smuggle a statement terminator, comment sequence, or nested DDL through a type parameter. Each test here FAILS if that guard is removed or loosened. + +It also pins how the module's whole-string allowlists anchor. Python's ``$`` +also matches just before a trailing newline, so a ``$``-anchored pattern let +``validate_ident("orders\\n")`` through. """ import pytest -from fluid_build.providers._sql_safety import SqlTypeError, validate_sql_type_param_payload +from fluid_build.providers._sql_safety import ( + _SAFE_IDENT, + _SQL_TYPE_NAME, + _SQL_TYPE_PARAM_PAYLOAD, + SqlTypeError, + validate_ident, + validate_sql_type_param_payload, +) class TestValidateSqlTypeParamPayload: @@ -54,3 +65,38 @@ def test_rejects_non_str_input(self): or silently mis-handle it.""" with pytest.raises(SqlTypeError): validate_sql_type_param_payload(100) # type: ignore[arg-type] + + +class TestValidateIdentLineBreaks: + @pytest.mark.parametrize( + "name", + ["orders\n", "orders\r\n", "ord\ners"], + ids=["trailing-lf", "trailing-crlf", "embedded-lf"], + ) + def test_rejects_line_break(self, name): + """An identifier with a line break anywhere in it is rejected. Only the + trailing ``\\n`` used to pass, because ``$`` matches just before it; the + other two pin that a fix neither strips the input nor matches it line + by line.""" + with pytest.raises(ValueError, match="Invalid SQL identifier"): + validate_ident(name) + + +class TestAllowlistAnchors: + @pytest.mark.parametrize( + ("pattern", "value"), + [ + (_SAFE_IDENT, "orders"), + (_SQL_TYPE_NAME, "VARCHAR(255)"), + (_SQL_TYPE_PARAM_PAYLOAD, "18,4"), + ], + ids=["ident", "type-name", "type-param"], + ) + def test_pattern_refuses_trailing_newline(self, pattern, value): + """Each whole-string allowlist refuses a trailing newline by itself. + ``validate_sql_type_name`` and ``validate_sql_type_param_payload`` strip + their input first, so the pattern is what holds if a later edit drops + that ``.strip()``. ``_SAFE_EXPR_CHARS`` is left out: its class contains + ``\\s``, which admits a newline anyway.""" + assert pattern.match(value) + assert pattern.match(value + "\n") is None From 9829ccd7eac16a4142288c19473eeb3ada5e021a Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Tue, 3 Nov 2026 20:52:12 +0100 Subject: [PATCH 2/3] docs(changelog): validate_ident refuses a name with a trailing newline --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c8ddb4c..3cd0370d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 passes when the region matches the declaration, fails with the fix when it does not, and is skipped when the contract declares none. +### Security + +- **`validate_ident` let a name with a trailing newline through.** Python's `$` also matches just + before a trailing newline, so the identifier allowlist in `providers/_sql_safety.py`, which the + DDL and OpenTofu emitters route names through, accepted `orders\n` and passed it on with the + newline: `fluid generate iac` wrote `CREATE EXTERNAL SCHEMA IF NOT EXISTS spectrum_events\n ...` + into `main.tf.json`. The module's allowlists now end in `\Z`, so such a name is refused with + `Invalid SQL identifier: 'spectrum_events\n'`. That includes a name that reaches it with a + trailing newline from an environment variable or a file; strip the value where it is set. + ## [0.26.0] — 2026-11-01 ### Added From 926e87fd588d0d25f4ae231bb7105bb7a85b1bde Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Tue, 3 Nov 2026 20:58:30 +0100 Subject: [PATCH 3/3] chore(secrets): move the two _sql_safety baseline hits to their new lines --- .secrets.baseline | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 233ab9f48..d2bd5129f 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -436,14 +436,14 @@ "filename": "fluid_build/providers/_sql_safety.py", "hashed_secret": "fb2620e2ed7e530fcb1ea0a2b2456751057a5178", "is_verified": false, - "line_number": 244 + "line_number": 248 }, { "type": "Secret Keyword", "filename": "fluid_build/providers/_sql_safety.py", "hashed_secret": "6d613a1ee01eec4c0f8ca66df0db71dca0c6e1cf", "is_verified": false, - "line_number": 245 + "line_number": 249 } ], "fluid_build/providers/datamesh_manager/README.md": [ @@ -2154,5 +2154,5 @@ } ] }, - "generated_at": "2026-10-31T09:00:16Z" + "generated_at": "2026-11-03T19:58:17Z" }