File upload fields added to contact forms should upload to private:// by default

Created on 24 January 2023, over 2 years ago

Problem/Motivation

When a file field is added to contact forms (the contact_message) entity, the uploaded files are stored in the public scheme by default. This creates a possible security risk if the user which adds the field to a contact form isn't aware of the security implications this has.

Proposed resolution

Set the default scheme for new fields to private by default. If the user wants, switching to the public scheme is still possible, but should be done explicitly.

Feature request
Status

Active

Version

10.1

Component
Contact 

Last updated 4 days ago

Created by

🇳🇱Netherlands Ruuds

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Ruuds
  • Status changed to Needs review over 2 years ago
  • Status changed to RTBC over 2 years ago
  • 🇮🇳India nidhi27

    I have tested this patch on 10.1.x-dev version.

    At the time of creating file field default value is selected as private for file uploading folder.

    I have attached the screenshot as well.

  • Status changed to Needs work over 2 years ago
  • 🇫🇮Finland lauriii Finland

    Seems sensible to me that contact form files would be considered as private by default.

    +++ b/core/modules/contact/contact.module
    @@ -248,3 +249,20 @@ function contact_form_user_admin_settings_submit($form, FormStateInterface $form
    +  if ($field->getTargetEntityTypeId() === 'contact_message' && $field->getType() === 'file') {
    

    Should we also check if the stream wrapper for private files exists? It seems like now we could be configuring an option by default which is not available for all sites.

    We should also add tests for this, just to confirm it works (and continues to do so).

  • Should we also check if the stream wrapper for private files exists?

    Correct. You cannot assume that wrapper exists. Webform module has the same idea and probably the code to borrow.

  • Status changed to Needs review over 2 years ago
  • 🇮🇳India mehul.gada Mumbai

    Here is the patch to check if the private files exists or not and accordingly set the default value.

  • Status changed to Needs work over 2 years ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    I think this is a good addition, however how about instead of forcing it, we target the field storage form with a form alter hook.

    Otherwise with this approach, the user could submit the form with 'public' and then go back to edit and see private and be confused.

    If we use a form alter, we can show a message as to why public isn't available as an option or similar.

    Also, we need some tests here.

    Great idea, love security by default!

  • Pipeline finished with Failed
    about 1 year ago
    Total: 241s
    #187294
  • Pipeline finished with Failed
    about 1 year ago
    Total: 378s
    #187301
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 452s
    #187307
  • Pipeline finished with Failed
    about 1 year ago
    Total: 512s
    #187323
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 657s
    #187351
  • Pipeline finished with Failed
    about 1 year ago
    Total: 520s
    #187362
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 502s
    #196511
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 126s
    #196520
  • Pipeline finished with Success
    about 1 year ago
    Total: 609s
    #196531
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 14s
    #197902
  • Pipeline finished with Success
    about 1 year ago
    Total: 568s
    #197905
  • Status changed to Needs review about 1 year ago
  • 🇳🇱Netherlands Ruuds

    I've implemented the suggestion of @larowlan and also added a test for it. If the private stream wrapper is not available a warning is shown it is advised to store file uploads for contact forms as private files.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 417s
    #203249
  • Status changed to RTBC about 1 year ago
  • 🇺🇸United States smustgrave

    Hiding patches for clarity

    Removing tests tag as coverage was added

    1) Drupal\Tests\contact\FunctionalJavascript\ContactFileFieldTest::testFileFieldHasPrivateSchemeByDefault
    Failed asserting that false is true.
    /builds/issue/drupal-3336081/core/modules/contact/tests/src/FunctionalJavascript/ContactFileFieldTest.php:72
    2) Drupal\Tests\contact\FunctionalJavascript\ContactFileFieldTest::testFileFieldHasPublicSchemeByDefaultWhenPrivateSchemeNotConfigured
    Behat\Mink\Exception\ResponseTextException: The text "It is advised to store file uploads for contact forms as private files. You can configure this in settings.php" was not found anywhere in the text of the current page.
    /builds/issue/drupal-3336081/vendor/behat/mink/src/WebAssert.php:907
    /builds/issue/drupal-3336081/vendor/behat/mink/src/WebAssert.php:293
    /builds/issue/drupal-3336081/core/tests/Drupal/Tests/WebAssert.php:975
    /builds/issue/drupal-3336081/core/modules/contact/tests/src/FunctionalJavascript/ContactFileFieldTest.php:115
    FAILURES!
    Tests: 2, Assertions: 20, Failures: 2.
    

    Applied some formatting changes to the hook and tests but appears @larowlan feedback from #7 has been addressed.

  • Pipeline finished with Success
    about 1 year ago
    Total: 500s
    #203252
  • Status changed to Needs work 12 months ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Added some review comments. I agree with @larowlan that this is a nice addition.

  • Pipeline finished with Failed
    12 months ago
    Total: 8574s
    #215977
  • Pipeline finished with Failed
    12 months ago
    Total: 601s
    #216079
  • Pipeline finished with Success
    12 months ago
    Total: 538s
    #216089
  • Pipeline finished with Failed
    12 months ago
    Total: 595s
    #216395
  • Pipeline finished with Failed
    12 months ago
    Total: 515s
    #216405
  • Pipeline finished with Failed
    12 months ago
    Total: 1251s
    #216427
  • Pipeline finished with Failed
    12 months ago
    Total: 153s
    #216506
  • Pipeline finished with Success
    12 months ago
    Total: 490s
    #216507
  • Status changed to Needs review 12 months ago
  • Status changed to RTBC 12 months ago
  • 🇺🇸United States smustgrave

    Believe feedback from @alexpott has been addressed from what I can see.

  • 🇳🇿New Zealand quietone

    I read the IS summary, the comments and the MR. The proposed resolution is out of dateAll questions are answered and the threads in the MR are correctly resolved.

    I then applied the diff and tested. This works as expected and is an improvement!

    I do think there follow up work.

    • Add this feature to media files as well.
    • The warning message uses the string 'It is advised' which is the first occurrence in core for a warning message. Because of that I think that the usability folks should review the string.
    • The first time I read "It is advised to store file uploads for contact forms as private files. You can configure this in settings.php" I read the first as something to action so I tried to. Of course, the option is not available and I knew that but I still tried! So, again, I think the a usability review would help.

    I think the above items can be done in followups. I am adding the tag for that.

    I finally read the test. Is there a reason it is only testing 1 code path?

  • Status changed to Needs work 10 months ago
  • 🇬🇧United Kingdom longwave UK

    I think the error message needs work. Is there a handbook page that we can link to on d.o that explains how and why you should configure private files? If so I think we should link there in either case, we don't need to separate out the two cases.

  • Status changed to Postponed about 2 months ago
  • 🇳🇿New Zealand quietone

    The Contact Module was approved for removal in 🌱 [Policy] Move Contact module to contrib Active .

    This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

    The deprecation work is in 📌 [meta] Tasks to deprecate the Contact module Active and the removal work in 📌 [12.x] [meta] Tasks to remove Contant module Active .

    Contact will be moved to a contributed project after the Drupal 12.x branch is open.

Production build 0.71.5 2024