Problem/Motivation

A D8 install can easily generate extremely lengthy file/directory names when caching Twig templates.

On some scenarios, the maximum length for a path might be limited by the OS:

  • Windows has a maximum path length of 260 characters. Can read more here.
  • 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.

To reproduce this issue on Windows, add the following to settings.php.

$settings['file_public_path'] = 'sites/default/files/0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-0123456789-01'; // That's a 255-char dir name.

Proposed resolution

Update as of #144 going with option #3

Option #3: Trigger a requirement error on windows if the Drupal root is deeper than 100 characters. Use our existing base64 hash and hmac functions so the hashes are half as long to start. Truncate the human-readable template name portion and hash portion of the to a maximum length so the directory names can never be longer than 51 characters. The cache file name itself has already been shortened to 47 characters using the bas64 hmac in the split-off child issue #2830596: MTimeProtectedFastFileStorage::getFullPath() creates really long filenames unnecessarily

Update: as of #39, options #2 is the preferred solution.

Option #1: Use the Unicode path prefix (\\?\) on Windows to avoid the 260 character limit (pushes the limit to around 32k which is well above Linux's limit of 4k characters). Unfortunately, PHP's support of these pathnames is limited. (see #16) which would require a much more disruptive fix.

Option #2: Use shorter hashes while retaining the "uniqueness" of these directory and file names. However, the filename hash contains the filemtime() of the containing directory. If, for example, file uploads were compromised and malicious code added to a cached Twig template, the containing directory's filemtime would change and the hash would not match. Using shorter hashes could potentially increase the risk of a compromised file still passing the hash test. (Security review requested).

Also, option #2 does not solve the underlying problem, but hopefully gives us some breathing room. Currently, there are two 64-character hashes in each cached Twig template.

Remaining tasks

As of #12:

  1. not needed since we're using option #2: FileStorage::unlink does not work with the Unicode prefix
  2. Needs security review to ensure that shortened hashs still give enough "uniqueness" for MTimeProtectedFastFileStorage
  3. Needs tests to assert that generated files do no exceed a (somewhat arbitrary) limit
  4. Done: Add hook_requirements to warn users when installing Drupal on Windows with a long path name.

User interface changes

Adds a new warning when installing Drupal on Windows at a path > 100 characters.

API changes

With option #2, none.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#218 interdiff.txt888 bytesxjm
#218 twig-2606772-218.patch5.68 KBxjm
#210 2606772-error.png28.63 KBmikeker
#205 2606772-187-205.interdiff.txt2.77 KBmikeker
#205 2606772-205-shortened-hashes.patch5.76 KBmikeker
#187 increment-2606772-187.txt938 bytespwolanin
#187 2606772-187.patch5.81 KBpwolanin
#186 2606772-186.patch5.81 KBpwolanin
#184 increment-2606772-184.txt1.54 KBpwolanin
#184 2606772-184.patch5.81 KBpwolanin
#180 2606772-176-180.interdiff.txt1.44 KBmikeker
#180 2606772-180.patch5.75 KBmikeker
#180 2606772-180-test-only.interdiff.txt1.58 KBmikeker
#180 2606772-180-test-only.patch2.7 KBmikeker
#176 2606772-171-176.interdiff.txt1.36 KBmikeker
#176 2606772-176.patch5.71 KBmikeker
#176 2606772-176-test-only.patch1.43 KBmikeker
#172 2606772-171.patch5.69 KBYesCT
#172 2606772-171-testsonly.patch2.1 KBYesCT
#166 interdiff.2606772.165.166.txt840 bytesYesCT
#166 2606772-166.patch5.68 KBYesCT
#165 interdiff.2606772.157.165.txt1.72 KBYesCT
#165 2606772-165.patch5.52 KBYesCT
#157 increment-2606772-157.txt1.34 KBpwolanin
#157 2606772-157.patch5.54 KBpwolanin
#155 increment-2606772-155.txt2.56 KBpwolanin
#155 2606772-155.patch5.53 KBpwolanin
#153 increment-2606772-153.txt1.32 KBpwolanin
#153 2606772-153.patch6.13 KBpwolanin
#12 2606772-12-max-path-windows.patch7.96 KBmikeker
#50 2606772-48-50.interdiff.txt1.63 KBmikeker
#50 2606772-50-shortened-hashes.patch4.5 KBmikeker
#63 2606772-63-shortened-hashes.patch3.84 KBmikeker
#63 2606772-50-63.interdiff.txt2.65 KBmikeker
#68 2606772-68-shortened-hashes.patch4.22 KBmikeker
#68 2606772-63-68.interdiff.txt1.41 KBmikeker
#70 2606772-70-shortened-hashes.patch4.22 KBmikeker
#70 2606772-68-70.interdiff.txt1.46 KBmikeker
#74 2606772-70-74.interdiff.txt1.34 KBmikeker
#74 2606772-74.png13.7 KBmikeker
#74 2606772-74-shortened-hashes.patch5.57 KBmikeker
#82 2606772-82-shortened-hashes.patch6.34 KBdavid_garcia
#84 2606772-82-shortened-hashes.patch8.31 KBdavid_garcia
#86 84-86-interdiff.txt2.69 KBalexpott
#86 2606772-86.patch8.73 KBalexpott
#91 2606772-91-shortened-hashes.patch8.95 KBmikeker
#91 2606772-86-91.interdiff.txt1.47 KBmikeker
#115 2606772-115-shortened-hashes.patch8.94 KBdavid_garcia
#120 interdiff.txt969 bytescilefen
#120 long_twig_cache-2606772-120.patch8.94 KBcilefen
#125 2606772-120-125.interdiff.txt4.67 KBmikeker
#125 2606772-125-shortened-hashes.patch9.89 KBmikeker
#129 2606772-125-129.interdiff.txt1.61 KBmikeker
#129 2606772-129-shortened-hashes.patch10.11 KBmikeker
#141 129-141-interdiff.txt6.83 KBalexpott
#141 2606772-2-141.patch8.01 KBalexpott
#144 2606772-144.patch3.98 KBpwolanin
#144 increment-2606772-144.txt7.57 KBpwolanin
#147 2606772-147.patch5.21 KBpwolanin
#147 increment-2606772-147.txt3.8 KBpwolanin
#151 2606772-151.patch5.56 KBpwolanin
#151 increment-2606772-151.txt4.25 KBpwolanin

Comments

hatuhay created an issue. See original summary.

marcingy’s picture

Priority: Critical » Major
Status: Active » Postponed (maintainer needs more info)

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

hatuhay’s picture

Installation done with
https://bitnami.com/redirect/to/75604/bitnami-drupal-8.0.0.rc2-0-dev-windows-installer.exe
Running under Apache and PHP 5.5.30
Reinstall Drupal 8 to reproduce issue.
With fresh installation, warnings still appearing, but site is up.
3b9a7135_block--system-messages-block.html.twig_1b89fea44fd124de67ee5957081934a6aa2d86b3295c1a8dd2211f15a8f19efb folder is empty and temporary twig files that cannot be renamed, remain on \sites\default\files\php\twig
This is the complete error message:
Warning: rename(sites/default/files/php/twig/.f4c86f3b64,sites/default/files/php/twig/3b9a7135_block--system-messages-block.html.twig_1b89fea44fd124de67ee5957081934a6aa2d86b3295c1a8dd2211f15a8f19efb/f0f090b909fe6a121bae2b9d5538964c7899b4a3f361bdd3c517cc83769742fc.php): No such file or directory en Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (linea 91 de C:\Bitnami\drupal-8.0.0.rc2-0\apps\drupal\htdocs\core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).
After installing same modules first time, site crashed.
Last module enabled admin_toolbar and admin_toolbar_tools.
I am leaving office now,so no more time for testing tonight, but apparently:

  • The site is crashing after installing admin_toolbar and admin_toolbar_tools, but unistalling both do not solve the problem.
  • From the beggining the twig cache files for block--system-messages-block.html.twig and block--system-branding-block.html.twig cannot be copied to their folders, but this is not crashing the site.
hatuhay’s picture

Status: Postponed (maintainer needs more info) » Active
Cottser’s picture

Title: Fail to rename block--system-messages-block.html.twig and block--system-branding-block.html.twig cache twig files » Long Twig cache directories can cause failures on Windows

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

Cottser’s picture

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.

Cottser’s picture

mikeker’s picture

Version: 8.0.0-rc2 » 8.0.x-dev
Assigned: Unassigned » mikeker
Issue tags: -Needs tests +Windows

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.

