FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

Provide ability to declare variables directly in .sql scripts (to be interpreted by ISQL and substituted in appropriate places) #8232

Open pavel-zotov opened 2 months ago

pavel-zotov commented 2 months ago
connect '192.168.23.45/3210:employee_1' user john password 'QweRty';
...
...
connect '192.168.19.23/3350:employee_2' user mike password 'AsdFgh';
...
...
connect '192.168.23.45/3210:employee_1' user john password 'QweRty';
...
...
connect '192.168.19.23/3350:employee_2' user mike password 'AsdFgh';

(we had specify connection string, user names and password two times; passwords are in open form)

What it could be instead:

declare $conn_1 varchar(512) =  '192.168.23.45/3210:employee_1';
declare $conn_2 varchar(512) =  '192.168.19.23/3350:employee_2';
declare $user_1 varchar(63) = 'john';
declare $user_2 varchar(63) = 'mike';
declare $pswf_1 varchar(128) = '/home/dba/.hidden/john.password.txt'; -- file with text: 'QweRty'
declare $pswf_2 varchar(128) = '/home/dba/.hidden/mike.password.txt'; -- file with text: 'AsdFgh'

connect '$conn_1' user $user_1 fetch password from '$pswf_1';
...
...
connect '$conn_2' user $user_2 fetch password from '$pswf_2';
mrotteveel commented 2 months ago

This doesn't sound like a good idea (at least, not with this syntax). You'd make it impossible to distinguish between literal values and parameters, making this a risk for SQL injection.

mrotteveel commented 2 months ago

If this were to be introduced, you'd need a separate syntax for literals to signify they should consider the variables mentioned. E.g., something like f'{$conn_1}', where the f introducer signifies that it supports a formatted string, and the {...} inside the literal signifies the expression to evaluate and include in the actual value produced (e.g. like Python's formatted-strings).

mrotteveel commented 2 months ago

Similarly, I'd say that the bare use of a variable in place of an identifier could be problematic, and might need some additional thought.

aafemt commented 2 months ago

It is not much hard to implement while these variables are used in isql client-side commands (including syntax similar to DSQL, i.e. connect :conn_1 user :user_1 fetch password from :pswf_1). But in this case they are next to useless. Real problem will be when you ask for assignment of a select result into these variables. Expanding isql parser to be a full-featured programming language kinda defeat purpose of isql as a primitive utility with minimal amount of bugs inside. If you need a rich interpreted language - you can use Python or PHP.

pavel-zotov commented 2 months ago

But in this case they are next to useless. Real problem will be when you ask for assignment of a select result into these variables

No, i just meant to have ability to specify some data only once. Nothing about assignment.

AlexPeshkoff commented 2 months ago

There are a lot various preprocessors performing similar tasks, starting with m4. No need to put everything into single utility.

pavel-zotov commented 2 months ago

You'd make it impossible to distinguish between literal values and parameters, making this a risk for SQL injection.

I could not understand here. Parameters (currently) can be specified only in PSQL units or in execute statement (which also can be only inside PSQL or execute block). I tried several times to make some kind of injections (for example, shown here: https://www.w3schools.com/sql/sql_injection.asp ), including loop with ES - but without success. FB compiles statements every time and rejects access if i had no appropriate privileges.

pavel-zotov commented 2 months ago

There are a lot various preprocessors performing similar tasks, starting with m4. No need to put everything into single utility.

This forces to have 3rd party utility and apply it to script before passing its result to ISQL, right ?

mrotteveel commented 2 months ago

You'd make it impossible to distinguish between literal values and parameters, making this a risk for SQL injection.

I could not understand here. Parameters (currently) can be specified only in PSQL units or in execute statement (which also can be only inside PSQL or execute block). I tried several times to make some kind of injections (for example, shown here: https://www.w3schools.com/sql/sql_injection.asp ), including loop with ES - but without success. FB compiles statements every time and rejects access if i had no appropriate privileges.

Not all SQL injection is malicious. It could be as simple as having literally a password $pswf_2 (or that string literal value elsewhere, e.g. in an INSERT), and your proposal would instead inject AsdFgh (or /home/dba/.hidden/mike.password.txt, because simply expecting a normal string literal to be replaced with the content of some file without some explicit syntax is also not something I'd want). At best that would result in a "user name or password" error, at worst, you'd inject that password somewhere where the value $pswf_2 was expected.

asfernandes commented 2 months ago

Oracle SQL*Plus has this (turned on by default) using SET DEFINE. https://www.oreilly.com/library/view/oracle-sqlplus-the/0596007469/re56.html

mrotteveel commented 2 months ago

To be clear, I'm not against a feature like this, but I do think it needs to get a little bit more thought about its syntax, and implications.

pavel-zotov commented 2 months ago

To be clear, I'm not against a feature like this, but I do think it needs to get a little bit more thought about its syntax, and implications.

You right: the f-notation (f'{$conn_1}') seems our best friend... :-)

mrotteveel commented 2 months ago

To be clear, I'm not against a feature like this, but I do think it needs to get a little bit more thought about its syntax, and implications.

You right: the f-notation (f'{$conn_1}') seems our best friend... :-)

But that would also need some careful thought, as - for example - doing so would preclude using that syntax in PSQL itself, to create formatted strings nicely (which admittedly, I think might be more valuable than being able to use something like this in ISQL).

pavel-zotov commented 2 months ago

doing so would preclude using that syntax in PSQL itself, to create formatted strings nicely

If we will able to do something like this:

declare $conn_1 varchar(512) =  '192.168.23.45/3210:employee_1';
declare $user_1 varchar(63) = 'john';
declare $pswf_1 varchar(128) = '/home/dba/.hidden/john.password.txt'; -- file with text: 'QweRty'
declare $checked_group_id int = 123;
set term ^;
execute block as
    declare v_ id int
begin
      for
          execute statement ('select ... from ... where group_id = ?')  (  f'$checked_group_id' )
          on external f'$conn_1' as user f'$user_1' fetch password from f'$pswf_1'
          as cursor c
    do begin
        ...
    end
end
^

-- then what's wrong ?

mrotteveel commented 2 months ago

You're missing the fact that it would be processed in ISQL, meaning that you would remove the ability to use such syntax in PSQL itself (as in, processed by the server, not ISQL).

pavel-zotov commented 2 months ago

Why ISQL (or some preprocessor) can not just search and replace the whole script before sending to server ? No matter is this PSQL block or direct statements.

asfernandes commented 2 months ago

As this could be used inside strings, it may be unusable in scripts that require the same marker inside scripts, so it must be configurable, like Oracle's SET DEFINE is. And if it's not turned ON by default (and I think it should not be ON by default), we should create no syntax character, just let people define it.

It should also be ready for dumb ISQL's client parser.

aafemt commented 2 months ago

-- then what's wrong ?

Everything: isql would have to have a full-size (P)SQL parser instead of current simplified one.

I would write your script like this:

declare conn_1 varchar(512) =  '192.168.23.45/3210:employee_1';
declare user_1 varchar(63) = 'john';
declare pswf_1 varchar(128) = '/home/dba/.hidden/john.password.txt'; -- file with text: 'QweRty'
declare checked_group_id int = 123;
set term ^;
execute block (conn varchar(512) = :conn_1, usr varchar(63) = :user_1, pass varchar(128) = :pswf_1) as
    declare v_ id int
begin
      for
          execute statement ('select ... from ... where group_id = ?')  (  :id )
          on external :conn as user :usr password :pass
          as cursor c
    do begin
        ...
    end
end
^

In this form isql wouldn't need full parser but DSQL would need named parameters.

mrotteveel commented 2 months ago

Why ISQL (or some preprocessor) can not just search and replace the whole script before sending to server ? No matter is this PSQL block or direct statements.

It can, but I'm talking about a hypothetical situation where ISQL has this f-string syntax, that you would not be able to introduce it for PSQL itself (or at least, you'd need something so that ISQL can distinguish between its own f-strings and PSQL f-strings).

That is what I mean with having to carefully consider the consequences of syntax changes, especially client-only syntax for one specific client (ISQL), where the proposed syntax looks a lot like the server syntax. It is very easy to paint yourself in a corner this way, and cause a lot of confusion with users, because I predict that if this is introduced, we'll soon get questions why this syntax doesn't work when using IB Expert, DBeaver or some other database query tool.

sim1984 commented 2 months ago

Still, I would distinguish macro substitutions from user-defined variables.

Substitutions are really easy to organize in ISQL and they are useful. You can take the Oracle implementation.

SET DEFINE [ON | OFF | <prefix>];

DEFINE <VAR> = <text>;
SET DEFINE &;
DEFINE VAR=1;
SELECT &VAR FROM RDB$DATABASE;

Defining bind variables is much more complex and they have their own limitations.

aafemt commented 2 months ago

We already have two syntax's for parameter's constitution: unnamed ? and named :<name>. What's the reason to introduce another? I prefer consistency.

sim1984 commented 2 months ago

The suggested syntax is not a query parameter. It is a macro substitution. And only at the ISQL level. With its help, you can, for example, substitute the name of a table or procedure, which cannot be done using a parameter. Consider it an analogue of the #define directive in C++.

aafemt commented 2 months ago

It can be done with execute statement.