In case of exception we return an html page in case the client is a b… #23557

DeepDiver1975 · 2016-03-24T13:31:12Z

…rowser

@PVince81 @LukasReschke the plugin approach to fix #23500

mention-bot · 2016-03-24T13:31:14Z

By analyzing the blame information on this pull request, we identified @LukasReschke, @icewind1991, @PVince81 and @DeepDiver1975 to be potential reviewers

DeepDiver1975 · 2016-03-24T13:31:42Z

apps/dav/lib/files/browsererrorpageplugin.php

+		if ($request->getMethod() !== 'GET') {
+			return false;
+		}
+		return $request->isUserAgent([Request::USER_AGENT_IE_8]);


@LukasReschke this is where I need all the mighty regex 🙈

In 9.0 we don't support IE8 any more 🔥
Only IE >= 9
See #15567 (comment)

PVince81 · 2016-03-24T13:48:05Z

Wow, nicely done!

PVince81 · 2016-03-24T13:55:10Z

  % curl -D - -X GET http://admin:admin@localhost/owncloud/remote.php/webdav/unexist.txt
HTTP/1.1 404 Not Found
Date: Thu, 24 Mar 2016 13:53:17 GMT
Server: Apache/2.4.18 (Linux/SUSE)
X-Powered-By: PHP/5.6.17
Set-Cookie: ockbfur1449k=f7fmc1arrtqi598dllgfb6l366n7pie51e9pbuil3bakk7jgc2t0; path=/owncloud; HttpOnly
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Set-Cookie: oc_sessionPassphrase=8llD2Q0WIFsVE%2BR2B4jwmE1lPUe3xjCeIpUaDf09EryVbncZvLtT0iUZogjAOWTr%2BlRfMRLfo2sbYGrU7juCQZcBE%2Fuj%2BErg0nxXTa1Ewq3VgWaAGkC7j98iNOmlHrCf; path=/owncloud; httponly
Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; frame-src *; img-src * data: blob:; font-src 'self' data:; media-src *; connect-src *
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-Frame-Options: Sameorigin
X-Robots-Tag: none
X-Download-Options: noopen
X-Permitted-Cross-Domain-Policies: none
Set-Cookie: ockbfur1449k=ho2b75ldlig380825usce0o5o207mrpd6ndja1k6fueln2iqlv40; path=/owncloud; HttpOnly
Content-Length: 239
Content-Type: application/xml; charset=utf-8

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>File with name unexist.txt could not be located</s:message>
</d:error>

Browser doesn't display a page:

PVince81 · 2016-03-24T13:55:45Z

Haha, maybe another trick would be to produce a XSLT file with style information ? 😛

PVince81 · 2016-03-24T13:56:43Z

isBrowserRequest is always false ??

LukasReschke · 2016-03-24T13:56:52Z

Browser doesn't display a page:

#23557 (comment)

… I'm still writing that magical Regex 😉 …

DeepDiver1975 · 2016-03-24T13:56:59Z

Browser doesn't display a page:

@LukasReschke has to add his regex - sorry - I was adding the 'to review' label too early

LukasReschke · 2016-03-24T14:03:02Z

lib/private/template.php

@@ -333,7 +333,7 @@ public static function printErrorPage( $error_msg, $hint = '' ) {
 	 * print error page using Exception details
 	 * @param Exception $exception
 	 */
-	public static function printExceptionErrorPage($exception) {
+	public static function printExceptionErrorPage($exception, $fetchPage = false) {


PHPDoc 😉

PVince81 · 2016-03-24T14:33:52Z

apps/dav/lib/files/browsererrorpageplugin.php

+			Request::USER_AGENT_MS_EDGE,
+			Request::USER_AGENT_CHROME,
+			Request::USER_AGENT_FIREFOX,
+			Request::USER_AGENT_SAFARI,


Too bad we can't just rely on "Accept" headers

Yep. Clients are fun 🙈

Once we have tokens this will be easy though since we know where the token belongs to.

PVince81 · 2016-03-24T14:41:45Z

We're almost there. The page says "Internal Server Error".
Can we make it display the actual error ? Like "File not found" for a 404 ?

DeepDiver1975 · 2016-03-24T14:58:00Z

We're almost there. The page says "Internal Server Error".
Can we make it display the actual error ? Like "File not found" for a 404 ?

how are you triggering the error?

PVince81 · 2016-03-24T15:05:59Z

See #23557 (comment)

It has the Webdav download link with a non-existing file.

PVince81 · 2016-03-24T15:07:09Z

Just open "http://localhost/owncloud/remote.php/webdav/unexist.txt" in Chromium 48 (after being logged in)

DeepDiver1975 · 2016-03-24T15:09:22Z

Okay - the real http status code is 404 and the explicit exception message is listed as well.

I simply used out exception error page which always prints 'internal server error'

PVince81 · 2016-03-24T15:12:10Z

It's not listed in my case:

DeepDiver1975 · 2016-03-24T15:13:52Z

It's not listed in my case:

ah - yes - the excption details are only displayed in debug mode.

What was the behavior with the ajax download? Which page did we display there?

PVince81 · 2016-03-24T15:16:57Z

It used to look like this in 8.2 with ajax download:

DeepDiver1975 · 2016-03-24T15:29:00Z

I see it explicitly renders the 404 template ....

DeepDiver1975 · 2016-03-24T15:31:59Z

I'm also getting a permanent reload loop here - due to NotAuthenticated

DeepDiver1975 · 2016-03-24T15:42:40Z

Okay - special 404 template added - any other cases we want to handle different?

PVince81 · 2016-03-24T15:43:51Z

403 in case it's blocked by the firewall or the file could not be decrypted, 423 if the file is locked.

Do these have to be handled one by one ?

PVince81 · 2016-03-24T15:44:12Z

Let me have a quick test with encryption

PVince81 · 2016-03-24T15:51:15Z

Tried with encryption and on stable8.2 the download runs forever... no page displayed 😦
On this branch no page, but Chromium shows its own "Invalid request" page.
I guess the "DecryptionFailedException" was never caught properly.

PVince81 · 2016-03-24T15:52:07Z

I thought we could just take whatever status code we have + exception name + exception description (if any) from the XML and display it as is in a page. But not "Internal Server Error".

PVince81 · 2016-03-24T15:55:59Z

Firewall blocking also shows "Internal Server Error" instead of "Forbidden" on this PR.

DeepDiver1975 · 2016-03-24T16:17:37Z

I thought we could just take whatever status code we have + exception name + exception description (if any) from the XML and display it as is in a page. But not "Internal Server Error".

I'll take care of that ....

DeepDiver1975 · 2016-03-24T16:50:38Z

PVince81 · 2016-03-24T16:56:08Z

Looks great 👍

DeepDiver1975 · 2016-03-24T17:11:43Z

let me squash that stuff a bit and then we can merge

@karlitschek do we want to backport this?

karlitschek · 2016-03-24T17:20:05Z

agreed. please backport :+1

…rowser

DeepDiver1975 · 2016-03-24T18:04:45Z

agreed. please backport :+1

THX - here we go #23574

lock · 2019-08-06T15:02:44Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added the 3 - To Review label Mar 24, 2016

DeepDiver1975 added this to the 9.1-current milestone Mar 24, 2016

DeepDiver1975 added the in progress label Mar 24, 2016

DeepDiver1975 reviewed Mar 24, 2016
View reviewed changes

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels Mar 24, 2016

LukasReschke reviewed Mar 24, 2016
View reviewed changes

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Mar 24, 2016

PVince81 reviewed Mar 24, 2016
View reviewed changes

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels Mar 24, 2016

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Mar 24, 2016

PVince81 added the app:dav label Mar 24, 2016

DeepDiver1975 and others added 2 commits March 24, 2016 19:02

In case of exception we return an html page in case the client is a b…

c46f480

…rowser

Add magical regex to catch browsers

95820fb

DeepDiver1975 force-pushed the sabre-plugin-browser-error-page branch from e4159b1 to 95820fb Compare March 24, 2016 18:02

DeepDiver1975 mentioned this pull request Mar 24, 2016

[stable9] Sabre plugin browser error page #23574

Merged

DeepDiver1975 added 2 - Developing 3 - To Review 4 - To release and removed 3 - To Review 2 - Developing labels Mar 31, 2016

DeepDiver1975 merged commit f6cea3c into master Apr 4, 2016

DeepDiver1975 deleted the sabre-plugin-browser-error-page branch April 4, 2016 11:51

DeepDiver1975 removed the in progress label Apr 4, 2016

lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019

In case of exception we return an html page in case the client is a b… #23557

In case of exception we return an html page in case the client is a b… #23557

Conversation

DeepDiver1975 commented Mar 24, 2016

mention-bot commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

LukasReschke commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

PVince81 commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

karlitschek commented Mar 24, 2016

DeepDiver1975 commented Mar 24, 2016

lock bot commented Aug 6, 2019