improve macros security

[belamov]

currently macros are registered like that:

Builder::macro(
    "where{$operatorName}",
    fn ($left, $right) => $this->whereRaw(
        sprintf('%s %s %s',
            $left instanceof Range ? $left->forSql() : $left,
            $operator,
            $right instanceof Range ? $right->forSql() : $right
        )
    )
);

https://github.com/belamov/postgres-range/blob/master/src/Macros/QueryBuilderMacros.php#L25

it is not sanitized in any way, so there's potential sql injection scenario, if you're using macros with unvalidated parameters

the better approach is to use PDO's bindings, like so:

Builder::macro(
    "where{$operatorName}",
    fn ($left, $right) => $this->whereRaw(
        "? $operator ?", [
            $left instanceof Range ? $left->forSql() : $left,
            $right instanceof Range ? $right->forSql() : $right 
        ]            
    )
);

or

Builder::macro(
    "where{$operatorName}",
    fn ($left, $right) => $this->where(
            $left instanceof Range ? $left->forSql() : $left,
            $operator
            $right instanceof Range ? $right->forSql() : $right             
    )
);

macros are tested like this:

$query = "where$macroMethod";
$items = Range::$query($columnName, $rangeObject)->get();

https://github.com/belamov/postgres-range/blob/master/tests/Feature/MacrosTest.php#L59

with current implementation there is no errors and every macro is working as expected

but with whereRaw and bindings approach we get sql generated like so:

select * from "ranges" where timestamp_range @> '[2020-04-24 06:52:19,2020-04-25 06:52:19)'::tsrange

and Ambiguous function: 7 ERROR:

operator is not unique: unknown @> unknown

and with where approach we get sql generated like so:

select * from "ranges" where "timestamp_range" @> '[2020-04-24 06:48:52,2020-04-25 06:48:52)'::tsrange

and Invalid text representation: 7 ERROR:

malformed range literal: "'[2020-04-24 06:48:52,2020-04-25 06:48:52)'::tsrange"

in both cases range is transformed as expected and if we run this generated queries with sql, there is no errors. so my guess is that somewhere in value binding to pdo statement this parameters are transformed in some way that pdo driver is not correctly casting types

would be great if somebody know what's the issue is and can help me out with this

[resohead]

I have an idea to prevent SQL injection.

In the macro builder:

• wrap $left (column name) in double quotes
• convert $right (range) to a Belamov\PostgresRange\Ranges\DateRange object if a string was passed.

For example,

$rangeString = '[2020-04-01,2020-05-01)';
Range::whereRangeOverlaps($columnName, $rangeString);

Macro builder uses the Range class to parse the string into an object.

use Belamov\PostgresRange\Ranges\DateRange;

// current = create from range arguments
$range = new DateRange('2010-01-10', '2010-01-15', '[', ')');

// or create from string with only 1 argument
$range = new DateRange('[2010-01-10,2010-01-15)');

// or using a parse method
$range = new DateRange::parse('[2010-01-10, 2010-01-15)');

The macro builder can then call ->toSql() and SQL injection attemps will not parse into a range object and throw an exception.

[belamov]

I don't really want to restrict macros api to accept only range objects. and there is additional problem with your approach - those macros (and range operators) are not for date ranges only. there is multiple range types, how would macro know the format that this string should be parsed into? what if we want query integer range overlapping? or numrange

but manually checking params before adding them to query builder is a good idea, much better than nothing, thanks. will dig into this deeper

[winkbrace]

I'm not exactly sure if this will help, but this works using PDO:

create table tmp (numbers int4range);

insert into tmp values ('[3, 15]');

select * from tmp
where numbers @> :numbers::int4range; 

$pdo = DB::connection()->getPdo();

$query = $pdo->prepare("select * from public.tmp where numbers @> :numbers::int4range");
$query->execute(['numbers' => '[3,4)']);

dd($query->fetchAll());