redash: Redash does not escape quotes in parameter values

Yes, it’s very annoying… We would like to use Redash for production environnement but it’s not possible because of this issue.

Though I am a bit hesitant to share at this point, I have come up with a couple commits that might be a start to solving this.

I started by reworking the parameter validation code a bit to simplify writing per type validation/normalization logic and to get a feel for that part of the code base. Before adding the escaping feature I added the unrelated feature of “Optional parameters” to get a better understanding of the interaction between frontend and the API, resulting in 3 main commits as well as a couple of smaller ones to fix some deprecation warnings.

I would not say this is ready for a PR yet, mostly because I feel like I am not comfortable with the codebase yet and I’d definitely not put everything in a single PR in the end.

It also only implements escaping for the PostgreSQL data source, but I guess adding it for your own data sources is not that big of a deal after checking out the pg.py diff. 😃

My current set of changes is at https://github.com/solute/redash/tree/improve_query_params

We are also checking out redash for various purposes right now and ran into this “issue”. In order for us to put an a bit of effort into this, I’d like to get a feel for how big solving this could get. There are a couple points I see would need to be answered before some effort could be put into this:

Escaping vs. quoting

Are we “only” talking about escaping or quoting? Using Query Based Dropdown List does “manual” quoting right now, but no escaping, so I guess technically we could be satistied with escaping only.

Should escaping be datasource aware

Different datasources may require different escaping styles or even require an active datasource connection to properly escape a string like with https://www.postgresql.org/docs/14/libpq-exec.html#LIBPQ-PQESCAPESTRING though from the docs I can’t tell if it just may not be producing the correct result or if there is a slight security risk because users might still escape from the escaping logic.

If escaping is datasource aware, should it be optional?

Should we require every datasource provider to support this and fix all of them before making this a feature, or should this be a optional? I’d say every datasource should have a way to escape strings in their lowlevel library, but I have a feeling that this assumption would just lead to a lot of datasources sporting a very naive handcrafted escaping logic that will ultimately turn out to be unsafe. 😦

Configuring parameters vs. using templates “correctly”

One could argue that the behavior of {{ }} is to escape (even though it only does html) so in our case it should probably escape with custom logic for the datasource, you could always use {{{ }}} to reference the unescaped variant. While this seems nice at first, we’d need to “look into the template” to see if a text type parameter is safe or not.

My 2 cents

I feel like doing “the right thing ™” could quickly get ugly and introduce a couple incompatibilities, I think I’d prefer to add and “Escape” option to the relevant parameter types and make the logic datasource aware, so we should do the escaping as late as possible so just in case we really need a datasource connection, we do have access to it in the query worker. I would keep escaping an optional feature, even though that would require a bit more work on the frontend side, exposing the fact that a datasource can do escaping in order for the “Escape” option to be available in the dialog.

On could argue that having a separate safe-text type would keep the ParameterizedQuery.is_safe property simpler, because it could just keep checking for the text type instead of having to inspect it’s configuration, but I think that is probably just a matter of taste and I’d prefer to consistently use the “Escape” option instead of sometimes using that and other times using a separate type.

Some feedback would really be appreciated. 😃