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

"Dangerous query method" deprecation warning matches `random()` function call #32995

Closed
searls opened this Issue May 25, 2018 · 39 comments

Comments

Projects
None yet
@searls
Copy link
Contributor

searls commented May 25, 2018

Steps to reproduce

Post.all.order('random()')

Expected behavior

It should silently work, invoking postgres's random() function for determining order

Actual behavior

I get this warning:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "random()". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from …)

System configuration

Rails version: 5.2.0

Ruby version: 2.5.1p57

Using postgres

@utilum

This comment has been minimized.

Copy link
Contributor

utilum commented May 26, 2018

This is expected. It's trying to tell you to do like so: Post.all.order(Arel.sql('random()')).

See 310c3a8

@searls

This comment has been minimized.

Copy link
Contributor Author

searls commented May 29, 2018

I'm sorry if I'm being dense, but my reaction on this being closed was to feel dubious of what value Arel.sql('random()') is providing me as a user in this case.

  • If wrapping any potentially-unsafe thing in Arel.sql(…) is simply the equivalent of a --force confirmation to protect users from themselves, it seems contrary to Rails' ethos of preferring terse expression even if it means trusting people with sharp tools.

  • If instead the wrapping Arel.sql(…) call serves some technical purpose, couldn't the framework detect this and do it for users?

If I'm right in assuming it's the former, then this sort of change reminds me a bit of strong_params, except instead of coercing users to re-think mass data access, the most likely outcome here will be for people to get angry when Rails 6.0 breaks a bunch of queries and then blindly sprinkle in Arel.sql() calls all over the place to make the errors go away. And in that case, wouldn't the end result will be messier codebases without making queries more secure (prompting a Rails 7 Arel.no_really_sql(Arel.sql(…)) wrapper)?

I just stumbled upon this, so it's likely I'm off-base here, but this seems like a default I'd rather be hidden behind an opt-in config option for people who want to prevent queries like this to be generated conveniently.

@eileencodes

This comment has been minimized.

Copy link
Member

eileencodes commented May 29, 2018

cc/ @mastahyeti since you implemented the original implementation what are your thoughts on this issue? It does seem a bit heavy handed for users of Rails.

@eileencodes eileencodes reopened this May 29, 2018

@eileencodes eileencodes added this to the 6.0.0 milestone May 29, 2018

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 29, 2018

Is not the deprecation message making users to re-think SQL injection by telling them to review the query to see if there is user input on it and if not call Arel.sql?

This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql().

That is the reason for this change. If users are blindly calling Arel.sql, well, that is where the provide sharp tools enters, it is their call.

I don't believe this should be opt-in. I strong believe we should not allow people to opt-in security because most likely they will not. Opt-out is an option, if you want to deal with this yourself I think you should be allowed. The good news is: it is already possible to opt-out using allow_unsafe_raw_sql. (it is missing documentation and I'll fix this right now).

If the message is not making users to re-think SQL injection, so we should improve it.

In summary, in my opinion, the current implementation we have is the one that maps better what we need. We need people to rethink SQL injection by not allowing untrusted values in methods that are possible to inject SQL.

@mastahyeti

This comment has been minimized.

Copy link
Contributor

mastahyeti commented May 29, 2018

Hi @searls. The issue #27947 was intended to address was

Post.order(params[:order])

which is a common pattern in Rails app, unfortunately leading to SQL injection.

... it seems contrary to Rails' ethos of preferring terse expression even if it means trusting people with sharp tools.

I'm not sure this has been my experience of the Rails ethos when it comes to security. While Rails doesn't have the best history of being "secure by default", some effort is made to protect users from themselves. Think html_safe.

The protections added in #27947 will likely add some friction for some users, but I don't think it will be so dire. The #pluck and #order APIs are usually passed column names. The decision was made to draw the line at allowing column_name or table_name.column_name, but I can see allowing some additional whitelist of "safe" functions if this becomes too much of a burden.

And in that case, wouldn't the end result will be messier codebases without making queries more secure (prompting a Rails 7 Arel.no_really_sql(Arel.sql(…)) wrapper)?

As someone who works on securing a large Rails app, I can tell you that having to audit for .order(Arel.sql(...)) calls is much easier than having to audit all .order(...) calls. It has been my experience that developers rarely have to pass Arel.sql(...) to these APIs because they rarely are wanting to pass anything other than a column name.

@searls

This comment has been minimized.

Copy link
Contributor Author

searls commented May 29, 2018

Thanks, @mastahyeti, for the added context. Knowing this applies just to a handful of Arel's methods is definitely helpful and makes this seem less dire of a concern than if it applied to any string passed into any of them.

I'm also glad to hear you'd be open to a whitelist of common functions, as I think you'd pretty quickly only see this error in pretty unusual circumstances.

[I'm curious, too, whether there's some other heuristic (a regex?) in any of these cases that could (performantly enough) establish confidence that nothing harmful could have been injected. But this is a well worn problem and I'm a security noob, so I'm sure that might be a silly suggestion.]

@jmckible jmckible referenced this issue May 30, 2018

Merged

Rails 5.2 #101

@al2o3cr

This comment has been minimized.

Copy link
Contributor

al2o3cr commented Jun 1, 2018

@searls regarding a regex, IIRC the core team's position is "don't write a SQL parser" :)

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Jun 6, 2018

I would like to echo my pain here upgrading Discourse to Rails 5.2. which is in progress

discourse/discourse@5373e84 (link will die at some point)

I would strongly prefer if we could just turn off this feature for Discourse.

Especially since I now have this kind of crazy.

 sent.where("created_at BETWEEN ? AND ?", start_date, end_date)
       .group("DATE(created_at)")
-      .order("DATE(created_at)")
+      .order(Arel.sql "DATE(created_at)")
       .count

This is very frustrating on a couple of counts

  1. I need to now train all our devs to remember that #pluck and #order are special snowflakes

  2. We are now allocating one more object (and one extra method call) unless we pull these Arel.sql magic things into a const or something

Would Rails core be open to making this new restriction configurable?

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Jun 6, 2018

@rafaelfranca reading through the source I am not seeing how this is optional?

It appears allow_unsafe_raw_sql can not be set to true for a bypass, it only checks if it is :deprecated or not

Are you open to a PR that allows this to be set to true and then we add a bypass prior to calling enforce_raw_sql_whitelist?

This also helps us cause we get to cut down on the whitelist match stuff for a very minor speed bump.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jun 6, 2018

I think we should allow it. I'm happy with PR.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 6, 2018

I can help with the asymmetry: group should almost certainly apply this rule too in a future version. where is a conscious exclusion, but others may still need consideration.

In your commit you seem to have added Arel.sql to a number of order calls that don't require it: what we're "--forcing" here is that a method which is generally passed a column name, is intentionally instead being passed a more complex SQL fragment. (e.g. 5 of 7 instances in post.rb)

We originally considered allowing allow_unsafe_raw_sql to disable the check entirely, but ultimately decided against it because the warning already provides a 'disabled' mode in the sense of "still works like 5.1"; a "works without complaining" would be unhelpful long term, because like all upgrade tools this setting will eventually go away -- we don't want to carry an opt-out conditional forever. Global settings also make life more complicated for gem authors, for example.

Shaving a flat one or two short-lived-object allocations off a full "build a query, run it, wait for the network, load the results" cycle doesn't seem performantly exciting to me. If Ruby had a way to identify a shared/static string, it'd be neat to cache their SQL-node forms, but I know of no such thing.

These methods are already pretty snowflaky: they mostly look like they take a column name, but sometimes they take an SQL fragment, but not if you want ? placeholders, but there's actually a special spelling to do that too.

If you really want to kill it you can presumably monkey-patch enforce_raw_sql_whitelist to a no-op, but I'm having a hard time buying a "developer ergonomics should trump a security/safety mechanism" argument the same week as "performance should trump developer ergonomics".

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Jun 6, 2018

I'm having a hard time buying a "developer ergonomics should trump a security/safety mechanism" argument the same week as "performance should trump developer ergonomics".

This is an unfair stab, especially since I never said that, all I said is that I wish performance was somehow mentioned in the Rails doctrine, even in a "Kaizen" form where we strive to make Rails faster every release. A faster Rails encourages developers to upgrade to the latest version. It encourages people to say "yes" to features so it overall "improves developer happiness".

Back to the issue at hand, I am monkey patching out the method (I just did that in a commit to Discourse) I will consider moving to the Rails-way here if you can demonstrate in a file or 2 how this does not cause the code to suffer and become ugly. The training vector really needs consideration here especially around the disparity with other methods.

