diff --git a/app/controllers/concerns/localize.rb b/app/controllers/concerns/localize.rb index db66819c3..7499c726b 100644 --- a/app/controllers/concerns/localize.rb +++ b/app/controllers/concerns/localize.rb @@ -111,7 +111,49 @@ module Localize end def switch_timezone(&action) - timezone = Current.family.try(:timezone) || Time.zone - Time.use_zone(timezone, &action) + Time.use_zone(resolved_timezone, &action) + end + + # How often to write a DebugLogEntry for the same (family, bad value) pair. + # switch_timezone runs on every request, so without this an affected + # family would write one row per page view forever. + INVALID_TIMEZONE_LOG_INTERVAL = 1.day + + # Family#timezone is a free-text IANA name (e.g. from an older DB dump, or + # a zone the tzdata maintainers later renamed, like the historical + # "Europe/Kiev" -> "Europe/Kyiv" switch). `Time.use_zone` raises + # ArgumentError on anything it doesn't recognize, which would otherwise + # take down every request/render for the affected family -- including the + # login page, since this runs on every request. Validate first and fall + # back to the app default instead of crashing. + def resolved_timezone + family = Current.family + requested = family.try(:timezone) + return Time.zone if requested.blank? + + zone = ActiveSupport::TimeZone[requested] + return zone if zone.present? + + log_invalid_timezone_once(family, requested) + Time.zone + end + + def log_invalid_timezone_once(family, requested) + cache_key = [ "invalid_family_timezone", family.id, requested ] + + # `fetch` is read-then-write, not atomic -- two concurrent requests could + # both see a miss and both log. `write(unless_exist: true)` maps to + # Redis's atomic SET NX in production, so only one request ever wins the + # lease and logs. + lease_acquired = Rails.cache.write(cache_key, true, expires_in: INVALID_TIMEZONE_LOG_INTERVAL, unless_exist: true) + return unless lease_acquired + + DebugLogEntry.capture( + category: "other", + level: "warn", + message: "Invalid family timezone #{requested.inspect}, falling back to #{Time.zone.name}", + source: "Localize#switch_timezone", + family: family + ) end end diff --git a/app/models/family.rb b/app/models/family.rb index adec49617..c1a6111c8 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -144,6 +144,7 @@ class Family < ApplicationRecord validates :moniker, inclusion: { in: MONIKERS } validates :assistant_type, inclusion: { in: ASSISTANT_TYPES } validates :default_account_sharing, inclusion: { in: SHARING_DEFAULTS } + validate :timezone_must_be_a_known_zone, if: :timezone_changed? before_validation :normalize_enabled_currencies! @@ -498,4 +499,28 @@ class Family < ApplicationRecord rescue Money::Currency::UnknownCurrencyError, ArgumentError nil end + + # Not a plain `inclusion: { in: ActiveSupport::TimeZone.all.map(&:name) }` + # on purpose: the settings form submits `tz.tzinfo.identifier` (e.g. + # "America/New_York"), not `tz.name` (e.g. "Eastern Time (US & Canada)") + # -- see LanguagesHelper#timezone_options. For every zone Rails ships, + # those two differ, so an inclusion check against `.name` would reject + # every legitimate value the form actually submits. `ActiveSupport::TimeZone[]` + # resolves both forms, and is the same lookup `Localize#resolved_timezone` + # uses at request time, so "valid at save time" and "valid when rendering" + # can't drift apart. + # + # Only runs when timezone is actually being changed (see the `if:` on the + # `validate` call above). A family that already has a stale value from + # before this validation existed (the exact case in #390) must still be + # able to save unrelated changes -- e.g. a settings update, or any + # background job touching the record -- without being blocked by a field + # nobody is currently trying to set. That value still can't crash a + # request either way, since Localize#resolved_timezone falls back safely + # regardless of whether this validation ever ran. + def timezone_must_be_a_known_zone + return if timezone.blank? + + errors.add(:timezone, :invalid) if ActiveSupport::TimeZone[timezone].blank? + end end diff --git a/test/controllers/concerns/localize_test.rb b/test/controllers/concerns/localize_test.rb index 2ee663edf..e05651b2b 100644 --- a/test/controllers/concerns/localize_test.rb +++ b/test/controllers/concerns/localize_test.rb @@ -54,4 +54,63 @@ class LocalizeTest < ActionDispatch::IntegrationTest assert_response :success assert_select "h1", text: /Configure your preferences/i end + + test "falls back to default timezone and logs a warning when family timezone is unrecognized" do + user = users(:family_admin) + # A deliberately nonexistent zone name, standing in for a stale/renamed IANA + # value slipping past validation (e.g. the historical "Europe/Kiev" -> + # "Europe/Kyiv" rename, or a migration that never ran). We can't use a real + # legacy alias like "Europe/Kiev" here: whether tzinfo still recognizes it + # depends on the host's installed tzdata version, which would make this + # test non-deterministic across machines/CI. + user.family.update_column(:timezone, "Invalid/Timezone") + sign_in user + + assert_difference "DebugLogEntry.count", 1 do + get root_url + end + + assert_response :success + + entry = DebugLogEntry.order(:created_at).last + assert_equal "warn", entry.level + assert_includes entry.message, "Invalid/Timezone" + assert_equal user.family, entry.family + end + + test "does not log when family timezone is valid" do + user = users(:family_admin) + user.family.update_column(:timezone, "America/New_York") + sign_in user + + assert_no_difference "DebugLogEntry.count" do + get root_url + end + + assert_response :success + end + + test "does not log again on a second request within the debounce window" do + user = users(:family_admin) + user.family.update_column(:timezone, "Invalid/Timezone") + sign_in user + + # The test environment's cache store is :null_store (config/environments/test.rb), + # which never actually caches anything -- every write is a no-op and every + # key looks nonexistent. Swap in a real store for this test so the + # debounce lease (Rails.cache.write unless_exist:) is meaningfully + # exercised instead of trivially passing. + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + + assert_difference "DebugLogEntry.count", 1 do + get root_url + get root_url + get root_url + end + + assert_response :success + ensure + Rails.cache = original_cache + end end diff --git a/test/models/family_test.rb b/test/models/family_test.rb index f53be0b3a..b214e5c1e 100644 --- a/test/models/family_test.rb +++ b/test/models/family_test.rb @@ -352,6 +352,45 @@ class FamilyTest < ActiveSupport::TestCase assert_not_includes Family.with_preview_features, family end + test "rejects a timezone ActiveSupport::TimeZone doesn't recognize" do + family = families(:dylan_family) + family.timezone = "Invalid/Timezone" + + assert_not family.valid? + assert_includes family.errors[:timezone], "is invalid" + end + + test "accepts a timezone identifier, the form the settings dropdown actually submits" do + family = families(:dylan_family) + # LanguagesHelper#timezone_options submits tz.tzinfo.identifier (e.g. + # "America/New_York"), not tz.name (e.g. "Eastern Time (US & Canada)") -- + # these differ for every zone Rails ships, so this is the case that + # actually matters, not just the display name. + family.timezone = "America/New_York" + + assert family.valid? + end + + test "allows a blank timezone" do + family = families(:dylan_family) + family.timezone = nil + + assert family.valid? + end + + test "does not re-validate an existing invalid timezone when saving unrelated changes" do + family = families(:dylan_family) + # Bypasses validations, simulating data that predates this validation -- + # e.g. the exact #390 scenario (a stale/renamed IANA zone already sitting + # in the DB). + family.update_column(:timezone, "Invalid/Timezone") + + family.name = "Updated name, timezone untouched" + + assert family.valid?, "an unrelated change must not be blocked by a pre-existing bad timezone" + assert family.save + end + private def set_preview_features(user, enabled) user.update!(preferences: (user.preferences || {}).merge("preview_features_enabled" => enabled))