Cottser’s picture

@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".

mikeker’s picture

Issue tags: +Needs tests

@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.

Cottser’s picture

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.

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Active » Needs review
FileSize
7.96 KB

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.

mikeker’s picture

Issue summary: View changes
Issue tags: +rc target triage

Added issue summary and tagging for RC consideration.

joelpittet’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Issue tags: +Needs manual testing

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.

mikeker’s picture

@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!

greggmarshall’s picture

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.

hass’s picture

I have the same problem.

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

joelpittet’s picture

@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.

Cottser’s picture

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.

hass’s picture

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

Cottser’s picture

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

hass’s picture

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.

joelpittet’s picture

@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.

Cottser’s picture

@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.

hass’s picture

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.

Cottser’s picture

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.

catch’s picture

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

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

greggmarshall’s picture

@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.

joelpittet’s picture

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?

Cottser’s picture

@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
mikeker’s picture

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.

greggmarshall’s picture

@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.

Cottser’s picture

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

Yup.

catch’s picture

So why not:

Before:

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

After:

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

Cottser’s picture

@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).

catch’s picture

Oh.

greggmarshall’s picture

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.

mikeker’s picture

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).

mikeker’s picture

Status: Needs work » Needs review
mikeker’s picture

Issue summary: View changes

Updated IS with security review details.

catch’s picture

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.

mikeker’s picture

@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?

Status: Needs review » Needs work

The last submitted patch, 39: 2606772-39-shortend-hashes.patch, failed testing.

Cottser’s picture

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?

hass’s picture

How should i handle such an exception? :-)

Cottser’s picture

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.

mikeker’s picture

Status: Needs work » Needs review
FileSize
2.87 KB

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... :)

Status: Needs review » Needs work

The last submitted patch, 48: 2606772-48-shortend-hashes.patch, failed testing.

mikeker’s picture

ugh... Forgot the fixes to the tests.

Cottser’s picture

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 ;)

greggmarshall’s picture

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.

Cottser’s picture

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

mikeker’s picture

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.

mikeker’s picture

Issue summary: View changes

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

drupalfan2’s picture

This problem still seems to be part oft drupal 8.0.1.

What can I do?

mikeker’s picture

@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.

acrosman’s picture

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.

david_garcia’s picture

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

dawehner’s picture

+++ 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.

mikeker’s picture

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!

mikeker’s picture

Status: Needs review » Needs work
mikeker’s picture

Status: Needs work » Needs review
FileSize
3.84 KB
2.65 KB

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?

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

+++ 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.

alexpott’s picture

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.

david_garcia’s picture

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.

mikeker’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.22 KB
1.41 KB

@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.

mstrelan’s picture

Status: Needs review » Needs work

Typo (x2): fxied fixed

mikeker’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.22 KB
1.46 KB

@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.

alexpott’s picture

@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.

mikeker’s picture

Assigned: Unassigned » mikeker
Status: Needs review » Needs work

@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?

mikeker’s picture

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

+= "on Windows."

mikeker’s picture

Assigned: mikeker » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.34 KB
13.7 KB
5.57 KB

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).

joelpittet’s picture

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

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

mikeker’s picture

@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.

mikeker’s picture

Issue summary: View changes
xjm’s picture

Issue tags: -rc target triage
david_garcia’s picture

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

RTBC again

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
6.34 KB

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

alexpott’s picture

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
david_garcia’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 84: 2606772-82-shortened-hashes.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
8.73 KB

Fixing the test...

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

Tested this manually, everything look OK.

Thanks!

mikeker’s picture

+++ 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.

david_garcia’s picture

Status: Reviewed & tested by the community » Needs work

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.

mikeker’s picture

Assigned: Unassigned » mikeker
mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
8.95 KB
1.47 KB

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.

mikeker’s picture

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.

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Issue tags: +rc target triage

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.

webchick’s picture

Issue tags: -rc target triage +rc target

@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.

effulgentsia’s picture

  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.

alexpott’s picture

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

effulgentsia’s picture

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?

effulgentsia’s picture

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.

catch’s picture

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.

alexpott’s picture

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.

alexpott’s picture

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.

mikeker’s picture

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.)

lauriii’s picture

Issue summary: View changes
alexpott’s picture

Issue tags: +Triaged D8 major

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

