Accept Conduit parameter values as strings (e.g. from curl) and convert to required type.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T10456: Conduit interprets all arguments from curl requests as strings
- Commits
- rPc71bb0550c5d: Conduit accept int/bool parameters as strings
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=1Fixes T10456.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- conduit-accept-strings
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 14081 Build 18268: arc lint + arc unit
Event Timeline
Comment Actions
- 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.
Comment Actions
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.
Comment Actions
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.