vendor/ is web accessible

+1 on this, as an understatement.

I suggested something similar in one of the Composer issues, and the push-back was that we want D8 to be installable in shared hosting, where the user can't change docroot.

Also: Boo that I'd need a login to bugcrowd to read those linked issues.

+1 to app, not docroot
app is standard in other php frameworks

Multiple reports in the Drupal 8 bug bounty program have highlighted vulnerabilities due to the presence of code in core/vendor being web accessible.

Really.

Beyond not adding crappy third party code to Drupal which I understand is too late now why don't we just htaccess deny that dir? (I think we had an issue for that or two)

I think I agree with this but it needs to be executed correctly, for example the way it's described now if you download the drupal zip file and extract to a folder named "drupal" you will have the the following structure:

• drupal
  • docroot (or app)
    • index.php
    • core
    • sites
    • modules
    • etc...
  • vendor

Which I think seems a bit of a mess, applications like Laravel have a public folder, is it too late to adopt that approach?

• drupal
  • docroot (or app)
    • core
    • modules
    • etc...
  • vendor
  • public
    • index.php
    • sites

In case we would do that, we should also document how people on shared hosting would be able / should be able to run drupal, given that they have less control over what is done.

I think we should certainly follow the suggestion of #6 ... there is no usecase in allowing to access anything from vendor.
In case we one day use composer and we get a usecase, we can tell people to opt in manually in .htaccess

If I understand the problem space enough, this would already solve all issues with vendor. Together with #8 it is probably the only thing left.
In case you control the service environment you also control how the code is build, so that you can implement whatever scheme you want to use.

+1 to #6/#8/#9, for the reasons cited, plus one more: the other proposed solution would be very disruptive.

So I wonder if there's a reason why #6/#8/#9 is insufficient.

For shared hosting we can provide an index.php at the root still:

- index.php

require __DIR__ . '/public/index.php';

- .htaccess

ONLY allow index.php

Then a more permissive .htaccess in public/ should be able to allow access to something like another front controller.

However I also think we should probably just lock out vendor/ and other dirs.

This may be a stupid suggestion, and I have no idea whether it's feasible on the d.o. infrastructure level, but I'm wondering whether it would make sense to have a packaged version of Drupal that's specifically targeted to shared-hosting environments and one that strictly mirrors the repository. This way we could fix this issue in a best-effort way for the former by implementing #6 and provide the full directory revamp only for the latter.

So I wonder if there's a reason why #6/#8/#9 is insufficient.

I was wondering too: I guess that, since .htaccess files may not be available/enabled, this is not a 100% reliable solution.

We always relied on .htaccess for security. Always. This is not new.

This move is way too big a disruption this far into beta.

Yes this looks like a straight bug/omission in our .htaccess.

I'd consider a directory move hardening, we should open a separate, major, issue for that.

Also like plach I'm not convinced we need to change the structure of the actual code base to support non-webroot even on shared hosting. index.php is already extremely light so handling that in packaging seems preferable.

@chx thanks for providing the patch, but the issue I see is what if #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead gets done. I know we can cross this bridge when we get to it but I was wondering if there is any way composer (run by the d.o packaging script) could add the .htaccess and web.config, maybe by running a post-install bash script?

The double packaging approach may also resolve many of the issues we have to make composer work properly with our codebase.

> I know we can cross this bridge when we get to it

This is a critical issue the linked one is a postponed normal. So yes, we will cross that bridge when we get there.

I already have a patch (that needs review) for getting composer install to run when packaging #2315545: Install composer dependencies to D8 when packaging so we could look at building on that.

note: whatever the packager is doing, testbot should do the same #1923582: Add ability for testbot to run 'composer install' during installation.

Unfollowing.

So yeah we should treat security issues over other issues this is for sure, but on the other hand I think not comitting the vendor dir is a good thing,
as it for example makes it easier to update 3rd party code, in case they would have a security issue.

Don't recall me wrong, but I think adding new files into the vendor directory shouldn't be an actual issue, composer should still be able to write all the other files in there.

Should we add some form of test coverage for that? I think having test coverage for a critical security issue is not a bad idea in general :)

Would #7 be that disruptive tough - It mostly means moving index.php down into a public/ folder ?

Also, regarding vendor being web accessible : a drupal site is not just the web head, it's the web head + drush + the site-specific drush commands + the drupal console + the site-specific console commands + maybe other custom scripts. All of this runs with a single code base (the "site codebase"), using a single set of libraries in a single vendor folder. So that vendor folder also contains libs for, say, console or drush commands, which are completely not intended for web use.

vendor being in the web docroot is indeed a view of the past :-)

