mosaic icon indicating copy to clipboard operation
mosaic copied to clipboard

Support query parameters in SQL generation

Open derekperkins opened this issue 5 months ago • 1 comments

DuckDB requires that you run a prepare statement to use query parameters, but most other databases allow them to be used in a single step. For most systems, this is the primary security for protecting against SQL injection. Since the mosaic server accepts raw SQL, other measures have to be taken, likely via database user permissions. The primary use case here would be to simplify escaping, or allow for performance improvements as suggestion in #483.

Considerations:

  • With the new visitor pattern to generate SQL, there would need to be some state to collect parameters as the SQL is generated
  • There probably also needs to be some accessible state to make an auto-incremented id or other unique id available, to avoid naming collisions
  • Positional parameters could be tricky to align, so I'd prefer to see named parameter support, but I think both could be supported by specific visitors

This isn't particularly urgent. I've been trained to always use params, so my knee jerk reaction when working on a BigQuery dialect was to use them, but as I typed out this issue, the whole mosaic query pathway is a giant SQL injection :)

Related issue:

  • https://github.com/uwdata/mosaic/issues/483

derekperkins avatar Sep 19 '25 02:09 derekperkins

I had a draft pull request in https://github.com/uwdata/mosaic/pull/486 for prepared statements/query params. It wasn't helping with perf and also surfaced a bug in ducked node. But I think with the visitor pattern from recently, redoing this feature should be a lot easier. I think it makes sense to move to query params as it should allow us to use native numbers as well rather than having to escape + stringify them.

domoritz avatar Oct 09 '25 15:10 domoritz