[SEMVER-MAJOR] Rework coercion of input arguments #343

bajtos · 2016-08-31T11:29:56Z

Simplify the design of input argument handling.

It's up to the Context and/or the Adapter to convert or coerce
input arguments as needed.
The only responsibility of SharedMethod is to validate input
arguments (check that they match the contract in "accepts").
Define two types of conversion/coercion:
a) When the input argument value comes from a source that preserves
type information (typically JSON), then the argument should be
converted with minimal coercion.
b) When the input argument value comes from a "sloppy" source
(typically string-encoded source like a query string), then
coercion rules should be applied.

Replace "Dynamic" API, which was relying on global singleton registry,
with a new concept of a "TypeRegistry" and a type "Converter".
The new registry is local to each RemoteObjects instance.

Each type converter provides three methods:

fromTypedValue to handle values coming from JSON
fromSloppyValue to handle values coming e.g. from query string
validate which is called by SharedMethod to validate argument
values

To prevent unnecessary try/catch blocks with the associated performance
costs, validation/conversion errors are not thrown but returned via
function return value instead.

The feature where one could encode all input arguments in a single
(query string) parameter "args" is removed.

This patch fixes many subtle bugs and changes the way how edge case
values are treated. The general approach is to make both conversion and
coercion more strict. When we are not sure how to treat an input value,
we rather return HTTP 400 Bad Request than coerce the value incorrectly.

Most notable breaking changes:

null value is accepted only for "object", "array" and "any"
Empty string is coerced to undefined to support ES6 default arguments
JSON requests providing scalar values for array-typed argument are
rejected
Empty value is not converted to an empty array
Array values containing items of wrong type are rejected. For
example, an array containing a string value is rejected when
the input argument expects an array of numbers.
Array items from JSON requests are not coerced. For example,
[true] is no longer coerced to [1] for number arrays,
and the request is subsequently rejected.
Deep members of object arguments are no longer coerced. For example,
a query like ?arg[count]=42 produces { count: '42' } now.
"any" coercion preserves too large numbers as a string, to prevent
losing precision.
Boolean types accepts only four string values:
'true', 'false', '0' and '1'
Values are case-insensitive, i.e. 'TRUE' and 'FaLsE' are work too.
Date type detects "Invalid Date" and rejects such requests.

Last but not least, conversion/validation error messages no longer
include input argument values, in order to prevent reflection-based
attacks.

@gunjpan @richardpringle @deepakrkris Please review.
@ritch @STRML PTAL too, I would like to hear your opinion on this change.
@0candy @davidcheung FYI

This patch is loosely inspired by #264.

The change-set is quite large and it may be too difficult to review all details. Here are the most important parts to check:

the overal design - converters, type registry, what work is done by Context vs. SharedMethod
changes in rest-coercion test suite - is there anything surprising?
changes in seemingly unrelated files (lib/context-base, lib/http-context, lib/http-invocation)

Connect to #312
Connect to strongloop-internal/scrum-loopback#974
Connect to strongloop-internal/scrum-loopback#885

TODO for @bajtos:

Add an entry to 3.0-RELEASE-NOTES
Update loopback@master to work with the new version - Use strong-remoting's new TypeRegistry loopback#2696
Ensure loopback-connector-remote works with the new version - Use the new API for defining remoting types loopback-connector-remote#58

bajtos · 2016-08-31T11:54:25Z

Cross-posting @STRML comment from #312 (comment)

If you're accepting an array input for a field, I don't see any semantic difference between [] (no items) and null (no items). But [] is much more convenient as you can simply iterate.

Here is an example where the difference is important: let's say we have a method find accepting an argument called extraFields. When extraFields is present, the method adds the given fields to the response data. When extraFields is not present, all extra fields are included.

Having said that, I see your point of view @STRML. I think we can achieve what you are asking for by supporting a default value in argument definition:

accepts: {
  arg: 'numbers', type: ['number'], default: []
}

Let's leave the support for default values out of scope of this patch though.

bajtos · 2016-08-31T11:57:30Z

Also: for long time, I though we can land (some of) these changes to 2.x in such way that backwards compatibility is preserved. I changed my mind after I checked the effect of #264 on the rest-coercion test suite, there are too many changes where I am concerned about the impact on existing applications. Our current plan is to release loopback@3 by the end of September, therefore I think it will be easier for all of us if this patch is landed on 3.0 only.

bajtos · 2016-08-31T13:54:26Z

I think the CI results cannot be trusted very much. First of all, this patch is targeting 3.0 release line, which is used in very few downstream projects. Secondly, loopback requires changes in strongloop/loopback#2696 to pass the CI, I am not sure if CIS will pick the right branch in loopback when running downstream tests of this pull request.

richardpringle · 2016-08-31T13:57:28Z

@bajtos,
The strong-remoting build itself was not passing before, that's why I re-triggered the test. It seems to be passing now.

richardpringle · 2016-09-01T13:03:10Z

3.0-RELEASE-NOTES.md

+   losing precision.
+ - Boolean types accepts only four string values:
+    'true', 'false', '0' and '1'
+   Values are case-insensitive, i.e. 'TRUE' and 'FaLsE' are work too.


~~are~~ work too

richardpringle · 2016-09-01T13:12:56Z

The review process could be much faster if this spread out into multiple commits.

richardpringle · 2016-09-01T13:35:10Z

lib/remote-objects.js

