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.

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

Remove .htaccess from ignored file list #5701

Closed
wedi opened this issue Apr 13, 2017 · 16 comments
Closed

Remove .htaccess from ignored file list #5701

wedi opened this issue Apr 13, 2017 · 16 comments
Milestone

Comments

@wedi
Copy link

wedi commented Apr 13, 2017

Remove .htaccess from ignored files list.

Reasoning

It is not possible to completely backup a web project that requires a .htaccess file. There is no way to remove .htaccess from the ignored files list as it is included in sync-exclude.lst.
.htaccess is a configuration file. It seems to be the only configuration file in the list. All other entries care about OS cruft and tmp files and stuff.

Example

For a university team project we are developing a web application. For mod_rewrite among others some server settings need to be changed using a .htaccess file. We don’t want to use http.conf here because we need to have these settings synced between the team for the application to run.

Expected behavior

.htaccess should be synced when enabling Synchronize hidden files.

Actual behaviour

.htaccess is always ignored.

Steps to reproduce

Add a .htaccess file to a folder, try to sync.

Client configuration

Client version: Version 2.3.1 (build 1602)
GIT-Revision 433ba0 on Mar 28 2017, 03:34:43 uses Qt 5.6.2, OpenSSL 1.0.2k 26 Jan 2017

Operating system: Mac OS X 10.11.6

OS language: German

Qt version used by client package (Linux only, see also Settings dialog): Qt 5.6.2

Installation path of client: /Application

@guruz
Copy link
Contributor

guruz commented Apr 13, 2017

FYI @pmaier1 @michaelstingl I remember some discussion

@pmaier1
Copy link

pmaier1 commented Apr 13, 2017

There is no way to remove .htaccess from the ignored files list

You can edit the local sync-exclude.lst if you want to change that.

The thing is that .htaccess files that are deeper in the hierarchy can override the .htaccess file ownCloud provides in it's main directory which can cause security issues afaik. We need to check if this can be disabled in the main .htaccess or if there's another way to prevent it. Also the server should decide which files really can't be synced.

In general all files that do not have a real justification for not being synced (like '.part' which is not accepted by the server for architecture reasons) should be removed from the list or not be excluded by default to avoid files not being synced without the user noticing or without the ability to change it.

I think we also agree on switching the default of 'Sync hidden files' to enabled as discussed here

@michaelstingl
Copy link
Contributor

@ogoffart
Copy link
Contributor

You can edit the local sync-exclude.lst if you want to change that.

No you can't. You can only add things to the local config file, not remove.

We could remove it from the default ignore list on the client, but it would still cause error because it is blacklisted on the server. But we would show a relevant error message in that case.

Personally, i think it makes sense to remove it from the client's ignore list, and let the server reject it with an appropriate error.

@pmaier1
Copy link

pmaier1 commented Apr 13, 2017

No you can't. You can only add things to the local config file, not remove.

Interesting, thanks for clarification.

Personally, i think it makes sense to remove it from the client's ignore list, and let the server reject it with an appropriate error.

Yes, and that should be the general default I would say.

@wedi
Copy link
Author

wedi commented Apr 15, 2017

Instead of rejecting files named .htaccess files any parsing of these files by the server should be deactivated.
If some code is not allowed for security reasons, disallow it altogether instead of just blocking the way to add the unwanted code. One can possibly overlook some way to implant a malicious .htaccess file. Disallowing it can only be bypassed if there are even worse things possible on the server.

The result of AllowOverride None is more secure and removes a sync limitation for the user.

@ghost
Copy link

ghost commented Apr 23, 2017

IMHO removing the .htaccess file from the sync client ignore list and keeping the https://github.com/owncloud/core/blob/v9.1.5/config/config.sample.php#L1101-L1106 blacklisted by default is the correct way of handling this.

With this approach you can:

  1. allow people knowing what they are doing (like e.g. @wedi) to remove the .htaccess from the blacklisted file at the server and configure a AllowOverride None to allow syncing .htaccess files
  2. protect people not knowing what they are doing (like tons of beginners doing their first steps with webservers and ownCloud) by default from malicious uploads of a .htaccess file

@benedictgoodman
Copy link

No you can't. You can only add things to the local config file, not remove.

It seems that this is only the case for the OSX client. I can still remove items from the ignore list in both the Linux and Windows clients by editing sync-exclude.lst. Does anyone know why this is the case? It would be nice to have that control with the OSX client too if possible!

@ghost
Copy link

ghost commented May 2, 2017

@benedictgoodman I think discussing / answering this question is out of the scope of this issue here. Maybe just create a new issue for that?

@pmaier1
Copy link

pmaier1 commented May 2, 2017

Following the discussion I think we can agree on removing '.htaccess' from the client's ignore list, letting the server decide and protecting less experienced people with the default as it is.

@pmaier1 pmaier1 added this to the 2.4.0 milestone May 2, 2017
@ogoffart
Copy link
Contributor

ogoffart commented May 5, 2017

How about other pattern such as *.part ?
there are also a bunch of other pattern which i am not sure what they are doing there.

ogoffart added a commit that referenced this issue May 5, 2017
As per issue #5701, if the server does not support it, let the server
show return an error, but we should not blacklist it localy
@ogoffart
Copy link
Contributor

ogoffart commented May 5, 2017

pull request: #5748

@pmaier1
Copy link

pmaier1 commented May 5, 2017

How about other pattern such as *.part ?
there are also a bunch of other pattern which i am not sure what they are doing there.

IMO we should remove all excluded file extensions unless there's real justification for it like file types that are OS-specific and really have no use in being synced. The current list has anyway been some quick shot, as @dragotin told me.

@phil-davis
Copy link
Contributor

phil-davis commented May 5, 2017

There are lots of patterns in there that "almost nobody" will want to sync - e.g. *.part files are created by Firefox when it is in the process of downloading a file. The .part file is then renamed/moved to the correct desired file name.
But then there will be some app that creates *.part files because P.A.R.T. is some acronym of something in the domain of the app, and the file extension happens to conflict.
By default new installs will want to have many/most of these patterns excluded, as a service to new users. So perhaps cut down the "fixed" list in sync-exclude.lst to just what is actually bad to sync.
Then populate the initial user-customized exclude list with the typical stuff that "normal users" (whoever they are) would want to exclude.
That leaves it open to users to remove from as well as add to that exclude list.
Perhaps for rebranding/commercial installs they would like to have a way to provide a list that adds to sync-exclude.lst and which the user cannot remove in the UI. That would allow commercial installs to provide a custom exclude list that their users cannot (easily) reduce.

Anyway - this sort of discussion should go in a more general issue somewhere. The particular .htaccess need here is good and should not be delayed by some long-winded discussion like I have made above!

@guruz
Copy link
Contributor

guruz commented May 8, 2017

@phil-davis Please feel free to create a new issue from your comment. (And check if we have already one..)

guruz pushed a commit that referenced this issue May 8, 2017
As per issue #5701, if the server does not support it, let the server
show return an error, but we should not blacklist it localy
@guruz
Copy link
Contributor

guruz commented May 8, 2017

PR has been merged
#5748

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants