-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Database limits no longer set in schema #1080
Comments
It's an issue for slugs because historically, there's a 255 maximum length but it's not covered by the validations (only a min, no max) so if it's exceeded you get an Error 500 with ActiveRecord::StatementInvalid. I was looking into updating the validations to fix the issue and I noticed the same thing as you : limit is no longer set for newer apps. I'd say it's better to have limits than not because without them, you get "undefined behavior" depending on your database engine. |
I would prefer to have limits in the schema instead of using application level constraints if possible. We could add application validations where they make sense, but I dont think we should use them to solve the database level issues with limits. With respect to the mysql concern, setting strict sql mode will make all warnings become errors, but that has its own draw back (this is the default when running migrations though). |
@Senjai Thanks for the mysql tip, I have been using pg for a while now so haven't come across this before. Does this sound like a reasonable plan:
|
Thanks a lot for the writeup, this is a pretty tricky issue and the clarity is very much appreciated. I'd prefer to do a new migration adding lengths rather than editing the existing migrations. I'm worried about the states different stores are going to end up in. Adding a single migration to ensure everything is in the right state seem like the right way to go for me. Of course we'll need to decide what that "right state" is and what limits we should have everywhere. I'm not a big fan on the DbMaximumLengthValidator (Assuming we're talking about the one that was added into spree 3), but more on principal than objecting to the implementation. |
I am also 👍 on one migration to make everything "correct" but also agree on the concerns around that. I'd be 👎 on the maximum length validator approach, just because I'd want to ensure consistency at the database layer instead of in the application layer. |
@cbrunsdon Yep, makes sense to apply it as a new migration but we will need to come up with a solution for all of the records that will become invalid for existing app's. @Senjai I may be mistaken but |
I'd like to throw this in as a potential way to set the validations. It will use the limit from the database if one exists, or fall back to the hardcoded value if nothing is set. module Spree
class Address < Spree::Base
validates :firstname, length: { maximum: columns_hash["firstname"].limit || 100 }
end
end |
@cbrunsdon looks like a good solution, should this only be on the user entered fields for now? e.g. addresses |
@cbrunsdon I was looking at |
@alex-handley ha, that was my thinking too. I prefer my inline to using the @jhawthorn opinion? |
@cbrunsdon The only feature I like about |
Should a limit be added to every string column or only to a subset of those fields like names and slugs ? |
@dangerdogz I am only going to add to user facing fields. I should have the migrate ready tomorrow for some feedback. |
Morning, this is my first attempt at creating the migration: I am going to create a db dump from our prod and see if I encounter any issues. |
@alex-handley we're going to need some kind of check/guard/something at the top of this migration to do a sql max() length of the fields to ensure we don't accidentally truncate anyones data. |
Hey, just to check in I have been pretty busy the last few weeks so only just got round to finishing this. Has anyone come across this before? |
Maybe this is why something like DbMaximumLengthValidator could be useful? Unless there's a better way, I think we would need to check that columns_hash exists and has a value (if so, use it) otherwise use a default value. Adding this to every single validates would be tedious to change and error prone, and using columns_hash as is would cause installation nightmares. @cbrunsdon Thoughts? (since you want to kill DbMaximumLength) |
Agreed @dangerdogz I think we should stick with something like |
I think the action items here should be:
Based on the conversations above, that seems like the best solution here. It doesn't solve the silent truncation issue with mySQL, but does solve the possibility of users overflowing user input fields with like a million lines of junk. How's that sound to everyone? |
@seand7565 I think that could be a good way forward. I'd just be mindful of how it will impact existing apps that currently rely on the absence of limits. With a proper deprecation approach, though, it shouldn't be a problem.
I'm not sure why you say that? If we have a limit on the length of the field at the application level, strings longer than the limit will not even reach the DB, unless I'm missing something. |
My proposal was to add the validator only to user input fields, not to admin input fields - Some stores that I've worked on overuse the product description field for instance, so putting a limit there might cause issues. In this case, the mySQL truncation would still happen on overly long fields for admin fields. Unless we want to just apply the validator to everything. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
I was looking at upstreaming some improvements over the weekend and noticed some inconsistencies that I showed to @jordan-brough and we both agreed might need some thought.
Rails 4.1 changed the behaviour of the
limit
attribute so now by default in Postgres it is no longer set - rails/rails#19001This behaviour can be seen here - https://github.com/alex-handley/solidus/commit/2b99ea4cace80d29f79fcfa0d8010b0d0a5b0a43
The spec in the branch will pass for both Sqlite and Postgres as they no longer have a field limit being set by default but will fail for MySQL because it still sets limits by default.
This leaves us with 3 potential issues:
This could lead to attackers dumping tons of data into fields as they have no limits.
Or more legitimately an address line1 could be 800 characters which would never fit on a label :)
There are a number of possible solutions:
DbMaximumLengthValidator
on all string fields, this would solve the issue for older apps.Has anyone encountered this already?
The text was updated successfully, but these errors were encountered: