Page MenuHomePhabricator

Conduit accept int/bool parameters as strings
ClosedPublic

Authored by gd on Oct 12 2016, 1:32 PM.

Details

Summary

Accept Conduit parameter values as strings (e.g. from curl) and convert to required type.

Test Plan

Call conduit method with int/bool parameter iusing curl and make sure it does not result in validation error, e.g.

$ curl http://$PHABRICATOR_HOST/api/maniphest.search -d api.token=$CONDUIT_TOKEN -d constraints[modifiedEnd]=$(date +%s) -d constraints[hasParents]=true -d limit=1

Fixes T10456.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gd updated this revision to Diff 40187.Oct 12 2016, 1:32 PM
gd retitled this revision from to Conduit accept int/bool parameters as strings.
gd updated this object.
gd edited the test plan for this revision. (Show Details)
gd added a reviewer: epriestley.
gd updated this revision to Diff 40188.Oct 12 2016, 1:56 PM
gd edited edge metadata.
  • s/validateStringList/parseStringList/ to be consistent with other parse<Type>() methods

I think it is a good idea to indicate that we may be returning entirely different value as opposed to just validating the passed one.

epriestley edited edge metadata.Oct 12 2016, 3:48 PM

This weakens JSON parsing, right? Today, if you call Conduit with one of the "pass some JSON" mechanisms -- versus passing raw HTTP parameters -- I think this produces a type error:

{
  "someBooleanParameter": "1"
}

I think that's correct, and that we should retain that behavior. If you're passing a boolean value via a protocol that supports JSON, you should pass an actual boolean.

I think this change will weaken this behavior, so that "1"will now be interpreted as true, even when it is appearing in JSON.

Instead, I think we should apply these more-liberal rules only when the parameters are coming in as raw HTTP parameters, so ?someBooleanParameter=1&... is fine, but "someBooleanParameter": "1" is a type error.

The existing HTTPParameter classes already have logic for liberal type interpretation, so it may make sense to reuse them. Roughly, I imagine something like this:

  • Instead of always taking the same pathway through ConduitParameterType, we add some slightly different pathway when the inbound request is raw HTTP.
  • That pathway applies the more liberal casts (possibly by invoking HTTPParameterType logic?) first, then runs through the normal logic?

The total number of types is fairly small and I don't expect them to expand much, so code reuse is probably not hugely important -- e.g., if trying to use AphrontBoolHTTPParameterType to parse boolean values is cumbersome, just duplicating the logic seems fine. It probably differs in some cases anyway (e.g., we probably should not accept comma-separated string lists for Conduit, but should continue to do so for manually-entered HTTP requests.

Upshot:

  • Don't weaken type validation logic when receiving JSON.
  • Instead, add a "liberal validation" pathway for Conduit calls where the parameters are raw HTTP parameters.
  • Reusing logic from HTTPParameterType classes would be vaguely nice if it isn't a mess, but it probably is a mess.
gd updated this revision to Diff 40223.Oct 14 2016, 11:25 AM
gd edited edge metadata.
  • Relaxed Conduit parameter parsing for non-JSON requests only
gd added a comment.Oct 14 2016, 11:56 AM

HTTPParameterType and ConduitParameterType seems to be more tied to the request source (e.g. web form or conduit) then request data type. Some fields for example are conduit-only and do not even return HTTPParameterType.

The knowledge of data type was lost in controller so what I tried to do is basically take it and pass it all the way down to ConduitParamterType where actual knowledge of expected data types is and where data validation/parsing happens.

epriestley accepted this revision.Oct 14 2016, 1:23 PM
epriestley edited edge metadata.

This seems reasonable to me, thanks!

This revision is now accepted and ready to land.Oct 14 2016, 1:23 PM
This revision was automatically updated to reflect the committed changes.