Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#47577 closed enhancement (fixed)

Detect HTTPS support and provide guidance

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch has-unit-tests has-dev-note
Focuses: administration Cc:

Description (last modified by flixos90)

Of all the WordPress sites today, 63.4% are using HTTPS. While this is already better than the average for the entire web, it is far from optimal. More and more modern web APIs require usage of HTTPS, let alone the security implications of not using it.
In order to close that gap, WordPress should do better to actively recommend administrators to switch their non-HTTPS site to use HTTPS, especially if their current environment already technically supports it.

In order to provide accurate recommendations to site owners about switching their site to HTTPS, we need to know whether HTTPS is even supported by their server and domain. This has been reliably detected in the PWA plugin for a while, and similar logic could be used in core.

Based on the result of the HTTPS support detection, we would recommend one of the following:

  • If supported, recommend to change the WordPress site URL, as that's all that's needed.
  • If not supported, recommend talking to the web host about enabling HTTPS.

This provide more accurate recommendations for the respective situation a site is in. Then, in separate follow-up tickets, we should look at simplifying the migration from HTTP to HTTPS itself which today is far too complex for the majority of WordPress users.

Attachments (5)

47577.diff (7.6 KB) - added by flixos90 5 years ago.
Screenshot 2019-06-20 at 13.47.12.png (65.5 KB) - added by flixos90 5 years ago.
Screenshot of when HTTPS is not enabled, but supported by the environment
Screenshot 2019-06-20 at 13.55.15.png (58.1 KB) - added by flixos90 5 years ago.
Screenshot of when HTTPS is not enabled and also not supported by the environment
47577.2.diff (10.6 KB) - added by miinasikk 5 years ago.
Includes adding Content-Security-Policy header with filter to opt out for Content-Security-Policy-Report-Only
Screenshot 2019-08-27 at 11.36.43.png (92.5 KB) - added by miinasikk 5 years ago.
HTTPS section with additional information about Content-Security-Policy

Download all attachments as: .zip

Change History (62)

@flixos90
5 years ago

#1 follow-up: @flixos90
5 years ago

  • Keywords has-patch added; needs-patch removed

47577.diff makes the following changes:

  • Introduce wp_is_using_https(), which detects whether the site uses HTTPS based on the WordPress site URL.
  • Introduce wp_is_https_supported() which detects whether HTTPS is supported by the environment (using a cached request to the HTTPS version of the site).
  • Enhance the Site Health test for HTTPS status to use the new function and provide accurate recommendation based on the results.
  • If HTTPS is active, filter and replace non-HTTP links to the site in content with their HTTPS counterparts.

@flixos90
5 years ago

Screenshot of when HTTPS is not enabled, but supported by the environment

@flixos90
5 years ago

Screenshot of when HTTPS is not enabled and also not supported by the environment

#2 @earnjam
5 years ago

It would be nice to allow filtering the Site Health action link/button for talking to your host about HTTPS in the same way we do for the PHP upgrade notices.

Hosts will have differing documentation about how to accomplish it on their environments and they may want to link directly to that.

#3 @johnbillion
5 years ago

Related: #28521 (for considerations related to filtering URL schemes).

#4 in reply to: ↑ 1 @westonruter
5 years ago

Replying to flixos90:

  • If HTTPS is active, filter and replace non-HTTP links to the site in content with their HTTPS counterparts.

I suggest also adding a upgrade-insecure-requests CSP directive to automatically handle this outside fo the content: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/upgrade-insecure-requests

#5 follow-up: @flixos90
5 years ago

@earnjam

It would be nice to allow filtering the Site Health action link/button for talking to your host about HTTPS in the same way we do for the PHP upgrade notices.

I like that suggestion, however I'd prefer if we approached this iteratively and opened a follow-up issue to make that URL filterable. The complexity we've learned about during the Servehappy project is that we probably wouldn't want them to replace the wordpress.org support URL, so we'd need to add more content and tweak the UI to allow for that, which would require more discussion.

@westonruter

I suggest also adding a upgrade-insecure-requests CSP directive to automatically handle this outside fo the content

That's worth exploring. I'm wondering whether that would cause problems with URLs pointing to external websites, that may still not be on HTTPS though - how does the directive deal with images or links from such websites? The other concern is that in order to add CSP headers into core, it may be better to work on a simple centralized solution as a developer API that would allow managing those directives.

#6 in reply to: ↑ 5 ; follow-up: @westonruter
5 years ago

Replying to flixos90:

I suggest also adding a upgrade-insecure-requests CSP directive to automatically handle this outside fo the content

That's worth exploring. I'm wondering whether that would cause problems with URLs pointing to external websites, that may still not be on HTTPS though - how does the directive deal with images or links from such websites? The other concern is that in order to add CSP headers into core, it may be better to work on a simple centralized solution as a developer API that would allow managing those directives.

Hyperlinks to other websites would be ignored since merely linking to another site does not cause a request. Images, videos, iframes, and other resources would need to be upgraded even on external domains as otherwise there would be insecure mixed content warnings.

Per MDN:

The upgrade-insecure-requests directive will not ensure that users visiting your site via links on third-party sites will be upgraded to HTTPS for the top-level navigation

#7 in reply to: ↑ 6 ; follow-up: @miinasikk
5 years ago

Replying to westonruter:

Hyperlinks to other websites would be ignored since merely linking to another site does not cause a request. Images, videos, iframes, and other resources would need to be upgraded even on external domains as otherwise there would be insecure mixed content warnings.

How would you handle the case where external images are not available via HTTPS? Or are you thinking that perhaps upgrade-insecure-requests could be optional? Or maybe require handling first and/or logging the issues that need fixing?

Per MDN:

Note that, if the requested resource is not actually available via HTTPS, the request will fail without any fallback to HTTP

#8 in reply to: ↑ 7 ; follow-up: @westonruter
5 years ago

Replying to miinasikk:

How would you handle the case where external images are not available via HTTPS? Or are you thinking that perhaps upgrade-insecure-requests could be optional? Or maybe require handling first and/or logging the issues that need fixing?

Yeah, I'm not sure. I think ultimately the insecure external assets would just need to fail. It's something the site owner (or theme/plugin developer) would need to fix (or find a replacement).

Without upgrade-insecure-requests browsers will still load insecure images. Other insecure subresources, scripts in particular, are blocked entirely. So it seems better to have this in place so that the upgrade is performed for the majority of subresources that should be available over HTTPS.

It would be nice if upgrade-insecure-requests could be configured to only upgrade non-image subresources, leaving images to flag insecure content warnings, but that doesn't seem to be the case.

So while upgrade-insecure-requests should be the default, it makes sense that a developer should be able to turn off that default behavior and instead opt to find insecure requests via:

Content-Security-Policy-Report-Only: default-src https:; report-uri /endpoint

This would require the support of a plugin like Reporting API. It's also not something that should be the default for all sites.

The 80% solution here—for the majority of users who don't know how to debug things like this—seems to be just to upgrade-insecure-requests. This should be come less and less of a problem as more sites support HTTPS.

#9 in reply to: ↑ 8 @miinasikk
5 years ago

Replying to westonruter:

The 80% solution here—for the majority of users who don't know how to debug things like this—seems to be just to upgrade-insecure-requests. This should be come less and less of a problem as more sites support HTTPS.

If we would add the upgrade-insecure-requests e.g. by adding 'wp_headers' filter to https-detection.php then this would mean that all the sites that already are using HTTPS would benefit from that. I'm wondering about the cases though where there might be hundreds of posts of which some might use third-party resources which might not support HTTPS, and this would happen without any notice about it after WP update. Not sure if there is a good way to inform about this behavior change, other than stating it in the added Security section. Or do you think it should probably be acceptable as-is since it would work for 80% of the cases? Not sure what's the general policy for these cases, is 80% rule the policy? :)

So while upgrade-insecure-requests should be the default, it makes sense that a developer should be able to turn off that default behavior and instead opt to find insecure requests via:

Content-Security-Policy-Report-Only: default-src https:; report-uri /endpoint

This would require the support of a plugin like Reporting API. It's also not something that should be the default for all sites.

We could add a filter which by default would add the upgrade-insecure-requests but when set to the opposite value (false vs true depending on the filter) would add Content-Security-Policy-Report-Only instead.

#10 @westonruter
5 years ago

If a site switches to HTTPS but they have HTTP resources, then (as I understand):

  1. Without upgrade-insecure-requests, insecure scripts will be blocked and insecure media will cause warnings.
  2. With upgrade-insecure-requests, all insecure resources will automatically be updated to HTTPS. If the hosts for the resources support HTTPS, then everything will work seamlessly. If a hosted resource is not available on HTTPS (scripts and media), then it will fail.

In both cases, the user would need to check their site to check for problems (unless it's for a new install). But my expectation is that upgrade-insecure-requests will generally result in fewer errors, though the ones that do occur (e.g. for images) will be harder (failures instead of warnings). So 2 seems like a better 80% solution.

@miinasikk
5 years ago

Includes adding Content-Security-Policy header with filter to opt out for Content-Security-Policy-Report-Only

@miinasikk
5 years ago

HTTPS section with additional information about Content-Security-Policy

#11 @miinasikk
5 years ago

Added the header + also information about Content-Security-Policy. That will only be visible if the HTTPS is not configured correctly though, the sites already using HTTPS won't see this. Not sure if this makes sense. Thoughts?

#12 follow-up: @flixos90
5 years ago

@westonruter

If a hosted resource is not available on HTTPS (scripts and media), then it will fail.

I'm okay with including upgrade-insecure-requests, but your point here leads me to believe that we should do a bit more to make sure it doesn't break users' sites. How about we also issue a request to a random media file, and potentially even a core asset, to make the same check (only if the root URL is different, e.g. when using a CDN)? Only if all of them support HTTPS, we'll tell the user that their site supports HTTPS. It is admittedly a special case, but probably enough people are using a CDN (and hopefully they're all good enough to support HTTPS) to make this worth having.

@miinasikk

Added the header + also information about Content-Security-Policy.

I'd prefer to not expose this to the user, at least not in here - it's very technical information and most folks will have no idea what it is. If we want to expose it, then I'd prefer just doing it in the Info part of Site Health, with all the other technical data.

After reading the previous discussion and reviewing the patch, I'd prefer if we just set the upgrade-insecure-requests value in the filter and not bothered about the very specific report-only cases. You'd need to use a filter to modify the behavior anyway, and you could just as well remove our filter if you wanted to do so. Let's keep the code simple here, covering the vast majority of cases. What we should probably do though is check, if the Content-Security-Policy header is already set, and if so append our value (maybe we should even check whether it's not already set). We don't wanna override other CSP headers people may be using already.

#13 in reply to: ↑ 12 @miinasikk
5 years ago

Replying to flixos90:

How about we also issue a request to a random media file, and potentially even a core asset, to make the same check (only if the root URL is different, e.g. when using a CDN)? Only if all of them support HTTPS, we'll tell the user that their site supports HTTPS. It is admittedly a special case, but probably enough people are using a CDN (and hopefully they're all good enough to support HTTPS) to make this worth having.

Are you suggesting that we should check if a random media file that's already on the site supports HTTPS and if it doesn't then consider the site not supporting HTTPS?

Some concerns with that:

  • We would check just one random media file. This would mean that it could happen that the chosen file is from CDN but it could also be from some other random source / be a leftover or irrelevant and not accurately reflect the site being ready for HTTPS.
  • How would you choose the random media file, could it differ at different times and have different results?
  • This wouldn't really reflect accurately if the site supports HTTPS, having external resources not supporting HTTPS means that the external site isn't supporting HTTPS, not the local.

If the CDN doesn't support HTTPS then maybe it would be good to change the service, too.

Also, the sites that aren't already supporting HTTPS aren't going to have the CSP header anyway -- these sites would actually need to take steps to convert the site to HTTPS where some issues can be expected. It would probably be unexpected for the sites already supporting HTTPS when the resources suddenly display broken, for those, however, the information about being or not being HTTPS ready wouldn't be relevant anymore. WDYT?

I'd prefer to not expose this to the user, at least not in here - it's very technical information and most folks will have no idea what it is. If we want to expose it, then I'd prefer just doing it in the Info part of Site Health, with all the other technical data.

Fair point that it's very technical information and out of the place. The technical information does make sense under Info more, however, we should first actually check if the header hasn't been overwritten by a filter before displaying the information about it. Also, if we already display the CSP header information then should others be displayed as well? Perhaps you're right and it's not that important to display it in such detail.

It would be good to have information under the Status with the HTTPS information though, however, not sure what would be a good way to tell that "External resources that don't support HTTPS might break after switching to HTTPS -- make sure that you don't have those!". Do you think it'd be unnecessary information for the user and we could just skip all the information part? Perhaps I'm overthinking.

After reading the previous discussion and reviewing the patch, I'd prefer if we just set the upgrade-insecure-requests value in the filter and not bothered about the very specific report-only cases. You'd need to use a filter to modify the behavior anyway, and you could just as well remove our filter if you wanted to do so.

That's true, however, I think that the report-only case would be helpful for transitioning, we could document the filter better to make it more clear what's it for, it's very likely that the report-only header is not something that everyone is aware of and the header would be just removed instead of trying to transition. So having that filter could help.

What we should probably do though is check, if the Content-Security-Policy header is already set, and if so append our value (maybe we should even check whether it's not already set). We don't wanna override other CSP headers people may be using already.

It wouldn't make sense to just append our value without knowing what's there before, that would cause duplicate directives and not really work (e.g. having default-src twice). Checking first if it already exists and adding the header only then is a good point, no need to overwrite the exising efforts.

#14 @flixos90
5 years ago

@miinasikk @westonruter

I wonder whether we should take a step back here and not upgrade-insecure-requests for now, except those that go against the actual site URL. While this can just as well cause issues because of e.g. media or assets on a separate host, I think it keeps the work here more scoped. Exploring upgrade-insecure-requests could be the second step - I'm afraid this gets lost in a can of worms otherwise.

Arguably, most WordPress sites serve all their files from the same origin, so for those the simple HTTP to HTTPS replacement should work. The complexity of checking for files that are served from different origins leads me to think that we should defer that work for now. Potentially it even is plugin territory: For example, a plugin hooking media up a CDN could (if that CDN doesn't already use HTTPS anyway) make use of wp_is_using_https() to act accordingly.

Last but not least, we need to keep in mind that users can at least change their URLs back to HTTP, should any resource unexpectedly cause their site to break.

#15 @westonruter
5 years ago

As noted in No More Mixed Messages About HTTPS, Chromium is going to start blocking all non-HTTP subresources on pages loaded over HTTPS:

In a series of steps starting in Chrome 79, Chrome will gradually move to blocking all mixed content by default. To minimize breakage, we will autoupgrade mixed resources to https://, so sites will continue to work if their subresources are already available over https://. Users will be able to enable a setting to opt out of mixed content blocking on particular websites, and below we’ll describe the resources available to developers to help them find and fix mixed content.

It states that HTTP-resources will be autoupgraded to HTTPS, at least audio, video, and images. However, it doesn't mention scripts or iframes.

So if WordPress served upgrade-insecure-requests it would seem that this would preempt what Chromium is already doing. It would prevent broken media as well eventually prevent broken scripts and iframes which would not get automatically upgraded, as far as I understand.

I'm not sure what Firefox or Safari are planning in this regard.

#16 @flixos90
4 years ago

Revisiting this ticket, I believe we're suffering of some scope creep here, so it would make sense to split this ticket into multiple (closely related, but different) efforts:

  1. Detect HTTPS support and provide guidance
  2. Streamline migrating from HTTP to HTTPS
  3. Upgrade insecure requests when site is using HTTPS

Technically the above order would also be a dependency chain for these three, but discussion on the different efforts can happen separately and focus on one topic.

I'm going to move forward and simplify this ticket to cover 1., then open new tickets for 2. and 3.

#17 @flixos90
4 years ago

  • Component changed from Administration to Security
  • Description modified (diff)
  • Focuses administration added
  • Keywords needs-patch added; 2nd-opinion has-patch removed
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from Streamline detecting and enabling HTTPS to Detect HTTPS support and provide guidance

I've reduced scope of this ticket as mentioned above and opened #51437 and #51438 for the above tasks 2. and 3.

This ticket was mentioned in PR #568 on WordPress/wordpress-develop by felixarntz.


4 years ago
#18

  • Keywords has-patch added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/47577

## Screenshots

HTTPS already supported:
<img width="1064" alt="Screenshot 2020-10-02 at 14 01 56" src="https://user-images.githubusercontent.com/3531426/94969916-67638100-04b8-11eb-9ca7-892b4f23ac61.png">

HTTPS not supported:
<img width="1065" alt="Screenshot 2020-10-02 at 14 02 57" src="https://user-images.githubusercontent.com/3531426/94969972-7d714180-04b8-11eb-8a52-b176972d3c14.png">

#19 @flixos90
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

In the latest commit in the PR https://github.com/WordPress/wordpress-develop/pull/568, I've added unit test coverage. I've also added a fix where now the siteurl option is accessed directly in wp_is_https_supported(). This is necessary because otherwise (via site_url()) the URL's scheme will be adjusted based on whether the current request uses HTTPS or not, which shouldn't affect the URL set here. @westonruter What do you think about this? I would think this may be an issue for the PWA plugin similarly?

#20 @westonruter
4 years ago

@flixos90 Yeah, calling site_url() to determine the underlying URL scheme seems problematic since set_url_scheme() will override that underlying scheme.

So it seems that get_option('siteurl') will work, and it will also account for the case where someone has defined the siteurl via the WP_SITEURL constant, since this is enforced via a lower-level option_siteurl filter rather than the site_url filter.

The only concern I have is if a site is using the site_url filter to manipulate the siteurl to be something else rather than what is returned by get_option('siteurl'). I suppose that would be a rare scenario. But one possibility to ensure to account for that would be to apply the filters yourself:

<?php
$site_url = apply_filters( 'site_url', get_option( 'siteurl' ), '/', null, get_current_blog_id() );

#21 @flixos90
4 years ago

  • Milestone changed from Future Release to 5.7
  • Owner set to flixos90
  • Status changed from new to assigned

#22 follow-up: @flixos90
4 years ago

@westonruter Good point, I've updated the PR to manually re-run the filter. It's a bit of a hack, but ensures consistent functionality. Altering the behavior of siteurl() itself to use the "real" URL scheme by default would be a breaking change and at least controversial, so that's out of scope for this ticket.

I plan to commit this to trunk in a few days.

#23 in reply to: ↑ 22 @westonruter
4 years ago

Replying to flixos90:

@westonruter Good point, I've updated the PR to manually re-run the filter. It's a bit of a hack, but ensures consistent functionality. Altering the behavior of siteurl() itself to use the "real" URL scheme by default would be a breaking change and at least controversial, so that's out of scope for this ticket.

Looks good.

I plan to commit this to trunk in a few days.

I've left a review.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


4 years ago

#25 @Clorith
4 years ago

As @westonruter already did a great run-through of recommended improvements code-wise, I'll just comment on the Site Health implementation it self.

I recall it was mentioned that having a way to reach the host would be convenient here, I believe this was brought up in the core chat while the ticket was discussed, and I think that might make sense. There's already a constant for PHP updates that hosts can make use of to direct users to their own documentation, and I think if HTTPS isn't available not all users know where to go /you'd be surprised how many don't actually know who their webhost is).

Would it make sense to introduce a new method for hosts to provide the URL for their support system (hosts could then filter this how they like to either be generic, or tailored for each clients site, that's up to them, but we'll facilitate it). There are various scenarios where you'd want to involve a host for support, so this is a feature that could be re-used a fair bit, also by plugins or themes looking to make use of functionality that is unsupported, and they could then give their users an actionable path forward.

The existing scenario supports an environment variable of WP_DIRECT_UPDATE_PHP_URL, having WP_HOST_SUPPORT_URL or similar (name not set in stone, just for discussion) might then be useful, with a helper function?

Other than that, I think the Site Health implementation seems simple enough, I'm curious if this might be a great first step into introducing direct actions? This would mean having the action link, which currently takes you to the options page, perform the options update for you, it could then verify the page still loads as expected wit ha loopback (I do love those loopbacks), and revert and direct users to host support if it fails, as that would indicate something is wrong 🤔

#26 @flixos90
4 years ago

@Clorith

The existing scenario supports an environment variable of WP_DIRECT_UPDATE_PHP_URL, having WP_HOST_SUPPORT_URL or similar (name not set in stone, just for discussion) might then be useful, with a helper function?

Thanks for bringing this up, I like it though I think a HTTPS update-specific constant would be more suitable here, and closer to the use-case, similar to how we have one for the PHP update. A general support URL could also be handy in many cases, but I think that's less specific to this ticket and would be better as its own. I suggest we introduce a WP_DIRECT_UPDATE_HTTPS_URL constant for e.g. hosts to modify.

Other than that, I think the Site Health implementation seems simple enough, I'm curious if this might be a great first step into introducing direct actions? This would mean having the action link, which currently takes you to the options page, perform the options update for you, it could then verify the page still loads as expected wit ha loopback

I like that idea as well. However, WordPress itself IMO should first do a better job of facilitating this update, as only updating the URLs is not feasible. #51437 is the ticket to do that, which is gonna be the next focus after this here is committed.

I suggest we apply both of your suggestions for #51437 which is about improving the update experience, where this here is more about improving detection.

#27 @Clorith
4 years ago

Sounds reasonable, and you are absolutely right.

#28 @flixos90
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49904:

Security, Site Health: Detect HTTPS support and encourage switching.

This changeset modifies the Site Health panel for HTTPS to provide more accurate recommendations based on whether the environment is already set up for HTTPS.

  • Introduces wp_is_using_https() to check whether the site is configured to use HTTPS (via its Site Address and WordPress Address).
  • Introduces wp_is_https_supported() to check whether the environment supports HTTPS. This relies on a cron job which periodically checks support using a loopback request.

Props Clorith, flixos90, miinasikk, westonruter.
Fixes #47577.

#29 @flixos90
4 years ago

  • Keywords needs-dev-note added

Requiring a dev-note, in combination with #51437 which is next.

#30 @pento
4 years ago

In 49909:

Site Health: Check that WordPress is installed before scheduling the HTTPS cron.

Trying to schedule cron jobs before WordPress is installed results in DB errors, which is suboptimal.

See #47577.

#31 follow-up: @johnjamesjacoby
4 years ago

A few thoughts.

Some of these new function names do not follow closely the recommended pattern. Relatedly, that means my admittedly aging brain has a harder-than-expected time understanding what they are intended to do without additional study.

  • wp_update_https_detection_errors - update what errors, from where?
  • wp_schedule_https_detection - schedule how and for what? Cron? Deletion/Addition?
  • wp_is_owned_html_output - Owned by who? Me?

wp_update_https_detection_errors seems to be named what it is because it calls update_option() internally (which is an assumption on my part) and doesn't provide much benefit to include that detail in the function name, as the storage mechanism is not contextually relevant. It could be: wp_detect_https_errors, which is vague about its storage but assertive about what it does.

wp_schedule_https_detection is following the pattern laid out by other wp_schedule_ functions, which are inconsistently named as well. update_network_counts, update_checks, delete_old_privace_export_files. Youch. Maybe wp_schedule_update_https_detection is better, even though it's not great?

wp_is_owned_html_output is introducing a new global term related to content ownership. I'd like this to be reconsidered to avoid any possible future collisions with other APIs like roles & caps, multisite networks, etc... For example, #5942 wants to introduce an "Owner" role. If that happens, this function name would probably be undesirable. wp_is_local_html_output maybe? But "local" has similar connotations with development environments... Not sure about this one.

These functions are also all marked as @access private but not prefixed with an underscore. Natch, this is broadly inconsistent, but has been enforced as recently as 5.5.0 with _wp_register_meta_args_allowed_list(). Just want to make doubly sure this was considered and makes sense for these here.

#33 @johnbillion
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as per above

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


4 years ago

#35 in reply to: ↑ 31 @flixos90
4 years ago

Replying to johnjamesjacoby:

wp_update_https_detection_errors seems to be named what it is because it calls update_option() internally (which is an assumption on my part) and doesn't provide much benefit to include that detail in the function name, as the storage mechanism is not contextually relevant. It could be: wp_detect_https_errors, which is vague about its storage but assertive about what it does.

This was named based on other cron hook functions that update an option. wp_detect_https_errors might be an option, but IMO it diverges from the existing naming and the name of the function sounds more like something a 3P developer would be welcome to use, which is not the case, since that cron hook is internal to core. wp_is_https_supported is the "public" function to use.

wp_schedule_https_detection is following the pattern laid out by other wp_schedule_ functions, which are inconsistently named as well. update_network_counts, update_checks, delete_old_privace_export_files. Youch. Maybe wp_schedule_update_https_detection is better, even though it's not great?

The lack of a pattern here is a good point, though discussing this without deciding on a pattern blocks this ticket due to another problem. We could open a separate ticket to establish such a pattern, although arguably that would end up as something to contribute to the WP coding standards and best practices. Regarding your proposal, I don't think that is intuitive because it schedules the wp_https_detection action, not the logic to update that option. Technically a pattern to potentially establish could be wp_schedule_{$hook_name} (i.e. wp_schedule_wp_https_detection), although that reads weird so maybe the pattern should be wp_schedule_{$hook_name_without_prefix} (wp_schedule_https_detection) - the latter is what's currently being used.

wp_is_owned_html_output is introducing a new global term related to content ownership. I'd like this to be reconsidered to avoid any possible future collisions with other APIs like roles & caps, multisite networks, etc... For example, #5942 wants to introduce an "Owner" role. If that happens, this function name would probably be undesirable. wp_is_local_html_output maybe? But "local" has similar connotations with development environments... Not sure about this one.

With this one, I agree there's room for improvement. I like the idea of "local" already better than "owned". For the site checking whether output comes from itself could be indicated by the "local" term - the same way like you could access your own server internally from "localhost" I guess?

These functions are also all marked as @access private but not prefixed with an underscore. Natch, this is broadly inconsistent, but has been enforced as recently as 5.5.0 with _wp_register_meta_args_allowed_list(). Just want to make doubly sure this was considered and makes sense for these here.

This is something I years ago had a conversation about with @boonebgorges, and, while this is not documented anywhere (we definitely lack a best practices/patterns group!), he back then said that underscore-prefixed functions shouldn't be used anymore and that marking them with private is the guidance. I've stuck with that ever since - but I guess since it's not documented, not everyone is aware of that.

Let's keep discussing to see whether it's worth updating these function names and how. I guess we have 1-2 weeks to get this defined, but before RC these should be set in stone.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


4 years ago

This ticket was mentioned in PR #948 on WordPress/wordpress-develop by felixarntz.


4 years ago
#37

This PR addresses follow-up feedback to https://core.trac.wordpress.org/changeset/49904 from https://github.com/WordPress/wordpress-develop/pull/870#issuecomment-763015259 (that PR is related, but targets another ticket).

It also renames a function as suggested by this comment.

Trac ticket: https://core.trac.wordpress.org/ticket/47577

adamsilverstein commented on PR #948:


4 years ago
#38

👍🏼 Overall code changes look great; i'll give this a test locally again as well.

TimothyBJacobs commented on PR #948:


4 years ago
#39

How do you feel about making wp_update_https_detection_errors shortcircuitable? The reason I ask is I maintain a plugin that functions similarly, but uses an external server to ping the site to check for HTTPS to account for instances where a loopback request results in a false negative. Ideally, we'd be able to override the check to use the more accurate detection, but still keep the user in Site Health.

felixarntz commented on PR #948:


4 years ago
#40

@TimothyBJacobs Great suggestion - I'll add that tomorrow.

adamsilverstein commented on PR #948:


4 years ago
#41

@felixarntz I did some testing locally and took some screenshots. Noticed a couple of tiny issues:

Accessing wp-admin via http (with my invalid "self signed" SSL cert), I noticed an issue where the title is getting some extraneous encoding:

https://i0.wp.com/user-images.githubusercontent.com/2676022/106097528-b337d700-60f4-11eb-86ea-95b4755c0224.png

When I changed my WordPress Address to http I got a different message, looks good:

https://i0.wp.com/user-images.githubusercontent.com/2676022/106097691-01e57100-60f5-11eb-9ae5-851b9f4b27fa.png

Site address only set to http, looks good:
https://i0.wp.com/user-images.githubusercontent.com/2676022/106097819-33f6d300-60f5-11eb-9dfe-b1fcb74c0a69.png

Both set to http, switched to accessing wp-admin via http:

There is a slight problem here, the space is missing after the work "There":

https://i0.wp.com/user-images.githubusercontent.com/2676022/106098031-918b1f80-60f5-11eb-94ff-43ab90829349.png

felixarntz commented on PR #948:


4 years ago
#42

@adamsilverstein @TimothyBJacobs Addressed your feedback.

Here are some screenshots of the different states.

HTTPS not set up, not supported
https://i0.wp.com/user-images.githubusercontent.com/3531426/106194324-8373e800-6163-11eb-8686-15fb600a3455.png

HTTPS not set up, but supported
https://i0.wp.com/user-images.githubusercontent.com/3531426/106194376-9686b800-6163-11eb-8189-d3af7c65c4b6.png

HTTPS not set up, but supported, with "WordPress Address" already using HTTPS though
https://i0.wp.com/user-images.githubusercontent.com/3531426/106194416-a8685b00-6163-11eb-99eb-4356ad38b5a8.png

HTTPS set up, but not supported (i.e. there are problems)
https://i0.wp.com/user-images.githubusercontent.com/3531426/106194455-b8803a80-6163-11eb-9c4e-9b6ddba3a497.png

@TimothyBJacobs One question you might know the answer to: Now that I've made the test asynchronous and added a REST endpoint for it, do I need to update wp-api-generated.js? At what point is that generated and how?

TimothyBJacobs commented on PR #948:


4 years ago
#43

@TimothyBJacobs One question you might know the answer to: Now that I've made the test asynchronous and added a REST endpoint for it, do I need to update wp-api-generated.js? At what point is that generated and how?

wp-api-generated.js gets rebuild when the full test suite is run. I typically handle rebuilding it by running the unit tests in my svn checkout. But I also commit to the PR directly sometimes instead.

#44 @flixos90
4 years ago

In 50072:

Security, Site Health: Improve accuracy in messaging about HTTPS support.

Following up on [49904], this changeset focuses mainly on improving the guidance about the current state of HTTPS in Site Health.

  • Correct the existing copy to indicate that both the Site Address and the WordPress Address need to be changed to fully switch to HTTPS.
  • Link to the respective input fields via anchor links rather than to the overall General Settings screen.
  • Show different copy if the site is using HTTPS for the WordPress Address (for example to have only the administration panel in HTTPS), but not for the Site Address.
  • Inform the user about potential problems even when the site is already using HTTPS, for example if the SSL certificate was no longer valid.
  • Always rely on fresh information for determining HTTPS support issues in Site Health, and therefore change the https_status test to become asynchronous.
  • Rename the new private wp_is_owned_html_output() function to a more appropriate wp_is_local_html_output().

Props adamsilverstein, flixos90, johnjamesjacoby, timothyblynjacobs.
See #47577.

TimothyBJacobs commented on PR #958:


4 years ago
#47

Looks great to me!

#48 @flixos90
4 years ago

In 50075:

Security: Allow short-circuiting the wp_update_https_detection_errors() process.

This changeset introduces a pre_wp_update_https_detection_errors filter which can be used to short-circuit the default logic for detecting problems with HTTPS support for the site, by returning a WP_Error object.

Props timothyblynjacobs.
See #47577.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#51 follow-up: @zodiac1978
4 years ago

If we are optimizing for https usage in 5.7, we could maybe add #49515

It seems that the hard work is already done in this ticket(s) here. The mentioned ticket just needs some UI/UX and copy decisions.

#52 in reply to: ↑ 51 @hellofromTonya
4 years ago

Replying to zodiac1978:

If we are optimizing for https usage in 5.7, we could maybe add #49515

It seems that the hard work is already done in this ticket(s) here. The mentioned ticket just needs some UI/UX and copy decisions.

5.7 is now past the deadline (i.e. Beta 1) for all new features and enhancements. #49515 could be a possible 5.8 candidate.

#53 @hellofromTonya
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

After discussing with @flixos90, closing the ticket as the enhancement work has been committed.

#54 @flixos90
4 years ago

In 50391:

Security: Fix bug in wp_is_local_html_output().

Prior to this changeset, the check for the correct RSD link output was relying on a specific protocol, although it needs to accept both the HTTP and HTTPS version of the URL.

Props TimothyBlynJacobs.
Fixes #52542. See #47577.

#56 @juliobox
4 years ago

Hello there
Question: Why don't we just do this 1 line return:

function wp_is_using_https() {
	return wp_is_home_url_using_https() && wp_is_site_url_using_https();
}

thanks

#57 @Rahe
4 years ago

Hello,

Can we add a filter on wp_is_local_html_output function ?
If we disable all the filters, we could add our own check.

Nicolas,

Note: See TracTickets for help on using tickets.