WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#19037 closed enhancement (fixed)

Patch for is_ssl, ssl_redirect and general http/https logic / bug

Reported by: MarcusPope Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.2.1
Component: Security Keywords: has-patch
Focuses: Cc:

Description

WP core uses several different methods for determining if the site is using SSL, and whether to redirect to an SSL scheme. Additionally some of those cases use complete logic checks and some only use partial checks.

This patch refactors the copy-pasted logic throughout several files and offers a unified approach to getting the correct scheme.

I've also removed parenthesis from is_ssl in code comments to reduce the number of false positive results when grepping for usage of is_ssl in the code.

site-info.php mistakenly reports the homepage url scheme as the scheme currently being used to browse the page and not the intended scheme of the site.

feed.php now operates on fallback assumption that if http is not "on" server_port could still properly catch a correct ssl session.

corrected inappropriate use of "schema" for scheme variable in functions.php

And it provides a central place for implementing future hooks related to http/https selection logic.

Not sure if component:security is the best choice, but http seemed to apply more to http.php than the ssl/non-ssl scheme selection.

Attachments (2)

ssl.patch (22.3 KB) - added by MarcusPope 4 years ago.
patch for ssl checking logic
19037.diff (11.7 KB) - added by ryan 3 years ago.
Partial refresh

Download all attachments as: .zip

Change History (13)

@MarcusPope4 years ago

patch for ssl checking logic

comment:1 @MarcusPope4 years ago

btw, if this patch doesn't properly apply to 3.3 because of other changes for that branch I can probably swing some time from my employer to recreate the patch. As it stands we were having to do an audit of full-site ssl-mode security and this just came with the territory of auditing the code.

Thanks,
Marcus

comment:2 @johnbillion4 years ago

  • Cc johnbillion@… added
  • Type changed from defect (bug) to enhancement

comment:4 @nacin4 years ago

I really like where this patch is going.

Ultimately I'd rather see set_url_scheme() in terms of a function name. I'll leave this ticket open though, as it actually begins to make the changes in core, rather than just introducing the new function.

comment:5 @MarcusPope4 years ago

Thanks for the ecouraging remarks Nacin. From a naming convention perspective I'm completely ignorant AND agnostic to wordpress standards on that front. So, please take this with a grain of salt if necessary :D

Since this function doesn't actually change the scheme in use, perhaps fix_url_scheme() would be more appropriate? I think set_url_scheme would apply more towards a hybrid of ssl_redirect(), force_ssl_admin() and force_ssl_login() where you could set the scheme to be used for future requests. As where this was/is designed to correct the url schemes of existing link generation functions based purely on the current request method.

I'll totally leave this up to you guys though, just giving my outsider's perspective.

comment:6 @marfarma4 years ago

  • Cc pauli.price@… added

Related: #13941, #15928, #18017, #19023, #18005

See wp-hacker's thread titled: "Fixing some SSL cases for 3.4" to follow the evolving discussion.

comment:7 @niklasbr3 years ago

Related: #19337?

comment:8 @johnbillion3 years ago

Related: #20759

Last edited 3 years ago by johnbillion (previous) (diff)

@ryan3 years ago

Partial refresh

comment:9 @ryan3 years ago

  • Milestone changed from Awaiting Review to 3.5

comment:10 @ryan3 years ago

In [21664]:

Use set_url_scheme(). Props johnbillion, MarcusPope. see #19037 #20759

comment:11 @ryan3 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.