WPCloudDeploy / wp-cloud-deploy

WPCloudDeploy is a WordPress plugin that allows you to easily deploy and manage your own dedicated high-performance WordPress servers and sites at any cloud server provider.
https://wpclouddeploy.com
Other
91 stars 42 forks source link

URI Encode and sanitization for query params #5

Closed vladolaru closed 2 years ago

vladolaru commented 2 years ago

This all started from using a AWS S3 secret key with '+' and '/' in it. This key didn't get saved properly and hence everything failed with relation to S3. It turned out this is a general issue with WPCD forms that pass their values through AJAX to be saved/deployed, etc.

I've applied URIEncode in JS to make sure the values are safe to be sent through URL queries. But this led to the necessity of proper decoding and sanitization on the PHP side since sanitize_text_field() applied before wp_parse_args() would strip away those URI encodings. The proper order is to:

elindydotcom commented 2 years ago

Two things with this.

  1. Did we get all the tab files? I counted 23 in the tabs folder and 19 in the tabs-server folder. So 42 files total. The PR is only showing 29 changed files.
  2. Some of the files missed running array_map around the $args array for sanitization. Even though it might seem as if some of the array elements aren't being referenced in the tabs, they might still end up being passed to the script that merges them into the commands that get run on the server. And, programmers can extend the forms to add new fields and add new command line arguments without directly modifying the tab files. Which is why in most cases we tried to sanitize the whole array whether it was being directly used or not. A quick perusal of the PR show that the backup tabs and the domain/change-domain are some of the tabs that missed sanitization.

Once those two items are resolved I think we can test and merge.

vladolaru commented 2 years ago

I only touched places where it felt safe doing so. I will do another run and see if there are places where I could apply the same logic.

vladolaru commented 2 years ago

I did another pass and I believe I've caught everything. I also made some minor improvements where I came across (since PHPStorm was screaming at me), mainly to not do echo wp_send_json() since it already does it and then die().