-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coercion refactor #264
Coercion refactor #264
Conversation
This fixes a number of subtle bugs and restricts "sloppy" argument coercion (e.g. 'true' to the bool true) to string-only HTTP datasources like querystrings and headers. Fixes strongloop#223 (coerced Number past MAX_SAFE_INTEGER) Possible fix for strongloop#208
This puts the bulk of the coercion into the http handlers, assuming that otherwise coercion is not desired in favor of strictness. Coercion now runs on qs/header/formdata regardless of 'any' type, and barely runs at all on JSON or direct invocation.
3448f71
to
be79cfe
Compare
Should we special-case arrays as something we coerce regardless? In remoting methods, it is awfully nice to be sure an array type is passed, regardless of whether or not the input was null - an empty array is simpler to deal with. This is the current behavior. |
@STRML thank you for the patch, could you please rebase it on top of the current master, which includes your older patch #231? Also PTAL at strongloop/loopback#1806, how will the changes in this pull request affect that issue? Are you aware of any edge cases where this patch may break existing applications? |
I find this a bit surprising. In my opinion, the coercion problem is transport-specific and thus it should be handled by transport-specific code (e.g. However, I am not very familiar with this part of strong-remoting codebase. If the solution proposed here is the best what we can achieve now, then I can live with that. @ritch @fabien @raymondfeng could you PTAL and review this patch? |
FYI integration test in project I work on started to fail with latest release because of incorrect parsing of boolean request params (where |
Yes, this is a bug fixed in this PR. I'll be back in the office soon and
|
@bajtos It seems odd to me, to make http coercion user-configurable. It should simply follow simple rules, and if the user wants to coerce further, he can do so for all possible input via This lets the user configure nice things, like auto-coercing strings into arrays of strings if the method only accepts arrays, and so on. As-is, I find it confusing how many coercion paths there are:
My first thought was, we could simply put all coercion logic in Dynamic, but this would make it impossible to e.g. properly coerce Dates, which can't be communicated over JSON. So instead, I've settled on:
Does this make sense? |
var message = util.format('Invalid value for argument \'%s\' of type ' + | ||
'\'%s\'. Received type was %s. Error: %s', | ||
name, targetType, typeof uarg, e.message); | ||
throw new badArgumentError(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am reluctant to remove request values from error messages. By including the request value, we make troubleshooting easier, e.g. when one doesn't have access to request bodies, only the error messages.
We already had a user asking to include values in validation errors, see loopbackio/loopback-datasource-juggler#389
method.invoke('ctx', { obj: '<script>alert(1)</script>' }, function(err) {
The use case where the request value contains <script>
element should be IMO handled by the client processing the responses, it's the client responsibility to html-encode any error messages before rendering them in the UI.
I am also not convinced that the argument value is the only place that can be abused for code injection, i.e. this change may be giving a false sense of security. However, if you insist, then I am willing to consider adding a feature flag to exclude request values from error messages, the flag should be disabled by default.
@raymondfeng @ritch thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is classic reflection, and production APIs simply shouldn't be echoing request data back. Yes, there are ways to mitigate it, and yes, you shouldn't be displaying it in the UI, but there are always edge cases (like MIME sniffing) that can make this dangerous. With this reflection hole open, in certain cases, XSS would be possible via a specially-crashed API link.
I am going to revert the patch that introduced the problem, to buy us more time to deal with these issues properly. See #269
I am afraid I'll need a bit more time to digest what you wrote (and the code changes proposed here) to answer. |
@bajtos ok, thanks! |
Update: I am working on bringing the changes proposed here to the current 2.x series, you can track my progress in https://github.com/strongloop/strong-remoting/tree/feature/coercion-refactor |
Closing in favour of #343 |
This fixes some of the issues in #208 and builds on top of #231 which was rebased out without warning.
There are two types of coercion Strong-Remoting should be doing, and they should not share code / be intermixed.
http-coerce
will coerce to the expected type as closely as possible, while special-casing'undefined'
and''
toundefined
.undefined
instead ofnull
so it plays nicely with ES6 default parameters.'?toggle'
(the empty string) coerce totrue
? If so, we should special-case this querystring behavior.For context, this grew out of some very frustrating behavior with
master
. Most of that behavior is documented in #231. This additionally fixes some bad behavior in #231, like the'boolean'
type coercing the string "false" totrue
.