Long Twig cache directories can cause failures on some filesystems

I can't recreate on a windows 10 install using xampp so this doesn't seem to be general d8 issue. Please provide more info about php etc

I seem to recall someone mentioning an issue on Windows with file paths that are over ~255 characters, and both of those templates would be (pardon the mixed slashes, just copying and pasting things together from your error messages):

C:\Bitnami\drupal-8.0.0.rc2-0\apps\drupal\htdocs\sites/default/files/php/twig/3b9a7135_block--system-messages-block.html.twig_1b89fea44fd124de67ee5957081934a6aa2d86b3295c1a8dd2211f15a8f19efb/f0f090b909fe6a121bae2b9d5538964c7899b4a3f361bdd3c517cc83769742fc.php

259 characters

Closed #2604486: Error during install (Twig unable to rename a file) as a duplicate of this issue. Thanks for the report!

I'm thinking the tests would ensure our cache filepaths end up as a reasonable length.

And I think #2587101: MTimeProtectedFastFileStorage (and likely others?) can generate paths that are too long for Windows was the first report but this now has a bit more info.

The root issue is that MAX_PATH on Windows is 260 chars. That page indicates that prepending \\?\ to an absolute path would remove the limit, but it has downsides such as not resolving .. and . within the path.

Funny, I was just starting to work on #2587101: MTimeProtectedFastFileStorage (and likely others?) can generate paths that are too long for Windows! I'll do that in this issue instead since it has more eyeballs on it.

Removing the Needs tests tag since we don't currently have Windows testing infrastructure.

@mikeker Thanks! I know we can't *really* test this but it's a nasty enough bug that it should have some sort of coverage I think, even if that coverage is just asserting "we don't generate super long paths".

@Cottser, the problem is we can limit our paths to whatever length we want, but if the site is installed at c:\very\very\very\long\path we'll end up over MAX_PATH anyhow. So there's not really a number we can pick to completely avoid this bug.

I guess we could assert that we never have a path longer than 257 so that you could always install Drupal at c:\ and it'll work... OK, you just watched me talk myself out of my own position. :)

I think the real solution is to introduce a utility class or trait that converts paths on Windows to use the \\?\ format and use that anyplace we use mkdir(). But that might be too disruptive at this point. I'll try and figure out a fix for this case and we can work on generalizing it later.

My thought was moreso that we could truncate those hashes that we are currently generating for the Twig cache filenames to something shorter but still with enough uniqueness, and to test it do something like compile all templates and ensure none of the paths are over a certain (yes, arbitrary) length. Just ideas.

What you're talking about sounds more like it would live in or near MTimeProtectedFastFileStorage (generically useful) and is probably the proper fix. But right now D8 seems too easy to break on Windows and I'd like to fix that sooner than later, even if it's somewhat of a band-aid.

First stab at the fix for this... FileStorage.php is a little funky so this has some cleanup in it for parts that I was already touching.

Still to do:

• Tests, as mentioned by @Cottser in #11.
• FileStorage:::unlink() doesn't work correctly on Windows as \DirectoryIterator doesn't handle Windows Unicode paths.

Note that this fix relies on the path to Drupal's temp dir being less than 260 characters.