Can't we just put the rule to deny core/vendor into the DRUPAL_ROOT htaccess file? That way the immediate security issue is fixed and we'll be covered in the future if any progress is made towards getting vendor out of the repo. Am I missing something?

Even after the IRC abuse I am happy to RTBC @chx's patch and will work on how we can remove vendor from the codebase, and how we can the packager to add .htaccess and web.config.

Adding a critical security fix to a packaging script won't cover people who cloned the repo? Is #27 not worth discussing? There are already rules in there to deny access to PHP files, why have two files?

This is pretty much what the patch in #15 is doing.

However when vendor is removed from the repo we need another solution, and this solution needs to work wherever vendor is (I hope it's not core/vendor).

@yched
You are doing an excelent job in being AFK.

But yes I agree, the vendor directory is full of stuff, which just don't belong into the land of front controllers. Front controllers are owned by the application,
and they use libraries.

Can't we just put the rule to deny core/vendor into the DRUPAL_ROOT htaccess file?

Good question, I was wondering that too but assumed there is an obvious reason why not do do so. I could imagine that its security hardening,
as people might use their own custom .htaccess file.

Ideally we should do both, as in the future, especially with issues @timmillwood is working on, this vendor directory might be not part of your custom checkout without the vendor dir preprared by Drupal itself.

However when vendor is removed from the repo we need another solution, and this solution needs to work wherever vendor is (I hope it's not core/vendor).

#27 solves exactly that. It works for the point in time when we do get vendor out of the repo and it works now. It also keeps an entirely generated folder "clean". Setting to NW, see what others think.

Also, the patch in #15 has trailing white space.

I agree with @dawehner, I can not see the harm in also excluding core/vendor and /vendor (let's future proof) in the root .htaccess and web.config files.

+++ b/core/vendor/.htaccess
@@ -0,0 +1,2 @@
+deny from all ¶

+++ b/core/vendor/web.config
@@ -0,0 +1,7 @@
+    </authorization> ¶

Unnecessary space at the end of these lines.

Good question, I was wondering that too but assumed there is an obvious reason why not do do so. I could imagine that its security hardening,
as people might use their own custom .htaccess file.

Another good point, people often move .htaccess rules into global Apache config to disable AllowOverrides along with other things. Having everything in the one .htaccess file makes it much less likely people miss some important settings.

Patch also adding vendor to .htaccess and web.config files

If we are going to change the directory structure, I opt for #2385387: Permanently split Drupal and Drupal core into seperate repositories

So are we sure we want to deny everything and not allow CSS/JS?

1. +++ b/core/vendor/.htaccess
   @@ -0,0 +1,2 @@
   +deny from all ¶
   
   +++ b/core/vendor/web.config
   @@ -0,0 +1,7 @@
   +    </authorization> ¶
   

   Trailing white space

2. +++ b/core/vendor/web.config
   @@ -0,0 +1,7 @@
   \ No newline at end of file
   

   And here.

Plus we need a newline at the end of the files.

So are we sure we want to deny everything and not allow CSS/JS?

Good question, not sure we should be including CSS/JS from vendor? Can't see any css/js files that we'd be using currently so lets just updated when that becomes a problem?

@dawehner I pretty sure since vendor has no way to interacting with our CSS and JS library system. If it does then it's a module and it goes somewhere else.

@pwolanin I think we can address this complexity with tests. See Drupal\system\Tests\System\HtaccessTest

Also re #7 and creating a docroot in Drupal, I ponder how we're going to make a module's .css and .js are going to end up as accessible if we adopted it. We already added and removed assetic once during D8.

Only CSS and JS in vendor is

./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/css/bootstrap.min.css
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/css/nv.d3.css
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/css/style.css

./behat/mink/driver-testsuite/web-fixtures/js/jquery-1.6.2-min.js
./behat/mink/driver-testsuite/web-fixtures/js/jquery-ui-1.8.14.custom.min.js
./guzzlehttp/ringphp/tests/Client/server.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/bootstrap.min.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/d3.min.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/holder.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/html5shiv.min.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/jquery.min.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/nv.d3.min.js
./phpunit/php-code-coverage/src/CodeCoverage/Report/HTML/Renderer/Template/js/respond.min.js

fixes from #41

I would rather see us enforce the same .htaccess we use for private files directly in vendor instead of further complicating the root .haccess.

We can't make vendor/ web server writable, so the .htaccess enforcing just won't work.

We could add a hook_requirements() warning that checks for the presence of the file and warns if it's not there, but that'd be an addition to the patch.

Overall if we have this in one place, I'd put it in the root .htaccess file - it's not impossible someone does rm -rf vendor && composer install which would nuke it.

Root .htaccess you're already required to keep it as is or take responsibility for not doing so.

@catch: +1 (also I really want to get rid of the vendor dir)

Whatever the outcome, should this issue need a change record? Or at least something documenting whatever change happens, especially with a recommendation for what you should do if you use Nginx or some other non-Apache non-IIS webserver.

@pwolanin while the vendor directory is in core, the .htaccess in vendor/ will be too - would need a revert of this issue to remove it.

Otherwise composer install is going to be run locally or by the package manager, Drupal can't do anything while it's actually being run.

I'm completely happy to have rules in both .htaccess and core/vendor/.htaccess for this issue.

https://groups.google.com/forum/#!topic/composer-dev/WgkZZY2qeoQ relevant discussion, although the answer was 'no' on there so far.

Yay we make the world safe for Apache!

What about everyone else?

I go back to the original recommendation: Harden the file structure so it has a docroot/ or app/ directory.

@Mile23 Apache and IIS. Other web servers already have to replicate the logic that is in .htaccess and web.config to be safe. This patch does nothing to change that.

I would love Drupal to ship with a separate docroot and which only contains index.php and the public files directory - but doing that is this issue is way out of scope for fixing the problem and is way out of scope for Drupal 8 because doing this is going to take a lot of effort and planning. It is much more than just moving some files around. We need to think about shared hosting, site installation, assets provided by modules and themes.

The patch still needs tests added to Drupal\system\Tests\System\HtaccessTest

@Mile23, please open a new issue for that. It's an entirely new hardening proposal.

This actual bug is due to an omission from our existing security layer we've had for years. I'm only interested in fixing that here, and that's the only thing I consider release blocking. It's possibly to run Drupal outside of the web directory, but we don't need to make it mandatory.

It's not just about adding the new directory as Alex points out - we'd need to change asset handling and other things too (like the statistics.php logger).

So the .htaccess blocking is obviously a good idea as a backup, but, um:

Multiple reports in the Drupal 8 bug bounty program have highlighted vulnerabilities due to the presence of code in core/vendor being web accessible. These include reflected XSS

Shouldn't we be filing upstream patches to fix any XSS vulnerabilities? I guess that wouldn't happen here (it would have to happen in private for most cases) but just don't want to see that point lost in all the hubbub...

+++ b/.htaccess
@@ -3,7 +3,7 @@
-<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">
+<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template|vendor)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">

^(\..*|Entries.*|Repository|Root|Tag|Template|vendor)$ does not prevent access to anything in vendor. It needs to be something like:

\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template|vendor\/.*|core\/vendor\/.*)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$

This protects both vendor/ and core/vendor/

So #56 is completely wrong! FilesMatch is only about matching files. There is no way from root .htaccess to block access to this directory.

I've written a composer script to ensure the .htaccess and web.config files are present. This can be tested by running the following command from DRUPAL_ROOT/core:

rm vendor/.htaccess; rm vendor/web.config; composer run-script post-install-cmd
<code>

I choose to test against <code>core/vendor/composer/installed.json

since it is not protected by any .php protection adding in the root .htaccess. And I don't think that this file will disappear.

Ah bummer, above I was thinking we'd use < Directory> but of course, you can't use that in htaccess. Do we just need someone to test the IIS rules here?

alexpott++ thanks for being awesome!

Will test and rtbc as soon as I can.

Looks like it will work wherever the vendor directory is, which is awesome, and will be added by packager once we get that running composer install!

I think we should use post-autoload-dump so that the ensureHtaccess script "occurs after the autoloader is dumped, either during install/update, or via the dump-autoload command."

Other than that I've tested the patch and it looks awesome.

@timmillwood
Do you mind writing a test that ensures that those files are really not accessible?

+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -44,6 +44,9 @@ protected function getProtectedFiles() {
+    // Try and access a non PHP file in the vendor directory.
+    $file_paths[] = 'core/vendor/composer/installed.json';
+

@dawehner that's what this does.

@pwolanin good point - we can just re-use FileStorage::htaccessLines() and in doing I found a bug in it!

Committed/pushed to 8.0.x, thanks!

Does this need a change record?

I really hope noone relied on this behaviour.

Btw, if you're running Nginx, here is the location directive you need to add to your server block to deny access to the root vendor directory:

    # Don't allow direct access to PHP files in the vendor directory.
    location ~ /vendor/.*\.php$ {
        deny all;
        return 404;
    }

You could also block access to all files in vendor:

# Block access tot he vendor directory.
location ^~ /vendor/ {
  deny all;
  return 404;
}