Core: TBD: Don't throw 404 in /core/preview.png?file= if no preview exists

Created on 23 Apr 2016 · 25Comments · Source: owncloud/core

Steps to reproduce

Having https://ossec.github.io/ installed
Open folder with ~20 files for which oC can't create previews
Get blocked with rule "Rule: 31151 (level 10) -> 'Multiple web server 400 error codes from same source ip.'"

The rule can be easily updated to not jump in if 404s are happening on /core/preview.png but still it might be better to not throw that much 404s.

Expected behaviour

https://demo.owncloud.org/core/preview.png?file=/foo should return a placeholder preview

Actual behaviour

https://demo.owncloud.org/core/preview.png?file=/foo is returning a 404 causing a HIDS to jump in

Server configuration

ownCloud version: 9.0.1

Bug previews

Source

ghost

Most helpful comment

But i don't see where this should apply here.

Just see the /core/preview.png?file=/foo URL provided by ownCloud as a dead link on your web page: You're better off in fixing the dead link pointing to the 404 than hiding the errors from the logfiles.

ghost on 31 Mar 2017

❤1 😄1

All 25 comments

It makes sense to return 404s here though, since the client asked for a resource and it was not found? Perhaps you need to tweak your OSSEC rules to exclude that path?

Xenopathic on 23 Apr 2016

Yes, sure. The rule is already tweaked:

<group name="web,accesslog,web_scan,recon,">
<rule id="100031" level="0">
    <if_sid>31151</if_sid>
    <decoded_as>web-accesslog</decoded_as>
    <url>/core/preview.png</url>
    <description>Ignore all non-existent previews</description>
  </rule>
</group>

However the question is, if the client is asking for a resource or oC is pointing the client to a non-existent resource?

ghost on 23 Apr 2016

PVince81 on 25 Apr 2016

Previews for unsupported file types just shouldn't be requested in the first place. Most nice and future-proof way to do so would be to export "previewable" (supported by backend) filetypes into frontend context at runtime and then request preview only for matching (so supported) files.

Admin setting would also be great

In other words, only supported files should have

background-image: url("/index.php/core/preview.png?file=...");

and all others should use statics like

background-image:url(/core/img/filetypes/application-pdf.svg)

kernins on 9 May 2016

❤2

I don't see any problems to return some placeholder image from server if preview can't be generated. Why not?
It can be fixed very fast, no need to write 100+ lines of code to implement some complex algorithm. Just return a placeholder like Windows does.
In future, you can make it more complex if it will be required.

PaulAnnekov on 14 May 2016

Why not?

At least 'coz this would be masking/working around the issue, not actually fixing it.

Also this would probably break file-type dependent fallback icons until you reimplement them at the server side by returning different placeholders based on file type. Which would be sort of code duplication.

some complex algorithm

Nothing complex there really

kernins on 14 May 2016

At least 'coz this would be masking/working around the issue, not actually fixing it.

It's a question of what fix is the TRUE one :). Sometimes there is no single TRUE solution.

Also this would probably break file-type dependent fallback icons until you reimplement them at the server side by returning different placeholders based on file type. Which would be sort of code duplication.

I'm not very acknowledged with OC. If it's true then you're right, we can't use placeholder images.

Nothing complex there really

Between placeholders instead of 404 and determining "previewable" on server-side, getting "previewable" filetypes, determining if this file is previewable on client side? Of course the 2nd option is complex.

PaulAnnekov on 15 May 2016

I think we can make more simple solution. We get an xml list of files with some properties on navigation using request to https://example.com/remote.php/webdav/folder. We can try to add "previewable" flag there for each file. If it's true - request preview.png.

PaulAnnekov on 15 May 2016

It's a question of what fix is the TRUE one :). Sometimes there is no single TRUE solution.

And I'm in no way claimimng that my is the only one) I just telling that making requests for non-previewable types is meaningless runtime work, so that route is [probably] suboptimal and so not true.

"Probably" is because I'm too not really familiar with OC code, my suggestion is based on assumption that there must be some sort of ThumbBuilder programmatic entity which by definition knows what is previewable and what is not, and that that knowlegde may be "exported" in declarative form (so no runtime checks for each file).

If runtime checks would be required for each file, things wouldn't be so obvious anymore, but I'm "pretty sure" "declarative export" should be possible

We can try to add "previewable" flag there for each file. If it's true - request preview.png

This is actually one of possible implementations of what I'm talking about)

kernins on 15 May 2016

Even when disabling previews altogether (in config.php) the clients continue to be served 404s. That's pointless. This one should definitely be fixed.

jknockaert on 15 May 2016

Logically, the current behaviour makes sense. The client asks the server for a preview of a file, the server reports there isn't one. A 404 isn't necessarily an error condition. The problem is that we don't allow the client to cache the 404s, which is bad for performance.

Xenopathic on 15 May 2016

Logically, the current behaviour makes sense.

No it doesn't. As per RFC7231:

The 4xx (Client Error) class of status code indicates that the client seems to have erred.

So there should be by definition an error condition.

jknockaert on 15 May 2016

Properly behaving servers SHOULD NOT throw 4xx around, and fill the logs, and overburden monitoring systems, and screw up IPS/IDS, etc.
Just upgraded from 8.2 to 9.0.3; I'm totally fed up...

cedric-dufour on 19 Jul 2016

when i change the apps/files/js/filelist.js file like this,,looks very well,,but the unit test error

diff -r ./apps/files/js/filelist.js /var/www/core/build/dist/owncloud/apps/files/js/filelist.js
1710d1709
< if (mime.indexOf("text") === 0 || mime.indexOf("image") === 0){
1749d1747
< }

milancoin-project on 24 Oct 2016

@milancoin-project Thanks for taking up this one.
@nickvergessen Not sure why you labelled it "enhancement"; in my opinion "bug" is more appropriate. It's in fact not very different from #23213 (i.e. just throw an exception that's handled elsewhere and in the meantime fill up logs with meaningless stuff); that one got labelled "sev2-high" (but got very little attention since).

jknockaert on 24 Oct 2016

just throw an exception that's handled elsewhere

Can you elaborate on this ? How would the exception be handled and what would be the actual response of "preview.png" then ?

The problem is that the server needs a way to tell the JS side that there was no actual preview for the requested file so it can fallback to displaying an icon. The current approach is returning 404. If we'd return 200, then it would believe that there is an actual preview and no icon would be shown.

I think the correct approach is to have the server provide a list of mime types for which we know there will be a preview. That list could be provided as part of "OC.MimeTypeList" (generated file) in a new "supportedPreviews" section.

PVince81 on 24 Oct 2016

@PVince81 Sorry for the poor language. What I mean is that OC uses the mechanism of one component issuing an instruction to another component that results in the other component throwing an error, that is caught lazily by the first component. In this case, the user browser (upon instruction of the server) asks for an non existing file, and the server passes an error back. In the case of #23213 the php server tries to insert an existing unique key in the database, which passes an error back. All these errors get however logged, which makes proper monitoring less efficient. It seems to me that this is not what error mechanisms are meant for: they should signal an error happening, whereas here they are part of intended behaviour.

jknockaert on 24 Oct 2016

The problem is that the server needs a way to tell the JS side that there was no actual preview for the requested file so it can fallback to displaying an icon. The current approach is returning 404. If we'd return 200, then it would believe that there is an actual preview and no icon would be shown.

What about "204 - No Content"? JS knows what to do without errors in the logfile... In my opinion a better response.

Eekiig on 4 Jan 2017

❤1

@DeepDiver1975 @PVince81 could we come to a conclusion here? I have gotten complaints about this behaviour from other places as well.
IMHO, 404 is not the correct reply, because for the majority out there it means that something is missing which SHOULD be there. This is clearly contradicting to the actual intentions of anyone configuring previews in their config.php.
If I go through the process of actually configuring previews, I absolutely expect to see such for the configured mime types and do not want to be bothered with anything else.

dercorn on 30 Mar 2017

Properly behaving servers SHOULD NOT throw 4xx around

This requirement cannot be fulfilled, especially with APIs like Webdav.
Because file conflicts are expected situations and will return a 409 conflict error code.
In some situations 4xx are expected to be checked against, not something an admin needs to worry about.
The mobile client usually does a HEAD call to the server to check if a file exists before uploading and expects to receive a 404. So 4xx errors cannot be completely avoided.

That said, for the previews I guess what bothers people is mostly that these 404 appear too often instead of being rare cases. The quickest solution then is probably to return a 204 - No content or something similar and have the JS code check against that and fallback to mime type icons.

@VicDeo can you take care of this ?

PVince81 on 31 Mar 2017

That said, for the previews I guess what bothers people is mostly that these 404 appear too often instead of being rare cases.

Exactly. Especially as the client / browser got pointed by ownCloud to a non-existing resource which is then throwing that 404. Its not the opposite that the browser just accessing the non-existing resource on its own.

ghost on 31 Mar 2017

Just another IMHO: for webservers that I maintain I always exclude static assets locations from error log. :)
It helps to track other [potentially harmful] activities a lot.