Importing images from https feed results into a cURL certificate error

This was reported too in #2510788: Remove query string from path in FeedsEnclosure.. Didn't the patch from that issue help?
Someone else that uses the jsonpath parser also noticed issues after updating Feeds, see #2548859: JSONPath import issues after update to Feeds 7.x-2.0-beta1 (I see you use the successor of that module, but maybe something in that issue could help). You can check your source with my Feeds import preview module. This allows you to see what your source looks like after it has been parsed.

By the way, the issue priority determines the seriousness of a bug to a project, not the priority for you, so I set it back to "normal". See also https://www.drupal.org/node/45111

This issue has something to with cURL. After trying to import the supplied image, the following error is recorded in watchdog (for some reason not in the feeds log):

cURL error (60) SSL certificate problem: unable to get local issuer certificate for https://scontent.xx.fbcdn.net/hphotos-xpf1/v/t1.0-9/p180x540/11052396_10...

I'm not an expert in cURL, but a possible workaround could be to add an option to the file mapper to allow invalid SSL certificates, just as the HTTP Fetcher has.

This could be implemented this way (@iamfredrik I'm not saying you should do this, unless you are a coder):

• Set a summary and form callback for the file:uri target.
• Add a property called $acceptInvalidCert to the FeedsEncloser class + a method to set it.
• In file_feeds_set_target() call this setter method.
• In FeedsEncloser::getContent(), pass $this->acceptInvalidCert to the http_request_get() call.

But it would be better to fix cURL, if possible. Maybe there is a cURL option set wrong or there is a bug in PHP's cURL library.

Patch extends parser interface to allow specific implementations to pass "certificate error ignore" setting into further processing.
CSV parser was patched to expose SSL handling option in config form.

Added necessary check.

Hm, I think adjusting the CSV parser to fix this issue is not a good idea, because the issue isn't specific to the parser at all. The issue is specific to the file target. Therefore I think a solution should be implemented as a mapping option on the file target as suggested in #5.

Closed #2803989: File upload fetcher can't accept invalid SSL certificates as a duplicate which contains a patch with a quick & dirty fix (= always accept invalid certificates).

I took a stab at what MegaChriz mentioned in #5.

The patch allows for fields to accept invalid SSL certificates per file mapping. I tried to follow the variable naming standard, but the patch will need some review.

Please let me know what you think and how this can be improved. Thanks.

Thanks for your work! Sorry it took me a while before I was able to give it a review.

I tested it manually by fetching images from Drupal.org using an old system with which I have some SSL issues with. With the patch applied and the option enabled I was able to fetch an image from Drupal.org, while without the patch the image wasn't fetched. So the patch is doing it's job well!

Code review:

1. +++ b/mappers/file.inc
   @@ -123,8 +135,13 @@ function file_feeds_set_target(FeedsSource $source, $entity, $target, array $val
   +            $v->setAllowedExtensions($instance_info['settings']['file_extensions']);              ¶
   

   (minor) whitespace.

2. +++ b/mappers/file.inc
   @@ -123,8 +135,13 @@ function file_feeds_set_target(FeedsSource $source, $entity, $target, array $val
   +            if ($mapping['accept_invalid_cert']) {
   +              $field[$language][$delta] += (array) $v->getFile($destination, $mapping['accept_invalid_cert']);
   +            } else {
   +              $field[$language][$delta] += (array) $v->getFile($destination);
   +            }
   

   I think a check for $mapping['accept_invalid_cert'] is not needed. If it is FALSE, it can just pass that value to the getFile() method.

3. +++ b/mappers/file.inc
   @@ -140,3 +157,41 @@ function file_feeds_set_target(FeedsSource $source, $entity, $target, array $val
   +  $options = _file_feeds_form_callback_options();
   ...
   +  //return t('Accept invalid SSL certificates: <strong>@accept test</strong>', array('@accept' => $options[0]));
   ...
   +/**
   + * Returns the list of available file methods.
   + *
   + * @return array
   + *   An array of file option titles.
   + */
   +function _file_feeds_form_callback_options() {
   +  return array(
   +    FEEDS_FILE_ACCEPT_INVALID_CERT => 'Accept Invalid Certificate',
   +  );
   +}
   

   It seems that _file_feeds_form_callback_options() is not needed. The constant FEEDS_FILE_ACCEPT_INVALID_CERT can be removed as well. It is an on/off option.

4. +++ b/mappers/file.inc
   @@ -62,6 +69,11 @@ function file_feeds_processor_targets($entity_type, $bundle_name) {
   +  // Add in default values.
   +  $mapping += array(
   +    'accept_invalid_cert' => FEEDS_FILE_ACCEPT_INVALID_CERT,
   +  );
   

   Simultaneously, the default value can just be FALSE.

5. +++ b/mappers/file.inc
   @@ -140,3 +157,41 @@ function file_feeds_set_target(FeedsSource $source, $entity, $target, array $val
   +  if (empty($mapping['accept_invalid_cert']) || $mapping['accept_invalid_cert'] === FEEDS_FILE_ACCEPT_INVALID_CERT) {
   +    return t('<strong>Not</strong> accepting invalid SSL certificates.');
   +  }
   

   Because it is more like an advanced setting, I think nothing has to be shown when the option is disabled.

6. +++ b/plugins/FeedsParser.inc
   @@ -453,7 +453,8 @@ class FeedsEnclosure extends FeedsElement {
   +  // Need to pass a value
   
   @@ -494,7 +495,7 @@ class FeedsEnclosure extends FeedsElement {
   +          $file = file_save_data($this->getContent($accept_invalid_cert), $destination . $filename);// need to pass getContent a value
   

   These code comments can be removed.

7. +++ b/plugins/FeedsParser.inc
   @@ -431,9 +431,9 @@ class FeedsEnclosure extends FeedsElement {
   +  public function getContent($accept_invalid_cert) {
   +++ b/plugins/FeedsParser.inc
   ...
   @@ -453,7 +453,8 @@ class FeedsEnclosure extends FeedsElement {
   +  public function getFile($destination, $accept_invalid_cert = FALSE) {
   

   The parameter $accept_invalid_cert should be documented in the function docblock above.

8. +++ b/plugins/FeedsParser.inc
   @@ -431,9 +431,9 @@ class FeedsEnclosure extends FeedsElement {
   +  public function getContent($accept_invalid_cert) {
   

   The parameter $accept_invalid_cert should be optional and default to FALSE. There could be external code that is calling getContent().

   Now that I'm looking at this, I wonder if it would be better to have the parameter defined as an $options array? This way it's easier to add additional options later and there would be less chance in breaking code in this case.

Great work so far. Well done!

Since #1171114: Allow user to choose the method of file handling. was committed, the latest patch no longer applies.

Thanks MegaChriz. I've reviewed the patch from #1171114 and it looks like with that patch in, there is less code for this patch. The attached patch includes your comments from #15 except for changing $options into an array. Would that be out of scope for this issue?

Please let me know what you think of the patch. Thanks.

diff --git a/sites/all/modules/feeds/mappers/file.inc b/sites/all/modules/feeds/mappers/file.inc
...
--- a/sites/all/modules/feeds/mappers/file.inc
+++ b/sites/all/modules/feeds/mappers/file.inc
...
diff --git a/sites/all/modules/feeds/plugins/FeedsParser.inc b/sites/all/modules/feeds/plugins/FeedsParser.inc
...
--- a/sites/all/modules/feeds/plugins/FeedsParser.inc
+++ b/sites/all/modules/feeds/plugins/FeedsParser.inc

The patch is created against the Drupal root directory, it should be created against the Feeds directory.

#facepalm

I don't think this option should be in the UI. People will use it, and that's a problem. However, I also see a problem of having a global option, if you only want to enable it for one importer, hmmm.

With great power comes great responsibility...

;)

Joking aside, I copied the text to display the warning from admin/structure/feeds/[feed_machine_name]/settings/FeedsHTTPFetcher. Hopefully the warning is strong enough to warn people why not to check it.

My use case:
My feed uses the file upload fetcher, so I don't see the Accept invalid SSL certificates checkbox. It would be strange, to me, to see the Accept invalid SSL certificates checkbox at admin/structure/feeds/[feed_machine_name]/settings/FeedsFileFetcher when the page is referring to uploading a file. Ideally I would resolve the SSL issue so this isn't an issue for me, but sadly that isn't possible (long story).

I think this would be a nice feature to have in the feeds module, but that is just for my 1 use case. Thanks.

We indeed don't want to encourage people to use this feature, but at the same time we don't want to slow down people if it is not needed. I have some problems with SSL on an old development machine, which I apparently only can solve by upgrading the system (which I will do, but takes time). In cases like these it is useful to have an option to skip the SSL check.

To keep the option a bit hidden, I thought it was best to not display its value in the summary when it was disabled (which I proposed in #15: "Because it is more like an advanced setting, I think nothing has to be shown when the option is disabled.").

What I was thinking about if we want to discourage people using the option was to display a warning on top of the page that it is discouraged to accept invalid certificates (only to be displayed when the option is enabled). This warning would be displayed on each configuration page of the importer. Or would that be too annoying?

Patch #21 review

1. When the option is enabled, it should be reflected in the UI. So the line 'Accept invalid SSL certificates' that was in file_feeds_summary_callback() in the patch from #14 may come back (but only return that line if the option is enabled of course). Maybe adding a warning sign to it would be nice.
2. Some nitpicks:

   1.     +++ b/mappers/file.inc
          @@ -84,6 +84,7 @@ function file_feeds_processor_targets($entity_type, $bundle_name) {
             $mapping += array('file_exists' => FILE_EXISTS_RENAME);
          +  $mapping += array('accept_invalid_cert' => FALSE);
          

      This can be merged into a single array.

   2.     +++ b/plugins/FeedsParser.inc
          @@ -428,12 +428,15 @@ class FeedsEnclosure extends FeedsElement {
          +   * @param bool $accept_invalid_cert|FALSE
          +   *   If the request should ignore all SSL errors and get the file.
      
          @@ -449,6 +452,8 @@ class FeedsEnclosure extends FeedsElement {
          +   * @param bool $accept_invalid_cert|FALSE
          +   *   If the request should ignore all SSL errors and get the file.
          

      For an optional parameter I usually start the description with '(optional)'. In the second line of the description I set what the default value is: 'Defaults to FALSE.', instead of adding |FALSE next to the variable name. Not sure if this is a coding standard or a personal preference.

3. I think an $options array would be more future-proof. Imagine we want to add an option to set username/password somewhere and pass that to http_request_get(), then FeedsEnclosure::getContent() could look like this:
   

     public function getContent($accept_invalid_cert = FALSE, $username = NULL, $password = NULL) {
   

   while http_request_get() looks like this:

     function http_request_get($url, $username = NULL, $password = NULL, $accept_invalid_cert = FALSE, $timeout = NULL) {
   

   I this example it is less elegant that the parameters of these two functions are in a different order.

   We could also change it later when needed, but then it would be an API break. Let me know what you think of it.

I also tested the patch manually again and it is still working great! Thanks for your work!

Drupal 7 reached end of life and the D7 version of Feeds is no longer being developed. To keep the issue queue focused on supported versions, we’re closing older D7 issues.

If you still have questions about using Feeds on Drupal 7, feel free to ask. While we won’t fix D7 bugs anymore, we’re happy to offer guidance to help you move forward. You can do so by opening (or reopening) a D7 issue, or by reaching out in the #feeds channel on Drupal Slack.

If this issue is still relevant for Drupal 10+, please open a follow-up issue or merge request with proposed changes. Contributions are always welcome!