sitemaps_host handles missing protocol#481
Closed
jasonkarns wants to merge 3 commits intokjvarga:masterfrom
Closed
sitemaps_host handles missing protocol#481jasonkarns wants to merge 3 commits intokjvarga:masterfrom
jasonkarns wants to merge 3 commits intokjvarga:masterfrom
Conversation
sitemaps_host is ones external (likely CDN) host, so it should prefer ones mailer config over controller config. This is because controller config can sometimes omit the protocol as they have request context available to infer the protocol from. Since mailers don't have a request context, they must always be explicit, and are more likely to be "correct" for this external sitemaps_host.
Just in case ones default_url_options have the :host set to nil or empty string, be extra safe here. (Existence of the key is not sufficient, the value itself must be present)
It's unlikely that action_mailer.asset_host is missing a protocol (if configured at all, it should definitely have one); because Mailers can't infer a protocol from the request (since there isn't one), it needs to be explicit. However, it's possible for an application to set action_controller.asset_host and _not_ set action_mailer.asset_host at all. In such a case, the inferred asset_host could legitimately be without a protocol. In such a case, we should run it through the full_url_for helper to ensure a protocol is present. The protocol that is inferred respects the config.force_ssl setting, when determining http vs https.
Author
|
This PR uses |
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
1 participant
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.
We want the sitemaps_host to be derived from the Rails app's
asset_host.Both
action_controller.asset_hostandaction_mailer.asset_hostdefault their value fromconfig.asset_host. ActionController does not require the asset_host to have a protocol, because it can infer the protocol from the request. ActionMailer's asset host must have a protocol specified, because there is no request from which it can be inferred.This means that if an app uses the top-level
config.asset_hostshortcut to set both action_controller and action_mailer's asset_host, then we can trust that it has a protocol.However, if the application sets them both independently (or if action_mailer.asset_host is not set at all!!), then we can not trust that the asset_host (from action_controller) has a protocol.
So to ensure that the host we use for sitemaps_host does have a protocol, we now run the inferred asset_host value through the
full_url_forhelper. This adds a protocol if it is missing; and as a bonus, respectsconfig.force_sslwhen adding said protocol (for determining http vs https).In addition to this safety check, we also (for robustness) want to ensure that an empty host (
nilor"") is inadvertently used in this helper. This would be a very unlikely scenario, but we already have to check for the:hostkey anyway; we might as well check for value presence.And lastly, we flip the precedence order for sitemaps_host to respect the mailer configuration over the controller configuration. In the common case, these will be the same, but if they are different, we likely want the "more external facing" host as used by ActionMailer.
As before, all of this logic is only for determining the default value for
config.sitemap.sitemaps_host. Any explicit setting on the railtie config, or onSitemapGenerator::Sitemap.sitemaps_hostwill remain untouched by the railtie.semi related: rails/rails#49656