Fix random replacement of http with https in resources (issue #3599) #3808

donker · 2020-06-03T07:44:13Z

An earlier "fix" for urls in emails added a blanket replace on each and every resource being retrieved. This is not just incorrect (as it also affects resources that should not be touched), it is also a performance hit.

This PR rolls back the fix and adds a "scheme" token to the portal settings. The [portal:scheme] token will resolve to either "https" or "http" depending on SSLEnabled for the portal. It is not used in any email in the framework currently, but if someone needs this in a template, it can be used.

Note that using the scheme token looks at the entire portal, so won't be a solution for those with mixed http/https situations. The [portal:fullurl] uses the current page's SSL setting, but obviously depends on context and so was never meant for emails.

…ware#3599)

sleupold · 2020-06-03T11:21:17Z

once this PR has been included, I'll have a look to update texts of the resource files.

bdukes · 2020-06-03T14:14:25Z

DNN Platform/Library/Entities/Portals/PortalSettings.cs

                case "url":
                    propertyNotFound = false;
                    result = PropertyAccess.FormatString(PortalAlias.HTTPAlias, format);
                    break;
-                case "fullurl": //return portal alias with protocol
+                case "fullurl": //return portal alias with protocol - note this depends on HttpContext


Does PortalAlias.HTTPAlias depend on HttpContext? I think if you use the PortalSettings constructor that takes a PortalAliasInfo, it shouldn't depend on context.

Emails might be sent by a background service without http context...

@bdukes : If you follow the AddHttp method, it uses HttpContext. I spent some time tracing this code and thought it was a good idea to include this in the comments as it means it is not safe for background tasks.

Globals.AddHTTP does reference HttpContent.Current, but it checks for null first, so it should be safe to use from a scheduled task.

bdukes · 2020-06-03T14:16:23Z

DNN Platform/Library/Entities/Portals/PortalSettings.cs

@@ -699,15 +699,19 @@ public string GetProperty(string propertyName, string format, CultureInfo format
            var isPublic = true;
            switch (lowerPropertyName)
            {
+                case "scheme":
+                    propertyNotFound = false;
+                    result = SSLEnabled ? "https" : "http";


The check in the code we're replacing is portalSettings.SSLEnabled || portalSettings.SSLEnforced. I don't know that it hurts anything for it to remain also checking SSLEnforced (probably better to err on the side of more HTTPS rather than less).

it should check for portalSettings.SSLEnabled && portalSettings.SSLEnforced - just SSLEnabled will deliver secured pages using http, if not called using https (e.g. if moved to a different location without https being bound to the IIS website.

My thought was indeed to err on the side of https. Rapidly more and more technologies are refusing to work with plain http (e.g. mobile apps). So if a site has so much as allowed SSL, I suggest the default return of "scheme" should be https. Rather than waiting to see if it is enforced.

Right, so SSLEnabled || SSLEnforced could be true more often than just SSLEnabled (though I'm not sure if there's any valid scenario where SSLEnforced is true while SSLEnabled is false)

Looking at this, and the option values, it seems that you could have SSL Enforced = true, but SSL Enabled = false. For this, I think we have to rely only on the SSLEnabled flag though, it may be a bit aggressive, but for current standards should be acceptable

bdukes · 2020-06-03T14:27:04Z

So, there's a small chance for this to be a breaking change. If someone has a modified version of an email template which uses the old http://[Portal:URL]/default.aspx?ctl=PasswordReset&resetToken=[Membership:PasswordResetToken] URL, it will no longer get auto-upgraded to https. @donker @mitchelsellers do you think it's worth either doing a find/replace in the email code, or running an update of localized resources during upgrade (i.e. find/replace http://[Portal:URL] with [Portal:FULLURL])?

sleupold · 2020-06-03T17:19:19Z

So, there's a small chance for this to be a breaking change. If someone has a modified version of an email template which uses the old http://[Portal:URL]/default.aspx?ctl=PasswordReset&resetToken=[Membership:PasswordResetToken] URL, it will no longer get auto-upgraded to https. @donker @mitchelsellers do you think it's worth either doing a find/replace in the email code, or running an update of localized resources during upgrade (i.e. find/replace http://[Portal:URL] with [Portal:FULLURL])?

Yes, this is a breaking change, but should not affect too many sites. However, we should definetely mention it in release notes

donker · 2020-06-03T18:29:55Z

So, there's a small chance for this to be a breaking change. If someone has a modified version of an email template which uses the old http://[Portal:URL]/default.aspx?ctl=PasswordReset&resetToken=[Membership:PasswordResetToken] URL, it will no longer get auto-upgraded to https. @donker @mitchelsellers do you think it's worth either doing a find/replace in the email code, or running an update of localized resources during upgrade (i.e. find/replace http://[Portal:URL] with [Portal:FULLURL])?

Absolutely. Which is why it goes in 9.7. Like Sebastian says, chances are slim and frankly, it is IMHO worse that those who included external fixed links using http find that the http was changed. So I am favouring those that highlighted that latter issue.

bdukes · 2020-06-03T18:46:21Z

Regarding the breaking change, the CoreMessagingScheduler, at least, does its own replace HTTP with HTTPS, so that should still continue to work:

Dnn.Platform/DNN Platform/Library/Services/Social/Messaging/Scheduler/CoreMessagingScheduler.cs

Lines 572 to 581 in 91223d2

    
           private static string RemoveHttpUrlsIfSiteisSSLEnabled(string stringContainingHttp, PortalSettings portalSettings) 
        
           { 
        
               if (stringContainingHttp.IndexOf("http") > -1 && portalSettings != null && (portalSettings.SSLEnabled || portalSettings.SSLEnforced)) 
        
               { 
        
                   var urlToReplace = GetPortalHomeUrl(portalSettings); 
        
                   var urlReplaceWith = $"https://{portalSettings.DefaultPortalAlias}"; 
        
                   stringContainingHttp = stringContainingHttp.Replace(urlToReplace, urlReplaceWith); 
        
               } 
        
               return stringContainingHttp; 
        
           }

bdukes · 2020-06-03T18:56:59Z

But it doesn't look like CoreMessagingScheduler uses the standard token replacement, it has custom tokens (e.g. [SITEURL] instead of [Portal:URL]). The scenario where we would expect breakage is the call to Mail.SendMail(newUser, MessageType.UserRegistrationPublic, PortalSettings) in UserModuleBase. @mitchelsellers @valadas are you okay with this small breaking change in 9.7? It's not a big deal to me.

mitchelsellers · 2020-06-05T12:56:39Z

@bdukes Yes, I think we are fine with this one, it errs on the side of security essentially. But I'm flagging as potentially breaking so we are sure to grab it in the release notes

mitchelsellers · 2020-06-05T12:58:49Z

DNN Platform/Library/Entities/Portals/PortalSettings.cs

@@ -699,15 +699,19 @@ public string GetProperty(string propertyName, string format, CultureInfo format
            var isPublic = true;
            switch (lowerPropertyName)
            {
+                case "scheme":
+                    propertyNotFound = false;
+                    result = SSLEnabled ? "https" : "http";


Looking at this, and the option values, it seems that you could have SSL Enforced = true, but SSL Enabled = false. For this, I think we have to rely only on the SSLEnabled flag though, it may be a bit aggressive, but for current standards should be acceptable

Fix random replacement of http with https in resources (issue dnnsoft…

7a3deee

…ware#3599)

donker added the Type: Bug label Jun 3, 2020

donker added this to the 9.7.0 milestone Jun 3, 2020

donker requested review from bdukes and mitchelsellers June 3, 2020 07:44

bdukes reviewed Jun 3, 2020

View reviewed changes

mitchelsellers added the Alert: Breaking Change label Jun 5, 2020

mitchelsellers approved these changes Jun 5, 2020

View reviewed changes

bdukes approved these changes Jun 5, 2020

View reviewed changes

bdukes merged commit f978d2c into dnnsoftware:develop Jun 5, 2020

valadas mentioned this pull request Jun 8, 2020

Localization.GetString autoreplacing http to https #3599

Closed

bdukes linked an issue Jun 8, 2020 that may be closed by this pull request

Localization.GetString autoreplacing http to https #3599

Closed

donker deleted the fixresxissue branch July 24, 2022 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix random replacement of http with https in resources (issue #3599) #3808

Fix random replacement of http with https in resources (issue #3599) #3808

donker commented Jun 3, 2020

sleupold commented Jun 3, 2020

bdukes Jun 3, 2020

sleupold Jun 3, 2020 •

edited

Loading

donker Jun 3, 2020

bdukes Jun 3, 2020

bdukes Jun 3, 2020

sleupold Jun 3, 2020

donker Jun 3, 2020

bdukes Jun 3, 2020

mitchelsellers Jun 5, 2020

bdukes commented Jun 3, 2020

sleupold commented Jun 3, 2020

donker commented Jun 3, 2020

bdukes commented Jun 3, 2020

bdukes commented Jun 3, 2020 •

edited

Loading

mitchelsellers commented Jun 5, 2020

mitchelsellers Jun 5, 2020

Fix random replacement of http with https in resources (issue #3599) #3808

Fix random replacement of http with https in resources (issue #3599) #3808

Conversation

donker commented Jun 3, 2020

sleupold commented Jun 3, 2020

Choose a reason for hiding this comment

sleupold Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdukes commented Jun 3, 2020

sleupold commented Jun 3, 2020

donker commented Jun 3, 2020

bdukes commented Jun 3, 2020

bdukes commented Jun 3, 2020 • edited Loading

mitchelsellers commented Jun 5, 2020

Choose a reason for hiding this comment

sleupold Jun 3, 2020 •

edited

Loading

bdukes commented Jun 3, 2020 •

edited

Loading