WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#7001 closed defect (bug) (fixed)

Admin SSL Support

Reported by: ryan Owned by:
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

Improve out-of-the-box support for visiting the admin over SSL. The default behavior should be to allow visiting the admin over http or https with an option to force https. There should be separate secure and non-secure login cookies. The secure cookie should be delivered only over SSL. Let's consider a flag that will bind the secure cookie to an SSL session id.

Implementation suggestions:

  • Define SECURE_AUTH_COOKIE
  • Set secure cookie for SSL-only delivery.
  • Optionally force https only logins with FORCE_HTTPS_LOGIN type define
  • Wrap HTTPS == 'on' check in is_ssl() function
  • Add admin_url() and includes_url() functions that will create https links if is_ssl()
  • Use admin_url() and includes_url() for all JS and CSS links.

Attachments (7)

admin_ssl.diff (8.2 KB) - added by ryan 7 years ago.
admin_ssl-1.diff (8.1 KB) - added by tellyworth 7 years ago.
adds a site_url() function
admin_ssl-2.diff (8.2 KB) - added by tellyworth 7 years ago.
basic validation, make is_ssl() case insensitive
admin_ssl.2.diff (10.2 KB) - added by ryan 7 years ago.
Use site_url() in script loader.
admin_ssl.3.diff (22.8 KB) - added by ryan 7 years ago.
More use of admin_url() and site_url()
admin_ssl.4.diff (24.7 KB) - added by ryan 7 years ago.
cookie_split.diff (19.8 KB) - added by ryan 7 years ago.
Separate logged in and auth cookies. Deliver auth cookies only for wp-admin

Download all attachments as: .zip

Change History (30)

@ryan7 years ago

comment:1 @ryan7 years ago

Patch partially implements suggestions. admin_url() and includes_url() need to spread to the script loader and beyond.

comment:2 @tellyworth7 years ago

I added some quick initial unit tests in TestSSLLinks:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_link_functions.php

Some quick suggestions:

  • is_ssl() should do a case insensitive check on the value
  • the block of code for fetching the base URL with http/https should be a separate function, it'll be useful elsewhere
  • caching the base value in $_wp_admin_url makes it difficult to test (and introduces unexpected behaviour: if siteurl changes, admin_url() should reflect that change)
  • behaviour for invalid $path values should be defined and it needs validation

@tellyworth7 years ago

adds a site_url() function

@tellyworth7 years ago

basic validation, make is_ssl() case insensitive

comment:3 @matt7 years ago

All of this breaks if you link to a non-SSL image etc in a post.

comment:4 @ryan7 years ago

Related #6474

@ryan7 years ago

Use site_url() in script loader.

@ryan7 years ago

More use of admin_url() and site_url()

comment:5 @ryan7 years ago

That gets most everything in the admin using SSL links. Image attachment links are still to do.

@ryan7 years ago

comment:6 @ryan7 years ago

(In [7998]) First cut and better admin SSL support. see #7001

comment:7 @ryan7 years ago

(In [8058]) More use of site_url(), admin_url(), and site_url(). Force login and admin links to be https if FORCE_SSL_LOGIN. see #7001

comment:8 @ryan7 years ago

(In [8061]) Don't forget to echo. Props DD32. fixes #7107 see #7001

@ryan7 years ago

Separate logged in and auth cookies. Deliver auth cookies only for wp-admin

comment:9 @ryan7 years ago

Patch adds a new cookie and changes cookie delivery. With this there are now three cookies:

  • wordpress - Auth cookie delivered for wp-admin for SSL and non-SSL sessions
  • wordpress_sec - Auth cookie delivered for wp-admin for SSL sessions only
  • wordpress_logged_in - Non-auth cookie delivered across the entire blog used to determine if a user is logged in.

wordpress and wordpress_sec are delivered only for wp-admin. These cookies will not be delivered for front page visits (at least on the browsers I tested). This prevents front page XSS from fiddling with them. The wordpress_logged_in cookie is delivered for the front page but cannot be used to get into the admin.

FORCE_SSL_LOGIN can be set to true to force all logins to happen over SSL. FORCE_SSL_ADMIN forces all logins and all admin sessions to be over SSL.

FORCE_SSL_LOGIN is for when you want to secure logins so that passwords are not sent in the clear but still want to allow non-SSL admin sessions (since SSL can be so damn slow). FORCE_SSL_ADMIN is for when you want to lock down logins and the admin so that both passwords and cookies are never sent in the clear.

comment:10 @ryan7 years ago

(In [8069]) Introduce logged_in cookie. Deliver auth cookies only to wp-admin. see #7001

comment:11 @ryan7 years ago

(In [8190]) SSL fixes. see #7001

comment:12 @ryan7 years ago

(In [8197]) Only use SSL for login POST links if SSL logins are forced. Clear old cookies. see #7001

comment:13 @alexrabe7 years ago

Changeset [8069] break some of my plugins which includes as well wp-admin/admin.php or the function auth_redirect(), what is now the desired way to check the login for plugins ?

comment:14 @ryan7 years ago

You're directly including admin.php from a file outside of wp-admin? auth_redirect() needs to be run from within wp-admin. Most plugins bootstrap through admin.php?page= so they are being run within wp-admin.

comment:15 @ryan7 years ago

So, 2.6 has two cookies. "wordpress_logged_in" is a read-only cookie that is delivered for all pages. It indicates that the user is logged in and allows looking at some of that's users private data. is_user_logged_in() checks this cookie. "wordpress" is a read/write cookie that is delivered only for wp-admin/. It has the power to make changes. auth_redirect() checks this cookie. Since it is only delivered for wp-admin/, files in the plugins directory that directly load admin.php will not be authorized. This is a back compat break, which sucks, but it also prevents attacks that mess around with files in the plugins directory from getting at the auth cookie. If direct loading admin.php from the plugins directory is a common practice, I guess we'll have to set an auth cookie for the plugins directory.

comment:16 @alexrabe7 years ago

I used this method for tinymce buttons and also for files (ajax, flash upload) which are called direct. I have seen similar practice for other plugins. If is_used_logged_in() did the same job I can change this for me. Thanks

comment:17 @ryan7 years ago

You can do this:

if ( !is_user_logged_in() )
     auth_redirect();

comment:18 @ryan7 years ago

I'll look at setting a cookie for the plugins directory, though. You won't have to change anything then.

comment:19 @ryan7 years ago

(In [8209]) Set auth cookie for plugins directory to support direct load plugin files that call auth_redirect(). see #7001

comment:20 @ryan7 years ago

Try that out.

comment:21 @alexrabe7 years ago

Thanks, Ryan [8209] work for me.

Anyway, Should I keep the old way or is !is_user_logged_in() a better way for plugins (I never checked the Codex for that) ?

comment:22 @ryan7 years ago

(In [8301]) Introduce content_url(). Don't prepend base url to content url in script loader. see #6938 #7001

comment:23 @ryan7 years ago

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