nickva commented on code in PR #5858:
URL: https://github.com/apache/couchdb/pull/5858#discussion_r3392273919
##########
src/mango/src/mango_selector.erl:
##########
@@ -427,15 +494,31 @@ match({[{<<"$elemMatch">>, Arg}]}, Values, Cmp) when
is_list(Values) ->
_:_ ->
false
end;
-match({[{<<"$elemMatch">>, _Arg}]}, _Value, _Cmp) ->
+match({[{<<"$elemMatch">>, _Arg}]}, _Value, #ctx{verbose = false}) ->
false;
+match({[{<<"$elemMatch">>, _Arg}]}, [], #ctx{negate = false} = Ctx) ->
+ [#failure{op = elemMatch, type = empty_list, ctx = Ctx}];
+match({[{<<"$elemMatch">>, _Arg}]}, [], #ctx{negate = true}) ->
+ [];
+match({[{<<"$elemMatch">>, Arg}]}, Values, #ctx{negate = true} = Ctx) ->
Review Comment:
I think we lost a `is_list(Values)` guard? As is we should try to have a
test where $elemMatch with/without negate and with verbose=true would blow up
on lists:enumerate/2
```
5> lists:enumerate(0, 42).
* exception error: no function clause matching lists:enumerate_1(0,1,42)
(lists.erl:1856)
```
##########
src/couch_mrview/src/couch_mrview.erl:
##########
@@ -248,12 +248,11 @@ validate(Db, DDoc) ->
ok
end,
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
- false ->
- ok;
+ try couch_query_servers:get_os_process(Lang) of
Review Comment:
One change here to watch out for is that previously if ddoc didn't have view
we didn't check out an OS process. Now we always do. Could we first check if we
have views or a VDU and only then go into `get_os_process()`? Not a huge deal
but OS processes are a limited resource and it would nice not have get them
from the proc manager gen_server on every ddoc update if doesn't have anything
to validate
##########
src/couch_mrview/src/couch_mrview.erl:
##########
@@ -263,6 +262,21 @@ validate(Db, DDoc) ->
ok
end.
+validate_vdu(Proc, #doc{body = {Props}}) ->
+ case config:get_boolean("couchdb", "validate_vdu", false) of
Review Comment:
This should be `true` as per default.ini value?
##########
src/mango/src/mango_selector.erl:
##########
@@ -548,22 +679,136 @@ match({[{<<"$", _/binary>> = Op, _}]}, _, _) ->
% We need to traverse value to find field. The call to
% mango_doc:get_field/2 may return either not_found or
% bad_path in which case matching fails.
-match({[{Field, Cond}]}, Value, Cmp) ->
+match({[{Field, Cond}]}, Value, #ctx{verbose = Verb, path = Path} = Ctx) ->
+ InnerPath = extend_path(Field, Path),
+ InnerCtx = Ctx#ctx{path = InnerPath},
case mango_doc:get_field(Value, Field) of
not_found when Cond == {[{<<"$exists">>, false}]} ->
- true;
+ case Verb of
+ true -> [];
+ false -> true
+ end;
not_found ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = not_found, ctx =
InnerCtx}];
+ false -> false
+ end;
bad_path ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = bad_path, ctx =
InnerCtx}];
+ false -> false
+ end;
SubValue when Field == <<"_id">> ->
- match(Cond, SubValue, fun mango_json:cmp_raw/2);
+ match(Cond, SubValue, InnerCtx#ctx{cmp = fun
mango_json:cmp_raw/2});
SubValue ->
- match(Cond, SubValue, Cmp)
+ match(Cond, SubValue, InnerCtx)
end;
-match({[_, _ | _] = _Props} = Sel, _Value, _Cmp) ->
+match({[_, _ | _] = _Props} = Sel, _Value, _Ctx) ->
error({unnormalized_selector, Sel}).
+extend_path(Field, Path) when is_binary(Field) ->
+ [Field | Path];
+extend_path(Field, Path) when is_list(Field) ->
+ lists:foldl(fun(F, Acc) -> [F | Acc] end, Path, Field).
+
+match_with_failure(Expr, Value, Op, Params, #ctx{negate = Neg} = Ctx) ->
+ case not match(Expr, Value, Ctx#ctx{verbose = false}) of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = Params, ctx = Ctx}]
+ end.
+
+compare(_, _, #ctx{verbose = false}, Cond) ->
+ Cond;
+compare(Op, Arg, #ctx{negate = Neg} = Ctx, Cond) ->
+ case not Cond of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = [Arg], ctx = Ctx}]
+ end.
+
+format_failure(#failure{op = Op, type = Type, params = Params, ctx = Ctx}) ->
+ Path = format_path(Ctx#ctx.path),
+ Msg = format_op(Op, Ctx#ctx.negate, Type, Params),
+ {[{<<"path">>, Path}, {<<"message">>, iolist_to_binary(Msg)}]}.
+
+format_op(Op, _, empty_list, _) ->
+ io_lib:format("operator $~p was invoked with an empty list", [Op]);
+format_op(Op, _, bad_value, [Value]) ->
+ io_lib:format("operator $~p was invoked with a bad value: ~s", [Op,
jiffy:encode(Value)]);
Review Comment:
In couch_util we use `json_encode(V) -> jiffy:encode(V, [force_utf8]).` I
am not sure if we'd want that here as well. Would we have broken surrogate
pairs or broken bytes and such? Probably not
##########
test/elixir/test/validate_doc_update_test.exs:
##########
@@ -77,6 +77,36 @@ defmodule ValidateDocUpdateTest do
assert resp.status_code == 403
end
+ @tag :with_db
+ test "invalid JavaScript VDU is detected on doc update", context do
+ Couch.put("/_node/_local/_config/couchdb/validate_vdu", body: "\"false\"")
Review Comment:
Do elixir tests automatically reset this back to true when done? (they may,
but if not we should cleanup after we're done).
##########
src/mango/src/mango_native_proc.erl:
##########
@@ -95,6 +100,12 @@ handle_call({prompt, [<<"nouveau_index_doc">>, Doc]},
_From, St) ->
Else
end,
{reply, Vals, St};
+handle_call({prompt, [<<"validate_fun">>, Selector0 | _Rest]}, _From, St) ->
+ Selector = mango_selector:normalize(Selector0),
Review Comment:
Should normalize call be protected as well? Otherwise this will crash this
gen_server. Normalize could take any random junk a user could put in a json
doc: `{"newDoc": {"$randomOp":42}}` or JS function but with a language = query.
##########
src/couch/src/couch_query_servers.erl:
##########
@@ -42,7 +42,12 @@
try_compile(Proc, FunctionType, FunctionName, FunctionSource) ->
try
- proc_prompt(Proc, [<<"add_fun">>, FunctionSource]),
+ case FunctionType of
+ validate_doc_update ->
+ proc_prompt(Proc, [<<"validate_fun">>, FunctionSource]);
Review Comment:
What will happen if query language is Erlang? Do we get an immediate 500 or
some nicer invalid language 400 response?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
