Closed djyotta closed 1 month ago
The documentation is currently not super clear:
$ParameterName works the same as :ParameterName, but it can also be set through a query parameter in the URL. If you add ?x=1&y=2 to the end of the URL of your page, $x will be set to the string '1' and $y will be set to the string '2'. If a query parameter was not provided, it is set to NULL.
The behavior is:
:x
: post variable set by form?x
(where supported) : get variable set through url parameter$x
: post variable, and when the post variable is not set, get variable. This has always been the case, and changing it would break applications. But I agree it's confusing, improperly documented, and could lead to security vulnerabilities if used improperly.
The solution for you in the short term is to use SET :x
instead of SET $x
.
But in the long term, I'm ambivalent about what should be done. What do you think?
Ah so, would I be correct in thinking the "safest" thing to do for now is use SET :x
for internal variables that should never be set by users? ie,
SET :x = 'safe/value.sql';
SELECT 'dynamic' AS component, sqlpage.run_sql(:x) AS properties; -- safe because of prior SET
A related observation: though I see I've used SET $content;
in my example, I personally have been using SET content;
(no $) everywhere, including when I hit this issue. It seems SET content;
behaves like SET $content;
is that correct?
:x : post variable set by form ?x (where supported) : get variable set through url parameter $x : post variable, and when the post variable is not set, get variable.
Ah OK, from the docs, my takeaway was to use $x everywhere unless I wanted to specifically discriminate between GET and POST params. But if SET is setting only the GET param (is it?) when using SET $x
, then the behaviour I'm seeing makes sense.
Here are my thoughts on current behaviour and potential improvements:
SET $x
should do or if it is well defined at allProposal: | Symbol | SET | SELECT |
---|---|---|---|
& |
Write internal var only | Resolve to internal var (if not null) else fallback to POST, then GET. Warn on using with run_sql |
|
! |
Write internal var only | Resolve to internal var only. Safe for use with run_sql |
|
$ |
Existing behaviour | Warn on usage with run_sql and SET. Recommend against using in SELECT - use & instead |
SET $x
should do or if it is well defined at allFor example, when referencing the value "reading":
SELECT $x; -- like SELECT COALESCE(:x, ?x)
But during assignment "writing":
SET $x; -- sets the GET parameter only
For :x
and ?x
I think the behaviour of the "write" op is obvious given the definition of the "read" op. But for $x
it's not intuitive.
I can see how it could make sense the way it is, but on the other hand, it's weird that:
-- :x is 'data'
SET $x = 'blah';
SELECT $x; -- returns 'data'
SET &x = 'blah'; -- doesn't affect GET or POST param, just sets internal var
SELECT &x; -- like SELECT COALESCE(!x, $x); -- favour internal var, then fallback to POST, then GET
I would even argue using the proposed &x
instead of $x
everywhere would result in expected behaviour for most users.
It would also make comparison of computed var &x with user supplied param $x reveal if a param has been modified internally or not without introducing a placeholder variable with a distinct name to keep the original and modified values separate.
SET !x = 'blah';
SELECT !x; -- NULL if `x` has not been set internally, regardless of presence in GET or POST params
Would make use of !x in run_sql completely safe:
SET !x = 'safe/value.sql';
SELECT 'dynamic' AS component, sqlpage.run_sql(!x) AS properties; -- safe because !x is NULL if not set internally
We could also warn on using sqlpage.run_sql with any of $x, :x, ?x, &x ...: "Potential use of user supplied value in run_sql"
It would also make comparison of computed var !x with user supplied param $x reveal if a param has been modified internally or not, or was even supplied by the user at all without introducing a placeholder variable with a distinct name to keep the original and computed values separate.
Introducing the new syntax (or equivalent) would obviously avoid breaking existing code.
Use of SET $x
could be discouraged by making it a warning: "Better to use SET &x;
or SET !x;
?
Syntax proposed by 3) is most important for security. Syntax proposed by 2) would be a more convenient way of writing COALECE(!x, $x)
in a SELECT and could also be the recommended way to reference parameters in general case as it behaves similar to how $x does already.
Hello and thank you for your thoughts! We cannot really use new and sqlpage-specific variable syntax, because we wouldn't be able to parse them with a standard sql parser, and because they already have their own semantics (!x
means NOT x
in MySQL, for instance).
I have another, slightly less ambitious but more realistic proposition:
SET $x
to match the one of SELECT $x
: set the post variable if it exists, otherwise set the GET variable. This removes the surprising and error-prone behavior where an user can override a variable by setting a POST parameter.$x
to reference a POST parameter. Warn the users that this post OR get behavior is deprecated.$x
, and clearly document the new, simple, behavior: :x
is a POST variable, $x
is a GET variable.Ah, I didn't realize $ and : where not already sqlpage specific. I mean I knew : is used for placeholders in prepared statements. I guess you have a thinner wrapper around pure SQL than I realized.
I see your point about !. Same would go for & too bitwise and or something.
I guess if we don't introduce any sqlpage specific we're stuck with overloading existing operators that won't mess up the parser.
I like your proposal, it would a big improvement. I think it still lacks ability to create variables that can only be set internally. Would you have any ideas for that could be done? I think it's not required if the dev just sets variables before using them. It would be nice if there was no room for error though...
Do you see a concrete case when reusing the same namespace as get variables would be problematic? I mean, the user being able to overwrite the programmer's variables like it is today is terrible, as you have highlighted already. But when this is fixed, I don't feel like we really need more.
I don't feel very strongly about either of these cases, but since you ask:
-- oops, I never set :inner, now user can dictate which sql is run or worse if sqlpage.exec
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
Case 1 can be hit without being detected if the var is unset only in rare cases, or if the error coming back from sqlpage.run_sql is squashed (not sure if that is possible).
Being unset (and so NULL) would potentially be invisible but could be exploited.
Consider some SQL (or function if db supports it) normalize
.
-- pollute the namespace with :x and :changed
SET ":x" = normalize(:var);
SET ":changed" = :x <> :var;
-- assign the normalized value back
SET ":var" = :x;
-- redirect only if URL changed after normalization
SET ":inner" = CASE :changed
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
It would be much nicer (in my opinion)
-- using my notation proposed above for consistency
SET "!var" = normalize(:var);
SET ":inner" = CASE "!var" <> :var
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
-- no need to assign back if references to $var later are like COALESCE(!var, :var, ?var)
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
NOTE: while typing this, I came up with a solution to the value swap using JSON, which is elegant enough for me. I'll probably use that moving forward unless there is a more elegant way to do it that you recommend.
Case 1 - unset var: not a huge risk.
Case 2 - value swap: it's a pain in all languages, but somehow seems particularly clunky in SQL...
If we had multiple return values I could do something like:
-- pollute the namespace with :changed
SET ":var", ":changed" = normalize(:var); -- postgresql pgSQL ext has SELECT INTO var1, var2, var3 ...
SET ":inner" = CASE ":changed"
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
Or I guess we can simulate multiple return values via JSON:
-- pollute the namespace with :ret
SET ":ret" = normalize(:var);
SET ":var" = :ret->>'var';
SET ":inner" = CASE ":ret"->>'changed';
WHEN 'true' THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
Even if we could declare methods SQLPage (client) side...
DECLARE normalize_and_maybe_redir(var) AS $$
-- local temp var so less namespace pollution (I guess inner scope sees :x)
SET ":x" = normalize(var);
-- assign the normalized value back (only for benefit of inner scope)
SET ":var" = :x;
-- redirect only if URL changed after normalization
SET ":inner" = CASE "!var" <> :var
WHEN TRUE THEN 'redirect.sql'
ELSE THEN 'empty.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
-- ideally could pass in :x instead of :var as an argument to run_sql
-- SELECT 'dynamic' AS component, sqlpage.run_sql(:inner, var=:x) AS properties;
RETURN :x;
$$ LANGUAGE sqlpage;
:var = normalize_and_maybe_redir(var);
:var2 = normalize_and_maybe_redir(var2);
... it still looks so clunky. I'm honestly not sure why. Am I missing something?
The JSON solution to the value swap is probably a decent solution and reads better than most of them, and doesn't require any changes.
@djyotta , can you test v0.22 ?
Yes, I will. I've been sick. Still recovering. Will do a test as soon as possible.
I wish you a prompt recovery!
Thanks @lovasoa . I'm feeling a bunch better already. But it must have been something nasty because it took 4 days before I felt like I could function normally again...
I did some tests. I used the following SQL to create a test form to GET and POST.
SET ":post" = sqlpage.variables('post');
SET ":get" = sqlpage.variables('get');
-- set GET variable
SET $x = 'get';
-- set POST variable
SET ":y" = 'post';
-- set POST variable
SELECT 'text' AS component, CASE COALESCE($x, '')
WHEN 'get-blah' THEN 'Fail - $x is overriden by GET param'
WHEN 'post-blah' THEN 'Fail - $x is overriden by POST param'
WHEN 'get' THEN 'Success - assigning $x worked'
WHEN '' THEN 'Nothing assigned to $x'
ELSE 'Fail - assigning $x failed for an unknown reason'
END AS contents;
SELECT 'text' AS component, CASE COALESCE(:y, '')
WHEN 'get-blah' THEN 'Fail - :y is overriden by GET param'
WHEN 'post-blah' THEN 'Fail - :y is overriden by POST param'
WHEN 'post' THEN 'Success - assigning :y worked'
WHEN '' THEN 'Nothing assigned to :y'
ELSE 'Fail - assigning :y failed for an unknown reason'
END AS contents;
SELECT 'debug' AS component;
SELECT $get AS "GET Params";
SELECT :post AS "POST Params";
SELECT 'table' AS component;
SELECT * FROM json_each(sqlpage.variables()) ORDER BY fullkey;
DROP TABLE IF EXISTS test_cases;
CREATE TEMPORARY TABLE test_cases(
id,
type text,
var text
);
INSERT INTO test_cases VALUES (1, 'get', 'x'), (2, 'get', 'y');
INSERT INTO test_cases VALUES (1, 'post', 'x'), (2, 'post', 'y');
SELECT 'button' AS component;
SELECT '/test.sql' AS link, 'Clear' AS title;
SELECT 'form' AS component, 'get' AS method;
SELECT 'submit' AS type, var AS name, 'Test GET Case '||id AS label, 'GET '||var||'=get-blah' AS value FROM test_cases WHERE type = 'get';
SELECT 'form' AS component, 'post' AS method;
SELECT 'submit' AS type, var AS name, 'Test POST Case '||id AS label, 'POST '||var||'=post-blah' AS value FROM test_cases WHERE type = 'post';
I observed the bug is present on v0.20.4 and fixed in 0.22.0
Test POST Case 1
fails on 0.20.4. The other cases are for completeness and are passing on both versions.I do wonder about the warning messages though.
In Test POST Case 1, I get Deprecation warning! $x was used to reference a form field value (a POST variable) instead of a URL parameter. This will stop working soon. Please use :x instead.
But that's because the POST has x=post-blah
and I'm using SET $x = 'get'
and referencing with $x
later as the intention is x is a GET param not a POST param. This means that a user sending parameters via POST instead of GET will cause the warning to log. The workaround would be to disallow POST on a GET endpoint which can be done with sqlpage.request_method
. Alternatively, directly referencing the GET param with ?x
instead of $x
would prevent the warning log from appearing too... but neither SET ?x
nor SELECT ?x
are working for me with SQLite in-memory db.
Otherwise testing seems ok. I've already put some of my production apps on v0.22.0 and they are working fine (but just lots of warnings as I haven't yet fixed my apps to reference :var
everywhere instead of $var
You would know better than me if it's behaving as you intended
Introduction
Given the following a POST is made with param
content=blah
to a sqlpage:Then the output is like this:
However, if the post param does not have the same name as the variable assigned:
Then the output is like this:
This is particularly dangerous when using variables internally to run_sql. If any of those variable names are POSTed then the attacker has full control over which sql is run.
Even this is not safe as the sql is run regardless of the where clause:
Aside from the security issue, it causes a headache when variable assignment is seemingly of none effect.
Note, this doesn't seem to affect GET , only POST
Version information
Additional context
Add any other context about the problem here.