@file_unmanaged_delete_recursive('/') gets called when clearing caches before running updb

By the way, if someone tries to run drush cr while having root privileges, this error might delete the whole server!

I would also suggest not defaulting to / when it comes to deleting things. You should never assume that people who run a website have their permissions set up properly and as such are probably running things as root, with potentially devastating consequences as a result.

Seems critical enough to me to even unpublish this release. Better be safe than sorry.

After updating to the latest version of google_tag

Is this in dev only, or in the 1.1 release? I can't view the code on GitLab at the moment.

@kevinquillen Fortunately it is only in dev version

Can be easily fixed with an empty check before trying to delete the assets. Which makes sense, because we don't want to delete anything if the directory hasn't been set. Alternatively we could default to the public://google_tag folder.

Here is a patch which checks for the presence of the folder before actually performing the recursive delete function

@mark_gerarts Sorry, it seems I've hidden your file while uploading mine!

I think it should either do only the empty check or maybe better even both, calling is_dir('') is a bit strange.

The config value is empty, not /, the / is only added afterwards. I assume The folder should only be used by google_tag, otherwise it can't know what it did write and what not.

@berdir
I added the is_dir() to check if the folder is actually there, otherwise do not run the command. I think I made the mistake to assume that it will convert the 'public://' on its own.

@bceyssens
If the user add '/' as config uri, I think the function should assume to look in the 'public://` folder.

Another patch which correctly check for the realpath of the public folder

is_dir('public://something') should work fine, that's not point, my point is that is_dir('') is strange and we should also explicitly check for an empty string.

Indeed @Berdir, as is_dir('/') will return true too, empty check should be mandatory, note realpath('/') will return '/'

the trailing slash is added after the is_dir() check, it would only be / if a user actually enters /, which should of course not be allowed and makes me wonder if this shouldn't be configuration at all or at least only within the public:// file system, which could be a separate setting.

The top directory should be forced ("//google_tag/") IMO, so the only disaster scenario in this case would be deleting google tag generated files which can be regenerated anyway. Someone may enter the wrong path by mistake and wind up deleting files in public or elsewhere that they do not intend to do.

I see that there is a commit pushed to the 1.x-dev branch, but it's still marked as "Needs Review". Is this fixed yet or are there still kinks that need to be worked out from the patch?

The commit fixes the initial purpose of this issue, namely that the "code might break stuff when clearing the cache before running updb". In that regard I consider the issue fixed.

I did not change status as the comments were diverting to a discussion of whether to restrict the directory used to store the snippets. (Quite possibly this should be a separate issue). I would be fine with changing this to allow user to specify a stream wrapper (and possibly a directory path beneath that) and using a directory of "wrapper://path/google_tag". Thoughts?

Hi, is this problem fixed?

Something I've spotted when testing patch #15 , when module configuration is missing, following line:

+    $uri = \Drupal::config('google_tag.settings')->get('uri');

Will return a NULL , then

+    $dir_exists = is_dir(\Drupal::service('file_system')->realpath($uri));

returns docroot , and is_dir() still gives TRUE, this ends up with following:

Fatal error: Uncaught Error: Call to a member function read() on boolean in /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php:378
Stack trace:
#0 /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php(383): Drupal\Core\File\FileSystem->deleteRecursive('//root', NULL)
#1 /var/www/html/docroot/core/includes/file.inc(883): Drupal\Core\File\FileSystem->deleteRecursive('/', NULL)
#2 /var/www/html/docroot/modules/contrib/google_tag/src/Entity/ContainerManager.php(198): file_unmanaged_delete_recursive('/')
#3 /var/www/html/docroot/modules/contrib/google_tag/google_tag.module(52): Drupal\google_tag\Entity\ContainerManager->createAllAssets()
#4 /var/www/html/docroot/modules/contrib/google_tag/google_tag.module(43): _google_tag_assets_create()
#5 [internal function]: google_tag_rebuild()
#6 /var/www/html/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('google_tag_rebu...', Array)
#7 /var/www/html/docroot/core/includes/common.inc(1162): Drupal\Core\Extension\ModuleHandler->inv in /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php on line 378
Drush command terminated abnormally due to an unrecoverable error.                                                                                                                                      [error]
Error: Uncaught Error: Call to a member function read() on boolean in /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php:378
Stack trace:
#0 /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php(383): Drupal\Core\File\FileSystem->deleteRecursive('//root', NULL)
#1 /var/www/html/docroot/core/includes/file.inc(883): Drupal\Core\File\FileSystem->deleteRecursive('/', NULL)
#2 /var/www/html/docroot/modules/contrib/google_tag/src/Entity/ContainerManager.php(198): file_unmanaged_delete_recursive('/')
#3 /var/www/html/docroot/modules/contrib/google_tag/google_tag.module(52): Drupal\google_tag\Entity\ContainerManager->createAllAssets()
#4 /var/www/html/docroot/modules/contrib/google_tag/google_tag.module(43): _google_tag_assets_create()
#5 [internal function]: google_tag_rebuild()
#6 /var/www/html/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('google_tag_rebu...', Array)
#7 /var/www/html/docroot/core/includes/common.inc(1162): Drupal\Core\Extension\ModuleHandler->inv in /var/www/html/docroot/core/lib/Drupal/Core/File/FileSystem.php, line 378

Why are you focusing on the patch in #15 when the problem was solved by the commit in #17?

As mentioned in #22 this issue has been fixed. If a discussion of the snippet directory needs to happen that is the topic of a separate issue/

Came across this problem yesterday with version 1.2 and resulted in public files being deleted. All this simply because the configuration settings for google_tag were not current with the 1.2 version (or what it expects).

The problem is when there is a case where "uri" is not found or defined in configuration. Regardless of operator or other unforeseen error, there should still be a condition in place to prevent a recursive delete on root "/" if "url" setting is empty.

@pmuzyk The 1.2 release includes the commit in #17 which I believe solves this problem. Please explain how the code could get to the delete statement with the uri setting being undefined. What steps were taken (and in what order) on your site that could leave it in a state in which the delete statement would be executed with an undefined uri setting, e.g. cache rebuild, db update, code update, etc.?

By more recent commit than #17, the trailing slash was removed from $uri. With this change, even if $uri is NULL and delete_recursive() is called, the latter will do nothing as NULL is neither a directory nor a file. [But this is not relevant to your claim.]

Hi there,

We also ran into this issue. We're running on 1.2 . The google_tag module tried to delete the root filesystem.

Re-opening the ticket.

@phayes Please answer the questions in #27. In order to flag something as a bug you need to demonstrate how this module causes the problem. Among other things that means list the steps to reproduce.

Also, try the dev release.

Seems obvious enough as commented before? If url is somehow empty, then it deletes "/". I've seen a lot of people who messed up updating mdoules and reimport old configuration.

Just add a check on $uri and don't do anything if empty. Considering how destructive this is, it doesn't hurt to be a little bit defensive.

@Berdir No it is not obvious enough. In fact, noone has demonstrated how the claim can occur in the 1.2 release (which includes the commit in #17). Can you? I understand how it could occur before the aforementioned commit, but not after. Please list steps to reproduce.

Also, try the dev release.

Do not try this at home :p

Given that you have a container.

drush config-set google_tag.settings uri ""
drush ev "_google_tag_assets_create()"

Yes, sure, url shouldn't be empty, but clearly it happens. I've had a ton of bug reports in pathauto too, mostly because people reimported their old configuration after running updates. And in pathauto, it was just missing slashes in their URL's, not deleting their whole fricking disk :)

Just add a if ($uri) wrapper around it.

Regarding the 1.2 release

Implied in your test setup from "given that you have a container" is that you had run the database update on the 1.2 release. After this update, a container would be created AND the uri setting would NOT be empty but would match the default value of 'public://google_tag'. In which case, the code would NOT attempt to delete the root file system.

With the two commands you indicate, you concede that this is a user error. Surely you are not one to dispute the fact that in an out-of-context manner (as you have indicated) all sorts of code can break. This raises the interesting question of how much sanitization of inputs should be done in a large system. When do you leave it to the users to take responsibility for themselves?

What if you had set the uri to '/', 'public://' or 'public://[dir-with-latest-file-uploads-that-are-not-backed-up] or ...? What are we to do then? Surely your suggested 'fix' is of no value in these cases?

Regarding the latest dev release

Those two commands you suggest, go ahead and try this at home :p

In the guidelines for filing a bug, does it not indicate to verify the problem against HEAD? (And I have asked twice already.)

To further demonstrate the fallacy of your suggested fix..

With the latest release, what if you set the uri to '/', 'public://' or 'public://[dir-with-latest-file-uploads-that-are-not-backed-up] or ...? Now what? Even your 'just add a wrapper around it' suggestion will not save you. This module is not responsible for operator error.

Issue remains Fixed.

Another case how this could go wrong. I ran into this problem on a production release. The cause, it turns out, is because the GTM settings are in a config split and the split version was not updated. Obviously user error. However, IMHO the size of the error does not really measure up to the size of the consequences. A simple check if the setting has any value seems reasonable. Break as hard as you like, but break before attempting to delete the entire file system of the server.

One last try. I'm just trying to help. As others have said, the impact of this is *severe* and results in massive data loss.

I had a look at the dev version, and there is no relevant change.

Config split is one example where something like this could happen, despite having executed updates. And yes, all cases where this could happen are user mistakes. But these mistake have happened multiple times and are fairly common.

All we're asking for is adding this:

if (empty($uri)) {
  throw new \Exception('Asset configuration is not configured, set google_tag.settings:uri');
}

to that function.

@Berdir If you were sincerely 'trying to help', then you would actually test the dev release and NOT just 'look at the dev version, and [conclude] there is no relevant change'. For the use case of this issue, namely the uri being empty (NULL, blank), then the code will delete NOTHING!!!

Read it again, boys. NOTHING!

Also, your solitary concern ONLY for the root directory seems rather short-sided. Did you miss the point about user error to set the uri to any directory that the web user should have write access to?

@Berdir, @eelkeblok This issue is resolved in the latest dev release.

Added feature request to recharacterize the uri setting and always append 'google_tag' for the snippet directory as mentioned in #20.

OK, just to prevent others from having to actually look up the code, the latest dev version does indeed do a check if the setting is empty and also includes #3088579: Treat the uri setting as the parent directory. This was not clear to me before, thanks for pointing that out clearly.