Overall a custom whitelist may be a way to unblock us here cause we could then explicitly whitelist some of this stuff. Then use something like LRURedux to bypass this evaluation for strings we know are good or something like that. Especially if stuff like order('something ?', something) automatically whitelists. I am concerned about running every order string via 100 regexes long term but there may be optimization.

A big concern here is that the "escape hatch" proposed in the deprecation message is not ideal. You see it enough times on upgrade and you take an atomic approach to adding it even where it is not needed. My screen filled up with add Arel.sql so I just blanket added it everywhere to make the spec suite stop nagging. If I know that date({column}) is always going to be good I should be able to just blanket allow it.

In other news Discourse does not appear to be any slower post the 5.2 upgrade (performance seems on par with the bench) so 🎊

@ignisf

This comment has been minimized.

Copy link

ignisf commented Jun 6, 2018

Hello,

So the assumption here is that there's widespread use of Model.order(<unsanitised params>). Could we not target the input of unsanitised params directly instead of assuming any string literal passed here is unsafe?

This would allow for a more precise warning to the users, too, or even an exception, stating that this invocation creates a vulnerability and is thus disallowed (see Model.where(params)).

Thinking about this, it leads me to a more general solution that allows param values to be marked as unsanitised?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 6, 2018

@ignisf the assumption is that it's a sufficiently widespread problem (and more generally, non-obvious danger in an API that looks like it's taking a column name) that someone made a website about it.

The stronger assumption, though, is that most calls to order are passing a simple column reference; much of the discussion in #27947 was around how to minimize the number of callers that would be affected by the warning.

It sounds like you're getting close to inventing string tainting, which has its own issues.

This case isn't really about rejecting user input, though -- it's about choosing which of two "identical"-looking APIs you intended to use.

A big concern here is that the "escape hatch" proposed in the deprecation message is not ideal. You see it enough times on upgrade and you take an atomic approach to adding it even where it is not needed.

I'm really not sure how much we can do about this. If it was as simple as making the change everywhere, we would've done it for you; the reason we're flooding your output with pointers to each individual call is that we want you to consider them individually.

If you want to alias rm="rm -f --no-preserve-root" the first time you actually need to rm /, you're free to do that... but I think most people will benefit from only disabling the safety when they need the unconstrained power.

Granted, Discourse has 5 instances of order-by-date... but those actually seem to have enough more in common (.where("<column> BETWEEN ? AND ?", x, y) .group("date(<column>)") .order("date(<column>)")) that they'd benefit more from a method extraction than a custom regexp.

The others all seem pretty impractical to define general rules for.

The code is expected to become incrementally uglier in places where it's already uglyish (SQL inside strings); I can't offer an example where it makes things prettier. The argument is only that it happens rarely enough that the safety trade-off is worth it.

an unfair stab

Quelle horreur. 😒

The Doctrine is David's description of his design philosophy. It also doesn't say anything about fixing bugs, for example. Most importantly, while its sentiment is weighed in considering how we present things to our users, it has no influence on how our volunteers spend their uncompensated free time.

@janko

This comment has been minimized.

Copy link
Contributor

janko commented Jun 6, 2018

FWIW, Sequel introduced the same change in version 5.0. You now need to explicitly mark the SQL string as a "literal" (just like Arel.sql):

Post.order(Sequel.lit("random()"))

I consider this to be a positive change, so I support the same changes in Active Record. Jeremy Evans argued that, in addition to being secure by default, this change also makes security audits for SQL injection easier, because you can grep for Sequel.lit (in this case Arel.sql).

