stringex
stringex copied to clipboard
rsl
acts_as_url can potentially load all records from a database
Consider the simple example:
class Article < ActiveRecord::Base
acts_as_url :title
end
If you try to do...
Article.new(title: nil).valid?
...it will load all of the records from the article table (see this condition: https://github.com/rsl/stringex/blob/master/lib/stringex/acts_as_url/adapter/base.rb#L118).
Additionally, if you did for example Article.new(title: "a").valid?, it will try to load all records that start with the letter 'a'. Both of these examples are very bad for large production databases, and unknowingly expose DOS exploits in your app.
I think the intention for doing a LIKE query and loading them with to_a is to avoid N queries when incrementing the number at the end of the slug to make it unique. Maybe it could be improved with pagination? I would think for an empty field though, that trying to create a unique slug is pointless for most use-cases (ie we have a validation on the presence of that field anyway). Even with pagination, I think the 'a' example above could still have potential for a very slow request as it still paginates through every 'a' record in the database. Maybe if we could specify an option like acts_as_url :title, if: :valid_title?? In which case the developer would impose some restrictions on the sluggable field.
Yes but even with a presence validation (or on length, etc), if a user tries to submit a form with an empty input, and you are relying on something like Article.create(params) on your server, despite it being an invalid record, acts_as_url will still try to generate that url and load all your rows.
Is this as simple as changing the callback_method to something other than :before_validation? This looks like an option, albeit undocumented, as far as I can tell.
I think before_save may be what I want here (assuming you don't use validations on the url, which you probably shouldn't since neither you nor the user has control over it).
i wonder if using pluck here might help. i think there's issues with it not being before_validation.
Pluck is definitely a huge improvement, although could still result in a few second query.
Interested to hear what issues you think there may be. I realize that doing it after validation means you can't validate the url itself directly, but I was going to work around this in the cases I needed it like so:
validate :url, if: -> model { model.url_changed? }
Generally you shouldn't need to validate it when it's auto-generated by acts_as_url.