diff --git a/mappers/file.inc b/mappers/file.inc index e9cbc26..b104044 100644 --- a/mappers/file.inc +++ b/mappers/file.inc @@ -104,7 +104,9 @@ function file_feeds_set_target($source, $entity, $target, array $values) { case 'uri': if ($v) { try { - $file = $v->getFile($destination); + // @todo Neither $destination nor $instance_info are set if + // $sub_field is not 'uri'. + $file = $v->getFile($destination, $instance_info['settings']['file_extensions']); $field[LANGUAGE_NONE][$delta] += (array) $file; // @todo: Figure out how to properly populate this field. $field[LANGUAGE_NONE][$delta]['display'] = 1; diff --git a/plugins/FeedsParser.inc b/plugins/FeedsParser.inc index 24ae01c..6315c36 100644 --- a/plugins/FeedsParser.inc +++ b/plugins/FeedsParser.inc @@ -275,7 +275,8 @@ class FeedsEnclosure extends FeedsElement { * The mime type of the resource. */ public function __construct($value, $mime_type) { - parent::__construct($value); + // Always store the decoded version to avoid double encoding. + parent::__construct(urldecode($value)); $this->mime_type = $mime_type; } @@ -301,16 +302,92 @@ 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) + * Gets the file name transformed for safe local saving. + * + * @param string $extensions + * A string with a space separated list of allowed extensions. * * @return - * Value with space characters changed to underscores. + * Safe value with space characters changed to underscores. * * @see FeedsElement::getValue() */ - public function getLocalValue() { - return str_replace(' ', '_', $this->getValue()); + public function getLocalFilename($extensions) { + $name = drupal_basename($this->getValue()); + + if (module_exists('transliteration')) { + require_once drupal_get_path('module', 'transliteration') . '/transliteration.inc'; + $name = transliteration_clean_filename($name); + } + + $name = str_replace(' ', '_', $name); + return $this->checkNameExtension($name, $extensions); + } + + + /** + * Sanitizes a user-input file name and extension. + * + * @param string $name + * The file name and extension. + * @param string $extensions + * A string with a space separated list of allowed extensions. + * + * @return string + * A safe file name and extension. + */ + protected function checkNameExtension($name, $extensions) { + // Get the part of the name after the last period ("."). + $name = explode('.', $name); + $last = array_pop($name); + + // Make it lowercase for consistency as much as security. + $extension = strtolower($last); + + // Is this a whitelisted extension? + if (!in_array($extension, explode(' ', $extensions))) { + // No. Restore it to the name and use the default extension, 'txt'. + $name[] = $last; + $extension = 'txt'; + } + + // Sanitize the name, apart from the extension. + $name = $this->checkName(implode('.', $name)); + + // Is there still a valid name? + if (0 === strlen($name)) { + // No. Use the default file name of 'file'. + $name = 'file'; + } + + // Munge the non-whitelisted secondary file extensions. + return file_munge_filename("$name.$extension", $extensions); + } + + /** + * Sanitizes a user-input file or directory name. + * + * @param string $name + * The file or directory name. + * + * @return string + * A safe name. + */ + protected function checkName($name) { + // Punctuation characters that are allowed, but not as first/last character. + $punctuation = '-_. '; + + $map = array( + // Replace (groups of) whitespace characters. + '!\s+!' => ' ', + // Replace multiple dots. + '!\.+!' => '.', + // Remove characters that are not alphanumeric or the allowed punctuation. + "![^0-9A-Za-z$punctuation]!" => '', + ); + + // Apply the regex replacements. Remove any leading or hanging punctuation. + return trim(preg_replace(array_keys($map), array_values($map), $name), $punctuation); } /** @@ -329,9 +406,11 @@ class FeedsEnclosure extends FeedsElement { /** * Get a Drupal file object of the enclosed resource, download if necessary. * - * @param $destination + * @param string $destination * The path or uri specifying the target directory in which the file is * expected. Don't use trailing slashes unless it's a streamwrapper scheme. + * @param string $extensions + * A string with a space separated list of allowed extensions. * * @return * A Drupal temporary file object of the enclosed resource. @@ -339,7 +418,7 @@ class FeedsEnclosure extends FeedsElement { * @throws Exception * If file object could not be created. */ - public function getFile($destination) { + public function getFile($destination, $extensions) { if ($this->getValue()) { // Prepare destination directory. @@ -350,7 +429,10 @@ class FeedsEnclosure extends FeedsElement { $file->uid = 0; $file->uri = $this->getValue(); $file->filemime = $this->mime_type; - $file->filename = basename($file->uri); + // Sanitize the filename even if it is based on the name of an existing + // file on the local filesystem. + // This whitelist of extensions is copied from file_save_upload(). + $file->filename = $this->getLocalFilename('jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp'); if (dirname($file->uri) != $destination) { $file = file_copy($file, $destination); } @@ -368,16 +450,12 @@ 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, '/') . '/'; } + $destination .= $this->getLocalFilename($extensions); try { - $file = file_save_data($this->getContent(), $destination . $filename); + $file = file_save_data($this->getContent(), $destination); } catch (Exception $e) { watchdog_exception('Feeds', $e, nl2br(check_plain($e))); @@ -388,8 +466,9 @@ class FeedsEnclosure extends FeedsElement { if (!isset($file)) { throw new Exception(t('Invalid enclosure %enclosure', array('%enclosure' => $this->getValue()))); } + + return $file; } - return $file; } } diff --git a/tests/feeds_mapper_file.test b/tests/feeds_mapper_file.test index dcf5d98..bc4cb7e 100644 --- a/tests/feeds_mapper_file.test +++ b/tests/feeds_mapper_file.test @@ -61,15 +61,9 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase { $nid = $this->createFeedNode('syndication', $GLOBALS['base_url'] . '/testing/feeds/flickr.xml'); $this->assertText('Created 5 nodes'); - $files = $this->listTestFiles(); - $entities = db_select('feeds_item') - ->fields('feeds_item', array('entity_id')) - ->condition('id', 'syndication') - ->execute(); - foreach ($entities as $entity) { - $this->drupalGet('node/' . $entity->entity_id . '/edit'); - $f = new FeedsEnclosure(array_shift($files), NULL); - $this->assertText($f->getLocalValue()); + foreach ($this->assertImageNids('syndication') as $nid => $filename) { + $this->drupalGet("node/$nid/edit"); + $this->assertText(str_replace(' ', '_', $filename)); } // 2) Test mapping local resources to file field. @@ -107,17 +101,7 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase { $this->assertText('Created 5 nodes'); // Assert: files should be in resources/. - $files = $this->listTestFiles(); - $entities = db_select('feeds_item') - ->fields('feeds_item', array('entity_id')) - ->condition('id', 'node') - ->execute(); - - foreach ($entities as $entity) { - $this->drupalGet('node/' . $entity->entity_id . '/edit'); - $f = new FeedsEnclosure(array_shift($files), NULL); - $this->assertRaw('resources/' . $f->getUrlEncodedValue()); - } + $this->assertImagesLocation('resources'); // 3) Test mapping of local resources, this time leave files in place. $this->drupalPost('import/node/delete-items', array(), 'Delete'); @@ -134,17 +118,7 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase { $this->assertText('Created 5 nodes'); // Assert: files should be in images/ now. - $files = $this->listTestFiles(); - $entities = db_select('feeds_item') - ->fields('feeds_item', array('entity_id')) - ->condition('id', 'node') - ->execute(); - - foreach ($entities as $entity) { - $this->drupalGet('node/' . $entity->entity_id . '/edit'); - $f = new FeedsEnclosure(array_shift($files), NULL); - $this->assertRaw('images/' . $f->getUrlEncodedValue()); - } + $this->assertImagesLocation('images'); // Deleting all imported items will delete the files from the images/ dir. $this->drupalPost('import/node/delete-items', array(), 'Delete'); @@ -200,22 +174,52 @@ class FeedsMapperFileTestCase extends FeedsMapperTestCase { $this->assertText('Created 5 nodes'); // Assert files exist. - $files = $this->listTestFiles(); - $entities = db_select('feeds_item') - ->fields('feeds_item', array('entity_id')) - ->condition('id', 'image_test') - ->execute(); - - foreach ($entities as $i => $entity) { - $this->drupalGet('node/' . $entity->entity_id . '/edit'); - $f = new FeedsEnclosure(array_shift($files), NULL); - $this->assertRaw($f->getUrlEncodedValue()); + $i = 0; + foreach ($this->assertImageNids('image_test') as $nid => $filename) { + $this->drupalGet("node/$nid/edit"); + $this->assertRaw(str_replace(' ', '_', $filename)); $this->assertRaw("Alt text $i"); $this->assertRaw("Title text $i"); + $i += 1; } } /** + * Asserts test image files are in the specified directory. + */ + protected function assertImagesLocation($directory) { + foreach ($this->assertImageNids('node') as $nid => $filename) { + $this->drupalGet("node/$nid/edit"); + $filename = rawurlencode($filename); + $this->assertRaw("$directory/$filename"); + } + } + + /** + * Gets a map of node NIDs to filenames for a Feeds importer. + */ + protected function assertImageNids($id) { + $return = array(); + $files = $this->listTestFiles(); + foreach ($this->listImageNids($id) as $i => $nid) { + $return[$nid] = $files[$i]; + } + + // Assert that there are the same number of nodes as test files. + $this->assert(count($return) === count($files), 'There is a node for each test file.'); + + return $return; + } + + /** + * Gets Node NIDs for a Feeds importer. + */ + protected function listImageNids($id) { + $sql = 'SELECT entity_id FROM {feeds_item} WHERE id = :id ORDER BY entity_id'; + return db_query($sql, array(':id' => $id))->fetchCol(); + } + + /** * Lists test files. */ protected function listTestFiles() {