Easy-digital-downloads: If download file doesn't exist on server, an unusable file, with correct name is downloaded

[Shelob9]

Steps to reproduce:
1) Migrate your site, but don't copy uploads/edd (BTW I think this is an issue with Migrate DB Pro's media files add-on)
2) Go to purchase history and click a download link
3) In my case I get a zip file with the right name, but its empty and therefore GNOME Archive Manger (and probably other zip apps) reports an error

[pippinsplugins]

@Shelob9 Are you able to provide a dump of post meta from the product for us?

[Shelob9]

@pippinsplugins Here is output of wp post meta list for an 1 download that was screwed up by my mistake: https://pastebin.com/xi9LMa4R

To be clear, the issue was that when I migrated the server, the EDD files did not come with it. So there is no way EDD should have been able to produce a proper download file.

I think the bug is bad error handling. It shouldn't list the download file in purchase details, it should show something like Error with download file, email Josh and tell him to fix it.

[pippinsplugins]

Specifically, the problem is this: "attachment_id":"33038". I think any way.

We should add better checks to determine if the attachment exists.

[pippinsplugins]

@Shelob9 did that attachment ID get migrated but without the file or was the attachment ID invalid as well?

[pippinsplugins]

PR attached that checks if the file exists. It does not trigger an error message but does then default back to trying to use the file URL (which could still exist) inside of post meta.

We could potentially go a step further by doing a wp_remote_head() request to the file URL to confirm the file does not return a 404 error.

[Shelob9]

@pippinsplugins Correct, the attachment got migrated, but the actual file did not.

[pippinsplugins]

@Shelob9 the site URL was unchanged, right?

[Shelob9]

The site URL was changed. WP Migrate DB Pro did update the meta properly to the new URL, it just didn't pull the download files properly, which I don't really consider a bug in WP Migrate DB Pro, since I had set nginx to block access to EDD dir.

[pippinsplugins]

Thoughts on approach in #5275?

It introduces a new action hook that provides an easy point at which we can inject remote head request to determine if the URL is accessible.

[pippinsplugins]

Quick way to test:

1. Add a file to a product that is valid
2. Add a file URL to a product that is invalid (any fake or protected URL will work)
3. Download the files. File 1 should work. File 2 should show an actual error message.

[SDavisMedia]

@pippinsplugins Is this the expected error? https://cl.ly/1o3z0h1k2s3D

I see a wp_die() error in your changes. What's the exact expected behavior here?

[Shelob9]

@pippinsplugins I'm reading, but not testing (sorry still getting caught up) but if I'm reading right this is going to happen when I click the download, not when it is listed, right? if so, I don't love. I'd rather it put a notice in the download details and fire an action -- which I would hook into on my site to send me an email or Slack message that I messed up.

It's not really an error that is useful to customer, it is useful to site admin..

[pippinsplugins]

@SDavisMedia no, that's not expected. You should see the wp_die() screen. Looks like you either specified a file path (this change affects URLs only, not paths) or your file has an attachment_id stored in the file download meta. Try deleting the file, saving the download, adding a new "bad" file URL, saving the download, then (yes really) saving the download again.

@Shelob9 This initial approach is shown to the customer, yes. The idea was to show an actual error message instead of downloading a damaged file. Showing an error message to the customer provides an actionable step by giving them something of value they can show to site support teams. At minimum this provides site admins with a way to track down what's wrong instead of just hearing from customers that the file is damaged.

We could certainly add a check / action to the file downloads section of the edit screen but that's a much bigger change than I can put in this point release.

which I would hook into on my site to send me an email or Slack message that I messed up.

How would this work? The only time the file could be checked is when viewing the product edit screen. Doing these checks in the background is not something we can realistically manage since we have to take into account sites with thousands of products.

[Shelob9]

@pippinsplugins Here is what I would do:

add_action( 'edd_process_download_pre_record_log', function(  $requested_file, $args ) {
    if ( ! filter_var( $requested_file, FILTER_VALIDATE_URL ) ) {
        wp_mail( '[email protected]', "File $requested_file does not exist for EDD", var_export( $args, true ) );
    }
}, 10, 2 );

