Page MenuHomePhabricator

Some typeaheads use nonscalar HTTP parameters, which fatal under new "phutil_build_http_query()" rules
Closed, ResolvedPublic

Description

See PHI1066. See https://discourse.phabricator-community.org/t/maniphest-is-giving-http-500-error/2385. See D20116. Some datasources currently rely on Datasource->setParameters(...) accepting nonscalar values (usually, lists of PHIDs). This no longer works after D20049 and fatals explicitly after D20116.

Affected typeaheads include:

  • The branch selector in "Land Revision".
  • Some custom field search typeaheads, e.g. on /maniphest/ with particular custom fields configured.

The exception itself is also opaque:

>>> UNRECOVERABLE FATAL ERROR <<<

Method PhutilURI::__toString() must not throw an exception

/home/.../phabricator/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php:0


┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻

See also PHI1069.

Revisions and Commits

rPHU libphutil
D20155
D20149
D20136
D20136
D20143
D20143
D20139
rP Phabricator
D20176
D20160
D20162
D20154
D20153
D20151
D20160
D20150
D20147
D20147
D20142
D20138
D20137
D20140
D20134
D20134

Event Timeline

epriestley triaged this task as Normal priority.Feb 11 2019, 2:34 PM
epriestley created this task.
epriestley renamed this task from Some typeaheads use nonscalar HTTP parameters to Some typeaheads use nonscalar HTTP parameters, which fatal under new "phutil_build_http_query()" rules.Feb 11 2019, 2:34 PM
epriestley added a project: Typeahead.

The error is less opaque in PHP 7.3:

>>> UNRECOVERABLE FATAL ERROR <<<

Method PhutilURI::__toString() must not throw an exception, caught Exception: HTTP query parameter (with key &quot;repositoryPHIDs&quot;) is not a scalar. Parameters must all be scalars.

/Users/epriestley/dev/core/lib/phabricator/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php:0


┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻

I'm strongly considering allowing phutil_build_http_querystring() to build PHP-flavored query strings out of arrays again.

One problem right now is that this code:

$str = 'http://example.com/?X';
$uri = new PhutilURI($str);
(string)$uri;

...can fatal on the string cast for some values of X. It seems like:

  • If the (string) cast would fatal, the fatal should probably be moved to construction time (new PhutilURI()) instead.
  • No "reasonable" value of X should fatal (maybe NULL bytes or invalid UTF8 are allowed to fatal, but normal character strings should not fatal).

Today, we use the PHP-flavored x[]=1&x[]=2 query string construction in some UIs. For example, the checkboxes in Slowvote approval votes are constructed like this. These aren't exceptionally common, and it's possible we should move away from them, but they're hard to grep for and may be difficult to exorcise decisively. If we shift the fatal to construction time, some actual Phabricator URIs will not be valid PhutilURIs.

Maybe the cleaner approach is to change the PhutilURI parameter representation to allow duplicates and use parseQueryStringToPairList(), not parseQueryString(). I'm leaning in this direction? Then we can shift the fatal without creating any weird contradictions.

There are a very small number of noncritical setQueryParam() callsites in arcanist/ (3), instances/ (1), and services/ (3). Of these 7 callsites, 6 are about building Diviner links. I'm going to leave these as-is for now to generally simplify rolling this out, but they should be updated before setQueryParam() is actually removed.

I believe all (?) of these are now fixed in both master and stable.