[Regression in 1.6] missing attachments from private://

Hi,

Same problem with webform attachments which is private...

Attached patch remove temporary hasAccess

Thanks for the report. The access checking is a new feature designed to improve security. The change record was linked from the release note. By default private files are not allowed.

Please can you take a look at the CR and see if you could write code to call Attachment::setAccess()?

If not, then do you have any suggestions how we could make it work for your site and still keep some security?

We could remove the access check for legacy emails (sent using Drupal core mailer)?? This makes some sense for compatibility.

I managed to add something like this snippet to the custom code for adding attachments which are located in private://

    /** @var \Drupal\symfony_mailer\Email $email */
    $email = $this->emailFactory->newTypedEmail($email_params['type'], $email_params['sub_type'], $email_params);
    if ($email_params['attachments']) {
      /** @var File $attachment */
      foreach ($email_params['attachments'] as $attachment) {
        $at = Attachment::fromPath($attachment->getFileUri(), $attachment->getFilename());
        if($at->hasAccess() === false) {
          // We don't have access to the attachment, so force allowed access here.
          $at->setAccess(AccessResult::allowed());
        }
        $email->attach($at);
      }
    }
    $email->send();

Hope this can help anyone for fixing sending attachments which are located / configured in the private files directory.

I understand this change was introduced for security reasons, but could you clarify specifically what vulnerability or risk this hasAccess() check is addressing?

On my current site, even public files are no longer being sent as attachments, which breaks expected behavior. Before applying a workaround like setAccess(AccessResult::allowed()), I’d like to understand if:

the change is strictly necessary in all cases,

and whether the implementation is fully correct, given that public files are also affected.

For now, solution #7 works fine, but I’m trying to assess whether this change is required and properly implemented. Thanks.

I understand this change was introduced for security reasons, but could you clarify specifically what vulnerability or risk this hasAccess() check is addressing?

This change/problem was introduced in #3382624: embed attachments API addition. The vulnerability is where non-trusted users can attach files - they could attach a file that they shouldn't be able to access and email it to themselves (this isn't just theoretical as there has already been a security issue like this in another module). Particularly this applies to #3284140: Email adjuster to attach/embed by scanning the email body, which currently is in 2.x but not yet in 1.x.

The solution is intended to work like this. Public files are allowed; http/https file are allowed if the URL can be opened with fopen; everything else is not allowed by default. Code that knows the attachment is from a trusted source can use the API to allow access to any file.

For legacy emails, AFAIK there has never been any access checking. The code that sets the attachment is responsible for ensuring that it is safe. Therefore we could reasonably do the same, as in #7. This was how it worked in 1.5, and I AFAIK it is safe.

On my current site, even public files are no longer being sent as attachments

and whether the implementation is fully correct, given that public files are also affected.

This is not correct, it would be a bug. Can anyone else confirm? There is an automatic test that should be covering this case, however the test could be wrong.

Fix in 2.x first then backport

v1.x version ready for testing please

We've reviewed the changes and will test the 1.x fix internally to confirm the regression is resolved.

One additional proposal: introduce a configuration option (e.g., checkbox or boolean field) that allows trusted code to opt into attaching files from private:// when needed. We totally agree that default should remain FALSE for security.

Use case: on a platform managing exams, a certificate is generated after completion and emailed to the user. The certificate contains personal data, so it's stored in private://. With the current restriction, sending it by email becomes impossible, despite being a fully controlled, trusted process.

Instead of requiring all developers to manually override access via setAccess(AccessResult::allowed()), exposing a controlled config flag would make the behavior clearer, safer, and easier to manage.

What do you think @adamps?

We've reviewed the changes and will test the 1.x fix internally to confirm the regression is resolved.

Thanks

Use case: on a platform managing exams, a certificate is generated after completion and emailed to the user.

Please can you explain how you are adding the attachment?

• If you are using the old/legacy mail interface, then it will work with this patch.
• If you are using the new interface, then I would expect you have code that calls email->attachFromPath() or similar. So could you add one extra line to call setAccess()?

Please can you explain how you are adding the attachment?

I'm using the function email->attachFromPath() so yeah, I can add the setAccess() just after.

For your patch, I just realized that it changes the legacy mailer that I don't use, so I will not be able to review it.

One additional proposal: introduce a configuration option (e.g., checkbox or boolean field) that allows trusted code to opt into attaching files from private:// when needed. We totally agree that default should remain FALSE for security.

As I was thinking about a same solution so this seems to be a good idea from me. This configuration form could have a field where you can set trusted sources for the attachments being sent with the module.

field where you can set trusted sources for the attachments being sent with the module.

This could potentially work.

a configuration option (e.g., checkbox or boolean field) that allows trusted code to opt into attaching files from private:// when needed.

I don't feel it's safe as a flag that would allow access to any file. The GUI admin might set the flag intending access to specific files, but then the attacker could add something else (such as settings.php😃).

I have another idea. We could allow attaching private files that the recipient would anyway have access to via HTTPS. It's a bit awkward to implement because there isn't an API - the code is tied into FileDownloadController.php. I guess we'd have to invoke hook_file_download().

I have raised #3530021: Allow selective access to attach private files.

Hi All,
in prepration for d11 i upgraded from 1.4 to 1.6.
no custom code, i use symfony_mailer with webforms.
/admin/config/system/mailsystem is set to default `Default PHP Mailer`.
/admin/config/system/mailer/transport is set to `sendmail`.
/admin/config/system/mailer/override no overrides enabled.
test webform submission puts a link for the uploaded file in the body, no attachment:
```
File
my.png (528.35 KB)
```
path for my.png is : http://mysite.dev/system/files/webform/casey_test_file_upload/4713/my.png
which is not accessable by anonymous user.

Patch #3 fixes the issue for now.

@Thanks ccarnnia. Please could you test MR176 as that is what I would like to commit?

After someone has tested and set it to RTBC then I will commit and make a new release.

Tested & +1 for RTBC here !

Great thanks

Now available in 1.6.2 release.