Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#4606 closed defect (bug) (fixed)

Long cookie value can lead to 414 Long Request Error resulting in AJAX failing

Reported by: r-a-y Owned by: r-a-y
Milestone: 1.8 Priority: normal
Severity: normal Version: 1.2
Component: Templates Keywords: has-patch early commit
Cc:

Description

Forgot to post this and just remembered.

In bp_dtheme_ajax_querystring(), we grab the entire $_COOKIE value and store it:
http://buddypress.trac.wordpress.org/browser/tags/1.6.1/bp-themes/bp-default/_inc/ajax.php#L95

What happens is when we run a BP AJAX query, the entire cookie string is passed on as well to wp-load.php. For sites that store many cookies (external scripts, tracking, etc.), this can lead to a 414 Long Request error resulting in AJAX failing.

We should only pass on BP's cookies instead of all the cookies.

Attachments (2)

4606.01.patch (14.3 KB) - added by r-a-y 9 years ago.
4606.02.patch (14.3 KB) - added by r-a-y 9 years ago.
Use local "jq" variable instead of "jQuery"

Download all attachments as: .zip

Change History (12)

#1 @DJPaul
9 years ago

Not sure this is as simple as that. What about plugins that might use their own cookies, and expect it to be passed by this AJAX request?

#2 @r-a-y
9 years ago

I think the approach would be okay if plugin developers used the same "bp-" prefix that BP uses for its own cookies and if we only pass on cookies with the "bp-" prefix.

#3 @johnjamesjacoby
9 years ago

  • Keywords needs-patch added

I'm okay restricting this to BuddyPress only cookies. It's likely that third party components would have their own ajax handlers anyhow, given how complex BuddyPress's is.

Patch, or punt.

@r-a-y
9 years ago

#4 @r-a-y
9 years ago

  • Keywords has-patch added; needs-patch removed

Attached a patch.

Had to do this at the javascript level since the AJAX call relies on it.

I've created a new JS function called bp_get_cookies(), which loops through all available cookies and only passes cookies with the 'bp-' prefix.

Let me know what you guys think!

@r-a-y
9 years ago

Use local "jq" variable instead of "jQuery"

#5 @DJPaul
9 years ago

Would rather 1.8-early this so it can get lots of testing.

#6 @r-a-y
9 years ago

  • Keywords early added
  • Milestone changed from 1.7 to 1.8

I'm okay with bumping this to 1.8.

#7 @boonebgorges
9 years ago

  • Keywords commit added

The patch looks good to me, but I agree it should get in early so that plugin/theme authors can test against it. r-a-y, I'll leave the honors to you :)

#8 @r-a-y
8 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 6940:

Only pass BP-related cookies to BP AJAX queries.

Previously, we would pass all cookies to admin-ajax.php. This could
potentially lead to a '414 Long Request' error because of the length of the
querystring, resulting in the AJAX request failing.

This commit only passes cookies with the 'bp-' prefix to admin-ajax.php,
thus limiting the length of the URI request.

Fixes #4606.

#9 @djpaul
7 years ago

In 9248:

Messages: update autocomplete script to better handle long cookie values.

See #4606 for previous similiar fixes.

Fixes #5959, props psycleuk

#10 @DJPaul
5 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.