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.

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

Static error-(404|500).html not honouring https protocol from .env SS_BASE_URL #20

Open
axllent opened this issue Mar 14, 2018 · 16 comments

Comments

@axllent
Copy link

axllent commented Mar 14, 2018

When static error pages are generated from the cli with dev/build the error-404.html and error-500.html files contain a <base> url that appears hard-coded to use http when the SS_BASE_URL is specified with https.

EG: SS_BASE_URL="https://www.site.com" will produce <base href="http://www.site.com/"><!--[if lte IE 6]></base><![endif]--> in the static files.

This causes "breakage" on the front-end when css & JS is blocked due to a downgraded protocol.

@chillu
Copy link
Member

chillu commented Mar 26, 2018

Right, it should honour SS_BASE_URL to avoid mixed content warnings - these won't appear when you load the error page over http://, but include CSS and JS via https://.

@tractorcow
Copy link
Contributor

tractorcow commented Apr 2, 2018

What if we used protocol-less absolute urls for base url? E.g. <link href="http://somesite" /> becomes <link href="//somesite">. If your site is being served across multiple protocols, even if your static files contain https you'll get mixed content warnings if your page errors on a non-https url.

I would make base_includes_protocol an option and default to false.

@axllent
Copy link
Author

axllent commented Apr 2, 2018

That's an interesting idea actually. I'm not sure whether that is (or isn't) allowed as I couldn't find an example in the specs of //.

@tractorcow
Copy link
Contributor

Try it out in a project I think it works fine ;) You can just hard-code it into your template.

@axllent
Copy link
Author

axllent commented Apr 3, 2018

I don't think that the fact it works it too relevant, there is a LOT of broken stuff that works in browsers ;-) It's more as to whether it is allowed/intended...

The base URL to be used throughout the document for relative URL addresses. If this attribute is specified, this element must come before any other elements with attributes whose values are URLs. Absolute and relative URLs are allowed.

Then the definition of absolute URL states:

An absolute-URL string must be one of the following

  • a URL-scheme string that is an ASCII case-insensitive match for a special scheme and not an ASCII case-insensitive match for "file", followed by U+003A (:) and a scheme-relative-special-URL string
  • a URL-scheme string that is not an ASCII case-insensitive match for a special scheme, followed by U+003A (:) and a relative-URL string
  • a URL-scheme string that is an ASCII case-insensitive match for "file", followed by U+003A (:) and a scheme-relative-file-URL string

It's a rabbit hole and to be honest I cannot find any reference to, or example of a base url using the "protocol-less" variation. I suspect this may not only be bad practice, but also something that should be avoided for something like base. Surely though the base href protocol should/could be parsed from a defined SS_BASE_URL?

@axllent
Copy link
Author

axllent commented Apr 3, 2018

@dhensby
Copy link
Contributor

dhensby commented Apr 4, 2018

oh, I really wouldn't do protocol less for the base tag...

@tractorcow
Copy link
Contributor

Could we statically cache both versions, and serve them up conditionally?

@tractorcow
Copy link
Contributor

The base URL to be used throughout the document for relative URL addresses. If this attribute is specified, this element must come before any other elements with attributes whose values are URLs. Absolute and relative URLs are allowed.

So it could be a relative url, just that google's crawler has a few issues with it.

I might suggest something that we can allow web devs to configure, rather than fix them to one solution.

@axllent
Copy link
Author

axllent commented Apr 4, 2018

No, I think that means that relative urls (eg <base href="/">) are fine, just not absolute urls without a protocol (eg <base href="//www.site.com/">).

I still don't understand however why the absolute URL cannot be taken directly from the defined SS_BASE_URL?

@chillu
Copy link
Member

chillu commented Apr 4, 2018

I think it should take SS_BASE_URL into consideration. I don't think we need to get into any conditional serving, that's overkill. If you're running https:// on your site, it's best practice to set up a blanket redirect anyway - in which case any 404s would be served from https://. I acknowledge that there's some situations where you need to run mixed (e.g. when third party <script> includes can't be served over https://), but that's a fringe case not worth spending a lot of time on.

@tractorcow
Copy link
Contributor

I guess subsites needs separate error files with different bases?

@dhensby
Copy link
Contributor

dhensby commented Apr 4, 2018

No, I think that means that relative urls (eg <base href="/">) are fine

they definitely aren't - I've tried that before, it's not compliant... works in some browsers but it's not meant to

@axllent
Copy link
Author

axllent commented Jan 22, 2019

Hey guys, considering the impact/high and effort/easy of this bug, I don't understand why it has been 10 months with no resolution? Surely everyone hosting with https is severely affected by this? I guess/hope it was simply overlooked ;-)

@chillu
Copy link
Member

chillu commented Jan 28, 2019

I'd put this back on impact/high, since I agree that mixed content warnings on "not found" and "server error" pages are bad. And this will affect most installs, since HTTPS is becoming fairly strongly encouraged by browsers now.

But: Can't you just work around the issue by not including the base tag in your template in the first place? As far as I understand, the only reason it's there is to allow serving SilverStripe from a subfolder (like http://localhost/site1 vs. http://localhost/site2). With the increasing use of VMs, I think that's an outdated concept, and we should remove support for it from SilverStripe 5.x.

@axllent
Copy link
Author

axllent commented Jan 29, 2019

That's a very valid point @chillu - and ironically it hadn't even crossed my mind (that I can recall) not to use the base tag, probably because it has always been a default is all templates including those still provided by SilverStripe.

I actually still use the http://localhost/site2 format for a lot of my development and probably will continue to for a great number of development sites. Having said that, I don't think the exclusion of the base tag should even affect those (at all) as we don't use the my-page/123/ format and $Link generates relative (to base) links... unless it is tied into the CMS for any reason.

OK, point noted, I will start removing these unless absolutely necessary in the meantime. +1 for getting it resolved anyway, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants