Issue #1848498 by twistor: Respect allowed file extensions in file mapper

mappers/file.inc

@@ -111,13 +111,14 @@ function file_feeds_set_target(FeedsSource $source, $entity, $target, array $val
       case 'uri':
         if ($v) {
           try {
+            $v->setAllowedExtensions($instance_info['settings']['file_extensions']);
             $file = $v->getFile($destination);
             $field[LANGUAGE_NONE][$delta] += (array) $file;
             // @todo: Figure out how to properly populate this field.
             $field[LANGUAGE_NONE][$delta]['display'] = 1;
           }
           catch (Exception $e) {
-            watchdog_exception('Feeds', $e, nl2br(check_plain($e)));
+            watchdog('feeds', check_plain($e->getMessage()));
           }
         }
         break;

plugins/FeedsParser.inc

@@ -264,7 +264,27 @@ class FeedsGeoTermElement extends FeedsTermElement {
  * Enclosure element, can be part of the result array.
  */
 class FeedsEnclosure extends FeedsElement {
-  protected $mime_type;
+
+  /**
+   * The mime type of the enclosure.
+   *
+   * @param string
+   */
+   protected $mime_type;
+
+   /**
+   * The default list of allowed extensions.
+   *
+   * @param string
+   */
+  protected $allowedExtensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp';
+
+  /**
+   * The sanitized local file name.
+   *
+   * @var string
+   */
+  protected $safeFilename;
 
   /**
    * Constructor, requires MIME type.
@@ -287,6 +307,17 @@ class FeedsEnclosure extends FeedsElement {
     return $this->mime_type;
   }
 
+  /**
+   * Sets the list of allowed extensions.
+   *
+   * @param string $extensions
+   *   The list of allowed extensions separated by a space.
+   */
+  public function setAllowedExtensions($extensions) {
+    // Normalize whitespace so that empty extensions are not allowed.
+    $this->allowedExtensions = trim(preg_replace('/\s+/', ' ', $extensions));
+  }
+
   /**
    * Use this method instead of FeedsElement::getValue() when fetching the file
    * from the URL.
@@ -301,20 +332,75 @@ class FeedsEnclosure extends FeedsElement {
   }
 
   /**
-   * Use this method instead of FeedsElement::getValue() to get the file name
-   * transformed for better local saving (underscores instead of spaces)
+   * Returns the full path to the file URI with a safe file name.
    *
-   * @return
+   * @return string
+   *   The safe file URI.
+   *
+   * @throws RuntimeException
+   *   Thrown if the file extension is invalid.
+   */
+  public function getSanitizedUri() {
+    return drupal_dirname($this->getValue()) . '/' . $this->getSafeFilename();
+  }
+
+  /**
+   * Returns the file name transformed for better local saving.
+   *
+   * @return string
    *   Value with space characters changed to underscores.
    *
-   * @see FeedsElement::getValue()
+   * @throws RuntimeException
+   *   Thrown if the file extension is invalid.
    */
   public function getLocalValue() {
-    return str_replace(' ', '_', $this->getValue());
+    return str_replace(' ', '_', $this->getSafeFilename());
   }
 
   /**
-   * @return
+   * Returns the safe file name.
+   *
+   * @return string
+   *   A filename that is safe to save to the filesystem.
+   *
+   * @throws RuntimeException
+   *   Thrown if the file extension is invalid.
+   */
+  protected function getSafeFilename() {
+    if (isset($this->safeFilename)) {
+      return $this->safeFilename;
+    }
+
+    $filename = rawurldecode(drupal_basename($this->getValue()));
+
+    if (module_exists('transliteration')) {
+      require_once drupal_get_path('module', 'transliteration') . '/transliteration.inc';
+      $filename = transliteration_clean_filename($filename);
+    }
+
+    // Remove leading and trailing whitespace and periods.
+    $filename = trim($filename, " \t\n\r\0\x0B.");
+
+    if (strpos($filename, '.') === FALSE) {
+      $extension = FALSE;
+    }
+    else {
+      $extension = substr($filename, strrpos($filename, '.') + 1);
+    }
+
+    if (!$extension || !in_array($extension, explode(' ', $this->allowedExtensions), TRUE)) {
+      throw new RuntimeException(t('The file @file has an invalid extension.', array('@file' => $filename)));
+    }
+
+    $this->safeFilename = file_munge_filename($filename, $this->allowedExtensions, FALSE);
+
+    return $this->safeFilename;
+  }
+
+  /**
+   * Downloads the content from the file URL.
+   *
+   * @return string
    *   The content of the referenced resource.
    */
   public function getContent() {
@@ -345,13 +431,14 @@ class FeedsEnclosure extends FeedsElement {
       // Prepare destination directory.
       file_prepare_directory($destination, FILE_MODIFY_PERMISSIONS | FILE_CREATE_DIRECTORY);
       // Copy or save file depending on whether it is remote or local.
-      if (drupal_realpath($this->getValue())) {
+      if (drupal_realpath($this->getSanitizedUri())) {
         $file           = new stdClass();
         $file->uid      = 0;
-        $file->uri      = $this->getValue();
+        $file->uri      = $this->getSanitizedUri();
         $file->filemime = $this->mime_type;
-        $file->filename = basename($file->uri);
-        if (dirname($file->uri) != $destination) {
+        $file->filename = $this->getSafeFilename();
+
+        if (drupal_dirname($file->uri) !== $destination) {
           $file = file_copy($file, $destination);
         }
         else {
@@ -368,16 +455,11 @@ class FeedsEnclosure extends FeedsElement {
         }
       }
       else {
-        $filename = basename($this->getLocalValue());
-        if (module_exists('transliteration')) {
-          require_once drupal_get_path('module', 'transliteration') . '/transliteration.inc';
-          $filename = transliteration_clean_filename($filename);
-        }
         if (file_uri_target($destination)) {
           $destination = trim($destination, '/') . '/';
         }
         try {
-          $file = file_save_data($this->getContent(), $destination . $filename);
+          $file = file_save_data($this->getContent(), $destination . $this->getLocalValue());
         }
         catch (Exception $e) {
           watchdog_exception('Feeds', $e, nl2br(check_plain($e)));
@@ -388,8 +470,9 @@ class FeedsEnclosure extends FeedsElement {
       if (!$file) {
         throw new Exception(t('Invalid enclosure %enclosure', array('%enclosure' => $this->getValue())));
       }
+
+      return $file;
     }
-    return $file;
   }
 }
 

tests/feeds_mapper_file.test

@@ -17,6 +17,10 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
     );
   }
 
+  public function setUp() {
+    parent::setUp(array('dblog'));
+  }
+
   /**
    * Basic test loading a single entry CSV file.
    */
@@ -36,7 +40,14 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
       // Reset all the caches!
       $this->resetAll();
     }
-    $typename = $this->createContentType(array(), array('files' => 'file'));
+    $typename = $this->createContentType(array(), array(
+      'files' => array(
+        'type' => 'file',
+        'instance_settings' => array(
+          'instance[settings][file_extensions]' => 'png, gif, jpg, jpeg',
+        ),
+      ),
+    ));
 
     // 1) Test mapping remote resources to file field.
 
@@ -68,8 +79,7 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
       ->execute();
     foreach ($entities as $entity) {
       $this->drupalGet('node/' . $entity->entity_id . '/edit');
-      $f = new FeedsEnclosure(array_shift($files), NULL);
-      $this->assertText($f->getLocalValue());
+      $this->assertText(str_replace(' ', '_', array_shift($files)));
     }
 
     // 2) Test mapping local resources to file field.
@@ -115,8 +125,7 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
 
     foreach ($entities as $entity) {
       $this->drupalGet('node/' . $entity->entity_id . '/edit');
-      $f = new FeedsEnclosure(array_shift($files), NULL);
-      $this->assertRaw('resources/' . $f->getUrlEncodedValue());
+      $this->assertRaw('resources/' . rawurlencode(array_shift($files)));
     }
 
     // 3) Test mapping of local resources, this time leave files in place.
@@ -142,8 +151,7 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
 
     foreach ($entities as $entity) {
       $this->drupalGet('node/' . $entity->entity_id . '/edit');
-      $f = new FeedsEnclosure(array_shift($files), NULL);
-      $this->assertRaw('images/' . $f->getUrlEncodedValue());
+      $this->assertRaw('images/' . rawurlencode(array_shift($files)));
     }
 
     // Deleting all imported items will delete the files from the images/ dir.
@@ -208,13 +216,58 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase {
 
     foreach ($entities as $i => $entity) {
       $this->drupalGet('node/' . $entity->entity_id . '/edit');
-      $f = new FeedsEnclosure(array_shift($files), NULL);
-      $this->assertRaw($f->getUrlEncodedValue());
+      $this->assertRaw(str_replace(' ', '_', array_shift($files)));
       $this->assertRaw("Alt text $i");
       $this->assertRaw("Title text $i");
     }
   }
 
+  public function testInvalidFileExtension() {
+    variable_set('feeds_never_use_curl', TRUE);
+
+    $typename = $this->createContentType(array(), array(
+      'files' => array(
+        'type' => 'file',
+        'instance_settings' => array(
+          'instance[settings][file_extensions]' => 'txt',
+        ),
+      ),
+    ));
+
+    // Create a CSV importer configuration.
+    $this->createImporterConfiguration('Node import from CSV', 'invalid_extension');
+    $this->setPlugin('invalid_extension', 'FeedsCSVParser');
+    $this->setSettings('invalid_extension', 'FeedsNodeProcessor', array('bundle' => $typename));
+    $this->setSettings('invalid_extension', NULL, array('content_type' => ''));
+    $this->addMappings('invalid_extension', array(
+      0 => array(
+        'source' => 'title',
+        'target' => 'title',
+      ),
+      1 => array(
+        'source' => 'file',
+        'target' => 'field_files:uri',
+      ),
+    ));
+
+    // Import.
+    $edit = array(
+      'feeds[FeedsHTTPFetcher][source]' => url('testing/feeds/files-remote.csv', array('absolute' => TRUE)),
+    );
+    $this->drupalPost('import/invalid_extension', $edit, 'Import');
+    $this->assertText('Created 5 nodes');
+
+    foreach (range(1, 5) as $nid) {
+      $node = node_load($nid);
+      $this->assertTrue(empty($node->field_files));
+    }
+
+    foreach ($this->listTestFiles() as $filename) {
+      $message = t('The file @file has an invalid extension.', array('@file' => $filename));
+      $this->assertTrue(db_query("SELECT 1 FROM {watchdog} WHERE message = :message", array(':message' => $message))->fetchField());
+    }
+  }
+
   /**
    * Lists test files.
    */