Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Jun 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions DNN Platform/Library/Entities/Portals/PortalSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sleupold sleupold Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

propertyNotFound = false;
result = PropertyAccess.FormatString(Globals.AddHTTP(PortalAlias.HTTPAlias), format);
break;
case "passwordreminderurl": //if regsiter page defined in portal settings, then get that page url, otherwise return home page.
case "passwordreminderurl": //if regsiter page defined in portal settings, then get that page url, otherwise return home page. - note this depends on HttpContext
propertyNotFound = false;
var reminderUrl = Globals.AddHTTP(PortalAlias.HTTPAlias);
if (RegisterTabId > Null.NullInteger)
Expand Down
13 changes: 1 addition & 12 deletions DNN Platform/Library/Services/Localization/LocalizationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,7 @@ public string GetString(string key, string resourceFileRoot, string language, Po
Logger.WarnFormat("Missing localization key. key:{0} resFileRoot:{1} threadCulture:{2} userlan:{3}", key, resourceFileRoot, Thread.CurrentThread.CurrentUICulture, language);
}

return string.IsNullOrEmpty(resourceValue) ? string.Empty : RemoveHttpUrlsIfSiteisSSLEnabled(portalSettings, resourceValue);
}

private string RemoveHttpUrlsIfSiteisSSLEnabled(PortalSettings portalSettings, string resourceValue)
{

if (portalSettings != null && (portalSettings.SSLEnabled || portalSettings.SSLEnforced))
{
resourceValue = resourceValue.Replace(@"http:", @"https:");
}

return resourceValue;
return string.IsNullOrEmpty(resourceValue) ? string.Empty : resourceValue;
}

/// <summary>
Expand Down