Migration for PR #11321 fails to add `instance.tld` if there is already an account named `instancetld`

[rtucker]

I'm not 100% sure this is what's doing it, but I think that if you have an account like @vulpineclub@vulpine.club, 20190715164535_add_instance_actor.rb fails

Expected behaviour

rails db:migrate should complete error-free

Actual behaviour

rtucker@smithwicks:~/vulpine.club/mastodon$ docker-compose run --rm web bundle exec rails db:migrate
Starting mastodon_db_1    ... done
Starting mastodon_es_1    ... done
Migrating to AddInstanceActor (20190715164535)
== 20190715164535 AddInstanceActor: migrating =================================
rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:

Validation failed: Username has already been taken
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/validations.rb:80:in `raise_validation_error'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/validations.rb:52:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:315:in `block in save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:387:in `block in with_transaction_returning_status'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:265:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:212:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:385:in `with_transaction_returning_status'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:315:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/suppressor.rb:48:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/persistence.rb:53:in `create!'
/opt/mastodon/db/migrate/20190715164535_add_instance_actor.rb:3:in `up'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:817:in `exec_migration'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:798:in `block (2 levels) in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:797:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:416:in `with_connection'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:796:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/strong_migrations-0.4.1/lib/strong_migrations/migration.rb:10:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/strategy.rb:70:in `wrap'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy.rb:201:in `strategy'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:977:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1292:in `block in execute_migration_in_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1343:in `block in ddl_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `block in transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:239:in `block in within_new_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:236:in `within_new_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:212:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1343:in `ddl_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1291:in `execute_migration_in_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1263:in `block in migrate_without_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1262:in `each'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1262:in `migrate_without_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1210:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1363:in `with_advisory_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1210:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/strategy.rb:70:in `wrap'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy.rb:201:in `strategy'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1036:in `up'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1011:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/tasks/database_tasks.rb:172:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/strong_migrations-0.4.1/lib/strong_migrations/database_tasks.rb:4:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/railties/databases.rake:60:in `block (2 levels) in <top (required)>'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands/rake/rake_command.rb:23:in `block in perform'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands/rake/rake_command.rb:20:in `perform'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/command.rb:48:in `invoke'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands.rb:18:in `<top (required)>'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `block in require_with_bootsnap_lfi'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/loaded_features_index.rb:92:in `register'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:21:in `require_with_bootsnap_lfi'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `block in require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:257:in `load_dependency'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
bin/rails:4:in `<main>'

Caused by:
ActiveRecord::RecordInvalid: Validation failed: Username has already been taken
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/validations.rb:80:in `raise_validation_error'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/validations.rb:52:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:315:in `block in save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:387:in `block in with_transaction_returning_status'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:265:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:212:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:385:in `with_transaction_returning_status'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:315:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/suppressor.rb:48:in `save!'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/persistence.rb:53:in `create!'
/opt/mastodon/db/migrate/20190715164535_add_instance_actor.rb:3:in `up'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:817:in `exec_migration'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:798:in `block (2 levels) in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:797:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:416:in `with_connection'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:796:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/strong_migrations-0.4.1/lib/strong_migrations/migration.rb:10:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/strategy.rb:70:in `wrap'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy.rb:201:in `strategy'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:977:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1292:in `block in execute_migration_in_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1343:in `block in ddl_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `block in transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:239:in `block in within_new_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:236:in `within_new_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/transactions.rb:212:in `transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1343:in `ddl_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1291:in `execute_migration_in_transaction'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1263:in `block in migrate_without_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1262:in `each'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1262:in `migrate_without_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1210:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1363:in `with_advisory_lock'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1210:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `block in migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/strategy.rb:70:in `wrap'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy.rb:201:in `strategy'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/chewy-5.0.0/lib/chewy/railtie.rb:37:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1036:in `up'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/migration.rb:1011:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/tasks/database_tasks.rb:172:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/strong_migrations-0.4.1/lib/strong_migrations/database_tasks.rb:4:in `migrate'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activerecord-5.2.3/lib/active_record/railties/databases.rake:60:in `block (2 levels) in <top (required)>'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands/rake/rake_command.rb:23:in `block in perform'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands/rake/rake_command.rb:20:in `perform'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/command.rb:48:in `invoke'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/railties-5.2.3/lib/rails/commands.rb:18:in `<top (required)>'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `block in require_with_bootsnap_lfi'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/loaded_features_index.rb:92:in `register'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:21:in `require_with_bootsnap_lfi'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `block in require'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:257:in `load_dependency'
/opt/mastodon/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
bin/rails:4:in `<main>'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

Steps to reproduce the problem

1. Create an account with the instance's domain minus the . (e.g. vulpineclub for vulpine.club
2. Attempt to apply 20190715164535_add_instance_actor.rb

Specifications

Running glitch-soc at 6db5669

[rtucker]

I believe unique_username_validator.rb is what's probably tripping it...

normalized_username = account.username.downcase.delete('.')

Update: Confirmed... this fixed it:

diff --git a/app/validators/unique_username_validator.rb b/app/validators/unique_username_validator.rb
index fb67105dd..2fb5893a1 100644
--- a/app/validators/unique_username_validator.rb
+++ b/app/validators/unique_username_validator.rb
@@ -4,7 +4,8 @@ class UniqueUsernameValidator < ActiveModel::Validator
   def validate(account)
     return if account.username.nil?
 
-    normalized_username = account.username.downcase.delete('.')
+    normalized_username = account.username.downcase
+    normalized_username = normalized_username.delete('.') unless account.username = Rails.configuration.x.local_domain
 
     scope = Account.where(domain: nil).where('lower(username) = ?', normalized_username)
     scope = scope.where.not(id: account.id) if account.persisted?

Dunno if there are any side effects etc

[ClearlyClaire]

I really don't know why there would be a need to delete dots, since they're disallowed anyway.
Could you open a PR that just removes the delete('.') unconditionally?

[Gargron]

The idea is that someone could impersonate firstnamelastname with firstname.lastname or first.namelastname etc. Aren't the dots allowed when SSO is enabled or something like that?

[ClearlyClaire]

They could also impersonate firstnamelastname with firstname_lastname I guess. Good question about SSO…

[rtucker]

I'll create a PR to nuke the .delete('.')... I can see where impersonation could be a concern, but there's plenty of other avenues totally outside of this one (e.g. $15 and vuIpine.club could be yours). :)

[Gargron]

They could also impersonate firstnamelastname with firstname_lastname I guess. Good question about SSO…

That's a fantastic point actually and there's been a wave of phishing sign-ups recently that attempt to use similar looking letters and underscores to impersonate server admins. We can't do anything about the similar looking letters (or can we?) but if we applied the same rule for _ as for . going forward that could help a lot.

[rtucker]

So hmm... if we want to keep this protection in place, then maybe the way to do that is to skip the .delete('.') only if the requested account ID is less than 0. Does this sound reasonable?

(edit: and maybe add a .delete('_'))

(edit 2: new issue: i can't stop seeing '.' and '_' as smiley faces, 😹 )

[Gargron]

We could probably define a mapping like l <=> I and rn <=> m (don't remember if there are any others) and check if substituting those characters yields an existing account, the problem would be the amount of combinations you'd have to check, for example local usernames can be up to 30 characters long, if that's 30 times l, that's a lot of possible permutations and combinations

[Gargron]

So hmm... if we want to keep this protection in place, then maybe the way to do that is to skip the .delete('.') only if the requested account ID is less than 0. Does this sound reasonable?

Yes, this would be the correct solution in my opinion.

[rtucker]

Cool, I'll get that PR in today.

IMO the broader lookalike-character/impersonation problem is likely its own project, and I'm not sure this is the place to fix it.

[ClearlyClaire]

@Gargron just checking the code, this validator is called under the exact same conditions as the regexp validator, which disallows dots (which cannot work with our username-based routes in Rails anyway), so this .delete('.') is useless and only causes issues with this particular migration which bypasses the account regexp.