Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download handler: support falling back on the redirect method (as an option, not a default) #30294

Merged
merged 7 commits into from
Jul 22, 2021

Conversation

barryhughes
Copy link
Member

@barryhughes barryhughes commented Jul 15, 2021

The product download handler supports 3 different methods of serving files:

Method Note
X-Accel/X-Sendfile Secure (customer cannot see the source URL)
Force Download Secure (customer cannot see the source URL)
Redirect Only Insecure (source URL is exposed to the customer)

Previously (before 5.5.0) the system included a fallback mechanism as follows:

  • If X-Accel/X-Sendfile is the preferred method but cannot be used, it falls back on Force Download.
  • If Force Download cannot be used and if the digital product file is remote, then a final fallback is made to the Redirect method
  • Redirect is the last method, there is no further fallback.

In f88586e we removed that final fallback, so as to avoid inadvertently exposing the digital asset's public path to the world (in other words, the idea was that redirects should be explicitly enabled or not happen at all), however there were some problems with this:

  • In some cases, even though the merchant may have selected one of the secure delivery methods, their server was not configured appropriately and so downloads were relying on the redirect fallback. Therefore, removing that fallback had the effect of breaking their customers' access to purchased digital assets.
  • In other cases, the merchant (knowingly or unkowingly) relied on the previous chain of fallbacks because they hosted their assets across a diverse range of storage platforms, some that were suitable for Force Download but others that were not. The previous system of fallbacks was therefore essential to supporting this arrangement.

In this change:

  • We add a new option "Allow using redirect mode as a last resort".
  • We add logging, so that a record is kept of any fallbacks that take place (letting the merchant or site operator find problems and take corrective steps if appropriate).

This hopefully gives the best of both worlds. Improved security (we don't allow redirects if the merchant hasn't specifically allowed for them) with the flexibility of the earlier, pre-5.5.0 approach (by allowing redirects as a final fallback, but only if explicitly enabled).

Closes #30288.

Screenshots

optional-redirect-fallback-option
☝️ New option, allowing fallbacks on redirects but making it something that needs to be explicitly enabled.

Screenshot 2021-07-19 at 15-43-14 WooCommerce settings ‹ WC Lab — WordPress
☝️ Revised message text for the deprecation warning that renders when "Redirect Only" is selected.

How to test the changes in this Pull Request:

Setup

  1. Configure your PHP so that allow_url_fopen is disabled.
  2. Visit WooCommerce ▸ Settings ▸ Products ▸ Downloadable Products.
  3. Set the File Download Method to Force Downloads.
  4. Enable the new "Allow using redirect mode as a last resort" option.
  5. Add a remote file to one of your downloadable products (remote := hosted on another site).

Testing

  1. Try purchasing and downloading.
  2. With "Allow using redirect mode as a last resort" enabled, the download should work (but via a redirect).
  3. Try again with "Allow using redirect mode as a last resort" disabled, the download should not work.

Other aspects

  1. See also this alternative set of testing steps (documented in a comment, below).
  2. If fallbacks (to redirect mode) take place, an entry should be recorded in the logs to document it.
  3. When "Redirect Only" is selected, the content of the resulting admin notice that renders (to warn that the method is deprecated) has been updated to reflect this change.

Other information

  • As an alternative, there is another draft PR which, like this PR, adds logging—but otherwise essentially reverts things such that the previous fallback logic is always enabled.
  • If we accept this change, we may need to adjust the following resources (not exhaustive):

Changelog entry

Fix - Add a new option allowing product downloads to be served using redirects as a last resort. #30288

@barryhughes barryhughes changed the base branch from fix/30288-1 to trunk July 19, 2021 23:00
@barryhughes
Copy link
Member Author

barryhughes commented Jul 19, 2021

📝 One failing E2E test, which I still need to review and update. Resolved. By adding a new option, the form submit button was being pushed under the 'How do you like our settings?' feedback banner, which the test understandably did not account for. Also took the opportunity to add some coverage for the new checkbox.

@barryhughes barryhughes marked this pull request as ready for review July 21, 2021 00:17
@barryhughes barryhughes requested review from a team and Konamiman and removed request for a team July 21, 2021 00:17
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some minor comments related to messaging.

includes/admin/settings/class-wc-settings-products.php Outdated Show resolved Hide resolved
includes/admin/settings/class-wc-settings-products.php Outdated Show resolved Hide resolved
@Konamiman
Copy link
Contributor

@barryhughes I have followed the testing instructions and it works fine. I have also tested as follows:

  1. Configure PHP so that allow_url_fopen is enabled.
  2. Configure File Download Method as X-Accel-Redirect/X-Sendfile (without having the required module installed)
  3. Add a locally hosted file to the download product.
  4. Purchase.
  5. Download the file, it should work.
  6. Verify that a WARNING (file URL) could not be served using the X-Accel-Redirect/X-Sendfile method. A Force Download will be used instead. message has been added to the logs.

Maybe you want to add this to the testing instructions.

@Konamiman Konamiman added this to the 5.5.2 milestone Jul 21, 2021
@barryhughes
Copy link
Member Author

barryhughes commented Jul 21, 2021

📝 Quick note on E2E test fails—they don't seem to relate to this change. At time of writing both fails are from specs/front-end/product-browse-search-sort.test.js:

  • Search, browse by categories and sort items in the shop › should let user search the store
  • Search, browse by categories and sort items in the shop › should let user browse products by categories

However, by creating a draft PR that is 1:1 with current trunk (will clean it up later) I can see exactly the same fails.

@Konamiman Konamiman merged commit 4c709a1 into trunk Jul 22, 2021
@Konamiman Konamiman deleted the fix/30288-2 branch July 22, 2021 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants