Skip to content

Commit 706fd12

Browse files
committed
feat(sql): null-check SELECT * + recognise CTE names (L4/L2 soundness)
Two more soundness holes in the SQL safety levels: - L4 (null-safety): `SELECT *` / `u.*` were not expanded, so nullable columns selected via a wildcard were silently not flagged. Expand a wildcard to the in-scope table columns (resolving the alias for a qualified `u.*`) and flag the nullable ones. - L2 (schema-binding): a `WITH cte AS (...)` name referenced in FROM was reported as 'table not found', a false positive. Collect CTE names and exclude them from the table-existence check. Updates l4_select_star (was a no-op documenting the gap) to assert the nullable columns are now flagged, and adds an L2 CTE test.
1 parent 00cbc4e commit 706fd12

2 files changed

Lines changed: 94 additions & 8 deletions

File tree

src/plugins/sql.rs

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,21 @@ impl SqlPlugin {
159159
.unwrap_or_else(|| qualifier.to_string())
160160
}
161161

162+
/// Names introduced by a `WITH` clause. These act as table sources within
163+
/// the query but are not part of the schema, so they must not be flagged as
164+
/// "table not found".
165+
fn extract_cte_names(statement: &Statement) -> Vec<String> {
166+
let mut names = Vec::new();
167+
if let Statement::Query(query) = statement
168+
&& let Some(with) = &query.with
169+
{
170+
for cte in &with.cte_tables {
171+
names.push(cte.alias.name.value.to_lowercase());
172+
}
173+
}
174+
names
175+
}
176+
162177
/// Extract all column references from a statement.
163178
fn extract_column_refs(statement: &Statement) -> Vec<(Option<String>, String)> {
164179
let mut cols = Vec::new();
@@ -386,8 +401,11 @@ impl QueryLanguagePlugin for SqlPlugin {
386401
// Check table references
387402
let table_refs = Self::extract_table_refs(stmt);
388403
let aliases = Self::extract_table_aliases(stmt);
404+
let cte_names = Self::extract_cte_names(stmt);
389405
for table_name in &table_refs {
390-
if !schema.tables.iter().any(|t| t.name == *table_name) {
406+
if !cte_names.contains(table_name)
407+
&& !schema.tables.iter().any(|t| t.name == *table_name)
408+
{
391409
issues.push(SchemaIssue {
392410
message: format!("Table '{}' not found in schema", table_name),
393411
});
@@ -518,6 +536,43 @@ impl QueryLanguagePlugin for SqlPlugin {
518536
});
519537
}
520538
}
539+
// Unqualified `*`: expand to every nullable column of
540+
// each table in scope (so `SELECT * FROM users` is
541+
// null-checked, not silently skipped).
542+
SelectItem::Wildcard(_) => {
543+
for table_name in &table_refs {
544+
if let Some(table) =
545+
schema.tables.iter().find(|t| t.name == *table_name)
546+
{
547+
for col in table.columns.iter().filter(|c| c.nullable) {
548+
issues.push(NullIssue {
549+
message: format!(
550+
"Nullable column '{}' selected via wildcard without COALESCE or null handling",
551+
col.name
552+
),
553+
column: col.name.clone(),
554+
});
555+
}
556+
}
557+
}
558+
}
559+
// Alias-qualified `u.*`: expand the resolved table only.
560+
SelectItem::QualifiedWildcard(obj, _) => {
561+
let table_name =
562+
Self::resolve_qualifier(&aliases, &obj.to_string().to_lowercase());
563+
if let Some(table) = schema.tables.iter().find(|t| t.name == table_name)
564+
{
565+
for col in table.columns.iter().filter(|c| c.nullable) {
566+
issues.push(NullIssue {
567+
message: format!(
568+
"Nullable column '{}' selected via wildcard without COALESCE or null handling",
569+
col.name
570+
),
571+
column: col.name.clone(),
572+
});
573+
}
574+
}
575+
}
521576
_ => {}
522577
}
523578
}

tests/integration_test.rs

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -608,17 +608,48 @@ fn l4_nullable_comment_author() {
608608
}
609609

610610
#[test]
611-
fn l4_select_star_not_flagged() {
612-
// SELECT * doesn't produce individual Identifier expressions for each column,
613-
// so the null checker won't flag individual columns.
611+
fn l4_select_star_flags_nullable() {
612+
// `SELECT *` is expanded to the table's columns, so its nullable columns
613+
// (users.email, users.age) are flagged like an explicit selection would be.
614614
let plugin = get_plugin("sql").unwrap();
615615
let schema = test_schema();
616616
let issues = plugin.null_check("SELECT * FROM users", &schema).unwrap();
617-
// The current implementation only checks UnnamedExpr(Identifier), not Wildcard.
618-
// This test documents current behavior.
619617
assert!(
620-
issues.is_empty(),
621-
"SELECT * is not individually checked for null (current behavior)"
618+
issues.iter().any(|i| i.column == "email"),
619+
"SELECT * must flag nullable 'email'. Got: {:?}",
620+
issues
621+
);
622+
assert!(
623+
issues.iter().any(|i| i.column == "age"),
624+
"SELECT * must flag nullable 'age'. Got: {:?}",
625+
issues
626+
);
627+
// Non-nullable columns (id, name) must NOT be flagged.
628+
assert!(
629+
!issues
630+
.iter()
631+
.any(|i| i.column == "id" || i.column == "name"),
632+
"SELECT * must not flag non-nullable columns. Got: {:?}",
633+
issues
634+
);
635+
}
636+
637+
#[test]
638+
fn l2_cte_name_not_flagged_as_missing_table() {
639+
// A CTE name is a valid in-query table source, not a schema table, so it
640+
// must not be reported as "not found in schema".
641+
let plugin = get_plugin("sql").unwrap();
642+
let schema = test_schema();
643+
let issues = plugin
644+
.schema_check(
645+
"WITH recent AS (SELECT id FROM users) SELECT id FROM recent",
646+
&schema,
647+
)
648+
.unwrap();
649+
assert!(
650+
!issues.iter().any(|i| i.message.contains("recent")),
651+
"CTE name 'recent' must not be flagged as a missing table. Got: {:?}",
652+
issues
622653
);
623654
}
624655

0 commit comments

Comments
 (0)