nickva commented on code in PR #5858:
URL: https://github.com/apache/couchdb/pull/5858#discussion_r3605668679
##########
src/mango/src/mango_native_proc.erl:
##########
@@ -95,6 +100,17 @@ handle_call({prompt, [<<"nouveau_index_doc">>, Doc]},
_From, St) ->
Else
end,
{reply, Vals, St};
+handle_call({prompt, [<<"validate_fun">>, Selector0 | _Rest]}, _From, St) ->
+ try mango_selector:normalize(Selector0) of
+ Selector ->
+ case validate_vdu(Selector) of
+ ok -> {reply, true, St};
+ Error -> {reply, {error, Error}, St}
+ end
+ catch
+ throw:{mango_error, mango_selector, Error} ->
Review Comment:
Could normalize throw other error besides `{mango_error, mango_selector,
_}`? For example
https://github.com/apache/couchdb/blob/2eed4886d1853b97459ffd4f0c827e0cdd816a91/src/mango/src/mango_selector.erl#L253
(norm_fields) will throw `norm_fields`. Basically don't hardcode the module
name, maybe something like `throw:{mango_error, _Mod, Error}` could work
##########
rel/overlay/etc/default.ini:
##########
@@ -132,6 +132,13 @@ view_index_dir = {{view_index_dir}}
; Javascript engine. The choices are: spidermonkey and quickjs
;js_engine = spidermonkey
+; When set to "true", the `validate_doc_update` field will be validated when
+; design documents are updated. For `javascript` design docs, the field must
+; contain a well-formed JavaScript function, and for `query` design docs it
+; must contain a Mango selector that is correctly structured to validate
+; document updates.
+validate_vdu = true
Review Comment:
We don't typically have live values in default.ini they should commented
out. Since Mango VDUs are new (not released yet), we can probably always
validate them. There is no reason to let uses shoot themselves in the foot. For
JS, we'd keep it false since it's a compatibility issue. Maybe set up to be
flipped in 4.x versions.
##########
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:
> Okay, I'll move these checks around so we avoid grabbing an OS process if
it's not needed.
I think we'd instead not want to call get_os_process() at all if we don't
have views and won't be checking vdus (because we may have them but they are js
vdus and we don't want to validate them because of the config)
##########
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:
That makes sense as yeah. Wonder if we have any tests for it. Say if add a
test for a vdu written in Lua. We shouldn't crash or try to validate it.
##########
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:
One thing to check is that we may throw other errors besides
`{compilation_error, _}` and `{os_process_error, _}`. In other words we'd
ensure that we things like `invalid_operator` or something still either
surfaces as a `{compilation_error, _}` or we handle the extra cases in the
catch part.
##########
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:
I just meant that we can reuse the json_encode macro or the util function
since it forces utf8 encoding and here we don't. It's mostly for consistency
and belt and suspenders.
##########
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:
Look at may be using `set_config(...)` from `CouchTestCase`. For example I
found this in one of the tests:
```
@tag :with_partitioned_db
test "partitioned query with query server config set", context do
db_name = context[:db_name]
create_partition_docs(db_name)
create_index(db_name, ["value"])
# this is to test that we bypass partition_query_limit for mango
set_config({"query_server_config", "partition_query_limit", "1"})
```
And then looking in `set_config`
```elixir
def set_config({section, key, value}) do
existing = set_config_raw(section, key, value)
on_exit(fn ->
Enum.each(existing, fn {node, prev_value} ->
if prev_value != "" do
url = "/_node/#{node}/_config/#{section}/#{key}"
headers = ["X-Couch-Persist": "false"]
body = :jiffy.encode(prev_value, [:use_nil])
resp = Couch.put(url, headers: headers, body: body)
assert resp.status_code == 200
else
url = "/_node/#{node}/_config/#{section}/#{key}"
headers = ["X-Couch-Persist": "false"]
resp = Couch.delete(url, headers: headers)
assert resp.status_code == 200
end
end)
end)
end
```
##########
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:
I meant that we'd first see if we'd even need an os process and only then
try to get it. I added a note about in a new review set of comments.
--
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]
