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.

Details

Commits
D20176 / rP66060b294bce: Fix a URI construction in remarkup macro/meme rules
D20160 / rPf77942a2d11b: (stable) Bump the markup cache version for URI changes
D20162 / rPbe21dd3b52b8: Fix some "URI->alter(X, null)" callsites
D20155 / rPHUda84d9c24445: Update "setQueryParam[s]()" calls in libphutil
D20154 / rP5892c7898603: Replace all "setQueryParam()" calls with "remove/replaceQueryParam()"
D20153 / rP241f06c9ffc6: Clean up final `setQueryParams()` callsites
D20151 / rP4c124201623a: Replace "URI->setQueryParams()" after initialization with a constructor argument
D20149 / rPHUf8b3f480f91a: Introduce new PhutilURI API methods to move away from the ambiguity of the…
D20160 / rP991368128e4d: Bump the markup cache version for URI changes
D20150 / rPfcd85b6d7bed: Replace "getRequestURI()->setQueryParams(array())" with "getPath()"
D20147 / rP9e0a954324e5: (stable) Fix "AphrontRequest->getRequestURI()" for requests with "x[]=1"…
D20147 / rP308c4f240754: Fix "AphrontRequest->getRequestURI()" for requests with "x[]=1" parameters in…
D20136 / rPHU70ec97f839b3: (stable) Fix destructive representation of "?x=1&x=2" in PhutilURI so it can…
D20136 / rPHUc6d02efc62c3: Fix destructive representation of "?x=1&x=2" in PhutilURI so it can raise…
D20143 / rPHU8151b3a30e7e: (stable) Let "phlog()" log Throwables
D20142 / rP187356fea515: Let the top-level exception handler dump a stack trace if we reach debug mode…
D20143 / rPHUf430af3cd431: Let "phlog()" log Throwables
D20138 / rPb9a1260ef5c0: Improve top-level fatal exception handling in PHP 7+
D20137 / rPe03079aaaad2: Try harder to present display/rendering exceptions to the user using standard…
D20140 / rP1275326ea6b6: Use "phutil_string_cast()" in TypeaheadDatasource
D20139 / rPHUbbd53c622271: Introduce "phutil_string_cast()" to work around interesting choices in "…
D20134 / rP905564db6b6d: (stable) Allow typeaheads to pass nonscalar data to datasources
D20134 / rP711871f6bc74: Allow typeaheads to pass nonscalar data to datasources

Event Timeline

epriestley triaged this task as Normal priority.
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.
epriestley updated the task description. (Show Details)Feb 11 2019, 2:38 PM

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.

epriestley updated the task description. (Show Details)Feb 11 2019, 11:54 PM

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.

epriestley closed this task as Resolved.Feb 16 2019, 3:19 AM

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

(See T13251 for followup.)