Explicitly fail if `phutil_build_http_querystring()` is passed a map of nonscalars
ClosedPublic
Actions

Authored by epriestley on Feb 7 2019, 12:34 AM.

Details

Reviewers

Commits

rPHU9adb7f5dbc3c: (stable) Promote 2019 Week 6
rPHU24a884ce0b4d: Explicitly fail if `phutil_build_http_querystring()` is passed a map of…

Summary

See D20114. Somewhat (un-?) surprisingly, http_build_query() accepts an arbitrarily deeply-nested mess of arrays and encodes them in a PHP-specific way.

We generally don't want this to happen, so fail explicitly when it does.

Test Plan

Published a story on master (without D20114) with feed.http-hooks enabled, got a more helpful/explicit error.

Diff Detail

Repository

rPHU libphutil

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 7 2019, 12:34 AM

Harbormaster completed remote builds in B21868: Diff 48028.Feb 7 2019, 12:34 AM

epriestley requested review of this revision.Feb 7 2019, 12:34 AM

amckinley accepted this revision.Feb 7 2019, 8:25 PM

This revision is now accepted and ready to land.Feb 7 2019, 8:25 PM

Closed by commit rPHU24a884ce0b4d: Explicitly fail if `phutil_build_http_querystring()` is passed a map of… (authored by epriestley). · Explain WhyFeb 7 2019, 10:55 PM

This revision was automatically updated to reflect the committed changes.

This broke some custom fields. Specifically, this custom field results in Phabricator fataling:

{
  "test": {
    "name": "Test",
    "type": "users",
    "search": true
  }
}

The error message is:

>>> UNRECOVERABLE FATAL ERROR <<<

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

/usr/local/src/phabricator/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php:0


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

In D20116#251992, @joshuaspence wrote:

This broke some custom fields. Specifically, this custom field results in Phabricator fataling:
[...]

"Land Revision" button is also broken, returns identical error...

>>> UNRECOVERABLE FATAL ERROR <<<

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

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


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

nickhutchinson added a subscriber: nickhutchinson.Feb 11 2019, 11:34 AM

Please report bugs to Discourse, not by making comments on changes or commits. If you believe you know which change caused a problem, you can link to it from your bug report on Discourse. It's easy for us to keep track of threads, and hard to keep track of random comments on already-closed changes. Thanks!

epriestley mentioned this in T13250: Some typeaheads use nonscalar HTTP parameters, which fatal under new "phutil_build_http_query()" rules.Feb 11 2019, 2:34 PM

epriestley mentioned this in D20134: Allow typeaheads to pass nonscalar data to datasources.Feb 11 2019, 2:54 PM

epriestley mentioned this in rP711871f6bc74: Allow typeaheads to pass nonscalar data to datasources.Feb 11 2019, 6:53 PM

In D20116#251999, @epriestley wrote:

Please report bugs to Discourse, not by making comments on changes or commits. If you believe you know which change caused a problem, you can link to it from your bug report on Discourse. It's easy for us to keep track of threads, and hard to keep track of random comments on already-closed changes. Thanks!

Fair enough

epriestley mentioned this in rP905564db6b6d: (stable) Allow typeaheads to pass nonscalar data to datasources.Feb 11 2019, 10:27 PM