dynamic / silverstripe-maintenance-mode

Maintainance/Offline Mode Module for SilverStripe
BSD 2-Clause "Simplified" License
21 stars 18 forks source link

Error on when using DisableRedirect #12

Closed spekulatius closed 8 years ago

spekulatius commented 8 years ago

Hello,

when im using DisableRedirect I get the following error:

error

FW/CMS 3.1.4

Cheers, Spek

dljoseph commented 8 years ago

Thanks for flagging this... @patricknelson introduced this feature. Perhaps he might like to review. I'm pretty stacked at the agency this week, but try to investigate this as soon as possible.

patricknelson commented 8 years ago

Naturally I have DisableRedirect enabled in my installation, however I'm not able to get that error. For what it's worth, my version is 3.1.13 but I don't think that will have an impact (general API should be the same). So -- I'll ask you a few questions to keep it simple as possible:

patricknelson commented 8 years ago

@spekulatius

Actually, I think I found the issue. This may have been a bug in the CMS itself which was fixed back in June 2014 and you're using a very outdated version which I think contains this bug, see: https://github.com/silverstripe/silverstripe-framework/issues/2467

This is because (as a normal part of flow) a 404 exception is being thrown in order to maintain the current URL (see: https://github.com/thisisbd/silverstripe-maintenance-mode/commit/e7e7967d4cd9a34dd5f75fd66ca78862e4c7030c#diff-28a47562a2f9d5468b3332e13b9d37c6R51) but the application isn't properly handling the controller stack, it appears.

I'd highly recommend you upgrade anyway, as I'm aware of at least a few security vulnerabilities in your current installation :) Give that a shot and report back.

dljoseph commented 8 years ago

@spekulatius - Did upgrading your CMS+Framework version work? Can this be closed as a non-issue now?

patricknelson commented 8 years ago

@dljoseph Out of curiosity: Did you confirm that you could reproduce the issue in later versions of 3.1 or 3.2?

dljoseph commented 8 years ago

@patricknelson I tested it 3.1.13, 3.1.14 and 3.1.15 and 3.2, but I could not reproduce the error in these versions.

patricknelson commented 8 years ago

I had never gotten to the point of even confirming that the issue would happen in an older version of SilverStripe either. The problem with your update however is that I'm not certain (just reading the code) that it will generate the necessary internal server error code (500) which is pretty critical for upstream caching, spiders and just good standards and etiquette. Can you confirm that this happens?

Currently I use this extension on a very high traffic website which sits behind Akamai and I cannot risk it caching the response. Also an issue if you're adding cache headers.

dljoseph commented 8 years ago

@patricknelson I cannot confirm this. Would you prefer if I removed the HTTP::add_cache_headers($response); lines. I am happy to do so.

patricknelson commented 8 years ago

No I think that's fine since that method is setup to be configurable in terms of cache length (albeit global) and after testing it in my case it's returning the appropriate HTTP error code response as well.

dljoseph commented 8 years ago

Fantastic. Thanks for reviewing.