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

Properly check the data dir #2513

Merged
merged 1 commit into from
Dec 6, 2016
Merged

Properly check the data dir #2513

merged 1 commit into from
Dec 6, 2016

Conversation

MorrisJobke
Copy link
Member

cc @nickvergessen @LukasReschke @rullzer

To test the setup check just comment out

file_put_contents($baseDir . '/.htaccess', $content);
and start the setup page.

To test the admin page check just remove the .htaccess file in the data dir and refresh ;)

@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Dec 5, 2016
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @icewind1991 and @rullzer to be potential reviewers.

@@ -209,7 +210,7 @@

$.ajax({
type: 'GET',
url: OC.linkTo('', oc_dataURL+'/htaccesstest.txt?t=' + (new Date()).getTime()),
Copy link
Member

Choose a reason for hiding this comment

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

then also kill the creation of this file

Copy link
Member Author

Choose a reason for hiding this comment

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

This was never created ;) (the server check creates it and kills it in the same call ;))

@LukasReschke
Copy link
Member

LGTM

* fixes #1364

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@codecov-io
Copy link

Current coverage is 57.32% (diff: 50.00%)

Merging #2513 into master will increase coverage by 0.09%

@@             master      #2513   diff @@
==========================================
  Files          1145       1202     +57   
  Lines         65670      72993   +7323   
  Methods        7365       7426     +61   
  Messages          0          0           
  Branches          0       1259   +1259   
==========================================
+ Hits          37585      41846   +4261   
- Misses        28085      31147   +3062   
  Partials          0          0           
Diff Coverage File Path
0% lib/private/legacy/util.php
•••••••••• 100% core/js/setupchecks.js

Powered by Codecov. Last update 572b078...a2867c0

@rullzer
Copy link
Member

rullzer commented Dec 6, 2016

Lets do this. 👍

@rullzer rullzer merged commit db6359d into master Dec 6, 2016
@rullzer rullzer deleted the fix-htaccess-checks branch December 6, 2016 07:57
@quenenni
Copy link

quenenni commented Dec 7, 2016

Hello,

I modified the 2 files but still have the same warning msg.
The call to "https://xxxxxxxxxxx/data/.ocdata?t=1481115617749" receives a response status of 200 and an empty responseText -> messages.push

Those things were checked:

  • The file data/.ocdata exists and is accessible directly (not "Forbiden page" msg as it should be)
  • The data/.htaccess file exists and should prevent the access to the data dir,
  • The AllowOverride All is in the vhost directives,
  • The "Require all denied" is set for "<Directory /data/>" in the vhost file,
  • I tested that on a simple vhost, with just a data dir and the same .htaccess file => the access to the data dir is blocked.

[EDIT]
We just tested nginx instead of Apache and we don't have that message anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config htaccess.RewriteBase makes admin datadir check fail
7 participants