Dnn.Platform icon indicating copy to clipboard operation
Dnn.Platform copied to clipboard

Ensure StandardFolderProvider always returns an absolute URL.

Open rhaiamz opened this issue 3 years ago • 10 comments
trafficstars

Fixes #5085

Includes the fixes suggested in PR #5086.

rhaiamz avatar Aug 22 '22 10:08 rhaiamz

@mitchelsellers In this case, what are the next steps? Your current concerns are the reason why in my initial implementation, I placed it at a lower level, so the changes only happen at runtime.

rhaiamz avatar Aug 23 '22 07:08 rhaiamz

I think this logic should only apply when there's an SSL offload header setup.

bdukes avatar Aug 23 '22 12:08 bdukes

I agree with @bdukes that only doing this logic if an offload url is set would be proper.

That is a situation where content owners will expect environmental specific urls.

mitchelsellers avatar Aug 23 '22 12:08 mitchelsellers

I would love to understand this scenario better though because I am confused a bit.

Say I setup an instance without SSL at http://mysite.com Then I setup any kind of thing that uses SLL offloading (say a reverse proxy for local test site). The proxy will take in external requests https://mysite.com and forward those requests to http://mysite.com The file URL would be relative like /Portals/0/files/somefile.jpg The client browser would have no clue about the inner workings of the infrastructure and that would translate to using https.

So I fail to understand how an absolute URL helps here as I was kind of expecting the exact opposite as the proper way. I remember we actually had some issues in the Persona bar with absolute links to some resource files which made some functions unusable with ssl-offloading...

valadas avatar Aug 23 '22 16:08 valadas

There is a very, very unique situation where the SSL Offload is done using a DIFFERENT URL rather than the actual public/normal URL.

mitchelsellers avatar Aug 23 '22 17:08 mitchelsellers

There is a very, very unique situation where the SSL Offload is done using a DIFFERENT URL rather than the actual public/normal URL.

Exactly the source of my confusion, being relative, it does not include the admin part in it right? So relative should work, unless I am missing something, I don't understand...

valadas avatar Aug 23 '22 17:08 valadas

I will post a more extended explanation in the following hours.

rhaiamz avatar Aug 24 '22 07:08 rhaiamz

We did a bit of digging on our end. The URL(line 175 in FileServerHandler) that leaves the DNN code remains relative before it is passed to Response.Redirect at line 190 in FileServerHandler.

As you can see, at line 2394 this will force the URL to be converted to an Absolute URL based on a configuration setting found in system.web/httpRuntime. This for DNN seems to default to true, possibly from the days when the RFC2616 which required the Location header to be an absolute URI. In 2014 it was replaced by RFC7231 which states that relative URIs in the Location header are now permitted.

Upon setting the "useFullyQualifiedRedirectUrl" to false, the URL remained relative in the Location header, and the request no longer changed from HTTPS to HTTP. While simply turning this option off would fix the issues mentioned, I still think that returning an absolute URL for the file would be a better option, as this would eliminate any confusion and possibility for such an issue to occur in the future.

rhaiamz avatar Aug 24 '22 09:08 rhaiamz

Thanks @rhaiamz, that clarification helps a lot.

As @valadas pointed out, using a relative URL here seems most correct, so if we could preserve the relative URL, that would resolve the issue completely from my perspective.

I would suggest that the PR add a config transform to set useFullyQualifiedRedirectUrl to false. The only issue noted in the docs for that setting states "Some browsers might have problems loading pages in cookieless sessions when this value is false."

My assumption is that "some browsers" includes old browsers we don't support, and we don't support cookieless sessions, either AFAIK, so this seems like a safe change to me.

bdukes avatar Aug 24 '22 13:08 bdukes

Thanks, that explanation helps me understand now

valadas avatar Aug 24 '22 16:08 valadas

As discussed, this PR cannot be merged this way as it would produce too many side-effects.

valadas avatar Jan 19 '23 05:01 valadas