[For 10.4] Set 599 HTTP code on error #36413

jvillafanez · 2019-11-12T10:06:07Z

Description

web page shows a 200 status instead of a 500 (or similar) in case of error

Related Issue

Motivation and Context

How Has This Been Tested?

Load any ownCloud web page without having connection to the DB (ownCloud was installed but the DB connection was cut later)

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

codecov · 2019-11-12T13:08:42Z

Codecov Report

Merging #36413 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #36413      +/-   ##
============================================
- Coverage     64.68%   64.66%   -0.03%     
+ Complexity    19023    19022       -1     
============================================
  Files          1268     1268              
  Lines         74362    74386      +24     
  Branches       1309     1309              
============================================
  Hits          48100    48100              
- Misses        25876    25900      +24     
  Partials        386      386

Flag	Coverage Δ	Complexity Δ
#javascript	`54% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.84% <0%> (-0.03%)`	`19022 <2> (-1)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Log/ErrorHandler.php	`8% <ø> (+1.75%)`	`9 <0> (-3)`	⬇️
lib/private/Setup.php	`24.68% <0%> (-0.11%)`	`57 <0> (ø)`
public.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
status.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
index.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
remote.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/base.php	`3.81% <0%> (-0.22%)`	`151 <2> (+2)`
console.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1fe5d2...3674fa0. Read the comment docs.

jvillafanez · 2019-11-13T16:44:44Z

Summing up the changes:

Shutdown log is removed due to unreliability. There is no guarantee the ownCloud's logger will be available during the shutdown (seen when there is no connection to the DB)
/favicon.ico route will be rewritten to /core/img/favicon.ico. This happens when there is no connection to the DB. The browser doesn't load any page due to the server crash, and make a request to "/favicon.ico" to get the favicon. The change will make the favicon load properly under these circunstances.
New logger for crashes and improved error handling on the endpoints.

Notes on the error handling:

The request processing goes as follows:

Try to process the request normally. If there isn't any problem, this will generate a normal response.
In case the request isn't handled properly and throws an exception that isn't handled, the endpoint should try to log that exception using ownCloud's logging facility and show a proper error message to the user.

This will likely depend on the endpoint, but generally, the expected HTTP status should be 500 and an error message should be presented to the user. ownCloud already provides some templates that can be used, such as OC_Template::printExceptionErrorPage. In this case, we can assume that ownCloud is working properly, and it's an app the one that failed to process the request.

Note that this exception handling is expected to be light, mostly log the error and show a nice message to the user
In case we can't handle the exception, this means that ownCloud is broken and we can't report the error properly to the user. This has been seen if the connection to the DB is stopped, but there could be some additional scenarios.

In this case we assume that ownCloud is unreliable (we've decided to just log and show an error in step 2, but we couldn't even do that). In order to provide a similar behaviour in all the endpoints, we'll perform the following actions:
- For web environments (not CLI):
  1. send a "599 Broken" HTTP status to the client. This is intended to help to distinguish between this situation and a 500, which could mean that an app failed the processing but ownCloud still works.
  2. log the exceptions in the "crash-Y-m-d.log" ("crash-2019-11-13.log", for example) file.
- For CLI:
  1. log the exceptions in the "crash-Y-m-d.log" file (same as above)
  2. log the original exception in the console.

This new crash log file will be generated in the configured data directory, unless the crashdirectory is present in the config.php file. If it's present, that directory must exists and must be writable by the web server.

jvillafanez · 2019-11-21T07:51:16Z

I checked the code list from mozilla, and the 599 didn't appear... it might not be the best code.
I'm opened to suggestions, but I'd like to avoid 500 and 503 in order to distinguish this particular case from other "normal" errors.

sharidas

Haven't tested. But code wise looks ok to me.

micbar

please add a changelog item

This could happen if the server crashes using the web UI. The browser sends a request to the /favicon.ico URL instead of /core/img/favicon.ico, which is configured in the ownCloud's web pages. Without this fix, the server responds with a 500 instead of a 200 or 404

It was causing an additional DBALException when there is no DB connection. The logger was trying to access to the DB during the shutdown process, causing an unhandled exception to appear in apache

The endpoints will return a 599 HTTP status to distinguish app failures, which shouldn't break ownCloud and can still be reported through normal means. It will use a different file to log what happened without messing with the owncloud.log file and without bothering the web server's log Include new configuration option in config.sample.php file

phil-davis · 2019-12-02T06:02:48Z

@micbar should this be added to 10.4.0 project?
It might be difficult to add acceptance tests, because we would have to induce a server error to make code paths happen. Existing CI being green is good!

