FIX: use allowlist and blocklist terminology (PR #10209)

This is a draft PR of the renaming whitelist to allowlist and blacklist to the blocklist.

Based on @cvx advice, I changed a few variables so they read better like for example path_whitelist -> allowed_paths or whitelisted_char?(c) -> self.allowed_char?(c)

I still need to ensure that official plugins are fine and not need adjustment.

In addition, we need to merge and publish version 2.0.0 of onebox gem before merging those changes -


Will this regress if we update babel in the future?

Good point. This PR shouldn’t touch any vendored libraries and their APIs. Those hopefully will be changed upstream and then we’ll be able to change them.

Don’t think this diff is intended?

Don’t think this diff is intended?

I think we need to keep support for the old onebox classes. Otherwise all existing posts will lose their styling. The onebox changes will only take effect for new posts or following a rebake.

As above, I think we need to keep backwards-compatibility here

I do not think we should be changing migrations too. On the other hand, I doubt there is anyone with a schema that old looking to migrate.

Looks good to me, great work! :clap:

Feel free to ignore any of my comments.

Same as above.

We usually try to avoid using ActiveRecord models in migrations. Given the use here, I think we are safe, but it would be an easy change to switch to SQL queries, so maybe it is worth it?

Nitpick: I prefer allowed.

    result = params.permit(*permitted).tap do |allowed|

Is this a line we can’t remove yet?


There are site settings here which have been added in previous versions, we need to deprecate the site settings in this version and then remove them in the next version. We have a SiteSetting::DeprecatedSettings class to do this.

Also renaming site settings in migrations will result in the “old” site settings being reset to their default values until the latest code is deployed. Even if we move this to a post migration, the “new” site settings will have the default values until the migration runs.

O this is tricky… deprecating site settings just prevents undefined method errors in case plugins are using them. It doesn’t help to prevent the window where site settings get reset to their default values. We need to think about how to close that window.

Is this column in a commonly used path? This rename will result in errors until the new code is deployed.

Yes, definitely avoid using AR in migrations :+1:

Yes we need to keep this, because onebox HTML structure is stored in post cooked content. We will need a full rebake before we can remove this. (Or a DB migration to find/replace cooked… but that sounds very risky)

As above, i think we do need to keep this