WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#28487 closed enhancement (maybelater)

Introduce a function to determine if the scheme of a URL is https

Reported by: johnbillion Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch
Focuses: Cc:
PR Number:

Description

[28674] introduces some logic which determines if the home URL's scheme is https. We should abstract this logic into a helper function which can be reused. I'll be using it in a patch on #28427 at least.

Adding to the Security component as this forms part of our work on improving SSL in 4.0.

Attachments (3)

28487.diff (619 bytes) - added by johnbillion 5 years ago.
28487.is_ssl.diff (1.1 KB) - added by georgestephanis 5 years ago.
28487.2.diff (1.4 KB) - added by johnbillion 5 years ago.

Download all attachments as: .zip

Change History (17)

@johnbillion
5 years ago

#1 @johnbillion
5 years ago

  • Keywords has-patch added

Patch

#2 follow-up: @SergeyBiryukov
5 years ago

To avoid confusion with is_ssl(), I'd suggest something like is_https_url() as a name.

#3 @nacin
5 years ago

Hmm. Not sure this is necessary. Probably would cause more confusion than not.

Still trying to see how things shake out and whether we need functions to wrap the https check for home/siteurl, versus a generic helper.

#4 in reply to: ↑ 2 @johnbillion
5 years ago

My patch on #28427 demonstrates the use case for this.

Replying to SergeyBiryukov:

To avoid confusion with is_ssl(), I'd suggest something like is_https_url() as a name.

Agreed.

#5 @georgestephanis
5 years ago

Just added 28487.is_ssl.diff -- which integrates this functionality into the existing is_ssl function as a possible alternative placement.

The existing code and new code both check things via rather discrete methods, so I'm unsure if it's best to logically clump them in the same function, but just a possibility to chew on.

#6 @georgestephanis
5 years ago

Also, I got something confused in my head when I wrote the patch, thinking parse_url gets cranky if passes a non-URL string, whereas it just returns false. The @ prefixing it is unnecessary and should be dropped. My bad.

@johnbillion
5 years ago

#7 @johnbillion
5 years ago

In 28894:

Introduce is_https_url() for testing whether the scheme for a given URL is https. See #28487.

#8 @johnbillion
5 years ago

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

I've gone ahead and dropped this function in in order to move some other SSL tickets along. If we decide to go with specific functions for home and siteurl rather than this generic helper, that's fine and we can re-open this.

#9 @nacin
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm concerned about limited utility and the ambiguity this introduces with it existing alongside is_ssl(). I suggest we remove it for now and return to a parse_url() check, since that's more clear anyway.

#10 @ocean90
5 years ago

In 29309:

Revert [28894] as it's currently unused in core.

see #28487.

#11 @ocean90
5 years ago

  • Milestone 4.0 deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

#12 @ocean90
5 years ago

In 29310:

Remove unit test for is_https_url(), see [29309].

see #28487.

#13 @ocean90
5 years ago

In 29311:

Replace is_https_url() with 'https' === parse_url( $url, PHP_URL_SCHEME ).

see #28427, #28487.

#14 @ocean90
5 years ago

My main reason for the revert was the naming of the function. I agree to nacin with the proximity to is_ssl(). For that 28487.is_ssl.diff looks much better. Otherwise I think has_url_scheme( $url, $scheme ) or get_url_scheme( $url ) would reduce the ambiguity with is_ssl() and aligns more with set_url_scheme().

Note: See TracTickets for help on using tickets.