Finally, this is really a workaround a limitation in PHP -- many file functions fail with Unicode paths (those that include the '\\?\' prefix which allows us to get past the 260 char path limitation). I haven't investigated any PHP bug reports regarding this yet.

Added issue summary and tagging for RC consideration.

looks like a decent solution for an unfortunate problem

1. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -20,6 +20,21 @@ class FileStorage implements PhpStorageInterface {
   +  protected $is_windows;
   ...
   +  protected $windows_path;
   

   camelCase, because member variable.

2. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -114,7 +183,7 @@ public static function htaccessLines($private = TRUE) {
   -   * Ensures the directory exists, has the right permissions, and a .htaccess.
   +   * Ensures the directory exists, has the right permissions and a .htaccess.
   

   leave oxford comma

Sorry for just nitpicks but you were mentioning how you wanted to use camelCase for OOP and I believe this is the place where we've been using it.

It would be nice to get someone else on windows who runs into this problem to give this a test as well to see if it resolves.

@joelpittet: Nitpicks are always welcome! Especially as we were just debating coding standards in IRC... :)

I've been playing with this a bunch over the past few days. I think the best solution is going to be a half-solution/half-workaround:

1. Drupal ensures that file paths it generates are no longer than, say, 240 characters (see below)
2. On Windows, the FileStorage class generates new files in Drupal's temp directory, which site builders will need to set to be less than 15 characters (eg: c:\temp or c:\Mike\temp), and then moves them to their final location
3. Similarly on unlink(), we would move a directory to the temp dir and delete the files there

(Edit: specified "on Windows" in the second bullet).

We do this complicated two-step because most PHP file functions on Windows do not support the long-path format prefixed with a '\\?\'. As far as I can tell, only

unlink()
mkdir()
rmdir()
rename()

work correctly with '\\?\' prefixed long paths. Which leaves us moving directories from a long-path to a short-path, manipulating them there, and then moving them back. Ugly, ugly, ugly... But I think it'll work.

It looks like PHP 7.x has support for Unicode paths, which should fix this problem as well. I haven't had a chance to try -- if anyone has a Windows setup with PHP 7.x on it, please report back your findings!

I tried the patch on a Drupal 8.0.0 site that started on an Acquia Cloud free instance and was copied down using Dev Desktop (November 22, 2015 release). It didn't seem to resolve the issue.

That was with PHP 5.6, so I switched to PHP 7.x and that didn't help the problem.

My temporary directory is set to C:\temp so it is short. The site location is better than if it were in my user directory at C:\drupal\drupalsites\bluedroprealty-dev\docroot, The resulting path that generates the error is
C:/drupal/drupalsites/bluedroprealty-dev/docroo/sites/bluedroprealty.dev.dd/files/php/twig/.942de757b6,sites/bluedroprealty.dev.dd/files/php/twig/53c3541f_status-messages.html.twig_4d23d10f88a68b61425c7d86bd20befdf23f8c3894a63344ee4d0d903cf03069/8f90a9801d2baa495aa89536da1d39bfb0f1ba3ca19ced5e55749b4770dd0b45.php which is roughly 314 characters long.

Right now the only work around I have figured out is disable twig caching.

I have the same problem.

Why is the filename not just a uuid? It is unique and short.

@hass to help find the compiled PHP for debugging I believe was the reason. Otherwise you'd have to search for the comment in the files.

To my knowledge there is currently: a hash for the service container to take deployments into account, a filename for easier debugging like @joelpittet said, and then a hash. And that's just the folder name. I still think we should consider shortening those last two hashes, not sure what the math is on if we would have a collision but seems unlikely with all the other data already in the path.

Can you decode these hashes to useful names or how does this help with debugging?

The status-messages.html.twig is the useful part (see a few posts up for an example file name).

That does not really answer my question.

Joel said we need the many hashes for debugging reasons. I see nothing useful from these hashes or can these decoded to something useful?

Otherwise we could just remove all hashes and use one uuid only.

This makes the filenames a lot shorter and already *unique* without adding ~5 useless hashes. Uuids are unique where random hashes may not as I know.

@hass, the 3 tools I use, phpstorm, sublime, atom all have ways of fuzzy searching file names. Which allows me to get to the file quickly, which is why it helps with debugging.

@hass - @joelpittet never said the hashes were for debugging. Please don't put words in people's mouths. He was saying the twig template file name is good for debugging and never mentioned hashes.

To start with we used the same structure as Twig itself I'm pretty sure, and we've just tweaked it from there.

If we remove all hashes things will break badly.

You spread fear that thing may break, but can you prove this? Uuids are unique, nothing can fail.

And well it is possible that twig not on drupal has the same bugs.

The uuid concept you're talking about hasn't so far been used in any Twig caching I've seen, whether ours or upstream. UUIDs might be worth considering over hashes, but there's more at play here.

Can we split bits of the filename out to sub-directories?

Then we'd have shorter individual filenames without losing any information.

@cottser, I understand the concerns about making changes, but as it is there is a significant operating system (at least for development) that is broken now. There has to be some way to make the file names a reasonable length so Windows doesn't choke.

Template suggestions would make the filenames quite long and less deterministic for size. So unless we do the '\\?\' magic. I'm willing to concede the nice for debugging file names for a quick fix and we can add them back when we or MS fix the directory length underlying problem.

I'm a bit wary about the UUID's collision points too and it's more risky to truncate those without finding out how unique we really need them. But the UUIDs aren't the problem, it's Windows directory length, the UUIDs and file names are just symptoms.

Does that sound like a decent trade?

@greggmarshall yes there definitely is and don't worry we understand it's broken for many people on Windows.

I am wondering why the path you shared seemingly has two sites/bluedroprealty.dd/files/php/twig in it, what's the deal with that? #17

C:/drupal/drupalsites/bluedroprealty-dev/docroo/sites/bluedroprealty.dev.dd/files/php/twig/.942de757b6,sites/bluedroprealty.dev.dd/files/php/twig/53c3541f_status-messages.html.twig_4d23d10f88a68b61425c7d86bd20befdf23f8c3894a63344ee4d0d903cf03069/8f90a9801d2baa495aa89536da1d39bfb0f1ba3ca19ced5e55749b4770dd0b45.php

I really don't want to lose the filenames in the cached Twig templates especially if we're waiting for MS to fix something in order to get it back :(

This is roughly what I'd propose, length of hashes can obviously be increased if we want, I went with the same length as the service container hash for this example:

Before

53c3541f_status-messages.html.twig_4d23d10f88a68b61425c7d86bd20befdf23f8c3894a63344ee4d0d903cf03069/8f90a9801d2baa495aa89536da1d39bfb0f1ba3ca19ced5e55749b4770dd0b45.php

After

53c3541f_status-messages.html.twig_4d23d10f/8f90a980.php

The hash used for the filename is necessary. It includes the containing directory's mtime() so if the files dir is compromised and someone replaces a rendered template the hash will fail since the mtime would have changed. (See Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php:112).

I believe the hash in the directory name is there for uniqueness and may be replaced with a UUID. Need to double-check, though. Seems to me we could either use a shorter hash (SHA1 is 40-char vs 64) or just use the first, say, 20 chars of the hash and assume that's good enough (a la Git commit references).

Regardless, even with crazy template suggestions, removing the template name from the path only gets us so far. A quick option would be to limit the amount of the template name included which should give you enough for fuzzy matching in IDEs.

The main issue, imo, is two 64-character hashes eating up nearly half of Windows (pathetically small) MAX_PATH.

@costter, I'm not sure but you are right it appears to be two paths separated by a comma. I just did a quick copy and paste, perhaps I grabbed more than I had anticipated when I tried to skip the rest of the error message.

The main issue, imo, is two 64-character hashes eating up nearly half of Windows (pathetically small) MAX_PATH.

Yup.

So why not:

Before:

53c3541f_status-messages.html.twig_4d23d10f88a68b61425c7d86bd20befdf23f8c3894a63344ee4d0d903cf03069/8f90a9801d2baa495aa89536da1d39bfb0f1ba3ca19ced5e55749b4770dd0b45.php

After:

53c3541f/status-messages.html.twig/4d23d10f88a68b61425c7d86bd20befdf23f8c3894a63344ee4d0d903cf03069/8f90a9801d2baa495aa89536da1d39bfb0f1ba3ca19ced5e55749b4770dd0b45.php

@catch I think because that doesn't solve the problem, it's not the filename length that's the issue it's the total length of the path (as far as I understand it anyway).

Oh.

That is my understanding also.

This also becomes an issue if you put your Drupal installations (even D7) inside subdirectories inside your windows "home" directory. The aggregated directories plus some really long file names for files (we have some PDFs with 150-200 character file names) puts you over the limit.

From #35 and #36:

@catch I think because that doesn't solve the problem, it's not the filename length that's the issue it's the total length of the path (as far as I understand it anyway).

That's correct, the total path length must be <= 260 chars.

In #31, @Cottser says:

I really don't want to lose the filenames in the cached Twig templates especially if we're waiting for MS to fix something in order to get it back :(

Agreed with the filenames. To be fair, MS fixed the issue ages ago (Windows NT, I believe) but the MAX_PATH constant remains for backward compatibility. Using the UNC-ish '\\?\' prefix (which allows for longer path and Unicode chars) would allow us up to 32k chars in path names. However, in PHP, only the four functions listed in #16 support those paths (as far as I can tell -- documentation on this is scarce). I haven't yet found an issue in the PHP buglist that directly addresses this and don't relish the idea of diving into PHP source code...

It should be pointed out that paths with Unicode characters will likely suffer from this bug as well, regardless of the path length. As far as I know, Unicode support on Windows was added only with the '\\?\' prefix. It would be great if someone could verify that...

Attached is a patch that shortens the hashes used to 10 chars and truncates the template name to 30 chars. This would help kick this problem another 100-odd characters down the road and the "proper" solution (whatever that may be) can be done in a followup. Let me know what you think about semi-solving this quick vs. solving this right.

In #38, @greggmarshall says:

The aggregated directories plus some really long file names for files (we have some PDFs with 150-200 character file names) puts you over the limit.

Unfortunately, this patch may not help you much if you have really long filenames for user-generated content. Until a real solution is found it would be best if you could truncate those names to a predictable length and arrange your directory structure so that paths don't exceed 260 chars.

Background: the 10 and 30 char limits are somewhat arbitrary. 10 chars of a SHA256 gives us a trillion unique options which seems plenty to me to avoid collisions (git commit mentions generally use less). But since this is tweaking a security feature which hardens Twig caching, I would appreciate input from someone with a better security perspective than mine (tagging as such).

Updated IS with security review details.

I was going to suggest we fork the implementation for Windows, but:

- we don't have automated tests for windows, so it'd be completely untested, as opposed to changing the implementation for everyone (even if we can't verify the windows bug is fixed)

- we should not change service definitions based on Windows or not, because of #2614202: CoreServiceProvider::registerUuid() assumes all environments have the same functions available - not that we'd necessarily do that here, but reminded me of that issue anyway.

@catch, The patch in #12 kinda forks the Windows implementation of MTimeProtectedFastFileStorage. The hope was to keep the API of MTimeProtectedFastFileStorage the same and handle Windows oddities internally. Is that what you are/were thinking?

I agree we should change the Twig cache filenames for everyone, not just windows.

I'm wondering (if it's feasible) if we should throw an exception in MTimeProtectedFastFileStorage on windows if we detect the filename is above the limit? Or is that a bad idea?

How should i handle such an exception? :-)

It would just be better than the current message is what I'm getting at. This is talking about fixing Twig but MTimeProtectedFastFileStorage is used by other things, not just Twig.

As @Cottser points out, MTimeProtectedFastFileStorage is used for more than just Twig caching. As such we can't trim the template names there as there are valid cases where the "name" would exceed 30-chars. That was causing some of the test failures.

The attached patch removes the filename trimming and makes the hash max length a protected property of MTimeProtectedFastFileStorage and TwigPhpStorageCache.

Sorry, I mangled things a bit and lost the easy interdiff (#2629240: Overwriting file-cached keys on Windows results in an access denied error was causing a lot of false failures). But it's a small enough patch that I'm hoping reviewers can overlook that... :)

ugh... Forgot the fixes to the tests.

Thanks @mikeker! This is looking very good to me, maxlenHash could maybe be hashMaxLength or something but that's probably about all I'd change and quite minor in the end. I'm not sure what kind of tests we should add beyond the tests that have been updated.

As far as I can tell this won't pose any problems for sites running in production, as soon as they see this new code they should switch to (and generate if necessary) the shorter directory/file names.

Seeing these nice tidy short names I almost wish we could add the extension (theme/module name) to the directory but maybe later ;)

One question, on line 86 is this line
$expected_filename = $expected_directory . '/' . substr(hash_hmac('sha256', $name, $this->secret . $directory_mtime), 0, 10) . '.php';

Is the 10 in the substr supposed to be $maxlenHash (or whatever it gets changed to)? Right now it looks like it is hard coded to be 10, which is what the constructor sets maxlenHash to.

@greggmarshall good question but that's in a test so I think it's fine.

From #52:

Is the 10 in the substr supposed to be $maxlenHash (or whatever it gets changed to)? Right now it looks like it is hard coded to be 10, which is what the constructor sets maxlenHash to.

@Cottser is correct, assuming you're referring to this line:

+++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
@@ -92,7 +92,7 @@ public function testInlineTemplate() {
-    $expected = $hash . '_inline-template' . '_' . hash('sha256', $class);
+    $expected = $hash . '_inline-template_' . substr(hash('sha256', $class), 0, 10);
     $this->assertEqual($expected, $cache->generateKey($name, $class));

(Edit: Dreditor code snippet was all messed up...)

The 10 is checking that the TwigEnvironment class uses 10 chars as it's max hash length. If someone were to change that value in TwigEnvironment, they would need to update the test. If they were to inherit from TwigEnvironment, they would want to write a different test for that child class using whatever value they specify in the child class.

From #51:

maxlenHash could maybe be hashMaxLength or something

I was following the pattern of numThings. Also I had a maxlenFilename in there until I realized that was messing up all the cache tests and they read better when there were two of them. Anyhow, happy to change it you feel it should be or it doesn't keep with Drupal's naming scheme. Feel free to bump back to NW if you think it should be changed.

Added updated about using shorter hashes to the IS. Also updated remaining tasks.

This problem still seems to be part oft drupal 8.0.1.

What can I do?

@DrupalFan2: at this point we need manual testing on Windows that the latest patch (currently #50) works as advertised. (Note this doesn't remove the problem -- paths in Windows can still go well over 260 chars -- it just makes it less likely to happen in a normal install). It has to be manual testing because we don't have an automated system for testing patches against Windows at this point.

It was suggested that we add automated tests to ensure that Drupal doesn't generate paths longer than a (somewhat arbitrary) limit of 240 chars. That would allow Drupal to be installed in a path such as c:\dev\mysite and still work. That has not been done at this point and I'm not sure if it is required to get this patch accepted.

This issue also needs signoff from a security perspective that the shortened hashes do not remove the protection created by MTimeProtectedFastFileStorage.

The patch in #50 appears to be working in my current context of a shared folder on a linux virtual box on a Windows host. I agree that a review by the security team would be reassuring.

The patch looks perfectly good and tests OK.

Just a question... if hash length is defaulted to 10, why are we using the 64 character length SHA256.

Something that already gives us shorter out of the box hashes might be a better option such as MD5 (32 characters)

And to reduce the probablity of collisions due to just taking the first 10 characters, there are other approaches.

I had to resort to something similar for cache keys in the supercache module, and this is what I ended up doing to have shorter hashes with a reduced collision probability:

  /**
   * Returns a 12 character length MD5.
   *
   * @param string $string
   * @return string
   */
  protected function shortMd5($string) {
    return substr(base_convert(md5($string), 16,32), 0, 12);
  }

The idea is taken from here.

http://stackoverflow.com/questions/3763728/shorter-php-cipher-than-md5

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -54,6 +61,9 @@ class TwigPhpStorageCache implements \Twig_CacheInterface {
+    // Prevent overly-long path names by using the first 10 chars of the hash.
+    // This gives us over a trillion potential keys.
+    $this->maxlenHash = 10;

what about just putting that as default value on the property itself? It would be great to somewhere document what we assume for which length as part of the directory name.

Sorry for the delay getting back to recent comments. Based on @david_garcia's suggestion in #59, setting this back to NW.

I have some concerns about the S.O. linked to in that post. Specifically the warning on the PHP base_convert doc page that says:

base_convert() may lose precision on large numbers due to properties related to the internal "double" or "float" type used. Please see the Floating point numbers section in the manual for more specific information and limitations.

In very quick testing, this warning seems to be borne out:

$test = "whatever";
$test = md5($test);
print $test . "\n";
print base_convert($test, 16, 31) . "\n";
print base_convert($test, 16, 32) . "\n";
print base_convert($test, 16, 33) . "\n";

results in:

008c5926ca861023c1d2a36653fd88e2
15co5dard70dei44ho5c537tf
hhcidik620g0000000000000
8l8wn22lq16apu5wrib3cwsc

It seems odd that the conversion to base-32 results in a number that's 12 digits shorter than base-31 or base-33. Also, shouldn't we convert to base-36 (0-9a-z) to full utilize the characters we can use in the filesystem?

re: #60: yeah, I should've done that originally. Will do in the next patch... Thanks, @dawehner!

The attached patch addresses #60 and #51.

From #59:

...if hash length is defaulted to 10, why are we using the 64 character length SHA256. Something that already gives us shorter out of the box hashes might be a better option such as MD5 (32 characters)

My main concern is that MD5 (and SHA1) are broken hashing functions, but SHA256 is not (yet). Using 40 bits of a 128 bit hash has the same number of collision possibilities as using 40 bits of a 256 bit hash. But hacking the input string to return the same hash is harder (currently impossible, but who knows for how long).

what I ended up doing to have shorter hashes with a reduced collision probability

I do like the idea of converting whatever hash into base-36 so as to fully use the 0-9/a-z character range we have available. But that only drops a 64-character hex string to 54 characters, or a 32-character string to 27. (Worst case scenario -- often ends up being a few characters less depending on the numeric value of the hex string). In playing with this idea, I ran into the fact that base_convert only handles about 14 digits decimal, which is 12 digits hex, adding complexity to the hashing system.

Perhaps the compromise is to truncate to 48 bits (12 hex characters) and convert to base-36 to shave off another character or two?

I think current implementation is more than good.

I've also tested by using this patch as part of our build process without issues so far. Twig cache paths are well bellow the 256 limit with this patch, the longest one's I have seen are between 100 and 125 characters long.

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -47,6 +47,13 @@ class MTimeProtectedFastFileStorage extends FileStorage {
+  protected $maxlenHash;

@@ -60,6 +67,7 @@ class MTimeProtectedFastFileStorage extends FileStorage {
+    $this->maxlenHash = 10;

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -44,6 +44,13 @@ class TwigPhpStorageCache implements \Twig_CacheInterface {
+  protected $maxlenHash;

@@ -54,6 +61,9 @@ class TwigPhpStorageCache implements \Twig_CacheInterface {
+    $this->maxlenHash = 10;

Did we consider making this a constant on PhpStorageInterface? And documenting that this is used to keep total file path lengths small enough for windows. This would mean if we decided to only support some new version of windows that supported large paths we'd just have one constant to update. It also seems to me that this maxHashLength is due to the implementation. So something feels off here. What we do for cache keys - where the max length is 255 is only hash once the path is longer than. I'm wondering why we don't take a similar approach here and determine the path length of the thing we're about to write and do something if it is too long.

I'm pondering what is going to update on existing sites when they apply this patch. Should there be a hook_update_N to clear the existing twig cache files... and what does that mean for multiple web heads.

Also I think we should add a hook_requirements check for windows and check the depth of the install path and issue a warning or something.

What we do for cache keys - where the max length is 255 is only hash once the path is longer than. I'm wondering why we don't take a similar approach here and determine the path length of the thing we're about to write and do something if it is too long.

I don't like this for paths because these paths might be relative to something and be mounted in Symlinks or the like. So from my application I might see that a path is < 255, but from somewhere else these path might be longer (or shorter).

check the depth of the install path and issue a warning or something.

Again, this is not ideal for the same reason. The path that my PHP application is seeing might not be the true path to the files. Still, what will the warning limit be? The truth is that even with the current patch it is not 100% sure that the max path length will be never exceeded, it just reduces - a lot - the probablity of that happening.

if we decided to only support some new version of windows that supported large paths

Actually windows supports long paths, even longer than *nix. It's not the version of Windows that matters, it's the PHP usage of the Windows API's.

I believe we should not be that picky and get this fixed ASAP because anyone running D8 on Windows is getting warnings out of the box. After all there are legacy - and not so legacy - parts in core that have much much more formal issues than the ones being raised here. If these warnings were being thrown on *nix I am sure that this issue would have been raised to critical a long time ago.

@alexpott, thank you for the review! Some comments to add to what @david_garcia wrote in #67:

Did we consider making this a constant on PhpStorageInterface? And documenting that this is used to keep total file path lengths small enough for windows.

Agreed, the documentation was missing. I've added that to this patch.

No, I didn't consider putting it in PhpStorageInterface -- I felt that was putting it at too abstract a level since it was only used in one implementation of that interface.

What we do for cache keys - where the max length is 255 is only hash once the path is longer than

I don't think that works here. MTimeProtectedFastFileStorage is using the directory's mtime() as part of the hash to ensure a file hasn't been changed maliciously. It would add a lot of complexity to determine the realpath to the filie and only hash if we're past some limit.

I'm pondering what is going to update on existing sites when they apply this patch. Should there be a hook_update_N to clear the existing twig cache files... and what does that mean for multiple web heads.

If a drush cr can be assumed during the update, there will be no interruption and stale files will not be left behind in the /files/php/twig directory. If there is no cache-rebuild, then the next request will be a cache-miss for the Twig files and they will be regenerated alongside the old version of the file. From the end-user's perspective, there is no change. From the developer's perspective, we will leave old cache files lying around until the next cache-rebuild.

As to multiple web heads, this is starting to get above my pay-grade... :)

I believe it won't make any difference -- if they have it working for multiple web heads to pull from a single file cache, then it should work the same as the single web head.

Typo (x2): fxied fixed

@mstrelan, thank you for catching that. I fxied it... :)

Removing the Needs Tests tag since there are tests and there isn't a way to add a test that we don't go past 260 characters since we'll never know how deep in the file system Drupal is installed.

@mikeker we can know exactly how deep drupal is installed - php has a realpath function and this issue should be adding a warning to windows user once DRUPAL_ROOT is over 10(or whatever people think is best) deep.

@alexpott, Fair enough, re: realpath. We need to keep in mind that the warning will have to be somewhat vague as we have no idea how long Twig template names will be. Currently the longest in core (excepting test templates) is 48 chars (config_translation_manage_form_element.html.twig). The cached version of that template, by default and with this patch, would reside in sites/default/files/php/0123456789_config_translation_manage_form_element.html.twig_0123456789/0123456789.php (109 chars).

Caveats: default can be replaced with subdomain.example.com, with template suggestions the filename length can grow substantially, and contrib could add very long template names. (I suppose contrib could also shorten those long template names.)

I'll add a install warning if realpath of DRUPAL_ROOT is > 100 chars. How does that sound? Is there any possible way to add a test for this?

I'll add a install warning if realpath of DRUPAL_ROOT is > 100 chars

+= "on Windows."

I've added an install warning if realpath(DRUPAL_ROOT) is greater than 100 characters on Windows only:

Since this adds new strings for translation, I assume that means it's bumped to 8.1.x? (Not sure so I'm not changing it for now).

8.1 yes. Got find a vm to test but 100 seems reasonable considering the core template + suggestions

@joelpittet, thanks for clarifying the 8.0.x vs 8.1.x.

Sorry should've mentioned that I tested this briefly on Windows10 running Apache 2.4.4, PHP 5.5, and MySQL 5.6. Two scenarios: one with Drupal root at 100-chars deep and another at 101-chars. Verified the warning show at 101 chars, and does not at 100 chars, and the install completes without issue in both situations.

With the 101-char deep install, the longest Twig cache path I saw generated was 204 chars long:

c:\Users\Mike\Documents\dev\drupal-core\123456789-123456789-123456789-123456789-123456789-12345678901\sites\default\files\php\twig\dc9af919_block--system-messages-block.html.twig_9e48272a34\cfbae4cd7f.php

That leaves us with 56 characters of breathing room to handle a long domain name replacing default or some crazy template suggestion strings. I think 100 chars is a good cutoff for the warning.

RTBC again

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -47,6 +47,17 @@ class MTimeProtectedFastFileStorage extends FileStorage {
+  protected $maxHashLength = 10;

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -44,6 +44,17 @@ class TwigPhpStorageCache implements \Twig_CacheInterface {
+  protected $maxHashLength = 10;

#65 still has not been addressed. I understand the reasons given #68 but they should still be a class constant rather than a property. It is not has if this is something that should be changed mid request.

Slightly different approach. I can think of a few places - at least in contrib - that would benefit from using this short hash thing.

I like this new approach! I think we should add comments to places where it is used as to why we're doing it here.

Drive by code style review...

1. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -136,4 +141,24 @@ public static function randomBytesBase64($count = 32) {
   +   * ¶
   ...
   +   * @param string $key ¶
   

   Lines end with a space.

2. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -136,4 +141,24 @@ public static function randomBytesBase64($count = 32) {
   +   * @return string
   

   Need a new line before the @return and it needs a description.

3. Need to test the new method in \Drupal\Tests\Component\Utility\CryptTest

Adressed everything from #83.

Was not sure what to test, just made sure that the method does what it has to do - not how it does it.

Let's see what the testbot thinks about it.

Fixing the test...

Tested this manually, everything look OK.

Thanks!

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -136,4 +141,26 @@ public static function randomBytesBase64($count = 32) {
+    if ($key) {
+      return substr(base_convert(hash_hmac('sha256', $name, $key), 16, 36), 0, $length);
+    }
+    else {
+      return substr(base_convert(hash('sha256', $name), 16, 36), 0, $length);
+    }

This is problematic: base_convert loses precision with large numbers (see #61). I was OK with it for MTimeProtectedFastStorage because we only needed "good enough" security there. But by putting this in Crypt, we're implying that this is good for use cases that require high precision.

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -136,4 +141,26 @@ public static function randomBytesBase64($count = 32) {
+   * Get a shortened sha256 hash.

Maybe we can mitigate the above concern with documentation? I think we should warn users of this function that a shortened hash is at greater risk of collisions. Though it is minimal at the default $length, this function shouldn't be used to hash passwords, for example.

This is problematic: base_convert loses precision with large numbers

Except for the weird base 32 case, I'm not sure we are really affected in this use case by the limited precision of floats in PHP.

Maybe we can mitigate the above concern with documentation?

Agreed.

Attached patch adds some docs and change the $name parameter to a more generic $data since this function is no longer limited to Twig caches.

Also

Except for the weird base 32 case, I'm not sure we are really affected in this use case by the limited precision of floats in PHP.

goes above my pay-grade. :)

I'll look into it further, but if anyone has any insights, I would love to hear it.

Great!

Tagging with "rc target triage" to discuss with the other core committers about whether we should commit this to 8.1.x. In my mind this bug has work arounds but it might be very hard to work out what you need to do. And just installing a new module might cause the bug on an otherwise working site.

@effulgentsia and I reviewed this patch and scanned the issue. Overall, we are comfortable promoting this to a formal rc target, but neither of us were confident in committing it outright without extra time to do a deep dive, given that 8.1.0 is less than a day away.

So removing the "triage" part of the tag, but leaving RTBC for now.

1. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -135,7 +137,8 @@ public function getFullPath($name, &$directory = NULL, &$directory_mtime = NULL)
   -    return $directory . '/' . hash_hmac('sha256', $name, $this->secret . $directory_mtime) . '.php';
   +    $hashed_name = Crypt::shortHash($name, $this->secret . $directory_mtime) . '.php';
   +    return $directory . '/' . $hashed_name;
   

   I'm not comfortable committing this line. This is the line for which this issue is tagged "needs security review", and in my opinion, 10 characters (40 bits ~50 bits with base36) is too little protection against arbitrary code execution on the server. Leaving at RTBC though in case another committer disagrees and wants to commit anyway.

2. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
   @@ -72,7 +73,7 @@ protected function storage() {
   -    $hash = hash('sha256', $className);
   +    $hash = Crypt::shortHash($className);
   

   This hash doesn't have a security requirement, so I'm fine with this one being shortened. But, why do we need a hash here at all? Can we shorten it to 0?

3. +++ b/core/includes/install.core.inc
   @@ -2105,6 +2105,25 @@ function install_check_requirements($install_state) {
   +  // up in cached Twig templates. Because of their variable length, for example
   +  // template suggestions can arbitrarily lengthen a filename, there is no set
   

   Can we control for this, like only use the debug-friendly filename if it's shorter than say 32 (or whatever arbitrary number), but if the template name is longer, then use a 32 character hash instead (or 21 characters from the template name and a 10 character hash)? I think all core template names are less than 32, and for a few contrib ones that are longer, I don't think the loss to debuggability is all that bad, since the original name is in the contents of the file and can be found in IDEs that way. Also if the cutoff number is a setting, then people wanting to debug those long template names can increase it, especially if they're not on Windows. Note that I think we can remove the ".html.twig" portion from the name too, since that's 10 extra characters for no benefit.

re #96.1 @effulgentsia isn't the protection on offer not about the length but about hashing the mtimes?

Re #97, yeah that's a fair point. But the length matters against a brute force attack. Assuming the server has a file upload vector, could someone attempt a quadrillion uploads in order to eventually succeed? I guess I could be convinced that that's sufficiently low risk, but per #96.2 and #96.3, is it a necessary one?

From #96.2:

But, why do we need a hash here at all?

Oh, I see now, that's a hash of the classname, not the template name. And you can have a situation of 2 different templates with the same template name (e.g., from different themes) co-existing on the same site. So yeah, we do need a hash there. 10 characters seems more than fine to protect against that collision, since in the 1 in quadrillion case of such a collision, all that happens is you get back markup from the wrong theme, which seems like a very low severity consequence of such an unlikely event, unlike the hash in #96.1, which has a much higher severity consequence from collision.

Per #98, it would also be possible to do the shortening only on Windows if that's a concern.

I'd really like to see a review from Heine here if he has time.

Also by default we use Drupal\Component\PhpStorage\MTimeProtectedFileStorage so even if an attacker manages to write to a file they'd need to also somehow keep the same mtime.

With \Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage if they write another file to the folder it'd break stuff... they'd have to write to the file that exists.

From #100:

it would also be possible to do the shortening only on Windows

Respectfully, I really don't like this idea. If the security is not good enough for Linux I believe it's not good enough for Windows as well. i don't like the idea of making Windows (even more of) a second-class citizen.

From #96 and #98:

This is the line for which this issue is tagged "needs security review", and in my opinion, 10 characters is too little protection against arbitrary code execution on the server.

But the length matters against a brute force attack. Assuming the server has a file upload vector, could someone attempt a quadrillion uploads in order to eventually succeed?

Assuming a file upload vulnerability, an attacker would have to upload a file with the same first 50-or-so bits matching the hash within the same second as Drupal caches the template since the directory's mtime is the secret when creating the hash. (mtime is granular to one second.)

Discussed @catch, @effulgentsia, @xjm , @cottser, @laurii, @joelpittet and @Wim Leers. We agreed this is a major issue because without this change windows users get unexpected errors.

Respectfully, I really don't like this idea. If the security is not good enough for Linux I believe it's not good enough for Windows as well. i don't like the idea of making Windows (even more of) a second-class citizen.

Well it's a second class citizen because it can't handle reasonable file name lengths - this is more like progressive enhancement on linux, it's not penalizing Windows.

Moving back to CNR, I'd really like to see Heine's thoughts on this issue.

Would it be possible to save (a part of) the needed hashes inside the file?

This is an issue when installing 8.2.x on Windows when using Acquia Dev Desktop, which is part of the tools set used in the First Time Sprint Workshop and Mentored sprint. As such, every windows user during the DrupalCon NOLA First time sprint will experience this issue.

@e.r.n.i.e: I suppose that's possible but I'm concerned about performance issues related having to open and read each file to validate the hash. Currently, we just calculate the hash and include the file. If the directory's mtime has changed then the hash will be different and the include will fail.

Adding the full hash to the file itself will require us to open the file, parse and remove the full hash, validate the full hash, and then include the file. I'm concerned this could be a performance issue.

@Deciphered: Agreed. I would love to get this fixed before the Friday sprints.

Can we only hope this is implemented sanely: http://mspoweruser.com/ntfs-260-character-windows-10/

Please MS don't mess it up.

I honestly don't understand why we don't swap out the implementation for windows - the proposed fix still doesn't warn Windows users when a file name is too long - it just makes it less likely.

One of the test files was moved so the patch did not apply anymore.

Made a couple of more changes:

* Crypt::shortHash now throws an Exception when $length > 50 (that is the length of the sha256 after changing base)
* Default length for Crypt::shortHash is no longer a constant in the Crypt class

Just FYI this is not only a Windows issue: if you are in Linux and have the filesystem where you have the site mounted using eCryptfs and its configured to encrypt filenames as well as the file content you will experience this same issue. You can read a little about this limitation here.

Tested on Windows. Works for me.

+++ b/core/includes/install.core.inc
@@ -1938,6 +1938,24 @@ function install_check_translations($langcode, $server_pattern) {
+  // up in cached Twig templates. Because of their variable length, for example
+  // template suggestions can arbitrarily lengthen a filename, there is no set
+  // limit beyond which Drupal will not function correctly on Windows. Thus we

This sentence "Because of their variable length, for example template suggestions can arbitrarily lengthen a filename, there is no set limit beyond which Drupal will not function correctly on Windows." is missing a comma or something because it doesn't make sense. I am also not ok with "set limit" because it is vague. "Predictable root directory depth" or similar would make more sense.

@yurii_2016 In the time it took to write the comment you then deleted when you asked me "ARE YOU NUTS" you could have written a patch. Just to demonstrate, this took three minutes of my time.

@yurii_2016 Please read the Drupal Code of Conduct carefully.

This is a collaborative work and everyone brings their best in it. But your comments are not motivating despite you think they are. They are even not useful in any kind.

P.S.:

i do not write patches, i'm against it.

That says it all about you as a "PHP certified engi".

1. I've been thinking about this change for the past week or so. I think minimising the path length makes sense, if we can do so in a way that does not impact security. Especially given that we might not just be talking about windows - see #116.
2. We're still missing the security review of the changes to mtimeprotected storage as discussed in #98. This needs to be addressed in a definitive comment before commit. The best answer so far seems to be #103.

3. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -168,4 +168,34 @@ public static function randomBytesBase64($count = 32) {
   +   * @param int $length
   +   *   Hash length. Defaults to 10 characters. Maximum 50 characters.
   

   This parameter is not tested.

4. +++ b/core/lib/Drupal/Component/Utility/Crypt.php
   @@ -168,4 +168,34 @@ public static function randomBytesBase64($count = 32) {
   +    if ($length > 50) {
   +      throw new \InvalidArgumentException(sprintf('shortHash length %s is longer than the maximum allowed of 50.', $length));
   +    }
   

   This exception is not tested.

Thank you @alexpott and @cilefen for the reviews.

From #118:

This sentence ... is missing a comma or something

Yikes, not the best example of my writing skills! :) Updated to read:

  // Installations on Windows can run into limitations with MAX_PATH if the
  // Drupal root directory is too deep in the filesystem. Generally this shows
  // up in cached Twig templates because template suggestions can arbitrarily
  // lengthen a filename. There is no predictable root directory depth beyond
  // which Drupal will not function correctly on Windows, thus we only warn if
  // Drupal is being installed more than 100 characters deep in the filesystem.

From #124:

1. Yea!
2. I emailed the security team during DrupalCon NOLA asking for input on this issue. I can email them again, unless someone knows a better way to contact them...
3. Fixed.
4. Fixed.

We're still missing the security review of the changes to mtimeprotected storage as discussed in #98. This needs to be addressed in a definitive comment before commit. The best answer so far seems to be #103.

This will make the issue stall forever, and everything points to the changes being proposed here not affecting security at all.

1. I'm going to ping the security team in #drupal-security and see if I can raise a response. I'll try and timebox it and also get @catch re-involved. I'm +1 on the change.

2. +++ b/core/includes/install.core.inc
   @@ -1940,6 +1940,23 @@ function install_check_translations($langcode, $server_pattern) {
   +  if (strpos(strtolower(php_uname('s')), 'win') === 0) {
   

   Elsewhere we use if (substr(PHP_OS, 0, 3) == 'WIN') { for this. See file_create_filename() for example. This would be the first usage of php_uname() in core.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptTest.php
   @@ -80,6 +80,42 @@ public function testHmacBase64Invalid($data, $key) {
   +   * @expectedException InvalidArgumentException
   

   We don't use annotations anymore for this see #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException - also we should assert on the message too.

Re-titling since this doesn't just affect Windows per #116.

Thanks for the review @alexpott. I'm looking forward to hearing from the security team on this.

This would be the first usage of php_uname() in core.

Yeah, I've got no idea why I used that instead of PHP_OS... I had to look it up to see what it returned! :)

Fixed along with the exepectedException change, also asserting the exception message.

Unrelated test fail it looks like. Tests look to be passing, I'll rerun them.

Back to what I said in #126 :)

It's also not clear to me why the few char gain here makes it the right approach. Also - using base 36 makes the hash longer than using the existing base64 encoding, what am I missing?

It seems like this is a more a bug in Twig if we are getting very long filenames. Why not hash those (or partially like cache IDs) so we have constant lengthor a defnitive maximum length filename for all twig templates?

see \Drupal\Core\Cache\DatabaseBackend::normalizeCid()

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -130,7 +132,8 @@ public function getFullPath($name, &$directory = NULL, &$directory_mtime = NULL)
-    return $directory . '/' . hash_hmac('sha256', $name, $this->secret . $directory_mtime) . '.php';
+    $hashed_name = Crypt::shortHash($name, $this->secret . $directory_mtime) . '.php';
+    return $directory . '/' . $hashed_name;

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -131,4 +131,35 @@ public static function randomBytesBase64($count = 32) {
+  public static function shortHash($data, $key = NULL, $length = 10) {
...
+    if ($key) {
+      return substr(base_convert(hash_hmac('sha256', $data, $key), 16, 36), 0, $length);
+    }

@pwolanin - the hash_hmac in the original code is not replace by a simple hash.
I think mixing hash_hmac function into this method makes it easier to make mistakes. We should two methods shortHash and shortHmac so that it is clear what you want to use and why.

Discussed at length with @pwolanin in #drupal-security. He pointed out that we should use \Drupal\Component\Utility\Crypt::hmacBase64() for shorter security hashes - therefore we don't lessen the security in windows or linux.

I also think it is worth separating the security change from the rest of the change here to twig. @pwolanin also made some sensible suggestion that this issue takes into account the length of the filename and hashes it once over a certain length. That way we can know the length of the all relative paths to twig files because they can't be longer. And then we can add a better check in system_requirements().

In order to separate out the security issue from the changes which are not security related I've created #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily

The compiled Twig files seem to have a comment specifying the original template, like:

/* core/themes/bartik/templates/form--search-block-form.html.twig */

So I do't think it's very important to have the template name in the directory name also, since a simple grep will find the filename of interest if it exists.

Patch attached removes the security related changes since they are being handled in #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily and also implements the logic similar to \Drupal\Core\Cache\DatabaseBackend::normalizeCid() to hash the filename if it gets to long. Also even without the security changes the patch attached with result in d8 working on windows as long as the root path is not more than 100 - which is what is being checked for in system_requirements().

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -78,9 +79,22 @@ public function generateKey($name, $className) {
+    // - 69 (/ + hmac_hash + .php)

With #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily this comes down to 48 giving us even more wiggle room.

Please remove the added static function Crypt::shortHash()

If you want to add to core a feature of proving substrings of hashes, I'd add an optional 2nd param to \Drupal\Component\Utility\Crypt::hashBase64() to specify the number of bytes of the hash output to use.

Given that this is not a very common need - we can just take the substring in this twig code I think, rather than baking it into core.

It can maybe be this simple?

produces a file listing like:

$ ls -1 sites/default/files/php/twig/
a649bad4_block--system-messages_nMgDJ2jz8KvAWj5uTds2nw
a649bad4_block.html.twig_7TZ-JuAm7LEEf5XTrx956z
a649bad4_breadcrumb.html.twig_Svk-l6odkj-fQoJxhU_BPl
a649bad4_container.html.twig_BZwzrIs-hSk82zofi2RtmE
a649bad4_details.html.twig_6L_zVxUpvZnTce3l9mvuw2
a649bad4_form-element-label.htm_Nyjnd4CuhRX15Wp04lTZLz
a649bad4_form-element.html.twig_FvXFVKo5G6JPAoh8c4u4cc
a649bad4_form.html.twig_Rxe6z2RwLmxc8MSM4oMQ3L
a649bad4_html.html.twig_LKAodGD5X4CPPzvdeExYbv
a649bad4_input.html.twig_wxjlaXL7USBwPC7bZfHfP7
a649bad4_links.html.twig_Uzbu8AnPHeILkDuN4ASyfR
a649bad4_menu--toolbar.html.twi_fBR4MKZXNUBtNXwrS5173H
a649bad4_page-title.html.twig_bFvt_P5mzW8AAGJs-X2LfK
a649bad4_page.html.twig_HtGmNlnAHPGZC2T-iUtMWH
a649bad4_region.html.twig_pKduiTm1x-Ul3mbsEmqZjm
a649bad4_select.html.twig_H6A34Bjo6d_SfwdllOl0OP
a649bad4_status-messages.html.t_h1NFrBKV6-51vIkG2aYV9h
a649bad4_system-config-form.htm_EuY2MCFGxHzGS3KVFzjqwt
a649bad4_toolbar.html.twig_3apNgls0fPSX998wKSPIr2

@pwolanin that looks really good to me. With this patch we're guaranteeing that with a usual set up the warning adding to system_requirements() is relevant and we're not introducing any security differences between windows and other servers.

1. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
   @@ -67,7 +68,8 @@ protected function storage() {
   +    $hash = substr(Crypt::hashBase64($className), 0, 22);
   

   Why 132 bits?

2. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
   @@ -78,9 +80,22 @@ public function generateKey($name, $className) {
   +    if (strlen($name) > 22) {
   +      $name = substr($name, 0, 22);
   +    }
   

   We need to test this shortening explicit somewhere. And we probably need a change record to announce the change in twig filenames.

Also this issue should add an empty post update function to system.post_update.php to force a cache clear so that all the old twig templates are removed.

If we assume #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily is going to be committed first (?) we can extend the hash and name parts by a few more characters.

Here's a patch with that assumption (uses a substring of 30) plus test.

Oh wait - I added an empty update function - which is the right approach? Do we always want to clear the twig cache after updates?

@pwolanin a post update to clear the cache is fine.

@pwolanin I wouldn't make the assumption about #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily landing first. Also I think having extra "wiggle" room is fine too. Because of the "sites/default/files" assumption... if people are doing multisite then that is going to grow.

@alexpott - why do we even need to clear the cache? If the expected file is not found I think Drupal just generates it.

Here's with a class const to avoid magic numbers in code.

We should clean up the files cause leaving around the old files is super confusing for people.

ok, then let's make sure #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily goes in 1st (since it's really a trivial patch) and we'll add the post-update here.

1. +++ b/core/includes/install.core.inc
   @@ -1940,6 +1940,23 @@ function install_check_translations($langcode, $server_pattern) {
   +  // Installations on Windows can run into limitations with MAX_PATH if the
   +  // Drupal root directory is too deep in the filesystem. Generally this shows
   +  // up in cached Twig templates and other public files with long directory or
   +  // file names. There is no definite root directory depth below which Drupal is
   +  // guaranteed to function correctly on Windows, but we start warning when
   +  // there are more than 100 characters in the Drupal root path.
   +  if (substr(PHP_OS, 0, 3) == 'WIN') {
   +    $depth = strlen(realpath(DRUPAL_ROOT));
   +    if ($depth > 100) {
   +      $requirements['max_path_on_windows'] = [
   +        'title' => t('Windows installation'),
   +        'description' => t('The path length to the Drupal root directory (@depth characters) is greater than 100 characters. This may cause problems when running on a Windows server. It is recommended to install Drupal on Windows in a shorter path.', ['@depth' => $depth]),
   +        'severity' => REQUIREMENT_WARNING,
   +      ];
   +    }
   +  }
   +
   

   I think this belongs in system_requirements() since sites can be moved. Also on install it should be an error not a warning.

2. +++ b/core/modules/system/system.post_update.php
   @@ -57,3 +57,11 @@ function system_post_update_add_region_to_entity_displays() {
   +
   +
   +/**
   + * Force Twig php file cache to be cleared.
   + */
   +function system_post_update_clear_twig_cache() {
   +  // Empty post-update hook.
   +}
   

   This can be removed as the post update has been committed in the other patch.

Ok, moving the requirements check. The other change doesn't show up in the interdiff since it was rebased away.

The issue summary needs to be updated too.

1. +++ b/core/modules/system/system.install
   @@ -904,6 +904,23 @@ function system_requirements($phase) {
   +        'severity' => REQUIREMENT_WARNING,
   

   I think we should make this an error during the install and a warning post install. If we prevent install in too deep paths then people have the chance to fix it. A warning means they can continue but their site will be broken. Hmmm... actually maybe this should always be an error. Since if you move a site to a place where the directory length is greater than 100 you are still going to have problems. Not sure.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigEnvironmentTest.php
   @@ -116,6 +118,19 @@ public function testCacheFilename() {
   +    $expected = strlen($hash) + 2 + 2 * TwigPhpStorageCache::SUFFIX_SUBSTRING_LENGTH;
   +    $this->assertEquals($expected, strlen($key));
   

   Thanks for adding the test - nice one.

ok, here's making is an error and slight rewording of the comment.

The latest work is feeling sensible to me.

1. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
   @@ -79,8 +83,20 @@ public function generateKey($name, $className) {
   +    // If the suffix is longer that 51 characters then we truncate it so the
   +    // length of the filename can't grow beyond that. Windows only supports 255
   +    // characters in a path. If the files directory is in the usual place of
   

   "Windows supports up to 255 characters..."

2. +++ b/core/modules/system/system.install
   @@ -904,6 +904,23 @@ function system_requirements($phase) {
   +  if (substr(PHP_OS, 0, 3) == 'WIN') {
   

   Do we want to give consideration to warning on other filesystems such as mentioned in #116?

@cilefen the issue there is the file name length not the root dir depth, so for now I think this is ok for just windows (especially since the trunation means we won't grow file names without limit).

Also with windows we know we have the issue 100% of the time - any other OS we know we don't have the issue 95+% of the time so I think only doing the notice windows is totally acceptable.

Noting some impact from #2569119: Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens which would increase the twig extension hash length.

Ok, that issue we've reduced the change to a few extra chars, so it's won't undo the work here.

I'm gonna look at this and also check what remaining tasks there might be.

Read the whole patch, also applied it and looked at the changes in context.

Just some nits (non-commit blockers: order of use statements and comment ending with punctuation) real quick (only in lines already being changed by the patch).

Next I will update this comment

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -67,8 +73,6 @@ protected function storage() {
     if (strpos($name, '{# inline_template_start #}') === 0) {
       // $name is an inline template, and can have characters that are not valid
       // for a filename. $hash is unique for each inline template so we just use

which needs updating since we removed the $hash var.

I think just replacing $hash with $suffix works to keep the intended meaning. (made 80 char re-wrapping needed.)

next I'm going to try and verify the numbers here.

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -79,8 +83,20 @@ public function generateKey($name, $className) {
+    // If the suffix is longer that 51 characters then we truncate it so the
+    // length of the filename can't grow beyond that. Windows only supports 255
+    // characters in a path. If the files directory is in the usual place of
+    // 'sites/default/files' then the maximum relative path of a twig file is
+    // 153 characters. We use 138 of that as follows:
+    // - 30 (/sites/default/files/php/twig/)
+    // - 9 (prefix + _)
+    // - 51 (suffix)
+    // - 48 (/ + hmac_hash + .php)

I think it's a great comment, just want to see where those come from (which will verify, and also help know when this might need to be updated, if know where they come from).

#166 works perfectly for me!

The issue is focused on Windows OS, but I got the same error running drupal 8 in a docker container (that uses AUFS).

@manuel.adan - thanks for the new information. AUFS seems to have two limits - see http://aufs.sourceforge.net/aufs3/man.html and search for 255 - the other limit is 242. Which one is affecting you? And does the latest patch on this issue fix it for you?

I have this issue with Docker, also. I'm using Kalabox for local development of sites on Pantheon, and running into these errors regularly. Additionally, on the Pantheon platform itself we're seeing these errors sometimes it seems (likely for the same reason). I haven't had a chance to test this patch yet but when I do I'll report back.

@alexpott as you mentioned, max filename size in AUFS is 242

Currently patch from #115 works for me on docker.

reroll. was a conflict with #2569119: Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens ($hash is now $prefix in TwigEnvironmentTest.php)
and a testsonly patch.

@manuel.adan can you try #172 (171)?

Fixes the tests from #171. We were using the Twig extension hash instead of the Twig prefix.

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -67,20 +73,30 @@ protected function storage() {
+    // If the suffix is longer that 51 characters then we truncate it so the
+    // length of the filename can't grow beyond that. Windows only supports 255

Typo: It should be "...longer than...". Why not "Truncate the suffix to a maximum of 51 characters."?

+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -67,20 +73,30 @@ protected function storage() {
+    // 153 characters. We use 138 of that as follows:
+    // - 30 (/sites/default/files/php/twig/)
+    // - 9 (prefix + _)
+    // - 51 (suffix)
+    // - 48 (/ + hmac_hash + .php)
+    $suffix = substr($name, 0, self::SUFFIX_SUBSTRING_LENGTH) . '_';

I am trying to understand this part of the comment, which like all comments, is an effort to be helpful when code is confusing. The maximum is 153, we are handling 51 of them right here, and we are using 138 ("as follows"). It begs the question of where the other 15 may come from.

@cilefen, thanks for the review!

First off, I updated the tests-only patch so that it fails on the added tests and not a syntax error....

I've tried rewriting the comment you mentioned so it makes more sense. However, I wasn't the one that added the comment so I'd appreciate someone ensuring I haven't lost any important details.

Is there a quickfix for completely disabling the hash suffix?

Not exactly a "quick" fix, but the Twig cache is a service that can be replaced like any other service in D8. You could extend TwigPhpStorageCache and override the generateKey method.

After discussion with cilefen, it doesn't seem like enumerating all the path lengths is really useful as a code comment and will be hard to maintain. We cna add it to the change record instead.

Here's a reword to try to make the comment clearer and also reflect the change to a requirement error.

Oops - actually it's 142 long now due to the prefix change in #2569119: Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens

CI seems to have some java error.

and... off by 1. forgot the leading slash. It's 143

Test failure seems unrelated -- re-queuing.

Given the docker issues, maybe we should have a requirements error on every platform at >= 100 root depth?

Or at least a requirements warning for non-Windows?