Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Adds toRawSql() method on both Query and Eloquent builder #39551

Closed
wants to merge 4 commits into from

Conversation

Braunson
Copy link

@Braunson Braunson commented Nov 9, 2021

What does it do?

This PR adds toRawSql() both the Query and Eloquent builders which will output the raw SQL query with the bindings inline.

Does it break anything?

No, these a new methods that do not conflict with any existing methods.

Benefits

The benefit is logging complete queries instead of an array of the query and the bindings separately. This helper method returns a string with the bindings inserted into the query. Seems this is popular request on Twitter.

Tests

Tests are added for both Query and Eloquent builders

@Braunson Braunson changed the title Added toRawSql method on both Query and Eloquent builder [8.x] Adds toRawSql() method on both Query and Eloquent builder Nov 9, 2021
@Wulfheart
Copy link

Wulfheart commented Nov 9, 2021

Is this also going to work with the new compound Builder intererfaces in Laravel 9?

@mfn
Copy link
Contributor

mfn commented Nov 9, 2021

There was an attempt not long ago: #38027

As a postgres user myself, this is the first coming to my mind too from #38027 (comment) (not that there's a problem, but the question about if it's compatible):

Your code is missing edge cases because of PostgreSQL. In PostgreSQL there are multiple operators consisting of a questionmark (e.g. the jsonb operators). These parameters can be used in queries but the question mark needs to be escaped by another question mark. So the operator ? will be ?? and ?| will be ??|.

You need to replace questionmarks which have no leading or trailing question mark like i did here for tpetry/laravel-postgresql-enhanced. And null values should not be escaped because they are a special sql keyword. A condition like WHERE foo IS 'null' is not the same as WHERE foo IS null.

@taylorotwell
Copy link
Member

See comments from @mfn

@taylorotwell taylorotwell marked this pull request as draft November 9, 2021 20:20
@dennisprudlo
Copy link
Contributor

There is also the possibility that whereRaw, selectRaw or similar functions are used to build the query. In those you can specify params not only with a question mark but with a colon, like so:

Post::whereRaw('likes > :likecount and type = :type', ...)

May be the exception but certainly used as seen here:
#39541

@shadoWalker89
Copy link
Contributor

But this has a huge security risk, this allows for sql injections

Let's say we have a User model

Writing this

User::query()->where('username', "melek\' OR 1 = 1 #")->toRawSql()

Will generate this sql

select * from `users` where `username` = 'melek\\' OR 1 = 1 #'

@dennisprudlo
Copy link
Contributor

But this has a huge security risk, this allows for sql injections

I don't think it's intended to run the returned string as a query, so it's not that important that the query isn't safe.

The benefit is logging complete queries instead of an array of the query and the bindings separately.

When having a complex query with multiple joins, groups and orders the toSql() function doesn't help much if you want to see what actually gets executed.

@inxilpro
Copy link
Contributor

I think the security concerns could be mitigated by introducing this as dumpRawSql and dumping the string rather than returning it. That would minimize the likelihood that someone would use the generated string to actually query the database.

In our app we have a dumpSql() macro and dump_sql()/dd_sql() helpers that do this and use jdorn/sql-formatter to syntax highlight/indent the end result. We definitely use it all the time.

@tpetry
Copy link
Contributor

tpetry commented Nov 16, 2021

But this has a huge security risk, this allows for sql injections

The code could use PDO::quote to make it safe.

@Braunson
Copy link
Author

As @dennisprudlo pointed out, this isn't intended to be used to run the returned string as a query but instead as a development tool. If that is a concern, it can be mitigated (i.e. using @tpetry's suggestion or changing the method to something like ->ddRawSql() and dumping the query).

Now the whereRaw, selectRaw use-cases and postgres usage makes things a bit more complicated. I'll make some adjustments..

@tpetry
Copy link
Contributor

tpetry commented Nov 17, 2021

As @dennisprudlo pointed out, this isn't intended to be used to run the returned string as a query but instead as a development tool.

It's 2021, there is no reason to even build an sql string with sql injections for "development purposes". When not escaping the values because it's just dumping them, the developer still has to manually fix all the sql injections before being able to run the query. And it's not any problem making it safe for multiple years now. You can simply let pdo the quoting as is i did here and you'll get a safe query string. (Bonus point: It's also correct for the special PostgreSQL operators containing a question mark)

Named sql placeholders and obscure raw sql query parts are difficult to solve, and you'll get in a deep rabbit hole when trying to fix them. I simply omitted them. Because the following query could only be correctly transformed to a raw sql query by using a full sql parser (which would need to be specific for every rdbms too):

DB::table('test')->whereRaw("col1 = 'why are ''you'' doing this?' AND col2 = ?", ['value'])-> toRawSql();

@driesvints
Copy link
Member

Please mark this as ready for review if you want this to be reviewed again. Otherwise we'll be closing this PR soon and it'll be better if you resend this in when it's finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants