Comment in \Drupal\Component\PhpStorage\FileStorage::createDirectory() that additional information is not included for security reasons

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Changed this to more a feature request.

Love the idea of more details!
At this time we will need a D10 version that may need to be updated for D10 and php8.

Please do not reroll without testing first.

Adding reroll for 10.1.x

@Abhisheksingh27 again please include an interdiff or diff with patches.

Change looks fine though

Immediate question I have reading this code:

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -120,7 +120,7 @@ protected function createDirectory($directory, $mode = 0777) {
-        trigger_error('mkdir(): Permission Denied', E_USER_WARNING);
+        trigger_error(sprintf('mkdir(%s): Permission Denied', $directory), E_USER_WARNING);

This isn't going to disclose the secret PHP storage hash, is it? Core's default, shared-hosting-friendly setup relies on security through obscurity for that directory. Is $directory here the parent or the actual directory?

Looking at the code, I think it's the actual directory. The preceding lines are:

   // If parent exists, try to create the directory and ensure to set its      
    // permissions, because mkdir() obeys the umask of the current process.     
    if ($parent_exists) {
      // We hide warnings and ignore the return because there may have been a   
      // race getting here and the directory could already exist.               
      @mkdir($directory);
      // Only try to chmod() if the subdirectory could be created.              
      if (is_dir($directory)) {
        // Avoid writing permissions if possible.                               
        if (fileperms($directory) !== $mode) {
          return chmod($directory, $mode);
        }
        return TRUE;
      }
      else {
        // Something failed and the directory doesn't exist.  

I think this directory path might include the secret file storage hash, and if so, it could reduce security to have this in logs -- or, worse, displayed to the UI if the web server isn't properly configured.

Tagging for security review and manual testing to verify whether this is OK or not.

Could we rely on

$config['system.logging']['error_level'] = 'verbose';

and only print the details if settings.php and friends ask us to?

It does seem risky to print full server path names in exception messages that could potentially be displayed all over the place. Definitely agree with @xjm in #23. I haven't reviewed any code or done any testing, leaving all tags for now.

Thanks,
-Derek

Posted in security-discussion to see if anyone can take a look.

Though #24 may be a good compromise?

I agree there could be a risk here in information disclosure. As this is in a component that is designed to be used outside of Drupal core, we also can't rely on Drupal config to determine whether to give more information.

If createDirectory() fails, ensureDirectory() does nothing. Is this also a bug? Ultimately it should probably be the caller of save() that reports an error to the user, although without refactoring all this to use exceptions instead, it's not going to be possible for higher level callers to tell exactly what the error was.

Tagging for framework review to see if it's worth doing

Ultimately it should probably be the caller of save() that reports an error to the user, although without refactoring all this to use exceptions instead, it's not going to be possible for higher level callers to tell exactly what the error was.

I don't think its worth reworking the component to throw errors, so inclined to mark this as won't fix

Yeah, I hadn’t looked at the code at all. If this is a Component(tm), I rescind my previous suggestion 😅

In that case, +1 to won’t fix.

I also agree with won't-fixing this if the security concerns apply, however, to avoid someone raising the same issue to propose this in the future (because they aren't aware that this has been raised/discussed/decided before), should we at the very least add an inline comment to those 2 places stating that the directory name is intentionally NOT disclosed for security reasons?

Perhaps also add to that inline comment something about what the alternative is, in which case I find the "it should probably be the caller of save() that reports an error to the user" phrase by @longwave above to be short and to the point. So perhaps tweak that and use it?

Sure, I guess an inline comment could be ok. But the caller of save() apparently won’t know the reason, either, since this component doesn’t throw exceptions, and we’re saying it’s not worth adding that.

Although, if we added a comment to core about every idea we considered and decided against, there’d be more comments than code. 😂

So reading the comments this appears to be a won't fix. If anyone feels strongly for comments would open a follow up referencing this ticket. But As @dww mentioned that could lead to a lot of comments.

FTR, I don't feel strongly re adding inline comments, but just wanted to say that more inline comments in not a bad thing, and that at the end of the day it'll be either more comments, or more potential confusion and more (duplicate) issues raised 🤷‍♂️

I added the comment lines and linked to this issue. Please let me know if I should rephrase.

Think change looks good. And matches IS.

Please update the issue summary and bring it inline with the changes proposed in the merge request.

I'm triaging RTBC issues → .

I read the issue summary and skimmed the comments. This proposed resolution is out of date, as reported in #24. I am setting this to Needs work for that. I also commented on the MR, that needs work as well. Both these tasks are suitable for 'Novice', so I am leaving the tag.

For anyone unfamiliar with issue summary updates, this should help, Write an issue summary for an existing issue → .

Trying for a better title

Fixing attribution.

And hiding old patch files for clarity.