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

Server error/hint pages with a 500 error code to avoid it being seen … #9995

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Server error/hint pages with a 500 error code to avoid it being seen … #9995

merged 2 commits into from
Jun 26, 2018

Conversation

MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jun 26, 2018 •
edited
Loading

…instead of the actual resource

Could this solve most of the issues we had in the past with files being overwritten? Because error pages were served with HTTP code 200 :/

This should at least avoid problems with clients and the web UI.

Usages:

bildschirmfoto 2018-06-26 um 09 20 00

@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Jun 26, 2018
@MorrisJobke MorrisJobke mentioned this pull request Jun 26, 2018
Closed
rullzer
rullzer approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🙈

ChristophWurst
ChristophWurst reviewed Jun 26, 2018
View reviewed changes
lib/private/legacy/template.php Outdated
@@ -317,6 +317,7 @@ public static function printErrorPage( $error_msg, $hint = '' ) {
$hint = '';
}

header(self::getHttpProtocol() . ' 500 Internal Server Error');
Copy link
Member

Choose a reason for hiding this comment

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

What about http://php.net/http_response_code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just used the way it was before. We can of course also move to this. What is the advantage? No need to juggle again with the protocol and terms behind the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would for now vote for this approach to have a minimal backportable solution and then fix the http_response_code in a separated PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed that it is not that easy and I will change it already to make the code more clear.

This was referenced Jun 26, 2018
Merged
Closed
@MorrisJobke MorrisJobke force-pushed the bugfix/noid/error-page-with-500-http-code branch from 6186e83 to 574aa04 Compare June 26, 2018 07:55
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 26, 2018
@MorrisJobke MorrisJobke force-pushed the bugfix/noid/error-page-with-500-http-code branch from 574aa04 to 6fb0ae2 Compare June 26, 2018 08:21
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress backport-request labels Jun 26, 2018
@codecov
Copy link

codecov bot commented Jun 26, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@a97cc29). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #9995   +/-   ##
=========================================
  Coverage          ?   51.98%           
  Complexity        ?    26014           
=========================================
  Files             ?     1660           
  Lines             ?    96145           
  Branches          ?     1290           
=========================================
  Hits              ?    49983           
  Misses            ?    46162           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
index.php 0% <0%> (ø) 0 <0> (?)
lib/private/legacy/template.php 33.33% <0%> (ø) 39 <7> (?)
public.php 0% <0%> (ø) 0 <0> (?)
lib/private/legacy/files.php 13.09% <0%> (ø) 78 <0> (?)
remote.php 0% <0%> (ø) 0 <0> (?)
lib/base.php 2.14% <0%> (ø) 169 <0> (?)

@MorrisJobke MorrisJobke force-pushed the bugfix/noid/error-page-with-500-http-code branch from 6fb0ae2 to 77826b3 Compare June 26, 2018 08:22
@MorrisJobke
Copy link
Member Author

Okay - I fixed a bit more, but now error and exception pages are properly covered. I would not backport it anymore, but this should be fine for now.

Ready for review 🚢

ChristophWurst
ChristophWurst reviewed Jun 26, 2018
View reviewed changes
lib/private/legacy/template.php Outdated
* @suppress PhanAccessMethodInternal
*/
public static function printErrorPage( $error_msg, $hint = '' ) {
public static function printErrorPage( $error_msg, $hint = '', $statusCode = \OC_Response::STATUS_INTERNAL_SERVER_ERROR ) {
Copy link
Member

Choose a reason for hiding this comment

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

This might be very specific and unexpected, but what if the OC_Response class has not been loaded at this point and autoloading causes another error/exception? Should we use 500 instead, just to be on the safe side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - let me update it.

@MorrisJobke MorrisJobke force-pushed the bugfix/noid/error-page-with-500-http-code branch from a2efd12 to 18e8c5c Compare June 26, 2018 09:19
ChristophWurst
ChristophWurst approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

LGTM!

@MorrisJobke
Server error/hint pages with a 500 error code to avoid it being seen …
8c155cd 
…instead of the actual resource

* found while reviewing #7205
* allow to specify a special status code

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Server exception error pages by default with a 500 status code
1399f6b 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the bugfix/noid/error-page-with-500-http-code branch from 18e8c5c to 1399f6b Compare June 26, 2018 09:51
@MorrisJobke MorrisJobke merged commit 1bb5527 into master Jun 26, 2018
@MorrisJobke MorrisJobke deleted the bugfix/noid/error-page-with-500-http-code branch June 26, 2018 10:30
@MorrisJobke MorrisJobke mentioned this pull request Jun 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@rullzer rullzer rullzer approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@blizzz blizzz Awaiting requested review from blizzz

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug
Projects
None yet
Milestone
Nextcloud 14
Development

Successfully merging this pull request may close these issues.

Status code should be 500 on error
4 participants
@MorrisJobke @rullzer @ChristophWurst @skjnldsv