What is in $args Does it have user ID of customer and payment ID, would be hella useful to me in that scenario.

[pippinsplugins]

@Shelob9 Ah yes, I like that idea.

[pippinsplugins]

Started playing with this but then decided I'm not ready to implement an email notification yet. When we do that, I want to be really thorough about it to ensure we send a notice on all possible failures that we can. For now I'm just going to make it possible to fire your own check via an action if this fails:

/**
 * Perform a head request on file URLs before attempting to download to check if they are accessible.
 *
 * @since  2.6.14
 * @param  string $requested_file The Requested File
 * @param  array  $args           Arguments
 * @param  string $method         The download mehtod being sed
 * @return void
 */
function edd_check_file_url_head( $requested_file, $args, $method ) {

    // If this is a file URL (not a path), perform a head request to determine if it's valid
    if( filter_var( $requested_file, FILTER_VALIDATE_URL ) ) {

        $valid   = true;
        $request = wp_remote_head( $requested_file );

        if( is_wp_error( $request ) ) {

            $valid   = false;
            $message = $request;
            $title   = __( 'Invalid file', 'easy-digital-downloads' );

        }

        if( 404 === wp_remote_retrieve_response_code( $request ) ) {

            $valid   = false;
            $message = __( 'The requested file could not be found. Error 404.', 'easy-digital-downloads' );
            $title   = __( 'File not found', 'easy-digital-downloads' );

        }

        if( ! $valid ) {

            do_action( 'edd_check_file_url_head_invalid', $requested_file, $args, $method );
            wp_die( $message, $title, array( 'response' => 403 ) );

        }

    }

}
add_action( 'edd_process_download_pre_record_log', 'edd_check_file_url_head', 10, 3 );

Seem reasonable?

[Shelob9]

I would not want the email notification all the time. I want the hook so I
can write an add-on that logs fails and sends me alert in Slack, Email
and/or Google Assistant first time it happens that day.

On Dec 6, 2016 8:13 PM, "Pippin Williamson" [e-mail]
wrote:

Started playing with this but then decided I'm not ready to implement an
email notification yet. When we do that, I want to be really thorough about
it to ensure we send a notice on all possible failures that we can. For now
I'm just going to make it possible to fire your own check via an action if
this fails:

/**

• Perform a head request on file URLs before attempting to download to check if they are accessible.
  *
• @since 2.6.14
• @param string $requested_file The Requested File
• @param array $args Arguments
• @param string $method The download mehtod being sed
• @return void
  */
  function edd_check_file_url_head( $requested_file, $args, $method ) {

// If this is a file URL (not a path), perform a head request to determine if it's valid
if( filter_var( $requested_file, FILTER_VALIDATE_URL ) ) {

  $valid   = true;
  $request = wp_remote_head( $requested_file );

  if( is_wp_error( $request ) ) {

      $valid   = false;
      $message = $request;
      $title   = __( 'Invalid file', 'easy-digital-downloads' );

  }

  if( 404 === wp_remote_retrieve_response_code( $request ) ) {

      $valid   = false;
      $message = __( 'The requested file could not be found. Error 404.', 'easy-digital-downloads' );
      $title   = __( 'File not found', 'easy-digital-downloads' );

  }

  if( ! $valid ) {

      do_action( 'edd_check_file_url_head_invalid', $requested_file, $args, $method );
      wp_die( $message, $title, array( 'response' => 403 ) );

  }

}

}
add_action( 'edd_process_download_pre_record_log', 'edd_check_file_url_head', 10, 3 );

Seem reasonable?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/5268#issuecomment-265325569,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5uR4ETfNXCdIJ11npSut0ZzhkkLBqkks5rFggdgaJpZM4K_z8_
.

[pippinsplugins]

Cool then that hook should work fine then :)

[pippinsplugins]

@SDavisMedia could you test this PR one more time before we merge it?

[SDavisMedia]

@pippinsplugins Yup. All good here on issue/5268. I get a wp_die with The requested file could not be found. Error 404. while other, valid files work as expected. :+1:

[pippinsplugins]

:+1:

[pippinsplugins]

This appears to be causing problems on some sites. Opened new issue to track it down: #5308