mcp: validate data product names before interpolating into SQL

[bobbyiliev]

Validates that data product names passed to read_data_product are syntactically valid SQL identifiers by parsing a probe query locally, preventing malformed or injected names from reaching the database as opaque SQL errors.

[github-actions]

Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone.

PR title guidelines

• Use imperative mood: "Fix X" not "Fixed X" or "Fixes X"
• Be specific: "Fix panic in catalog sync when controller restarts" not "Fix bug" or "Update catalog code"
• Prefix with area if helpful: compute: , storage: , adapter: , sql:

Pre-merge checklist

• The PR title is descriptive and will make sense in the git log.
• This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
• If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.
• This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
• If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
• If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).

[ggevay]

I'm wondering, couldn't we instead use something like StrExt::quoted? The quotation should make any SQL keywords not be parsed as keywords, but parsed as part of the name. Also, StrExt::quoted makes sure to escape quotation marks, so the user can't break out of the quotation.

[ggevay]

Why not allow an unqualified name? Actually, the test

assert!(validate_data_product_name("my_view").is_ok());

seems to indicate that we do allow unqualified names.

[bobbyiliev]

Ops yes, the error message was misleading! We do accept unqualified names. Updated the message to show both forms as examples.

[bobbyiliev]

I'm wondering, couldn't we instead use something like StrExt::quoted? The quotation should make any SQL keywords not be parsed as keywords, but parsed as part of the name. Also, StrExt::quoted makes sure to escape quotation marks, so the user can't break out of the quotation.

@ggevay I think StrExt::quoted might not work here because it escapes double quotes with backslash (\") which is for display/logging, whereas SQL identifiers use doubled quotes (""). So I'm not sure it would be safe to use directly in SQL.

The other thing is the name arrives from mz_mcp_data_products as a pre-formatted multi-part identifier like "materialize"."schema"."view", so we can't really wrap the whole thing in one set of quotes since it needs to be parsed as a qualified name.

The AST validation approach makes sure the name is safe to interpolate as-is without transforming it. But I might be missing something, do you have a different approach in mind?

[ggevay]

Ah yes, StrExt::quoted is not good.

My problem with the PR's approach is that it is a lot of brittle code to maintain. E.g., when someone later adds to the AST, they'll have to not forget to update this code as well. This could be enforced somewhat by relying more on pattern matching, with no wildcards, so that one would get compile errors in validate_data_product_name if they forget to attend to validate_data_product_name when adding AST stuff.

But another alternative approach would be to call parse_item_name (making it pub) to get an UnresolvedItemName (which takes care of having or not having qualifications), and then when interpolating into the final SQL query (not a probe, but the real query) print it with an AstFormatter that has FormatMode::Stable, so that everything will be quoted (see AstDisplay for Ident), so that nasty things can't escape from it. This would be just a few lines of code, unless I'm missing something. Do you think this would be feasible?

[bobbyiliev]

@ggevay ah nice suggestion, just implemented it, much cleaner! Added a public parse_item_name to mz-sql-parser (following the same pattern as parse_expr/parse_data_type), and replaced the AST inspection with:

fn safe_data_product_name(name: &str) -> Result<String, McpRequestError> {
    let parsed = parse_item_name(name).map_err(|_| ...)?;
    Ok(parsed.to_ast_string_stable())
}

Thanks for the suggestion and let me know what you think!

[ggevay]

LGTM, thanks for iterating on it!

[ggevay]

(stale comment)