micbar · 2019-12-02T12:23:20Z

@phil-davis 10.4 is ok.
IMO this could be done via Changelog testing.

phil-davis · 2019-12-02T12:30:04Z

Someone needs to decide what to do about the codecov result.

VicDeo · 2019-12-04T10:38:38Z

@micbar
ErrorHandler: only removed lines, adding tests won't increase coverage (and the method is static too)
Setup.php: 1 line was added into a static method with no proper DI
base.php: static method once again technically it is possible to cover crashLog method but it would be not a classic unit-test.

the rest is too low-level for unit tests
console.php
index.php
public.php
remote.php
status.php

micbar

Accepted the PR, Needs a manual Release test during QA phase.
@davitol Please post an example of the crashlog contents.

phil-davis

LGTM - as in the discussion, automated tests are not realistic.
Someone with privs needs to override the codecov result and merge.

jvillafanez added the 3 - To Review label Nov 12, 2019

jvillafanez self-assigned this Nov 12, 2019

jvillafanez force-pushed the http_status_on_error branch from 250228d to d84eafc Compare November 13, 2019 17:09

jvillafanez requested a review from DeepDiver1975 November 13, 2019 17:12

jvillafanez changed the title ~~Set 500 HTTP code on error~~ Set 599 HTTP code on error Nov 13, 2019

jvillafanez force-pushed the http_status_on_error branch 2 times, most recently from 0dc63cf to 4eed679 Compare November 20, 2019 07:53

sharidas reviewed Nov 28, 2019

View reviewed changes

micbar requested changes Nov 28, 2019

View reviewed changes

jvillafanez added 5 commits November 28, 2019 14:21

Set 500 HTTP code on error

6910328

Remove onShutdown error handler

faddb89

It was causing an additional DBALException when there is no DB connection. The logger was trying to access to the DB during the shutdown process, causing an unhandled exception to appear in apache

Add changelog item

3674fa0

jvillafanez force-pushed the http_status_on_error branch from f79ca51 to 3674fa0 Compare November 28, 2019 13:21

micbar approved these changes Nov 28, 2019

View reviewed changes

phil-davis changed the title ~~Set 599 HTTP code on error~~ [For 10.4] Set 599 HTTP code on error Dec 2, 2019

VicDeo self-assigned this Dec 2, 2019

micbar approved these changes Dec 4, 2019

View reviewed changes

phil-davis approved these changes Dec 4, 2019

View reviewed changes

micbar merged commit 471637d into master Dec 4, 2019

delete-merged-branch bot deleted the http_status_on_error branch December 4, 2019 11:43

mmattel mentioned this pull request Dec 5, 2019

New config-to-docs run neccessary owncloud/docs#2140

Closed

12 tasks

davitol mentioned this pull request Jan 9, 2020

Tracking test results for ownCloud 10.4.0 #36732

Closed

39 tasks

VicDeo mentioned this pull request Apr 7, 2020

Cryptic error message not helping at all to solve the issue at hand #15272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[For 10.4] Set 599 HTTP code on error #36413

[For 10.4] Set 599 HTTP code on error #36413

jvillafanez commented Nov 12, 2019 •

edited by micbar

Loading

codecov bot commented Nov 12, 2019 •

edited

Loading

jvillafanez commented Nov 13, 2019

jvillafanez commented Nov 21, 2019

sharidas left a comment

micbar left a comment

phil-davis commented Dec 2, 2019

micbar commented Dec 2, 2019

phil-davis commented Dec 2, 2019

VicDeo commented Dec 4, 2019

micbar left a comment

phil-davis left a comment

[For 10.4] Set 599 HTTP code on error #36413

[For 10.4] Set 599 HTTP code on error #36413

Conversation

jvillafanez commented Nov 12, 2019 • edited by micbar Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Nov 12, 2019 • edited Loading

Codecov Report

jvillafanez commented Nov 13, 2019

jvillafanez commented Nov 21, 2019

sharidas left a comment

Choose a reason for hiding this comment

micbar left a comment

Choose a reason for hiding this comment

phil-davis commented Dec 2, 2019

micbar commented Dec 2, 2019

phil-davis commented Dec 2, 2019

VicDeo commented Dec 4, 2019

micbar left a comment

Choose a reason for hiding this comment

phil-davis left a comment

Choose a reason for hiding this comment

jvillafanez commented Nov 12, 2019 •

edited by micbar

Loading

codecov bot commented Nov 12, 2019 •

edited

Loading