Dnn.platform: Localization.GetString autoreplacing http to https

[vmasanas]

Description of bug

When site is running with https (settings SSLEnabled or SSLEnforce enabled) the method GetString will replace any http to https on content received from resource files.

I understand this should only be happening (if ever) on urls that are local to the portal.
But a global replace on any resource is just wrong.

Since this has been added to the system a number of links we had on some resouces are failing because of this replacement.

Offending line is here:
https://github.com/dnnsoftware/Dnn.Platform/blob/7c790d82a6af27b28b1e2d65ba8737b2a450c257/DNN%20Platform/Library/Services/Localization/LocalizationProvider.cs#L83

Steps to reproduce

1. Configure a site to run with https
2. Set either or both settings SSLEnabled or SSLEnforced to true
3. On any resx file add a link to any domain for example http://www.domain.com
4. Use the value of the resource in any place

Current behavior

The text you'll get has been changed to https

Expected behavior

The value that's on the resx file is used and not touched in any way

[bdukes]

For reference, this change was made in 898cd7766b98f85abd20b3f91b25ded9a9a76604, historical discussion is in DNN-10289 and DNN-8020.

This fix is Not Good™ and now we're stuck needing to figure out a way to fix the fix without breaking folks relying on the fix. Maybe instead of resourceValue.Replace(@"http:", @"https:") we could do resourceValue.Replace(@"http://[Portal:URL]", @"https://[Portal:URL]") to limit the scope a bit more? Or even something like resourceValue.Replace(@"http://[Portal:URL]", @"[Portal:FullURL]").

Any ideas @mitchelsellers @donker @sleupold?

[mitchelsellers]

Wow @bdukes you are 100% this is beyond a Not Good™ fix, this is a true disaster.

Before I get into the specifics, this process also explains an interesting performance item that I noticed with regards to an excessive number of strings being generated on sites, I now know the root cause. Given that .Replace() is used on EVERY single resource item, and strings are immutable in C# this means that we get 2 strings created in memory for every resource loaded EVERY time we use HTTPS. So lots of memory thrashing that in 90% of the requests isn't needed, as a basic site will easily have 100-300 strings loaded from resource files.

However, I'll let that go for a minute and focus on the task at hand. A few thoughts.

1. Taking a step back on this, I'd almost encourage that we "fix the glitch" that was initially reported by updating the defaults in the DNN Platform to use Protocol-Relative links, so // instead of https:// or http:// for the links. Technically, given the prevalence of SSL implementation being strongly encouraged this is a bit of an anti-pattern now, but since we are talking downstream assets, this would be great. This would allow us to remove the call to .Replace and other items and would have a substantial performance improvement and would fix things. HOWEVER, for other resources in third-party items, this would be a breaking change since it has been this way now since 9.2.0

2.) We could look at using a replacement scheme as you mentioned, however, that would only work in situations where there is a single portal alias, which most would be, but it could still be less than perfect for all users.

There are some other options, but those require new API's etc. In the end, it appears this was a very bad hack for an issue that was limited in scope.

[sleupold]

IMO we should just do resourceValue.Replace(@"http://[Portal:URL]", @"https://[Portal:URL]"), if using https. External links to other sites should not be affected

[bdukes]

@mitchelsellers the use case for the "fix" is in generating emails, so the protocol-relative fix won't work there, DNN needs to know to generate the URL with the right protocol.

I'm _not_ saying that we look for the portal alias (e.g. .Replace("http://dnncommunity.org", "https://dnncommunity.org")), but actually replacing the token. The scenario we're fixing is that the default email templates used to include a URL using a pattern like http://[Portal:URL]/default.aspx?ctl=PasswordReset&resetToken=[Membership:PasswordResetToken], e.g. see https://github.com/dnnsoftware/Dnn.Platform/blob/898cd7766b98f85abd20b3f91b25ded9a9a76604/Website/App_GlobalResources/GlobalResources.resx#L724

One thing to note is that those URLs were replaced with a [Portal:FULLURL] token, e.g. https://github.com/dnnsoftware/Dnn.Platform/blob/develop/DNN%20Platform/Website/App_GlobalResources/GlobalResources.resx#L686

So this fix only covers folks who are using a customized version of those emails with the old pattern.

That said, I'd be okay with removing this behavior altogether as a small breaking change.

[mitchelsellers]

@bdukes You know...in this case then can we fix this elsewhere and make a win for everyone. If this was a fix for the emails, can we fix the emails and then remove this from the localization process. We would retain email functionality AND would gain in performance.

I think we could just do the fix on the template after we get the template as we are really fixing the email to be valid. This would save the cost on all other requests.

[valadas]

Oh, this explains a lot, I had relative links (no protocol) in all resource files (as it should be) but it caused email templates to not work. I would totally vote for @mitchelsellers solution: make all urls relative except mail templates, remove the bad fix and handle the email situation in the email code only.

[sleupold]

I didn't investigate, but as far as I understand, using relative urls with proper <base href="URL"> should work best inside HTML emails.

[valadas]

I didn't investigate, but as far as I understand, using relative urls with proper should work best inside HTML emails.

That works for browsers, not sure how good or bad support we have for this in emails... Worth trying and testing multiple mail clients.

[Navilois]

Still an issue I believe

[donker]

I am astonished that the earlier "fix" was ever accepted. The messages in question go through token replace. It is there that the http/s should be inserted, not through a blanket replace of evry single resource. The latter is a gross overreach. I'm submitting a PR to get this corrected in 9.7. Then we'll discuss on how to help those email links. IMHO the people who have urls erroneously replaced take precedence over those that have bad email links.

[valadas]

I think this is resolved by #3808 right? @donker we can close this issue?

[valadas]

And make it the 9.7.0 milestone ?

[bdukes]

Yes, it can be closed. Is 9.7.0 next? I thought we had been targeting 9.6.2

[valadas]

Well, @donker mentioned 9.7.0 but it was merged in, so I guess it goes in 9.6.2 then :)