Revert #7470 (default_url_options strip) -- exposes locale-leak regression#7476
Merged
Conversation
3 tasks
Contributor
Author
|
Filed #7477 with the cross-request locale-leak repro and candidates list. Once that lands, this revert can be reversed. |
orangewolf
approved these changes
May 26, 2026
Test Results 17 files ±0 17 suites ±0 3h 26m 6s ⏱️ - 7m 50s Results for commit ad72d6d. ± Comparison against base commit 7f0e346. This pull request removes 449 and adds 447 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Reverts #7470. That PR (merged earlier today) changed
Hyrax::Controller#default_url_optionsto only emitlocale:when the active locale differs fromI18n.default_locale. After deploy to koppie (https://pg.nurax.samvera.org), Hyrax began rendering pages in random non-default locales for users who had never requested those locales.The regression is real and user-visible. The fix in #7470 is correct in concept but uncovered a latent cross-request
I18n.localeleak in Hyrax that the previous always-append behavior was inadvertently masking. The leak deserves its own diagnosis and fix; this PR restores the pre-#7470 behavior so staging/production aren't degraded while that happens.Reproduction (live on pg.nurax)
Six consecutive cookie-less, Accept-Language-less curls all return a sign-in form whose action carries a non-default locale:
Output (during my session):
LaRita and I also saw it interactively: navigating from catalog search to the dashboard randomly flips the UI to Portuguese, French, Italian, German etc., even though the user only browses in English and never picked a non-English locale.
Why #7470's symptom is the leak being unmasked
Pre-#7470,
default_url_optionsunconditionally mergedlocale: I18n.locale. Every link, redirect, and form action carried?locale=X. Every subsequent request hitHyrax::Controller#set_locale:with a non-nil
params[:locale], soI18n.localegot forcibly pinned to a known value at the start of every request. Any other code path that mutatedI18n.localeorI18n.default_localewas effectively overwritten on the next request.Post-#7470, default-locale users navigate URLs with no
?locale=param.set_localefalls through toI18n.default_locale. If anything in the request lifecycle is mutatingI18n.locale(orI18n.default_locale) and the value persists across requests on the same Puma worker, the next visitor inherits that stale value. The deterministic per-worker stickiness in the curl repro above is consistent with this.I have not yet identified the exact mutator. Candidates worth checking:
app/models/user_mailbox.rb-- usesI18n.t(..., locale: preferred_locale)(kwarg form should be safe but worth ruling out)app/channels/hyrax/notifications_channel.rb-- writescurrent_user.update(preferred_locale: data['locale'])app/controllers/concerns/hyrax/works_controller_behavior.rb-- has its ownlocale = params[:locale] || current_user&.preferred_locale || I18n.default_localebranchapp/presenters//app/renderers/that readI18n.localeat URL-build timeI18n.default_locale =(vsI18n.locale =) anywhere in the boot/initializer pathRefs
Plan to re-land
Test plan
action=\"/users/sign_in?locale=en\"(or other stable locale) instead of 6 differing locales.🤖 Generated with Claude Code