Default behaviour Rails chose to adopt, going all-in on blocking raw SQL has not been pleasing many. I won’t go into to many details as it can be found here and all over the Internet
4 Likes
It’s so common to need this that there should be a shorthand for it that’s better than Arel.sql, if it exists at all.
4 Likes
Hah! I run into this one all the time at work, actually, because we have to write some very weird SQL sometimes.
There seems to be a lot of controversy around this issue, but maybe there’s something that can cut through the thicket.
I like @masterleep’s idea about finding a shorthand, just like we have h as an alias for html_safe in views. Or maybe there’s a way to make AR easier to extend so that Arel.sql is less necessary? Who knows!
2 Likes
This whole decision has always felt so backwards to me. All the warning does is encourage people to automatically wrap SQL strings in Arel.sql without really understanding why, which is no safer than just not having this, and needlessly verbose. The only alternative generally is to rely on ARel, which gets more opaque as time goes on (for which there’s a different WTF thread).
5 Likes
I feel like either a global setting or a proper unsafe function, like .unsafe_sort would partially solve this problem.
Or why not just have Rails automatically do it for you?
It’s easier than trying to wrap all queries in the system with Arel.sql.
No matter how this is handled, Rails should do that for you.
To be honest, unsafe_sort and the like has never felt right to me because it implies that ordering by an SQL function for instance is somehow “unsafe”.
Most people on this thread seem to also be on What has happened to Arel - #38 by tenderlove but just in case I want to link the two threads – there’s a lot of discussion about improving the AR/Arel relationship there, and that seems relevant to solving this problem.
1 Like
I mean, it’s only a suggestion. It doesn’t have to be that.
Sorry I wasn’t commenting on your suggestion specifically, just it’s something I’ve seen discussed in the github issue in the past.
This feels like the crux of the issue to me.
Can you talk more about what you mean when you say “Rails should do that?” I don’t think you mean to imply that Rails should be all-knowing and all-powerful, I think you probably mean something more specific, and I bet that if you can figure out how to articulate it it’ll help us crack this problem open.
2 Likes
Got it. Thanks. I was just clarifying. I know there has been extensive discussion there regarding this.
Of course.
Rails does many things “automagically” and assumes a lot of conventions for us. And this is one of the reasons it is such a great framework.
Why can’t Rails always wrap Arel.sql if it detects it is a “unsafe” sql vs a field/direction?
There are a lot of things we always need to use like LOWER() SQL function. What I am doing right now is wrapping queries myself, but that seems really ugly and counter-productive. There must be a more elegant way of Rails always “escape” in order to allow some function uses there.
Okay, that makes sense. I worry it would have weird unexpected behaviors in existing codebases, but I’m not sure whether that’s a data-backed concern or not.
Since you’re also active in What has happened to Arel - #8 by flanger001, this seems like it might be a good feature proposal to make to Rafael & Aaron either there or in the Basecamp project for coordinating “deprecation of public references to Arel.”
Detecting unsafe SQL is why it needs you to use Arel.sql – we can’t tell whether you intended to supply an arbitrary (and possibly injection-y) SQL injection, or it was instead a user-supplied value that you-the-developer thought it was safe to pass straight through as .order(params[:sort_by]), expecting only a column name to work.
There’s no “escaping” for us to do: the intended behaviour [once you opt in to it] is for us to inject the supplied string directly as SQL to be executed.
I asked there to be part of the Basecamp project.
Can you add me @tenderlove @rafaelfranca ? I would like to help.
I think that the reason for this is that they may consider something unsafe (by dint of it being a string) and it may be entirely benign (in my example, using lower to ensure that sort is human rather than pure lexicographic). But it could just as easily be something really “Little Bobby Tables” dangerous. If Rails were to wrap that up automagically, there’s no guarantee that Arel would find the exploit of the Mom, and defuse it. And then you’ve buried the very important warning. I can see the need for the warning here. It’s like wrapping the hammer handle with barbed wire so you’ll reach for the screwdriver instead.
Walter
Makes sense, @matthewd .
But I would like to be able to tell Rails it’s safe but not necessarily wrapping everything in Arel. So, one way to do that would be having a different method that would allow raw SQL to be executed.
2 Likes
Makes sense, @walterdavis.
The other alternative would be allow us to tell rails it is ok. That could be done with another method (alternative to order).
I’m confused because “tell Rails it is ok” is what Arel.sql is for. Is your objection specifically to the use of the word ‘Arel’?
@matthewd my objectification is to have wrap all functions. See discourse for example. They’ve got tons of custom queries. It would be much easier to replace order with some other function for example, then to have to wrap a bunch of things inside order.
Also, by creating some sort of flag, allowing the deactivation of this feature until people are able to catch up could be interesting. If you see this report I pointed on the topic, a bunch of people are relying on monkey patches to work around this.