+ *     return error ? { error: error } : { value: value };
+ *   },
+ *
+ *   fromSloppyValue: function(ctx, value) {


Maybe add a definition of a "sloppy value"?

Good point. I took the concept of "sloppy" values from #264 but on the second thought the name is not descriptive enough.

The sloppy conversion/coercion is invoked when the value comes from a string-only source like HTTP header or more importantly from a query string parsed via qs.

A sloppy value is one of:

a string

an array containing sloppy values

an object where property values are sloppy

I am tempted to rename fromSloppyValue to fromStringValue, but that would hide the fact that an array or an object can be passed in too.

@ritch @richardpringle Any suggestions for a better name?

I reworked the comments in 0b7df4f to make this matter more clear, PTAL.

ritch · 2016-09-01T18:07:21Z

Removing Dynamic will break a lot of things in LoopBack core.

ritch · 2016-09-01T18:10:53Z

lib/remote-objects.js

 * });
 * ```
 *
- * **Note: the alias `remotes.convert()` is deprecated.**
+ * Under the hood, a converter is created that ensures the input data
+ * is an object (or sloppy value is coerced to an object) and calls


sloppy value
I see this referenced in various places, and I am not sure what it means.

Does this mean the raw value provided from req.param() or equiv?

See #343 (comment)

The sloppy conversion/coercion is invoked when the value comes from a string-only source like HTTP header or more importantly from a query string parsed via qs.

Should I expand this comment to make it more clear what is meant by sloppy values?

bajtos · 2016-09-02T06:56:38Z

@ritch

Removing Dynamic will break a lot of things in LoopBack core.

Not really. We use Dynamic in one place only and the upgrade is trivial - see strongloop/loopback@8262059.

Amir-61 · 2016-09-07T03:22:59Z

lib/context-base.js

+  assert(method && typeof method === 'object',
+    'method must be a SharedClass instance');
+  assert(typeRegistry && typeof typeRegistry === 'object',
+    'typeRegistry must be a TypeRegistry instance');


We have used assert(...) a lot in dao.js as well; they are so neat; however I just wonder if the assertion fails, does it dump the stack trace?

Yes, failed assertions dump the stack trace. That way it's easy to find the place in user code that's calling ContextBase with incorrect arguments.

Amir-61 · 2016-09-07T03:56:12Z

lib/type-registry.js

+    if (this._options.warnWhenOverridingType !== false)
+      g.warn('Warning: overriding remoting type %s', typeName);
+    else
+      debug('Warning: overriding remoting type %s', typeName);


Actually I love this ^ syntax; I mean skipping curly braces for one line if/else, but is this what team follows? It reminds me of coffee script too :-)

I don't think we have a rule for that (yet). Since eslint checks are passing, the style should be good.

TBH, I don't really mind. If you think I should add curly braces, then I'll do so. Please let me know.

Thats fine; TBH I like it too 👍 IIRC eslint used to be complaining about that.

Amir-61 · 2016-09-07T03:57:20Z

@bajtos I reviewed almost one thirds of your pull request tonight; I added a bunch of comments, mostly questions and some suggestions. I'll try to review the rest tomorrow to unblock you :-)

Amir-61 · 2016-09-07T04:06:26Z

lib/types/any.js

+  },
+
+  fromSloppyValue: function(ctx, value) {
+    if (value === 'null' || value === null)


Question: If value is a string of null; is it always lowercase null and not like NULL?

Yes, only lowercase null is recognized right now. Do you think this should be changed?

bajtos · 2016-09-07T09:08:39Z

I reviewed almost one thirds of your pull request tonight; I added a bunch of comments, mostly questions and some suggestions. I'll try to review the rest tomorrow to unblock you :-)

@Amir-61 you are awesome, thank you!

Amir-61 · 2016-09-07T14:40:46Z

lib/type-registry.js

+  assert(typeof typeName === 'string' && typeName,
+    'typeName must be a non-empty string');
+  assert(typeof converter === 'object' && converter,
+    'converter must be an object');


Are you just checking the type of converter. In this case shouldn't be like the following?

assert(typeof converter === 'object', 'converter must be an object');

Unfortunately, typeof null is also object:

> var converter = null undefined > typeof converter === 'object' true

Oh right...I missed that! 👍

Amir-61 · 2016-09-07T19:29:22Z

Hey @bajtos,
I went through all changes and changes LGTM; I'm just not 100% sure if the changes break things in LoopBack core module which can be hard to detect. However, all I can say is tests for dependants (including Loopback) are all passing.

Please squash the commits and rebase them on top of master.

Simplify the design of input argument handling. - It's up to the Context and/or the Adapter to convert or coerce input arguments as needed. - The only responsibility of SharedMethod is to validate input arguments (check that they match the contract in "accepts"). - Define two types of conversion/coercion: a) When the input argument value comes from a source that preserves type information (typically JSON), then the argument should be converted with minimal coercion. b) When the input argument value comes from a "sloppy" source (typically string-encoded source like a query string), then coercion rules should be applied. Replace "Dynamic" API, which was relying on global singleton registry, with a new concept of a "TypeRegistry" and a type "Converter". The new registry is local to each RemoteObjects instance. Each type converter provides three methods: - `fromTypedValue` to handle values coming from JSON - `fromSloppyValue` to handle values coming e.g. from query string - `validate` which is called by SharedMethod to validate argument values To prevent unnecessary try/catch blocks with the associated performance costs, validation/conversion errors are not thrown but returned via function return value instead. The feature where one could encode all input arguments in a single (query string) parameter "args" is removed. This patch fixes many subtle bugs and changes the way how edge case values are treated. The general approach is to make both conversion and coercion more strict. When we are not sure how to treat an input value, we rather return HTTP 400 Bad Request than coerce the value incorrectly. Most notable breaking changes: - `null` value is accepted only for "object", "array" and "any" - Empty string is coerced to undefined to support ES6 default arguments - JSON requests providing scalar values for array-typed argument are rejected - Empty value is not converted to an empty array - Array values containing items of wrong type are rejected. For example, an array containing a string value is rejected when the input argument expects an array of numbers. - Array items from JSON requests are not coerced. For example, `[true]` is no longer coerced to `[1]` for number arrays, and the request is subsequently rejected. - Deep members of object arguments are no longer coerced. For example, a query like `?arg[count]=42` produces `{ count: '42' }` now. - "any" coercion preserves too large numbers as a string, to prevent losing precision. - Boolean types accepts only four string values: 'true', 'false', '0' and '1' Values are case-insensitive, i.e. 'TRUE' and 'FaLsE' are work too. - Date type detects "Invalid Date" and rejects such requests. Last but not least, conversion/validation error messages no longer include input argument values, in order to prevent reflection-based attacks.

bajtos assigned ritch, richardpringle and gunjpan Aug 31, 2016

bajtos added the #review label Aug 31, 2016

bajtos added this to the #Epic: Coercion Cleanup milestone Aug 31, 2016

This was referenced Aug 31, 2016

Use strong-remoting's new TypeRegistry strongloop/loopback#2696

Merged

Incorrect dependency on juggler and strong-remoting strongloop/loopback-connector-remote#5

Closed

bajtos mentioned this pull request Sep 1, 2016

Use the new API for defining remoting types strongloop/loopback-connector-remote#58

Merged

3 tasks

richardpringle reviewed Sep 1, 2016
View reviewed changes

bajtos assigned deepakrkris Sep 1, 2016

richardpringle reviewed Sep 1, 2016
View reviewed changes

ritch reviewed Sep 1, 2016
View reviewed changes

bajtos mentioned this pull request Sep 6, 2016

Coercion refactor #264

Closed

Amir-61 reviewed Sep 7, 2016
View reviewed changes

bajtos force-pushed the feature/coercion-overhaul branch from 437eab4 to 6aaafd0 Compare September 9, 2016 07:11

bajtos merged commit 13a0710 into master Sep 9, 2016

bajtos deleted the feature/coercion-overhaul branch September 9, 2016 07:26

bajtos removed the #review label Sep 9, 2016

bajtos mentioned this pull request Sep 12, 2016

[SEMVER-MAJOR] Fix object type conversion #347

Merged

richardpringle removed their assignment Sep 12, 2016

bajtos mentioned this pull request Nov 10, 2016

Loopback try to convert strings with numeric values into integer values strongloop/loopback#1870

Closed

bajtos mentioned this pull request Apr 27, 2018

[PoC - WIP] very rough PoC on request parameter type coercion/validation loopbackio/loopback-next#1256

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEMVER-MAJOR] Rework coercion of input arguments #343

[SEMVER-MAJOR] Rework coercion of input arguments #343

bajtos commented Aug 31, 2016 •

edited

Loading

bajtos commented Aug 31, 2016

bajtos commented Aug 31, 2016

bajtos commented Aug 31, 2016

richardpringle commented Aug 31, 2016

richardpringle Sep 1, 2016

richardpringle commented Sep 1, 2016

richardpringle Sep 1, 2016

bajtos Sep 2, 2016 •

edited

Loading

ritch commented Sep 1, 2016

ritch Sep 1, 2016

bajtos Sep 2, 2016

bajtos commented Sep 2, 2016

Amir-61 Sep 7, 2016 •

edited

Loading

bajtos Sep 7, 2016

Amir-61 Sep 7, 2016 •

edited

Loading

bajtos Sep 7, 2016

Amir-61 Sep 7, 2016

Amir-61 commented Sep 7, 2016 •

edited

Loading

Amir-61 Sep 7, 2016 •

edited

Loading

bajtos Sep 7, 2016

bajtos commented Sep 7, 2016

Amir-61 Sep 7, 2016

bajtos Sep 8, 2016

Amir-61 Sep 8, 2016

Amir-61 commented Sep 7, 2016 •

edited

Loading

[SEMVER-MAJOR] Rework coercion of input arguments #343

[SEMVER-MAJOR] Rework coercion of input arguments #343

Conversation

bajtos commented Aug 31, 2016 • edited Loading

bajtos commented Aug 31, 2016

bajtos commented Aug 31, 2016

bajtos commented Aug 31, 2016

richardpringle commented Aug 31, 2016

Choose a reason for hiding this comment

richardpringle commented Sep 1, 2016

Choose a reason for hiding this comment

bajtos Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

ritch commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 2, 2016

Amir-61 Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amir-61 Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amir-61 commented Sep 7, 2016 • edited Loading

Amir-61 Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amir-61 commented Sep 7, 2016 • edited Loading

bajtos commented Aug 31, 2016 •

edited

Loading

bajtos Sep 2, 2016 •

edited

Loading

Amir-61 Sep 7, 2016 •

edited

Loading

Amir-61 Sep 7, 2016 •

edited

Loading

Amir-61 commented Sep 7, 2016 •

edited

Loading

Amir-61 Sep 7, 2016 •

edited

Loading

Amir-61 commented Sep 7, 2016 •

edited

Loading