Thousand separator silently ruins inputted value in settings

bug
1

Horror story ahead.

We had a community manager reach out to us because they saw that the number of users on their forum was not rising as well as they were used to.

After some research we found that a lot of users that just signed up were being removed because they were deemed inactive.

This turned out to be because the setting clean_up_inactive_users_after_days was set to 1 :scream:

So I clicked the history icon and saw that the setting was changed from 730 (two years) to 1,095 (three years) over a year ago.

Yes people, when you enter a comma (or a dot, as is common in non-English speaking countries) as a digit group separator in a numeric site setting, it will silently(!) cut off everything behind the first separator. To make things even worse, this will only become visible after a hard page refresh.

12 Likes
3

I wonder if any other settings that use high numeric values are affected this way as well? :thinking:

6 Likes
4

Well, yes.
Every numeric setting is affected, and there are a lot of days, minutes and generic high value settings

imap_polling_old_emails:1000
decompressed_theme_max_file_size_mb:1000
topic_views_heat_low:1000
secure_uploads_max_email_embed_image_size_kb:1024
maximum_session_age:1440
post_edit_time_limit:1440
composer_media_optimization_image_resize_dimensions_threshold:1920
composer_media_optimization_image_resize_width_target:1920
max_bookmarks_per_user:2000
topic_views_heat_medium:2000
default_other_new_topic_duration_minutes:2880
polling_interval:3000
topic_views_heat_high:3500
max_image_size_kb:4096
max_attachment_size_kb:4096
min_topic_views_for_delete_confirm:5000
max_form_template_content_length:5000
invite_link_max_redemptions_limit:5000
sitemap_page_size:10000
max_notifications_per_user:10000
short_progress_text_threshold:10000
auto_close_topics_post_count:10000
max_drafts_per_user:10000
anonymous_account_duration_minutes:10080
default_email_digest_frequency:10080
tl3_requires_posts_read_cap:20000
anon_polling_interval:25000
max_post_length:32000
tl2_post_edit_time_limit:43200
max_export_file_size_kb:50000
max_bulk_invites:50000
background_polling_interval:60000
decompressed_backup_max_file_size_mb:100000
search_enable_recent_regular_posts_offset_size:200000
default_other_auto_track_topics_after_msecs:300000
max_draft_length:400000
composer_media_optimization_image_bytes_optimization_threshold:524288

  suggested_topics_unread_max_days_old:
  suggested_topics_max_days_old:
  pending_users_reminder_delay_minutes:
  invite_expiry_days:
  purge_unactivated_users_grace_period_days:
  anonymous_account_duration_minutes:
  ignored_users_message_gap_days:
  clean_up_inactive_users_after_days:
  clean_up_unused_staged_users_after_days:
  show_time_gap_days:
  old_post_notice_days:
  returning_users_days:
  suppress_digest_email_after_days:
  disallow_reply_by_email_after_days:
  delete_email_logs_after_days:
  reset_bounce_score_after_days:
  delete_rejected_email_after_days:
  purge_deleted_uploads_grace_period_days:
  tl2_requires_days_visited:
  tl3_requires_days_visited:
  invalidate_inactive_admin_email_after_days:
  send_old_credential_reminder_days:
  search_query_log_max_retention_days:
  cold_age_days_low:
  cold_age_days_medium:
  cold_age_days_high:
  delete_drafts_older_than_n_days:
  delete_merged_stub_topics_after_days:
  default_other_new_topic_duration_minutes:
  retain_web_hook_events_period_days:
  revoke_api_keys_unused_days:
  revoke_api_keys_maxlife_days:
  revoke_user_api_keys_unused_days:
  revoke_user_api_keys_maxlife_days:
6 Likes
6

Thanks for the report, @RGJ! :pray:

This is a scary one, indeed. We have fixed this here by 1) only allowing numeric inputs in the UI, and 2) removing any non-digits on the back-end for extra safety.

11 Likes
9

Reopening this, seems like things regressed and even got worse.

  • Go to Admin → Settings → Basic Setup → suggested topics

Firefox on desktop

  • Enter 5.0 then save (no error) and refresh → Find it has been set to 50
  • Enter 5,0 then save (no error) and refresh → Find it has been set to 0

Safari on iOS

  • Enter 5.0 then save (no error) and refresh → Find it has been set to 50
  • Enter 5,0 then save (no error) and refresh → Find it has been set to 50
4 Likes
11

Thanks Richard for reporting, I can definitely repro this.

2 Likes
12

Thanks for the report, Richard! I am also able to replicate what you are describing. It’s all “by design” as of today, except for the 5,0 resulting in 0 in firefox which is a bug. That’s a weird one.

Looks like we do need to take another pass through this, to at least update what the admin sees so it is accurate and does not include the separators. Currently you have to refresh your web browser after saving changes to see it.

But I think we can also do more here to help the admin avoid making these mistakes and avoid being confused by them. It feels wrong to me that you can put in “364.5 days” for suggested topics max days old and have that turn into 3645 days. Maybe we just don’t allow non-numbers to be entered in these types of fields.

6 Likes
15

Fixed this in FIX: Do not allow , or . in site setting integer input by martin-brennan · Pull Request #27618 · discourse/discourse · GitHub. It’s no longer possible to put anything but numbers in these fields. No more , or . separators.

We previously sanitized input for integer site settings
on the server side only, which was a bit confusing when
users would enter e.g. 100.5 and end up with 1005, and
not see this reflected in the UI.

Now that we are using native number inputs for these settings,
we can improve the experience a bit by not allowing . or ,
in the input, because it should be whole numbers only, and
add a step size of 1. All other characters are already prevented
in this native number input.

6 Likes
18

This topic was automatically closed after 3 days. New replies are no longer allowed.