Problem/Motivation

The .htaccess added to the config sync directory is untested - we should test it.

Proposed resolution

Add tests

Remaining tasks

User interface changes

None

API changes

None

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -121,6 +121,10 @@ public function testFileAccess() {
+    // Ensure yml files are not accessible in the sync directory.

SUPERNIT: s/yml/YAML/ ?

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +security

Doesn't the file directory htaccess have more restrictive rules?

On the other hand, it should only have YAML in it, as opposed to file upload directories which could have any old crap, but this wasn't discussed so far so let's at least document here why it's OK to skip it.

alexpott’s picture

Issue summary: View changes

@catch yes you are totally correct. Requiring the config sync directory to be writable is a really tricky bug though. I've improved the IS.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 7: 2723411-7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
pjcdawkins’s picture

Both the reasoning and the code look good to me.

pjcdawkins’s picture

Status: Needs review » Reviewed & tested by the community

... and a wise colleague has mentioned that means I can set this RTBC

geerlingguy’s picture

+1 on the RTBC, this seems adequate to me.

geerlingguy’s picture

Issue tags: +needs security review

But does it need a more official security review, just for completeness?

neclimdul’s picture

Status: Reviewed & tested by the community » Needs review

This makes me a nervous because it seems like we're swapping the file_save_access('', $true) on the config directory for the file_save_access('') on the parent files directory and the fact that the root .htaccess by default blocks all .yml files.

If someone modifies that for a totally reasonable "I need people to be able to upload yml files to my site" they could very easily open up their config directory without realizing it.

alexpott’s picture

@neclimdul ok. I think if someone opens up .yml in the main .htaccess they also have a problem with module version disclosure. However I guess given the possible contents of the directory we should go the extra mile.

alexpott’s picture

Title: Stop writing .htaccess to config sync directory » Test the .htaccess added config sync directory
Category: Bug report » Task
Issue summary: View changes
Issue tags: -needs security review
FileSize
818 bytes

So let's not attempt to do this at all. We can just go back to only enforcing the htaccess if the directory is writable in #2466197: Staging directory should not have to be writeable.. What we should do here is add proper tests for the .htaccess so we'll know if we break this protection.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wturrell’s picture

Issue tags: +Needs tests