e.r.n.i.e’s picture

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

Deciphered’s picture

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

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.

mikeker’s picture

@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.

ptsimard’s picture

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

Please MS don't mess it up.

ivanjaros’s picture

alexpott’s picture

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.

Anonymous’s picture

thanks for patch. Life was too easy, now we have twig..

ppl who write about brute-force attack.. pff, cache dir is protected from web access

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

david_garcia’s picture

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

cesarmiquel’s picture

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.

slasher13’s picture

Status: Needs review » Reviewed & tested by the community

Tested on Windows. Works for me.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Anonymous’s picture

nvm

cilefen’s picture

Status: Needs work » Needs review
FileSize
969 bytes
8.94 KB

@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.

slasher13’s picture

Status: Needs review » Reviewed & tested by the community
Anonymous’s picture

as edited says 'nvm'
i don't believe in 3 minutes. just don't lie. it should take at least 10 mins
also i do not write patches, i'm against it.

also, if you prove it is so easy why to do this https://www.drupal.org/node/2606772#comment-11776337 ? Guy has provided you a solution and after 1 year you type about comma? Omg man, thanks my last comment on Drupal.

see ya

p.s.
also it took 1 year and it will take another 6 month to commit this stuff eventually, according to my observations of drupal surroundings

pingwin4eg’s picture

@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".

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

mikeker’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
9.89 KB

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.
david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

catch’s picture

Title: Long Twig cache directories can cause failures on Windows » Long Twig cache directories can cause failures on some filesystems

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

mikeker’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
10.11 KB

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.

Status: Needs review » Needs work

The last submitted patch, 129: 2606772-129-shortened-hashes.patch, failed testing.

The last submitted patch, 129: 2606772-129-shortened-hashes.patch, failed testing.

The last submitted patch, 129: 2606772-129-shortened-hashes.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

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

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

Back to what I said in #126 :)

david_garcia’s picture

Issue summary: View changes
david_garcia’s picture

Issue summary: View changes
pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

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()

alexpott’s picture

+++ 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.

alexpott’s picture

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

pwolanin’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -needs security review
FileSize
6.83 KB
8.01 KB

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().

alexpott’s picture

+++ 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.

pwolanin’s picture

Status: Needs review » Needs work

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
7.57 KB

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
alexpott’s picture

Status: Needs review » Needs work

@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.

alexpott’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
3.8 KB

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.

pwolanin’s picture

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

alexpott’s picture

@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.

pwolanin’s picture

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

pwolanin’s picture

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

alexpott’s picture

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

pwolanin’s picture

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.

alexpott’s picture

Status: Needs review » Needs work
  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.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
5.53 KB
2.56 KB

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.

alexpott’s picture

  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.

pwolanin’s picture

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

pwolanin’s picture

Issue summary: View changes
cilefen’s picture

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?

pwolanin’s picture

@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).

alexpott’s picture

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.

pwolanin’s picture

pwolanin’s picture

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

YesCT’s picture

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

YesCT’s picture

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.

YesCT’s picture

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).

khaled.zaidan’s picture

#166 works perfectly for me!

manuel.adan’s picture

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

alexpott’s picture

@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?

bmcclure’s picture

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.

manuel.adan’s picture

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

Currently patch from #115 works for me on docker.

YesCT’s picture

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.

YesCT’s picture

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

The last submitted patch, 172: 2606772-171-testsonly.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 172: 2606772-171.patch, failed testing.

mikeker’s picture

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

The last submitted patch, 176: 2606772-176-test-only.patch, failed testing.

cilefen’s picture

Status: Needs review » Needs work
+++ 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.

ivanjaros’s picture

Is there a quickfix for completely disabling the hash suffix?

mikeker’s picture

@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.

mikeker’s picture

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.

The last submitted patch, 180: 2606772-180-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 180: 2606772-180.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
5.81 KB
1.54 KB

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.

Status: Needs review » Needs work

The last submitted patch, 184: 2606772-184.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.81 KB

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.

pwolanin’s picture

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

Status: Needs review » Needs work

The last submitted patch, 187: 2606772-187.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review

Test failure seems unrelated -- re-queuing.

Status: Needs review » Needs work

The last submitted patch, 187: 2606772-187.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
pwolanin’s picture

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?

ivanjaros’s picture

BTW: why do we need these long names anyway? Can't we just use template names?

pwolanin’s picture

@ivanjros - read the comments on class MTimeProtectedFileStorage and MTimeProtectedFastFileStorage and look at related issues.

pwolanin’s picture

discussed with alexpott and he suggested a follow-up issue to discuss requirements on non-Windows.

Belba’s picture

Hi

Is there found a solution for below isue. A solution for users who dont know coding :

Warning: rename(sites/default/files/php/twig/.c8f11c368b,sites/default/files/php/twig/16fd8d9f_block--system-messages-block.html.twig_c0493f9769b7032f5275fe4f26f33947f47fbbeeeeae6956e258589eb0c5b633/43969a40fb9932c6f99be001c55d4191ef50230540825bd474d4bd1562e04377.php): The system cannot find the path specified. (code: 3) in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 86 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).
Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save('16fd8d9f_block--system-messages-block.html.twig_c0493f9769b7032f5275fe4f26f33947f47fbbeeeeae6956e258589eb0c5b633', '<?php

Or maybe a solution to remove the warning, because the website is working fine.

Thanks

kevla’s picture

This has just caught me out. The patch in #187 seems to work ok. However this has stung me on a client site that insists on Windows. It's not really a good idea to run a dev version on production so I'm probably not able to deploy the patch till its released.

Are we still saying that Drupal 8.2 is compatible with Windows, despite this bug? It seems rather serious.

alexpott’s picture

I really want to land this change because it is hard to find out about this fix - but I'm pondering whether or not we should test the depth of the public files directory since in a multi-site install these can get quite big.

cilefen’s picture

It is mentioned in the release notes

kevla’s picture

Do you not think this should be highlighted more clearer than just the release notes. Perhaps on this page https://www.drupal.org/docs/8/system-requirements/web-server

(left a comment on that to open it up)

I really would advocate saying that Drupal 8 in it's current state is not reliably compatible with Windows.

ivanjaros’s picture

alexpott: yeah, I am storing templates in /tmp/appname/[uuid]/php/twig/..... so you can imagine the length. On Windows, this might be even longer with c:\windows\...

I understand the reason for these names but it is just so unpractical. It's like that long ass config directory name :D

---

Also I have a lot of theme suggestions so the template name itself will be "quite" long.

Maybe skipping the directories completely and using single directory with all templates could be fine?

cilefen’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record

I am setting this to needs work for alexpott's suggestion in #198. We can't ignore multisite. Merely testing DRUPAL_ROOT and warning based on that could paint an inaccurate picture of what may happen when you install for any site directory longer than "default". Let's find the public files directory depth and base the requirements error on that.

cilefen’s picture

@kevla You mentioned making a comment on https://www.drupal.org/docs/8/system-requirements/web-server but I don't see one. However, it is not clear where such a note would be best placed. The problem is OS-based, not web server based.

On the other hand, when you follow links to Download Drupal 8.2.5, you go to the release notes, and it is right there.

kevla’s picture

The problem is OS-based, not web server based.

True, it's hard to see where we should place a warning.

On the other hand, when you follow links to Download Drupal 8.2.5, you go to the release notes, and it is right there.

I would suggest that it's not clear enough. No one is going trawl through all those release notes just to see one tiny footnote which brings them to this huge thread. I vote that a stronger notice needs to be placed somewhere prominent. however I am in agreement with you that where to put that isn't clear with the current layout of the requirements page.

mikeker’s picture

Status: Needs work » Needs review
FileSize
5.76 KB
2.77 KB

Changed to measure the public files directory path length instead of Drupal root and bumped the limit to 120 characters.

This won't prevent the situation where a new, longer site is added to a shorter multi-site install. (Eg: really-long-name-goes-here.example.com is added to example.com.) I think that argues for raising the profile of this limitation as @kevla suggests.

cilefen’s picture

Nevertheless, the attempt to add a new site with a long directory name will result in a requirements error.

mikeker’s picture

Nevertheless, the attempt to add a new site with a long directory name will result in a requirements error.

Of course. They have to go through the install for each multi-site... Don't know what I was thinking!?!

I've added "read through the installation guide and find a good place for this" to my growing post-it of things to do while tests/migrations run... :)

pwolanin’s picture

We need to at least file a follow-up for a requirement warning on non-Windows too

alexpott’s picture

@pwolanin I'm not convinced about the non windows warning at all. This would mean a warning on every site with paths > 120 and most of them are not going to have an issue. Sure we can have a followup to discuss further but I don't think it should block this issue.

Can someone please provide test evidence i.e. a screenshot that warning works on windows? I don't have a windows install to prove it :)

mikeker’s picture

FileSize
28.63 KB

@alexpott, sorry, should've included a screenshot in #205...

With Drupal root at 101 characters deep, meaning sites/default/files (19 chars) plus the slash is 121 characters deep. Installation gives this:

error message

I should point out that you won't even get to that if you have $settings['extension_discovery_scan_tests'] = TRUE; (which is part of the default settings.local.php file). Instead you'll get an uncaught exception when ExtensionDiscovery tries to read invalid_module_name_over_the_maximum_allowed_character_length.info.yml. I'll open a followup for that, but I wanted to make note of it here.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I think this last change looks fine - let's get it in.

ivanjaros’s picture

Since this is just about adding a notice and not solving the issue itself, wouldn't be an idea to add settings.php setting that would allow to disable this level of security in cases where the directory structure has to be longer acceptable?

mikeker’s picture

Issue tags: -Needs manual testing

@ivanjaros, This does solve the underlying issue as far as we can solve it (the true root issue is the max path limitation). This patch prevents Twig cache filenames from exceeding that as well as adding the install error.

Removed the manual testing tag because of #210. Also, I've been using this patch on two projects on my localhost without issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 205: 2606772-205-shortened-hashes.patch, failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

sporadic test fail - queued a retest

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
5.68 KB
888 bytes

@wheatpenny, @pwolanin, @cilefen, and I reviewed the text for this issue. We came up with this more readable text:

The public files directory path is %depth characters. Paths longer than 120 characters will cause problems on Windows.

Attached updates the error to this message.

120 characters is hardcoded as a magic number; should we use a constant?

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Good, I like this shorter version.

alexpott’s picture

120 characters is hardcoded as a magic number; should we use a constant?

I guess we could have a constant in system.install - the other option would be bootstrap.inc or system.module but I'm not sure about what would also need this value. If the requirements stuff was a class then this would totally make sense as a class constant.

pwolanin’s picture

I don't think a constant is needed when this is only used in one place

xjm’s picture

Issue tags: +DrupalCampNJ2017
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.3.0 release notes

Committed and pushed ba455a1 to 8.4.x and 737fb7d to 8.3.x. Thanks!

Tagging for 8.3.0 release notes as committing this means that we should be able to take out the Windows warning from the release notes.

  • alexpott committed ba455a1 on 8.4.x
    Issue #2606772 by mikeker, pwolanin, YesCT, david_garcia, alexpott,...

  • alexpott committed 737fb7d on 8.3.x
    Issue #2606772 by mikeker, pwolanin, YesCT, david_garcia, alexpott,...
LittleCoding’s picture

Is it possible that this fix will make it into the 8.2.x branch?

cilefen’s picture

Re #223 Well, sort-of. There are existing sites and relocated sites to consider.

cilefen’s picture

@LittleCoding I started a conversation with xjm about backporting because I somehow became convinced this is critical priority (not sure how), when in fact it is major because of its limited scope and workarounds. No, we will not backport this to 8.2.x. Quoting xjm: "no further patch release is planned for 8.2.x barring critical or security fixes, so affected sites can try the patch for now and plan to upgrade to 8.3.0 April 5".

LittleCoding’s picture

Has anyone tested with the current 8.2.x releases? (as I am asking for a current site 8.2.6 and I do see some ill effect due to this bug)

cilefen’s picture

The most recent patch does not apply 8.2.x. A new one would need crafting.

mikeker’s picture

@LittleCoding: I'm running this on two 8.2.x project currently (I develop on Windows) without issues for a couple months. The patch doesn't apply as is, but if you (manually) remove the changes to TwigEnvironmentTest.php it applies cleanly.

LittleCoding’s picture

++ @mikeker thanks for the tip!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
mikeker’s picture

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

I don't think this was supposed to be moved to 8.3.x as it was committed to 8.4.x. Regardless, it's a closed issue so I don't think it matters...

cilefen’s picture

I think we usually leave it at the lowest branch it was committed to.