WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months 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 2 years ago.
patch for ssl checking logic
19037.diff (11.7 KB) - added by ryan 20 months ago.
Partial refresh

Download all attachments as: .zip

Change History (13)

MarcusPope2 years ago

patch for ssl checking logic

comment:1 MarcusPope2 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 johnbillion2 years ago

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

comment:4 nacin2 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 MarcusPope2 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 marfarma2 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 niklasbr2 years ago

Related: #19337?

comment:8 johnbillion22 months ago

Related: #21161

Version 0, edited 22 months ago by johnbillion (next)

ryan20 months ago

Partial refresh

comment:9 ryan20 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:10 ryan20 months ago

In [21664]:

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

comment:11 ryan20 months ago

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