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

Skip SSL verification for IIS support. #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

msigley
Copy link

@msigley msigley commented Jun 3, 2014

Fixes issue #57.

@westonruter
Copy link
Contributor

@msigley many would probably want sslverify left intact.

Perhaps it would be better to just throw in another mu-plugin which does:

$minify_action_priority = 10;

add_action( Dependency_Minification::CRON_MINIFY_ACTION, function () {
    add_filter( 'https_local_ssl_verify', '__return_false' );
    add_filter( 'https_ssl_verify', '__return_false' );
}, $minify_action_priority - 1 );

add_action( Dependency_Minification::CRON_MINIFY_ACTION, function () {
    remove_filter( 'https_local_ssl_verify', '__return_false' );
    remove_filter( 'https_ssl_verify', '__return_false' );
}, $minify_action_priority + 1 );

Too bad that https_ssl_verify and https_local_ssl_verify don't pass the $url as an additional filter arg, and then these could be used target those specific JS requests.

@msigley
Copy link
Author

msigley commented Jun 3, 2014

We could also do an $is_IIS check as well and only disable the sslverify option in the HTTP request if IIS is detected.
http://codex.wordpress.org/Global_Variables
The biggest issue is for some reason the default WIMP stack installed on Windows via the Web PI service doesn't install the dependencies necessary for SSL verification via PHP because IIS servers don't use OpenSSL (yeah no heartbleed). You are fairly safe to assume 99% of people running PHP with IIS aren't running OpenSSL, so disabling sslverify for all IIS users is a fair fix.

Let me know the direction you wish to go in and I will revise or revoke my pull request.

@westonruter
Copy link
Contributor

@msigley does my plugin above not do the trick to turn off SSL verification on your site?

@msigley
Copy link
Author

msigley commented Jun 5, 2014

It does, but this is still a IIS compatibility issue in the plugins implementation. Again virtually anyone running IIS will experience this issue.

@@ -841,7 +841,7 @@ static function minify( $cached ) {

// Dependency is not self-hosted or it the filesystem read failed, so do HTTP request
if ( false === $contents ) {
$r = wp_remote_get( $src );
$r = wp_remote_get( $src, array( 'sslverify' => false ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

@msigley I think this should be changed to be something like this:

$sslverify = apply_filters( 'depmin_https_ssl_verify', empty( $GLOBALS['is_IIS'] ), $src );
$r = wp_remote_get( $src, compact( 'sslverify' ) );

This then turns off SSL verification for IIS only by default, but also allows it to be filtered.

Copy link
Author

Choose a reason for hiding this comment

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

Looks good to me. Merge and revise in a commit. Nice work @westonruter!

Copy link
Contributor

Choose a reason for hiding this comment

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

@msigley are you going to add another commit to your PR with this change?

Copy link
Author

Choose a reason for hiding this comment

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

Commited the changes requested

Copy link
Author

Choose a reason for hiding this comment

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

@westonruter do the changes meet your approval?

@westonruter
Copy link
Contributor

@msigley I was trying to apply your patch to the develop branch, because there was some cleanup and refactoring going on there which is still not merged. If you could open another PR to merge into the develop branch that would be great.

Also, before doing another plugin release (1.0), I want to get unit tests in place (#6) so that we can check for regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants