Skip to content

Commit

Permalink
Rework coercion of input arguments
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bajtos committed Aug 31, 2016
1 parent 4269cda commit 93109df
Show file tree
Hide file tree
Showing 44 changed files with 1,304 additions and 1,089 deletions.
16 changes: 15 additions & 1 deletion lib/context-base.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,28 @@

'use strict';

var assert = require('assert');
var EventEmitter = require('events').EventEmitter;
var TypeRegistry = require('./type-registry');
var inherits = require('util').inherits;

module.exports = ContextBase;

/**
* A base class for all Context instances
*/
function ContextBase(method) {
function ContextBase(method, typeRegistry) {
// NOTE(bajtos) we are not asserting via "instanceof" to allow
// multiple copies of strong-remoting to cooperate together
assert(method && typeof method === 'object',
'method must be a SharedClass instance');
assert(typeRegistry && typeof typeRegistry === 'object',
'typeRegistry must be a TypeRegistry instance');

EventEmitter.call(this);

this.method = method;
this.typeRegistry = typeRegistry;
}

inherits(ContextBase, EventEmitter);
Expand All @@ -29,3 +39,7 @@ ContextBase.prototype.getScope = function() {
method.ctor ||
method.sharedMethod && method.sharedMethod.ctor;
};

ContextBase.prototype.setReturnArgByName = function(name, value) {
// no-op
};
127 changes: 0 additions & 127 deletions lib/dynamic.js

This file was deleted.

Loading

0 comments on commit 93109df

Please sign in to comment.