Sequel also provides an extension to disable this behaviour, and from what I understand it will be supported indefinitely. It would be good if Active Record would offer the same indefinitely, though I understand the wish to remove it at some point. It could be a module (a "concern", if you will) that gets included when the user enables the old functionality; that way these conditionals will be separated from the main code (that's how Sequel does it).

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Jun 6, 2018

I'm really not sure how much we can do about this. If it was as simple as making the change everywhere, we would've done it for you; the reason we're flooding your output with pointers to each individual call is that we want you to consider them individually.

Oh, one simple change that we could make here is only alerting in STDERR the first time the call site is detected, a very big pain here was that the same line (which was covered many times) kept on showing up over and over again on one test run.

The argument is only that it happens rarely enough that the safety trade-off is worth it.

It could be that I was a lot biased by having to through the entire app in one go, its just a lot of changes and we use SQL a lot.

So to summarize

  1. Alert only on first call site now to ease upgrade
  2. Enforce this more consistently group, joins and others so it correctly protects
  3. Consider perhaps a diff syntax for whitelisting

The idea around 3 would possibly be allowing:

.order('count(*) desc')
.safe

or maybe:

.unsafe_order(

not sure.

note: I was super careful not to mention string tainting, cause this is very much a feature I want erased from Ruby

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 6, 2018

The logspam from a single frequently-hit deprecation is a fair point, though we should address it generally in the deprecation reporter. Personally, I think it feels really good when I make a single change and a few thousand warnings go away, but to each his own 😄

#27947 started at unsafe_order; Arel.sql felt more like HTML-escaping, and is consistent with the fact that everything ultimately passes through Arel anyway. (And correspondingly avoids us having to pass (value, safeness) pairs around everywhere internally.)

Are you trying to avoid the allocation here, or is it an aesthetic concern?

(Yeah, I only mention tainting as a self-evident explanation that that path is impractical.)

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Jun 6, 2018

Are you trying to avoid the allocation here, or is it an aesthetic concern?

I know it is mega weird coming from the "performance guy" but yeah this is mostly and aesthetic/symmetry concern, the extra single digit RVALUEs are not the end of the world.

@mastahyeti

This comment has been minimized.

Copy link
Contributor

mastahyeti commented Jun 6, 2018

I still have a preference for unsafe_raw_sql_order(). I think it's harder for a developer to type that method name without second guessing their approach.

kamipo added a commit to kamipo/rails that referenced this issue Jul 10, 2018

Allow frozen string literal as a safe raw SQL string
rails#27947 was intended to address `Post.order(params[:order])` which
leading to SQL injection.

But the protection caused very annoying warnings, especially for calling
by a string literal which invoke a function like `Post.order("length(title)")`.

Calling by a string literal should be safe since it is not an user
input. So I'd like to allow string literals as a safe raw SQL string.

There is no reliable way to distinguish between`params[:order]` and
`"length(title)"`, but at least `params[:order]` as an user input is
always a mutable string.

So I think that we can add frozen strings/objects to the whitelist to
mitigate the annoying warnings without losing the original intention of
rails#27947.

Resolves rails#32995.
@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jul 10, 2018

How about allowing frozen strings/objects to the whitelist since an user input is always a mutable string?
I've proposed the PR #33330.

@andynu

This comment has been minimized.

Copy link

andynu commented Sep 6, 2018

Override methods like unsafe_order and unsafe_group would be wonderful, and consistent with the security by default, but with a way around approach used with strong params and to_unsafe_h. Or a config to disable this safety check entirely.

For those of us that build atop legacy third party databases or have a need of simple functions it seems preferable to have a rails api method for those needs rather than having to jump over to the Arel api.

@mastahyeti

This comment has been minimized.

Copy link
Contributor

mastahyeti commented Sep 6, 2018

Naming methods safe_<dangeroug_thing>, especially when they just bypass a security feature, seems like not a great pattern 😆

@andynu

This comment has been minimized.

Copy link

andynu commented Sep 6, 2018

Agreed! That raises a similar point about readability of order(Arel.sql(...)) vs. unsafe_order(...).

The latter more clearly expresses a need for caution to future readers of the code.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 6, 2018

it seems preferable to have a rails api method for those needs rather than having to jump over to the Arel api

That seems a bit of an arbitrary distinction: there is an API, and it happens to include the word "Arel". Could you help me understand what about that word makes it a bad name?

The latter more clearly expresses a need for caution to future readers of the code.

Disagree: the former expresses that the argument is SQL; the latter gives a vague "here be dragons".

@andynu

This comment has been minimized.

Copy link

andynu commented Sep 6, 2018

In my experience the ActiveRecord query interface services almost every querying need. Arel is lower level api. It isn't mentioned at all in the query guide. Until now users of the ActiveRecord api could be blissfully unaware Arel. This strikes me as jumping across levels of abstraction. I readily concede that we're talking about advanced use here, so maybe that's fine. The second point seems more important to me.

I agree that the the current form of order(Arel.sql(...)) express that it is SQL, but saying what it is doesn't express that using SQL is unusual and may be dangerous. The point of the original change is to warn people of the dangers of unsafe input in SQL. As @mastahyeti says above, typing the word unsafe should make developers think twice, and having that word there is more explicit about it being unusual for future readers. This seems similar to to the conventions of method vs method!, if there is a ! the reader is takes a moment to consider the more dangerous form. Same in the existing api with params.to_h vs params.to_unsafe_h. And I believe same here with order vs unsafe_order. "here may be dragons" seems exactly right.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 6, 2018

The intended analogy is to h() (as in "HTML"). "Unsafe" is in the eye of the beholder; to me the only aim is for the caller to say "this thing I am passing, I intend to be interpreted as SQL" -- because most of the time it's just a column name, and therefore a reasonable caller could pass an unchecked value while believing that the only thing that'll be accepted is a column name. "You should only rarely need to pass raw SQL, maybe reconsider your need" is moving from "we need to understand/confirm the caller's intent" to "we want to influence the caller's design decisions"... we don't not do that, but I think requiring explicitness is a better line of trade-off than making people lecture themselves each time they want to use the thing. (See a certain minitest method.)

to_unsafe_h is a bit different: there, what's unsafe is the thing that you get back. It's not "unsafely give me a hash", it's "give me an unsafe hash".

If people think order_sql(...) as an alias for order(Arel.sql(...)) would improve the ergonomics, that doesn't seem impossibly bad... but there was some reasoning behind using Arel.sql:

  1. it's the same thing you can use in other interesting places,
  2. it's short [without polluting any global namespaces],
  3. it's an initial tentative step towards a gentler boundary between AR and Arel,
  4. because it provides an extra level of argument wrapping, it can help with parameter binding, so you in turn need to do less manual SQL manipulation,
  5. while pluck is more constrained, there's a whole family of order methods that'd need _sql variants, which seem they'd be ugly to document, learn, and switch between.
@qrush

This comment has been minimized.

Copy link
Contributor

qrush commented Sep 6, 2018

I would much prefer a order! or pluck! that explicitly allows unsafe SQL. Some of that "unsafe" SQL for our app is very reasonable and 100% "safe".

Here's some snippets using pluck / order as noted above from our codebase that do emit errors on Rails 5.2.1. All of these ARE safe and not from user-input, but we'd have to wrap a lot of this code in Arel.sql instead, making it harder to read and understand.

# complex COUNT's
.pluck("COUNT(*) FILTER (WHERE start_time > '#{28.days.ago}')")

# MAX across a join
.pluck(:exercise_id, 'MAX(chat_session_exercises.teacher_rating)')

# jsonb access
.pluck("context->'example'")

# Just a simple LOWER
.order("LOWER(word) ASC")

# Date functions...
.order("DATE_TRUNC('second',user_achievements.updated_at) ASC, user_achievements.key ASC")

# Not even a comparsion or NULLS FIRST?
.order("user_media_sources.rating <= 2 DESC NULLS FIRST")

I think the unstated goal here to push people towards writing only Ruby instead of SQL snippets interspersed with their Ruby. While that's a nice goal, I'm confident that is not how a lot of apps get written. Also IMO, writing complex SQL functions is not easy or approachable currently with Arel. ActiveRecord for years has let developers get the most of out data abstractions by keeping the "under the hood" SQL just hidden enough. This feels like a step in the opposite (or at least different) direction that directly impedes with that goal.

I can imagine the explosion of warnings from most apps that actually use SQL this way will impede upgrades to Rails 5.2 and beyond. I'd suggest the following as a course of action if Rails wants to continue down this path:

  1. Make this an opt-in behavior if possible (perhaps I missed that memo?)
  2. Remove the warning for the next Rails point release
  3. Introduce new "unsafe" method
  4. Start warning for 6.0 or outright pushing people towards the new "unsafe" method

🙇

@andynu

This comment has been minimized.

Copy link

andynu commented Sep 6, 2018

@matthewd Thank you for explaining the intent. Point taken about the unsafeness of the param vs the result and I didn't know that there was an intention to soften the boundary between ActiveRecord and Arel. I still have a preference for order_sql and like @qrush's suggested plan.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 7, 2018

I think the unstated goal here to push people towards writing only Ruby instead of SQL snippets interspersed with their Ruby.

Absolutely not, and given neither the PR author nor any member of the Rails team has suggested that over ~150 comments, I think it'd be easier if we discuss the actual (and conveniently, stated) goals. Being able to easily drop to SQL is a core feature of the Active Record API; having seemingly-insignificant parameters surprisingly interpreted as SQL, not so much.

Some of that "unsafe" SQL for our app is very reasonable and 100% "safe".

Yep, and that's one of the reasons I'm pushing against (and the API currently avoids) using the word "unsafe" here. It's not unsafe; it is SQL.


To summarize the recent suggestions:

# now shows warning
.pluck("context->'example'")

# adding one character: acceptable
.pluck!("context->'example'")

# adding four characters: maybe
.pluck_sql("context->'example'")

# adding eight characters: so bad it prevents upgrades
.pluck(Arel.sql "context->'example'")

Another advantage of Arel.sql is in wrappers. Given:

def my_pluck(something)
  pluck(:id, something).to_h
end

With Arel.sql, you can keep using that method, and pass it either :email or Arel.sql("first_name || ' ' || last_name"). With a pluck_sql, you'd either have to change the above method to always use it, thereby exposing all of its callers to the potential danger of having a seemingly-column-name argument interpreted as SQL, or define a parallel my_pluck_sql.

@KelseyDH

This comment has been minimized.

Copy link

KelseyDH commented Sep 7, 2018

My ActiveRecord query below is triggering this deprecation warning on Rails 5.2:

INBOX = "Uncategorized"

.order ActiveRecord::Base.send(:sanitize_sql_array, ['tracker_categories.name = ? DESC, position ASC', INBOX])

which I find a bit crazy since it's clearly not a SQL injection risk, and I'm a bit lost how to go about rewriting this to make it compliant without needing to throw in horrendously ugly calls to Arel.sql. (If there is a better Railsy way of writing this .order query, I'm all ears.)

jfly added a commit to jfly/worldcubeassociation.org that referenced this issue Sep 12, 2018

jfly added a commit to jfly/worldcubeassociation.org that referenced this issue Sep 16, 2018

Fix "dangerous query method" deprecation warning.
See rails/rails#32995.

I introduced an explicit "psych sheet" type. This approach reduces the
number of @ variables we need to create in our
controller, which I think cleans things up nicely.

jfly added a commit to thewca/worldcubeassociation.org that referenced this issue Sep 16, 2018

Fix "dangerous query method" deprecation warning.
See rails/rails#32995.

I introduced an explicit "psych sheet" type. This approach reduces the
number of @ variables we need to create in our
controller, which I think cleans things up nicely.

kamipo added a commit to kamipo/rails that referenced this issue Sep 28, 2018

Allow frozen string literal as a safe raw SQL string
rails#27947 was intended to address `Post.order(params[:order])` which
leading to SQL injection.

But the protection caused very annoying warnings, especially for calling
by a string literal which invoke a function like `Post.order("length(title)")`.

Calling by a string literal should be safe since it is not an user
input. So I'd like to allow string literals as a safe raw SQL string.

There is no reliable way to distinguish between`params[:order]` and
`"length(title)"`, but at least `params[:order]` as an user input is
always a mutable string.

So I think that we can add frozen strings/objects to the whitelist to
mitigate the annoying warnings without losing the original intention of
rails#27947.

Resolves rails#32995.

va7map added a commit to sfu/canvas-lms that referenced this issue Nov 1, 2018

Fix SQL enforcement issue in Copyright Survey API
The use of non-attribute arguments (in this case, calling the `RANDOM()`
function) in a query is deprecated in Rails 5.2 and disallowed in 6.0.
Additionally, Canvas is enforcing this new behaviour early in Rails 5.1.

Related commit: instructure@e8f0be2

The workaround is to wrap known safe values in `Arel.sql()` before
passing it into `order()`. We know it is safe in this case because the
value does not depend on user inputs.

See also: rails/rails#32995

Test plan:

1. Call the Copyright Survey API as usual
2. Verify that it does not fail with a 500 Internal Server Error

lgebhardt added a commit to cerebris/jsonapi-resources that referenced this issue Nov 7, 2018

Add Arel.sql to pluck fields
Needed to avoid "DEPRECATION WARNING: Dangerous query method..."
See rails/rails#32995

@lgebhardt lgebhardt referenced this issue Nov 7, 2018

Merged

Wrap pluck fields with Arel.sql #1190

0 of 12 tasks complete
@codeodor

This comment has been minimized.

Copy link
Contributor

codeodor commented Dec 9, 2018

I'm interested in this one too. Trying to upgrade to 5.2 I noticed it, and now I've updated all our pluck methods that use function calls to use Arel.sql('funct()') instead.

I guess we need to do order too? Anything else?

@codeodor

This comment has been minimized.

Copy link
Contributor

codeodor commented Dec 9, 2018

In particular, do we always need to do this with a string?

It's easy enough to convert simple attributes to a symbol to be sure we don't trigger it there, but what about when we're ordering by attributes in multiple tables: e.g., order('table1.col1, table2.col2')?

Should that be done using Arel.sql as well, or only function calls?

@codeodor

This comment has been minimized.

Copy link
Contributor

codeodor commented Dec 9, 2018

I'm also incredibly scared to upgrade to 6.0 when it becomes available. This is a massive breaking change to simple stuff that we might not often test against. We're not typically shooting for 100% coverage in every place we might use order or pluck, so it's going to be very costly to upgrade to 6.0 safely.

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Dec 9, 2018

@codeodor I would not carry some sort of fear, Discourse are going to have to solve this problem some way or other and have a public code base, at the moment we monkey patch this out. We are going to upgrade to 6.0 very shortly post release.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 11, 2018

After reviewing each comment on this tread I don't see any reason why we should change the current behavior.

The reasoning for this change was made clear and I don't think anyone disagrees with the reasons why we require SQL literals to be wrapped on Arel.sql calls for those methods.

We are now discussing if Arel.sql is a "pretty" call or not. I'd say this is matter of test and while we are open to revisit this API I don't think it is productive to be bikesheding over the name given all suggestion where already considered and the core team still thinks the current API makes sense.

Others ORMs like Sequel also made similar changes with similar API so I think more people thinks this is a good idea.

About being safe to upgrade or not: this show deprecation warnings if you are not using the current API, so even if you don't have test coverage for your entire application it should be possible to use the deprecations to find where the code needs to change. This is not specific for this change though. Any API change would cause this fear, so I don't think this is an argument to remove this protection.

Thank you all for the discussion.

@andrey-skat

This comment has been minimized.

Copy link

andrey-skat commented Jan 10, 2019

I'm a bit too late, but I think, that main reason - protect novices - is fail here because nothing stops juniors to mindlessly write Arel.sql just to get things done. Moreover, they may think that that method can magically make their code safe :)
Usually that kind of checks make not ORM code, but linters/static analyzers. Also, sanitize raw SQL - it's fundamental web development knowledge, not only for rails.

About naming. order! looks like it can throw exception (because it's Rails convention)

kamipo added a commit to kamipo/rails that referenced this issue Feb 5, 2019

Allow frozen string literal as a safe raw SQL string
rails#27947 was intended to address `Post.order(params[:order])` which
leading to SQL injection.

But the protection caused very annoying warnings, especially for calling
by a string literal which invoke a function like `Post.order("length(title)")`.

Calling by a string literal should be safe since it is not an user
input. So I'd like to allow string literals as a safe raw SQL string.

There is no reliable way to distinguish between`params[:order]` and
`"length(title)"`, but at least `params[:order]` as an user input is
always a mutable string.

So I think that we can add frozen strings/objects to the whitelist to
mitigate the annoying warnings without losing the original intention of
rails#27947.

Resolves rails#32995.

kamipo added a commit to kamipo/rails that referenced this issue Feb 5, 2019

Allow frozen string literal as a safe raw SQL string
rails#27947 was intended to address `Post.order(params[:order])` which
leading to SQL injection.

But the protection caused very annoying warnings, especially for calling
by a string literal which invoke a function like `Post.order("length(title)")`.

Calling by a string literal should be safe since it is not an user
input. So I'd like to allow string literals as a safe raw SQL string.

There is no reliable way to distinguish between`params[:order]` and
`"length(title)"`, but at least `params[:order]` as an user input is
always a mutable string.

So I think that we can add frozen strings/objects to the whitelist to
mitigate the annoying warnings without losing the original intention of
rails#27947.

Resolves rails#32995.
@jeremywadsack

This comment has been minimized.

Copy link
Contributor

jeremywadsack commented Feb 8, 2019

Would it be helpful to add documentation on Arel.sql? Right now there's nothing to indicate what applying this is doing.

I also think that all the example (e.g. in Ruby Guides on ActiveRecord Query Interface) should be updated to reflect this, along with a discussion about what Arel.sql is doing. (That it's marking something as reviewed-and-safe, not doing anything to make it safe.)

@LucasArruda

This comment has been minimized.

Copy link

LucasArruda commented Mar 15, 2019

Allowing frozen string literal, as here ( kamipo@fe120e2 ) seems to be the perfect solution to this problem, if we don't want to automatically wrap unsafe raw SQL into Arel, which would be how users expected Rails to behave.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.