Add default salt to hash IDs for new models #12958
Open
+101
−7
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the problem
Most of our hash IDs are currently able to be calculated by anyone, as most models do not use any salt when calculating the hash ID. This makes it so that hash IDs are only slightly more effective in preventing people from enumerating over all IDs for a resource than using plain numerical IDs.
We recently started adding salts to new models manually, but it is not currently the default for new models.
Describe your changes
Make using a hash ID salt the default for new models! This PR sets the salt configured in the environment variable as the default using an initializer, and then overrides that default to set an empty salt in existing models to not break existing links.
I also removed the inclusion of
Hashid::Railsfrom thePublicIdentifiableconcern, since it is not clear thatPublicIdentifiableincludesHashid::Rails, and applying the empty salt override only needs to happen in existing models. (At the very least, the empty salt override needs to be in the invididual models - I think it's also clearer if we includeHashid::Railsin the individual models alongsidePublicIdentifiable, however if the annoyance of forgetting to includeHashid::RailsalongsidePublicIdentifiableis a bigger concern, that's fine too).