Skip to:
Content

Opened 15 months ago

Closed 14 months ago

Last modified 11 months ago

#4761 closed defect (bug) (fixed)

Modify ajax url to work properly with FORCE_SSL_ADMIN

Reported by: will_c Owned by:
Milestone: 1.7 Priority: normal
Severity: major Version: 1.7
Component: Core Keywords:
Cc:

Description

With FORCE_SSL_ADMIN enabled on my site using BP 1.7-trunk and WP 3.5 I am experiencing issues with AJAX that are preventing the activity feed from functioning properly (I cannot make new updates or comment on existing activity items). I believe this problem is caused by having the ajax url using the https protocol rather than http.

To replicate this issue, turn on FORCE_ADMIN_SSL in your wp-config file and then try to post to the newsfeed (while browsing your site over http). In my testing, I receive an error saying:

"Are you sure you want to do this?
Please try again."

If you then switch to https browsing or disable FORCE_SSL_ADMIN, you should be able to post to the activity feed again.

I believe the issue is that admin_url() (in bp-core/bp-core-cssjs.php and bp-templates/bp-legacy/buddypress-functions.php) returns the https variant when FORCE_SSL_ADMIN is enabled (as it should). This causes problems in my setup for sites that are using http on the frontend.

My proposed solution would be to switch to using network_site_url('/wp-admin/admin-ajax.php'), which would also account for multisite installations, or using site_url('/wp-admin/admin-ajax.php') if it's preferable to only deal with single site installs at the moment.

This is my first patch for BuddyPress, so let me know if there are reporting conventions that I should follow in the future. Also, I realize there are probably other approaches to fixing this issue or there may be other logic behind the use of admin_url that I'm missing, so I'd appreciate any feedback.

Attachments (2)

admin_ssl_ajax_fix_patch.txt (1022 bytes) - added by will_c 15 months ago.
4761.01.patch (1.1 KB) - added by r-a-y 14 months ago.
Revert to admin_url(), but use second parameter to define the scheme

Download all attachments as: .zip

Change History (16)

comment:1 DJPaul15 months ago

  • Milestone changed from Awaiting Review to 1.7

comment:2 will_c15 months ago

I was about to report a similar issue with bbPress 2.2.3 (which uses the same approach), but found out that bbPress 2.3 includes a new way to handle AJAX. It is included in http://bbpress.svn.wordpress.org/trunk/includes/common/ajax.php. It looks like bbPress now uses it's own AJAX handler and circumvents the reliance on admin-ajax.php.

I realize this might take a lot of work to implement for BP 1.7, but may be relevant for future releases. I'm happy to submit a new enhancement ticket for this approach, but still think my original suggestion would be a good patch for 1.7 because it will prevent errors for people using force_ssl_admin.

Just wanted to bring this up as an alternate way to address this issue. Not sure if it's already been under discussion previously.

comment:3 DJPaul15 months ago

Kinda relevant: #WP12400

comment:4 r-a-y15 months ago

will_c: Appreciate the patch. I don't have a SSL install to currently verify this, but your thoughts are valid.

If we don't address this for 1.7, you can temporarily filter 'admin_url' and force the AJAX url to use non-SSL.

Reference:
https://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/link-template.php#L1994

Update: Was pretty simple to set up a SSL cert on my local install. Will be testing this some time this week.

Last edited 15 months ago by r-a-y (previous) (diff)

comment:5 r-a-y14 months ago

  • Keywords commit added

will_c: I have duplicated your problem and your fix works.

The question, as you noted in your ticket, is whether we should use site_url() vs. network_site_url().

Since the FORCE_SSL_ADMIN define is in wp-config.php, it probably makes sense to use network_site_url().

If another dev can give some feedback on this, that would be great. Otherwise, I'll commit this.

Thanks for contributing, will_c.

Last edited 14 months ago by r-a-y (previous) (diff)

comment:6 boonebgorges14 months ago

Sounds good to me, r-a-y.

comment:7 r-a-y14 months ago

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

(In [6790]) Use network_site_url() to build the 'ajaxurl' JS variable.

Previously, for sites on HTTPS and using the 'FORCE_SSL_ADMIN' constant,
when viewing the frontend of the site on HTTP, the 'ajaxurl' JS
variable would still use HTTPS due to the usage of admin_url().

This would break most BP AJAX actions that use check_admin_referer() to
verify the nonce.

Switching admin_url() to use network_site_url() fixes this problem.

Fixes #4761. Props will_c.

comment:8 boonebgorges14 months ago

  • Keywords needs-patch needs-unit-tests added; dev-feedback commit removed
  • Resolution fixed deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

The changes introduced in r6790 do not work properly when BP_ROOT_BLOG is defined as something other than the default. That's because network_site_url() will always point to the main site's wp-admin, and on some setups, some network users do not have access to any wp-admin URLs on the main site. Thus, for those users, AJAX requests fail silently. See #4850

I'll try to figure something out (and write some tests so we don't get in this mess again). But in the meantime, if we can't figure out a solution we are confident with, we should revert r6790 for 1.7, since the HTTPS issue here is not a regression, but breaking AJAX for alternative root blog setups *is*.

comment:9 r-a-y14 months ago

  • Keywords dev-feedback added

How about?

admin_url( 'admin-ajax.php', is_ssl() ? 'admin' : 'http' )

The second parameter in admin_url() can be used to enforce which scheme to use.

I've tested this with FORCE_SSL_ADMIN defined and the AJAX URL does not use SSL when the frontend of the site is on HTTP.

Last edited 14 months ago by r-a-y (previous) (diff)

r-a-y14 months ago

Revert to admin_url(), but use second parameter to define the scheme

comment:10 r-a-y14 months ago

  • Keywords has-patch added; needs-patch removed

comment:11 boonebgorges14 months ago

  • Keywords needs-unit-tests dev-feedback has-patch removed

This looks good, r-a-y. I wrote some tests for it and it looks like it's passing all the problematic cases: https://github.com/buddypress/BuddyPress-Unit-Tests/commit/5cec5a154f568c83fd97f1e8ce5333a9183f2a4d

comment:12 boonebgorges14 months ago

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

(In [6826]) Improvements in the way ajaxurl is determined

Introduces bp_core_ajax_url() for easier unit testing

Switches bp_core_ajax_url() back to using admin_url(), but now it's more
sensitive to various HTTPS setups, including FORCE_SSL_ADMIN.

Corresponding unit test: https://github.com/buddypress/BuddyPress-Unit-Tests/commit/5cec5a154f568c83fd97f1e8ce5333a9183f2a4d

Partially reverts r6790. See #4850.

Fixes #4761

Props r-a-y

comment:13 will_c13 months ago

Apologies for the trouble boonebgorges. Thanks to you and r-a-y for the solution.

comment:14 boonebgorges11 months ago

In 7059:

Fixes test_bp_core_ajax_url() for new multisite unit test global manipulation

The changes introduced in r7058, which adds improvemens to go_to() for use on
multisite installations, uncovered a limitation in the test_bp_core_ajax_url()
test. Basically, it shows that bp_core_ajax_url() will always point to
admin-ajax.php of the current site, *not* the root blog. This should not be a
problem in most cases, though we should keep an eye out in the future to see
whether problems arise on mapped domains, where cross-domain AJAX calls may be
affected.

See #4761

Note: See TracTickets for help on using tickets.