Page MenuHomePhabricator

GET variable for load.php might exceed 512 bytes
Closed, ResolvedPublic

Description

Author: ivan

Description:
Perhaps this is not a bug, but I don't know where to document it as I thought some users will experience the same problem. Please point me to where I can document this.

My hosting provider use suhosin. I found that on some pages (in my case, a LiquidThreads talk page) the value of GET variable for load.php exceeds 512 bytes, the default maximum value length of suhosin.

Here's the sample:

debug=false&lang=id&modules=ext.liquidThreads%7Cjquery.autoEllipsis%7Cjquery.checkboxShiftClick%7Cjquery.client%7Cjquery.collapsibleTabs%7Cjquery.cookie%7Cjquery.delayedBind%7Cjquery.highlightText%7Cjquery.makeCollapsible%7Cjquery.messageBox%7Cjquery.placeholder%7Cjquery.suggestions%7Cjquery.tabIndex%7Cjquery.tablesorter%7Cjquery.ui.button%7Cjquery.ui.core%7Cjquery.ui.dialog%7Cjquery.ui.draggable%7Cjquery.ui.mouse%7Cjquery.ui.position%7Cjquery.ui.resizable%7Cjquery.ui.widget%7Cmediawiki.action.watch.ajax%7Cmediawiki.language%7Cmediawiki.legacy.ajax%7Cmediawiki.legacy.wikibits%7Cmediawiki.util&skin=vector

Workaround: Sets suhosin.get.max_value_length in PHP ini to a number greater than 512. I use 1024.

suhosin.get.max_value_length = 1024


Version: 1.18.x
Severity: normal

Details

Reference
bz28738

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:28 PM
bzimport set Reference to bz28738.

Another solution would be to automatically split up requests on the client if the get query is going to be too long. I prefer however the server config change option.

I think this should be detected during installation, right? Printing a warning then would be a good time to tell the person installing that they need to change things.

(In reply to comment #2)

I think this should be detected during installation, right? Printing a warning
then would be a good time to tell the person installing that they need to
change things.

Good idea.

Possible URL reduction method:

&modules=jquery:autoEllipsis,checkboxShiftClick,client,collapsibleTabs|jquery.ui:button,core,dialog

In addition to saving on module names, it also sends 1 byte for comma instead of 3 bytes for pipe which you have to percent-encode.

Note that an installation-time check isn't necessarily sufficient -- if your hosting provider changes their configuration later on, you won't be warned.

Max's suggestion at shortening the URLs by collapsing out common prefixes is quite clever, though dependency ordering may mean that doesn't always work in an optimized fashion. It also won't protect against a few more modules coming in and bumping it beyond the limit again...

(In reply to comment #5)

Note that an installation-time check isn't necessarily sufficient -- if your
hosting provider changes their configuration later on, you won't be warned.

Max's suggestion at shortening the URLs by collapsing out common prefixes is
quite clever, though dependency ordering may mean that doesn't always work in
an optimized fashion. It also won't protect against a few more modules coming
in and bumping it beyond the limit again...

Dependency ordering is irrelevant. The order of modules in the query string determines the order in which they are delivered, yes, but the client-side loader is smart enough to execute everything in the right order regardless of which order the delivery order.

(In reply to comment #6)

Dependency ordering is irrelevant. The order of modules in the query string
determines the order in which they are delivered, yes, but the client-side
loader is smart enough to execute everything in the right order regardless of
which order the delivery order.

Furthermore, the module list is always sorted before it's turnd into query string.

Note that an installation-time check isn't necessarily sufficient --
if your hosting provider changes their configuration later on, you
won't be warned.

Agreed. This really goes for almost all the warnings at installation.

Further, I like WordPress's admin interface that lets you know when an
update is available for WP itself or for one of the WP extensions. I
think some work has already been done in that way,

So, I probably need to file a new bug requesting this admin page
feature that would provide all sorts of warnings (“Security upgrade
available”, “Suhosin flags are wrong”, etc.

(In reply to comment #1)

Another solution would be to automatically split up requests on the client if
the get query is going to be too long. I prefer however the server config
change option.

This is by far the most robust solution, in my opinion.

I'll work on this today and implement both Trevor/Andrew's suggestion (split up requests if too long) and Max's suggestion (shorten URLs drastically so this hopefully doesn't ever happen in practice). I would like requests to be broken up as infrequently as possible, because every time that happens we take a performance hit.

(In reply to comment #11)

I'll work on this today and implement both Trevor/Andrew's suggestion (split up
requests if too long) and Max's suggestion (shorten URLs drastically so this
hopefully doesn't ever happen in practice). I would like requests to be broken
up as infrequently as possible, because every time that happens we take a
performance hit.

Try doing it with a config option, that way those who can optimize their setup to avoid splitting up requests where they would be by default.

Configurable requests splitting implemented in r87203. I'll refine this later by checking for suhosin.get.max_value_length in the installer and setting $wgResourceLoaderMaxQueryLength appropriately in the generated LocalSettings.php file, and by implementing the parameter shortening scheme MaxSem suggested.

Installer check added in r87494. This makes sure fresh installs with suhosin.get.max_value_length set will have the appropriate value for $wgResourceLoaderMaxQueryLength in their LocalSettings.php ; not sure how to do this cleanly for upgrades, will ask Chad.

(In reply to comment #4)

In addition to saving on module names, it also sends 1 byte for comma instead
of 3 bytes for pipe which you have to percent-encode.

Turns out that's not true: jQuery is encoding my commas and colons as well.

(In reply to comment #4)

Possible URL reduction method:

&modules=jquery:autoEllipsis,checkboxShiftClick,client,collapsibleTabs|jquery.ui:button,core,dialog

Implemented this in r87497, except that I'm using dots instead of colons because 1) that's one fewer character with a special meaning and 2) the fact that a dot is one character after urlencoding but a colon is three is inconvenient when doing the query string length bookkeeping required to avoid hitting the limit.

So the URL from your example now looks like this:

&modules=jquery.autoEllipsis,checkboxShiftClick,client,collapsibleTabs|jquery.ui.button,core,dialog

Closing this bug as FIXED because I believe I've done every reasonable thing that can be done to address this issue. Maybe we can do something in the upgrader too, but I'll have to ask Chad about that.

  • Bug 29056 has been marked as a duplicate of this bug. ***